Sync 14x b13 tests#2694
Open
dimoffon wants to merge 584 commits into
Open
Conversation
Author: Daniel Gustafsson <daniel@yesql.se> Discussion: https://postgr.es/m/A05874AE-8771-4C61-A24E-0B6249B8F3C2@yesql.se
Commit 028350f changed its behavior from "at most" to "exactly", but forgot to update the documentation. Backpatch to 9.6. Patch by Justin Pryzby, per Yaroslav Schekin's report. Discussion: https://www.postgresql.org/message-id/20201005191922.GE17626%40telsasoft.com
The output for this query changed in commit 4e2477b. Backport to 9.6 like that commit. Patch by Justin Pryzby, per Yaroslav Schekin's report. Discussion: https://www.postgresql.org/message-id/20201005191922.GE17626%40telsasoft.com
ALTER SEQUENCE RESTART was made transactional in commit 3d79013. Backpatch to v10, where that was introduced. Patch by Justin Pryzby, per Yaroslav Schekin's report. Discussion: https://www.postgresql.org/message-id/20201005191922.GE17626%40telsasoft.com
- Misc grammar and punctuation fixes. - Stylistic cleanup: use spaces between function arguments and JSON fields in examples. For example "foo(a,b)" -> "foo(a, b)". Add semicolon after last END in a few PL/pgSQL examples that were missing them. - Make sentence that talked about "..." and ".." operators more clear, by avoiding to end the sentence with "..". That makes it look the same as "..." - Fix syntax description for HAVING: HAVING conditions cannot be repeated Patch by Justin Pryzby, per Yaroslav Schekin's report. Backpatch to all supported versions, to the extent that the patch applies easily. Discussion: https://www.postgresql.org/message-id/20201005191922.GE17626%40telsasoft.com
Since commit 913bbd8, check_sql_fn_retval() can either insert type coercion steps in-line in the Query that produces the SQL function's results, or generate a new top-level Query to perform the coercions, if modifying the Query's output in-place wouldn't be safe. However, it appears that the latter case has never actually worked, because the code tried to inject the new Query back into the query list it was passed ... which is not the list that will be used for later processing when we execute the SQL function "normally" (without inlining it). So we ended up with no coercion happening at run-time, leading to wrong results or crashes depending on the datatypes involved. While the regression tests look like they cover this area well enough, through a huge bit of bad luck all the test cases that exercise the separate-Query path were checking either inline-able cases (which accidentally didn't have the bug) or cases that are no-ops at runtime (e.g., varchar to text), so that the failure to perform the coercion wasn't obvious. The fact that the cases that don't work weren't allowed at all before v13 probably contributed to not noticing the problem sooner, too. To fix, get rid of the separate "flat" list of Query nodes and instead pass the real two-level list that is going to be used later. I chose to make the same change in check_sql_fn_statements(), although that has no actual bug, just so that we don't need that data structure at all. This is an API change, as evidenced by the adjustments needed to callers outside functions.c. That's a bit scary to be doing in a released branch, but so far as I can tell from a quick search, there are no outside callers of these functions (and they are sufficiently specific to our semantics for SQL-language functions that it's not apparent why any extension would need to call them). In any case, v13 already changed the API of check_sql_fn_retval() compared to prior branches. Per report from pinker. Back-patch to v13 where this code came in. Discussion: https://postgr.es/m/1603050466566-0.post@n3.nabble.com
When told to process all databases, clusterdb, reindexdb, and vacuumdb would reconnect by replacing their --maintenance-db parameter with the name of the target database. If that parameter is a connstring (which has been allowed for a long time, though we failed to document that before this patch), we'd lose any other options it might specify, for example SSL or GSS parameters, possibly resulting in failure to connect. Thus, this is the same bug as commit a45bc8a fixed in pg_dump and pg_restore. We can fix it in the same way, by using libpq's rules for handling multiple "dbname" parameters to add the target database name separately. I chose to apply the same refactoring approach as in that patch, with a struct to handle the command line parameters that need to be passed through to connectDatabase. (Maybe someday we can unify the very similar functions here and in pg_dump/pg_restore.) Per Peter Eisentraut's comments on bug #16604. Back-patch to all supported branches. Discussion: https://postgr.es/m/16604-933f4b8791227b15@postgresql.org
Change the attribute 'name' to 'slot_name' in pg_stat_replication_slots view to make it clear and that way we will be consistent with the other places like pg_stat_wal_receiver view where we display the same attribute. In the passing, fix the typo in one of the macros in the related code. Bump the catversion as we have modified the name in the catalog as well. Reported-by: Noriyoshi Shinoda Author: Noriyoshi Shinoda Reviewed-by: Sawada Masahiko and Amit Kapila Discussion: https://postgr.es/m/CA+fd4k5_pPAYRTDrO2PbtTOe0eHQpBvuqmCr8ic39uTNmR49Eg@mail.gmail.com
More precisely, correctly handle the ONLY flag indicating not to recurse. This was implemented in 86f5759 by recursing in trigger.c, but that's the wrong place; use ATSimpleRecursion instead, which behaves properly. However, because legacy inheritance has never recursed in that situation, make sure to do that only for new-style partitioning. I noticed this problem while testing a fix for another bug in the vicinity. This has been wrong all along, so backpatch to 11. Discussion: https://postgr.es/m/20201016235925.GA29829@alvherre.pgsql
80f8eb7 has added to the normalization quick check headers some code generated by PerfectHash.pm that is incompatible with the settings of gitattributes for this repository, as whitespaces followed a set of tabs for the first element of a line in the table. Instead of adding a new exception to gitattributes, rework the format generated so as a right padding with spaces is used instead of a left padding. This keeps the table generated in a readable shape with its set of columns, making unnecessary an update of gitattributes. Reported-by: Peter Eisentraut Author: John Naylor Discussion: https://postgr.es/m/d601b3b5-a3c7-5457-2f84-3d6513d690fc@2ndquadrant.com
After de8feb1, some warnings remained that were only visible when using GCC on Windows. Fix those as well. Note that the ecpg test source files don't use the full pg_config.h, so we can't use pg_funcptr_t there but have to do it the long way.
Commit 8dace66 added #ifdefs for a number of errno symbols because they were not present on Windows. Later, commit 125ad53 added replacement #defines for some of those symbols. So some of the changes from the first commit are made dead code by the second commit and can now be removed. Discussion: https://www.postgresql.org/message-id/flat/6dee8574-b0ad-fc49-9c8c-2edc796f0033@2ndquadrant.com
Theoretically one could go into src/test/thread and build/run this program there. In practice, that hasn't worked since 96bf88d, and probably much longer on some platforms (likely including just the sort of hoary leftovers where this test might be of interest). While it wouldn't be too hard to repair the breakage, the fact that nobody has noticed for two years shows that there is zero usefulness in maintaining this build pathway. Let's get rid of it and decree that thread_test.c is *only* meant to be built/used in configure. Given that decision, it makes sense to put thread_test.c under config/ and get rid of src/test/thread altogether, so that's what I did. In passing, update src/test/README, which had been ignored by some not-so-recent additions of subdirectories. Discussion: https://postgr.es/m/227659.1603041612@sss.pgh.pa.us
Should cause tests to be a bit faster
psql's \connect claims to be able to re-use previous connection parameters, but in fact it only re-uses the database name, user name, host name (and possibly hostaddr, depending on version), and port. This is problematic for assorted use cases. Notably, pg_dump[all] emits "\connect databasename" commands which we would like to have re-use all other parameters. If such a script is loaded in a psql run that initially had "-d connstring" with some non-default parameters, those other parameters would be lost, potentially causing connection failure. (Thus, this is the same kind of bug addressed in commits a45bc8a and 8e5793a, although the details are much different.) To fix, redesign do_connect() so that it pulls out all properties of the old PGconn using PQconninfo(), and then replaces individual properties in that array. In the case where we don't wish to re-use anything, get libpq's default settings using PQconndefaults() and replace entries in that, so that we don't need different code paths for the two cases. This does result in an additional behavioral change for cases where the original connection parameters allowed multiple hosts, say "psql -h host1,host2", and the \connect request allows re-use of the host setting. Because the previous coding relied on PQhost(), it would only permit reconnection to the same host originally selected. Although one can think of scenarios where that's a good thing, there are others where it is not. Moreover, that behavior doesn't seem to meet the principle of least surprise, nor was it documented; nor is it even clear it was intended, since that coding long pre-dates the addition of multi-host support to libpq. Hence, this patch is content to drop it and re-use the host list as given. Per Peter Eisentraut's comments on bug #16604. Back-patch to all supported branches. Discussion: https://postgr.es/m/16604-933f4b8791227b15@postgresql.org
There is a handful of places where we called list_delete_ptr() to remove some element from a List. In many of these places we know, or with very little additional effort know the index of the ListCell that we need to remove. Here we change all of those places to instead either use one of; list_delete_nth_cell(), foreach_delete_current() or list_delete_last(). Each of these saves from having to iterate over the list to search for the element to remove by its pointer value. There are some small performance gains to be had by doing this, but in the general case, none of these lists are likely to be very large, so the lookup was probably never that expensive anyway. However, some of the calls are in fairly hot code paths, e.g process_equivalence(). So any small gains there are useful. Author: Zhijie Hou and David Rowley Discussion: https://postgr.es/m/b3517353ec7c4f87aa560678fbb1034b@G08CNEXMBPEKD05.g08.fujitsu.local
Mark Dilger, reviewed by Peter Geoghegan, Andres Freund, Álvaro Herrera, Michael Paquier, Amul Sul, and by me. Some last-minute cosmetic revisions by me. Discussion: http://postgr.es/m/12ED3DA8-25F0-4B68-937D-D907CFBF08E7@enterprisedb.com
The check for whether to complain about not having an old connection to get parameters from was seriously out of date: it had not been rethought when we invented connstrings, nor when we invented the -reuse-previous option. Replace it with a check that throws an error if reuse-previous is active and we lack an old connection to reuse. While that doesn't move the goalposts very far in terms of easing reconnection after a server crash, at least it's consistent. If the user specifies a connstring plus additional parameters (which is invalid per the documentation), the extra parameters were silently ignored. That seems like it could be really confusing, so let's throw a syntax error instead. Teach the connstring code path to re-use the old connection's password in the same cases as the old-style-syntax code path would, ie if we are reusing parameters and the values of username, host/hostaddr, and port are not being changed. Document this behavior, too, since it was unmentioned before. Also simplify the implementation a bit, giving rise to two new and useful properties: if there's a "password=xxx" in the connstring, we'll use it not ignore it, and by default (i.e., except with --no-password) we will prompt for a password if the re-used password or connstring password doesn't work. The previous code just failed if the re-used password didn't work. Given the paucity of field complaints about these issues, I don't think that they rise to the level of back-patchable bug fixes, and in any case they might represent undesirable behavior changes in minor releases. So no back-patch. Discussion: https://postgr.es/m/235210.1603321144@sss.pgh.pa.us
Mark Dilger, with a couple of stray semicolons removed by me. Discussion: http://postgr.es/m/2A7DA1A8-C4AA-43DF-A985-3CA52F4DC775@enterprisedb.com
If you write the literal 'abc''def' in an EXEC SQL command, that will come out the other end as 'abc'def', triggering a syntax error in the backend. Likewise, "abc""def" is reduced to "abc"def" which is wrong syntax for a quoted identifier. The cause is that the lexer thinks it should emit just one quote mark, whereas what it really should do is keep the string as-is. Add some docs and test cases, too. Although this seems clearly a bug, I fear users wouldn't appreciate changing it in minor releases. Some may well be working around it by applying an extra doubling of affected quotes, as for example sql/dyntest.pgc has been doing. Per investigation of a report from 1250kv, although this isn't exactly what he/she was on about. Discussion: https://postgr.es/m/673825.1603223178@sss.pgh.pa.us
ECPG's PREPARE ... FROM and EXECUTE IMMEDIATE can optionally take the target query as a simple literal, rather than the more usual string-variable reference. This was previously documented as being a C string literal, but that's a lie in one critical respect: you can't write a data double quote as \" in such literals. That's because the lexer is in SQL mode at this point, so it'll parse double-quoted strings as SQL identifiers, within which backslash is not special, so \" ends the literal. I looked into making this work as documented, but getting the lexer to switch behaviors at just the right point is somewhere between very difficult and impossible. It's not really worth the trouble, because these cases are next to useless: if you have a fixed SQL statement to execute or prepare, you might as well write it as a direct EXEC SQL, saving the messiness of converting it into a string literal and gaining the opportunity for compile-time SQL syntax checking. Instead, let's just document (and test) the workaround of writing a double quote as an octal escape (\042) in such cases. There's no code behavioral change here, so in principle this could be back-patched, but it's such a niche case I doubt it's worth the trouble. Per report from 1250kv. Discussion: https://postgr.es/m/673825.1603223178@sss.pgh.pa.us
There's no functional change at all here, but I'm curious to see whether this change successfully shuts up Coverity's warning about a useless strcmp(), which appeared with the previous update. Discussion: http://mm.icann.org/pipermail/tz/2020-October/029370.html
DST law changes in Palestine, with a whopping 120 hours' notice. Also some historical corrections for Palestine.
This replaces the existing binary search with two perfect hash functions for the composition and the decomposition in the backend code, at the cost of slightly-larger binaries there (35kB in libpgcommon_srv.a). Per the measurements done, this improves the speed of the recomposition and decomposition by up to 30~40 times for the NFC and NFKC conversions, while all other operations get at least 40% faster. This is not as "good" as what libicu has, but it closes the gap a lot as per the feedback from Daniel Verite. The decomposition table remains the same, getting used for the binary search in the frontend code, where we care more about the size of the libraries like libpq over performance as this gets involved only in code paths related to the SCRAM authentication. In consequence, note that the perfect hash function for the recomposition needs to use a new inverse lookup array back to to the existing decomposition table. The size of all frontend deliverables remains unchanged, even with --enable-debug, including libpq. Author: John Naylor Reviewed-by: Michael Paquier, Tom Lane Discussion: https://postgr.es/m/CAFBsxsHUuMFCt6-pU+oG-F1==CmEp8wR+O+bRouXWu6i8kXuqA@mail.gmail.com
Thinko in commit 1375422. EvalPlanQualStart() was mistakenly resetting the parent EState's es_result_relations, when it should initialize the field in the child EPQ EState it just created. That was clearly wrong, but it didn't cause any ill effects, because es_result_relations is currently not used after the ExecInit* phase. Author: Amit Langote Discussion: https://www.postgresql.org/message-id/CA%2BHiwqFEuq8AAAmxXsTDVZ1r38cHbfYuiPQx_%3DYyKe2DC-6q4A%40mail.gmail.com
The behavioural change in the -t/--table option happened around 15 years ago and there seems little point in keeping it around. Author: Ian Barwick Discussion: https://www.postgresql.org/message-id/CAB8KJ%3Dh-XALik4M7gv-pX48%3D%2BSPWexfaYwa%2ByTnPwD3DxceXrg%40mail.gmail.com
The order of AuthenticationGSSContinue and AuthenticationSSPI was swapped, based on the other Authentication* protocol messages being listed in subcode order.
The ExplainCloseGroup arguments for incremental sort usage data didn't match the corresponding ExplainOpenGroup. This only matters for XML-format output, which is probably why we'd not noticed. Daniel Gustafsson, per bug #16683 from Frits Jalvingh Discussion: https://postgr.es/m/16683-8005033324ad34e9@postgresql.org
The tests added by commit 866e24d failed on big-endian machines due to lack of attention to endianness considerations. Fix that. While here, improve a few small cosmetic things, such as running it through perltidy. Mark Dilger Discussion: https://postgr.es/m/30B8E99A-2D9C-48D4-A55C-741C9D5F1563@enterprisedb.com
Commit 62e221e added new test which should be adapted for GPDB, since incremental sort is currently disabled. Also add ORCA output.
The c9ae5cb commit in src/backend/storage/ipc/ipc.c added a conditional error to the cancel_before_shmem_exit function, while an earlier commit 1617960 had already added a call to this function for RemoveTempRelationsCallback. If this callback has not even been registered yet (which happens conditionally in the AtEOXact_Namespace function), then an error results in an assertion, for example, in the instr_in_shmem_terminate test. Add a global variable before_shmem_exit_callback_registered and check it when canceling the callback.
Commit ad1c36b added new test, adapt it for GPDB and add its output to ORCA. Use enable_nestloop to achieve plans that better match upstream.
Commit a929e17 added new tests to src/test/regress/sql/partition_prune.sql. Adapt them for GPDB and add them to ORCA.
Commit 9ca7bae incorrectly fixed the compilation of the aocs_compaction.c and appendonly_compaction.c files, which resulted in similar warnings being generated for AO tables in many tests: problem in alloc set TopTransactionContext: detected write past chunk end in block 0x615ef0207560, chunk 0x615ef0207dc8. Remove unnecessary allocation and assignment.
Commit 41efb83 removed unused subplans from various plans, update this GPDB-specific test too.
Commit 540612f added a new row to the DATE_TBL table. That change results in some queries in the expressions test. Fix the output of these queries in the optimizer output file.
Commit 489c9c3 changed the way negative dates are handled, so GPDB-specific test that was relying on this should be updated as well.
Commit bf797a8 changed generated.sql test. This patch changes output for the optimizer.
Commit cd6f479 added new column refobjversion to pg_depend. However, there is a GPDB-specific test that does SELECT * FROM pg_depend. Since it's the only test doing that, and it doesn't actually need the output data, just replace it with count(*) to avoid future problems.
Commit 3c99230 added a new test for stats_ext. Add its output to the optimizer output.
Commit 41efb83 removed unused subplans. Remove in GPDB-specific tests.
Commit 92f8718 in src/backend/executor/nodeSubplan.c added a conditional error message in the ExecInitSubPlan function, which is inappropriate for the GPDB query executor.
Commit 76f412a removed factorial operators, leaving only the factorial() function. Replace in GPDB-specific tests.
…ut (#2676) 1) Commit 2000b6c in src/backend/executor/execMain.c in the InitResultRelInfo function removed the call to the RelationGetPartitionQual function. This prevented the AccessShareLock lock from being acquired on the root partition when working with partitioned tables (in RelationGetPartitionQual -> generate_partition_qual). Remove this from the GPDB-specific tests. 2) Commit 3d351d9 redefined pg_class.reltuples to be -1 before the first VACUUM or ANALYZE. Fix this in the leaf_parts_analyzed function.
1) Commit 4d346de added new tests to src/test/regress/sql/groupingsets.sql. Adapt them for the GPDB and add them to the ORCA. 2) Commit d6c08e2 added a new hash_mem_multiplier GUC and changed work_mem to hash_mem in many places. Explicitly increase hash_mem_multiplier to preserve plans in a couple of tests.
Commit 3d351d9 redefined pg_class.reltuples to be -1 before the first VACUUM or ANALYZE. Adapt the output of GPDB-specific tests previously missed.
…#2673) 1) Commit 9fc2122 in src/backend/commands/indexcmds.c changed the error message in the DefineIndex function. Change this in the GPDB-specific test outputs. 2) Commit 6b2c4e5 in src/backend/partitioning/partbounds.c changed the error position display in the check_new_partition_bound function. Add the error position display in the GPDB-specific test outputs.
The conflict-resolution commit 7b89d86 adopted upstream PG14's removal of the inh_indexes field (index handling for LIKE INCLUDING INDEXES moved to the new expandTableLikeClause), but left the GPDB distribution-key loop in transformDistributedBy() still iterating cxt->inh_indexes, so the backend failed to compile ("CreateStmtContext has no member named inh_indexes"). Restore the field and its NIL initializers, matching the resolution on our PG14 merge branch: inh_indexes is a vestigial empty list (never populated -- cloned indexes are produced later by expandTableLikeClause), so the loop in transformDistributedBy compiles and harmlessly iterates an empty list. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
pgrowlocks_internal() was refactored to scan a local 'rel' variable
(table_beginscan(rel, ...)), but the GPDB HeapTupleSatisfiesUpdate() call --
which takes the Relation as its first argument -- still referenced the older
'mydata->rel', which no longer exists in this function ("mydata undeclared").
Pass the local 'rel'.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…it (sync-14x-b13)
The PG14 top fast-exit in SyncRepWaitForLSN() ("if (!SyncRepRequested() ||
!sync_standbys_defined) return;") breaks the GPDB coordinator standby: the
QD's standby is not configured via synchronous_standby_names (so
sync_standbys_defined is false for it); the IS_QUERY_DISPATCHER block below
decides synchronously by scanning for an active gp_walreceiver, but the fast
exit returns first, so coordinator commits never blocked on the standby
(segwalrep/commit_blocking_on_standby: CREATE committed instead of blocking).
Exempt the QD from the sync_standbys_defined fast path (the second guard
further down already has this exemption). Verified: commit_blocking_on_standby
passes.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…14x-b13) getIndexes() never populates IndxInfo.inddependcollnames/versions (the producer side of PG14's index-collation-version feature was not merged), and IndxInfo is pg_malloc'd (not zeroed), so appendIndexCollationVersion() read uninitialized garbage pointers and crashed (strlen segfault) during --binary-upgrade dumps -- breaking pg_upgrade. NULL-initialize the two fields in getIndexes() and skip the restore when they are NULL. Verified: pg_dump --binary-upgrade and the full pg_upgrade leg pass. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…-14x-b13) Same robustness fixes as on our PG14 merge branch: regenerate certs over leftovers (a stale 0400 ~/.postgresql/postgresql.key made openssl -keyout fail silently, producing a mismatched cert/key -> "key values mismatch"), and run clear_ssl.sh even when pg_regress fails (otherwise the whole cluster is left SSL-enabled with cert-only hba, breaking every later suite). Verified: ssl suite passes. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
PG14's ssl_client_dn()/ssl_issuer_dn() truncate the DN to NAMEDATALEN (64) via be_tls_get_peer_*; the old expected had the full PG13-era DN. Regenerate. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The upstream create_table test "user-defined operator class in partition key" crashed the coordinator on sync-14x-b13 under GPORCA. Root cause: when the partition key uses a custom btree opclass whose support function is an SQL function, RelationBuildPartitionDesc executes that function (via create_range_bounds -> FunctionCall2Coll -> fmgr_sql) to sort/validate the range bounds. When the partition descriptor is built lazily during ORCA's metadata retrieval, this re-enters the planner/executor while the outer ORCA optimization is still active, corrupting ORCA's memory pool and crashing later in CMDTypeInt4GPDB's constructor (the faulting pointer even embeds the partition bound value). The crash also poisons subsequent statements in the session. ORCA's metadata translation does not support non-default partition operator classes anyway, so detect them up front in standard_planner and fall back to the PostgreSQL planner (which is re-entrant and handles the SQL support function safely) before ORCA ever opens the relation. Verified: the exact upstream test sequence now passes under optimizer=on (INSERTs succeed, SELECT returns the rows, no crash), while a normal default-opclass partitioned table still plans with GPORCA. This mirrors the fix already present on claude-merge-2. Co-Authored-By: Claude Opus 4.8 <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
19 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