4711 Commits (eca456edc1caf4e93b5d4480b201cd091acede1d)

Author	SHA1	Message	Date
Peter Eisentraut	24c5b73eb6	meson: Fix sepgsql installation ... The sepgsql.sql file should be installed under share/contrib/, not share/extension/, since it is not an extension. This makes it match what make install does. Discussion: https://www.postgresql.org/message-id/flat/651a5baf-5c45-4a5a-a202-0c8453a4ebf8@eisentraut.org	12 months ago
Tom Lane	2b72fed2dd	Repair incorrect handling of AfterTriggerSharedData.ats_modifiedcols. ... This patch fixes two distinct errors that both ultimately trace to commit 71d60e2aa, which added the ats_modifiedcols field. The more severe error is that ats_modifiedcols wasn't accounted for in afterTriggerAddEvent's scanning loop that looks for a pre-existing duplicate AfterTriggerSharedData. Thus, a new event could be incorrectly matched to an AfterTriggerSharedData that has a different value of ats_modifiedcols, resulting in the wrong tg_updatedcols bitmap getting passed to the trigger whenever it finally gets fired. We'd not noticed because (a) few triggers consult tg_updatedcols, and (b) we had no tests exercising a case where such a trigger was called as an AFTER trigger. In the test case added by this commit, contrib/lo's trigger fails to remove a large object when expected because (without this fix) it thinks the LO OID column hasn't changed. The other problem was introduced by commit ce5aaea8c, which copied the modified-columns bitmap into trigger-related storage. It made a copy for every trigger event, whereas what we really want is to make a new copy only when we make a new AfterTriggerSharedData entry. (We could imagine adding extra logic to reduce the number of bitmap copies still more, but it doesn't look worthwhile at the moment.) In a simple test of an UPDATE of 10000000 rows with a single AFTER trigger, this thinko roughly tripled the amount of memory consumed by the pending-triggers data structures, from 160446744 to 480443440 bytes. Fixing the first problem requires introducing a bms_equal() call into afterTriggerAddEvent's scanning loop, which is slightly annoying from a speed perspective. However, getting rid of the excessive bms_copy() calls from the second problem balances that out; overall speed of trigger operations is the same or slightly better, in my tests. Discussion: https://postgr.es/m/3496294.1737501591@sss.pgh.pa.us Backpatch-through: 13	12 months ago
Thomas Munro	66aaabe7a1	Restore smgrtruncate() prototype in back-branches. ... It's possible that external code is calling smgrtruncate(). Any external callers might like to consider the recent changes to RelationTruncate(), but commit 38c579b0 should not have changed the function prototype in the back-branches, per ABI stability policy. Restore smgrtruncate()'s traditional argument list in the back-branches, but make it a wrapper for a new function smgrtruncate2(). The three callers in core can use smgrtruncate2() directly. In master (18-to-be), smgrtruncate2() is effectively renamed to smgrtruncate(), so this wart is cleaned up. Reviewed-by: Michael Paquier <michael@paquier.xyz> Discussion: https://postgr.es/m/CA%2BhUKG%2BThae6x6%2BjmQiuALQBT2Ae1ChjMh1%3DkMvJ8y_SBJZrvA%40mail.gmail.com	1 year ago
Tom Lane	89962bfef6	postgres_fdw: re-issue cancel requests a few times if necessary. ... Despite the best efforts of commit 0e5c82380, we're still seeing occasional failures of postgres_fdw's query_cancel test in the buildfarm. Investigation suggests that its 100ms timeout is still not enough to reliably ensure that the remote side starts the query before receiving the cancel request --- and if it hasn't, it will just discard the request because it's idle. We discussed allowing a cancel request to kill the next-received query, but that would have wide and perhaps unpleasant side-effects. What seems safer is to make postgres_fdw do what a human user would likely do, which is issue another cancel request if the first one didn't seem to do anything. We'll keep the same overall 30 second grace period before concluding things are broken, but issue additional cancel requests after 1 second, then 2 more seconds, then 4, then 8. (The next one in series is 16 seconds, but we'll hit the 30 second timeout before that.) Having done that, revert the timeout in query_cancel.sql to 10 ms. That will still be enough on most machines, most of the time, for the remote query to start; but now we're intentionally risking the race condition occurring sometimes in the buildfarm, so that the repeat-cancel code path will get some testing. As before, back-patch to v17. We might eventually contemplate back-patching this further, and/or adding similar logic to dblink. But given the lack of field complaints to date, this feels like mostly an exercise in test case stabilization, so v17 is enough. Discussion: https://postgr.es/m/colnv3lzzmc53iu5qoawynr6qq7etn47lmggqr65ddtpjliq5d@glkveb4m6nop	1 year ago
Thomas Munro	0350b876b0	Fix corruption when relation truncation fails. ... RelationTruncate() does three things, while holding an AccessExclusiveLock and preventing checkpoints: 1. Logs the truncation. 2. Drops buffers, even if they're dirty. 3. Truncates some number of files. Step 2 could previously be canceled if it had to wait for I/O, and step 3 could and still can fail in file APIs. All orderings of these operations have data corruption hazards if interrupted, so we can't give up until the whole operation is done. When dirty pages were discarded but the corresponding blocks were left on disk due to ERROR, old page versions could come back from disk, reviving deleted data (see pgsql-bugs #18146 and several like it). When primary and standby were allowed to disagree on relation size, standbys could panic (see pgsql-bugs #18426) or revive data unknown to visibility management on the primary (theorized). Changes: * WAL is now unconditionally flushed first * smgrtruncate() is now called in a critical section, preventing interrupts and causing PANIC on file API failure * smgrtruncate() has a new parameter for existing fork sizes, because it can't call smgrnblocks() itself inside a critical section The changes apply to RelationTruncate(), smgr_redo() and pg_truncate_visibility_map(). That last is also brought up to date with other evolutions of the truncation protocol. The VACUUM FileTruncate() failure mode had been discussed in older reports than the ones referenced below, with independent analysis from many people, but earlier theories on how to fix it were too complicated to back-patch. The more recently invented cancellation bug was diagnosed by Alexander Lakhin. Other corruption scenarios were spotted by me while iterating on this patch and earlier commit 75818b3a. Back-patch to all supported releases. Reviewed-by: Michael Paquier <michael@paquier.xyz> Reviewed-by: Robert Haas <robertmhaas@gmail.com> Reported-by: rootcause000@gmail.com Reported-by: Alexander Lakhin <exclusion@gmail.com> Discussion: https://postgr.es/m/18146-04e908c662113ad5%40postgresql.org Discussion: https://postgr.es/m/18426-2d18da6586f152d6%40postgresql.org	1 year ago
Tomas Vondra	3668c1d506	Detect version mismatch in brin_page_items ... Commit dae761a87e modified brin_page_items() to return the new "empty" flag for each BRIN range. But the new output parameter was added in the middle, which may cause crashes when using the new binary with old function definition. The ideal solution would be to introduce API versioning similar to what pg_stat_statements does, but it's too late for that as PG17 was already released (so we can't introduce a new extension version). We could do something similar in brin_page_items() by checking the number of output columns (and ignoring the new flag), but it doesn't seem very nice. Instead, simply error out and suggest updating the extension to the latest version. pageinspect is a superuser-only extension, and there's not much reason to run an older version. Moreover, there's a precedent for this approach in 691e8b2e18. Reported by Ľuboslav Špilák, investigation and patch by me. Backpatch to 17, same as dae761a87e. Reported-by: Ľuboslav Špilák Reviewed-by: Michael Paquier, Hayato Kuroda, Peter Geoghegan Backpatch-through: 17 Discussion: https://postgr.es/m/VI1PR02MB63331C3D90E2104FD12399D38A5D2@VI1PR02MB6333.eurprd02.prod.outlook.com Discussion: https://postgr.es/m/flat/3385a58f-5484-49d0-b790-9a198a0bf236@vondra.me	1 year ago
Tom Lane	3652de36e4	contrib/earthdistance: Use SQL-standard function bodies. ... The @extschema:name@ feature added by 72a5b1fc8 allows us to make earthdistance's references to the cube extension fully search-path-secure, so long as all those references are resolved at extension installation time not runtime. To do that, we must convert earthdistance's SQL functions to the new SQL-standard style; but we wanted to do that anyway. The functions can be updated in our customary style by running CREATE OR REPLACE FUNCTION in an extension update script. However, there's still problems in the "CREATE DOMAIN earth" command: its references to cube functions could be captured by hostile objects in earthdistance's installation schema, if that's not where the cube extension is. Worse, the reference to the cube type itself as the domain's base could be captured, and that's not something we could fix after-the-fact in the update script. What I've done about that is to change the "CREATE DOMAIN earth" command in the base script earthdistance--1.1.sql. Ordinarily, changing a released extension script is forbidden; but I think it's okay here since the results of successful (non-trojaned) script execution will be identical to before. A good deal of care is still needed to make the extension's scripts proof against search-path-based attacks. We have to make sure that all the function and operator invocations have exact argument-type matches, to forestall attacks based on supplying a better match. Fortunately earthdistance isn't very big, so I've just gone through it and inspected each call to be sure of that. The only actual code changes needed were to spell all floating-point constants in the style '-1'::float8, rather than depending on runtime type conversions and/or negations. (I'm not sure that the shortcuts previously used were attackable, but removing run-time effort is a good thing anyway.) I believe that this fixes earthdistance enough that we could mark it trusted and remove the warnings about it that were added by 7eeb1d986; but I've not done that here. The primary reason for dealing with this now is that we've received reports of pg_upgrade failing for databases that use earthdistance functions in contexts like generated columns. That's a consequence of 2af07e2f7 having restricted the search_path used while evaluating such expressions. The only way to fix that is to make the earthdistance functions independent of run-time search_path. This patch is very much nicer than the alternative of attaching "SET search_path" clauses to earthdistance's functions: it is more secure and doesn't create a run-time penalty. Therefore, I've chosen to back-patch this to v16 where @extschema:name@ was added. It won't help unless users update to 16.7 and issue "ALTER EXTENSION earthdistance UPDATE" before upgrading to 17, but at least there's now a way to deal with the problem without manual intervention in the dump/restore process. Tom Lane and Ronan Dunklau Discussion: https://postgr.es/m/3316564.aeNJFYEL58@aivenlaptop Discussion: https://postgr.es/m/6a6439f1-8039-44e2-8fb9-59028f7f2014@mailbox.org	1 year ago
Michael Paquier	dc5f905418	Fix invalidation of local pgstats references for entry reinitialization ... 818119afcc has introduced the "generation" concept in pgstats entries, incremented a counter when a pgstats entry is reinitialized, but it did not count on the fact that backends still holding local references to such entries need to be refreshed if the cache age is outdated. The previous logic only updated local references when an entry was dropped, but it needs also to consider entries that are reinitialized. This matters for replication slot stats (as well as custom pgstats kinds in 18~), where concurrent drops and creates of a slot could cause incorrect stats to be locally referenced. This would lead to an assertion failure at shutdown when writing out the stats file, as the backend holding an outdated local reference would not be able to drop during its shutdown sequence the stats entry that should be dropped, as the last process holding a reference to the stats entry. The checkpointer was then complaining about such an entry late in the shutdown sequence, after the shutdown checkpoint is finished with the control file updated, causing the stats file to not be generated. In non-assert builds, the entry would just be skipped with the stats file written. Note that only logical replication slots use statistics. A test case based on TAP is added to test_decoding, where a persistent connection peeking at a slot's data is kept with concurrent drops and creates of the same slot. This is based on the isolation test case that Anton has sent. As it requires a node shutdown with a check to make sure that the stats file is written with this specific sequence of events, TAP is used instead. Reported-by: Anton A. Melnikov Reviewed-by: Bertrand Drouvot Discussion: https://postgr.es/m/56bf8ff9-dd8c-47b2-872a-748ede82af99@postgrespro.ru Backpatch-through: 15	1 year ago
Álvaro Herrera	b0e572819d	Clean up newlines following left parentheses ... Most came in during the 17 cycle, so backpatch there. Some (particularly reorderbuffer.h) are very old, but backpatching doesn't seem useful. Like commits c9d2977519, c4f113e8fe.	1 year ago
Peter Geoghegan	7af6d13061	Count contrib/bloom index scans in pgstat view. ... Maintain the pg_stat_user_indexes.idx_scan pgstat counter during contrib/Bloom index scans. Oversight in commit 9ee014fc, which added the Bloom index contrib module. Author: Masahiro Ikeda <ikedamsh@oss.nttdata.com> Reviewed-By: Peter Geoghegan <pg@bowt.ie> Discussion: https://postgr.es/m/c48839d881388ee401a01807c686004d@oss.nttdata.com Backpatch: 13- (all supported branches).	1 year ago
Alexander Korotkov	a6fa869cfa	Fix arrays comparison in CompareOpclassOptions() ... The current code calls array_eq() and does not provide FmgrInfo. This commit provides initialization of FmgrInfo and uses C collation as the safe option for text comparison because we don't know anything about the semantics of opclass options. Backpatch to 13, where opclass options were introduced. Reported-by: Nicolas Maus Discussion: https://postgr.es/m/18692-72ea398df3ec6712%40postgresql.org Backpatch-through: 13	1 year ago
Amit Kapila	c4b8a916f8	Stabilize the test added by commit 022564f60c. ... The test was unstable in branches 14 and 15 as we were relying on the number of changes in the table having a toast column to start streaming. On branches >= 16, we have a GUC debug_logical_replication_streaming which can stream each change, so the test was stable in those branches. Change the test to use PREPARE TRANSACTION as that should make the result consistent and test the code changed in 022564f60c. Reported-by: Daniel Gustafsson as per buildfarm Author: Hou Zhijie, Amit Kapila Backpatch-through: 14 Discussion: https://postgr.es/m/8C2F86AA-981E-4803-B14D-E264C0255330@yesql.se	1 year ago
Nathan Bossart	a356d23fd3	Fix Y2038 issues with MyStartTime. ... Several places treat MyStartTime as a "long", which is only 32 bits wide on some platforms. In reality, MyStartTime is a pg_time_t, i.e., a signed 64-bit integer. This will lead to interesting bugs on the aforementioned systems in 2038 when signed 32-bit integers are no longer sufficient to store Unix time (e.g., "pg_ctl start" hanging). To fix, ensure that MyStartTime is handled as a 64-bit value everywhere. (Of course, users will need to ensure that time_t is 64 bits wide on their system, too.) Co-authored-by: Max Johnson Discussion: https://postgr.es/m/CO1PR07MB905262E8AC270FAAACED66008D682%40CO1PR07MB9052.namprd07.prod.outlook.com Backpatch-through: 12	1 year ago
Amit Kapila	9181077590	Fix fetching default toast value during decoding of in-progress transactions. ... During logical decoding of in-progress transactions, we perform the toast table scan while fetching the default toast value for an attribute. We forgot to initialize the flag during this scan to indicate that the system table scan is in progress. We need this flag to ensure that during logical decoding we never directly access the tableam or heap APIs because we check for concurrent aborts only in systable_* APIs. Reported-by: Alexander Lakhin Author: Takeshi Ideriha, Hou Zhijie Reviewed-by: Amit Kapila, Hou Zhijie Backpatch-through: 14 Discussion: https://postgr.es/m/18641-6687273b7f15269d@postgresql.org	1 year ago
Tom Lane	fee8cb9473	Use generateClonedIndexStmt to propagate CREATE INDEX to partitions. ... When instantiating an existing partitioned index for a new child partition, we use generateClonedIndexStmt to build a suitable IndexStmt to pass to DefineIndex. However, when DefineIndex needs to recurse to instantiate a newly created partitioned index on an existing child partition, it was doing copyObject on the given IndexStmt and then applying a bunch of ad-hoc fixups. This has a number of problems, primarily that it implies fresh lookups of referenced objects such as opclasses and collations. Since commit 2af07e2f7 caused DefineIndex to restrict search_path internally, those lookups could fail or deliver different results than the original one. We can avoid those problems and save a few dozen lines of code by using generateClonedIndexStmt in this code path too. Another thing this fixes is incorrect propagation of parent-index comments to child indexes (because the copyObject approach copies the idxcomment field while generateClonedIndexStmt doesn't). I had noticed this in connection with commit c01eb619a, but not run the problem to ground. I'm tempted to back-patch this further than v17, but the only thing it's known to fix in older branches is the comment issue, which is pretty minor and doesn't seem worth the risk of introducing new issues in stable branches. (If anyone does care about that, clearing idxcomment in the copied IndexStmt would be a safer fix.) Per bug #18637 from usamoi. Back-patch to v17 where the search_path change came in. Discussion: https://postgr.es/m/18637-f51e314546e3ba2a@postgresql.org	1 year ago
Michael Paquier	7db9bfc1f7	Add missing query ID reporting in extended query protocol ... This commit adds query ID reports for two code paths when processing extended query protocol messages: - When receiving a bind message, setting it to the first Query retrieved from a cached cache. - When receiving an execute message, setting it to the first PlannedStmt stored in a portal. An advantage of this method is that this is able to cover all the types of portals handled in the extended query protocol, particularly these two when the report done in ExecutorStart() is not enough (neither is an addition in ExecutorRun(), actually, for the second point): - Multiple execute messages, with multiple ExecutorRun(). - Portal with execute/fetch messages, like a query with a RETURNING clause and a fetch size that stores the tuples in a first execute message going though ExecutorStart() and ExecuteRun(), followed by one or more execute messages doing only fetches from the tuplestore created in the first message. This corresponds to the case where execute_is_fetch is set, for example. Note that the query ID reporting done in ExecutorStart() is still necessary, as an EXECUTE requires it. Query ID reporting is optimistic and more calls to pgstat_report_query_id() don't matter as the first report takes priority except if the report is forced. The comment in ExecutorStart() is adjusted to reflect better the reality with the extended query protocol. The test added in pg_stat_statements is a courtesy of Robert Haas. This uses psql's \bind metacommand, hence this part is backpatched down to v16. Reported-by: Kaido Vaikla, Erik Wienhold Author: Sami Imseih Reviewed-by: Jian He, Andrei Lepikhov, Michael Paquier Discussion: https://postgr.es/m/CA+427g8DiW3aZ6pOpVgkPbqK97ouBdf18VLiHFesea2jUk3XoQ@mail.gmail.com Discussion: https://postgr.es/m/CA+TgmoZxtnf_jZ=VqBSyaU8hfUkkwoJCJ6ufy4LGpXaunKrjrg@mail.gmail.com Discussion: https://postgr.es/m/1391613709.939460.1684777418070@office.mailbox.org Backpatch-through: 14	1 year ago
Tom Lane	b9645dca10	Replace usages of xmlXPathCompile() with xmlXPathCtxtCompile(). ... In existing releases of libxml2, xmlXPathCompile can be driven to stack overflow because it fails to protect itself against too-deeply-nested input. While there is an upstream fix as of yesterday, it will take years for that to propagate into all shipping versions. In the meantime, we can protect our own usages basically for free by calling xmlXPathCtxtCompile instead. (The actual bug is that libxml2 keeps its nesting counter in the xmlXPathContext, and its parsing code was willing to just skip counting nesting levels if it didn't have a context. So if we supply a context, all is well. It seems odd actually that it works at all to not supply a context, because this means that XPath parsing does not have access to XML namespace info. Apparently libxml2 never checks namespaces until runtime? Anyway, this seems like good future-proofing even if its only immediate effect is to dodge a bug.) Sadly, this hack only offers protection with libxml2 2.9.11 and newer. Before that there are multiple similar problems, so if you are processing untrusted XML it behooves you to get a newer version. But we have some pretty old libxml2 in the buildfarm, so it seems impractical to add a regression test to verify this fix. Per bug #18617 from Jingzhou Fu. Back-patch to all supported versions. Discussion: https://postgr.es/m/18617-1cee4d2ed1f4e7ae@postgresql.org Discussion: https://gitlab.gnome.org/GNOME/libxml2/-/issues/799	1 year ago
Tom Lane	e3b2c651bc	Remove obsolete comment in pg_stat_statements. ... Commit 76db9cb63 removed the use of multiple nesting counters, but missed one comment describing that arrangement. Back-patch to v17 where 76db9cb63 came in, just to avoid confusion. Julien Rouhaud Discussion: https://postgr.es/m/gfcwh3zjxc2vygltapgo7g6yacdor5s4ynr234b6v2ohhuvt7m@gr42joxalenw	1 year ago
Nathan Bossart	9b3c3c0fc2	Fix contrib/pageinspect's test for sequences. ... I managed to break this test in two different ways in commit 05036a3155. First, the output of the new call to tuple_data_split() on the test sequence is dependent on endianness. This is fixed by setting a special start value for the test sequence that produces the same output regardless of the endianness of the machine. Second, on versions older than v15, the new test case fails under "force_parallel_mode = regress" with the following error: ERROR: cannot access temporary tables during a parallel operation This is because pageinspect's disk-accessing functions are incorrectly marked PARALLEL SAFE on versions older than v15 (see commit aeaaf520f4 for details). This one is fixed by changing the test sequence to be permanent. The only reason it was previously marked temporary was to avoid needing a DROP SEQUENCE command at the end of the test. Unlike some other tests in this file, the use of a permanent sequence here shouldn't result in any test instability like what was fixed by commit e2933a6e11. Reviewed-by: Tom Lane Discussion: https://postgr.es/m/ZuOKOut5hhDlf_bP%40nathan Backpatch-through: 12	1 year ago
Nathan Bossart	6ea7f04b73	Reintroduce support for sequences in pgstattuple and pageinspect. ... Commit 4b82664156 restricted a number of functions provided by contrib modules to only relations that use the "heap" table access method. Sequences always use this table access method, but they do not advertise as such in the pg_class system catalog, so the aforementioned commit also (presumably unintentionally) removed support for sequences from some of these functions. This commit reintroduces said support for sequences to these functions and adds a couple of relevant tests. Co-authored-by: Ayush Vatsa Reviewed-by: Robert Haas, Michael Paquier, Matthias van de Meent Discussion: https://postgr.es/m/CACX%2BKaP3i%2Bi9tdPLjF5JCHVv93xobEdcd_eB%2B638VDvZ3i%3DcQA%40mail.gmail.com Backpatch-through: 12	1 year ago
Tom Lane	8749d850f9	Make postgres_fdw's query_cancel test less flaky. ... This test occasionally shows +WARNING: could not get result of cancel request due to timeout which appears to be because the cancel request is sometimes unluckily sent to the remote session between queries, and then it's ignored. This patch tries to make that less probable in three ways: 1. Use a test query that does not involve remote estimates, so that no EXPLAINs are sent. 2. Make sure that the remote session is ready-to-go (transaction started, SET commands sent) before we start the timer. 3. Increase the statement_timeout to 100ms, to give the local session enough time to plan and issue the query. We might have to go higher than 100ms to make this adequately stable in the buildfarm, but let's see how it goes. Back-patch to v17 where this test was introduced. Jelte Fennema-Nio and Tom Lane Discussion: https://postgr.es/m/578934.1725045685@sss.pgh.pa.us	1 year ago
Masahiko Sawada	dbed2e3662	Fix memory counter update in ReorderBuffer. ... Commit 5bec1d6bc5 changed the memory usage updates of the ReorderBufferTXN to zero all at once by subtracting txn->size, rather than updating it for each change. However, if TOAST reconstruction data remained in the transaction when freeing it, there were cases where it further subtracted the memory counter from zero, resulting in an assertion failure. This change calculates the memory size for each change and updates the memory usage to precisely the amount that has been freed. Backpatch to v17, where this was introducd. Reviewed-by: Amit Kapila, Shlok Kyal Discussion: https://postgr.es/m/CAD21AoAqkNUvicgKPT_dXzNoOwpPkVTg0QPPxEcWmzT0moCJ1g%40mail.gmail.com Backpatch-through: 17	1 year ago
Peter Eisentraut	47b47a5617	Remove incidental md5() function use from test ... To allow test to pass in OpenSSL FIPS mode, similar to 657f5f223e, for a new test that has been added since. Reviewed-by: Tomas Vondra <tomas@vondra.me> Discussion: https://www.postgresql.org/message-id/86763810-70a1-4872-8ba7-1676f788e5a2@eisentraut.org	1 year ago
Masahiko Sawada	fdf218f1d5	Restrict accesses to non-system views and foreign tables during pg_dump. ... When pg_dump retrieves the list of database objects and performs the data dump, there was possibility that objects are replaced with others of the same name, such as views, and access them. This vulnerability could result in code execution with superuser privileges during the pg_dump process. This issue can arise when dumping data of sequences, foreign tables (only 13 or later), or tables registered with a WHERE clause in the extension configuration table. To address this, pg_dump now utilizes the newly introduced restrict_nonsystem_relation_kind GUC parameter to restrict the accesses to non-system views and foreign tables during the dump process. This new GUC parameter is added to back branches too, but these changes do not require cluster recreation. Back-patch to all supported branches. Reviewed-by: Noah Misch Security: CVE-2024-7348 Backpatch-through: 12	1 year ago
Alvaro Herrera	d329a515f4	postgres_fdw: Split out the query_cancel test to its own file ... This allows us to skip it in Cygwin, where it's reportedly flaky because of platform bugs or something. Backpatch to 17, where the test was introduced by commit 2466d6654f. Reported-by: Alexander Lakhin <exclusion@gmail.com> Discussion: https://postgr.es/m/e4d0cb33-6be5-e4d5-ae49-9eac3ff2b005@gmail.com	1 year ago
Etsuro Fujita	935fe79ea1	postgres_fdw: Avoid "cursor can only scan forward" error. ... Commit d844cd75a disallowed rewind in a non-scrollable cursor to resolve anomalies arising from such a cursor operation. However, this failed to take into account the assumption in postgres_fdw that when rescanning a foreign relation, it can rewind the cursor created for scanning the foreign relation without specifying the SCROLL option, regardless of its scrollability, causing this error when it tried to do such a rewind in a non-scrollable cursor. Fix by modifying postgres_fdw to instead recreate the cursor, regardless of its scrollability, when rescanning the foreign relation. (If we had a way to check its scrollability, we could improve this by rewinding it if it is scrollable and recreating it if not, but we do not have it, so this commit modifies it to recreate it in any case.) Per bug #17889 from Eric Cyr. Devrim Gunduz also reported this problem. Back-patch to v15 where that commit enforced the prohibition. Reviewed by Tom Lane. Discussion: https://postgr.es/m/17889-e8c39a251d258dda%40postgresql.org Discussion: https://postgr.es/m/b415ac3255f8352d1ea921cf3b7ba39e0587768a.camel%40gunduz.org	1 year ago
Michael Paquier	38e271d3c2	Propagate query IDs of utility statements in functions ... For utility statements defined within a function, the query tree is copied to a PlannedStmt as utility commands do not require planning. However, the query ID was missing from the information passed down. This leads to plugins relying on the query ID like pg_stat_statements to not be able to track utility statements within function calls. Tests are added to check this behavior, depending on pg_stat_statements.track. This is an old bug. Now, query IDs for utilities are compiled using their parsed trees rather than the query string since v16 (3db72ebcbe), leading to less bloat with utilities, so backpatch down only to this version. Author: Anthonin Bonnefoy Discussion: https://postgr.es/m/CAO6_XqrGp-uwBqi3vBPLuRULKkddjC7R5QZCgsFren=8E+m2Sg@mail.gmail.com Backpatch-through: 16	1 year ago
Masahiko Sawada	068674f4ab	Fix possibility of logical decoding partial transaction changes. ... When creating and initializing a logical slot, the restart_lsn is set to the latest WAL insertion point (or the latest replay point on standbys). Subsequently, WAL records are decoded from that point to find the start point for extracting changes in the DecodingContextFindStartpoint() function. Since the initial restart_lsn could be in the middle of a transaction, the start point must be a consistent point where we won't see the data for partial transactions. Previously, when not building a full snapshot, serialized snapshots were restored, and the SnapBuild jumps to the consistent state even while finding the start point. Consequently, the slot's restart_lsn and confirmed_flush could be set to the middle of a transaction. This could lead to various unexpected consequences. Specifically, there were reports of logical decoding decoding partial transactions, and assertion failures occurred because only subtransactions were decoded without decoding their top-level transaction until decoding the commit record. To resolve this issue, the changes prevent restoring the serialized snapshot and jumping to the consistent state while finding the start point. On v17 and HEAD, a flag indicating whether snapshot restores should be skipped has been added to the SnapBuild struct, and SNAPBUILD_VERSION has been bumpded. On backbranches, the flag is stored in the LogicalDecodingContext instead, preserving on-disk compatibility. Backpatch to all supported versions. Reported-by: Drew Callahan Reviewed-by: Amit Kapila, Hayato Kuroda Discussion: https://postgr.es/m/2444AA15-D21B-4CCE-8052-52C7C2DAFE5C%40amazon.com Backpatch-through: 12	2 years ago
Peter Eisentraut	95b44bb025	Unify some error messages	2 years ago
Etsuro Fujita	8cfbac1492	postgres_fdw: Refuse to send FETCH FIRST WITH TIES to remote servers. ... Previously, when considering LIMIT pushdown, postgres_fdw failed to check whether the query has this clause, which led to pushing false LIMIT clauses, causing incorrect results. This clause has been supported since v13, so we need to do a remote-version check before deciding that it will be safe to push such a clause, but we do not currently have a way to do the check (without accessing the remote server); disable pushing such a clause for now. Oversight in commit 357889eb1. Back-patch to v13, where that commit added the support. Per bug #18467 from Onder Kalaci. Patch by Japin Li, per a suggestion from Tom Lane, with some changes to the comments by me. Review by Onder Kalaci, Alvaro Herrera, and me. Discussion: https://postgr.es/m/18467-7bb89084ff03a08d%40postgresql.org	2 years ago
Heikki Linnakangas	e6cd857726	Make RelationFlushRelation() work without ResourceOwner during abort ... ReorderBufferImmediateInvalidation() executes invalidation messages in an aborted transaction. However, RelationFlushRelation sometimes required a valid resource owner, to temporarily increment the refcount of the relache entry. Commit b8bff07daa worked around that in the main subtransaction abort function, AbortSubTransaction(), but missed this similar case in ReorderBufferImmediateInvalidation(). To fix, introduce a separate function to invalidate a relcache entry. It does the same thing as RelationClearRelation(rebuild==true) does when outside a transaction, but can be called without incrementing the refcount. Add regression test. Before this fix, it failed with: ERROR: ResourceOwnerEnlarge called after release started Reported-by: Alexander Lakhin <exclusion@gmail.com> Discussion: https://www.postgresql.org/message-id/e56be7d9-14b1-664d-0bfc-00ce9772721c@gmail.com	2 years ago
Alexander Korotkov	945ec4c4bc	amcheck: Fixes for right page check during unique constraint check ... * Don't forget to pfree() the right page when it's to be ignored. * Report error on unexpected non-leaf right page even if this page is not to be ignored. This restores the logic which was unintendedly changed in 97e5b0026f. Reported-by: Pavel Borisov	2 years ago
Alexander Korotkov	97e5b0026f	amcheck: Report an error when the next page to a leaf is not a leaf ... This is a very unlikely condition during checking a B-tree unique constraint, meaning that the index structure is violated badly, and we shouldn't continue checking to avoid endless loops, etc. So it's worth immediately throwing an error. Reported-by: Peter Geoghegan Discussion: https://postgr.es/m/CAH2-Wzk%2B2116uOXdOViA27SHcr31WKPgmjsxXLBs_aTxAeThzg%40mail.gmail.com Author: Pavel Borisov	2 years ago
Alexander Korotkov	0b5c161248	amcheck: Don't load the right sibling page into BtreeCheckState ... 5ae2087202 implemented a cross-page unique constraint check by loading the right sibling to the BtreeCheckState.target variable. This is wrong, because bt_target_page_check() shouldn't change the target page. Also, BtreeCheckState.target shouldn't be changed alone without BtreeCheckState.targetblock. However, the above didn't cause any visible bugs for the following reasons. 1. We do a cross-page unique constraint check only for leaf index pages. 2. The only way target page get accessed after a cross-page unique constraint check is loading of the lowkey. 3. The only place lowkey is used is bt_child_highkey_check(), and that applies only to non-leafs. The reasons above don't diminish the fact that changing BtreeCheckState.target for a cross-page unique constraint check is wrong. This commit changes this check to temporarily store the right sibling to the local variable. Reported-by: Peter Geoghegan Discussion: https://postgr.es/m/CAH2-Wzk%2B2116uOXdOViA27SHcr31WKPgmjsxXLBs_aTxAeThzg%40mail.gmail.com Author: Pavel Borisov	2 years ago
Alexander Korotkov	532d94fec3	amcheck: Refactoring the storage of the last visible entry ... This commit introduces a new data structure BtreeLastVisibleEntry comprising information about the last visible heap entry with the current value of key. Usage of this data structure allows us to avoid passing all this information as individual function arguments. Reported-by: Alexander Korotkov Discussion: https://www.postgresql.org/message-id/CAPpHfdsVbB9ToriaB1UHuOKwjKxiZmTFQcEF%3DjuzzC_nby31uA%40mail.gmail.com Author: Pavel Borisov, Alexander Korotkov	2 years ago
Robert Haas	12933dc604	Re-allow planner to use Merge Append to efficiently implement UNION. ... This reverts commit 7204f35919, thus restoring 66c0185a3 (Allow planner to use Merge Append to efficiently implement UNION) as well as the follow-on commits d5d2205c8, 3b1a7eb28, 7487044d6. Per further discussion on pgsql-release, we wish to ship beta1 with this feature, and patch the bug that was found just before wrap, rather than shipping beta1 with the feature reverted.	2 years ago
Tom Lane	7204f35919	Revert commit 66c0185a3 and follow-on patches. ... This reverts 66c0185a3 (Allow planner to use Merge Append to efficiently implement UNION) as well as the follow-on commits d5d2205c8, 3b1a7eb28, 7487044d6. In addition to those, 07746a8ef had to be removed then re-applied in a different place, because 66c0185a3 moved the relevant code. The reason for this last-minute thrashing is that depesz found a case in which the patched code creates a completely wrong plan that silently gives incorrect query results. It's unclear what the cause is or how many cases are affected, but with beta1 wrap staring us in the face, there's no time for closer investigation. After we figure that out, we can decide whether to un-revert this for beta2 or hold it for v18. Discussion: https://postgr.es/m/Zktzf926vslR35Fv@depesz.com (also some private discussion among pgsql-release)	2 years ago
Peter Eisentraut	17974ec259	Revise GUC names quoting in messages again ... After further review, we want to move in the direction of always quoting GUC names in error messages, rather than the previous (PG16) wildly mixed practice or the intermittent (mid-PG17) idea of doing this depending on how possibly confusing the GUC name is. This commit applies appropriate quotes to (almost?) all mentions of GUC names in error messages. It partially supersedes a243569bf6 and 8d9978a717, which had moved things a bit in the opposite direction but which then were abandoned in a partial state. Author: Peter Smith <smithpb2250@gmail.com> Discussion: https://www.postgresql.org/message-id/flat/CAHut%2BPv-kSN8SkxSdoHano_wPubqcg5789ejhCDZAcLFceBR-w%40mail.gmail.com	2 years ago
Michael Paquier	110eb4aefb	Remove enum WaitEventExtension ... This enum was used to determine the first ID to use when assigning a custom wait event for extensions, which is always 1. It was kept so as it would be possible to add new in-core wait events in the category "Extension". There is no such thing currently, so let's remove this enum until a case justifying it pops up. This makes the code simpler and easier to understand. This has as effect to switch back autoprewarm.c to use PG_WAIT_EXTENSION rather than WAIT_EVENT_EXTENSION, on par with v16 and older stable branches. Thinko in c9af054653. Reported-by: Peter Eisentraut Discussion: https://postgr.es/m/195c6c45-abce-4331-be6a-e87724e1d060@eisentraut.org	2 years ago
Peter Eisentraut	8aee330af5	Revert temporal primary keys and foreign keys ... This feature set did not handle empty ranges correctly, and it's now too late for PostgreSQL 17 to fix it. The following commits are reverted: 6db4598fcb Add stratnum GiST support function 46a0cd4cef Add temporal PRIMARY KEY and UNIQUE constraints 86232a49a4 Fix comment on gist_stratnum_btree 030e10ff1a Rename pg_constraint.conwithoutoverlaps to conperiod a88c800deb Use daterange and YMD in without_overlaps tests instead of tsrange. 5577a71fb0 Use half-open interval notation in without_overlaps tests 34768ee361 Add temporal FOREIGN KEY contraints 482e108cd3 Add test for REPLICA IDENTITY with a temporal key c3db1f30cb doc: clarify PERIOD and WITHOUT OVERLAPS in CREATE TABLE 144c2ce0cc Fix ON CONFLICT DO NOTHING/UPDATE for temporal indexes Discussion: https://www.postgresql.org/message-id/d0b64a7a-dfe4-4b84-a906-c7dedfa40a3e@eisentraut.org	2 years ago
Tom Lane	da256a4a7f	Pre-beta mechanical code beautification. ... Run pgindent, pgperltidy, and reformat-dat-files. The pgindent part of this is pretty small, consisting mainly of fixing up self-inflicted formatting damage from patches that hadn't bothered to add their new typedefs to typedefs.list. In order to keep it from making anything worse, I manually added a dozen or so typedefs that appeared in the existing typedefs.list but not in the buildfarm's list. Perhaps we should formalize that, or better find a way to get those typedefs into the automatic list. pgperltidy is as opinionated as always, and reformat-dat-files too.	2 years ago
Alvaro Herrera	6f8bb7c1e9	Revert structural changes to not-null constraints ... There are some problems with the new way to handle these constraints that were detected at the last minute, and require fixes that appear too invasive to be doing this late in the cycle. Revert this (again) for now, we'll try again with these problems fixed. The following commits are reverted: b0e96f3119 Catalog not-null constraints 9b581c5341 Disallow changing NO INHERIT status of a not-null constraint d0ec2ddbe0 Fix not-null constraint test ac22a9545c Move privilege check to the right place b0f7dd915b Check stack depth in new recursive functions 3af7217942 Update information_schema definition for not-null constraints c3709100be Fix propagating attnotnull in multiple inheritance d9f686a72e Fix restore of not-null constraints with inheritance d72d32f52d Don't try to assign smart names to constraints 0cd711271d Better handle indirect constraint drops 13daa33fa5 Disallow NO INHERIT not-null constraints on partitioned tables d45597f72f Disallow direct change of NO INHERIT of not-null constraints 21ac38f498 Fix inconsistencies in error messages Discussion: https://postgr.es/m/202405110940.joxlqcx4dogd@alvherre.pgsql	2 years ago
Thomas Munro	cff4e5a36b	Skip citext_utf8 test on Windows. ... On other Windows build farm animals it is already skipped because they don't use UTF-8 encoding. On "hamerkop", UTF-8 is used, and then the test fails. It is not clear to me (a non-Windows person looking only at buildfarm evidence) whether Windows is less sophisticated than other OSes and doesn't know how to downcase Turkish İ with the standard Unicode database, or if it is more sophisticated than other systems and uses locale-specific behavior like ICU does. Whichever the reason, the result is the same: we need to skip the test on Windows, just as we already do for ICU, at least until a Windows-savvy developer comes up with a better idea. The technique for detecting the OS is borrowed from collate.windows.win1252.sql. This was anticipated by commit c2e8bd27, but the problem only surfaced when Windows build farm animals started using Meson. Reviewed-by: Tom Lane <tgl@sss.pgh.pa.us> Discussion: https://postgr.es/m/CA%2BhUKGJ1LeC3aE2qQYTK95rFVON3ZVoTQpTKJqxkHdtEyawH4A%40mail.gmail.com	2 years ago
David Rowley	a42fc1c903	Fix an assortment of typos ... Author: Alexander Lakhin Discussion: https://postgr.es/m/ae9f2fcb-4b24-5bb0-4240-efbbbd944ca1@gmail.com	2 years ago
Tom Lane	a3021aafce	Make postgres_fdw request remote time zone 'GMT' not 'UTC'. ... This should have the same results for all practical purposes. The advantage of selecting 'GMT' is that it's guaranteed to work even when the remote system's timezone database is missing entries, because pg_tzset() hard-wires handling of that, at least in 9.2 and later. (It seems like it would be a good idea to similarly hard-wire correct handling of 'UTC', but that'll be a little more invasive than I want to consider back-patching. Leave that for another day when we're not in feature freeze.) Per trouble report from Adnan Dautovic. Back-patch to all supported branches. Discussion: https://postgr.es/m/465248.1712211585@sss.pgh.pa.us	2 years ago
Daniel Gustafsson	950d4a2cb1	Fix typos and duplicate words ... This fixes various typos, duplicated words, and tiny bits of whitespace mainly in code comments but also in docs. Author: Daniel Gustafsson <daniel@yesql.se> Author: Heikki Linnakangas <hlinnaka@iki.fi> Author: Alexander Lakhin <exclusion@gmail.com> Author: David Rowley <dgrowleyml@gmail.com> Author: Nazir Bilal Yavuz <byavuz81@gmail.com> Discussion: https://postgr.es/m/3F577953-A29E-4722-98AD-2DA9EFF2CBB8@yesql.se	2 years ago
Tomas Vondra	0bd4b0689b	Stabilize test of BRIN parallel create ... As explained in 4d916dd876, the test instability is caused by delayed cleanup of deleted rows. This commit removes the DELETE, stabilizing the test without accidentally disabling parallel builds. The intent of the delete however was to produce empty ranges, and test that the parallel index build populates those correctly. But there's another way to create empty ranges - partial indexes, which does not rely on cleanup of deleted rows. Idea to use partial indexes by Matthias van de Meent, patch by me. Discussion: https://postgr.es/m/95d9cd43-5a92-407c-b7e4-54cd303630fe%40enterprisedb.com	2 years ago
Tomas Vondra	a89cd7bfcd	Revert "Stabilize test of BRIN parallel create" ... This reverts commit 4d916dd876. The goal of that commit was to stabilize a test of parallel BRIN build, but using a TEMPORARY table disables parallel index builds on that table, making the test useless. Discussion: https://postgr.es/m/95d9cd43-5a92-407c-b7e4-54cd303630fe%40enterprisedb.com	2 years ago
Tomas Vondra	4d916dd876	Stabilize test of BRIN parallel create ... The test for parallel create of BRIN indexes added by commit 8225c2fd40 happens to be unstable - a background transaction (e.g. auto-analyze) may hold back global xmin for the initial VACUUM / CREATE INDEX. If the cleanup happens before the next CREATE INDEX, the indexes will not be exactly the same. This is the same issue as e2933a6e11, so fix it the same way by making the table TEMPORARY, which uses an up-to-date cutoff xmin that is not held back by other processes. Reported by Alexander Lakhin, who also suggested the fix. Author: Alexander Lakhin Discussion: https://postgr.es/m/b58901cd-a7cc-29c6-e2b1-e3d7317c3c69@gmail.com Discussion: https://postgr.es/m/2892135.1668976646@sss.pgh.pa.us	2 years ago
Tomas Vondra	8225c2fd40	Add regression test for BRIN parallel builds ... Adds a regression test for parallel CREATE INDEX for BRIN indexes, to improve coverage for BRIN code, particularly code to allow parallel index builds introduced by b437571714. The test is added to pageinspect, as that allows comparing the index to one built without parallelism. Another option would be to just build the index with parallelism and then check it produces correct results. But checking the index is exactly as if built without parallelism makes these query checks unnecessary. Discussion: https://postgr.es/m/1df00a66-db5a-4e66-809a-99b386a06d86%40enterprisedb.com	2 years ago