[test] Renew merge#1738
Open
chenjinbao1989 wants to merge 5780 commits into
Open
Conversation
When using a pipeline, a transaction starts from the first command and is committed with a Sync message or when the pipeline ends. Functions like IsInTransactionBlock() or PreventInTransactionBlock() were already able to understand a pipeline as being in a transaction block, but it was not the case of CheckTransactionBlock(). This function is called for example to generate a WARNING for SET LOCAL, complaining that it is used outside of a transaction block. The current state of the code caused multiple problems, like: - SET LOCAL executed at any stage of a pipeline issued a WARNING, even if the command was at least second in line where the pipeline is in a transaction state. - LOCK TABLE failed when invoked at any step of a pipeline, even if it should be able to work within a transaction block. The pipeline protocol assumes that the first command of a pipeline is not part of a transaction block, and that any follow-up commands is considered as within a transaction block. This commit changes the backend so as an implicit transaction block is started each time the first Execute message of a pipeline has finished processing, with this implicit transaction block ended once a sync is processed. The checks based on XACT_FLAGS_PIPELINING in the routines checking if we are in a transaction block are not necessary: it is enough to rely on the existing ones. Some tests are added to pgbench, that can be backpatched down to v17 when \syncpipeline is involved and down to v14 where \startpipeline and \endpipeline are available. This is unfortunately limited regarding the error patterns that can be checked, but it provides coverage for various pipeline combinations to check if these succeed or fail. These tests are able to capture the case of SET LOCAL's WARNING. The author has proposed a different feature to improve the coverage by adding similar meta-commands to psql where error messages could be checked, something more useful for the cases where commands cannot be used in transaction blocks, like REINDEX CONCURRENTLY or VACUUM. This is considered as future work for v18~. Author: Anthonin Bonnefoy Reviewed-by: Jelte Fennema-Nio, Michael Paquier Discussion: https://postgr.es/m/CAO6_XqrWO8uNBQrSu5r6jh+vTGi5Oiyk4y8yXDORdE2jbzw8xw@mail.gmail.com Backpatch-through: 13
Commit 9044fc1d45a added some files from upstream LLVM. These files have different whitespace rules, which make the git whitespace checks powered by gitattributes fail. To fix, add those files to the exclude list.
…ing. During pgbench's table initialization, progress updates could display leftover characters from the previous message if the new message was shorter. This commit resolves the issue by appending spaces to the current message to fully overwrite any remaining characters from the previous line. Back-patch to all the supported versions. Author: Yushi Ogiwara, Tatsuo Ishii, Fujii Masao Reviewed-by: Tatsuo Ishii, Fujii Masao Discussion: https://postgr.es/m/9a9b8b95b6a709877ae48ad5b0c59bb9@oss.nttdata.com
1. The error reporting of "port setrequested list-of-packages..." changed, hiding errors we were relying on to know if all packages in our list were already installed. Use a loop instead. 2. The cached MacPorts installation was shared between PostgreSQL major branches, though each branch wanted different packages. Add the list of packages to cache key, so that different branches, when tested in one github account/repo such as postgres/postgres, stop fighting with each other, adding and removing packages. Back-patch to 15 where CI began. Author: Thomas Munro <thomas.munro@gmail.com> Author: Nazir Bilal Yavuz <byavuz81@gmail.com> Suggested-by: Andres Freund <andres@anarazel.de> Discussion: https://postgr.es/m/au2uqfuy2nf43nwy2txmc5t2emhwij7kzupygto3d2ffgtrdgr%40ckvrlwyflnh2
This reverts commit d77f91214fb7 on all stable branches, due to concerns regarding the compatility side effects this could create in a minor release. The change still exists on HEAD. Discussion: https://postgr.es/m/CA+TgmoZqRgeFTg4+Yf_CMRRXiHuNz1u6ZC4FvVk+rxw0RmOPnw@mail.gmail.com Backpatch-through: 13
Commit 9044fc1d45a0 skipped SectionMemoryManager.h in headerscheck, and by extension also cpluspluscheck, because it's C++ and would fail both tests. That worked in master and REL_17_STABLE due to 7b8e2ae2fd3b, but older branches have a separate cpluspluscheck script. We need to copy the filtering rule into there too. This problem was being reported by CI's CompilerWarnings task in the 15 and 16 branches, but was a victim of alert fatigue syndrome (unrelated problems in the back-branches). Fix 16, and back-patch to 13, as those are the live branches that have a separate cpluspluscheck script.
If we are pulling up a subquery that's under an outer join, and the subquery's target list contains a strict expression that uses both a subquery variable and a lateral-reference variable, it's okay to pull up the expression without wrapping it in a PlaceHolderVar. That's safe because if the subquery variable is forced to NULL by the outer join, the expression result will come out as NULL too, so we don't have to force that outcome by evaluating the expression below the outer join. It'd be correct to wrap in a PHV, but that can lead to very significantly worse plans, since we'd then have to use a nestloop plan to pass down the lateral reference to where the expression will be evaluated. However, when we do that, we should not mark the lateral reference variable as being nulled by the outer join, because it isn't after we pull up the expression in this way. So the marking logic added by cb8e50a4a was incorrect in this detail, leading to "wrong varnullingrels" errors from the consistency-checking logic in setrefs.c. It seems to be sufficient to just not mark lateral references at all in this case. (I have a nagging feeling that more complexity may be needed in cases where there are several levels of outer join, but some attempts to break it with that didn't succeed.) Per report from Bertrand Mamasam. Back-patch to v16, as the previous patch was. Discussion: https://postgr.es/m/CACZ67_UA_EVrqiFXJu9XK50baEpH=ofEPJswa2kFxg6xuSw-ww@mail.gmail.com
As I'd feared, commit 5c9d8636d was still a few bricks shy of a load. We can't just leave pulled-up lateral-reference Vars with no new nullingrels: we have to carefully compute what subset of the to-be-replaced Var's nullingrels apply to them, else we still get "wrong varnullingrels" errors. This is a bit tedious, but it looks like we can use the nullingrel data this patch computes for other purposes, enabling better optimization. We don't want to inject unnecessary plan changes into stable branches though, so leave that idea for a later HEAD-only patch. Patch by me, but thanks to Richard Guo for devising a test case that broke 5c9d8636d, and for preliminary investigation about how to fix it. As before, back-patch to v16. Discussion: https://postgr.es/m/E1tGn4j-0003zi-MP@gemulon.postgresql.org
The loops over cursor argument variables neglected to ever advance "prevvar". The code would accidentally do the right thing anyway when removing the first or second list entry, but if it had to remove the third or later entry then it would also remove all entries between there and the first entry. AFAICS this would only matter for cursors that reference out-of-scope variables, which is a weird Informix compatibility hack; between that and the lack of impact for short lists, it's not so surprising that nobody has complained. Nonetheless it's a pretty obvious bug. It would have been more obvious if these loops used a more standard coding style for chasing the linked lists --- this business with the "prev" pointer sometimes pointing at the current list entry is confusing and overcomplicated. So rather than just add a minimal band-aid, I chose to rewrite the loops in the same style we use elsewhere, where the "prev" pointer is NULL until we are dealing with a non-first entry and we save the "next" pointer at the top of the loop. (Two of the four loops touched here are not actually buggy, but it seems better to make them all look alike.) Coverity discovered this problem, but not until 2b41de4a5 added code to free no-longer-needed arguments structs. With that, the incorrect link updates are possibly touching freed memory, and it complained about that. Nonetheless the list corruption hazard is ancient, so back-patch to all supported branches.
Previously, it set only DELAY_CHKPT_COMPLETE. That was important, because it meant that if the XLOG_SMGR_TRUNCATE record preceded a XLOG_CHECKPOINT_ONLINE record in the WAL, then the truncation would also happen on disk before the XLOG_CHECKPOINT_ONLINE record was written. However, it didn't guarantee that the sync request for the truncation was processed before the XLOG_CHECKPOINT_ONLINE record was written. By setting DELAY_CHKPT_START, we guarantee that if an XLOG_SMGR_TRUNCATE record is written to WAL before the redo pointer of a concurrent checkpoint, the sync request queued by that operation must be processed by that checkpoint, rather than being left for the following one. This is a refinement of commit 412ad7a. Back-patch to all supported releases, like that commit. Author: Robert Haas <robertmhaas@gmail.com> Reported-by: Thomas Munro <thomas.munro@gmail.com> Discussion: https://postgr.es/m/CA%2BhUKG%2B-2rjGZC2kwqr2NMLBcEBp4uf59QT1advbWYF_uc%2B0Aw%40mail.gmail.com
The corresponding read-only server settings have been removed since in PG16. See commit b0f6c43. Author: Pierre Giraud <pierre.giraud@dalibo.com> Discussion: https://www.postgresql.org/message-id/flat/a75a2fb0-f4b3-4c0c-be3d-7a62d266d760%40dalibo.com
These format codes produce or consume strings of digits, so they should be labeled with is_digit = true, but they were not. This has effect in only one place, where is_next_separator() is checked to see if the preceding format code should slurp up all the available digits. Thus, with a format such as '...SSFF3' with remaining input '12345', the 'SS' code would consume all five digits (and then complain about seconds being out of range) when it should eat only two digits. Per report from Nick Davies. This bug goes back to d589f94 where the FFn codes were introduced, so back-patch to v13. Discussion: https://postgr.es/m/AM8PR08MB6356AC979252CFEA78B56678B6312@AM8PR08MB6356.eurprd08.prod.outlook.com
Yoran Heling reported a case where a data type could be dropped while references to its OID remain behind in pg_amproc. This causes getObjectDescription to fail, which blocks dropping the operator family (since our DROP code likes to construct descriptions of everything it's dropping). The proper fix for this requires adding more pg_depend entries. But to allow DROP to go through with already-corrupt catalogs, tweak getObjectDescription to print "???" for the type instead of failing when it processes such an entry. I changed the logic for pg_amop similarly, for consistency, although it is not known that the problem can manifest in pg_amop. Per report from Yoran Heling. Back-patch to all supported branches (although the problem may be unreachable in v13). Discussion: https://postgr.es/m/Z1MVCOh1hprjK5Sf@gmai021
Usually an entry in pg_amop or pg_amproc does not need a dependency on its amoplefttype/amoprighttype/amproclefttype/amprocrighttype types, because there is an indirect dependency via the argument types of its referenced operator or procedure, or via the opclass it belongs to. However, for some support procedures in some index AMs, the argument types of the support procedure might not mention the column data type at all. Also, the amop/amproc entry might be treated as "loose" in the opfamily, in which case it lacks a dependency on any particular opclass; or it might be a cross-type entry having a reference to a datatype that is not its opclass' opcintype. The upshot of all this is that there are cases where a datatype can be dropped while leaving behind amop/amproc entries that mention it, because there is no path in pg_depend showing that those entries depend on that type. Such entries are harmless in normal activity, because they won't get used, but they cause problems for maintenance actions such as dropping the operator family. They also cause pg_dump to produce bogus output. The previous commit put a band-aid on the DROP OPERATOR FAMILY failure, but a real fix is needed. To fix, add pg_depend entries showing that a pg_amop/pg_amproc entry depends on its lefttype/righttype. To avoid bloating pg_depend too much, skip this if the referenced operator or function has that type as an input type. (I did not bother with considering the possible indirect dependency via the opclass' opcintype; at least in the reported case, that wouldn't help anyway.) Probably, the reason this has escaped notice for so long is that add-on datatypes and relevant opclasses/opfamilies are usually packaged as extensions nowadays, so that there's no way to drop a type without dropping the referencing opclasses/opfamilies too. Still, in the absence of pg_depend entries there's nothing that constrains DROP EXTENSION to drop the opfamily entries before the datatype, so it seems possible for a DROP failure to occur anyway. The specific case that was reported doesn't fail in v13, because v13 prefers to attach the support procedure to the opclass not the opfamily. But it's surely possible to construct other edge cases that do fail in v13, so patch that too. Per report from Yoran Heling. Back-patch to all supported branches. Discussion: https://postgr.es/m/Z1MVCOh1hprjK5Sf@gmai021
When short-circuiting WindowAgg node evaluation on the top-level WindowAgg node using quals on monotonic window functions, because the WindowAgg run condition can mean there's no need to evaluate subsequent window function results in the same partition once the run condition becomes false, it was possible that the executor would use stale results from the previous invocation of the window function in some cases. A fix for this was partially done by a5832722, but that commit only fixed the issue for non-top-level WindowAgg nodes. I mistakenly thought that the top-level WindowAgg didn't have this issue, but Jayesh's example case clearly shows that's incorrect. At the time, I also thought that this only affected 32-bit systems as all window functions which then supported run conditions returned BIGINT, however, that's wrong as ExecProject is still called and that could cause evaluation of any other window function belonging to the same WindowAgg node, one of which may return a byref type. The only queries affected by this are WindowAggs with a "Run Condition" which contains at least one window function with a byref result type, such as lead() or lag() on a byref column. The window clause must also contain a PARTITION BY clause (without a PARTITION BY, execution of the WindowAgg stops immediately when the run condition becomes false and there's no risk of using the stale results). Reported-by: Jayesh Dehankar Discussion: https://postgr.es/m/193261e2c4d.3dd3cd7c1842.871636075166132237@zohocorp.com Backpatch-through: 15, where WindowAgg run conditions were added
818119afccd3 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
pgstat_write_statsfile() discards any entries marked as dropped from being written to the stats file at shutdown, and also included an assertion based on the same condition. The intention of the assertion is to track that no pgstats entries should be left around as terminating backends should drop any entries they still hold references on before the stats file is written by the checkpointer, and it not worth taking down the server in this case if there is a bug making that possible. Let's improve the comment of this area to document clearly what's intended. Based on a discussion with Bertrand Drouvot and Anton A. Melnikov. Author: Bertrand Drouvot Discussion: https://postgr.es/m/a13e8cdf-b97a-4ecb-8f42-aaa367974e29@postgrespro.ru Backpatch-through: 15
Our parallel-mode code only works when we are executing a query in full, so ExecutePlan must disable parallel mode when it is asked to do partial execution. The previous logic for this involved passing down a flag (variously named execute_once or run_once) from callers of ExecutorRun or PortalRun. This is overcomplicated, and unsurprisingly some of the callers didn't get it right, since it requires keeping state that not all of them have handy; not to mention that the requirements for it were undocumented. That led to assertion failures in some corner cases. The only state we really need for this is the existing QueryDesc.already_executed flag, so let's just put all the responsibility in ExecutePlan. (It could have been done in ExecutorRun too, leading to a slightly shorter patch -- but if there's ever more than one caller of ExecutePlan, it seems better to have this logic in the subroutine than the callers.) This makes those ExecutorRun/PortalRun parameters unnecessary. In master it seems okay to just remove them, returning the API for those functions to what it was before parallelism. Such an API break is clearly not okay in stable branches, but for them we can just leave the parameters in place after documenting that they do nothing. Per report from Yugo Nagata, who also reviewed and tested this patch. Back-patch to all supported branches. Discussion: https://postgr.es/m/20241206062549.710dc01cf91224809dd6c0e1@sraoss.co.jp
Follow-up commit to a9d58bf. Backpatch down to v16 where this was added in order to keep the code consistent for future backpatches. Author: Tofig Aliev <t.aliev@postgrespro.ru> Reviewed-by: Daniel Gustafsson <daniel@yesql.se> Reviewed-by: Masahiko Sawada <sawada.mshk@gmail.com> Discussion: https://postgr.es/m/bba4313fdde9db46db279f96f3b748b1@postgrespro.ru Backpatch-through: 16
It looks like the example case was once modified to increase the number of rows but the EXPLAIN ANALYZE output wasn't updated to reflect that. Also adjust the text which discusses the index sizes. With the example table size, the bloom index isn't quite 8 times more space efficient than the btree indexes. Discussion: https://postgr.es/m/CAApHDvovx8kQ0=HTt85gFDAwmTJHpCgiSvRmQZ_6u_g-vQYM_w@mail.gmail.com Backpatch-through: 13, all supported versions
This routine documented that "iterations" would use a default value if set to 0 by the caller. However, the iteration should always be set by the caller to a value strictly more than 0, as documented by an assertion. Oversight in b577743, that has made the iteration count of SCRAM configurable. Author: Matheus Alcantara Discussion: https://postgr.es/m/ac858943-4743-44cd-b4ad-08a0c10cbbc8@gmail.com Backpatch-through: 16
The GUC assign and check hooks used "assign_timezone_abbreviations", which was incorrect. Issue noticed while browsing this area of the code, introduced in 0a20ff5. Reviewed-by: Tom Lane Discussion: https://postgr.es/m/Z1eV6Y8yk77GZhZI@paquier.xyz Backpatch-through: 16
Since commit 97550c0711972a9856b5db751539bbaf2f88884c, these failed with "PANIC: proc_exit() called in child process" due to uninitialized or stale MyProcPid. That was reachable if close() failed in ClosePostmasterPorts() or setlocale(category, "C") failed, both unlikely. Back-patch to v13 (all supported versions). Discussion: https://postgr.es/m/20241208034614.45.nmisch@google.com
On failure, the pg_upgrade log files are automatically appended to the test log file, but the information reported was inconsistent. A header, with the log file name, was reported with note(), while the log contents and a footer used print(), making it harder to diagnose failures when these are split into console output and test log file because the pg_upgrade log file path in the header may not be included in the test log file. The output is now consolidated so as the header uses print() rather than note(). An extra note() is added to inform that the contents of a pg_upgrade log file are appended to the test log file. The diffs from the regression test suite and dump files all use print() to show their contents on failure. Author: Joel Jacobson Reviewed-by: Daniel Gustafsson Discussion: https://postgr.es/m/49f7e64a-b9be-4a90-a9fe-210a7740405e@app.fastmail.com Backpatch-through: 15
If an owned sequence is considered interesting, force its owning table to be marked interesting too. This ensures, in particular, that we'll fetch the owning table's column names so we have the data needed for ALTER TABLE ... ADD GENERATED. Previously there were edge cases where pg_dump could get SIGSEGV due to not having filled in the column names. (The known case is where the owning table has been made part of an extension while its identity sequence is not a member; but there may be others.) Also, if it's an identity sequence, force its dumped-components mask to exactly match the owning table: dump definition only if we're dumping the table's definition, dump data only if we're dumping the table's data, etc. This generalizes the code introduced in commit b965f26 that set the sequence's dump mask to NONE if the owning table's mask is NONE. That's insufficient to prevent failures, because for example the table's mask might only request dumping ACLs, which would lead us to still emit ALTER TABLE ADD GENERATED even though we didn't create the table. It seems better to treat an identity sequence as though it were an inseparable aspect of the table, matching the treatment used in the backend's dependency logic. Perhaps this policy needs additional refinement, but let's wait to see some field use-cases before changing it further. While here, add a comment in pg_dump.h warning against writing tests like "if (dobj->dump == DUMP_COMPONENT_NONE)", which was a bug in this case. There is one other example in getPublicationNamespaces, which if it's not a bug is at least remarkably unclear and under-documented. Changing that requires a separate discussion, however. Per report from Artur Zakirov. Back-patch to all supported branches. Discussion: https://postgr.es/m/CAKNkYnwXFBf136=u9UqUxFUVagevLQJ=zGd5BsLhCsatDvQsKQ@mail.gmail.com
The @extschema:name@ feature added by 72a5b1f 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 7eeb1d9; 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
An \if command appearing within a false (not-to-be-executed) \if branch was incorrectly treated the same as \elif. This could allow statements within the inner \if to be executed when they should not be. Also the missing inner \if stack entry would result in an assertion failure (in assert-enabled builds) when the final \endif is reached. Report and patch by Michail Nikolaev. Back-patch to all supported branches. Discussion: https://postgr.es/m/CANtu0oiA1ke=SP6tauhNqkUdv5QFsJtS1p=aOOf_iU+EhyKkjQ@mail.gmail.com
The test failed if you ran the regression tests with TEMP_CONFIG with recovery_min_apply_delay = '500ms'. Fix the race condition by waiting for transaction to be applied in the replica, like in a few other tests. The failing test was introduced in commit cbfbda7841. Backpatch to all supported versions like that commit (except v12, which is no longer supported). Reported-by: Alexander Lakhin Discussion: https://www.postgresql.org/message-id/09e2a70a-a6c2-4b5c-aeae-040a7449c9f2@gmail.com
If a dshash table grows very large (e.g., the dshash table for cumulative statistics when there are millions of tables), resizing it may fail with an error like: ERROR: invalid DSA memory alloc request size 1073741824 To fix, permit dshash resizing to allocate more than 1 GB by providing the DSA_ALLOC_HUGE flag. Reported-by: Andreas Scherbaum Author: Matthias van de Meent Reviewed-by: Cédric Villemain, Michael Paquier, Andres Freund Discussion: https://postgr.es/m/80a12d59-0d5e-4c54-866c-e69cd6536471%40pgug.de Backpatch-through: 13
Bug fixes in C code: - Fix XLOG_FPI_FOR_HINT WAL replay crash: reorder XLogRecHasBlockImage check before XLogReadBufferForRedo to match upstream PG16 (xlog.c) - Fix libpqwalreceiver load_file failure: Cloudberry links libpqwalreceiver directly into the backend, replace load_file() with guarded libpqwalreceiver_PG_init() calls (worker.c, subscriptioncmds.c) - Fix Slab allocator VMEM crash: use gp_malloc/gp_free consistently instead of mixing with malloc/free (slab.c) - Fix postmaster to start archiver process in utility mode (postmaster.c) - Fix xlogrecovery to use correct WAL receiver start condition and restore point pause logic (xlogrecovery.c) Test framework (Cluster.pm): - Add Cloudberry-specific pg_ctl options (gp_role=utility, gp_dbid, gp_contentid, maintenance_mode) for TAP test nodes Recovery TAP test adaptations: - 001: Add shared_buffers, skip unlogged sequence test (SKIP block) - 003: Add shared_buffers, add Cloudberry pg_ctl options to run_log - 009, 012, 023: Skip tests using PREPARE TRANSACTION (not supported in utility mode) - 014: Skip unlogged sequence tests with SKIP block - 016: Increase shared_buffers for Cloudberry - 018: Skip PREPARE TRANSACTION and statement-level trigger tests with SKIP blocks - 019: Add shared_buffers and adjust expected error messages - 020: Restore SKIP block for .ready file cleanup - 021: Skip PREPARE TRANSACTION visibility tests with SKIP block - 024: Use $node->start instead of raw pg_ctl, check WARNING instead of FATAL - 027: Disable (parallel_schedule requires full Cloudberry cluster) - 035: Add Cloudberry pg_ctl options to run_log, skip WAL retention and on-access pruning invalidation checks - 037: Add allow_system_table_mods, skip 2PC interruption test - 039: Skip split-zone WAL magic number test - 101, 103, 138: Migrate from PostgresNode/TestLib to PG16 PostgreSQL::Test::Cluster/Utils API - 102: Fix for PG16 API (backup_fs_cold, wal_level=replica, remove DISTRIBUTED BY) - 138: Enable archive_mode before promotion for PG16 compatibility
035_standby_logical_decoding.pl: 1. WAL retention (Scenario 1): Cloudberry defaults wal_keep_size=320MB and min_wal_size=320MB (vs PG's 0 and 80MB), with wal_segment_size=64MB (vs 16MB). These prevented WAL file removal after slot invalidation. Fix: override wal_keep_size=0 and min_wal_size=128MB in standby config. 2. On-access pruning (Scenario 5): Cloudberry uses BLCKSZ=32KB (vs PG 8KB). With char(2000) tuples (~2032 bytes) and fillfactor=75, the minfree threshold is 8192 bytes. The original 4 UPDATEs only created 5 tuples (~10KB used, ~22KB free), never triggering heap_page_prune_opt(). Fix: use 14 UPDATEs to fill the page past the minfree threshold, and restore upstream tests (make_slot_active, check_pg_recvlogical_stderr, change_hot_standby_feedback_and_wait_for_xmins with invalidated=1). 039_end_of_wal.pl: 3. Split-zone xlp_magic test: with 32KB WAL pages, the split zone leaves only 8 bytes on the current page. Writing a full 24-byte record header spills xl_prev=0xDEADBEEF into the next page, making xlp_magic=0xBEEF instead of the expected 0x0000. Fix: use substr() to only write the bytes that fit on the current page, keeping the next page zeroed.
1. quotamodel.c: Move RequestAddinShmemSpace/RequestNamedLWLockTranche to shmem_request_hook (PG16 requirement). 2. smgr.c: Add file_extend_hook call to smgrzeroextend() so diskquota hardlimit enforcement works with PG16 bulk extend. 3. DDL files: Fix relation_size function - change table alias from 'relstorage' to 'c' to avoid PG16 name resolution conflict, remove relstorage column reference (removed in PG16), and convert from LANGUAGE SQL to LANGUAGE plpgsql to prevent function inlining.
Convert EXPLAIN plans from atmsort simplified format to raw format with proper column headers, row counts, and correct Optimizer text to prevent atmsort GP_IGNORE plan comparison failures.
Cloudberry uses BLCKSZ=32KB (vs PostgreSQL's 8KB). With shared_buffers=1MB, this gives only 32 buffers instead of the 128 buffers PostgreSQL gets. The test relies on buffer eviction to verify relfilenode reuse correctness, and 32 buffers is insufficient for the eviction pattern to work as intended. Increase shared_buffers to 4MB to restore the expected ~128 buffer count.
…ery reconnect 1. passwordcheck: Add expected NOTICE for resource queue default assignment when CREATE USER is executed without specifying a resource queue. 2. isolation2: Add "the database system is not accepting connections" to the retryable error list in connectdb(). After crash recovery, PG16 may return this error with "Hot standby mode is disabled" before the system is fully ready, causing crash recovery tests to fail instead of retrying.
Add exclusion entries in pom.xml for 115 files introduced by the PG16 merge that lack recognized license headers. These are PG upstream files including Cirrus CI configs, LINGUAS translation catalogs, meson build files, generated headers, GEQO algorithm sources, PG tools/scripts, and contrib config/spec files. Uses wildcard patterns where possible (**/po/LINGUAS, src/tools/ci/**, src/tools/pg_bsd_indent/tests/**, src/include/pch/*.h) to keep the exclusion list concise.
Skip only the specific "update to reused relfilenode" verification in 032_relfilenode_reuse.pl. Cloudberry's 32KB page size results in fewer buffers, making the eviction-based check unreliable.
CI environments may not emit the resource queue NOTICE depending on configuration. Add passwordcheck_1.out as alternate expected output so pg_regress can match either variant.
Update expected output to match standard demo cluster with 1 coordinator + 3 primaries: gp_buffercache_summary returns 4 rows and buffer count matches shared_buffers * primary_count.
…able_gp timestamp - pgcrypto setup_fips: change gpstop -air to -afr (fast instead of immediate shutdown) to avoid segments being down during subsequent contrib tests like pg_buffercache - join_gp: add start_matchignore for non-deterministic tenk2 index choice (tenk2_unique2 vs tenk2_hundred) in explain output - pax_storage alter_table_gp: wrap non-deterministic COUNT(DISTINCT ts) in start_ignore/end_ignore to match main repo
The test is unreliable with Cloudberry's 32KB page size: shared_buffers=4MB gives only ~128 buffers, causing pg_prewarm-based eviction to timeout or crash before producing any TAP output. Skip the whole test instead of just one check point.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
20 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #ISSUE_Number
What does this PR do?
Type of Change
Breaking Changes
Test Plan
make installcheckmake -C src/test installcheck-cbdb-parallelImpact
Performance:
User-facing changes:
Dependencies:
Checklist
Additional Context
CI Skip Instructions