aboutsummaryrefslogtreecommitdiff
path: root/src
Commit message (Collapse)AuthorAge
* Correct type of front_pathkey to PathKeyTomas Vondra2022-01-23
| | | | | | | | | | | | In sort_inner_and_outer we iterate a list of PathKey elements, but the variable is declared as (List *). This mistake is benign, because we only pass the pointer to lcons() and never dereference it. This exists since ~2004, but it's confusing. So fix and backpatch to all supported branches. Backpatch-through: 10 Discussion: https://postgr.es/m/bf3a6ea1-a7d8-7211-0669-189d5c169374%40enterprisedb.com
* Check syscache result in AlterStatisticsTomas Vondra2022-01-23
| | | | | | | | | | | | The syscache lookup may return NULL even for valid OID, for example due to a concurrent DROP STATISTICS, so a HeapTupleIsValid is necessary. Without it, it may fail with a segfault. Reported by Alexander Lakhin, patch by me. Backpatch to 13, where ALTER STATISTICS ... SET STATISTICS was introduced. Backpatch-through: 13 Discussion: https://postgr.es/m/17372-bf3b6e947e35ae77%40postgresql.org
* Flush table's relcache during ALTER TABLE ADD PRIMARY KEY USING INDEX.Tom Lane2022-01-22
| | | | | | | | | | | | | | | | | Previously, unless we had to add a NOT NULL constraint to the column, this command resulted in updating only the index's relcache entry. That's problematic when replication behavior is being driven off the existence of a primary key: other sessions (and ours too for that matter) failed to recalculate their opinion of whether the table can be replicated. Add a relcache invalidation to fix it. This has been broken since pg_class.relhaspkey was removed in v11. Before that, updating the table's relhaspkey value sufficed to cause a cache flush. Hence, backpatch to v11. Report and patch by Hou Zhijie Discussion: https://postgr.es/m/OS0PR01MB5716EBE01F112C62F8F9B786947B9@OS0PR01MB5716.jpnprd01.prod.outlook.com
* Fix race condition in gettext() initialization in libpq and ecpglib.Tom Lane2022-01-21
| | | | | | | | | | | | | | | | | | | | In libpq and ecpglib, multiple threads can concurrently enter the initialization logic for message localization. Since we set the its-done flag before actually doing the work, it'd be possible for some threads to reach gettext() before anyone has called bindtextdomain(). Barring bugs in libintl itself, this would not result in anything worse than failure to localize some early messages. Nonetheless, it's a bug, and an easy one to fix. Noted while investigating bug #17299 from Clemens Zeidler (much thanks to Liam Bowen for followup investigation on that). It currently appears that that actually *is* a bug in libintl itself, but that doesn't let us off the hook for this bit. Back-patch to all supported versions. Discussion: https://postgr.es/m/17299-7270741958c0b1ab@postgresql.org Discussion: https://postgr.es/m/CAE7q7Eit4Eq2=bxce=Fm8HAStECjaXUE=WBQc-sDDcgJQ7s7eg@mail.gmail.com
* fsync pg_logical/mappings in CheckPointLogicalRewriteHeap().Andres Freund2022-01-21
| | | | | | | | | | | While individual logical rewrite files were synced to disk, the directory was not. On some filesystems that could lead to loosing directory entries after a crash. Reported-By: Tom Lane <tgl@sss.pgh.pa.us> Author: Nathan Bossart <bossartn@amazon.com> Discussion: https://postgr.es/m/867F2E29-2782-4869-970E-B984C6D35A8F@amazon.com Backpatch: 10-
* Fix one-off bug causing missing commit timestamps for subtransactionsMichael Paquier2022-01-21
| | | | | | | | | | | | | | | | | | | The logic in charge of writing commit timestamps (enabled with track_commit_timestamp) for subtransactions had a one-bug bug, where it would be possible that commit timestamps go missing for the last subtransaction committed. While on it, simplify a bit the iteration logic in the loop writing the commit timestamps, as per suggestions from Kyotaro Horiguchi and Tom Lane, so as some variable initializations are not part of the loop itself. Issue introduced in 73c986a. Analyzed-by: Alex Kingsborough Author: Alex Kingsborough, Kyotaro Horiguchi Discussion: https://postgr.es/m/73A66172-4050-4F2A-B7F1-13508EDA2144@amazon.com Backpatch-through: 10
* Tighten TAP tests' tracking of postmaster state some more.Tom Lane2022-01-20
| | | | | | | | | | | | | | | | | | | Commits 6c4a8903b et al. had a couple of deficiencies: * The logic I added to Cluster::start to see if a PID file is present could be fooled by a stale PID file left over from a previous postmaster. To fix, if we're not sure whether we expect to find a running postmaster or not, validate the PID using "kill 0". * 017_shm.pl has a loop in which it just issues repeated Cluster::start calls; this will fail if some invocation fails but leaves self->_pid set. Per buildfarm results, the above fix is not enough to make this safe: we might have "validated" a PID for a postmaster that exits immediately after we look. Hence, match each failed start call with a stop call that will get us back to the self->_pid == undef state. Add a fail_ok option to Cluster::stop to make this work. Discussion: https://postgr.es/m/CA+hUKGKV6fOHvfiPt8=dOKzvswjAyLoFoJF1iQXMNpi7+hD1JQ@mail.gmail.com
* Allow clean.bat to be run from anywhereAndrew Dunstan2022-01-20
| | | | | | | | | | | This was omitted from c3879a7b4c which modified the other msvc .bat files. Per request from Juan José Santamaría Flecha Discussion: https://postgr.es/m/CAC+AXB0_fxYGbQoaYjCA8um7TTbOVP4L9aXnVmHwK8WzaT4gdA@mail.gmail.com Backpatch to all live branches.
* Try to stabilize reloptions test, again.Thomas Munro2022-01-20
| | | | | | | | | | Since the test requires reproducible behavior from VACUUM, and since DISABLE_PAGE_SKIPPING doesn't actually disable all forms of page skipping, let's use a temporary table to avoid contention. Back-patch to 12, like commit 3414099c. Discussion: https://postgr.es/m/20220120052404.sonrhq3f3qgplpzj%40alap3.anarazel.de
* TAP tests: check for postmaster.pid anyway when "pg_ctl start" fails.Tom Lane2022-01-19
| | | | | | | | | | | | "pg_ctl start" might start a new postmaster and then return failure anyway, for example if PGCTLTIMEOUT is exceeded. If there is a postmaster there, it's still incumbent on us to shut it down at script end, so check for the PID file even though we are about to fail. This has been broken all along, so back-patch to all supported branches. Discussion: https://postgr.es/m/647439.1642622744@sss.pgh.pa.us
* Try to stabilize the reloptions test.Thomas Munro2022-01-19
| | | | | | | | | | | Where we test vacuum_truncate's effects, sometimes this is failing to truncate as expected on the build farm. That could be explained by page skipping, so disable it explicitly, with the theory that commit fe246d1c didn't go far enough. Back-patch to 12, where the vacuum_truncate tests were added. Discussion: https://postgr.es/m/CA%2BhUKGLT2UL5_JhmBzUgkdyKfc%3D5J-gJSQJLysMs4rqLUKLAzw%40mail.gmail.com
* Fix psql \d's query for identifying parent triggers.Tom Lane2022-01-17
| | | | | | | | | | | | | | | | | | | | | The original coding (from c33869cc3) failed with "more than one row returned by a subquery used as an expression" if there were unrelated triggers of the same tgname on parent partitioned tables. (That's possible because statement-level triggers don't get inherited.) Fix by applying LIMIT 1 after sorting the candidates by inheritance level. Also, wrap the subquery in a CASE so that we don't have to execute it at all when the trigger is visibly non-inherited. Aside from saving some cycles, this avoids the need for a confusing and undocumented NULLIF(). While here, tweak the format of the emitted query to look a bit nicer for "psql -E", and add some explanation of this subquery, because it badly needs it. Report and patch by Justin Pryzby (with some editing by me). Back-patch to v13 where the faulty code came in. Discussion: https://postgr.es/m/20211217154356.GJ17618@telsasoft.com
* Avoid calling gettext() in signal handlers.Tom Lane2022-01-17
| | | | | | | | | | | | | | | | | | | | | | | | | It seems highly unlikely that gettext() can be relied on to be async-signal-safe. psql used to understand that, but someone got it wrong long ago in the src/bin/scripts/ version of handle_sigint, and then the bad idea was perpetuated when those two versions were unified into src/fe_utils/cancel.c. I'm unsure why there have not been field complaints about this ... maybe gettext() is signal-safe once it's translated at least one message? But we have no business assuming any such thing. In cancel.c (v13 and up), I preserved our ability to localize "Cancel request sent" messages by invoking gettext() before the signal handler is set up. In earlier branches I just made src/bin/scripts/ not localize those messages, as psql did then. (Just for extra unsafety, the src/bin/scripts/ version was invoking fprintf() from a signal handler. Sigh.) Noted while fixing signal-safety issues in PQcancel() itself. Back-patch to all supported branches. Discussion: https://postgr.es/m/2937814.1641960929@sss.pgh.pa.us
* Avoid calling strerror[_r] in PQcancel().Tom Lane2022-01-17
| | | | | | | | | | | | | | | | | | | | | | | | PQcancel() is supposed to be safe to call from a signal handler, and indeed psql uses it that way. All of the library functions it uses are specified to be async-signal-safe by POSIX ... except for strerror. Neither plain strerror nor strerror_r are considered safe. When this code was written, back in the dark ages, we probably figured "oh, strerror will just index into a constant array of strings" ... but in any locale except C, that's unlikely to be true. Probably the reason we've not heard complaints is that (a) this error-handling code is unlikely to be reached in normal use, and (b) in many scenarios, localized error strings would already have been loaded, after which maybe it's safe to call strerror here. Still, this is clearly unacceptable. The best we can do without relying on strerror is to print the decimal value of errno, so make it do that instead. (This is probably not much loss of user-friendliness, given that it is hard to get a failure here.) Back-patch to all supported branches. Discussion: https://postgr.es/m/2937814.1641960929@sss.pgh.pa.us
* Teach hash_ok_operator() that record_eq is only sometimes hashable.Tom Lane2022-01-16
| | | | | | | | | | | The need for this was foreseen long ago, but when record_eq actually became hashable (in commit 01e658fa7), we missed updating this spot. Per bug #17363 from Elvis Pranskevichus. Back-patch to v14 where the faulty commit came in. Discussion: https://postgr.es/m/17363-f6d42fd0d726be02@postgresql.org
* Fix psql's tab-completion of enum label values.Tom Lane2022-01-16
| | | | | | | | | | | | | | | | | | | Since enum labels have to be single-quoted, this part of the tab completion machinery got side-swiped by commit cd69ec66c. A side-effect of that commit is that (at least with some versions of Readline) the text string passed for completion will omit the leading quote mark of the enum label literal. Libedit still acts the same as before, though, so adapt COMPLETE_WITH_ENUM_VALUE so that it can cope with either convention. Also, when we fail to find any valid completion, set rl_completion_suppress_quote = 1. Otherwise readline will go ahead and append a closing quote, which is unwanted. Per report from Peter Eisentraut. Back-patch to v13 where cd69ec66c came in. Discussion: https://postgr.es/m/8ca82d89-ec3d-8b28-8291-500efaf23b25@enterprisedb.com
* Build inherited extended stats on partitioned tablesTomas Vondra2022-01-15
| | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | Commit 859b3003de disabled building of extended stats for inheritance trees, to prevent updating the same catalog row twice. While that resolved the issue, it also means there are no extended stats for declaratively partitioned tables, because there are no data in the non-leaf relations. That also means declaratively partitioned tables were not affected by the issue 859b3003de addressed, which means this is a regression affecting queries that calculate estimates for the whole inheritance tree as a whole (which includes e.g. GROUP BY queries). But because partitioned tables are empty, we can invert the condition and build statistics only for the case with inheritance, without losing anything. And we can consider them when calculating estimates. It may be necessary to run ANALYZE on partitioned tables, to collect proper statistics. For declarative partitioning there should no prior statistics, and it might take time before autoanalyze is triggered. For tables partitioned by inheritance the statistics may include data from child relations (if built 859b3003de), contradicting the current code. Report and patch by Justin Pryzby, minor fixes and cleanup by me. Backpatch all the way back to PostgreSQL 10, where extended statistics were introduced (same as 859b3003de). Author: Justin Pryzby Reported-by: Justin Pryzby Backpatch-through: 10 Discussion: https://postgr.es/m/20210923212624.GI831%40telsasoft.com
* Ignore extended statistics for inheritance treesTomas Vondra2022-01-15
| | | | | | | | | | | | | | | | | | | | | | | | | | | Since commit 859b3003de we only build extended statistics for individual relations, ignoring the child relations. This resolved the issue with updating catalog tuple twice, but we still tried to use the statistics when calculating estimates for the whole inheritance tree. When the relations contain very distinct data, it may produce bogus estimates. This is roughly the same issue 427c6b5b9 addressed ~15 years ago, and we fix it the same way - by ignoring extended statistics when calculating estimates for the inheritance tree as a whole. We still consider extended statistics when calculating estimates for individual child relations, of course. This may result in plan changes due to different estimates, but if the old statistics were not describing the inheritance tree particularly well it's quite likely the new plans is actually better. Report and patch by Justin Pryzby, minor fixes and cleanup by me. Backpatch all the way back to PostgreSQL 10, where extended statistics were introduced (same as 859b3003de). Author: Justin Pryzby Reported-by: Justin Pryzby Backpatch-through: 10 Discussion: https://postgr.es/m/20210923212624.GI831%40telsasoft.com
* Fix possible HOT corruption when RECENTLY_DEAD changes to DEAD while pruning.Andres Freund2022-01-14
| | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | Since dc7420c2c92 the horizon used for pruning is determined "lazily". A more accurate horizon is built on-demand, rather than in GetSnapshotData(). If a horizon computation is triggered between two HeapTupleSatisfiesVacuum() calls for the same tuple, the result can change from RECENTLY_DEAD to DEAD. heap_page_prune() can process the same tid multiple times (once following an update chain, once "directly"). When the result of HeapTupleSatisfiesVacuum() of a tuple changes from RECENTLY_DEAD during the first access, to DEAD in the second, the "tuple is DEAD and doesn't chain to anything else" path in heap_prune_chain() can end up marking the target of a LP_REDIRECT ItemId unused. Initially not easily visible, Once the target of a LP_REDIRECT ItemId is marked unused, a new tuple version can reuse it. At that point the corruption may become visible, as index entries pointing to the "original" redirect item, now point to a unrelated tuple. To fix, compute HTSV for all tuples on a page only once. This fixes the entire class of problems of HTSV changing inside heap_page_prune(). However, visibility changes can obviously still occur between HTSV checks inside heap_page_prune() and outside (e.g. in lazy_scan_prune()). The computation of HTSV is now done in bulk, in heap_page_prune(), rather than on-demand in heap_prune_chain(). Besides being a bit simpler, it also is faster: Memory accesses can happen sequentially, rather than in the order of HOT chains. There are other causes of HeapTupleSatisfiesVacuum() results changing between two visibility checks for the same tuple, even before dc7420c2c92. E.g. HEAPTUPLE_INSERT_IN_PROGRESS can change to HEAPTUPLE_DEAD when a transaction aborts between the two checks. None of the these other visibility status changes are known to cause corruption, but heap_page_prune()'s approach makes it hard to be confident. A patch implementing a more fundamental redesign of heap_page_prune(), which fixes this bug and simplifies pruning substantially, has been proposed by Peter Geoghegan in https://postgr.es/m/CAH2-WzmNk6V6tqzuuabxoxM8HJRaWU6h12toaS-bqYcLiht16A@mail.gmail.com However, that redesign is larger change than desirable for backpatching. As the new design still benefits from the batched visibility determination introduced in this commit, it makes sense to commit this narrower fix to 14 and master, and then commit Peter's improvement in master. The precise sequence required to trigger the bug is complicated and hard to do exercise in an isolation test (until we have wait points). Due to that the isolation test initially posted at https://postgr.es/m/20211119003623.d3jusiytzjqwb62p%40alap3.anarazel.de and updated in https://postgr.es/m/20211122175914.ayk6gg6nvdwuhrzb%40alap3.anarazel.de isn't committable. A followup commit will introduce additional assertions, to detect problems like this more easily. Bug: #17255 Reported-By: Alexander Lakhin <exclusion@gmail.com> Debugged-By: Andres Freund <andres@anarazel.de> Debugged-By: Peter Geoghegan <pg@bowt.ie> Author: Andres Freund <andres@andres@anarazel.de> Reviewed-By: Peter Geoghegan <pg@bowt.ie> Discussion: https://postgr.es/m/20211122175914.ayk6gg6nvdwuhrzb@alap3.anarazel.de Backpatch: 14-, the oldest branch containing dc7420c2c92
* Revert error handling improvements for cryptohashesMichael Paquier2022-01-14
| | | | | | | | | | | | This reverts commits ab27df2, af8d530 and 3a0cced, that introduced pg_cryptohash_error(). In order to make the core code able to pass down the new error types that this introduced, some of the MD5-related routines had to be reworked, causing an ABI breakage, but we found that some external extensions rely on them. Maintaining compatibility outweights the error report benefits, so just revert the change in v14. Reported-by: Laurenz Albe Discussion: https://postgr.es/m/9f0c0a96d28cf14fc87296bbe67061c14eb53ae8.camel@cybertec.at
* Fix ruleutils.c's dumping of whole-row Vars in more contexts.Tom Lane2022-01-13
| | | | | | | | | | | | | | | | | | Commit 7745bc352 intended to ensure that whole-row Vars would be printed with "::type" decoration in all contexts where plain "var.*" notation would result in star-expansion, notably in ROW() and VALUES() constructs. However, it missed the case of INSERT with a single-row VALUES, as reported by Timur Khanjanov. Nosing around ruleutils.c, I found a second oversight: the code for RowCompareExpr generates ROW() notation without benefit of an actual RowExpr, and naturally it wasn't in sync :-(. (The code for FieldStore also does this, but we don't expect that to generate strictly parsable SQL anyway, so I left it alone.) Back-patch to all supported branches. Discussion: https://postgr.es/m/efaba6f9-4190-56be-8ff2-7a1674f9194f@intrans.baku.az
* Fix incorrect comments in hmac.c and hmac_openssl.cMichael Paquier2022-01-13
| | | | | | | | | Both files referred to pg_hmac_ctx->data, which, I guess, comes from the early versions of the patch that has resulted in commit e6bdfd9. Author: Sergey Shinderuk Discussion: https://postgr.es/m/8cbb56dd-63d6-a581-7a65-25a97ac4be03@postgrespro.ru Backpatch-through: 14
* Fix memory leak in indexUnchanged hint mechanism.Peter Geoghegan2022-01-12
| | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | Commit 9dc718bd added a "logically unchanged by UPDATE" hinting mechanism, which is currently used within nbtree indexes only (see commit d168b666). This mechanism determined whether or not the incoming item is a logically unchanged duplicate (a duplicate needed only for MVCC versioning purposes) once per row updated per non-HOT update. This approach led to memory leaks which were noticeable with an UPDATE statement that updated sufficiently many rows, at least on tables that happen to have an expression index. On HEAD, fix the issue by adding a cache to the executor's per-index IndexInfo struct. Take a different approach on Postgres 14 to avoid an ABI break: simply pass down the hint to all indexes unconditionally with non-HOT UPDATEs. This is deemed acceptable because the hint is currently interpreted within btinsert() as "perform a bottom-up index deletion pass if and when the only alternative is splitting the leaf page -- prefer to delete any LP_DEAD-set items first". nbtree must always treat the hint as a noisy signal about what might work, as a strategy of last resort, with costs imposed on non-HOT updaters. (The same thing might not be true within another index AM that applies the hint, which is why the original behavior is preserved on HEAD.) Author: Peter Geoghegan <pg@bowt.ie> Reported-By: Klaudie Willis <Klaudie.Willis@protonmail.com> Diagnosed-By: Tom Lane <tgl@sss.pgh.pa.us> Discussion: https://postgr.es/m/261065.1639497535@sss.pgh.pa.us Backpatch: 14-, where the hinting mechanism was added.
* Fix comment related to pg_cryptohash_error()Michael Paquier2022-01-12
| | | | | | | | | One of the comments introduced in b69aba7 was worded a bit weirdly, so improve it. Reported-by: Sergey Shinderuk Discussion: https://postgr.es/m/71b9a5d2-a3bf-83bc-a243-93dcf0bcfb3b@postgrespro.ru Backpatch-through: 14
* Clean up error message reported after \password encryption failure.Tom Lane2022-01-11
| | | | | | | | | | | | | | | Experimenting with FIPS mode enabled, I saw regression=# \password joe Enter new password for user "joe": Enter it again: could not encrypt password: disabled for FIPS out of memory because PQencryptPasswordConn was still of the opinion that "out of memory" is always appropriate to print. Minor oversight in b69aba745. Like that one, back-patch to v14.
* Improve error handling of cryptohash computationsMichael Paquier2022-01-11
| | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | The existing cryptohash facility was causing problems in some code paths related to MD5 (frontend and backend) that relied on the fact that the only type of error that could happen would be an OOM, as the MD5 implementation used in PostgreSQL ~13 (the in-core implementation is used when compiling with or without OpenSSL in those older versions), could fail only under this circumstance. The new cryptohash facilities can fail for reasons other than OOMs, like attempting MD5 when FIPS is enabled (upstream OpenSSL allows that up to 1.0.2, Fedora and Photon patch OpenSSL 1.1.1 to allow that), so this would cause incorrect reports to show up. This commit extends the cryptohash APIs so as callers of those routines can fetch more context when an error happens, by using a new routine called pg_cryptohash_error(). The error states are stored within each implementation's internal context data, so as it is possible to extend the logic depending on what's suited for an implementation. The default implementation requires few error states, but OpenSSL could report various issues depending on its internal state so more is needed in cryptohash_openssl.c, and the code is shaped so as we are always able to grab the necessary information. The core code is changed to adapt to the new error routine, painting more "const" across the call stack where the static errors are stored, particularly in authentication code paths on variables that provide log details. This way, any future changes would warn if attempting to free these strings. The MD5 authentication code was also a bit blurry about the handling of "logdetail" (LOG sent to the postmaster), so improve the comments related that, while on it. The origin of the problem is 87ae969, that introduced the centralized cryptohash facility. Extra changes are done for pgcrypto in v14 for the non-OpenSSL code path to cope with the improvements done by this commit. Reported-by: Michael Mühlbeyer Author: Michael Paquier Reviewed-by: Tom Lane Discussion: https://postgr.es/m/89B7F072-5BBE-4C92-903E-D83E865D9367@trivadis.com Backpatch-through: 14
* Avoid warning about uninitialized value in MSVC python3 testsAndrew Dunstan2022-01-10
| | | | | | Juan José Santamaría Flecha Backpatch to all live branches
* Fix issues with describe queries of extended statistics in psqlMichael Paquier2022-01-08
| | | | | | | | | | | | | | | | This addresses some problems in the describe queries used for extended statistics: - Two schema qualifications for the text type were missing for \dX. - The list of extended statistics listed for a table through \d was ordered based on the object OIDs, but it is more consistent with the other commands to order by namespace and then by object name. - A couple of aliases were not used in \d. These are removed. This is similar to commits 1f092a3 and 07f8a9e. Author: Justin Pryzby Discussion: https://postgr.es/m/20220107022235.GA14051@telsasoft.com Backpatch-through: 14
* Allow MSVC .bat wrappers to be called from anywhereAndrew Dunstan2022-01-07
| | | | | | | | | | | | Instead of using a hardcoded or default path to the perl file the .bat file is a wrapper for, we use a path that means the file is found in the same directory as the .bat file. Patch by Anton Voloshin, slightly tweaked by me. Backpatch to all live branches Discussion: https://postgr.es/m/2b7a674b-5fb0-d264-75ef-ecc7a31e54f8@postgrespro.ru
* Prevent altering partitioned table's rowtype, if it's used elsewhere.Tom Lane2022-01-06
| | | | | | | | | | | | | | We disallow altering a column datatype within a regular table, if the table's rowtype is used as a column type elsewhere, because we lack code to go around and rewrite the other tables. This restriction should apply to partitioned tables as well, but it was not checked because ATRewriteTables and ATPrepAlterColumnType were not on the same page about who should do it for which relkinds. Per bug #17351 from Alexander Lakhin. Back-patch to all supported branches. Discussion: https://postgr.es/m/17351-6db1870f3f4f612a@postgresql.org
* Reduce relcache access in WAL sender streaming logical changesMichael Paquier2022-01-05
| | | | | | | | | | | | | | | | | | | | | | get_rel_sync_entry(), which is called each time a change needs to be logically replicated, is a rather hot code path in the WAL sender sending logical changes. This code path was doing a relcache access on relkind and relpartition for each logical change, but we only need to know this information when building or re-building the cached information for a relation. Some measurements prove that this is noticeable in perf profiles, particularly when attempting to replicate changes from relations that are not published as these cause less overhead in the WAL sender, delaying further the replication of changes for relations that are published. Issue introduced in 83fd453. Author: Hou Zhijie Reviewed-by: Kyotaro Horiguchi, Euler Taveira Discussion: https://postgr.es/m/OS0PR01MB5716E863AA9E591C1F010F7A947D9@OS0PR01MB5716.jpnprd01.prod.outlook.com Backpatch-through: 13
* Fix silly mistake in AssertAlvaro Herrera2022-01-04
|
* Allow special SKIP LOCKED condition in Assert()Alvaro Herrera2022-01-04
| | | | | | | | | | | | | | | | | Under concurrency, it is possible for two sessions to be merrily locking and releasing a tuple and marking it again as HEAP_XMAX_INVALID all the while a third session attempts to lock it, miserably fails at it, and then contemplates life, the universe and everything only to eventually fail an assertion that said bit is not set. Before SKIP LOCKED that was indeed a reasonable expectation, but alas! commit df630b0dd5ea falsified it. This bug is as old as time itself, and even older, if you think time begins with the oldest supported branch. Therefore, backpatch to all supported branches. Author: Simon Riggs <simon.riggs@enterprisedb.com> Discussion: https://postgr.es/m/CANbhV-FeEwMnN8yuMyss7if1ZKjOKfjcgqB26n8pqu1e=q0ebg@mail.gmail.com
* Fix index-only scan plans, take 2.Tom Lane2022-01-03
| | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | Commit 4ace45677 failed to fix the problem fully, because the same issue of attempting to fetch a non-returnable index column can occur when rechecking the indexqual after using a lossy index operator. Moreover, it broke EXPLAIN for such indexquals (which indicates a gap in our test cases :-(). Revert the code changes of 4ace45677 in favor of adding a new field to struct IndexOnlyScan, containing a version of the indexqual that can be executed against the index-returned tuple without using any non-returnable columns. (The restrictions imposed by check_index_only guarantee this is possible, although we may have to recompute indexed expressions.) Support construction of that during setrefs.c processing by marking IndexOnlyScan.indextlist entries as resjunk if they can't be returned, rather than removing them entirely. (We could alternatively require setrefs.c to look up the IndexOptInfo again, but abusing resjunk this way seems like a reasonably safe way to avoid needing to do that.) This solution isn't great from an API-stability standpoint: if there are any extensions out there that build IndexOnlyScan structs directly, they'll be broken in the next minor releases. However, only a very invasive extension would be likely to do such a thing. There's no change in the Path representation, so typical planner extensions shouldn't have a problem. As before, back-patch to all supported branches. Discussion: https://postgr.es/m/3179992.1641150853@sss.pgh.pa.us Discussion: https://postgr.es/m/17350-b5bdcf476e5badbb@postgresql.org
* Fix index-only scan plans when not all index columns can be returned.Tom Lane2022-01-01
| | | | | | | | | | | | | | | | | | | | | | | | | | | | | If an index has both returnable and non-returnable columns, and one of the non-returnable columns is an expression using a Var that is in a returnable column, then a query returning that expression could result in an index-only scan plan that attempts to read the non-returnable column, instead of recomputing the expression from the returnable column as intended. To fix, redefine the "indextlist" list of an IndexOnlyScan plan node as containing null Consts in place of any non-returnable columns. This solves the problem by preventing setrefs.c from falsely matching to such entries. The executor is happy since it only cares about the exposed types of the entries, and ruleutils.c doesn't care because a correct plan won't reference those entries. I considered some other ways to prevent setrefs.c from doing the wrong thing, but this way seems good since (a) it allows a very localized fix, (b) it makes the indextlist structure more compact in many cases, and (c) the indextlist is now a more faithful representation of what the index AM will actually produce, viz. nulls for any non-returnable columns. This is easier to hit since we introduced included columns, but it's possible to construct failing examples without that, as per the added regression test. Hence, back-patch to all supported branches. Per bug #17350 from Louis Jachiet. Discussion: https://postgr.es/m/17350-b5bdcf476e5badbb@postgresql.org
* Revert b2a459edf "Fix GRANTED BY support in REVOKE ROLE statements"Daniel Gustafsson2021-12-30
| | | | | | | | | | | The reverted commit attempted to fix SQL specification compliance for the cases which 6aaaa76bb left. This however broke existing behavior which takes precedence over spec compliance so revert. The introduced tests are left after the revert since the codepath isn't well covered. Per bug report 17346. Backpatch down to 14 where it was introduced. Reported-by: Andrew Bille <andrewbille@gmail.com> Discussion: https://postgr.es/m/17346-f72b28bd1a341060@postgresql.org
* Fix overly generic name in with.sql test.Thomas Munro2021-12-30
| | | | | | | | | | | | Avoid the name "test". In the 10 branch, this could clash with alter_table.sql, as seen in the build farm. That other instance was already renamed in later branches by commit 2cf8c7aa, but it's good to future-proof the name here too. Back-patch to 10. Reviewed-by: Tom Lane <tgl@sss.pgh.pa.us> Discussion: https://postgr.es/m/CA%2BhUKGJf4RAXUyAYVUcQawcptX%3DnhEco3SYpuPK5cCbA-F1eLA%40mail.gmail.com
* Correct comment and some documentation about REPLICA_IDENTITY_INDEXMichael Paquier2021-12-22
| | | | | | | | | | | | | | | | | | | catalog/pg_class.h was stating that REPLICA_IDENTITY_INDEX with a dropped index is equivalent to REPLICA_IDENTITY_DEFAULT. The code tells a different story, as it is equivalent to REPLICA_IDENTITY_NOTHING. The behavior exists since the introduction of replica identities, and fe7fd4e even added tests for this case but I somewhat forgot to fix this comment. While on it, this commit reorganizes the documentation about replica identities on the ALTER TABLE page, and a note is added about the case of dropped indexes with REPLICA_IDENTITY_INDEX. Author: Michael Paquier, Wei Wang Reviewed-by: Euler Taveira Discussion: https://postgr.es/m/OS3PR01MB6275464AD0A681A0793F56879E759@OS3PR01MB6275.jpnprd01.prod.outlook.com Backpatch-through: 10
* Remove assertion for ALTER TABLE .. DETACH PARTITION CONCURRENTLYMichael Paquier2021-12-22
| | | | | | | | | | | | | | | | | | | | | | | | | One code path related to this flavor of ALTER TABLE was checking that the relation to detach has to be a normal table or a partitioned table, which would fail if using the command with a different relation kind. Views, sequences and materialized views cannot be part of a partition tree, so these would cause the command to fail anyway, but the assertion was triggered. Foreign tables can be part of a partition tree, and again the assertion would have failed. The simplest solution is just to remove this assertion, so as we get the same failure as the non-concurrent code path. While on it, add a regression test in postgres_fdw for the concurrent partition detach of a foreign table, as per a suggestion from Alexander Lakhin. Issue introduced in 71f4c8c. Reported-by: Alexander Lakhin Author: Michael Paquier, Alexander Lakhin Reviewed-by: Peter Eisentraut, Kyotaro Horiguchi Discussion: https://postgr.es/m/17339-a9e09aaf38a3457a@postgresql.org Backpatch-through: 14
* Ensure casting to typmod -1 generates a RelabelType.Tom Lane2021-12-16
| | | | | | | | | | | | | | | | | | | | Fix the code changed by commit 5c056b0c2 so that we always generate RelabelType, not something else, for a cast to unspecified typmod. Otherwise planner optimizations might not happen. It appears we missed this point because the previous experiments were done on type numeric: the parser undesirably generates a call on the numeric() length-coercion function, but then numeric_support() optimizes that down to a RelabelType, so that everything seems fine. It misbehaves for types that have a non-optimized length coercion function, such as bpchar. Per report from John Naylor. Back-patch to all supported branches, as the previous patch eventually was. Unfortunately, that no longer includes 9.6 ... we really shouldn't put this type of change into a nearly-EOL branch. Discussion: https://postgr.es/m/CAFBsxsEfbFHEkouc+FSj+3K1sHipLPbEC67L0SAe-9-da8QtYg@mail.gmail.com
* Adjust behavior of some env settings for the TAP tests of MSVCMichael Paquier2021-12-15
| | | | | | | | | | | | | | | | | | | | | | | | | | | edc2332 has introduced in vcregress.pl some control on the environment variables LZ4, TAR and GZIP_PROGRAM to allow any TAP tests to be able use those commands. This makes the settings more consistent with src/Makefile.global.in, as the same default gets used for Make and MSVC builds. Each parameter can be changed in buildenv.pl, but as a default gets assigned after loading buldenv.pl, it is not possible to unset any of these, and using an empty value would not work with "||=" either. As some environments may not have a compatible command in their PATH (tar coming from MinGW is an issue, for one), this could break tests without an exit path to bypass any failing test. This commit changes things so as the default values for LZ4, TAR and GZIP_PROGRAM are assigned before loading buildenv.pl, not after. This way, we keep the same amount of compatibility as a GNU build with the same defaults, and it becomes possible to unset any of those values. While on it, this adds some documentation about those three variables in the section dedicated to the TAP tests for MSVC. Per discussion with Andrew Dunstan. Discussion: https://postgr.es/m/YbGYe483803il3X7@paquier.xyz Backpatch-through: 10
* Fix datatype confusion in logtape.c's right_offset().Tom Lane2021-12-14
| | | | | | | | | | | | | This could only matter if (a) long is wider than int, and (b) the heap of free blocks exceeds UINT_MAX entries, which seems pretty unlikely. Still, it's a theoretical bug, so backpatch to v13 where the typo came in (in commit c02fdc922). In passing, also make swap_nodes() use consistent datatypes. Ma Liangzhu Discussion: https://postgr.es/m/17336-fc4e522d26a750fd@postgresql.org
* Remove assertion for replication origins in PREPARE TRANSACTIONMichael Paquier2021-12-14
| | | | | | | | | | | | | | | | | When using replication origins, pg_replication_origin_xact_setup() is an optional choice to be able to set a LSN and a timestamp to mark the origin, which would be additionally added to WAL for transaction commits or aborts (including 2PC transactions). An assertion in the code path of PREPARE TRANSACTION assumed that this data should always be set, so it would trigger when using replication origins without setting up an origin LSN. Some tests are added to cover more this kind of scenario. Oversight in commit 1eb6d65. Per discussion with Amit Kapila and Masahiko Sawada. Discussion: https://postgr.es/m/YbbBfNSvMm5nIINV@paquier.xyz Backpatch-through: 11
* isolationtester: append session name to application_name.Andres Freund2021-12-13
| | | | | | | | | | | | | | | | | | | | When writing / debugging an isolation test it sometimes is useful to see which session holds what lock etc. To make it easier, both as part of spec files and interactively, append the session name to application_name. Since b1907d688 application_name already contains the test name, this appends the session's name to that. insert-conflict-specconflict did something like this manually, which can now be removed. As we have done lately with other test infrastructure improvements, backpatch this change, to make it easier to backpatch tests. Author: Andres Freund <andres@anarazel.de> Reviewed-By: Michael Paquier <michael@paquier.xyz> Reviewed-By: Andrew Dunstan <andrew@dunslane.net> Discussion: https://postgr.es/m/20211211012052.2blmzcmxnxqawd2z@alap3.anarazel.de Backpatch: 10-, to make backpatching of tests easier.
* Fix alignment in multirange_get_range() functionAlexander Korotkov2021-12-13
| | | | | | | | | The multirange_get_range() function fails when two boundaries of the same range have different alignments. Fix that by adding proper pointer alignment. Reported-by: Alexander Lakhin Discussion: https://postgr.es/m/17300-dced2d01ddeb1f2f%40postgresql.org Backpatch-through: 14
* Fix some typos with {a,an}Michael Paquier2021-12-09
| | | | | | | | One of the changes impacts the documentation, so backpatch. Author: Peter Smith Discussion: https://postgr.es/m/CAHut+Pu6+c+r3mY24VT7u+H+E_s6vMr5OdRiZ8NT3EOa-E5Lmw@mail.gmail.com Backpatch-through: 14
* Fix double publish of child table's data.Amit Kapila2021-12-09
| | | | | | | | | | | | | | | | We publish the child table's data twice for a publication that has both child and parent tables and is published with publish_via_partition_root as true. This happens because subscribers will initiate synchronization using both parent and child tables, since it gets both as separate tables in the initial table list. Ensure that pg_publication_tables returns only parent tables in such cases. Author: Hou Zhijie Reviewed-by: Greg Nancarrow, Amit Langote, Vignesh C, Amit Kapila Backpatch-through: 13 Discussion: https://postgr.es/m/OS0PR01MB57167F45D481F78CDC5986F794B99@OS0PR01MB5716.jpnprd01.prod.outlook.com
* Revert "Check that we have a working tar before trying to use it"Andrew Dunstan2021-12-08
| | | | | | | | | | | This reverts commit f920f7e799c587228227ec94356c760e3f3d5f2b. The patch in effect fixed a problem we didn't have and caused another instead. Backpatch to release 14 like original Discussion: https://postgr.es/m/3655283.1638977975@sss.pgh.pa.us
* Check that we have a working tar before trying to use itAndrew Dunstan2021-12-08
| | | | | | Issue exposed by commit edc2332550 and the buildfarm. Backpatch to release 14 where this usage started.
* Fix origin timestamp during decoding of ROLLBACK PREPARED operation.Amit Kapila2021-12-08
| | | | | | | | | | This happens because we were passing incorrect arguments to ReorderBufferFinishPrepared(). Author: Masahiko Sawada Reviewed-by: Vignesh C Backpatch-through: 14 Discussion: https://postgr.es/m/CAD21AoBqhUqgDZUhUVnnwKRubPDNJ6m6fJDPgok3E5cWJLL+pA@mail.gmail.com
generated by cgit v1.2.3 (git 2.25.1) at 2025-07-16 09:32:47 +0000