Correctifs appliqués

Magnus Hagander pushed:

Tom Lane pushed:

  • Minor code review for parse_phrase_operator(). Fix its header comment, which described the old behavior of the <N> phrase distance operator; we missed updating that in commit 028350f61. Also, reset errno before strtol() call, to defend against the possibility that it was already ERANGE at entry. (The lack of complaints says that it generally isn't, but this is at least a latent bug.) Very minor stylistic improvements as well. Victor Drobny noted the obsolete comment, I noted the errno issue. Back-patch to 9.6 where this code was added, just in case the errno issue is a live bug in some cases. Discussion:
  • Avoid useless "x = ANY(ARRAY[])" test for empty partition list. This arises in practice if the partition only admits NULL values. Jeevan Ladhe Discussion:
  • Improve wait logic in TAP tests for streaming replication. Remove hard-wired sleep(2) delays in in favor of using poll_query_until to check for the desired state to appear. In addition, add such a wait before the last test in the script, as it's possible to demonstrate failures there after upcoming improvements in pg_ctl. (We might end up adding polling before each of the get_slot_xmins calls in this script, but I feel no great need to do that until shown necessary.) In passing, clarify the description strings for some of the test cases. Michael Paquier and Craig Ringer, pursuant to a complaint from me Discussion:
  • Reduce pg_ctl's reaction time when waiting for postmaster start/stop. pg_ctl has traditionally waited one second between probes for whether the start or stop request has completed. That behavior was embodied in the original shell script written in 1999 (commit 5b912b089) and I doubt anyone's questioned it since. Nowadays, machines are a lot faster, and the shell script is long since replaced by C code, so it's fair to reconsider how long we ought to wait. This patch adjusts the coding so that the wait time can be any even divisor of 1 second, and sets the actual probe rate to 10 per second. That's based on experimentation with the src/test/recovery TAP tests, which include a lot of postmaster starts and stops. This patch alone reduces the (non-parallelized) runtime of those tests from ~4m30s to ~3m5s on my machine. Increasing the probe rate further doesn't help much, so this seems like a good number. In the real world this probably won't have much impact, since people don't start/stop production postmasters often, and the shutdown checkpoint usually takes nontrivial time too. But it makes development work and testing noticeably snappier, and that's good enough reason for me. Also, by reducing the dead time in postmaster restart sequences, this change has made it easier to reproduce some bugs that have been lurking for awhile. Patches for those will follow. Discussion:
  • Ignore old stats file timestamps when starting the stats collector. The stats collector disregards inquiry messages that bear a cutoff_time before when it last wrote the relevant stats file. That's fine, but at startup when it reads the "permanent" stats files, it absorbed their timestamps as if they were the times at which the corresponding temporary stats files had been written. In reality, of course, there's no data out there at all. This led to disregarding inquiry messages soon after startup if the postmaster had been shut down and restarted within less than PGSTAT_STAT_INTERVAL; which is a pretty common scenario, both for testing and in the field. Requesting backends would hang for 10 seconds and then report failure to read statistics, unless they got bailed out by some other backend coming along and making a newer request within that interval. I came across this through investigating unexpected delays in the src/test/recovery TAP tests: it manifests there because the autovacuum launcher hangs for 10 seconds when it can't get statistics at startup, thus preventing a second shutdown from occurring promptly. We might want to do some things in the autovac code to make it less prone to getting stuck that way, but this change is a good bug fix regardless. In passing, also fix pgstat_read_statsfiles() to ensure that it re-zeroes its global stats variables if they are corrupted by a short read from the stats file. (Other reads in that function go into temp variables, so that the issue doesn't arise.) This has been broken since we created the separation between permanent and temporary stats files in 8.4, so back-patch to all supported branches. Discussion:
  • Don't lose walreceiver start requests due to race condition in postmaster. When a walreceiver dies, the startup process will notice that and send a PMSIGNAL_START_WALRECEIVER signal to the postmaster, asking for a new walreceiver to be launched. There's a race condition, which at least in HEAD is very easy to hit, whereby the postmaster might see that signal before it processes the SIGCHLD from the walreceiver process. In that situation, sigusr1_handler() just dropped the start request on the floor, reasoning that it must be redundant. Eventually, after 10 seconds (WALRCV_STARTUP_TIMEOUT), the startup process would make a fresh request --- but that's a long time if the connection could have been re-established almost immediately. Fix it by setting a state flag inside the postmaster that we won't clear until we do launch a walreceiver. In cases where that results in an extra walreceiver launch, it's up to the walreceiver to realize it's unwanted and go away --- but we have, and need, that logic anyway for the opposite race case. I came across this through investigating unexpected delays in the src/test/recovery TAP tests: it manifests there in test cases where a master server is stopped and restarted while leaving streaming slaves active. This logic has been broken all along, so back-patch to all supported branches. Discussion:
  • Reduce wal_retrieve_retry_interval in applicable TAP tests. By default, wal_retrieve_retry_interval is five seconds, which is far more than is needed in any of our TAP tests, leaving the test cases just twiddling their thumbs for significant stretches. Moreover, because it's so large, we get basically no testing of the retry-before- master-is-ready code path. Hence, make PostgresNode::init set up wal_retrieve_retry_interval = '500ms' as part of its customization of test clusters' postgresql.conf. This shaves quite a few seconds off the runtime of the recovery TAP tests. Back-patch into 9.6. We have wal_retrieve_retry_interval in 9.5, but the test infrastructure isn't there. Discussion:
  • Support tcp_keepalives_idle option on Solaris. Turns out that the socket option for this is named TCP_KEEPALIVE_THRESHOLD, at least according to the tcp(7P) man page for Solaris 11. (But since that text refers to "SunOS", it's likely pretty ancient.) It appears that the symbol TCP_KEEPALIVE does get defined on that platform, but it doesn't seem to represent a valid protocol-level socket option. This leads to bleats in the postmaster log, and no tcp_keepalives_idle functionality. Per bug #14720 from Andrey Lizenko, as well as an earlier report from Dhiraj Chawla that nobody had followed up on. The issue's been there since we added the TCP_KEEPALIVE code path in commit 5acd417c8, so back-patch to all supported branches. Discussion:
  • Re-allow SRFs and window functions within sub-selects within aggregates. check_agg_arguments_walker threw an error upon seeing a SRF or window function, but that is too aggressive: if the function is within a sub-select then it's perfectly fine. I broke the SRF case in commit 0436f6bde by copying the logic for window functions ... but that was broken too, and had been since commit eaccfded9. Repair both cases in HEAD, and the window function case back to 9.3. 9.2 gets this right.
  • Second try at fixing tcp_keepalives_idle option on Solaris. Buildfarm evidence shows that TCP_KEEPALIVE_THRESHOLD doesn't exist after all on Solaris < 11. This means we need to take positive action to prevent the TCP_KEEPALIVE code path from being taken on that platform. I've chosen to limit it with "&& defined(__darwin__)", since it's unclear that anyone else would follow Apple's precedent of spelling the symbol that way. Also, follow a suggestion from Michael Paquier of eliminating code duplication by defining a couple of intermediate symbols for the socket option. In passing, make some effort to reduce the number of translatable messages by replacing "setsockopt(foo) failed" with "setsockopt(%s) failed", etc, throughout the affected files. And update relevant documentation so that it doesn't claim to provide an exhaustive list of the possible socket option names. Like the previous commit (f0256c774), back-patch to all supported branches. Discussion:
  • Change pg_ctl to detect server-ready by watching status in Traditionally, "pg_ctl start -w" has waited for the server to become ready to accept connections by attempting a connection once per second. That has the major problem that connection issues (for instance, a kernel packet filter blocking traffic) can't be reliably told apart from server startup issues, and the minor problem that if server startup isn't quick, we accumulate "the database system is starting up" spam in the server log. We've hacked around many of the possible connection issues, but it resulted in ugly and complicated code in pg_ctl.c. In commit c61559ec3, I changed the probe rate to every tenth of a second. That prompted Jeff Janes to complain that the log-spam problem had become much worse. In the ensuing discussion, Andres Freund pointed out that we could dispense with connection attempts altogether if the postmaster were changed to report its status in, which "pg_ctl start" already relies on being able to read. This patch implements that, teaching postmaster.c to report a status string into the pidfile at the same state-change points already identified as being of interest for systemd status reporting (cf commit 7d17e683f). pg_ctl no longer needs to link with libpq at all; all its functions now depend on reading server files. In support of this, teach AddToDataDirLockFile() to allow addition of lines in not-necessarily-sequential order. This is needed on Windows where the SHMEM_KEY line will never be written at all. We still have the restriction that we don't want to truncate the pidfile; document the reasons for that a bit better. Also, fix the pg_ctl TAP tests so they'll notice if "start -w" mode is broken --- before, they'd just wait out the sixty seconds until the loop gives up, and then report success anyway. (Yes, I found that out the hard way.) While at it, arrange for pg_ctl to not need to #include miscadmin.h; as a rather low-level backend header, requiring that to be compilable client-side is pretty dubious. This requires moving the #define's associated with the pidfile into a new header file, and moving PG_BACKEND_VERSIONSTR someplace else. For lack of a clearly better "someplace else", I put it into port.h, beside the declaration of find_other_exec(), since most users of that macro are passing the value to find_other_exec(). (initdb still depends on miscadmin.h, but at least pg_ctl and pg_upgrade no longer do.) In passing, fix main.c so that PG_BACKEND_VERSIONSTR actually defines the output of "postgres -V", which remarkably it had never done before. Discussion:
  • Ooops, WIN32 code in pg_ctl.c still needs PQExpBuffer. Per buildfarm.
  • Eat XIDs more efficiently in recovery TAP test. The point of this loop is to insert 1000 rows into the test table and consume 1000 XIDs. I can't see any good reason why it's useful to launch 1000 psqls and 1000 backend processes to accomplish that. Pushing the looping into a plpgsql DO block shaves about 10 seconds off the runtime of the src/test/recovery TAP tests on my machine; that's over 10% of the runtime of that test suite. It is, in fact, sufficiently more efficient that we now demonstrably need wait_slot_xmins() afterwards, or the slaves' xmins may not have moved yet.
  • Fix walsender to exit promptly if client requests shutdown. It's possible for WalSndWaitForWal to be asked to wait for WAL that doesn't exist yet. That's fine, in fact it's the normal situation if we're caught up; but when the client requests shutdown we should not keep waiting. The previous coding could wait indefinitely if the source server was idle. In passing, improve the rather weak comments in this area, and slightly rearrange some related code for better readability. Back-patch to 9.4 where this code was introduced. Discussion:
  • Check for error during PQendcopy. Oversight in commit 78c8c8143; noted while nosing around the walreceiver startup/shutdown code.
  • Fix race conditions and missed wakeups in syncrep worker signaling. When a sync worker is waiting for the associated apply worker to notice that it's in SYNCWAIT state, wait_for_worker_state_change() would just patiently wait for that to happen. This generally required waiting for the 1-second timeout in LogicalRepApplyLoop to elapse. Kicking the worker via its latch makes things significantly snappier. While at it, fix race conditions that could potentially result in crashes: we can *not* call logicalrep_worker_wakeup_ptr() once we've released the LogicalRepWorkerLock, because worker->proc might've been reset to NULL after we do that (indeed, there's no really solid reason to believe that the LogicalRepWorker slot even belongs to the same worker anymore). In logicalrep_worker_wakeup(), we can just move the wakeup inside the lock scope. In process_syncing_tables_for_apply(), a bit more code rearrangement is needed. Also improve some nearby comments.
  • Shorten timeouts while waiting for logicalrep worker slot attach/detach. When waiting for a logical replication worker process to start or stop, we have to busy-wait until we see it add or remove itself from the LogicalRepWorker slot in shared memory. Those loops were using a one-second delay between checks, but on any reasonably modern machine, it doesn't take more than a couple of msec for a worker to spawn or shut down. Reduce the loop delays to 10ms to avoid wasting quite so much time in the related regression tests. In principle, a better solution would be to fix things so that the waiting process can be awakened via its latch at the right time. But that seems considerably more invasive, which is undesirable for a post-beta fix. Worker start/stop performance likely isn't of huge interest anyway for production purposes, so we might not ever get around to it. In passing, rearrange the second wait loop in logicalrep_worker_stop() so that the lock is held at the top of the loop, thus saving one lock acquisition/release per call, and making it look more like the other loop. Discussion:
  • Reduce delay for last logicalrep feedback message when master goes idle. The regression tests contain numerous cases where we do some activity on a master server and then wait till the slave has ack'd flushing its copy of that transaction. Because WAL flush on the slave is asynchronous to the logicalrep worker process, the worker cannot send such a feedback message during the LogicalRepApplyLoop iteration where it processes the last data from the master. In the previous coding, the feedback message would come out only when the loop's WaitLatchOrSocket call returned WL_TIMEOUT. That requires one full second of delay (NAPTIME_PER_CYCLE); and to add insult to injury, it could take more than that if the WaitLatchOrSocket was interrupted a few times by latch-setting events. In reality we can expect the slave's walwriter process to have flushed the WAL data after, more or less, WalWriterDelay (typically 200ms). Hence, if there are unacked transactions pending, make the wait delay only that long rather than the full NAPTIME_PER_CYCLE. Also, move one of the send_feedback() calls into the loop main line, so that we'll check for the need to send feedback even if we were woken by a latch event and not either socket data or timeout. It's not clear how much this matters for production purposes, but it's definitely helpful for testing. Discussion:
  • Clean up misuse and nonuse of poll_query_until(). Several callers of PostgresNode::poll_query_until() neglected to check for failure; I do not think that's optional. Also, rewrite one place that had reinvented poll_query_until() for no very good reason.
  • Improve TAP test function PostgresNode::poll_query_until(). Add an optional "expected" argument to override the default assumption that we're waiting for the query to return "t". This allows replacing a handwritten polling loop in recovery/t/ with use of poll_query_until(); AFAICS that's the only remaining ad-hoc polling loop in our TAP tests. Change poll_query_until() to probe ten times per second not once per second. Like some similar changes I've been making recently, the one-second interval seems to be rooted in ancient traditions rather than the actual likely wait duration on modern machines. I'd consider reducing it further if there were a convenient way to spawn just one psql for the whole loop rather than one per probe attempt. Discussion:
  • Try to improve readability of recovery/t/ test. The original coding here was very confusing, because it named the two servers it set up "master" and "slave" even though it swapped their replication roles multiple times. At any given point in the script it was very unobvious whether "$node_master" actually referred to the server named "master" or the other one. Instead, pick arbitrary names for the two servers --- I used "london" and "paris" --- and distinguish those permanent names from the nonce references $cur_master and $cur_slave. Add logging to help distinguish which is which at any given point. Also, use distinct data and transaction names to make all the prepared transactions easily distinguishable in the postmaster logs. (There was one place where we intentionally tested that the server could cope with re-use of a transaction name, but it seems like one place is sufficient for that purpose.) Also, add checks at the end to make sure that all the transactions that were supposed to be committed did survive. Discussion:
  • Fix bug in PostgresNode::query_hash's split() call. By default, Perl's split() function drops trailing empty fields, which is not what we want here. Oversight in commit fb093e4cb. We'd managed to miss it thus far thanks to the very limited usage of this function. Discussion:
  • Fix race condition in recovery/t/ test. Since reducing pg_ctl's reaction time in commit c61559ec3, some slower buildfarm members have shown erratic failures in this test. The reason turns out to be that the test assumes synchronous replication (because it does not provide any lag time for a commit to replicate before shutting down the servers), but it had only enabled sync rep in one direction. The observed symptoms correspond to failure to replicate the last committed transaction in the other direction, which can be expected to happen if the shutdown command is issued soon enough and we are providing no synchronous-commit guarantees. Fix that, and add a bit more paranoid state checking at the bottom of the script. Michael Paquier and myself Discussion:

Stephen Frost pushed:

  • Do not require 'public' to exist for pg_dump -c. Commit 330b84d8c4 didn't contemplate the case where the public schema has been dropped and introduced a query which fails when there is no public schema into pg_dump (when used with -c). Adjust the query used by pg_dump to handle the case where the public schema doesn't exist and add tests to check that such a case no longer fails. Back-patch the specific fix to 9.6, as the prior commit was. Adding tests for this case involved adding support to the pg_dump TAP tests to work with multiple databases, which, while not a large change, is a bit much to back-patch, so that's only done in master. Addresses bug #14650 Discussion:

Andrew Gierth pushed:

Peter Eisentraut pushed:

Robert Haas pushed:

�lvaro Herrera pushed:

  • Fix locking in WAL receiver/sender shmem state structs. In WAL receiver and WAL server, some accesses to their corresponding shared memory control structs were done without holding any kind of lock, which could lead to inconsistent and possibly insecure results. In walsender, fix by clarifying the locking rules and following them correctly, as documented in the new comment in walsender_private.h; namely that some members can be read in walsender itself without a lock, because the only writes occur in the same process. The rest of the struct requires spinlock for accesses, as usual. In walreceiver, fix by always holding spinlock while accessing the struct. While there is potentially a problem in all branches, it is minor in stable ones. This only became a real problem in pg10 because of quorum commit in synchronous replication (commit 3901fd70cc7c), and a potential security problem in walreceiver because a superuser() check was removed by default monitoring roles (commit 25fff40798fc). Thus, no backpatch. In passing, clean up some leftover braces which were used to create unconditional blocks. Once upon a time these were used for volatile-izing accesses to those shmem structs, which is no longer required. Many other occurrences of this pattern remain. Author: Micha�l Paquier Reported-by: Micha�l Paquier Reviewed-by: Masahiko Sawada, Kyotaro Horiguchi, Thomas Munro, Robert Haas Discussion:

Correctifs en attente

Amit Langote sent in two more revisions of a patch to set pd_lower correctly in the GIN metapage, set pd_lower correctly in the BRIN index metapage, and set pd_lower correctly in the SP-GiST index metapage.

Atsushi Torikoshi sent in a patch to remove old comments in dependencies.c and README.dependencies.

Daniel Gustafsson sent in a patch for psql to free allocated memory when 2 patterns are used, use consistent function comments for metacommands, add a server version check, include all the details from normal (\dRp) in verbose mode for \dRp (\dRp+), use PQExpBuffer for all table titles, and improve consistency in "object not found" notices in psql.

Kyotaro HORIGUCHI sent in a patch to fix a race between SELECT and ALTER TABLE NO INHERIT.

Micha�l Paquier sent in two revisions of a patch to fix an issue where pg_basebackup fails on Windows when using tablespace mapping.

Thomas Munro sent in a patch to fix an out of date comment in predicate.c.

Mithun Cy sent in two more revisions of a patch to implement auto_prewarm.

Shubham Barai sent in another revision of a patch to enable predicate locking in hash index.

Thomas Munro sent in another revision of a patch to implement [[Parallel] Shared] Hash.

Thomas Munro sent in another revision of a patch to fix an issue that manifested as pg_serial early wraparound.

Fabien COELHO sent in two more revisions of a patch to psql to add a special variable to reflect the last query status.

Andres Freund sent in three more revisions of a patch to add fallocate.

Petr Jel�nek sent in a patch to rework subscription worker and relation status handling.

Mark Rofail sent in another revision of a patch to implement foreign key arrays.

Yugo Nagata sent in a patch to create a pg_reload_backend to signal SIGHUP to a specific backend.

Micha�l Paquier sent in a patch to fix a comment in xlog.c

Zero King sent in a patch to change some wording about libedit.

Amit Khandekar and Robert Haas traded patches to enable UPDATEs to the partition key of a declaratively partitioned table which would move the tuple to a different partition.

Etsuro Fujita sent in another revision of a patch to add support for tuple routing to foreign partitions.

Adrien Nayrat sent in a patch to add a --createdb-only option to pg_dumpall.

Thomas Munro sent in a patch to implement synchronous replay.

Beena Emerson sent in another revision of a patch to implement default partition for declarative range partitions.

Tom Lane sent in two revisions of a patch to fix an apparent walsender bug triggered by logical replication.

Yugo Nagata sent in a patch to fix an apparent documentation error in DROP SUBSCRIPTION.

Peter Eisentraut sent in a patch for pg_ctl to make failure to complete operation a nonzero exit.

Amit Langote and Dean Rasheed traded patches to simplify code that checks range partition overlap, and relax some rules about unbounded range partition bounds.

Tom Lane sent in a WIP patch for avoiding duplicate initdb runs during "make check".