Merging community commits from f58f1fb6c0f0990558d0859018b31412b1338447 Add missing GETTEXT_FLAGS entry to 054325c5eeb3140a067ba66735c3d811163ecd6a libpq: Improve idle state handling in pipeline mode #54
Conversation
This file contains 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
(cherry picked from commit f58f1fb6c0f0990558d0859018b31412b1338447)
numGroups is unused since commit b563594; let's get rid of it. XueJing Zhao, reviewed by Richard Guo Discussion: https://postgr.es/m/DM6PR05MB64923CC8B63A2CAF3B2E5D47B7AD9@DM6PR05MB6492.namprd05.prod.outlook.com (cherry picked from commit f172b11d616e4e440be6b3235721e283e4c16460)
Per buildfarm members sungazer and mylodon. Back-patch to v15, which introduced this test. Discussion: https://postgr.es/m/[email protected] (cherry picked from commit 4f4c72c2dc06e944950305b0c1f48071ff49e263)
Now that the more-generic variable exists, use it. Discussion: https://postgr.es/m/[email protected] (cherry picked from commit c99c67fc43f8659517310c62e8a2637d56d0e449)
pg_attribute_nonnull(...) can be used to generate compiler warnings when a function is called with the specified arguments set to NULL, as per an idea from Andres Freund. An empty argument list indicates that no pointer arguments can be NULL. pg_attribute_nonnull() only works for compilers that support the nonnull function attribute. If nonnull is not supported, pg_attribute_nonnull() has no effect. As a beginning, this commit uses it for the DefineCustomXXXVariable() functions to generate warnings when the "name" and "value" arguments are set to NULL. This will likely be expanded to other places in the future, where it makes sense. Author: Nathan Bossart Reviewed by: Michael Paquier, Tom Lane Discussion: https://postgr.es/m/[email protected] (cherry picked from commit 0507977aa4a356052ea0e5da209162e4b2125c1d)
POSIX shm_open() can sleep for a long time and fail spuriously because of contention on an internal lock file on Solaris (and presumably illumos). Commit 389869af fixed the main problem with this, namely that we could crash, but it's now clear that "posix" is not a good default. Therefore, choose "sysv" at initdb time on Solaris and illumos. Other choices are still available by editing the postgresql.conf file. Back-patch only to 15, because contention is much less likely further back, and it doesn't seem like a good idea to change this in released branches. This should clear up the failures on build farm animal margay. Discussion: https://postgr.es/m/CA%2BhUKGKqKrCV5xKWfh9rnm%3Do%3DDwZLTLtnsj_XpUi9g5%3DV%2B9oyg%40mail.gmail.com (cherry picked from commit 94ebf8117c93f19218e60eb24f3f6bd09b796767)
Reformat some comments in node field definitions to avoid long lines. This makes room for per-field annotations in a future patch to generate node support functions automatically. Discussion: https://www.postgresql.org/message-id/[email protected] (cherry picked from commit 835d476fd21bcfb60b055941dee8c3d9559af14c)
Allows extension authors to more easily debug problems related to the sequence of update scripts that are executed. Discussion: https://postgr.es/m/5636a7534a4833884172fe4369d825b26170b3cc.camel%40j-davis.com Reviewed-by: Peter Eisentraut, Nathan Bossart (cherry picked from commit 43470717c47092194832b90737dc74ec6ab9ef33)
ecpglib has been calling it once per SQL query and once per EXEC SQL GET DESCRIPTOR. Instead, if newlocale() has not succeeded before, call it while establishing a connection. This mitigates three problems: - If newlocale() failed in EXEC SQL GET DESCRIPTOR, the command silently proceeded without the intended locale change. - On AIX, each newlocale()+freelocale() cycle leaked memory. - newlocale() CPU usage may have been nontrivial. Fail the connection attempt if newlocale() fails. Rearrange ecpg_do_prologue() to validate the connection before its uselocale(). The sort of program that may regress is one running in an environment where newlocale() fails. If that program establishes connections without running SQL statements, it will stop working in response to this change. I'm betting against the importance of such an ECPG use case. Most SQL execution (any using ECPGdo()) has long required newlocale() success, so there's little a connection could do without newlocale(). Back-patch to v10 (all supported versions). Reviewed by Tom Lane. Reported by Guillaume Lelarge. Discussion: https://postgr.es/m/[email protected] (cherry picked from commit 5633836ef306aa4d2be44821b601494054b479d7)
Per buildfarm member prairiedog, this platform rejects uninitialized global variables in shared libraries. Back-patch to v10, like the addition of the variable. Reviewed by Tom Lane. Discussion: https://postgr.es/m/[email protected] (cherry picked from commit e2bc242833da27cd73c279bebfb321a65384808f)
These are especially useless because the whole point of pg_free() was to do that very check before calling free(). pg_free() could be removed altogether, but I'm keeping it here to keep the API consistent. Discussion: https://www.postgresql.org/message-id/flat/dac5d2d0-98f5-94d9-8e69-46da2413593d%40enterprisedb.com (cherry picked from commit 098c703d308fa88dc9e3f9f623ca023ce4717794)
Per applicable standards, free() with a null pointer is a no-op. Systems that don't observe that are ancient and no longer relevant. Some PostgreSQL code already required this behavior, so this change does not introduce any new requirements, just makes the code more consistent. Discussion: https://www.postgresql.org/message-id/flat/dac5d2d0-98f5-94d9-8e69-46da2413593d%40enterprisedb.com (cherry picked from commit 02c408e21a6e78ff246ea7a1beb4669634fa9c4c)
These functions already had the free()-like behavior of handling null pointers as a no-op. But it wasn't documented, so add it explicitly to the documentation, too. Discussion: https://www.postgresql.org/message-id/flat/dac5d2d0-98f5-94d9-8e69-46da2413593d%40enterprisedb.com (cherry picked from commit 5faef9d582012433db9ad05af27a77bd591508e1)
After commit 662dbe2, psql tab completion didn't conveniently support the case of "ALTER EXTENSION foo UPDATE". It'd always add "TO", which is fine if you want to specify a target version but not if you don't ... and surely the latter is the much more common case. To fix, remove "TO" from the initially offered completion; you now need to press TAB one additional time to get that. We won't try to duplicate the old behavior of attempting initial completion on the target version along with TO. It's too squirrelly to get the quoting right, and this is such an infrequent usage that it doesn't seem worth expending a lot of effort and special code on. Noted by Noah Misch. Back-patch to v15. Discussion: https://postgr.es/m/[email protected] (cherry picked from commit 22a67fdd5d2756860e0e0813e4a1ae11b69e21c7)
Back-patch to v15, the first version to install these programs. Discussion: https://postgr.es/m/[email protected] (cherry picked from commit b6a5158f98fd5158f66943d721061418f183b370)
Interpret its privileges argument as a comma-separated list of privilege names, as in has_table_privilege and other functions. This is actually net less code, since the support routine to parse that already exists, and we can drop convert_priv_string() which had no other use-case. Robins Tharakan Discussion: https://postgr.es/m/[email protected] (cherry picked from commit b762bbde30d21d6a091d44cc2cbbfb1c9550be52)
None of the other bison parsers contains this directive, and it gives rise to some unfortunate and impenetrable messages, so just remove it. Backpatch to release 12, where it was introduced. Per gripe from Erik Rijkers Discussion: https://postgr.es/m/[email protected] (cherry picked from commit 89a39d4a4da01b13dddcbcf9bcdac2205c9b1279)
Should be 8 for int8, not -1. Reviewed-by: Nathan Bossart <[email protected]> Discussion: https://www.postgresql.org/message-id/flat/[email protected] (cherry picked from commit 4e85b97304a74f5f0fc82136b95f0d5a67b7fd53)
It was int4, but in the other replication commands, timelines are returned as int8. Reviewed-by: Nathan Bossart <[email protected]> Discussion: https://www.postgresql.org/message-id/flat/[email protected] (cherry picked from commit ec40f3422412cfdc140b5d3f67db7fd2dac0f1e2)
Amendment to ec40f3422412cfdc140b5d3f67db7fd2dac0f1e2: We also need to change the way the datum is supplied to int8. Otherwise, the value is still cut off as an int4, and it will crash on 32-bit platforms. (cherry picked from commit 8ba3cb2f1863e3243aa40d73633bd88f774f74ce)
Attempting such an operation would already fail, but in various and confusing ways. For example, while in recovery, some elog() messages would be reported, but these should never be user-facing. This commit restricts any write operations done on large objects in a read-only context, so as the errors generated are more user-friendly. This is per the discussion done with Tom Lane and Robert Haas. Some regression tests are added to check the case of all the SQL functions working on large objects (including an update of the test's alternate output). Author: Yugo Nagata Discussion: https://postgr.es/m/[email protected] (cherry picked from commit 55f4802785f66a584c05dca40e5d9b25491674b2)
Use it for RelationSyncEntry->streamed_txns, which is currently using an integer list. The API support is not complete, not because it is hard to write but because it's unclear that it's worth the code space, there being so little use of XID lists. Discussion: https://postgr.es/m/[email protected] Reviewed-by: Amit Kapila <[email protected]> (cherry picked from commit f10a025cfe97c1a341f636368e67af5ca644c5d8)
Some routines open-coded the construction of RowDescription messages. Instead, we have support for doing this using tuple descriptors and DestRemoteSimple, so use that instead. Reviewed-by: Nathan Bossart <[email protected]> Discussion: https://www.postgresql.org/message-id/flat/[email protected] (cherry picked from commit 2ce648f750a91b04bfa371a8f966703a382fcc97)
durable_rename_excl() attempts to avoid overwriting any existing files by using link() and unlink(), and it falls back to rename() on some platforms (aka WIN32), which offers no such overwrite protection. Most callers use durable_rename_excl() just in case there is an existing file, but in practice there shouldn't be one (see below for more details). Furthermore, failures during durable_rename_excl() can result in multiple hard links to the same file. As per Nathan's tests, it is possible to end up with two links to the same file in pg_wal after a crash just before unlink() during WAL recycling. Specifically, the test produced links to the same file for the current WAL file and the next one because the half-recycled WAL file was re-recycled upon restarting, leading to WAL corruption. This change replaces all the calls of durable_rename_excl() to durable_rename(). This removes the protection against accidentally overwriting an existing file, but some platforms are already living without it and ordinarily there shouldn't be one. The function itself is left around in case any extensions are using it. It will be removed on HEAD via a follow-up commit. Here is a summary of the existing callers of durable_rename_excl() (see second discussion link at the bottom), replaced by this commit. First, basic_archive used it to avoid overwriting an archive concurrently created by another server, but as mentioned above, it will still overwrite files on some platforms. Second, xlog.c uses it to recycle past WAL segments, where an overwrite should not happen (origin of the change at f0e37a8) because there are protections about the WAL segment to select when recycling an entry. The third and last area is related to the write of timeline history files. writeTimeLineHistory() will write a new timeline history file at the end of recovery on promotion, so there should be no such files for the same timeline. What remains is writeTimeLineHistoryFile(), that can be used in parallel by a WAL receiver and the startup process, and some digging of the buildfarm shows that EEXIST from a WAL receiver can happen with an error of "could not link file \"pg_wal/xlogtemp.NN\" to \"pg_wal/MM.history\", which would cause an automatic restart of the WAL receiver as it is promoted to FATAL, hence this should improve the stability of the WAL receiver as rename() would overwrite an existing TLI history file already fetched by the startup process at recovery. This is a bug fix, but knowing the unlikeliness of the problem involving one or more crashes at an exceptionally bad moment, no backpatch is done. Also, I want to be careful with such changes (aaa3aed did the opposite of this change by removing HAVE_WORKING_LINK so as Windows would do a link() rather than a rename() but this was not concurrent-safe). A backpatch could be revisited in the future. This is the second time this change is attempted, ccfbd92 being the first one, but this time no assertions are added for the case of a TLI history file written concurrently by the WAL receiver or the startup process because we can expect one to exist (some of the TAP tests are able to trigger with a proper timing). Author: Nathan Bossart Reviewed-by: Robert Haas, Kyotaro Horiguchi, Michael Paquier Discussion: https://postgr.es/m/20220407182954.GA1231544@nathanxps13 Discussion: https://postgr.es/m/[email protected] (cherry picked from commit dac1ff30906b9cef7859380905d038892b32968b)
A previous commit replaced all the calls to this function with durable_rename() as of dac1ff3, making it used nowhere in the tree. Using it in extension code is also risky based on the issues described in this previous commit, so let's remove it. This makes possible the removal of HAVE_WORKING_LINK. Author: Nathan Bossart Reviewed-by: Robert Haas, Kyotaro Horiguchi, Michael Paquier Discussion: https://postgr.es/m/20220407182954.GA1231544@nathanxps13 (cherry picked from commit eb64ceac7ec3422f2370b8824dce62ee8fe52dca)
Containing the types of the columns returned by the prepared statement. Prompted by question from IRC user mlvzk. Author: Dagfinn Ilmari Mannsåker <[email protected]> Discussion: https://www.postgresql.org/message-id/flat/[email protected] (cherry picked from commit 84ad713cf85aeffee5dd39f62d49a1b9e34632da)
As noted by Thomas Munro, CLDR 36 has added SOUND RECORDING COPYRIGHT (U+2117), and we use CLDR 41, so this can be removed from the set of special cases. The set of regression tests is expanded for degree signs, which are two of the special cases, and a fancy case with U+210C in Latin-ASCII.xml that we have discovered about when diving into what could be done for Cyrillic characters (this last part is material for a future patch, not tackled yet). While on it, some of the assertions of generate_unaccent_rules.py are expanded to report the codepoint on which a failure is found, something useful for debugging. Extracted from a larger patch by the same author. Author: Przemysław Sztoch Discussion: https://postgr.es/m/[email protected] (cherry picked from commit e3dd7c06e62774628e102c3cd47ee46e85519de7)
Amendment to 84ad713cf85aeffee5dd39f62d49a1b9e34632da: Not all prepared statements have a result descriptor. As currently coded, this would crash when reading pg_prepared_statements. Make those cases return null for result_types instead. Also add a test case for it. (cherry picked from commit 6ffff0fd225432fe2ae4bd5abb7ff6113e255418)
The existing wording wasn't clear enough and some details weren't anywhere, such as the fact that autosummarization is off by default. Improve. Authors: Roberto Mello, Jaime Casanova, Justin Pryzby, Álvaro Herrera Discussion: https://postgr.es/m/CAKz==bK_NoJytRyQfX8K-erCW3Ff7--oGYpiB8+ePVS7dRVW_A@mail.gmail.com Discussion: https://postgr.es/m/[email protected] (cherry picked from commit 5001b44b11381f6e1787403ae81bce1ff1f78a99)
We were going into IDLE state too soon when executing queries via PQsendQuery in pipeline mode, causing several scenarios to misbehave in different ways -- most notably, as reported by Daniele Varrazzo, that a warning message is produced by libpq: message type 0x33 arrived from server while idle But it is also possible, if queries are sent and results consumed not in lockstep, for the expected mediating NULL result values from PQgetResult to be lost (a problem which has not been reported, but which is more serious). Fix this by introducing two new concepts: one is a command queue element PGQUERY_CLOSE to tell libpq to wait for the CloseComplete server response to the Close message that is sent by PQsendQuery. Because the application is not expecting any PGresult from this, the mechanism to consume it is a bit hackish. The other concept, authored by Horiguchi-san, is a PGASYNC_PIPELINE_IDLE state for libpq's state machine to differentiate "really idle" from merely "the idle state that occurs in between reading results from the server for elements in the pipeline". This makes libpq not go fully IDLE when the libpq command queue contains entries; in normal cases, we only go IDLE once at the end of the pipeline, when the server response to the final SYNC message is received. (However, there are corner cases it doesn't fix, such as terminating the query sequence by PQsendFlushRequest instead of PQpipelineSync; this sort of scenario is what requires PGQUERY_CLOSE bit above.) This last bit helps make the libpq state machine clearer; in particular we can get rid of an ugly hack in pqParseInput3 to avoid considering IDLE as such when the command queue contains entries. A new test mode is added to libpq_pipeline.c to tickle some related problematic cases. Reported-by: Daniele Varrazzo <[email protected]> Co-authored-by: Kyotaro Horiguchi <[email protected]> Discussion: https://postgr.es/m/CA+mi_8bvD0_CW3sumgwPvWdNzXY32itoG_16tDYRu_1S2gV2iw@mail.gmail.com (cherry picked from commit 054325c5eeb3140a067ba66735c3d811163ecd6a)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Merging community commits from f58f1fb6c0f0990558d0859018b31412b1338447 Add missing GETTEXT_FLAGS entry to 054325c5eeb3140a067ba66735c3d811163ecd6a libpq: Improve idle state handling in pipeline mode.
Extension side PR: amazon-aurora/babelfish_extensions#54