aboutsummaryrefslogtreecommitdiff
path: root/src/backend
Commit message (Collapse)AuthorAge
...
* Sync PG_VERSION file in CREATE DATABASE.Noah Misch2024-02-01
| | | | | | | | | | | An OS crash could leave PG_VERSION empty or missing. The same symptom appeared in a backup by block device snapshot, taken after the next checkpoint and before the OS flushes the PG_VERSION blocks. Device snapshots are not a documented backup method, however. Back-patch to v15, where commit 9c08aea6a3090a396be334cc58c511edab05776a introduced STRATEGY=WAL_LOG and made it the default. Discussion: https://postgr.es/m/20240130195003.0a.nmisch@google.com
* Handle interleavings between CREATE DATABASE steps and base backup.Noah Misch2024-02-01
| | | | | | | | | | | | | | | | | Restoring a base backup taken in the middle of CreateDirAndVersionFile() or write_relmap_file() would lose the function's effects. The symptom was absence of the database directory, PG_VERSION file, or pg_filenode.map. If missing the directory, recovery would fail. Either missing file would not fail recovery but would render the new database unusable. Fix CreateDirAndVersionFile() with the transam/README "action first and then write a WAL entry" strategy. That has a side benefit of moving filesystem mutations out of a critical section, reducing the ways to PANIC. Fix the write_relmap_file() call with a lock acquisition, so it interacts with checkpoints like non-CREATE DATABASE calls do. Back-patch to v15, where commit 9c08aea6a3090a396be334cc58c511edab05776a introduced STRATEGY=WAL_LOG and made it the default. Discussion: https://postgr.es/m/20240130195003.0a.nmisch@google.com
* Apply band-aid fix for an oversight in reparameterize_path_by_child.Tom Lane2024-02-01
| | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | The path we wish to reparameterize is not a standalone object: in particular, it implicitly references baserestrictinfo clauses in the associated RelOptInfo, and if it's a SampleScan path then there is also the TableSampleClause in the RTE to worry about. Both of those could contain lateral references to the join partner relation, which would need to be modified to refer to its child. Since we aren't doing that, affected queries can give wrong answers, or odd failures such as "variable not found in subplan target list", or executor crashes. But we can't just summarily modify those expressions, because they are shared with other paths for the rel. We'd break things if we modify them and then end up using some non-partitioned-join path. In HEAD, we plan to fix this by postponing reparameterization until create_plan(), when we know that those other paths are no longer of interest, and then adjusting those expressions along with the ones in the path itself. That seems like too big a change for stable branches however. In the back branches, let's just detect whether any troublesome lateral references actually exist in those expressions, and fail reparameterization if so. This will result in not performing a partitioned join in such cases. Given the lack of field complaints, nobody's likely to miss the optimization. Report and patch by Richard Guo. Apply to 12-16 only, since the intended fix for HEAD looks quite different. We're not quite ready to push the HEAD fix, but with back-branch releases coming up soon, it seems wise to get this stopgap fix in place there. Discussion: https://postgr.es/m/CAMbWs496+N=UAjOc=rcD3P7B6oJe4rZw08e_TZRUsWbPxZW3Tw@mail.gmail.com
* Fix stats_fetch_consistency with stats for fixed-numbered objectsMichael Paquier2024-02-01
| | | | | | | | | | | | | | | | | | | | | This impacts the statistics retrieved in transactions for the following views when updating the value of stats_fetch_consistency, leading to behaviors contrary to what is documented since 605994651b6a as an update of this parameter should discard all statistics snapshot data: - pg_stat_archiver - pg_stat_bgwriter - pg_stat_checkpointer - pg_stat_io - pg_stat_slru - pg_stat_wal For example, updating stats_fetch_consistency from "snapshot" to "cache" in a transaction did not re-fetch any fresh data, using data cached from the time when "snapshot" was in use. Author: Shinya Kato Discussion: https://postgr.es/m/d77fc5190d4dbe1738d77231488e768b@oss.nttdata.com Backpatch-through: 15
* Fix various issues with ALTER TEXT SEARCH CONFIGURATIONMichael Paquier2024-01-31
| | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | This commit addresses a set of issues when changing token type mappings in a text search configuration when using duplicated token names: - ADD MAPPING would fail on insertion because of a constraint failure after inserting the same mapping. - ALTER MAPPING with an "overridden" configuration failed with "tuple already updated by self" when the token mappings are removed. - DROP MAPPING failed with "tuple already updated by self", like previously, but in a different code path. The code is refactored so the token names (with their numbers) are handled as a List with unique members rather than an array with numbers, ensuring that no duplicates mess up with the catalog inserts, updates and deletes. The list is generated by getTokenTypes(), with the same error handling as previously while duplicated tokens are discarded from the list used to work on the catalogs. Regression tests are expanded to cover much more ground for the cases fixed by this commit, as there was no coverage for the code touched in this commit. A bit more is done regarding the fact that a token name not supported by a configuration's parser should result in an error even if IF EXISTS is used in a DROP MAPPING clause. This is implied in the code but there was no coverage for that, and it was very easy to miss. These issues exist since at least their introduction in core with 140d4ebcb46e, so backpatch all the way down. Reported-by: Alexander Lakhin Author: Tender Wang, Michael Paquier Discussion: https://postgr.es/m/18310-1eb233c5908189c8@postgresql.org Backpatch-through: 12
* Doc: mention foreign keys can reference unique indexesDavid Rowley2024-01-30
| | | | | | | | | | | | | | | | We seem to have only documented a foreign key can reference the columns of a primary key or unique constraint. Here we adjust the documentation to mention columns in a non-partial unique index can be mentioned too. The header comment for transformFkeyCheckAttrs() also didn't mention unique indexes, so fix that too. In passing make that header comment reflect reality in the various other aspects where it deviated from it. Bug: 18295 Reported-by: Gilles PARC Author: Laurenz Albe, David Rowley Discussion: https://www.postgresql.org/message-id/18295-0ed0fac5c9f7b17b%40postgresql.org Backpatch-through: 12
* Fix incompatibilities with libxml2 >= 2.12.0.Tom Lane2024-01-29
| | | | | | | | | | | | | | | | | | | | | | libxml2 changed the required signature of error handler callbacks to make the passed xmlError struct "const". This is causing build failures on buildfarm member caiman, and no doubt will start showing up in the field quite soon. Add a version check to adjust the declaration of xml_errorHandler() according to LIBXML_VERSION. 2.12.x also produces deprecation warnings for contrib/xml2/xpath.c's assignment to xmlLoadExtDtdDefaultValue. I see no good reason for that to still be there, seeing that we disabled external DTDs (at a lower level) years ago for security reasons. Let's just remove it. Back-patch to all supported branches, since they might all get built with newer libxml2 once it gets a bit more popular. (The back branches produce another deprecation warning about xpath.c's use of xmlSubstituteEntitiesDefault(). We ought to consider whether to back-patch all or part of commit 65c5864d7 to silence that. It's less urgent though, since it won't break the buildfarm.) Discussion: https://postgr.es/m/1389505.1706382262@sss.pgh.pa.us
* Fix locking when fixing an incomplete split of a GIN internal pageHeikki Linnakangas2024-01-29
| | | | | | | | | | | | | | | | | ginFinishSplit() expects the caller to hold an exclusive lock on the buffer, but when finishing an earlier "leftover" incomplete split of an internal page, the caller held a shared lock. That caused an assertion failure in MarkBufferDirty(). Without assertions, it could lead to corruption if two backends tried to complete the split at the same time. On master, add a test case using the new injection point facility. Report and analysis by Fei Changhong. Backpatch the fix to all supported versions. Reviewed-by: Fei Changhong, Michael Paquier Discussion: https://www.postgresql.org/message-id/tencent_A3CE810F59132D8E230475A5F0F7A08C8307@qq.com
* Fix catalog lookup due to wrong snapshot for subtransactions during decoding.Amit Kapila2024-01-29
| | | | | | | | | | | | | | | | | | | | | | | | | | In commit 272248a0c, we fixed the catalog lookup due to the wrong snapshot for transactions and subtransactions during decoding. We failed to consider the case where top-level xact is already marked as containing catalog change but its subtransaction is not yet marked as containing catalog change even though it contained such a change. This can happen when during decoding, none of the WAL records from the subtransaction was decoded and top-level xact contains a DDL. We fix it by marking the transaction and all its subtransactions as containing catalog changes if the top-level xact contains any catalog change and it is present in the initial running xacts array. This fix is required only for 14 and 15 because in prior branches we already always mark the transaction and all its subtransactions as containing catalog changes in the same case. In 16 and above, we preserve the list of transaction IDs and sub-transaction IDs, that have modified catalogs and are running during snapshot serialization, to the serialized snapshot (see commit 7f13ac8123). Author: Fei Changhong Reviewed-by: Amit Kapila, Hayato Kuroda, Andy Fan Discussion: https://postgr.es/m/18280-4c8060178cb41750@postgresql.org
* Add more LOG messages when starting and ending recovery from a backupMichael Paquier2024-01-29
| | | | | | | | | | | | | | | | | | | | | Three LOG messages are added in the recovery code paths, providing information that can be useful to track corruption issues depending on the state of the cluster, telling that: - Recovery has started from a backup_label. - Recovery is restarting from a backup start LSN, without a backup_label. - Recovery has completed from a backup. This was originally applied on HEAD as of 1d35f705e191, and there is consensus that this can be useful for older versions. This applies cleanly down to 15, so do it down to this version for now (older versions have heavily refactored the WAL recovery paths, making the change less straight-forward to do). Author: Andres Freund Reviewed-by: David Steele, Laurenz Albe, Michael Paquier Discussion: https://postgr.es/m/20231117041811.vz4vgkthwjnwp2pp@awork3.anarazel.de Backpatch-through: 15
* Detect Julian-date overflow in timestamp[tz]_pl_interval.Tom Lane2024-01-26
| | | | | | | | | | | | | | | | | | | | We perform addition of the days field of an interval via arithmetic on the Julian-date representation of the timestamp's date. This step is subject to int32 overflow, and we also should not let the Julian date become very negative, for fear of weird results from j2date. (In the timestamptz case, allow a Julian date of -1 to pass, since it might convert back to zero after timezone rotation.) The additions of the months and microseconds fields could also overflow, of course. However, I believe we need no additional checks there; the existing range checks should catch such cases. The difficulty here is that j2date's magic modular arithmetic could produce something that looks like it's in-range. Per bug #18313 from Christian Maurer. This has been wrong for a long time, so back-patch to all supported branches. Discussion: https://postgr.es/m/18313-64d2c8952d81e84b@postgresql.org
* Track LLVM 18 changes.Thomas Munro2024-01-25
| | | | | | | | | | A function was given a newly standard name from C++20 in LLVM 16. Then LLVM 18 added a deprecation warning for the old name, and it is about to ship, so it's time to adjust that. Back-patch to all supported releases. Discussion: https://www.postgresql.org/message-id/CA+hUKGLbuVhH6mqS8z+FwAn4=5dHs0bAWmEMZ3B+iYHWKC4-ZA@mail.gmail.com
* Fix ALTER TABLE .. ADD COLUMN with complex inheritance treesMichael Paquier2024-01-24
| | | | | | | | | | | | | | | | This command, when used to add a column on a parent table with a complex inheritance tree, tried to update multiple times the same tuple in pg_attribute for a child table when incrementing attinhcount, causing failures with "tuple already updated by self" because of a missing CommandCounterIncrement() between two updates. This exists for a rather long time, so backpatch all the way down. Reported-by: Alexander Lakhin Author: Tender Wang Reviewed-by: Richard Guo Discussion: https://postgr.es/m/18297-b04cd83a55b51e35@postgresql.org Backpatch-through: 12
* Improve handling of dropped partitioned indexes for REINDEX INDEXMichael Paquier2024-01-18
| | | | | | | | | | | | | | | | | | | A REINDEX INDEX done on a partitioned index builds a list of the indexes to work on before processing its partitions in individual transactions. When combined with a DROP of the partitioned index, there was a window where it was possible to see some unexpected "could not open relation with OID", synonym of relation lookup error. The code was robust enough to handle the case where the parent relation is missing, but not the case where an index would be gone missing. This is similar to 1d65416661bb. Support for REINDEX on partitioned relations has been introduced in a6642b3ae060, so backpatch down to 14. Author: Fei Changhong Discussion: https://postgr.es/m/tencent_6A52106095ACDE55333E3AD33F304C0C3909@qq.com Backpatch-through: 14
* Add try_index_open(), conditional variant of index_open()Michael Paquier2024-01-18
| | | | | | | | | | | | | | try_index_open() is able to open an index if its relkind fits, except that it would return NULL instead of generated an error if the relation does not exist. This new routine will be used by an upcoming patch to make REINDEX on partitioned relations more robust when an index in a partition tree is dropped. Extracted from a larger patch by the same author. Author: Fei Changhong Discussion: https://postgr.es/m/tencent_6A52106095ACDE55333E3AD33F304C0C3909@qq.com Backpatch-through: 14
* lwlock: Fix quadratic behavior with very long wait listsAndres Freund2024-01-18
| | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | Until now LWLockDequeueSelf() sequentially searched the list of waiters to see if the current proc is still is on the list of waiters, or has already been removed. In extreme workloads, where the wait lists are very long, this leads to a quadratic behavior. #backends iterating over a list #backends long. Additionally, the likelihood of needing to call LWLockDequeueSelf() in the first place also increases with the increased length of the wait queue, as it becomes more likely that a lock is released while waiting for the wait list lock, which is held for longer during lock release. Due to the exponential back-off in perform_spin_delay() this is surprisingly hard to detect. We should make that easier, e.g. by adding a wait event around the pg_usleep() - but that's a separate patch. The fix is simple - track whether a proc is currently waiting in the wait list or already removed but waiting to be woken up in PGPROC->lwWaiting. In some workloads with a lot of clients contending for a small number of lwlocks (e.g. WALWriteLock), the fix can substantially increase throughput. This has been originally fixed for 16~ with a4adc31f6902 without a backpatch, and we have heard complaints from users impacted by this quadratic behavior in older versions as well. Author: Andres Freund <andres@anarazel.de> Reviewed-by: Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com> Discussion: https://postgr.es/m/20221027165914.2hofzp4cvutj6gin@awork3.anarazel.de Discussion: https://postgr.es/m/CALj2ACXktNbG=K8Xi7PSqbofTZozavhaxjatVc14iYaLu4Maag@mail.gmail.com Backpatch-through: 12
* Fix incorrect comment on how BackendStatusArray is indexedHeikki Linnakangas2024-01-17
| | | | | | | | | | | The comment was copy-pasted from the call to ProcSignalInit() in AuxiliaryProcessMain(), which uses a similar scheme of having reserved slots for aux processes after MaxBackends slots for backends. However, ProcSignalInit() indexing starts from 1, whereas BackendStatusArray starts from 0. The code is correct, but the comment was wrong. Discussion: https://www.postgresql.org/message-id/f3ecd4cb-85ee-4e54-8278-5fabfb3a4ed0@iki.fi Backpatch-through: v14
* Close socket in case of errors in setting non-blockingDaniel Gustafsson2024-01-17
| | | | | | | | | | | | | If configuring the newly created socket non-blocking fails we error out and return INVALID_SOCKET, but the socket that had been created wasn't closed. Fix by issuing closesocket in the errorpath. Backpatch to all supported branches. Author: Ranier Vilela <ranier.vf@gmail.com> Discussion: https://postgr.es/m/CAEudQApmU5CrKefH85VbNYE2y8H=-qqEJbg6RAPU65+vCe+89A@mail.gmail.com Backpatch-through: v12
* Prevent access to an unpinned buffer in BEFORE ROW UPDATE triggers.Tom Lane2024-01-14
| | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | When ExecBRUpdateTriggers switches to a new target tuple as a result of the EvalPlanQual logic, it must form a new proposed update tuple. Since commit 86dc90056, that tuple (the result of ExecGetUpdateNewTuple) has been a virtual tuple that might contain pointers to by-ref fields of the new target tuple (in "oldslot"). However, immediately after that we materialize oldslot, causing it to drop its buffer pin, whereupon the by-ref pointers are unsafe to use. This is a live bug only when the new target tuple is in a different page than the original target tuple, since we do still hold a pin on the original one. (Before 86dc90056, there was no bug because the EPQ plantree would hold a pin on the new target tuple; but now that's not assured.) To fix, forcibly materialize the new tuple before we materialize oldslot. This costs nothing since we would have done that shortly anyway. The real-world impact of this is probably minimal. A visible failure could occur if the new target tuple's buffer were recycled for some other page in the short interval before we materialize newslot within the trigger-calling loop; but that's quite unlikely given that we'd just touched that page. There's a larger hazard that some other process could prune and repack that page within the window. We have lock on the new target tuple, but that wouldn't prevent it being moved on the page. Alexander Lakhin and Tom Lane, per bug #17798 from Alexander Lakhin. Back-patch to v14 where 86dc90056 came in. Discussion: https://postgr.es/m/17798-0907404928dcf0dd@postgresql.org
* Re-pgindent catcache.c after previous commit.Tom Lane2024-01-13
| | | | | Discussion: https://postgr.es/m/1393953.1698353013@sss.pgh.pa.us Discussion: https://postgr.es/m/CAGjhLkOoBEC9mLsnB42d3CO1vcMx71MLSEuigeABbQ8oRdA6gw@mail.gmail.com
* Cope with catcache entries becoming stale during detoasting.Tom Lane2024-01-13
| | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | We've long had a policy that any toasted fields in a catalog tuple should be pulled in-line before entering the tuple in a catalog cache. However, that requires access to the catalog's toast table, and we'll typically do AcceptInvalidationMessages while opening the toast table. So it's possible that the catalog tuple is outdated by the time we finish detoasting it. Since no cache entry exists yet, we can't mark the entry stale during AcceptInvalidationMessages, and instead we'll press forward and build an apparently-valid cache entry. The upshot is that we have a race condition whereby an out-of-date entry could be made in a backend's catalog cache, and persist there indefinitely causing indeterminate misbehavior. To fix, use the existing systable_recheck_tuple code to recheck whether the catalog tuple is still up-to-date after we finish detoasting it. If not, loop around and restart the process of searching the catalog and constructing cache entries from the top. The case is rare enough that this shouldn't create any meaningful performance penalty, even in the SearchCatCacheList case where we need to tear down and reconstruct the whole list. Indeed, the case is so rare that AFAICT it doesn't occur during our regression tests, and there doesn't seem to be any easy way to build a test that would exercise it reliably. To allow testing of the retry code paths, add logic (in USE_ASSERT_CHECKING builds only) that randomly pretends that the recheck failed about one time out of a thousand. This is enough to ensure that we'll pass through the retry paths during most regression test runs. By adding an extra level of looping, this commit creates a need to reindent most of SearchCatCacheMiss and SearchCatCacheList. I'll do that separately, to allow putting those changes in .git-blame-ignore-revs. Patch by me; thanks to Alexander Lakhin for having built a test case to prove the bug is real, and to Xiaoran Wang for review. Back-patch to all supported branches. Discussion: https://postgr.es/m/1393953.1698353013@sss.pgh.pa.us Discussion: https://postgr.es/m/CAGjhLkOoBEC9mLsnB42d3CO1vcMx71MLSEuigeABbQ8oRdA6gw@mail.gmail.com
* Allow subquery pullup to wrap a PlaceHolderVar in another one.Tom Lane2024-01-11
| | | | | | | | | | | | | | | | | | | | | | | The code for wrapping subquery output expressions in PlaceHolderVars believed that if the expression already was a PlaceHolderVar, it was never necessary to wrap that in another one. That's wrong if the expression is underneath an outer join and involves a lateral reference to outside that scope: failing to add an additional PHV risks evaluating the expression at the wrong place and hence not forcing it to null when the outer join should do so. This is an oversight in commit 9e7e29c75, which added logic to forcibly wrap lateral-reference Vars in PlaceHolderVars, but didn't see that the adjacent case for PlaceHolderVars needed the same treatment. The test case we have for this doesn't fail before 4be058fe9, but now that I see the problem I wonder if it is possible to demonstrate related errors before that. That's moot though, since all such branches are out of support. Per bug #18284 from Holger Reise. Back-patch to all supported branches. Discussion: https://postgr.es/m/18284-47505a20c23647f8@postgresql.org
* Handle WindowClause.runCondition in tree walker/mutator functions.Tom Lane2024-01-10
| | | | | | | | | | | | | | | | | Commit 9d9c02ccd, which added the notion of a "run condition" for window functions, neglected to teach nodeFuncs.c to process the new field. Remarkably, that doesn't seem to have had any ill effects before we invented Var.varnullingrels, but now it can cause visible failures in join-removal scenarios. I have no faith that there's not reachable problems in v15 too, so back-patch the code change to v15 where 9d9c02ccd came in. The test case seems irrelevant to v15, though. Per bug #18277 from Zuming Jiang. Diagnosis and patch by Richard Guo. Discussion: https://postgr.es/m/18277-089ead83b329a2fd@postgresql.org
* Fix indentation in ExecParallelHashIncreaseNumBatches()Alexander Korotkov2024-01-08
| | | | Backpatch-through: 12
* Fix oversized memory allocation in Parallel Hash JoinAlexander Korotkov2024-01-07
| | | | | | | | | | | | During the calculations of the maximum for the number of buckets, take into account that later we round that to the next power of 2. Reported-by: Karen Talarico Bug: #16925 Discussion: https://postgr.es/m/16925-ec96d83529d0d629%40postgresql.org Author: Thomas Munro, Andrei Lepikhov, Alexander Korotkov Reviewed-by: Alena Rybakina Backpatch-through: 12
* Avoid masking EOF (no-password-supplied) conditions in auth.c.Tom Lane2024-01-03
| | | | | | | | | | | | | | | CheckPWChallengeAuth() would return STATUS_ERROR if the user does not exist or has no password assigned, even if the client disconnected without responding to the password challenge (as libpq often will, for example). We should return STATUS_EOF in that case, and the lower-level functions do, but this code level got it wrong since the refactoring done in 7ac955b34. This breaks the intent of not logging anything for EOF cases (cf. comments in auth_failed()) and might also confuse users of ClientAuthentication_hook. Per report from Liu Lang. Back-patch to all supported versions. Discussion: https://postgr.es/m/b725238c-539d-cb09-2bff-b5e6cb2c069c@esgyn.cn
* Fix failure to verify PGC_[SU_]BACKEND GUCs in pg_file_settings view.Tom Lane2023-12-26
| | | | | | | | | | | | | | | | | | | | | | | | | | | | | | set_config_option() bails out early if it detects that the option to be set is PGC_BACKEND or PGC_SU_BACKEND class and we're reading the config file in a postmaster child; we don't want to apply any new value in such a case. That's fine as far as it goes, but it fails to consider the requirements of the pg_file_settings view: for that, we need to check validity of the value even though we have no intention to apply it. Because we didn't, even very silly values for affected GUCs would be reported as valid by the view. There are only half a dozen such GUCs, which perhaps explains why this got overlooked for so long. Fix by continuing when changeVal is false; this parallels the logic in some other early-exit paths. Also, the check added by commit 924bcf4f1 to prevent GUC changes in parallel workers seems a few bricks shy of a load: it's evidently assuming that ereport(elevel, ...) won't return. Make sure we bail out if it does. The lack of trouble reports suggests that this is only a latent bug, i.e. parallel workers don't actually reach here with elevel < ERROR. (Per the code coverage report, we never reach here at all in the regression suite.) But we clearly don't want to risk proceeding if that does happen. Per report from Rıdvan Korkmaz. These are ancient bugs, so back-patch to all supported branches. Discussion: https://postgr.es/m/2089235.1703617353@sss.pgh.pa.us
* Avoid trying to fetch metapage of an SPGist partitioned index.Tom Lane2023-12-21
| | | | | | | | | | | | | | | | | | | This is necessary when spgcanreturn() is invoked on a partitioned index, and the failure might be reachable in other scenarios as well. The rest of what spgGetCache() does is perfectly sensible for a partitioned index, so we should allow it to go through. I think the main takeaway from this is that we lack sufficient test coverage for non-btree partitioned indexes. Therefore, I added simple test cases for brin and gin as well as spgist (hash and gist AMs were covered already in indexing.sql). Per bug #18256 from Alexander Lakhin. Although the known test case only fails since v16 (3c569049b), I've got no faith at all that there aren't other ways to reach this problem; so back-patch to all supported branches. Discussion: https://postgr.es/m/18256-0b0e1b6e4a620f1b@postgresql.org
* Fix BEFORE ROW trigger handling in cross-partition MERGE update.Dean Rasheed2023-12-21
| | | | | | | | | | | | | | | | | | | | | | | | | | | | Fix a bug during MERGE if a cross-partition update is attempted on a partitioned table with a BEFORE DELETE ROW trigger that returns NULL, to prevent the update. This would cause an error to be thrown, or an assert failure in an assert-enabled build. This was an oversight in 9321c79c86, which failed to properly distinguish a DELETE prevented by a trigger from one prevented by a concurrent update. Fix by having ExecDelete() return the TM_Result status to ExecCrossPartitionUpdate(), so that it can distinguish the two cases, and make ExecCrossPartitionUpdate() return the TM_Result status to ExecUpdateAct(), so that it can return the correct status from a concurrent update. In addition, ensure that the command tag is correctly updated by having ExecMergeMatched() pass canSetTag to ExecUpdateAct(), rather than passing false, so that it updates the command tag if it does a cross-partition update, making this code path in ExecMergeMatched() consistent with ExecUpdate(). Per bug #18238 from Alexander Lakhin. Back-patch to v15, where MERGE was introduced. Dean Rasheed, reviewed by Richard Guo and Jian He. Discussion: https://postgr.es/m/18238-2f2bdc7f720180b9%40postgresql.org
* Fix bugs in manipulation of large objects.Tom Lane2023-12-15
| | | | | | | | | | | | | | | | | | | | | | | | | | | | | | In v16 and up (since commit afbfc0298), large object ownership checking has been broken because object_ownercheck() didn't take care of the discrepancy between our object-address representation of large objects (classId == LargeObjectRelationId) and the catalog where their ownership info is actually stored (LargeObjectMetadataRelationId). This resulted in failures such as "unrecognized class ID: 2613" when trying to update blob properties as a non-superuser. Poking around for related bugs, I found that AlterObjectOwner_internal would pass the wrong classId to the PostAlterHook in the no-op code path where the large object already has the desired owner. Also, recordExtObjInitPriv checked for the wrong classId; that bug is only latent because the stanza is dead code anyway, but as long as we're carrying it around it should be less wrong. These bugs are quite old. In HEAD, we can reduce the scope for future bugs of this ilk by changing AlterObjectOwner_internal's API to let the translation happen inside that function, rather than requiring callers to know about it. A more bulletproof fix, perhaps, would be to start using LargeObjectMetadataRelationId as the dependency and object-address classId for blobs. However that has substantial risk of breaking third-party code; even within our own code, it'd create hassles for pg_dump which would have to cope with a version-dependent representation. For now, keep the status quo. Discussion: https://postgr.es/m/2650449.1702497209@sss.pgh.pa.us
* Fix description of I/O timing info for shared buffers in EXPLAIN (BUFFERS)Michael Paquier2023-12-14
| | | | | | | | | | | | | | | | This fixes an error introduced by efb0ef909f60, that changed the description of this field to "shared/local" while these I/O timings relate to shared buffers. This information is available when track_io_timing is enabled. Note that HEAD has added new counters for local buffers in 295c36c0c1fa, so there is no need to touch it. The description is updated to "shared" to be compatible with HEAD. Per discussion with Nazir Bilal Yavuz and Hubert Depesz Lubaczewski, whose EXPLAIN analyzer tool was not actually able to parse the previous term because of the slash character. Discussion: https://postgr.es/m/ZTCTiUqm_H3iBihl@paquier.xyz Backpatch-through: 15
* Prevent tuples to be marked as dead in subtransactions on standbysMichael Paquier2023-12-12
| | | | | | | | | | | | | | | | | | | | | | | Dead tuples are ignored and are not marked as dead during recovery, as it can lead to MVCC issues on a standby because its xmin may not match with the primary. This information is tracked by a field called "xactStartedInRecovery" in the transaction state data, switched on when starting a transaction in recovery. Unfortunately, this information was not correctly tracked when starting a subtransaction, because the transaction state used for the subtransaction did not update "xactStartedInRecovery" based on the state of its parent. This would cause index scans done in subtransactions to return inconsistent data, depending on how the xmin of the primary and/or the standby evolved. This is broken since the introduction of hot standby in efc16ea52067, so backpatch all the way down. Author: Fei Changhong Reviewed-by: Kyotaro Horiguchi Discussion: https://postgr.es/m/tencent_C4D907A5093C071A029712E73B43C6512706@qq.com Backpatch-through: 12
* Fix typo in commentDaniel Gustafsson2023-12-12
| | | | | | | | Commit 98e675ed7af accidentally mistyped IDENTIFY_SYSTEM as IDENTIFY_SERVER. Backpatch to all supported branches. Reported-by: Alexander Lakhin <exclusion@gmail.com> Discussion: https://postgr.es/m/68138521-5345-8780-4390-1474afdcba1f@gmail.com
* Be more wary about OpenSSL not setting errno on error.Tom Lane2023-12-11
| | | | | | | | | | | | | | | | | | | | | | OpenSSL will sometimes return SSL_ERROR_SYSCALL without having set errno; this is apparently a reflection of recv(2)'s habit of not setting errno when reporting EOF. Ensure that we treat such cases the same as read EOF. Previously, we'd frequently report them like "could not accept SSL connection: Success" which is confusing, or worse report them with an unrelated errno left over from some previous syscall. To fix, ensure that errno is zeroed immediately before the call, and report its value only when it's not zero afterwards; otherwise report EOF. For consistency, I've applied the same coding pattern in libpq's pqsecure_raw_read(). Bare recv(2) shouldn't really return -1 without setting errno, but in case it does we might as well cope. Per report from Andres Freund. Back-patch to all supported versions. Discussion: https://postgr.es/m/20231208181451.deqnflwxqoehhxpe@awork3.anarazel.de
* Fix an undetected deadlock due to apply worker.Amit Kapila2023-12-11
| | | | | | | | | | | | | | | | | | | | | | | | | | | | The apply worker needs to update the state of the subscription tables to 'READY' during the synchronization phase which requires locking the corresponding subscription. The apply worker also waits for the subscription tables to reach the 'SYNCDONE' state after holding the locks on the subscription and the wait is done using WaitLatch. The 'SYNCDONE' state is changed by tablesync workers again by locking the corresponding subscription. Both the state updates use AccessShareLock mode to lock the subscription, so they can't block each other. However, a backend can simultaneously try to acquire a lock on the same subscription using AccessExclusiveLock mode to alter the subscription. Now, the backend's wait on a lock can sneak in between the apply worker and table sync worker causing deadlock. In other words, apply_worker waits for tablesync worker which waits for backend, and backend waits for apply worker. This is not detected by the deadlock detector because apply worker uses WaitLatch. The fix is to release existing locks in apply worker before it starts to wait for tablesync worker to change the state. Reported-by: Tomas Vondra Author: Shlok Kyal Reviewed-by: Amit Kapila, Peter Smith Backpatch-through: 12 Discussion: https://postgr.es/m/d291bb50-12c4-e8af-2af2-7bb9bb4d8e3e@enterprisedb.com
* Fix potential pointer overflow in xlogreader.c.Thomas Munro2023-12-08
| | | | | | | | | | | | | | | | | | | While checking if a record could fit in the circular WAL decoding buffer, the coding from commit 3f1ce973 used arithmetic that could overflow. 64 bit systems were unaffected for various technical reasons, which probably explains the lack of problem reports. Likewise for 32 bit systems running known 32 bit kernels. The systems at risk of problems appear to be 32 bit processes running on 64 bit kernels, with unlucky placement in memory. Per complaint from GCC -fsanitize=undefined -m32, while testing variations of 039_end_of_wal.pl. Back-patch to 15. Reviewed-by: Nathan Bossart <nathandbossart@gmail.com> Reviewed-by: Robert Haas <robertmhaas@gmail.com> Discussion: https://postgr.es/m/CA%2BhUKGKH0oRPOX7DhiQ_b51sM8HqcPp2J3WA-Oen%3DdXog%2BAGGQ%40mail.gmail.com
* Fix compilation on Windows with WAL_DEBUGMichael Paquier2023-12-06
| | | | | | | | | | This has been broken since b060dbe0001a that has reworked the callback mechanism of XLogReader, most likely unnoticed because any form of development involving WAL happens on platforms where this compiles fine. Author: Bharath Rupireddy Discussion: https://postgr.es/m/CALj2ACVF14WKQMFwcJ=3okVDhiXpuK5f7YdT+BdYXbbypMHqWA@mail.gmail.com Backpatch-through: 13
* Fix incorrect error message for IDENTIFY_SYSTEMDaniel Gustafsson2023-12-05
| | | | | | | | | | | | | | | | Commit 5a991ef8692e accidentally reversed the order of the tuples and fields parameters, making the error message incorrectly refer to 3 tuples with 1 field when IDENTIFY_SYSTEM returns 1 tuple and 3 or 4 fields. Fix by changing the order of the parameters. This also adds a comment describing why we check for < 3 when postgres since 9.4 has been sending 4 fields. Backpatch all the way since the bug is almost a decade old. Author: Tomonari Katsumata <t.katsumata1122@gmail.com> Reviewed-by: Tom Lane <tgl@sss.pgh.pa.us> Bug: #18224 Backpatch-through: v12
* Check collation when creating partitioned indexPeter Eisentraut2023-12-01
| | | | | | | | | | | | | | | | | When creating a partitioned index, the partition key must be a subset of the index's columns. But this currently doesn't check that the collations between the partition key and the index definition match. So you can construct a unique index that fails to enforce uniqueness. (This would most likely involve a nondeterministic collation, so it would have to be crafted explicitly and is not something that would just happen by accident.) This patch adds the required collation check. As a result, any previously allowed unique index that has a collation mismatch would no longer be allowed to be created. Reviewed-by: Tom Lane <tgl@sss.pgh.pa.us> Discussion: https://www.postgresql.org/message-id/flat/3327cb54-f7f1-413b-8fdb-7a9dceebb938%40eisentraut.org
* Use BIO_{get,set}_app_data instead of BIO_{get,set}_data.Tom Lane2023-11-28
| | | | | | | | | | | | | | | | | | | | | | We should have done it this way all along, but we accidentally got away with using the wrong BIO field up until OpenSSL 3.2. There, the library's BIO routines that we rely on use the "data" field for their own purposes, and our conflicting use causes assorted weird behaviors up to and including core dumps when SSL connections are attempted. Switch to using the approved field for the purpose, i.e. app_data. While at it, remove our configure probes for BIO_get_data as well as the fallback implementation. BIO_{get,set}_app_data have been there since long before any OpenSSL version that we still support, even in the back branches. Also, update src/test/ssl/t/001_ssltests.pl to allow for a minor change in an error message spelling that evidently came in with 3.2. Tristan Partin and Bo Andreson. Back-patch to all supported branches. Discussion: https://postgr.es/m/CAN55FZ1eDDYsYaL7mv+oSLUij2h_u6hvD4Qmv-7PK7jkji0uyQ@mail.gmail.com
* Fix assertions with RI triggers in heap_update and heap_delete.Heikki Linnakangas2023-11-28
| | | | | | | | | | | | If the tuple being updated is not visible to the crosscheck snapshot, we return TM_Updated but the assertions would not hold in that case. Move them to before the cross-check. Fixes bug #17893. Backpatch to all supported versions. Author: Alexander Lakhin Backpatch-through: 12 Discussion: https://www.postgresql.org/message-id/17893-35847009eec517b5%40postgresql.org
* Avoid unconditionally filling in missing values with NULL in pgoutput.Amit Kapila2023-11-27
| | | | | | | | | | | | | | | | | | | | 52e4f0cd4 introduced a bug in pgoutput in which missing values in tuples were incorrectly filled in with NULL. The problem was the use of CreateTupleDescCopy where CreateTupleDescCopyConstr was required, as the former drops the constraints in the tuple description (specifically, the default value constraint) on the floor. The bug could result in incorrectness when a table replicated via `REPLICA IDENTITY FULL` underwent a schema change that added a column with a default value. The problem is that in such cases updates fill NULL values in old tuples for missing columns for default values. Then on the subscriber, we failed to find a matching tuple and missed updating the required row. Author: Nikhil Benesch Reviewed-by: Hou Zhijie, Amit Kapila Backpatch-through: 15 Discussion: http://postgr.es/m/CAPWqQZTEpZQamYsGMn6ZDRvVywwpVPiKH6OY4KSgA+NmeqFNzA@mail.gmail.com
* Fix timing-dependent failure in GSSAPI data transmission.Tom Lane2023-11-23
| | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | When using GSSAPI encryption in non-blocking mode, libpq sometimes failed with "GSSAPI caller failed to retransmit all data needing to be retried". The cause is that pqPutMsgEnd rounds its transmit request down to an even multiple of 8K, and sometimes that can lead to not requesting a write of data that was requested to be written (but reported as not written) earlier. That can upset pg_GSS_write's logic for dealing with not-yet-written data, since it's possible the data in question had already been incorporated into an encrypted packet that we weren't able to send during the previous call. We could fix this with a one-or-two-line hack to disable pqPutMsgEnd's round-down behavior, but that seems like making the caller work around a behavior that pg_GSS_write shouldn't expose in this way. Instead, adjust pg_GSS_write to never report a partial write: it either reports a complete write, or reflects the failure of the lower-level pqsecure_raw_write call. The requirement still exists for the caller to present at least as much data as on the previous call, but with the caller-visible write start point not moving there is no temptation for it to present less. We lose some ability to reclaim buffer space early, but I doubt that that will make much difference in practice. This also gets rid of a rather dubious assumption that "any interesting failure condition (from pqsecure_raw_write) will recur on the next try". We've not seen failure reports traceable to that, but I've never trusted it particularly and am glad to remove it. Make the same adjustments to the equivalent backend routine be_gssapi_write(). It is probable that there's no bug on the backend side, since we don't have a notion of nonblock mode there; but we should keep the logic the same to ease future maintenance. Per bug #18210 from Lars Kanis. Back-patch to all supported branches. Discussion: https://postgr.es/m/18210-4c6d0b14627f2eb8@postgresql.org
* Fix resource leak when a FDW's ForeignAsyncRequest function failsHeikki Linnakangas2023-11-23
| | | | | | | | | | | | | | | | | | | | | | | | If an error is thrown after calling CreateWaitEventSet(), the memory of a WaitEventSet is free'd as it's allocated in the short-lived memory context, but the file descriptor (on epoll- or kqueue-based systems) or handles (on Windows) that it contains are leaked. Use PG_TRY-FINALLY to ensure it gets freed. (On master, I will apply a better fix, using ResourceOwners to track the WaitEventSet, but that's not backpatchable.) The added test doesn't check for leaking resources, so it passed even before this commit. But at least it covers the code path. In the passing, fix misleading comment on what the 'nevents' argument to WaitEventSetWait means. Report by Alexander Lakhin, analysis and suggestion for the fix by Tom Lane. Fixes bug #17828. Backpatch to v14 where async execution was introduced, but master gets a different fix. Discussion: https://www.postgresql.org/message-id/17828-122da8cba23236be@postgresql.org Discussion: https://www.postgresql.org/message-id/472235.1678387869@sss.pgh.pa.us
* Fix the initial sync tables with no columns.Amit Kapila2023-11-22
| | | | | | | | | | | | The copy command formed for initial sync was using parenthesis for tables with no columns leading to syntax error. This patch avoids adding parenthesis for such tables. Reported-by: Justin G Author: Vignesh C Reviewed-by: Peter Smith, Amit Kapila Backpatch-through: 15 Discussion: http://postgr.es/m/18203-df37fe354b626670@postgresql.org
* Lock table in DROP STATISTICSTomas Vondra2023-11-19
| | | | | | | | | | | | | | | | | | | | | The DROP STATISTICS code failed to properly lock the table, leading to ERROR: tuple concurrently deleted when executed concurrently with ANALYZE. Fixed by modifying RemoveStatisticsById() to acquire the same lock as ANALYZE. This function is called only by DROP STATISTICS, as ANALYZE calls RemoveStatisticsDataById() directly. Reported by Justin Pryzby, fix by me. Backpatch through 12. The code was like this since it was introduced in 10, but older releases are EOL. Reported-by: Justin Pryzby Reviewed-by: Tom Lane Backpatch-through: 12 Discussion: https://postgr.es/m/ZUuk-8CfbYeq6g_u@pryzbyj2023
* Guard against overflow in interval_mul() and interval_div().Dean Rasheed2023-11-18
| | | | | | | | | | | | | | | | | | | | Commits 146604ec43 and a898b409f6 added overflow checks to interval_mul(), but not to interval_div(), which contains almost identical code, and so is susceptible to the same kinds of overflows. In addition, those checks did not catch all possible overflow conditions. Add additional checks to the "cascade down" code in interval_mul(), and copy all the overflow checks over to the corresponding code in interval_div(), so that they both generate "interval out of range" errors, rather than returning bogus results. Given that these errors are relatively easy to hit, back-patch to all supported branches. Per bug #18200 from Alexander Lakhin, and subsequent investigation. Discussion: https://postgr.es/m/18200-5ea288c7b2d504b1%40postgresql.org
* llvmjit: Use explicit LLVMContextRef for inliningDaniel Gustafsson2023-11-17
| | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | When performing inlining LLVM unfortunately "leaks" types (the types survive and are usable, but a new round of inlining will recreate new structurally equivalent types). This accumulation will over time amount to a memory leak which for some queries can be large enough to trigger the OOM process killer. To avoid accumulation of types, all IR related data is stored in an LLVMContextRef which is dropped and recreated in order to release all types. Dropping and recreating incurs overhead, so it will be done only after 100 queries. This is a heuristic which might be revisited, but until we can get the size of the context from LLVM we are flying a bit blind. This issue has been reported several times, there may be more references to it in the archives on top of the threads linked below. This is a backpatch of 9dce22033d5 to all supported branches. Reported-By: Justin Pryzby <pryzby@telsasoft.com> Reported-By: Kurt Roeckx <kurt@roeckx.be> Reported-By: Jaime Casanova <jcasanov@systemguards.com.ec> Reported-By: Lauri Laanmets <pcspets@gmail.com> Author: Andres Freund and Daniel Gustafsson Discussion: https://postgr.es/m/7acc8678-df5f-4923-9cf6-e843131ae89d@www.fastmail.com Discussion: https://postgr.es/m/20201218235607.GC30237@telsasoft.com Discussion: https://postgr.es/m/CAPH-tTxLf44s3CvUUtQpkDr1D8Hxqc2NGDzGXS1ODsfiJ6WSqA@mail.gmail.com Backpatch-through: v12
* Ensure we preprocess expressions before checking their volatility.Tom Lane2023-11-16
| | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | contain_mutable_functions and contain_volatile_functions give reliable answers only after expression preprocessing (specifically eval_const_expressions). Some places understand this, but some did not get the memo --- which is not entirely their fault, because the problem is documented only in places far away from those functions. Introduce wrapper functions that allow doing the right thing easily, and add commentary in hopes of preventing future mistakes from copy-and-paste of code that's only conditionally safe. Two actual bugs of this ilk are fixed here. We failed to preprocess column GENERATED expressions before checking mutability, so that the code could fail to detect the use of a volatile function default-argument expression, or it could reject a polymorphic function that is actually immutable on the datatype of interest. Likewise, column DEFAULT expressions weren't preprocessed before determining if it's safe to apply the attmissingval mechanism. A false negative would just result in an unnecessary table rewrite, but a false positive could allow the attmissingval mechanism to be used in a case where it should not be, resulting in unexpected initial values in a new column. In passing, re-order the steps in ComputePartitionAttrs so that its checks for invalid column references are done before applying expression_planner, rather than after. The previous coding would not complain if a partition expression contains a disallowed column reference that gets optimized away by constant folding, which seems to me to be a behavior we do not want. Per bug #18097 from Jim Keener. Back-patch to all supported versions. Discussion: https://postgr.es/m/18097-ebb179674f22932f@postgresql.org
* Don't release index root page pin in ginFindParents().Tom Lane2023-11-13
| | | | | | | | | | | | | | | | | | | | | It's clearly stated in the comments that ginFindParents() must keep the pin on the index's root page that's associated with the topmost GinBtreeStack item. However, the code path for the case that the desired downlink has been pushed down to the next index level ignored this proviso, and would release the pin anyway if we were still examining the root level. That led to an assertion failure or "buffer NNNN is not owned by resource owner" error later, when we try to release the pin again at the end of the insertion. This is quite hard to reproduce, since it can only happen if an index root page split occurs concurrently with our own insertion. Thanks to Jeff Janes for finding a test case that triggers it often enough to allow investigation. This has been there since the beginning of GIN, so back-patch to all supported branches. Discussion: https://postgr.es/m/CAMkU=1yCAKtv86dMrD__Ja-7KzjE=uMeKX8y__cx5W-OEWy2ow@mail.gmail.com
generated by cgit v1.2.3 (git 2.25.1) at 2025-08-05 07:18:28 +0000