Sync from Upstream #163
Merged
Sync from Upstream #163
Conversation
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
The previous protocol expected a set of matching characters to check if a message sent was the last one or not, that changed depending on the destination wanted: - 't' and 'f' tracked the last message of a log sent to stderr. - 'T' and 'F' tracked the last message of a log sent to csvlog. This could be extended with more characters when introducing new destinations, but using a bitmask is much more elegant. This commit changes the protocol so as a bitmask is used in the header of a log chunk message sent to the syslogger, with the following options available for now: - log_destination as stderr. - log_destination as csvlog. - if a message is the last chunk of a message. Sehrope found this issue in a patch set to introduce JSON as an option for log_destination, but his patch made the size of the protocol header larger. This commit keeps the same size as the original, and adapts the protocol as wanted. Thanks also to Andrew Dunstan and Greg Stark for the discussion. Author: Michael Paquier, Sehrope Sarkuni Discussion: https://postgr.es/m/CAH7T-aqswBM6JWe4pDehi1uOiufqe06DJWaU5=X7dDLyqUExHg@mail.gmail.com
These are added in the existing tests of pg_ctl for log rotation, that already tested stderr. The same amount of coverage is added for csvlog: - Checks for pg_current_logfile(). - Log rotation with expected file name. - Log contents generated. This test is refactored to minimize the amount of work required to add tests for new log formats, easing some upcoming work. Author: Michael Paquier, Sehrope Sarkuni Discussion: https://postgr.es/m/CAH7T-aqswBM6JWe4pDehi1uOiufqe06DJWaU5=X7dDLyqUExHg@mail.gmail.com
The subscription parameters are rearranged into two groups: a) those that control what happens during Create Subscription b) those that control the replication behavior This makes the documentation of Create Subscription easier to follow. Author: Peter Smith Reviewed-by: Amit Kapila Discussion: https://postgr.es/m/CAHut+PtPJDSOxtuMGpO2yDrRPKxcYGL4n7HqJP9HernZE=Cj+g@mail.gmail.com
An out-of-memory failure happening when allocating the structures to store the connection parameter keywords and values would mess up with the set of connections saved, as on failure the pthread mutex would still be hold with the new connection object listed but free()'d. Rather than just unlocking the mutex, which would leave the static list of connections into an inconsistent state, move the allocation for the structures of the connection parameters before beginning the test manipulation. This ensures that the list of connections and the connection mutex remain consistent all the time in this code path. This error is unlikely going to happen, but this could mess up badly with ECPG clients in surprising ways, so backpatch all the way down. Reported-by: ryancaicse Discussion: https://postgr.es/m/17186-b4cfd8f0eb4d1dee@postgresql.org Backpatch-through: 9.6
While processing toast changes in logical decoding, we rejigger the tuple change to point to in-memory toast tuples instead to on-disk toast tuples. And, to make sure the memory accounting is correct, we were subtracting the old change size and then after re-computing the new tuple, re-adding its size at the end. Now, if there is any error before we add the new size, we will release the changes and that will update the accounting info (subtracting the size from the counters). And we were underflowing there which leads to an assertion failure in assert enabled builds and wrong memory accounting in reorder buffer otherwise. Author: Bertrand Drouvot Reviewed-by: Amit Kapila Backpatch-through: 13, where memory accounting was introduced Discussion: https://postgr.es/m/92b0ee65-b8bd-e42d-c082-4f3f4bf12d34@amazon.com
Ordinarily, using EXIT this way would draw "control reached end of function without RETURN". However, if the function is one where we don't require an explicit RETURN (such as a DO block), that should not happen. It did anyway, because add_dummy_return() neglected to account for the case. Per report from Herwig Goemans. Back-patch to all supported branches. Discussion: https://postgr.es/m/868ae948-e3ca-c7ec-95a6-83cfc08ef750@gmail.com
Improve the descriptions of some options. Fix sloppy grammar and markup. Peter Smith and Tom Lane Discussion: https://postgr.es/m/CAHut+PtPJDSOxtuMGpO2yDrRPKxcYGL4n7HqJP9HernZE=Cj+g@mail.gmail.com
Commits ffa2e46 and 52a1022 caused libpq's connection-establishment functions to usually leave a nonempty string in the connection's errorMessage buffer, even after a successful connection. While that was intentional on my part, more sober reflection says that it wasn't a great idea: the string would be a bit confusing. Also this broke at least one application that checked for connection success by examining the errorMessage, instead of using PQstatus() as documented. Let's clear the buffer at success exit, restoring the pre-v14 behavior. Discussion: https://postgr.es/m/4170264.1620321747@sss.pgh.pa.us
Two functions, both named check_permissions(), used the same checks to verify if a user had required privileges to work on replication slots. This commit removes the duplication, and moves the function doing the checks to slot.c to be centralized. Author: Bharath Rupireddy Reviewed-by: Nathan Bossart, Euler Taveira Discussion: https://postgr.es/m/CALj2ACUPpVw1u7sQocFVWrSs0n10pt_G_4NPZKSxXK6cW1dErw@mail.gmail.com
…ors. If an allocation failed within LLVM it is not safe to call back into LLVM as LLVM is not generally safe against exceptions / stack-unwinding. Thus errors while in LLVM code are promoted to FATAL. However llvm_shutdown() did call back into LLVM even in such cases, while llvm_release_context() was careful not to do so. We cannot generally skip shutting down LLVM, as that can break profiling. But it's OK to do so if there was an error from within LLVM. Reported-By: Jelte Fennema <Jelte.Fennema@microsoft.com> Author: Andres Freund <andres@anarazel.de> Author: Justin Pryzby <pryzby@telsasoft.com> Discussion: https://postgr.es/m/AM5PR83MB0178C52CCA0A8DEA0207DC14F7FF9@AM5PR83MB0178.EURPRD83.prod.outlook.com Backpatch: 11-, where jit was introduced
This is an abstract node that shouldn't have a node tag defined. Reviewed-by: Jacob Champion <pchampion@vmware.com> Discussion: https://www.postgresql.org/message-id/c091e5cd-45f8-69ee-6a9b-de86912cc7e7@enterprisedb.com
These handle node fields that are inline arrays (as opposed to dynamically allocated arrays handled by COPY_POINTER_FIELD and COMPARE_POINTER_FIELD). These cases were hand-coded until now. Reviewed-by: Jacob Champion <pchampion@vmware.com> Discussion: https://www.postgresql.org/message-id/c091e5cd-45f8-69ee-6a9b-de86912cc7e7@enterprisedb.com
We have a few WRITE_{name of type}_ARRAY macros, but the one case
using the Index type was hand-coded. Wrap it into a macro as well.
This also changes the behavior slightly: Before, the field name was
skipped if the length was zero. Now it prints the field name even in
that case. This is more consistent with how other array fields are
handled.
Reviewed-by: Jacob Champion <pchampion@vmware.com>
Discussion: https://www.postgresql.org/message-id/c091e5cd-45f8-69ee-6a9b-de86912cc7e7@enterprisedb.com
It's possible for us to copy an AlternativeSubPlan expression node into multiple places, for example the scan quals of several partition children. Then it's possible that we choose a different one of the alternatives as optimal in each place. Commit 41efb83 failed to consider this scenario, so its attempt to remove "unused" subplans could remove subplans that were still used elsewhere. Fix by delaying the removal logic until we've examined all the AlternativeSubPlans in a given query level. (This does assume that AlternativeSubPlans couldn't get copied to other query levels, but for the foreseeable future that's fine; cf qual_is_pushdown_safe.) Per report from Rajkumar Raghuwanshi. Back-patch to v14 where the faulty logic came in. Discussion: https://postgr.es/m/CAKcux6==O3NNZC3bZ2prRYv3cjm3_Zw1GfzmOjEVqYN4jub2+Q@mail.gmail.com
Formerly, we sent signals for outgoing NOTIFY messages within ProcessCompletedNotifies, which was also responsible for sending relevant ones of those messages to our connected client. It therefore had to run during the main-loop processing that occurs just before going idle. This arrangement had two big disadvantages: * Now that procedures allow intra-command COMMITs, it would be useful to send NOTIFYs to other sessions immediately at COMMIT (though, for reasons of wire-protocol stability, we still shouldn't forward them to our client until end of command). * Background processes such as replication workers would not send NOTIFYs at all, since they never execute the client communication loop. We've had requests to allow triggers running in replication workers to send NOTIFYs, so that's a problem. To fix these things, move transmission of outgoing NOTIFY signals into AtCommit_Notify, where it will happen during CommitTransaction. Also move the possible call of asyncQueueAdvanceTail there, to ensure we don't bloat the async SLRU if a background worker sends many NOTIFYs with no one listening. We can also drop the call of asyncQueueReadAllNotifications, allowing ProcessCompletedNotifies to go away entirely. That's because commit 7900269 added a call of ProcessNotifyInterrupt adjacent to PostgresMain's call of ProcessCompletedNotifies, and that does its own call of asyncQueueReadAllNotifications, meaning that we were uselessly doing two such calls (inside two separate transactions) whenever inbound notify signals coincided with an outbound notify. We need only set notifyInterruptPending to ensure that ProcessNotifyInterrupt runs, and we're done. The existing documentation suggests that custom background workers should call ProcessCompletedNotifies if they want to send NOTIFY messages. To avoid an ABI break in the back branches, reduce it to an empty routine rather than removing it entirely. Removal will occur in v15. Although the problems mentioned above have existed for awhile, I don't feel comfortable back-patching this any further than v13. There was quite a bit of churn in adjacent code between 12 and 13. At minimum we'd have to also backpatch 51004c7, and a good deal of other adjustment would also be needed, so the benefit-to-risk ratio doesn't look attractive. Per bug #15293 from Michael Powers (and similar gripes from others). Artur Zakirov and Tom Lane Discussion: https://postgr.es/m/153243441449.1404.2274116228506175596@wrigleys.postgresql.org
pg_import_system_collations() was a bit inconsistent about how it reported locales (names output by "locale -a") that it didn't make pg_collation entries for. IMV we should print some suitable message for every locale that we reject, except when it matches a pre-existing pg_collation entry. (This is all at DEBUG1 log level, though, so as not to create noise during initdb.) Add messages for the two cases that were previously not logged, namely unrecognized encoding and client-only encoding. Re-word the existing messages to have a consistent style. Anton Voloshin and Tom Lane Discussion: https://postgr.es/m/429d64ee-188d-3ce1-106a-53a8b45c4fce@postgrespro.ru
All the types supported were listed directly in the README, but it was very outdated. Rather than listing all the types supported in the README, this commit adds a reference to look at ResourceOwnerData in resowner.c to get this information. The order of the paragraphs is reworked a bit for clarity. Author: Amit Langote Discussion: https://postgr.es/m/CA+HiwqHtfT9z=4H5+F7DOy0OyNHAaVwuRcakt9b2t2uADOaiag@mail.gmail.com
Also remove obsolete comments about why the 64-bit integers need to be printed in a separate buffer. The reason used to be portability, but now the remaining reason is that we need the string lengths for the progress displays. That is evident by looking at the code right below, so a new comment doesn't seem necessary.
The available refresh options are specified as refresh_options under REFRESH PUBLICATION, and DROP PUBLICATION itself has an option named refresh. Clarify what we mean by refresh options to avoid confusion. Backpatch through v14 where ALTER SUBSCRIPTION ... DROP PUBLICATION was introduced. Author: Masahiko Sawada <sawada.mshk@gmail.com> Reviewed-by: Amit Kapila <amit.kapila16@gmail.com> Reviewed-by: Peter Eisentraut <peter.eisentraut@enterprisedb.com> Reviewed-by: Peter Smith <smithpb2250@gmail.com> Discussion: https://postgr.es/m/CAD21AoCm1wJ3A8Q9EmBjRbShYkJ+o+Oa_z9O0hvwhvhUa2BSyg@mail.gmail.com Backpatch-through: 14
Commit a3d2b1b neglected to initialize the type_id field of the synthesized type cache entry, so it would make a new one on every call. Also, better use the per-function memory context for this; otherwise it leaks memory. Discussion: https://www.postgresql.org/message-id/flat/17158-8a2ba823982537a4%40postgresql.org
In most cases, the prefix string in a node output is the upper case of the node structure name, e.g., MergeAppend -> MERGEAPPEND. There were a few exceptions that for either no apparent reason or perhaps minor aesthetic reasons deviated from this. In order to simplify this and perhaps allow automatic generation without having to deal with exception cases, make them all match. Discussion: https://www.postgresql.org/message-id/c091e5cd-45f8-69ee-6a9b-de86912cc7e7@enterprisedb.com
It's possible to execute user-defined SQL in some background processes; for example, logical replication workers can fire triggers. This opens the possibility that someone would try to execute LISTEN in such a context. But since only regular backends ever call ProcessNotifyInterrupt, no messages would actually be received, and thus the registered listener would simply prevent the message queue from being cleaned. Eventually NOTIFY would stop working, which is bad. Perhaps someday somebody will invent infrastructure to make listening in a background worker actually useful. In the meantime, forbid it. Back-patch to v13, which is where we introduced the MyBackendType variable. It'd be a lot harder to implement the check without that, and it doesn't seem worth the trouble. Discussion: https://postgr.es/m/153243441449.1404.2274116228506175596@wrigleys.postgresql.org
Similar to Cost and Selectivity, this is just a double, which can be used in path and plan nodes to give some hint about the meaning of a field. Discussion: https://www.postgresql.org/message-id/c091e5cd-45f8-69ee-6a9b-de86912cc7e7@enterprisedb.com
Up to now the size of a query's rangetable has been limited by the constants INNER_VAR et al, which mustn't be equal to any real rangetable index. 65000 doubtless seemed like enough for anybody, and it still is orders of magnitude larger than the number of joins we can realistically handle. However, we need a rangetable entry for each child partition that is (or might be) processed by a query. Queries with a few thousand partitions are getting more realistic, so that the day when that limit becomes a problem is in sight, even if it's not here yet. Hence, let's raise the limit. Rather than just increase the values of INNER_VAR et al, this patch adopts the approach of making them small negative values, so that rangetables could theoretically become as long as INT_MAX. The bulk of the patch is concerned with changing Var.varno and some related variables from "Index" (unsigned int) to plain "int". This is basically cosmetic, with little actual effect other than to help debuggers print their values nicely. As such, I've only bothered with changing places that could actually see INNER_VAR et al, which the parser and most of the planner don't. We do have to be careful in places that are performing less/greater comparisons on varnos, but there are very few such places, other than the IS_SPECIAL_VARNO macro itself. A notable side effect of this patch is that while it used to be possible to add INNER_VAR et al to a Bitmapset, that will now draw an error. I don't see any likelihood that it wouldn't be a bug to include these fake varnos in a bitmapset of real varnos, so I think this is all to the good. Although this touches outfuncs/readfuncs, I don't think a catversion bump is required, since stored rules would never contain Vars with these fake varnos. Andrey Lepikhov and Tom Lane, after a suggestion by Peter Eisentraut Discussion: https://postgr.es/m/43c7f2f5-1e27-27aa-8c65-c91859d15190@postgrespro.ru
An upcoming patch splits single user mode handling out of PostgresMain(). The startup time only needs to be determined in single user mode. Currently the initialization happens late, which makes the split a bit harder. As postmaster determines the time earlier it makes sense to move the time for single user mode to a roughly similar point in time. Reviewd-By: Kyotaro Horiguchi <horikyota.ntt@gmail.com> Author: Andres Freund <andres@anarazel.de> Discussion: https://postgr.es/m/20210802164124.ufo5buo4apl6yuvs@alap3.anarazel.de
Until now, the -C option of postgres was handled before a small subset of GUCs computed at runtime are initialized, leading to incorrect results as GUC machinery would fall back to default values for such parameters. For example, data_checksums could report "off" for a cluster as the control file is not loaded yet. Or wal_segment_size would show a segment size at 16MB even if initdb --wal-segsize used something else. Worse, the command would fail to properly report the recently-introduced shared_memory, that requires to load shared_preload_libraries as these could ask for a chunk of shared memory. Support for runtime GUCs comes with a limitation, as the operation is now allowed on a running server. One notable reason for this is that _PG_init() functions of loadable libraries are called before all runtime-computed GUCs are initialized, and this is not guaranteed to be safe to do on running servers. For the case of shared_memory_size, where we want to know how much memory would be used without allocating it, this limitation is fine. Another case where this will help is for huge pages, with the introduction of a different GUC to evaluate the amount of huge pages required for a server before starting it, without having to allocate large chunks of memory. This feature is controlled with a new GUC flag, and four parameters are classified as runtime-computed as of this change: - data_checksums - shared_memory_size - data_directory_mode - wal_segment_size Some TAP tests are added to provide some coverage here, using data_checksums in the tests of pg_checksums. Per discussion with Andres Freund, Justin Pryzby, Magnus Hagander and more. Author: Nathan Bossart Discussion: https://postgr.es/m/F2772387-CE0F-46BF-B5F1-CC55516EB885@amazon.com
All size_t variables declared in procarray.c are actually int ones. Let's use int instead of size_t for those variables. Which would reduce Wsign-compare compiler warnings. Back-patch to v14 where commit 941697c added size_t variables in procarray.c, to make future back-patching easy though this patch is classified as refactoring only. Reported-by: Ranier Vilela Author: Ranier Vilela, Aleksander Alekseev https://postgr.es/m/CAEudQAqyoTZC670xWi6w-Oe2_Bk1bfu2JzXz6xRfiOUzm7xbyQ@mail.gmail.com
ProcArrayGroupClearXid function has a parameter named "proc", but the same name was used for its local variables. This commit fixes this variable shadowing, to improve code readability. Back-patch to all supported versions, to make future back-patching easy though this patch is classified as refactoring only. Reported-by: Ranier Vilela Author: Ranier Vilela, Aleksander Alekseev https://postgr.es/m/CAEudQAqyoTZC670xWi6w-Oe2_Bk1bfu2JzXz6xRfiOUzm7xbyQ@mail.gmail.com
Session statistics, as introduced by 960869d, had several shortcomings: - an additional GetCurrentTimestamp() call that also impaired the accuracy of the data collected This can be avoided by passing the current timestamp we already have in pgstat_report_stat(). - an additional statistics UDP packet sent every 500ms This is solved by adding the new statistics to PgStat_MsgTabstat. This is conceptually ugly, because session statistics are not table statistics. But the struct already contains data unrelated to tables, so there is not much damage done. Connection and disconnection are reported in separate messages, which reduces the number of additional messages to two messages per session and a slight increase in PgStat_MsgTabstat size (but the same number of table stats fit). - Session time computation could overflow on systems where long is 32 bit. Reported-By: Andres Freund <andres@anarazel.de> Author: Andres Freund <andres@anarazel.de> Author: Laurenz Albe <laurenz.albe@cybertec.at> Discussion: https://postgr.es/m/20210801205501.nyxzxoelqoo4x2qc%40alap3.anarazel.de Backpatch: 14-, where the feature was introduced.
…_BACKEND. An upcoming patch splits single user mode into its own function. This makes that easier. Split out for easier review / testing. Reviewed-By: Kyotaro Horiguchi <horikyota.ntt@gmail.com> Author: Andres Freund <andres@anarazel.de> Discussion: https://postgr.es/m/20210802164124.ufo5buo4apl6yuvs@alap3.anarazel.de
The rewriter transformation for SEARCH BREADTH FIRST produces a FieldSelect on a Var of type RECORD, where the Var references the recursive union's worktable output. EXPLAIN VERBOSE failed to handle this case, because it only expected such Vars to appear in CteScans not WorkTableScans. Fix that, and add some test cases exercising EXPLAIN on SEARCH and CYCLE queries. In principle this oversight is an old bug, but it seems that the case is unreachable without SEARCH BREADTH FIRST, because the parser fails when attempting to create such a reference manually. So for today I'll just patch HEAD/v14. Someday we might find that the code portion of this patch needs to be back-patched further. Per report from Atsushi Torikoshi. Discussion: https://postgr.es/m/5bafa66ad529e11860339565c9e7c166@oss.nttdata.com
The output generated on Msys is incorrect because of the different way IPC::Run processes outputs with native Perl (converts natively \r\n to \n) and Msys perl (\r\n kept as-is), causing this test to fail. For now, just disable the test to bring the buildfarm to a green state. I think that the correct long-term solution would be to tweak all the routines command_checks_* in PostgresNode.pm to handle this output like psql does when using Msys, by discarding \r automatically before comparing it. Per report from jacana and fairywren. Thanks to Tom Lane for the ping. Discussion: https://postgr.es/m/1252480.1631829409@sss.pgh.pa.us
A WAL segment closed during a WAL stream for pg_receivewal would generate incorrect error messages depending on the context, as the file name used when referring to a WAL segment ignored partial files or the compression method used. In such cases, the error message generated (failure on close, seek or rename) would not match a physical file name. The same code paths are used by pg_basebackup, but it uses no partial suffix so it is not impacted. 7fbe0c8 has introduced in walmethods.c a callback to get the exact physical file name used for a given context, this commit makes use of it to improve those error messages. This could be extended to more code paths of pg_basebackup/ in the future, if necessary. Extracted from a larger patch by the same author. Author: Georgios Kokolatos Discussion: https://postgr.es/m/ZCm1J5vfyQ2E6dYvXz8si39HQ2gwxSZ3IpYaVgYa3lUwY88SLapx9EEnOf5uEwrddhx2twG7zYKjVeuP5MwZXCNPybtsGouDsAD1o2L_I5E=@pm.me
Commit 55dc86e changed pull_varnos to use (if possible) the associated ph_eval_at for a PlaceHolderVar. I missed a fine point though: we might be looking at a PHV in the quals or tlist of a child appendrel, in which case we need to compute a ph_eval_at value that's been translated in the same way that the PHV itself has been (cf. adjust_appendrel_attrs). Fortunately, enough info is available in the PlaceHolderInfo to make such translation possible without additional outside data, so we don't need another round of uglification of planner APIs. This is a little bit complicated, but since it's a hard-to-hit corner case, I'm not much worried about adding cycles here. Per report from Jaime Casanova. Back-patch to v12, like the previous commit. Discussion: https://postgr.es/m/20210915230959.GB17635@ahch-to
An elog that reports the value of a transaction ID stored on a deleted nbtree page was added by commit e5d8a99, which taught page deletion to store full 64-bit XIDs. It seems very chatty on further reflection, so lower its elevel from NOTICE to DEBUG2. Author: Peter Geoghegan <pg@bowt.ie> Backpatch: 14-, just like the nbtree XID enhancement.
The following things are improved: - Fetch the system identifier from the source server before any WAL streaming loop. This triggers extra checks to make sure that pg_receivewal is still connected to a server with the same system ID with a correct timeline. - Switch umask() (for file creation mode mask) and RetrieveWalSegSize() (to fetch the size of WAL segments) a bit later before the initial stream attempt. If the connection was done with a database, pg_receivewal would fail but those commands were still executed, which was a waste. The slot creation and drop are now done before retrieving the segment size. Author: Bharath Rupireddy Reviewed-by: Ronan Dunklau, Michael Paquier Discussion: https://postgr.es/m/CALj2ACX00YYeyBfoi55Cy=NrP-FcfMgiYYx1qRUEib3yjCVoaA@mail.gmail.com
It was harder than necessary to understand PostgresMain() because the code for a normal backend was interspersed with single-user mode specific code. Split most of the single-user mode code into its own function PostgresSingleUserMain(), that does all the necessary setup for single-user mode, and then hands off after that to PostgresMain(). There still is some single-user mode code in InitPostgres(), and it'd likely be worth moving at least some of it out. But that's for later. Reviewed-By: Kyotaro Horiguchi <horikyota.ntt@gmail.com> Author: Andres Freund <andres@anarazel.de> Discussion: https://postgr.es/m/20210802164124.ufo5buo4apl6yuvs@alap3.anarazel.de
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
8 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.
No description provided.