Correctifs appliqués

Tom Lane pushed:

  • Replace over-optimistic Assert in partitioning code with a runtime test. get_partition_parent felt that it could simply Assert that systable_getnext found a tuple. This is unlike any other caller of that function, and it's unsafe IMO --- in fact, the reason I noticed it was that the Assert failed. (OK, I was working with known-inconsistent catalog contents, but I wasn't expecting the DB to fall over quite that violently. The behavior in a non-assert-enabled build wouldn't be very nice, either.) Fix it to do what other callers do, namely an actual runtime-test-and-elog. Also, standardize the wording of elog messages that are complaining about unexpected failure of systable_getnext. 90% of them say "could not find tuple for <object>", so make the remainder do likewise. Many of the holdouts were using the phrasing "cache lookup failed", which is outright misleading since no catcache search is involved.
  • Don't be so trusting that shm_toc_lookup() will always succeed. Given the possibility of race conditions and so on, it seems entirely unsafe to just assume that shm_toc_lookup() always finds the key it's looking for --- but that was exactly what all but one call site were doing. To fix, add a "bool noError" argument, similarly to what we have in many other functions, and throw an error on an unexpected lookup failure. Remove now-redundant Asserts that a rather random subset of call sites had. I doubt this will throw any light on buildfarm member lorikeet's recent failures, because if an unnoticed lookup failure were involved, you'd kind of expect a null-pointer-dereference crash rather than the observed symptom. But you never know ... and this is better coding practice even if it never catches anything. Discussion:
  • Code review for shm_toc.h/.c. Declare the toc_nentry field as uint32 not Size. Since shm_toc_lookup() reads the field without any lock, it has to be atomically readable, and we do not assume that for fields wider than 32 bits. Performance would be impossibly bad for entry counts approaching 2^32 anyway, so there is no need to try to preserve maximum width here. This is probably an academic issue, because even if reading int64 isn't atomic, the high order half would never change in practice. Still, it's a coding rule violation, so let's fix it. Adjust some other not-terribly-well-chosen data types too, and copy-edit some comments. Make shm_toc_attach's Asserts consistent with shm_toc_create's. None of this looks to be a live bug, so no need for back-patch. Discussion:
  • Docs: improve CREATE TABLE ref page's discussion of partition bounds. Clarify in the syntax synopsis that partition bound values must be exactly numeric literals or string literals; previously it said "bound_literal" which was defined nowhere. Replace confusing --- and, I think, incorrect in detail --- definition of how range bounds work with a reference to row-wise comparison plus a concrete example (which I stole from Robert Haas). Minor copy-editing in the same area. Discussion: Discussion:
  • Fix bit-rot in pg_upgrade's, and improve documentation. Doing a cross-version upgrade test with evidently hasn't been tested since circa 9.2, because the script lacked case branches for old-version servers newer than 9.1. Future-proof that a bit, and clean up breakage induced by our recent drop of V0 function call protocol (namely that oldstyle_length() isn't in the regression suite anymore). (This isn't enough to make the test work perfectly cleanly across versions, but at least it finishes and provides dump files that you can diff manually. One issue I didn't touch is that we might want to execute the "reindex_hash.sql" file in the new DB before dumping it, so that the hash indexes don't vanish from the dump.) Improve the TESTING doc file: put the tl;dr version at the top not the bottom, and bring its explanation of how to run a cross-version test up to speed, since the installcheck target isn't there and won't be resurrected. Improve the comment in the Makefile about why not. In passing, teach .gitignore and "make clean" about a couple more junk output files. Discussion:
  • Handle unqualified SEQUENCE NAME options properly in parse_utilcmd.c. generateSerialExtraStmts() was sloppy about handling the case where SEQUENCE NAME is given with a not-schema-qualified name. It was generating a CreateSeqStmt with an unqualified sequence name, and an AlterSeqStmt whose "owned_by" DefElem contained a T_String Value with a null string pointer in the schema-name position. The generated nextval() argument was also underqualified. This accidentally failed to fail at runtime, but only so long as the current default creation namespace at runtime is the right namespace. That's bogus; the parse-time transformation is supposed to be inserting the right schema name in all cases, so as to avoid any possible skew in that selection. I'm not sure this could fail in pg_dump's usage, but it's still wrong; we have had real bugs in this area before adopting the policy that parse_utilcmd.c should generate only fully-qualified auxiliary commands. A slightly lesser problem, which is what led me to notice this in the first place, is that pprint() dumped core on the AlterSeqStmt because of the bogus T_String. Noted while poking into the open problem with ALTER SEQUENCE breaking pg_upgrade.

Heikki Linnakangas pushed:

Andrew Dunstan pushed:

Andres Freund pushed:

  • Fix record length computation in pg_waldump/xlogdump. The current method of computing the record length (excluding the lenght of full-page images) has been wrong since the WAL format has been revamped in 2c03216d831160bedd72d45f712601b6f7d03f1c. Only the main record's length was counted, but that can be significantly too little if there's data associated with further blocks. Fix by computing the record length as total_lenght - fpi_length. Reported-By: Chen Huajun Bug: #14687 Reviewed-By: Heikki Linnakangas Discussion: Backpatch: 9.5-
  • Prevent possibility of panics during shutdown checkpoint. When the checkpointer writes the shutdown checkpoint, it checks afterwards whether any WAL has been written since it started and throws a PANIC if so. At that point, only walsenders are still active, so one might think this could not happen, but walsenders can also generate WAL, for instance in BASE_BACKUP and logical decoding related commands (e.g. via hint bits). So they can trigger this panic if such a command is run while the shutdown checkpoint is being written. To fix this, divide the walsender shutdown into two phases. First, checkpointer, itself triggered by postmaster, sends a PROCSIG_WALSND_INIT_STOPPING signal to all walsenders. If the backend is idle or runs an SQL query this causes the backend to shutdown, if logical replication is in progress all existing WAL records are processed followed by a shutdown. Otherwise this causes the walsender to switch to the "stopping" state. In this state, the walsender will reject any further replication commands. The checkpointer begins the shutdown checkpoint once all walsenders are confirmed as stopping. When the shutdown checkpoint finishes, the postmaster sends us SIGUSR2. This instructs walsender to send any outstanding WAL, including the shutdown checkpoint record, wait for it to be replicated to the standby, and then exit. Author: Andres Freund, based on an earlier patch by Michael Paquier Reported-By: Fujii Masao, Andres Freund Reviewed-By: Michael Paquier Discussion: Backpatch: 9.4, where logical decoding was introduced
  • Have walsenders participate in procsignal infrastructure. The non-participation in procsignal was a problem for both changes in master, e.g. parallelism not working for normal statements run in walsender backends, and older branches, e.g. recovery conflicts and catchup interrupts not working for logical decoding walsenders. This commit thus replaces the previous WalSndXLogSendHandler with procsignal_sigusr1_handler. In branches since db0f6cad48 that can lead to additional SetLatch calls, but that only rarely seems to make a difference. Author: Andres Freund Reviewed-By: Michael Paquier Discussion: Backpatch: 9.4, earlier commits don't seem to benefit sufficiently
  • Unify SIGHUP handling between normal and walsender backends. Because walsender and normal backends share the same main loop it's problematic to have two different flag variables, set in signal handlers, indicating a pending configuration reload. Only certain walsender commands reach code paths checking for the variable (START_[LOGICAL_]REPLICATION, CREATE_REPLICATION_SLOT ... LOGICAL, notably not base backups). This is a bug present since the introduction of walsender, but has gotten worse in releases since then which allow walsender to do more. A later patch, not slated for v10, will similarly unify SIGHUP handling in other types of processes as well. Author: Petr Jelinek, Andres Freund Reviewed-By: Michael Paquier Discussion: Backpatch: 9.2-, bug is present since 9.0
  • Wire up query cancel interrupt for walsender backends. This allows to cancel commands run over replication connections. While it might have some use before v10, it has become important now that normal SQL commands are allowed in database connected walsender connections. Author: Petr Jelinek Reviewed-By: Andres Freund, Michael Paquier Discussion:
  • Revert "Prevent panic during shutdown checkpoint". This reverts commit 086221cf6b1727c2baed4703c582f657b7c5350e, which was made to master only. The approach implemented in the above commit has some issues. While those could easily be fixed incrementally, doing so would make backpatching considerably harder, so instead first revert this patch. Discussion:
  • Clean up latch related code. The larger part of this patch replaces usages of MyProc->procLatch with MyLatch. The latter works even early during backend startup, where MyProc->procLatch doesn't yet. While the affected code shouldn't run in cases where it's not initialized, it might get copied into places where it might. Using MyLatch is simpler and a bit faster to boot, so there's little point to stick with the previous coding. While doing so I noticed some weaknesses around newly introduced uses of latches that could lead to missed events, and an omitted CHECK_FOR_INTERRUPTS() call in worker_spi. As all the actual bugs are in v10 code, there doesn't seem to be sufficient reason to backpatch this. Author: Andres Freund Discussion: Backpatch: -
  • Again report a useful error message when walreceiver's connection closes. Since 7c4f52409a8c (merged in v10), a shutdown master is reported as FATAL: unexpected result after CommandComplete: server closed the connection unexpectedly by walsender. It used to be LOG: replication terminated by primary server FATAL: could not send end-of-streaming message to primary: no COPY in progress while the old message clearly is not perfect, it's definitely better than what's reported now. The change comes from the attempt to handle finished COPYs without erroring out, needed for the new logical replication, which wasn't needed before. There's probably better ways to handle this, but for now just explicitly check for a closed connection. Author: Petr Jelinek Reviewed-By: Andres Freund Discussion: Backpatch: -
  • Use standard interrupt handling in logical replication launcher. Previously the exit handling was only able to exit from within the main loop, and not from within the backend code it calls. Fix that by using the standard die() SIGTERM handler, and adding the necessary CHECK_FOR_INTERRUPTS() call. This requires adding yet another process-type-specific branch to ProcessInterrupts(), which hints that we probably should generalize that handling. But that's work for another day. Author: Petr Jelinek Reviewed-By: Andres Freund Discussion:

Peter Eisentraut pushed:

Michael Meskes pushed:

Robert Haas pushed:

Joe Conway pushed:

Correctifs en attente

Jing Wang sent in a patch to add support for COMMENT ON ... CURRENT DATABASE.

Pavel Stěhule sent in another revision of a patch to implement \gdesc in psql.

Atsushi Torikoshi and Ashutosh Bapat traded patches to fix a typo in README.dependencies.

Tom Lane sent in a patch to retry shm attach for Windows.

Etsuro Fujita sent in another revision of a patch to adjust inherited update tlist.

Beena Emerson sent in another revision of a patch to implement default partition for range-partitioned tables.

Etsuro Fujita sent in a patch to update comments in nodeModifyTable.c.

Yura Sokolov sent in a patch to fix a performance degradation from contended LWLocks on NUMA.

Rahila Syed and Mithun Cy traded patches to implement auto_prewarm.

Amul Sul sent in another revision of a patch to implement hash partitioning.

Dang Minh Huong and Man Trieu traded patches to ensure that the unaccenting process actually ends in completely unaccented letters.

Andres Freund sent in a patch to clean up latch related code.

Tom Lane sent in a patch to log parallel worker start and stop.

Michaël Paquier sent in another revision of a patch to improve TAP test stability.

Thomas Munro sent in three more revisions of a patch to fix an infelicity between transition tables and wCTEs.

Jeevan Ladhe and Ashutosh Bapat traded patches to add support for default partitions.

Chapman Flack sent in a patch to PostgresNode to make get_new_node be able to be a class-method.

Michaël Paquier sent in another revision of a patch to fix race conditions with WAL sender PID lookups.

Kyotaro HORIGUCHI sent in a patch to install include/statistics directory on make install.

Amit Khandekar sent in two more revisions of a patch to update partition keys in partitioned tables.

Fabien COELHO sent in another revision of a patch to add pgbench TAP tests and minor fixes.

Michaël Paquier sent in a patch to implement pg_receivewal --endpos.

Amit Langote sent in a patch to implement fixes around partition constraint handling when attaching.

Tom Lane sent in two revisions of a patch to prohibit conditional SRF.

Thomas Munro sent in two more revisions of a patch to fix transition tuples from child tables.

Thomas Munro sent in two revisions of a patch to fix an infelicity between transition tables and ON CONFLICT.

Konstantin Knizhnik sent in another revision of a patch to implement projection functional indexes.

Jim Van Fleet sent in a patch to split ProcArrayLock into multiple parts.

Heikki Linnakangas sent in a patch to allow multiple hostaddrs to go with multiple hostnames.

Masahiko Sawada and Jeff Janes traded patches to add an "All Tables" column to psql's \dRp and \dRp+.

Vinayak Pokale sent in two revisions of a patch to add WHENEVER statement DO CONTINUE support to ECPG.

Erik Rijkers sent in a patch to improve some comments in tablesync.c.

Ashutosh Sharma sent in a patch to add ICU support on Windows.

Mark Rofail sent in a patch to put in some infrastructure for implementing foreign key arrays.

Álvaro Herrera sent in a patch to fix an earlier patch for translation-friendliness.

Dean Rasheed sent in a patch to give relation_is_updatable() the message about partitioned tables.

Christian Ullrich sent in a patch to work around the fact that the VS 2013 CRT's setlocale() function is not entirely thread-safe.

Tom Lane sent in a patch to fix sequence pg_upgrade.

Yugo Nagata sent in a patch to fix a documentation typo in ALTER TABLE example.

Thomas Munro sent in two revisions of a patch to fix some infelicities between named tuplestores and enrtuples.

Ashutosh Sharma sent in a patch to fix ICU collation on Windows.