Claude merge 2#2612
Draft
dimoffon wants to merge 2165 commits into
Draft
Conversation
Commit 2453ea1 redefined pg_proc.proargtypes to include the types of OUT parameters, for procedures only. While that had some advantages for implementing the SQL-spec behavior of DROP PROCEDURE, it was pretty disastrous from a number of other perspectives. Notably, since the primary key of pg_proc is name + proargtypes, this made it possible to have multiple procedures with identical names + input arguments and differing output argument types. That would make it impossible to call any one of the procedures by writing just NULL (or "?", or any other data-type-free notation) for the output argument(s). The change also seems likely to cause grave confusion for client applications that examine pg_proc and expect the traditional definition of proargtypes. Hence, revert the definition of proargtypes to what it was, and undo a number of complications that had been added to support that. To support the SQL-spec behavior of DROP PROCEDURE, when there are no argmode markers in the command's parameter list, we perform the lookup both ways (that is, matching against both proargtypes and proallargtypes), succeeding if we get just one unique match. In principle this could result in ambiguous-function failures that would not happen when using only one of the two rules. However, overloading of procedure names is thought to be a pretty rare usage, so this shouldn't cause many problems in practice. Postgres-specific code such as pg_dump can defend against any possibility of such failures by being careful to specify argmodes for all procedure arguments. This also fixes a few other bugs in the area of CALL statements with named parameters, and improves the documentation a little. catversion bump forced because the representation of procedures with OUT arguments changes. Discussion: https://postgr.es/m/3742981.1621533210@sss.pgh.pa.us
We've accumulated quite a mix of instances of "an SQL" and "a SQL" in the documents. It would be good to be a bit more consistent with these. The most recent version of the SQL standard I looked at seems to prefer "an SQL". That seems like a good lead to follow, so here we change all instances of "a SQL" to become "an SQL". Most instances correctly use "an SQL" already, so it also makes sense to use the dominant variation in order to minimise churn. Additionally, there were some other abbreviations that needed to be adjusted. FSM, SSPI, SRF and a few others. Also fix some pronounceable, abbreviations to use "a" instead of "an". For example, "a SASL" instead of "an SASL". Here I've only adjusted the documents and error messages. Many others still exist in source code comments. Translator hint comments seem to be the biggest culprit. It currently does not seem worth the churn to change these. Discussion: https://postgr.es/m/CAApHDvpML27UqFXnrYO1MJddsKVMQoiZisPvsAGhKE_tsKXquw%40mail.gmail.com
We have a dozen PQset*() functions. PQresultSetInstanceData() and this were the libpq setter functions having a different word order. Adopt the majority word order. Reviewed by Alvaro Herrera and Robert Haas, though this choice of name was not unanimous. Discussion: https://postgr.es/m/20210605060555.GA216695@rfd.leadboat.com
Resolve the disagreement with nodes/*funcs.c field order in favor of the latter, which is better-aligned with the IndexStmt field order. This field is new in v14. Discussion: https://postgr.es/m/20210611045546.GA573364@rfd.leadboat.com
The list of options provided by the tab completion was outdated for the following commands: - ALTER SUBSCRIPTION - CREATE SUBSCRIPTION - ALTER PUBLICATION - CREATE PUBLICATION Author: Vignesh C Reviewed-by: Bharath Rupireddy Discussion: https://postgr.es/m/CALDaNm18oHDFu6SFCHE=ZbiO153Fx7E-L1MG0YyScbaDV--U+A@mail.gmail.com
The code added to mark replication slots invalid in commit c655077 had the race condition that a slot can be dropped or advanced concurrently with checkpointer trying to invalidate it. Rewrite the code to close those races. The changes to ReplicationSlotAcquire's API added with c655077 are not necessary anymore. To avoid an ABI break in released branches, this commit leaves that unchanged; it'll be changed in a master-only commit separately. Backpatch to 13, where this code first appeared. Reported-by: Andres Freund <andres@anarazel.de> Author: Andres Freund <andres@anarazel.de> Author: Álvaro Herrera <alvherre@alvh.no-ip.org> Discussion: https://postgr.es/m/20210408001037.wfmk6jud36auhfqm@alap3.anarazel.de
Commit b663a41 introduced bulk inserts for FDW, but the handling of tuple slots turned out to be problematic for two reasons. Firstly, the slots were re-created for each individual batch. Secondly, all slots referenced the same tuple descriptor - with reasonably small batches this is not an issue, but with large batches this triggers O(N^2) behavior in the resource owner code. These two issues work against each other - to reduce the number of times a slot has to be created/dropped, larger batches are needed. However, the larger the batch, the more expensive the resource owner gets. For practical batch sizes (100 - 1000) this would not be a big problem, as the benefits (latency savings) greatly exceed the resource owner costs. But for extremely large batches it might be much worse, possibly even losing with non-batching mode. Fixed by initializing tuple slots only once (and reusing them across batches) and by using a new tuple descriptor copy for each slot. Discussion: https://postgr.es/m/ebbbcc7d-4286-8c28-0272-61b4753af761%40enterprisedb.com
Per 96540f8; the awkward API introduced by c655077 is no longer needed. Author: Andres Freund <andres@anarazel.de> Reviewed-by: Álvaro Herrera <alvherre@alvh.no-ip.org> Discussion: https://postgr.es/m/20210408020913.zzprrlvqyvlt5cyy@alap3.anarazel.de
Commit acb7e4e added a new implementation for PQsendQuery so that it works in pipeline mode (by using extended query protocol), but it behaves differently from the 'Q' message (in simple query protocol) used by regular implementation: the new one doesn't close the unnamed portal. Change the new code to have identical behavior to the old. Reported-by: Yura Sokolov <y.sokolov@postgrespro.ru> Author: Álvaro Herrera <alvherre@alvh.no-ip.org> Discussion: https://postgr.es/m/202106072107.d4i55hdscxqj@alvherre.pgsql
apply_handle_tuple_routing(), having detected and reported that the tuple it needed to update didn't exist, tried to update that tuple anyway, leading to a null-pointer dereference. logicalrep_partition_open() failed to ensure that the LogicalRepPartMapEntry it built for a partition was fully independent of that for the partition root, leading to trouble if the root entry was later freed or rebuilt. Meanwhile, on the publisher's side, pgoutput_change() sometimes attempted to apply execute_attr_map_tuple() to a NULL tuple. The first of these was reported by Sergey Bernikov in bug #17055; I found the other two while developing some test cases for this sadly under-tested code. Diagnosis and patch for the first issue by Amit Langote; patches for the others by me; new test cases by me. Back-patch to v13 where this logic came in. Discussion: https://postgr.es/m/17055-9ba800ec8522668b@postgresql.org
We were already reporting it, but only after the parallel workers were finished, which is visibly much later than what happens in a serial build. With this change we report it when the leader starts its own sort phase when participating in the build (the normal case). Now this might happen a little later than when the workers start their sorting phases, but a) communicating the actual phase start from workers is likely to be a hassle, and b) the sort phase start is pretty fuzzy anyway, since sorting per se is actually initiated by tuplesort.c internally earlier than tuplesort_performsort() is called. Backpatch to pg12, where the progress reporting code for CREATE INDEX went in. Reported-by: Tomas Vondra <tomas.vondra@enterprisedb.com> Author: Matthias van de Meent <boekewurm+postgres@gmail.com> Reviewed-by: Greg Nancarrow <gregn4422@gmail.com> Reviewed-by: Álvaro Herrera <alvherre@alvh.no-ip.org> Discussion: https://postgr.es/m/1128176d-1eee-55d4-37ca-e63644422adb
Reported-by: Justin Pryzby
941697c changed ProcArrayAdd()/Remove() substantially. As reported by zhanyi, I missed that due to the introduction of the PGPROC->pgxactoff ProcArrayRemove() does not need to search for the position in procArray->pgprocnos anymore - that's pgxactoff. Remove the search loop. The removal of the search loop reduces assertion coverage a bit - but we can easily do better than before by adding assertions to other loops over pgprocnos elements. Also do a bit of cleanup, mostly by reducing line lengths by introducing local helper variables and adding newlines. Author: zhanyi <w@hidva.com> Author: Andres Freund <andres@anarazel.de> Discussion: https://postgr.es/m/tencent_5624AA3B116B3D1C31CA9744@qq.com
ab55d74 has introduced some tests with rows found as missing in logical replication subscriptions for partitioned tables, relying on a logic with a lookup of the logs generated, scanning the whole file. This commit makes the logic more precise, by scanning the logs only from the position before the key queries are run to the position where we check for the logs. This will reduce the risk of issues with log patterns overlapping with each other if those tests get more complicated in the future. Per discussion with Tom Lane. Discussion: https://postgr.es/m/YMP+Gx2S8meYYHW4@paquier.xyz Backpatch-through: 13
This routine is designed to never return an empty description or NULL, providing description fallbacks even if missing objects are accepted, but it included a code path where this was considered possible. All the callers of this routine already consider NULL as not possible, so change a bit the code to map with the assumptions of the callers, and add more comments close to the callers of this routine to outline the behavior expected. This code is new as of 2a10fdc, so no backpatch is needed. Discussion: https://postgr.es/m/YMNY6RGPBRCeLmFb@paquier.xyz
Commit caba8f0 wasn't quite right for msys, as demonstrated by several buildfarm animals, including jacana and fairywren. We need to use the msys perl in the archive command, but call it in such a way that Windows will understand the path. Furthermore, inside the copy script we need to convert a Windows path to an msys path.
Using an Assert to check the validity of incoming messages is an extremely poor decision. In a debug build, it should not be that easy for a broken or malicious remote client to crash the logrep worker. The consequences could be even worse in non-debug builds, which will fail to make such checks at all, leading to who-knows-what misbehavior. Hence, promote every Assert that could possibly be triggered by wrong or out-of-order replication messages to a full test-and-ereport. To avoid bloating the set of messages the translation team has to cope with, establish a policy that replication protocol violation error reports don't need to be translated. Hence, all the new messages here use errmsg_internal(). A couple of old messages are changed likewise for consistency. Along the way, fix some non-idiomatic or outright wrong uses of hash_search(). Most of these mistakes are new with the "streaming replication" patch (commit 4648243), but a couple go back a long way. Back-patch as appropriate. Discussion: https://postgr.es/m/1719083.1623351052@sss.pgh.pa.us
Previously, a zero value for the relfilenode resulted in a confusing error message about "unexpected duplicate". This function returns NULL for other invalid relfilenode values, so zero should be treated likewise. It's been like this all along, so back-patch to all supported branches. Justin Pryzby Discussion: https://postgr.es/m/20210612023324.GT16435@telsasoft.com
Several TAP tests use poll_query_until() to wait for the postmaster to restart. They were checking to see if a trivial query (e.g. "SELECT 1") succeeds. However, that's problematic in the wake of commit 11e9caf, because now that we feed said query to psql via stdin, we risk IPC::Run whining about a SIGPIPE failure if psql quits before reading the query. Hence, we can't use a nonempty query in cases where we need to wait for connection failures to stop happening. Per the precedent of commits c757a3d and 6d41dd0, we can pass "undef" as the query in such cases to ensure that IPC::Run has nothing to write. However, then we have to say that the expected output is empty, and this exposes a deficiency in poll_query_until: if psql fails altogether and returns empty stdout, poll_query_until will treat that as a success! That's because, contrary to its documentation, it makes no actual check for psql failure, looking neither at the exit status nor at stderr. To fix that, adjust poll_query_until to insist on empty stderr as well as a stdout match. (I experimented with checking exit status instead, but it seems that psql often does exit(1) in cases that we need to consider successes. That might be something to fix someday, but it would be a non-back-patchable behavior change.) Back-patch to v10. The test cases needing this exist only as far back as v11, but it seems wise to keep poll_query_until's behavior the same in v10, in case we back-patch another such test case in future. (9.6 does not currently need this change, because in that branch poll_query_until can't be told to accept empty stdout as a success case.) Per assorted buildfarm failures, mostly on hoverfly. Discussion: https://postgr.es/m/CAA4eK1+zM6L4QSA1XMvXY_qqWwdUmqkOS1+hWvL8QcYEBGA1Uw@mail.gmail.com
This is similar to the work done in 8279f68 for TestLib.pm, where environment variables set may cause unwanted failures if using a temporary installation with pg_regress. The list of variables reset is adjusted in each stable branch depending on what is supported. Comments are added to remember that the lists in TestLib.pm and pg_regress.c had better be kept in sync. Reviewed-by: Álvaro Herrera Discussion: https://postgr.es/m/YMNR9GYDn+fHlMta@paquier.xyz Backpatch-through: 9.6
Translate path slashes on target directory path. This was confusing old branches, but is applied to all branches for the sake of uniformity. Perl is perfectly able to understand paths with forward slashes. Along the way, restore the previous archive_wait query, for the sake of uniformity with other tests, per gripe from Tom Lane.
Recent glibc versions have made mktime() fail if tm_isdst is inconsistent with the prevailing timezone; in particular it fails for tm_isdst = 1 when the zone is UTC. (This seems wildly inconsistent with the POSIX-mandated treatment of "incorrect" values for the other fields of struct tm, so if you ask me it's a bug, but I bet they'll say it's intentional.) This has been observed to cause cosmetic problems when pg_restore'ing an archive created in a different timezone. To fix, do mktime() using the field values from the archive, and if that fails try again with tm_isdst = -1. This will give a result that's off by the UTC-offset difference from the original zone, but that was true before, too. It's not terribly critical since we don't do anything with the result except possibly print it. (Someday we should flush this entire bit of logic and record a standard-format timestamp in the archive instead. That's not okay for a back-patched bug fix, though.) Also, guard our only other use of mktime() by having initdb's build_time_t() set tm_isdst = -1 not 0. This case could only have an issue in zones that are DST year-round; but I think some do exist, or could in future. Per report from Wells Oliver. Back-patch to all supported versions, since any of them might need to run with a newer glibc. Discussion: https://postgr.es/m/CAOC+FBWDhDHO7G-i1_n_hjRzCnUeFO+H-Czi1y10mFhRWpBrew@mail.gmail.com
The extra checks added by the recompression of toast data introduced in bbe0a81 is proving to have a performance impact on VACUUM or CLUSTER even if no recompression is done. This is more noticeable with more toastable columns that contain non-NULL values. Improvements could be done to make those extra checks less expensive, but that's not material for 14 at this stage, and we are not sure either if the code path of VACUUM FULL/CLUSTER is adapted for this job. Per discussion with several people, including Andres Freund, Robert Haas, Álvaro Herrera, Tom Lane and myself. Discussion: https://postgr.es/m/20210527003144.xxqppojoiwurc2iz@alap3.anarazel.de
An object found as dropped when digging into the list of objects returned by pg_event_trigger_ddl_commands() could cause a cache lookup error, as the calls grabbing for the object address and the type name would fail if the object was missing. Those lookup errors could be seen with combinations of ALTER TABLE sub-commands involving identity columns. The lookup logic is changed in this code path to get a behavior similar to any other SQL-callable function by ignoring objects that are not found, taking advantage of 2a10fdc. The back-branches are not changed, as they require this commit that is too invasive for stable branches. While on it, add test cases to exercise event triggers with identity columns, and stress more cases with the event ddl_command_end for relations. Author: Sven Klemm, Aleksander Alekseev, Michael Paquier Discussion: https://postgr.es/m/CAMCrgp2R1cEXU53iYKtW6yVEp2_yKUz+z=3-CTrYpPP+xryRtg@mail.gmail.com
User-defined objects that reference some built-in array functions will need to be recreated in PG 14. Reported-by: Justin Pryzby Discussion: https://postgr.es/m/20210608225618.GR16435@telsasoft.com
Reported-by: Justin Pryzby Discussion: https://postgr.es/m/20210612034551.GU16435@telsasoft.com
Items related to logical replication attribution and BRIN indexes Reported-by: Tomas Vondra, John Naylor Discussion: https://postgr.es/m/0db66294-a668-2caa-2b5e-a8db60b30662@enterprisedb.com, CAFBsxsH21KnteYdk33F1oZu2O726NSD6_XBq51Tn0jytsA1AnA@mail.gmail.com
I overlooked that one condition was logically inverted. The fix is a little bit more involved than simply negating the condition, to make the code easier to read. Fix some outdated comments left by the same commit, while at it. Author: Masahiko Sawada <sawada.mshk@gmail.com> Author: Álvaro Herrera <alvherre@alvh.no-ip.org> Reviewed-by: Amit Kapila <amit.kapila16@gmail.com> Discussion: https://postgr.es/m/YMRlmB3/lZw8YBH+@paquier.xyz
Revert the pg_description entry to its v13 form, since those messages usually remain shorter and don't discuss individual parameters. No catversion bump, since pg_description content does not impair backend compatibility or application compatibility. Justin Pryzby Discussion: https://postgr.es/m/20210612182743.GY16435@telsasoft.com
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
…t_function merge Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
…ffer stubs Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
…andler Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
…clitems - Fix broken sed-escaped simple_prompt calls in scripts/common.c - Replace fe_utils/connect_utils.h include with GPDB-specific declarations in common.h (our connectDatabase has different PG13-era signature) - Fix dumputils.c: raclitems -> revokeitems (variable name mismatch) Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
- Remove PG13 if/else connect block from CloneArchive (PG14 connects above) - Fix createdb.c to use GPDB connectMaintenanceDatabase individual-arg API Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
PG14 scripts use ConnParams struct but GPDB connectMaintenanceDatabase uses individual args. Add ConnParams typedef to common.h and a wrapper function that unpacks ConnParams into individual args. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Some scripts pass ConnParams by pointer (function params), others by value (local vars). Remove & when already a pointer. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
…pers Take cloudberry scripts/common.c and common.h which fully adopt PG14's fe_utils/connect_utils.h ConnParams-based connection API. Revert all connectDatabase_cparams/connectMaintenanceDatabase_cparams wrapper calls back to standard PG14 connectDatabase/connectMaintenanceDatabase. Restore ConnParams usage in createdb.c. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
…arrayelem, fix exec_prepare_plan - Remove PG13 duplicate query/plan members from PLpgSQL_expr - Restore PLpgSQL_arrayelem struct (GPDB-specific, lost in merge) - Remove extra keepplan arg from exec_prepare_plan calls (PG14 removed it) Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
…_single_test args Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
…oesn't have it) Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
…id_le) Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
…eeds uuid_le) Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
…(PG14) PG14 split errstart into errstart (warm) and errstart_cold (cold path for ERROR and above). Update EXPECT_EREPORT mock to expect errstart_cold when LOG_LEVEL >= ERROR. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
PG14 uses errstart_cold for elevel >= ERROR. Update EXPECT_EREPORT macros in all test files to set expectations on errstart_cold instead of errstart when testing ERROR/FATAL paths. Files fixed: dfmgr_test.c, session_state_test.c, postinit_test.c, runaway_cleaner_test.c, gp_replication_test.c Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Added: compute_query_id, enable_resultcache, idle_session_timeout, ssl_crl_dir Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
…ion, vacuum_failsafe Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
…_after_crash, vacuum_multixact_failsafe_age Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
…ways Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
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
17 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.
Here are some reminders before you submit the pull request
make installcheck