wait_event_timing: reviewable patch series (stats / DSA / trace) - #2
Open
DmitryNFomin wants to merge 43 commits into
Open
wait_event_timing: reviewable patch series (stats / DSA / trace)#2DmitryNFomin wants to merge 43 commits into
DmitryNFomin wants to merge 43 commits into
Conversation
DmitryNFomin
force-pushed
the
wet-series
branch
2 times, most recently
from
June 11, 2026 15:16
86593a6 to
983f0ec
Compare
Owner
Author
|
Series reshaped into the 7-patch deliverable (for review before pgsql-hackers submission). The 10 development commits were redistributed into 7 submission-grade patches — the cleanup pass and the seqlock-test commit folded into their logical parents, and the trace docs/tests moved into the patches that introduce the code they cover: Verification: every intermediate stage was rebuilt and re-verified on the box (timing + stub builds, regress+isolation; the seqlock TAP test at 0006), and the final tree is bit-identical to the previously validated tip ( The old 10-commit history is preserved at branch |
Independent overhead benchmark — resultsRan an independent overhead measurement of this series on a dedicated 16-vCPU box (Hetzner CCX43, PG19beta1, Headline (honest version):
Validity checks that passed: the build is genuinely instrumented (compile gate verified; a stub built with the flag omitted used as a control), capture is genuinely exercised (12M live waits/65 backends, full 32-bucket histograms captured mid-run, trace ring filled), and Not yet covered (future work): a real LWLock-contention storm (LWLock was <1% of waits here), a write/WAL workload, and a true >RAM IO-bound regime. Happy to run these next. Net: the low-overhead design holds up — |
DmitryNFomin
force-pushed
the
wet-series
branch
2 times, most recently
from
July 3, 2026 21:28
fc378b1 to
89392c1
Compare
3 tasks
Sequence synchronization requires the page_lsn field returned by pg_get_sequence_data(), which was added in PostgreSQL 19. Previously, requesting sequence synchronization against an older publisher (via ALTER SUBSCRIPTION ... REFRESH SEQUENCES or by running ALTER SUBSCRIPTION ... CONNECTION on a disabled subscription with sequences in the INIT state and subsequently enabling the subscription) would cause the sequence synchronization worker to repeatedly fail with a confusing "invalid query response" error. Check the publisher's server version up front in both AlterSubscription_refresh_seq() and copy_sequences(), and error out immediately when it predates PostgreSQL 19. Also document the PostgreSQL 19 publisher requirement for sequence replication in the logical replication documentation and in ALTER SUBSCRIPTION ... REFRESH SEQUENCES. Reported-by: Noah Misch <noah@leadboat.com> Author: vignesh C <vignesh21@gmail.com> Reviewed-by: Shveta Malik <shveta.malik@gmail.com> Reviewed-by: Hayato Kuroda <kuroda.hayato@fujitsu.com> Reviewed-by: Amit Kapila <amit.kapila16@gmail.com> Backpatch-through: 19 Discussion: https://postgr.es/m/20260710045217.f0.noahmisch@microsoft.com
injection_wait() only clears its slot in the waiter array after the wait loop finishes. When the waiting query is canceled or the backend is terminated (wait look has a CHECK_FOR_INTERRUPS), the slot leaks. Later wakeups of the same point then bump the counter of the leaked slot instead of the real waiter, that sleeps forever. Repeated leaks can exhaust all the slots. The code is changed so as the waiting loop is wrapped with PG_ENSURE_ERROR_CLEANUP, so as the injection point slots, that are shared resources, can be cleaned up on ERROR as much as a FATAL. An isolation test is added: cancel one waiter, terminate another waiter, then check that a later waiter still receives a wakeup. Without the fixed code, the test would fail on timeout. Author: Zsolt Parragi <zsolt.parragi@percona.com> Discussion: https://postgr.es/m/CAN4CZFO+KF=cc0-iEg28RhqRBp_fTs6D4b8b7D7DB-pGYP3Ccg@mail.gmail.com Backpatch-through: 17
The documentation of pg_stat_activity used an incomplete list of values for backend_type. While on it, it is improved to use an itemized list, now ordered alphabetically, with a short description about each item. Author: Laurenz Albe <laurenz.albe@cybertec.at> Reviewed-By: Michael Paquier <michael@paquier.xyz> Reviewed-By: Fujii Masao <masao.fujii@gmail.com> Discussion: https://postgr.es/m/5e94c0196084f648ae6a00107125494f5804318a.camel@cybertec.at
socket_putmessage_noblock() used pq_putmessage(), which redirects to PqCommMethods->putmessage. In the common cases, this points to socket_putmessage(), but it would become incorrect if PqCommMethods points to a different implementation. This change may look like a bug, but as far as I can see this is mostly cosmetic. The code is able to work currently, as the repalloc() done in the noblock() call ensures that the blocking path of internal_putbytes() is never reached. The issue has gone unnoticed since 2bd9e41. Author: Anthonin Bonnefoy <anthonin.bonnefoy@datadoghq.com> Reviewed-by: Fujii Masao <masao.fujii@gmail.com> Discussion: https://postgr.es/m/CAO6_Xqpf5+Rzw_-XOOz-d-R5x6_2JHtpnzXP0nrYWiHyZokA_Q@mail.gmail.com
Improve the documentation for pg_stat_recovery in several ways: - Mention the view in high-availability.sgml as a way to monitor recovery state and replay progress, alongside the existing recovery information functions. - Clarify that the view returns at most one row, not exactly one row, and no rows to users who lack the pg_read_all_stats privilege. - Correct the description of last_replayed_end_lsn to clarify that it is the end LSN of the last replayed record plus one. - Document that replay_end_tli equals last_replayed_tli when no WAL record is currently being replayed. - Clarify that current_chunk_start_time is NULL until streaming WAL has been received. Backpatch to v19, where pg_stat_recovery was introduced. Author: Fujii Masao <masao.fujii@gmail.com> Reviewed-by: Michael Paquier <michael@paquier.xyz> Discussion: https://postgr.es/m/CAHGQGwGRavm18HqnQn_f68QB96qk6arhjET1V93OJH09Mgojkg@mail.gmail.com Backpatch-through: 19
For subscriptions using SERVER, changing the owner can change the effective connection string. However, ALTER SUBSCRIPTION ... OWNER TO did not validate the generated conninfo for the new owner. As a result, ownership could be transferred to a non-superuser whose generated connection string did not satisfy password_required=true. The ownership change succeeded, but the subscription would fail later when the worker or another command tried to connect. Fix this by making ALTER SUBSCRIPTION ... OWNER TO validate the new owner's generated conninfo with walrcv_check_conninfo(). Backpatch to v19, where SERVER subscriptions were introduced. Author: Fujii Masao <masao.fujii@gmail.com> Reviewed-by: Yuanchao Zhang <145zhangyc@gmail.com> Reviewed-by: Hayato Kuroda <kuroda.hayato@fujitsu.com> Discussion: https://postgr.es/m/CAHGQGwFGa6+wWVgUmZPFwN=fBY59mYPkMK3=TxT=Pv5C1mNNRQ@mail.gmail.com Backpatch-through: 19
Commit fd36606 added tests intended to verify that rows inserted on the publisher are replicated to the subscriber when using multiple publications, with one excluding the target table via EXCEPT and another including it. However, the tests queried the publisher instead of the subscriber. Since the rows were inserted directly into the publisher, the checks would always succeed, providing no coverage of replication. Fix this by querying the subscriber so the tests verify the replicated state. Author: Fujii Masao <masao.fujii@gmail.com> Reviewed-by: Ayush Tiwari <ayushtiwari.slg01@gmail.com> Reviewed-by: Amit Kapila <amit.kapila16@gmail.com> Discussion: https://postgr.es/m/CAHGQGwGfXUO7f4t6KNGurYwg6QsnLtpP0K3EACbAwYWtxGfKfQ@mail.gmail.com Backpatch-through: 19
Document table_name, column_name, and schema_name in the CREATE PUBLICATION and ALTER PUBLICATION reference pages. Also add anchors for the ALTER PUBLICATION parameter list, matching the style already used by CREATE PUBLICATION. Author: Peter Smith <smithpb2250@gmail.com> Reviewed-by: Fujii Masao <masao.fujii@gmail.com> Discussion: https://postgr.es/m/CAHut+Ptekz+TO4ui8-fiBm4Y+O2v=HQnkK_cW4G=w9ep8654EA@mail.gmail.com
Previously, if a sequence synchronization batch contained both a sequence that had been dropped on the publisher and another for which the replication role lacked SELECT privilege, the latter was reported twice: once as a permission failure and again as missing on the publisher. This happened because the permission-denied sequence was not marked as found on the publisher. As a result, when another sequence in the batch was genuinely missing, the later missing-sequence check incorrectly classified the permission-denied sequence as missing as well. Fix this by marking the permission-denied sequence as found before reporting the permission failure, so it is not later reported as missing. Reported-by: Noah Misch <noah@leadboat.com> Author: Vignesh C <vignesh21@gmail.com> Reviewed-by: Fujii Masao <masao.fujii@gmail.com> Discussion: https://postgr.es/m/CALDaNm3LsUjW7PahuCsbYAxajSF+S328tw5E9rF0erdh7dKOXw@mail.gmail.com Backpatch-through: 19
pg_database_size() allows access to users who have either CONNECT
privilege on the target database or privileges of the pg_read_all_stats
role. However, previously, psql's \l+ checked only for CONNECT,
so users with privileges of pg_read_all_stats still saw "No Access" for
databases they could not connect to.
Fix this by making \l+ also check
pg_has_role('pg_read_all_stats', 'USAGE'), matching
pg_database_size()'s permission rules.
For back branches, emit the pg_read_all_stats check only when
connected to PostgreSQL 10 or later, since earlier releases do not have
that predefined role.
Backpatch to all supported versions.
Author: Christoph Berg <myon@debian.org>
Reviewed-by: Álvaro Herrera <alvherre@kurilemu.de>
Reviewed-by: Fujii Masao <masao.fujii@gmail.com>
Discussion: https://postgr.es/m/amCo6qRmnfPVk4-V@msg.df7cb.de
Backpatch-through: 14
Commit f9b7fc6 fixed a race when predicate-locking completely empty btrees: without a buffer lock held, a matching key could be inserted between _bt_search and the PredicateLockRelation call, so the scan would miss concurrently inserted tuples while the writer wouldn't see the reader's predicate lock. That commit only fixed _bt_first's _bt_search path, though. Scans without useful insertion scan keys return early from _bt_first via _bt_endpoint, which still didn't recheck if the relation was empty. To fix, add handling to _bt_endpoint that is analogous to the handling added to _bt_search by commit f9b7fc6. Author: Peter Geoghegan <pg@bowt.ie> Discussion: https://postgr.es/m/CAH2-WzkNoTn3yXY0iGkSuavJ+sL8EROf+kitW+_2v2tJVWuKmA@mail.gmail.com Backpatch-through: 14
Oversight in commit fb23cc7. Reported-by: Anton Voloshin <a.voloshin@postgrespro.ru> Discussion: https://postgr.es/m/ad5d772e-09d9-4248-97a4-0011afab9e71@postgrespro.ru
Add coverage for predicate locking of completely empty nbtree indexes, where we must predicate lock the entire relation (instead of some individual leaf page). Both paths that can find the index empty (and must consider whether it's still empty after PredicateLockRelation returns) are covered by a new isolation test that uses injection points. Catalog relation scans skip the injection points. The waiting session runs catalog queries of its own after arming the (session-local) points, and could otherwise suspend itself with nothing lined up to wake it. Follow-up to bugfix commits ce3f19e (the _bt_endpoint fix) and f9b7fc6 (the _bt_first/_bt_search fix). Author: Peter Geoghegan <pg@bowt.ie> Discussion: https://postgr.es/m/CAH2-WzkNoTn3yXY0iGkSuavJ+sL8EROf+kitW+_2v2tJVWuKmA@mail.gmail.com
Backwards scans have unique concurrency rules: rather than unreservedly trusting a saved left link, the scan optimistically rechecks its pointed-to leaf page's right link (i.e. whether it still points back to the page that _bt_readpage just read). Usually, the left sibling of the just-read page won't have changed, in which case the scan can proceed with reading the left sibling as planned. But it's possible that the key space that the scan needs to read next is no longer covered by the original left sibling page due to concurrent page splits and/or page deletions. When that happens, the scan must recover by relocating the new/current left sibling of the just-read page. Test coverage for backwards scans was limited to the happy path. Add an isolation test (and associated injection points) that test the recovery path. This covers several distinct recovery scenarios (concurrent page splits, concurrent page deletions, and minor variants thereof). Author: Peter Geoghegan <pg@bowt.ie> Reviewed-by: Andrey Borodin <x4mmm@yandex-team.ru> Discussion: https://postgr.es/m/CAH2-WzmD+jUBOpFS2jrnqqrdPSAjoxqyL9FPKaE1BtnY=8Nntg@mail.gmail.com
Add pg_regress tests that exercise the row compare logic that commit 7d9cd2d added to _bt_set_startikey. Also add tests that exercise the _bt_set_startikey SAOP array path. Author: Peter Geoghegan <pg@bowt.ie> Discussion: https://postgr.es/m/CAH2-Wz=KjQsD2W2a=b51uH905=0mF6Le4evhWkN2FL1+uRPhUg@mail.gmail.com Backpatch-through: 19
This replaces an O(N) hash_seq_search() loop by an O(1) lookup, removing a TODO item, making the invalidation callback faster when dealing with many relations. This can work because LogicalRepPartMap is keyed by a partition OID, and a relmapentry's localreloid matches with it. An assertion is added in logicalrep_partition_open() to enforce the fact that localreloid matches with the hash key. Author: DaeMyung Kang <charsyam@gmail.com> Discussion: https://postgr.es/m/20260417174450.4158878-1-charsyam@gmail.com
Noticed while doing some routine work. Oversight in e395fbd.
Commit 8d829f5 introduced the JSCTOR_JSON_ARRAY_QUERY constructor type so that ruleutils.c could deparse JSON_ARRAY(subquery) using its original syntax, storing the transformed subquery in a new orig_query field. However, the input FORMAT clause of JSON_ARRAY(subquery FORMAT ...) was not preserved for deparsing. The format was recorded only in the executable expression kept in the func field, which ruleutils.c does not inspect, so it is silently dropped. This is more than cosmetic, because FORMAT JSON changes the result: without it a text value is treated as a string to be quoted, while with it the value is treated as already-formatted JSON. To fix, record the input FORMAT in a new deparse-only field of JsonConstructorExpr, alongside orig_query, and emit it in ruleutils.c. Bump catalog version. Author: Chao Li <li.evan.chao@gmail.com> Reviewed-by: Ewan Young <kdbase.hack@gmail.com> Reviewed-by: Richard Guo <guofenglinux@gmail.com> Discussion: https://postgr.es/m/4C89B193-7D54-4705-9CF9-F0D484B9E099@gmail.com Backpatch-through: 19
1. Stop a running sequence synchronization worker when ALTER SUBSCRIPTION ... DISABLE is executed. The worker did not reread its subscription after starting a transaction, so it kept running with a stale copy and missed the disable. It now calls maybe_reread_subscription() after StartTransactionCommand(), matching the apply worker. 2. Restore the invariant that publisher-side synchronization slots are dropped last during ALTER SUBSCRIPTION ... REFRESH PUBLICATION. The slot-drop loop now runs after the sequence-removal loop, so the non-transactional slot drops happen only after all catalog changes that could still be rolled back on error. 3. Restore psql tab completion for ALTER SUBSCRIPTION ... REFRESH PUBLICATION WITH (. 4. Make pg_stat_subscription report NULL for the fields that do not apply to a sequence synchronization worker, which does not stream from a walsender, and update the documentation accordingly. 5. Update the pg_subscription_rel.srsublsn catalog documentation to describe its semantics for sequence rows. Reported-by: Noah Misch <noah@leadboat.com> Author: vignesh C <vignesh21@gmail.com> Reviewed-by: Hayato Kuroda <kuroda.hayato@fujitsu.com> Reviewed-by: Amit Kapila <amit.kapila16@gmail.com> Backpatch-through: 19, where it was introduced Discussion: https://postgr.es/m/20260710045217.f0.noahmisch@microsoft.com
Offset 0 is the "invalid" marker in pg_multixact/offsets since offsets
went 64-bit and the allocator stopped skipping it. pg_resetwal could
still produce it via -O 0 or guessed control values, breaking the first
multixact created after the reset ("MultiXact n has invalid offset",
and vacuum of the affected table fails from then on). Reject -O 0 like
-m and -o already do, and guess 1 like initdb does.
Author: Zsolt Parragi <zsolt.parragi@percona.com>
Discussion: https://www.postgresql.org/message-id/CAN4CZFNoO6MUkg526TmA=mC_RjY2gp4VKCnvK6y12v3ppOkhJA@mail.gmail.com
Backpatch-through: 19
Commit 8e72d91 recorded the range column's name in ForPortionOfExpr and used that for deparsing FOR PORTION OF. This gives the wrong answer if the ForPortionOfExpr is saved in a rule or SQL function and then the column gets renamed. Drop the ForPortionOfExpr.range_name field; instead fetch the current column name from the catalogs when needed. Also drop ForPortionOfState.fp_rangeName, which wasn't being used anywhere. Full disclosure: an earlier draft of this patch was made with Claude Opus 4.8. Reported-by: John Naylor <johncnaylorls@gmail.com> Author: Tom Lane <tgl@sss.pgh.pa.us> Reviewed-by: Richard Guo <guofenglinux@gmail.com> Reviewed-by: Chao Li <li.evan.chao@gmail.com> Discussion: https://postgr.es/m/CANWCAZYFEpJ5Oi45gi4q9Y6LYa4_oiAXxuNNWe-1ym-i0fF8Pw@mail.gmail.com Backpatch-through: 19
With wal_level = 'replica', logical decoding is enabled on demand when the first logical replication slot is created: When enabling logical decoding, EnableLogicalDecoding() flips the shared logical_decoding_enabled flag and writes an XLOG_LOGICAL_DECODING_STATUS_CHANGE record so that standbys follow the status change. The initial "already enabled?" check and the WAL record write happen under two separate acquisitions of LogicalDecodingControlLock, since the lock must be released while waiting for the ProcSignalBarrier: processes absorbing the barrier acquire the same lock in shared mode. Consequently, if two backends concurrently created the first logical slots, both could pass the initial check and both write a status-change record. The redundant record lands after the decoding start point already reserved by the other backend's slot, so decoding that slot processes the record and fails with "unexpected logical decoding status change", as xlog_decode() assumes that no such record can appear within the WAL range any slot decodes. Fix by re-checking the status after re-acquiring the lock, so that only the backend that actually performs the disabled->enabled transition writes the WAL record. Reported-by: Srinath Reddy Sadipiralla <srinath2133@gmail.com> Author: Srinath Reddy Sadipiralla <srinath2133@gmail.com> Reviewed-by: Masahiko Sawada <sawada.mshk@gmail.com> Reviewed-by: Amit Kapila <amit.kapila16@gmail.com> Discussion: https://postgr.es/m/CAFC+b6oYzmAgp7F0ivrhfZT46-CjvCTrU9pWuMNcem-52YjOTw@mail.gmail.com Backpatch-through: 19
Commit 181b618 failed to do anything useful with a whole-row Var, deeming it "fishy". But it is legal to put such a Var into an expression index column, so let's expand it as the name of the table. Another problem reachable via that one is that we could generate an empty index column name, which isn't really legal although by chance nothing complained about it. It's not clear whether any other such cases remain, but as cheap insurance let's use "expr" if the tree walk fails to generate any text. Reported-by: Chauhan Dhruv <chauhandhruv351@gmail.com> Author: Chauhan Dhruv <chauhandhruv351@gmail.com> Co-authored-by: Tom Lane <tgl@sss.pgh.pa.us> Discussion: https://postgr.es/m/CANWwWcp_DCJjq8pomeqp6W=fbygvzXXQO028VDJ9_6sLPjQnVA@mail.gmail.com
index_create_copy is used to create copy definitions of existing indexes. Currently, it passes 0 as constr_flags to index_create(), which results in the copied index to always be created as immediate (indimmediate set to true). For deferrable unique constraints, it means that the transient index used during the phase 2 of REINDEX CONCURRENTLY forces immediate constraint checks on concurrent inserts, which can cause unexpected constraint violations based on the definition of the parent table, inconsistently set in the copied index. To fix this without violating the contract of constr_flags (which should only be used when creating constraints) and without relaxing the strict assertion in index_create(), this introduces a new index creation flag: INDEX_CREATE_DEFERRABLE. If set, a copied index's indimmediate is set to false, meaning that unique constraints are not enforced immediately on insertion, but at transaction commit time. An isolation test for REINDEX CONCURRENTLY is added, based on an injection point waiting after phase 1 of the operation, where an index copy has been built and is able to accept DMLs for its validation in phase 2. The test is tentatively backpatched down to v17. INJECTION_POINT() is outside a transaction context, which should be fine on HEAD since 8daeaa9 but I suspect may cause issues in v19 and older branches due to the wait facility depending on condition variables and a DSM setup, but let's see what the buildfarm tells. Author: Nitin Motiani <nitinmotiani@google.com> Discussion: https://postgr.es/m/CAH5HC97JmjPpgiQOqW9xm8qXhNiu7zZ1Qh+FfhEESJuDv69kuQ@mail.gmail.com Backpatch-through: 14
The mapped user name is built upon the OS user name of the environment where the test is run. Depending on the characters used in the OS user name, CREATE ROLE may not get parsed (the author has mentioned hyphens as one case), causing a failure of the test. Let's use double-quotes around the mapped user name, which should be a solution good enough for the environments where this test tends to run. The buildfarm issued no complaint over the years. Oversight in 3c4e26a, so backpatch down to v19. Perhaps 3c4e26a and this commit should be backpatched further down, but let's leave that for another day, if it proves necessary. Author: Yugo Nagata <nagata@sraoss.co.jp> Discussion: https://postgr.es/m/20260727133857.fbd23d43d422f10f376a8bee@sraoss.co.jp Backpatch-through: 19
UPDATE/DELETE ... FOR PORTION OF inserts leftover rows for the untouched parts of the original row. These hidden inserts should not affect the command tag or ROW_COUNT, so they call ExecInsert() with canSetTag set to false. However, ExecInsert() still processed the RETURNING list whenever the target ResultRelInfo had ri_projectReturning set. That caused RETURNING expressions to be evaluated for leftover rows even though their results were discarded. As a result, expressions with side effects and information-leaking functions could be executed on the leftover rows, in addition to the visibly updated or deleted row. Fix by having ExecInsert() skip RETURNING processing when it is handling an internal FOR PORTION OF leftover insert. Use both the presence of a FOR PORTION OF clause and mtstate->operation == CMD_INSERT for this check, so that the auxiliary INSERT of a cross-partition UPDATE with a FOR PORTION OF clause still processes RETURNING normally. Back-patch to v19, where support for FOR PORTION OF was added. Author: Chao Li <lic@highgo.com> Reviewed-by: Dean Rasheed <dean.a.rasheed@gmail.com> Reviewed-by: Paul A Jungwirth <pj@illuminatedcomputing.com> Discussion: https://postgr.es/m/07C125E5-F6ED-460C-A394-E6503DAE18FB@gmail.com Backpatch-through: 19
Commit fd83c83 turned the recursive posting-tree cleanup in ginVacuumPostingTreeLeaves() into an iterative sweep that follows the tree's leaf pages via their rightlinks. The recursive version called vacuum_delay_point() while processing the tree, but that call was removed and never re-added to the new loop. As that commit only set out to fix a deadlock, the removal appears to have been unintentional. Consequently the leaf-page sweep of a single posting tree runs with no vacuum_delay_point(), and therefore no CHECK_FOR_INTERRUPTS(). A posting tree stores all the TIDs for one indexed key, so for a frequently occurring key it can span a large number of leaf pages. While such a tree is being vacuumed the operation ignores vacuum_cost_delay and does not respond to query cancellation or statement_timeout; an autovacuum worker likewise cannot be interrupted mid-sweep when another backend requests a conflicting lock. Restore the call, placed after the current page has been unlocked and released so that no buffer content lock is held across a potential delay (cf. 21c27af). The sibling loops in ginbulkdelete() and ginvacuumcleanup() already call vacuum_delay_point() once per page. Author: Paul Kim <mok03127@gmail.com> Co-authored-by: Alexander Korotkov <aekorotkov@gmail.com> Reviewed-by: Michael Paquier <michael@paquier.xyz> Reviewed-by: Andrey Borodin <x4mmm@yandex-team.ru> Reviewed-by: solai v <solai.cdac@gmail.com> Discussion: https://postgr.es/m/178447127453.110.12276981925360691905%40mail.gmail.com Backpatch-through: 14
pg_get_publication_tables() collects the OIDs of the published tables on its first call, without locking them, and then reopens each table later, once per result row, to compute its column list and fetch its row filter. The reopen used table_open(), which errors out with "could not open relation with OID" if the table has been dropped in the meantime. This could happen for any published table without an explicit column list, which is every table in FOR ALL TABLES and FOR TABLES IN SCHEMA publications, but also FOR TABLE entries without a column list. The failure is common in environments where many tables are created and dropped while publication tables are being queried, e.g. by table synchronization on a subscriber. Fix by opening every table with try_table_open(), which returns NULL if the relation no longer exists, and skipping the table in that case. Concurrently dropped tables are thus simply absent from the result set, which is the expected point-in-time behavior. As a side effect, tables with an explicit column list, which were previously returned without being opened, are now also locked with AccessShareLock, so the function can block behind concurrent DDL on such tables where it previously did not. Backpatch to v16, where we added the table_open() call in pg_get_publication_tables(). Author: Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com> Reviewed-by: Bertrand Drouvot <bertranddrouvot.pg@gmail.com> Reviewed-by: shveta malik <shveta.malik@gmail.com> Reviewed-by: Ajin Cherian <itsajin@gmail.com> Reviewed-by: Masahiko Sawada <sawada.mshk@gmail.com> Reviewed-by: Chao Li <li.evan.chao@gmail.com> Discussion: https://www.postgresql.org/message-id/CALj2ACVYYooWH-5tJ6cPKkU%2BmutVxwb_z4S%2BqAi-zdrFqxXE2Q%40mail.gmail.com Backpatch-through: 16
A two-phase transaction that is assigned an XID but produces no change to be decoded -- for example, one that only acquires row locks via SELECT ... FOR SHARE -- has no base snapshot in the reorder buffer. ReorderBufferReplay() already skips such a transaction at PREPARE time and never invokes the begin_prepare/change/prepare callbacks for it, but ReorderBufferFinishPrepared() still called the commit_prepared (or rollback_prepared) callback. As a result a spurious COMMIT/ROLLBACK PREPARED was sent to the output plugin with no preceding PREPARE. For the built-in subscriber this breaks replication (the apply worker fails to find the prepared transaction), and test_decoding could even crash. Fix this by detecting an empty transaction (base_snapshot == NULL) in ReorderBufferFinishPrepared() and cleaning it up without invoking the commit/rollback prepared callbacks, mirroring the existing empty transaction handling in ReorderBufferReplay(). On v18 and newer versions, commit 072ee84 changed ReorderBufferPrepare() to send the prepare whenever it had not already been sent, which also fires for empty transactions and emits a spurious PREPARE. On those branches ReorderBufferPrepare() is therefore additionally guarded with base_snapshot != NULL. This guard and the Assert(!rbtxn_sent_prepare()) added in ReorderBufferFinishPrepared(), are not necessary on v17 and older versions: there ReorderBufferPrepare() only sends a prepare for concurrently-aborted transactions (which never applies to an empty transaction) and the RBTXN_SENT_PREPARE flag does not exist. Back-patch to v14, where decoding of two-phase transactions was introduced. Bug: #19556 Reported-by: Alexander Kozhemyakin <a.kozhemyakin@postgrespro.ru> Reviewed-by: Amit Kapila <amit.kapila16@gmail.com> Discussion: https://postgr.es/m/19556-daa6d7ea65054d48@postgresql.org Backpatch-through: 14
The file_copy strategy check in createdb() runs during option validation, before the transaction has an XID and before the pg_database row exists, so the datachecksumsworker launcher can start in that window and see neither the new database nor the transaction creating it. It then raw-copies a template that was not processed yet, and those files stay unchecksummed, failing verification from then on. Recheck the state in CreateDatabaseUsingFileCopy(): the XID is assigned by then, so a launcher starting after this point waits for the transaction and finds the new database, and the copy errors out instead. Add an injection point before the catalog insert to test the window. Backpatch to v19 where online checksums were introduced. Author: Zsolt Parragi <zsolt.parragi@percona.com> Reviewed-by: Daniel Gustafsson <daniel@yesql.se> Discussion: https://postgr.es/m/CAN4CZFPEBsz8JeY4ixQ1V4ZL_xOY6pJaZS8ZLGH7R+wF--pEtg@mail.gmail.com Backpatch-through: 19
Enable errors out early with a hint when an invalid database exists, since the worker cannot connect to it and its files stay on disk. A worker that started but failed gets the same dropped-database heuristic as one that failed to start, so a concurrent drop during processing no longer aborts the whole run. The existence check locks the database first, otherwise a DROP DATABASE ... WITH (FORCE) which killed the worker is still only halfway done and the database looks like it is there to stay. Backpatch to v19 where online checksums were introduced. Author: Zsolt Parragi <zsolt.parragi@percona.com> Reviewed-by: Daniel Gustafsson <daniel@yesql.se> Discussion: https://postgr.es/m/CAN4CZFOGdqxtZ5-6gb4apqmvoH=Z+TNH8RKJ3mVtoR1HirKQWg@mail.gmail.com Backpatch-through: 19
find_nonnullable_rels and find_nonnullable_vars mistakenly treated a ScalarArrayOpExpr that could return FALSE as strict, but that's okay only at top level of a qual expression; further down, we've got to insist on a guaranteed-NULL result. The result was that we could draw mistaken conclusions about whether outer joins can be simplified, if the decision hinged on a non-top-level ScalarArrayOpExpr with a potentially-empty array argument. I believe this error dates to commit 72a070a, which taught find_nonnullable_rels to descend into non-top-level parts of qual expressions. is_strict_saop (added earlier by 72153c0) already had enough intelligence to do the case correctly, but it wasn't passed the proper flag, ie "top_level" needs to be passed for "falseOK". e006a24 copied that mistake into find_nonnullable_vars. Later, over-eager refactoring in commit 2f153dd broke contain_nonstrict_functions' handling of ScalarArrayOpExpr by treating it as though it were no different from an OpExpr. It is, because we must also prove the array is non-empty before concluding that the expression is strict. This could result in misclassifying an expression as strict when it is not, leading to assorted planning mistakes such as inlining a SQL function that shouldn't be inlined. We can almost fix this by just re-adding the previous handling of ScalarArrayOpExpr in that function, but doing only that would lead to also calling check_functions_in_node() and thus redundantly checking the operator's strictness. Avoid that by turning the if-series into an else-if chain, as it arguably should have been all along. The reason these errors have escaped detection for decades is that they are exposed only in arcane corner cases. ScalarArrayOpExpr with an empty array isn't typical usage, and even when that's possible several other conditions apply before the planner can reach a mistaken conclusion. While it's possible to build test cases demonstrating these mistakes, I (tgl) judged them too indirect and special-purpose to justify consuming regression test cycles forevermore. Author: Ayush Tiwari <ayushtiwari.slg01@gmail.com> Reviewed-by: Tom Lane <tgl@sss.pgh.pa.us> Discussion: https://postgr.es/m/CAJTYsWV3vqRJmST-gv1NsXEef-zOnjVJpYS910aBaiuMij4nFg@mail.gmail.com Discussion: https://postgr.es/m/CAJTYsWWcLGmz0f8_QPP_Liq-fc7-geiFSCdqoq3XGeRHPPsWeA@mail.gmail.com Backpatch-through: 14
This commit replaces two calls of strcpy() and one call of strncpy() to use strlcpy(), which are patterns that static analyzers (mostly LLMs, it seems) have been complaining regarding buffer overflow risks. The existing calls are safe, here are more details for each one of them: - MarkAsPreparingGuts()'s strcpy() was guarded by MarkAsPreparing(). - PrepareRedoAdd()'s strcpy() is safe because the record-level CRC check prevents corrupted data from reaching it unless intentionally crafted. The replay code also assumes that the GID is within the allowed bounds, as WAL records are trusted. - Similarly, ParsePrepareRecord() stores its GID in a buffer bounded by GIDSIZE while trusting the length provided by the record. As a result, these changes are purely cosmetic. They adopt a more defensive coding style and should also silence some of the static analysis reports received recently. Author: Matt Suiche <matt@tolmo.com> Discussion: https://postgr.es/m/CAGf6Lfx2kbQfcEnCi99V2i65JSWD6ij_E29F+UkY=TyMUyeG6A@mail.gmail.com
While collecting the sequences to synchronize, the sequence sync worker opened each INIT sequence with RowExclusiveLock and held it until the transaction committed. With many such sequences, this could exhaust the shared lock table and fail with "out of shared memory". The worker only reads each sequence's identity (namespace and name) here and needs it to stay stable while read, for which AccessShareLock is enough, as it conflicts with the AccessExclusiveLock taken by DROP, RENAME, and SET SCHEMA. Take that lock instead and release it as soon as the identity is read. The later synchronization re-opens each sequence, so it does not rely on the lock being retained. Reported-by: Noah Misch <noah@leadboat.com> Author: vignesh C <vignesh21@gmail.com> Reviewed-by: Hayato Kuroda <kuroda.hayato@fujitsu.com> Reviewed-by: Amit Kapila <amit.kapila16@gmail.com> Backpatch-through: 19, where it was introduced Discussion: https://postgr.es/m/20260710045217.f0.noahmisch@microsoft.com
TerminateBackgroundWorkersForDatabase() uses BackendPidGetProc() and, until now, accessed fields of the returned PGPROC after releasing ProcArrayLock, including its database OID. If the PGPROC slot is recycled during this window, the database OID being checked may belong to a different backend, causing an unrelated background worker to be terminated. Triggering this bug requires a very narrow race: the background worker identified by BackendPidGetProc() must exit, its PGPROC slot must be released and reused, and only then must TerminateBackgroundWorkersForDatabase() examine the database OID. TerminateBackgroundWorkersForDatabase() holds BackgroundWorkerLock, preventing parallel workers and dynamically registered workers (such as those created by worker_spi) from reusing the slot. As far as I know, the only plausible scenario is a static background worker that exits and is restarted quickly enough to reuse the same PGPROC slot within the race window. In practice, this race is extremely unlikely, still reachable in theory. Oversight in f1e251b. Author: Chao Li <li.evan.chao@gmail.com> Reviewed-by: Aya Iwata <iwata.aya@fujitsu.com> Reviewed-by: Haibo Yan <tristan.yim@gmail.com> Discussion: https://postgr.es/m/78E81763-EA1D-4788-9741-4092BCB997A5@gmail.com Backpatch-through: 19
The getdatabaseencoding function was added in bf00bbb in 1998 but was never documented. While mostly used in tests, there is no reason not to document it as this function isn't going anywhere and is already used in extensions. Author: Ian Barwick <barwick@gmail.com> Reviewed-by: Thom Brown <thom@linux.com> Reviewed-by: surya poondla <suryapoondla4@gmail.com> Reviewed-by: Daniel Gustafsson <daniel@yesql.se> Discussion: https://postgr.es/m/CAB8KJ=ij+pznQGub=DkyJuKL=tC=Q=07qSahTyw7TLb0DdNJsg@mail.gmail.com
…_capture GUC Introduce the compile-time option --enable-wait-event-timing (meson: -Dwait_event_timing=true) defining USE_WAIT_EVENT_TIMING, and the runtime GUC wait_event_capture (PGC_SUSET, enum off|stats, default off). This commit is scaffolding only: it wires the flag through both build systems and registers the GUC with check/assign hooks, but adds no instrumentation yet -- later commits in this series add the recording hot path, the SQL surface, and the trace level. In builds compiled without --enable-wait-event-timing, the GUC's check hook rejects any value other than off (downgrading to off with a warning for non-interactive sources), so the variable exists uniformly for tooling but cannot be enabled.
…vel)
Implement wait_event_capture = stats. When enabled, every transition
through pgstat_report_wait_start()/pgstat_report_wait_end() records the
wait duration and accumulates per-(backend, event) statistics -- count,
total and maximum duration, and a 32-bucket log2 duration histogram -- in
shared memory.
Storage is a per-backend slot array in the main shared memory segment,
sized at postmaster start from wait_event_timing_max_tranches. Because
each slot lives for the entire life of its backend, the hot path needs no
lazy attach and no teardown gating: pgstat_report_wait_start_timing()
records a timestamp and the current event, and
pgstat_report_wait_end_timing() computes the duration and accumulates.
Each backend writes only to its own slot, so no locking is required and
the SRF reader is lock-free.
The inline gate in pgstat_report_wait_start()/_end() is a single load of
wait_event_capture plus a branch, with the bodies kept out-of-line so the
many inlined call sites stay compact; while capture is off the hot path
adds only that branch.
Non-LWLock wait events map to a flat array via a class table generated
from wait_event_names.txt by generate-wait_event_types.pl. LWLock events,
whose tranche ids are unbounded, use a per-backend open-addressing hash
capped by wait_event_timing_max_tranches (PGC_POSTMASTER, default 192).
SQL surface:
- pg_stat_get_wait_event_timing(pid) and the pg_stat_wait_event_timing
view, one row per backend per event with a non-zero count;
- pg_wait_event_timing_histogram_buckets, naming the 32 histogram bins.
Builds compiled without --enable-wait-event-timing keep the GUC (its check
hook rejects any non-off value) and empty-result SQL stubs, so tooling
sees a uniform surface.
A later commit in the series exposes the per-backend overflow counters
(maintained here) and adds the reset functions.
Surface the per-backend truncation counters maintained by the recording path, and add the ability to reset wait-event-timing statistics. pg_stat_get_wait_event_timing_overflow(pid) and the pg_stat_wait_event_timing_overflow view report, per backend, lwlock_overflow_count (LWLock waits dropped because the per-backend tranche hash was full), flat_overflow_count (events whose class index was out of range), and reset_count. Resets use a lock-free request/response so the hot path stays single writer: each slot carries an atomic reset_generation, bumped by the resetter; the owning backend compares it against a backend-local last-seen value at its next wait_end and clears its own counters, incrementing reset_count. pg_stat_reset_wait_event_timing(pid) resets one backend -- synchronously when it targets the caller's own session (any user; pid defaults to NULL), or asynchronously for another backend (requiring pg_signal_backend, matching pg_stat_reset_backend_stats). pg_stat_reset_wait_event_timing_all() resets every backend and is superuser-only. Builds without --enable-wait-event-timing keep empty-result/feature-not- supported stubs for the new functions.
Add --enable-wait-event-timing to the "Linux - Autoconf" GitHub Actions task so the wait-event-timing build path -- including the expected output src/test/regress/expected/wait_event_timing.out -- is exercised on every push. That task already runs check-world under the undefined/alignment sanitizers with a small segment size, giving the timing code meaningful coverage. Every other CI task keeps building without the flag, so the stub path and its alternate output wait_event_timing_1.out remain covered as well.
Convert the per-backend wait-event-timing slot array from eager
main-segment shared memory to a lazily-allocated DSA region. Only a
small control struct (a DSA handle plus an LWLock) now lives in fixed
shared memory; the large array -- ~30 KB per backend at the default
wait_event_timing_max_tranches -- is allocated the first time any backend
in the cluster sets wait_event_capture to a non-off value. A build that
compiles the feature in but never enables it therefore pays no
per-backend memory, and a SELECT against the views on a cluster that
never enabled capture does not even create the DSA.
This is a pure refactor: the SQL surface and observable behavior are
unchanged, and the existing regression tests pass without modification.
Backends attach to the array on their first wait event under capture, in
pgstat_wait_event_timing_lazy_attach(). Because that runs from the
wait-event hot path, it carries the guards that make DSA work safe there:
- skip while CritSectionCount > 0 (dsa_attach -> MemoryContextAlloc
asserts inside a critical section);
- skip while MyProc->lwWaiting != LW_WS_NOT_WAITING (a nested
LWLockQueueSelf on the control lock would PANIC);
- an in_attach re-entrancy guard, because dsa_create / dsa_allocate /
the control-lock acquisition can themselves emit LWLock wait events
that re-enter the hot path;
- a before_shmem_exit gate (wait_event_timing_writes_disabled) so the
hot path stops touching DSA once proc_exit begins tearing the
mappings down -- shmem_exit runs all before_shmem_exit callbacks
before dsm_backend_shutdown, so the gate is up before any unmap.
A new LWLock tranche, WaitEventTimingDSA, names the control lock.
Add the third capture level, wait_event_capture = trace. On top of the
STATS aggregates, every completed wait is pushed into a per-session ring
buffer in DSA -- one record per wait -- allocated lazily on first use so
only sessions that enable trace pay the (default 4 MB) per-ring cost.
The ring is exposed two ways:
- pg_get_backend_wait_event_trace() / the pg_backend_wait_event_trace
view read the calling backend's own ring;
- pg_get_wait_event_trace(procnumber) reads any backend's ring
cross-process, including rings left behind by exited backends.
A backend's ring is not freed when it exits: the slot transitions to
ORPHANED and the ring stays in DSA so cross-backend consumers can read
the dying backend's final waits (important for short-lived parallel
workers). An orphan is reclaimed when a new backend reuses the same
procNumber (clear-on-init) or by pg_stat_clear_orphaned_wait_event_rings()
(execution revoked from PUBLIC; delegable with GRANT). Slot transitions
(FREE/OWNED/ORPHANED) are serialised by the WaitEventTraceControl lock;
the ring size is fixed cluster-wide at server start by
wait_event_trace_ring_size (power of two, default 4 MB).
The single-writer hot path writes each record under a seqlock (odd seq
while writing, even when complete). Cross-backend readers use a
POSITION-ENCODED IDENTITY seqlock -- a record at ring index i is valid
only if its seq equals the writer's complete value for that exact position
-- which rejects stale previous-cycle reads that a parity-only seqlock
would accept under cross-process visibility lag. A TAP test drives an
injection point between the writer's write_pos advance and its seq stamp
to prove exactly that: with the ring wrapped and the writer wedged
mid-record, a cross-backend read returns ring_size - 1 records, skipping
the in-flight slot whose stale prior-cycle record a parity-only check
would have emitted.
The trace ring writer carries the same teardown discipline as the stats
path: a wait_event_trace_writes_disabled gate (raised around slot
transitions and at proc_exit) plus in_attach/in_release re-entrancy guards
keep the DSA-internal LWLock waits that those operations emit from
recursing into a ring that is being freed or orphaned. A further
re-entrancy guard on the record writer itself keeps wait events emitted
mid-record-write (reachable only via the injection point; the write is
plain stores in production) from recursing into the writer.
Privileges: both trace SRFs and the session-local view are REVOKE'd from
PUBLIC and GRANT'ed to pg_read_all_stats (reading a session's trace
exposes its wait sequence).
Query-attribution markers and their executor/protocol hooks are added in
the next commit; this commit records wait events only.
At the trace level, interleave query-boundary markers with the wait
events in the per-session ring so a reader can tell which query each wait
belongs to. Two marker families are emitted:
- ExecStart/ExecEnd bracket every executor run
(ExecutorStart/ExecutorEnd), the primary attribution signal --
every executable statement, including those in parallel workers and
pipelined extended-protocol messages, is bracketed;
- QueryStart/QueryEnd fire at top-level query_id transitions
(pgstat_report_query_id) and at the transition to idle
(send_ready_for_query), providing the inter-statement boundaries the
executor markers cannot -- e.g. the ClientRead wait between
statements.
In the pipelined extended protocol a Parse/Bind/Execute can arrive while
the previous query's id is still set and the session is still RUNNING
(no Sync->idle in between), so the prior id is flushed with force=true at
those message boundaries to fire its QUERY_END before the new query
starts. That flush is gated on wait_event_capture = trace, so when trace
is off it is a no-op and pg_stat_activity.query_id behaves exactly as
before.
Marker emission requires a non-zero query_id (compute_query_id) and
track_activities; a WARNING is logged when trace is enabled without them.
The markers themselves are no-ops unless capture is at trace, so the only
cost when the feature is off is the inline gate already present.
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
12 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.
Purpose
This PR holds the submission-ready 7-patch series for the wait_event_timing feature, staged here for Andrey's review before it goes to pgsql-hackers. It is the reworked form of the original 8k-line monolith (#1), restructured per Andrey Borodin's review: "send them all together, but break into chewable parts ... defer DSA refactoring ... so the committer can see value from a first commit without venturing into 8k lines."
Base: upstream master
9fa2c1ebd17(branchwet-series-base). Each commit builds and passes the full test suite on its own (bisectable); docs and tests travel with the code they cover; both build systems are updated in lockstep.The 7 patches
cdbd46f--enable-wait-event-timing(autoconf + meson) and thewait_event_captureGUC (off|stats); pure scaffolding, no behavior1f194d6pg_stat_wait_event_timing+ histogram-buckets views; docs + tests6f7067cc49c7cc2cee54c95ed40eff9a94bGroups for the cover letter: patches 1–4 = stats (independently committable), patch 5 = storage refactor, patches 6–7 = trace.
Reviewer-concern checklist (from the #1 review)
unlikely()on the inline gate-O2, ±0.09 % TPS. Gates carry "no unlikely()" comments with the rationaleValidation summary
Key facts (CCX43, 16 dedicated vCPU, gcc 13):
Correctness: full
meson test(regression + isolation + recovery/subscription/pg_upgrade TAP): 366 Ok / 0 Fail timing build, 365 Ok / 0 Fail stub build (seqlock TAP test correctly skips in stub). Every intermediate patch stage rebuilt and re-verified (builds + regress; seqlock test at 0006). UBSan-clean.Stress (cassert): 300-backend churn + parallel workers attaching in critical sections + concurrent TPC-B, capture forced cluster-wide at stats and at trace (incl. 20× trace↔off toggles, 102 orphan rings reclaimed) — zero asserts/panics.
Concurrency: two-session cross-backend reset serviced asynchronously; post-mortem orphan-ring read after owner exit + explicit reclaim.
Performance (pgbench, 5 interleaved runs/cell):
-Sread-only (cached)-Swait-saturated (32 MB SB)An initial TPC-B −3 % was proven a binary-layout artifact with a gate-compiled-out control (the gate-removed build measured slower than the gate-included one). Off-mode cost is below the measurement floor.
Mechanical integrity: the 7-patch set applies cleanly with
git amonto9fa2c1ebd17and reproduces a tree bit-identical to the fully-validated development tip. Adversarial reviews passed for each risky patch and for the whole series.Fixes over the original #1 (surfaced by the split + reviews)
pg_stat_reset_wait_event_timingdocumentedDEFAULT NULLbut lackedproargdefaults— no-arg call would error.SELECTon a never-enabled cluster created (and pinned) an unused DSA segment.query_idflush perturbedpg_stat_activityeven with the feature off — now gated on trace.Status & deliverables
format-patchfiles + filled cover letter:patches-v1/(local; nothing sent to pgsql-hackers yet).wet-series-10commit.