01 [BUG?] check_exclusion_or_unique_constraint false negative Patch Review 29 msgs Oct 5, 23:28
This extensive thread investigates a potential bug related to `check_exclusion_or_unique_constraint` leading to false negatives, particularly concerning `SnapshotDirty` scans missing logical tuples during concurrent updates. The proposer iterated on a solution, rebasing patches multiple times and fixing test races. A reviewer provided a significant rebase of the patch, addressing conflicts due to upstream changes. The reviewer also identified a key behavioral change introduced by the patch: the MVCC scan approach would no longer wait for in-flight `INSERT` transactions, potentially leading to earlier `update_missing` reports compared to the existing `SnapshotDirty` behavior. The proposer acknowledged this change, noting that the current master's behavior in this specific scenario is considered a race rather than a guarantee, and updated the commit message accordingly.
02 PSQL schema "describe" \dn is not escaping quotes Patch Review 17 msgs Oct 5, 18:29
The thread identifies and addresses a bug where the psql `\dn` command fails for schema names containing quotes due to incorrect escaping, and the publication footer is missing for double-quoted patterns. The proposer submits an initial patch (v1), which receives feedback on indentation and function usage from reviewer 1, and consistency from reviewer 2, leading to v2. After v3 includes test cases as requested by reviewer 3, reviewer 5 points out that the patch introduces unintended pattern matching behavior, creating misleading output for multiple schemas. The proposer rewrites the logic (v4) to align with `\d table` behavior. Reviewer 6 then discovers a regression in v4 concerning case-sensitive quoted schema names, which is fixed in v5. Further memory leak and initialization issues found by reviewer 6 and reviewer 1 are addressed in v6 and v7. The latest review for v7 raises significant concerns about user-visible output changes, backporting implications, inconsistent no-match behavior compared to `\dt`, breaking CSV output with multiple headers, and minor coding nits.
03 aio: worker: Free SMGR objects when idle Discussing 14 msgs Oct 5, 19:30
The proposer identified a resource leak in I/O worker processes where `SMgrRelation` objects accumulate without being destroyed because `smgropen()` is called outside of a transaction. The initial patch proposed cleanup when the worker is idle. Commenter 1 noted a related bug and suggested an invalidation message-based approach, but reviewer 1 raised concerns about spinlock contention. Commenter 2 proposed an alternative LRU-based solution with I/O stamps and checkpoint-triggered cleanup to avoid shared locks. The proposer refined their patch to trigger cleanup based on the number of I/Os or unpinned entries, and to handle idle workers. The discussion continues to explore the best mechanism to ensure timely cleanup without introducing performance bottlenecks.
04 [PATCH] pg_upgrade: add --initdb option to create the new cluster automatically Discussing 19 msgs Oct 5, 13:29
This thread is about adding an `--initdb` option to `pg_upgrade` to automate the new cluster's initialization, aiming to reduce manual errors. Initial discussions covered handling `--check` with `--initdb`, documentation needs, security concerns regarding command injection, and cleanup of orphan directories. Over several versions, the proposer and a collaborating developer refined the patch, making `--check --initdb` a dry run, implementing automatic cleanup for failed upgrades, using secure command assembly, and splitting documentation into a separate patch. However, the latest review of v6 identifies new critical issues: `--check --initdb` still skips most compatibility checks, the utility fails to find its binary without the `-B` option, and it incorrectly rejects the `-O` option when `--initdb` is used, indicating significant unresolved design problems.
05 PG19: two RI fast-path issues found while testing the batching revert Discussing 18 msgs Oct 5, 13:29
This thread began with reports of two pre-existing bugs in PostgreSQL 19's Referential Integrity (RI) fast-path, found during testing of an RI batching revert: incorrect column-level SELECT permission checks and improper invalidation of the RI cast cache. The proposer provided initial fixes, which a reviewer refined. Later, additional issues were discovered, including a race condition in `ri_HashCompareOp()` and problems with metadata invalidation on `pg_amop` changes. Some of these issues have already been committed. However, the latest messages indicate further problems with the RI fast path directly calling cast functions, leading to incorrect error handling for NULL returns and stale cast definitions. New patches are proposed to address these and similar issues in older RI paths.
06 [PATCH] Fix segmentation fault caused by reentrancy in RI_Fkey_cascade_del (ri_triggers.c) Patch Review 12 msgs Oct 5, 15:29
The thread addresses a segmentation fault in `RI_Fkey_cascade_del` caused by a use-after-free issue when `BEFORE DELETE` triggers on a self-referential table invalidate the plan cache during a reentrant RI trigger execution. The proposer provided a two-part patch: a test case to reproduce the bug and the fix. Initial discussion by a reviewer acknowledged the crash but questioned whether `BEFORE DELETE` triggers should perform such actions (deleting other rows). The proposer argued for legitimate use cases or for explicit error messages if such actions are forbidden. The proposed fix involves introducing reentrancy guards and reference counting for plans to prevent premature freeing. A detailed review identified several issues with the reference counting, including leaks on error paths, incorrect entry removal, and potential hash key reuse. The proposer addressed these points in subsequent patch versions (v4), incorporating a ResourceOwner for robust error handling and a delete-later list for invalidated plans. The latest review of v4 was positive, confirming that the bookkeeping issues were resolved. The proposer then rebased the patches to v5 to apply cleanly on master, with a minor change in the deferred plan freeing logic due to upstream changes. The reviewer of v5 confirms the new placement is safe and the fix still looks correct.
07 Logical replication: lost updates/deletes and invalid log messages caused by SnapshotDirty + concurrent updates Discussing 4 msgs Oct 5, 23:28
This thread was initiated to focus on the logical replication implications of a bug where `SnapshotDirty`/`SnapshotSelf` scans can miss logical tuples during concurrent updates. The proposer detailed how these missed tuples lead to "lost DELETEs", "lost UPDATEs", and invalid log messages in logical replication, providing concrete scenarios. A developer confirmed this behavior is a bug. The proposer then proposed documenting the issue as a temporary measure. In the latest exchange, the proposer addressed a reviewer's concern (from a related thread) about a behavioral change, explaining that the proposed fix would cause `update_missing` to be reported sooner for in-flight `INSERT`s than on master. The proposer justified this by stating that master's current behavior is racy rather than a guarantee and that the commit message for the fix has been updated to clarify this.
08 UNDO with constant time recovery (CTR) Patch Review 9 msgs Oct 5, 21:28
The proposer introduced a patch set implementing UNDO logging for PostgreSQL, focusing initially on FILEOPS rather than a new table access method, diverging from previous ZHeap and Zedstore projects. The goal is to improve recovery consistency by removing files created by crashed transactions. A reviewer noted several issues with an early version of the patch, including tests needing later patches, remaining directories after CREATE DATABASE crashes, and server panics due to filesystem errors within critical sections. Another reviewer questioned the "constant-time recovery" claim, noting that both ROLLBACK latency and crash recovery duration appeared linear with the number of undo records in testing, not constant. The proposer acknowledged these issues and indicated they are working on corrections, including re-organizing the patch series and rechecking assumptions around unlogged relations and transactional boundaries for UNDO. The performance measurements for recovery and rollback were also shared and analyzed by a commenter, confirming linear behavior.
09 pg_resetwal: refuse to run when backup_label exists Discussing 10 msgs Oct 6, 00:29
This thread discusses a proposal to enhance `pg_resetwal` by making it refuse to run when a `backup_label` file exists in the data directory. The goal is to prevent users from accidentally corrupting valid backups. An initial reviewer raised concerns about restricting a powerful salvaging tool, suggesting an alternative `--cluster-state` option. The proposer clarified that the change only enforces removing `backup_label` *before* `pg_resetwal`, which is already implicitly required for server startup. Subsequent reviewers supported this, noting it forces deliberate action and provides clearer warnings, preferring manual removal over an `-f` override. The `--cluster-state` option was deemed out of scope. Despite some initial reservations about the patch's value from one reviewer, the patch was ultimately moved to "Ready for Committer", with the final decision on the `-f` override left to the committer.
10 Parallel Apply Patch Review 12 msgs Oct 5, 13:29
This thread focuses on developing a "Parallel Apply" feature for logical replication. Early discussions covered the rationale for using different hash table implementations (`dshash`, `simplehash`, `HTAB`) based on specific requirements. Reviewers provided feedback on comments, naming conventions, and the scope of hash table entries. The proposer iteratively updated the patches (v17-v21), incorporating fixes for dependency detection (including toasted columns and partitioned tables), refining memory limit controls for dependency tracking, and improving code readability. A significant update involved collecting unique indexes from leaf partitions for partitioned tables. The latest message indicates that the current patch set requires rebasing to continue the review process.
11 [PATCH v1] Fix races in Windows pthread emulation Discussing 6 msgs Oct 5, 20:28
This thread addresses a race condition in the Windows pthread mutex emulation, specifically impacting ARM64 builds where the ECPG thread/alloc test could hang. The proposer identified that the `InterlockedExchange` function, used in mutex initialization, could overwrite a fully initialized state, causing subsequent `pthread_mutex_unlock` calls to fail and leave critical sections locked. The initial patch proposed using `InterlockedCompareExchange` and interlocked operations for mutex state and `pthread_once_t` to ensure correct memory ordering. Commenters confirmed the issue's occurrence on both ARM64 and AMD64. The discussion also covered potential performance overhead of the interlocked operations and the backpatch compatibility of changing `pthread_once_t` from `bool` to `LONG`. The proposer is considering revising the patch to potentially separate the mutex and `pthread_once` fixes.
12 NOT NULL NOT ENFORCED Patch Review 18 msgs Oct 5, 07:29
The thread discusses the implementation of `NOT NULL NOT ENFORCED` constraints in PostgreSQL to complete SQL feature F492. The proposer outlines rules for single-column constraints, partitioned tables, and inheritance. An early discussion point concerned `pg_dump` behavior: whether `NOT NULL NOT ENFORCED` should be dumped with column definitions or as separate `ALTER TABLE` statements. A reviewer argued for inline dumping for user-friendliness, and the proposer later updated the patch, finding the change less complex than initially thought. Further review included a suggestion for tab-completion and comprehensive testing of a later patch version (v5) across various scenarios, which yielded positive results. The commit message was also refined for clarity and a rebased v6 patch was provided.
13 enhancing pg_basebackup speeds up to ~23Gbps (small fixes + io_uring/Direct I/O) Patch Review 6 msgs Oct 5, 16:28
This thread focuses on significantly enhancing `pg_basebackup` performance, aiming for speeds up to ~23Gbps by optimizing various aspects without altering its single-threaded, single-connection design. The proposer identified several areas for improvement: reducing syscalls, avoiding redundant memory copies, and leveraging Direct I/O with asynchronous submission using `io_uring`. Initial benchmarks show nearly a 2x speedup. The proposed patches include new blackhole targets for better bottleneck identification, a transfer rate timing message, and increased buffer lengths to reduce `pread()` calls. Another contributor joined the thread with an improvement for incremental backups, using `posix_fadvise()` for prefetching scattered blocks, showing significant speedups. This incremental backup optimization was merged into the main patch set. The current discussion includes reviewing specific patch details, like buffer alignment and checksum verification logic.
14 amcheck: add index-all-keys-match verification for B-Tree Patch Review 13 msgs Oct 5, 09:29
This thread proposes a new `amcheck` feature, `indexallkeysmatch`, to detect B-tree corruptions where index keys do not match their corresponding heap tuple keys. The initial patch utilized a Bloom filter for efficiency. Early reviews identified issues with duplicate visibility checks and potential memory leaks, which the proposer subsequently addressed. Further discussions uncovered false positive corruption reports under concurrent `VACUUM` operations and unhandled errors when heap segments were missing. The proposer delivered a revised patchset (v6) that resolved the `VACUUM` issue by mirroring MVCC index scan logic for heap lookups and introduced dedicated checks for dangling entries and lost heap segments. The latest development involves a compilation issue caused by recent core changes, for which a reviewer provided a fixup patch.
15 [PG19] COPY (query) TO ... (FORMAT json) uses the table's column names Patch Review 3 msgs Oct 5, 23:28
The thread discusses a bug in PostgreSQL 19's `COPY (query) TO ... (FORMAT json)` feature. The proposer identified that JSON keys were named after the underlying table's columns instead of the query's projected or aliased column names, leading to incorrect output. A developer confirmed the bug and an initial fix idea. The same developer then proposed an optimized patch to avoid a 4-7% performance regression. A reviewer subsequently conducted extensive testing on the revised patch, confirming it resolved the reported issue and even an unmentioned related bug, with no observable regressions or failures across numerous complex scenarios. This positive review indicates the patch is likely ready for commitment.
16 WAL segment file descriptor leak on read errors can PANIC the server Patch Review 14 msgs Oct 5, 22:29
This thread discusses a critical file descriptor (FD) leak in WAL read paths, which can lead to server PANICs or 'Too many open files' errors. The proposer identified that WAL segment FDs, opened via BasicOpenFile(), are not properly closed on error paths, particularly in logical decoding, 2PC WAL read code, and the WAL summarizer. The initial proposed fix involved registering a memory context reset callback in XLogReaderAllocate(). Commenter 1 confirmed the issue and noted that even the walsender leaks FDs on normal exits. Commenter 2 suggested a more flexible approach: lazy registration of the callback only when a segment is actually opened, and anchoring it to the reader's memory context. The proposer agreed and submitted a v2 patch implementing lazy registration. However, Commenter 2 then raised concerns about a layer violation in v2's approach and proposed a different patch with a helper routine XLogReaderRegisterReset() to define the callback within segment open callbacks, aiming for less invasiveness and better alignment with BasicOpenFile(). Commenter 3 provided minor review comments on the latest patch, and Commenter 2 is awaiting further feedback from the proposer.
17 pgstat: allow a stats kind to use its own dedicated dsa/dshash Patch Review 5 msgs Oct 5, 23:28
The thread proposes enhancing the `pgstat` infrastructure to allow specific statistics kinds, such as `pg_stat_statements`, to utilize their own dedicated DSA (Dynamic Shared Area) and `dshash` (Dynamic Shared Hash). The proposer argues this isolation would improve performance by reducing scan overhead and lock contention, and enable better memory management through independent sizing. A reviewer provided detailed feedback, acknowledging the value but suggesting several improvements, including more robust OOM handling, optimized reset logic, and reconsidering the design of shared memory allocation. After a request to rebase due to upstream changes, the proposer delivered a v2 patch, implementing several of the reviewer's suggestions while holding off on others that involve more significant design decisions or changes in failure semantics, awaiting further discussion.
18 Vacuum statistics Patch Review 92 msgs Oct 4, 14:28
This extensive thread discusses an effort to enhance PostgreSQL's VACUUM statistics. Initially, the proposer aimed to integrate a comprehensive set of new statistics directly into core, including various tuple and page counts, WAL metrics, and timing information. Early in the discussion, a reviewer raised concerns about the significant increase in statistics volume and suggested a more granular approach, potentially involving a GUC. The proposer addressed initial review comments, fixing issues like commit message clarity and distinguishing between heap and index statistics. A major turning point occurred when a new reviewer questioned the approach, advocating for a design similar to pg_stat_statements, where core provides hooks and an external module handles the collection and storage of most detailed statistics. The proposer agreed to this hybrid model, restructuring the patch set to include a small core component and a new ext_vacuum_statistics extension. Subsequent reviews focused on addressing concerns with the extension-based approach, specifically regarding garbage accumulation for dropped objects and safe handling of statistics updates during error callbacks. The proposer revised the patches to use object_access_hook for cleanup and refactored error reporting to avoid locks in error handlers. The latest iteration of patches also included fixes for CI issues, rebase efforts, and minor function naming adjustments. However, the most recent feedback from a reviewer has identified new issues concerning parallel VACUUM double-counting resource usage and improper privilege handling for extension reset functions, indicating ongoing development and refinement.
19 Proposal: SELECT * EXCLUDE (...) command Discussing 9 msgs Oct 5, 10:29
This thread proposes implementing SELECT * EXCLUDE (...) functionality, which allows excluding specific columns from a SELECT * expansion. The feature has been discussed previously and is also being considered for the SQL standard, with a paper outlining proposed semantics shared by a SQL standard proposer. Initial patch review revealed several deviations from the proposed standard, including incorrect error handling for non-existent or ambiguous columns, and issues with dropped columns, exclusion leakage, and qualified name resolution. There's an ongoing debate regarding the ambiguity rules and column privilege checks, with some arguing for a more lenient implementation than the SQL standard proposal. The latest messages confirm an updated SQL standard syntax (including REPLACE and RENAME clauses) and the proposer's intent to release a v3 patch incorporating these changes and fixing the reported bugs.
20 pg_dump/restore failure (dependency?) on BF serinus Discussing 7 msgs Oct 5, 13:29
The thread started with a `pg_dump`/`pg_restore` failure on a build farm member, specifically a foreign key constraint creation failing due to a missing unique constraint. Initially suspected as a dependency issue, further investigation by a commenter revealed a deeper "catalog check race" as the root cause. This race occurs during concurrent `ATTACH` statements for 2-level partitioned tables, where the primary key index of the parent table is left permanently `indisvalid=false`. The issue stems from concurrent `ATTACH` statements incompletely counting valid indexes before all transactions commit. A patch was previously suggested to address this, but the latest message indicates its removal, signifying ongoing work to find a resolution.
21 Let an ordering index scan hand its ORDER BY value to the target list Discussing 5 msgs Oct 5, 18:29
This thread proposes an optimization for `ORDER BY` clauses utilizing index ordering values. The proposer observes that the current system re-computes ranking values even when an index provides an exact value, leading to inefficiency and preventing direct exposure of scores in the result set. The proposed solution is to allow the executor to substitute the index's exact `ORDER BY` value into the target list, mirroring `Index Only Scan` behavior. An initial patch (v1, then v2) is provided. A reviewer challenges the motivation, questioning the claim of "incorrect results," and reiterates a long-standing core developer's principle that an index's plan choice should not alter the query's row-data output. The proposer clarifies that the primary goal is optimization and exposing the score, not changing the operator's meaning.
22 issues with eager aggregation Committed 8 msgs Oct 6, 00:29
This thread addresses several issues found with the eager aggregation feature. The proposer reported three specific bugs leading to wrong answers or internal errors, alongside a more debatable behavioral change: a division-by-zero error occurring with eager aggregation that wouldn't happen otherwise. This occurs because eager aggregation can evaluate expressions on rows that are later filtered by joins. The primary developer confirmed the three bugs and provided fixes for them. For the behavioral change, the developer argued it aligns with existing query transformations (e.g., pushed-down WHERE/HAVING clauses, inlined CTEs) where expressions are evaluated on rows not necessarily present in the final result. Remedies for users, such as using `CASE` or specific GUCs, were discussed. The three bugs were eventually fixed and committed, and it was agreed that the documentation should be updated to clarify the expression evaluation behavior.
23 pg_*_advice: tsv load failure, etc. Committed 21 msgs Oct 5, 21:28
This thread discusses various issues and potential improvements for the pg_plan_advice and pg_stash_advice modules, identified through an Opus review. Initial concerns included an empty stashed advice string persisting, pg_start_stash_advice_worker() silently losing persisted advice, and stash-supplied advice overriding GUC settings. Secondary issues involved cached generic plans ignoring advice changes and lack of syntax validation for stashed advice. A set of patches was proposed to address some of these, including documentation for GEQO vs. pg_plan_advice, fixes for JOIN_ORDER advice feedback, disallowing empty sublists, and handling partition names without schemas. Two of these patches (disallowing empty sublists and partition names) were committed. Later, a commenter identified new cases where pg_plan_advice generated unenforceable GATHER advice for set operations and where PARTITIONWISE advice on plain tables disabled all scan paths. A doc patch was also submitted to clarify that advice is consulted only at plan time and how to invalidate cached plans. The main developer provided a new patch set fixing brown-paper-bag bugs related to child_append_relid_sets and the JOIN_ORDER feedback, acknowledging the complexity and that not all reported issues were resolved. They indicated these fixes would be committed and back-patched to v19.
24 Limiting WAL retained for archiving, like max_slot_wal_keep_size Rejected 3 msgs Oct 5, 19:30
The proposer suggested adding a configuration setting similar to `max_slot_wal_keep_size` but for WAL archiving. This would allow PostgreSQL to automatically remove older unarchived WAL segments if the archiving process falls behind or breaks, preventing disk space exhaustion. The server would record this action, alerting tools about gaps in Point-in-Time Recovery (PITR). A commenter expressed strong reservations, arguing that such a feature would compromise the guarantee of PITR by allowing critical WAL segments to be lost. They emphasized that the decision to discard WAL should reside with the archiving tool, not the core PostgreSQL server.
25 [PATCH v1] amcheck: Allow interrupting the child-level rightlink walk Committed 5 msgs Oct 5, 12:29
This thread discusses a patch to improve the robustness of `amcheck` by allowing interruptions during the child-level rightlink walk in `bt_child_highkey_check()`. The proposer identified that this specific loop in `verify_nbtree.c` lacked `CHECK_FOR_INTERRUPTS()`, making it uninterruptible, which is a significant concern when checking potentially corrupt indexes with long or circular rightlink chains. The patch, a single-line addition, was quickly reviewed and approved, being recognized as a consistency fix and a safety improvement.
26 Parallel autovacuum: DROP DATABASE WITH (FORCE) fails on the parallel workers Patch Review 5 msgs Oct 6, 00:29
This thread discusses a bug where `DROP DATABASE WITH (FORCE)` fails for non-superuser roles with `pg_signal_backend` if a parallel autovacuum worker is active on that database. The core issue is that autovacuum leaders do not publish a role ID (`InvalidOid`), while their parallel workers publish `BOOTSTRAP_SUPERUSERID`, leading to a permission denied error when attempting to terminate the workers. The proposer initially offered a specific fix. A reviewer suggested a more general approach, applying the fix to all background workers running parallel queries by checking a lock group member against its leader's role ID, and backporting the change. The proposer agreed and submitted an updated patch. However, the latest review identified a critical race condition: the patch attempts to access a `PGPROC` pointer for a lock group leader without holding the necessary lock, potentially leading to incorrect permission checks if the leader exits and its `PGPROC` is recycled.
27 doc: Document Linux cgroup memory limits Patch Review 11 msgs Oct 5, 15:29
The thread proposes adding documentation about Linux cgroup memory limits to the PostgreSQL manual, as many installations run Postgres in containers where cgroup limits dictate OOM killer behavior. The initial patch focused on `memory.max`, how backends are killed, and huge page caveats. Reviewer 1 provided detailed testing feedback, suggesting clarifications on `memory_hugetlb_accounting` (huge pages counting) and the impact of `memory.oom.group` on postmaster termination and crash recovery. The proposer incorporated these changes in v2, adding a paragraph on `memory.oom.group`. Further refinement in v3 corrected the wording regarding automatic restart versus recovery after a group kill. In v4, additional points were integrated, covering systemd's `OOMPolicy` when running as a service, and the `oom_score_adj -1000` exemption for the postmaster from group kills. Commenter 1 provided broader context on PostgreSQL's memory management, suggesting it's risky to run on cgroups2 without more aggressive self-limiting or connection rotation. The proposer responded by softening the wording in the final paragraph of v5, recommending a safety margin, but deferring broader memory management discussions as outside the scope of this documentation patch. The patch has been deemed "Ready for Committer" by reviewer 1.
28 COPY FROM ... WHERE fails for negated operators Patch Review 2 msgs Oct 5, 20:28
This thread identifies a bug in `COPY FROM stdin WHERE NOT (a > 0)` which results in a "cache lookup failed for function" error. The proposer pinpoints the problem to `DoCopy()` neglecting to call `fix_opfuncids()`, a necessary step to properly fill in operator function IDs, similar to how `RelationGetIndexPredicate()` operates. This omission causes `negate_clause()` to be called with incorrect arguments. A patch was provided to include the `fix_opfuncids()` call within `DoCopy()`. A core committer reviewed the patch, acknowledging the immediate necessity of the fix while also noting a broader desire to refactor and remove `fix_opfuncids()` entirely in the future.
29 Reducing relcache memory usage 2: shrink sizeof(RelationData) Patch Review 4 msgs Oct 5, 12:29
This thread details an effort to significantly reduce PostgreSQL's `relcache` memory usage by shrinking the `sizeof(RelationData)` structure. The proposer introduced a patch set aimed at reducing the size from 488 bytes to 256 bytes, potentially saving approximately 53 MiB of `CacheMemoryContext` per backend for large schemas. The patch set includes changes like introducing unions for mutually exclusive states, reordering members for better packing, lazy allocation of partition information, and removing redundant members. A reviewer provided feedback on individual patches, raising concerns about lazy computation overheads and the use of sentinel values. The proposer addressed these concerns, confirming the safety of lazy computations and running `pgbench` tests on partitioned tables, which showed no measurable performance regressions.
30 Planning time quadratic in the IN-list length for "c = X AND (a, b) IN (...)" with BitmapOr Patch Review 6 msgs Oct 5, 19:30
The proposer reported a quadratic increase in query planning time for `c = X AND (a, b) IN (...)` queries with long `IN` lists, specifically when `BitmapOr` scans are involved. Planning times reached several seconds for 4000 entries across recent PostgreSQL versions. A reviewer quickly provided a patch that reorders the predicate testing logic in `predtest.c`, prioritizing the arm at the same position. This change dramatically reduced planning times from seconds to milliseconds for thousands of entries. The proposer thoroughly tested the patch on multiple PostgreSQL versions, confirming its effectiveness and correctness. The patch has been registered in the commitfest, and the proposer volunteered as its reviewer, confirming its readiness for committing.
31 Limiting WAL retained for archiving, like max_slot_wal_keep_size Discussing 2 msgs Oct 5, 22:29
This thread introduces a proposal to limit WAL retention for archiving, similar to 'max_slot_wal_keep_size', to prevent 'pg_wal' from filling up if archiving fails or cannot keep pace. The proposer suggested an opt-in setting that would automatically remove the oldest unarchived WAL segments once a limit is exceeded, and record this event in 'pg_stat_archiver' to signal a missing WAL gap and the need for a new base backup. Commenter 1, while acknowledging the problem, expressed significant concerns about the proposed solution. These concerns include the risk of silently broken recoveries, incorrect success reports for backups whose WAL ranges include dropped segments, the non-durable nature of 'pg_stat_archiver' for recording critical gaps, and potential confusion for the archiver process. Instead of automatically dropping WAL, Commenter 1 suggested a less risky alternative: enhancing 'pg_stat_archiver' to expose metrics on archiving lag, such as the oldest unarchived segment or bytes pending archiving, to allow proactive alerting.
32 Reduce SyncRepLock contention on the commit path Patch Review 3 msgs Oct 5, 13:29
This thread proposes a patch series to reduce `SyncRepLock` contention in synchronous replication, a critical bottleneck for high commit rates. The proposer's four patches aim to move non-critical work out of the lock's critical section: waking waiters after lock release, computing synced positions before acquiring the lock, releasing waiters once per batch of replies, and skipping the lock if acknowledgement has already arrived. After initial feedback and rebase by a reviewer, the proposer submitted an updated version (v3) addressing minor issues and fixing a test case. The reviewer then confirmed that the patch series is ready for commit-fest, indicating its maturity.
33 Serverside SNI support in libpq Patch Review 75 msgs Oct 5, 15:29
The thread focuses on serverside SNI support in libpq, particularly addressing issues related to SSL configuration reloads and potential crashes. An early bug involved a NULL pointer SIGSEGV when `ssl_sni` was changed during a failed reload, leading to access of non-existent SSL configuration. The proposer developed a series of patches, iteratively refining the solution based on feedback from reviewers. Initial proposed fixes for `ssl_sni` handling were discussed, with the goal of aligning `ssl_sni` behavior with other SSL configuration parameters on reload failures. Subsequent patches addressed memory leaks, dangling pointers, and `SSL_CTX` cleanup. Reviewers identified issues such as missing null checks and the need for better test coverage. The latest versions of the patches have been reviewed positively, with additional comprehensive tests proposed to cover more edge cases, including encrypted per-host keys and turning SNI off with missing files. The proposer indicated plans to commit the fixes soon, after addressing the test format.
34 Fix "unexpected logical decoding status change" error; from concurrent logical decoding activation Patch Review 41 msgs Oct 5, 22:29
This thread addresses a race condition that causes "unexpected logical decoding status change" errors, particularly exposed by concurrent REPACK operations. The proposer explained that simultaneous creation of logical slots can lead to multiple XLOG_LOGICAL_DECODING_STATUS_CHANGE records being written due to a window between LWLock acquisitions and a barrier wait in EnableLogicalDecoding(). The second record, if found by a decoding slot, triggers an error. The discussion expanded to include challenges with logical slot synchronization on standbys, specifically when the standby lags in replay. An earlier solution could lead to repeated creation and dropping of valid slots. To mitigate this, the proposed patch introduced a check that allows a slot to be retained if its restart_lsn is ahead of the standby's replay LSN, preventing unnecessary drops when the standby is simply behind. This new logic ensures that the slot waits for replay to catch up rather than being discarded. The latest exchanges involved refining the comments for this behavior and suggesting minor improvements for the accompanying TAP test.
35 REPACK (CONCURRENTLY) might keep dropped-column data Patch Review 10 msgs Oct 5, 16:28
This thread identifies a bug in `REPACK (CONCURRENTLY)` where dropped-column data might be retained under specific scenarios, leading to bloat. The proposer provided two main replication cases: one involving logical replication on a subscriber when a column is dropped and the publisher updates rows, and another involving a `BEFORE UPDATE` trigger that returns `OLD` or a modified `NEW` tuple after a column is dropped. In both cases, the `REPACK (CONCURRENTLY)` operation, intended to remove bloat, failed to clear the dropped column data, resulting in larger-than-expected table sizes. An initial fix was proposed and committed, which explicitly sets dropped columns to NULL in the tuple before writing it out during the repack process. Further adjustments to the test cases and the fix's implementation (e.g., handling `tts_isnull`) were also committed. The latest message introduces new cases (replayed INSERT, UPDATE moving rows between partitions, COPY, MERGE, INSERT ON CONFLICT) where dropped column data might still be carried and stored, potentially causing `REPACK` failures due to oversized rows.
36 Fix reindexdb with parallel index-level conrurrent run Discussing 4 msgs Oct 5, 20:28
The thread discusses a bug in `reindexdb` where using parallel jobs (`-j N`) with `REINDEX CONCURRENTLY` for multiple indexes on the *same* table fails. The issue arises because `reindexdb` batches DDL commands for a single table, implicitly creating a transaction which conflicts with `REINDEX CONCURRENTLY`. A patch was proposed to send commands individually. However, subsequent review revealed that this fix introduced performance regressions by eliminating parallel execution for single-index tables and did not properly handle cancels or failures. A reviewer also questioned the practical benefits of parallelizing concurrent reindexing at the index level due to inherent wait phases. The latest suggestion is to have `reindexdb` return an explicit error for this specific problematic scenario, rather than attempting to fix it, as a fix wouldn't necessarily improve speed.
37 two small tab-completion patches (ALTER CONSTRAINT, CHECK modifiers) Proposed 1 msgs Oct 5, 21:28
The proposer introduced two small patches to enhance psql's tab-completion. The first patch aims to add completion for DEFERRABLE, ENFORCED, INHERIT, and their negating forms (e.g., NOT DEFERRABLE) after ALTER TABLE t ALTER CONSTRAINT c. The second patch proposes adding NOT ENFORCED, NOT VALID, and NO INHERIT as completion options after ADD [CONSTRAINT c] CHECK (...). These changes are intended to improve the user experience when defining or modifying constraints using psql.
38 Direct TOAST v2, faster, smaller and no migration needed Discussing 28 msgs Oct 5, 16:28
This thread introduces "Direct TOAST," an alternative storage format for out-of-line (TOASTed) variable-length attributes, aiming for faster performance, smaller footprint, and zero-migration. The core idea is to bypass the traditional TOAST B-tree index by embedding physical TIDs directly in the `varlena` pointer and within hierarchical chunk trees. This is claimed to reduce index insertion/WAL overhead, eliminate B-tree root-to-leaf traversals for reads, and improve vacuuming by enabling self-pruning without index cleanup. Initial benchmarks are presented with significant TPS improvements. However, a committer and other reviewers raise serious concerns about the design tradeoffs, questioning the universality of the performance claims, the impact on existing TOAST features like independent TOAST-only VACUUMs, and the complexity introduced. The proposer clarifies that the feature is optional, controlled by a storage parameter, and aims to address some of the raised issues (e.g., self-pruning for direct TOAST chunks, dynamic schema adaptation). The debate continues over the need for new infrastructure for this approach versus optimizing existing B-tree storage, and the implications for tools like VACUUM FULL, CLUSTER, and REPACK, which the proposer states would be forbidden directly on the TOAST table but would work fine on the main table.
39 Report relation extension blockers within parallel lock groups Committed 12 msgs Oct 5, 20:28
The thread discusses a fix for `pg_blocking_pids()` which previously failed to report processes blocking relation extension requests within the same parallel lock group. This oversight stemmed from a prior commit that introduced conflicts for relation extension locks among parallel group members without updating the blocking process detection logic. The proposer submitted a patch to add a specific exception for relation extension locks, ensuring `pg_blocking_pids()` correctly identifies blockers. The patch also included documentation updates to clarify that a process might appear to block itself when another member of its parallel group is the actual blocker. After a brief clarification on an earlier reverted change related to page locks, and a suggestion for documentation restructuring which was incorporated, the patch was committed.
40 examine_variable ignored CollateExpr Patch Review 6 msgs Oct 5, 05:29
This thread addresses an issue in `examine_variable` where `CollateExpr` nodes are implicitly stripped through `RelabelType` transformations, potentially causing inaccurate row estimates for queries using `COLLATE` clauses in `GROUP BY` or `WHERE` conditions. The proposer submitted a patch showing improved estimates for a specific test case. A commenter proposed a more general solution involving re-normalizing expressions, which was initially criticized for being too expensive. The commenter then refined their approach to an in-place function to avoid performance overhead. The discussion further progressed with a new reviewer identifying a scenario where the proposed patch might lead to worse estimates when no extended statistics object is present, suggesting a more conditional application of the stripping logic.
41 hashjoins vs. Bloom filters (yet again) Discussing 78 msgs Oct 5, 16:28
This thread discusses the reintroduction of a patch to integrate Bloom filters into hash joins to improve performance, particularly for selective joins or when batching (spilling to disk) is necessary. The proposer has previously worked on this twice. The core challenge is accurately estimating when the Bloom filter provides a benefit, as it introduces overhead if not selective enough. Previous attempts stalled due to this estimation difficulty and sizing concerns. A new version of the patch is being discussed, with a proof-of-concept for a semi-join + false-positive model for selectivity estimation. There's a debate between a "bottom-up, cost-based" approach (which requires better selectivity estimation for unmatched rows) and an "opportunistic" approach (which is simpler but might miss some optimizations). Specific issues raised include the need for exact matching of build relids for correct estimates, and potential trade-offs in snowflake schemas. A reviewer also provided a PoC patch for a semi-join + false-positive model. The thread also discusses challenges in estimating unmatched rows with current statistics infrastructure.
42 Session in aborted transaction misses effective_wal_level change Patch Review 9 msgs Oct 5, 18:29
This thread addresses a bug where an `effective_wal_level` change can be missed in a session with an aborted transaction, preventing subsequent transactions from being logically decoded. The proposer provides a reproducer script and an initial patch (v1) to move the `AtEOXact_LogicalCtl` call later. Reviewer 1 confirms the issue, provides an updated patch (v2) with regression tests, and suggests wider application of the fix. After additional reviews, a discussion ensues regarding the optimal placement of the `AtEOXact_LogicalCtl` call to avoid potential race conditions during memory cleanup callbacks. This leads to an updated patch (v4) that places the call after resetting the top-level transaction ID for a more robust solution.
43 Fix out-of-bounds array indexing in JsonValueList Patch Review 4 msgs Oct 5, 15:29
The thread discusses an out-of-bounds array indexing error in `JsonValueList` within `jsonpath_exec.c`, triggered when compiling PostgreSQL with `-fsanitize=bounds` or `-fstrict-flex-arrays=1`. The proposer identified that the `JsonValueList` structure, intended to act as a chunked list with its first chunk on the stack, incorrectly indexed past the declared `BASE_JVL_ITEMS` size. Although no memory overruns were reported in standard builds, the issue represents undefined behavior under C11. The proposer suggested a fix involving separating `JsonValueList` into two distinct structs: `JsonValueList` for the initial items and `JsonValueListChunk` for subsequent items. A committer acknowledged the problem, stating they had also encountered it, and later provided an alternative, simpler solution. This alternative solution uses a union within the existing `JsonValueList` struct, which effectively addresses the bounds issue while keeping the code simpler, with the caveat that it requires not exceeding `-fstrict-flex-arrays=1` (which is already a limitation for other reasons). A reviewer provided a positive assessment of the committer's proposed solution.
44 Fix apply worker crash when subscriber table has only a deferrable primary key Patch Review 22 msgs Oct 5, 09:29
This thread began by reporting an apply worker crash (or silent data loss in non-debug builds) when a subscriber table had only a `DEFERRABLE` primary key and the published table lacked `REPLICA IDENTITY FULL`. The issue stemmed from a discrepancy in how deferrable primary keys were handled by different functions within the apply worker. The initial patch rectified this by aligning the logic, resulting in the correct error message instead of a crash, and was subsequently committed after several rounds of review. The thread continued with a 'remaining issue' to further enhance the consistency and safety of the index lookup logic within the apply worker, specifically by utilizing an already computed local index OID rather than re-fetching it. A new patch addressing this refinement has been submitted and is currently under review.
45 Per-thread leak in ECPG's memory.c Committed 1 msgs Oct 5, 19:30
This thread addresses a per-thread memory leak in ECPG's `memory.c`, specifically within the `auto_mem_destructor()` function. The initial reporter identified two main problems: incorrect retrieval of thread-specific data at thread exit and an erroneous attempt to free user-owned heap objects by `ECPGfree_auto_mem()`. A patch was proposed to correct these issues by properly utilizing the destructor's argument and changing the freeing mechanism to `ecpg_clear_auto_mem()`, which correctly frees only the internal list structure, not the user's data. A reviewer confirmed the patch's effectiveness in preventing a double-free and suggested a minor cleanup for the destructor.
46 Improving scalability of Parallel Bitmap Heap/Index Scan Discussing 3 msgs Oct 5, 09:29
This thread introduces a proof-of-concept patch set aimed at significantly improving the scalability of Parallel Bitmap Index Scans (BIS), Bitmap Heap Scans (BHS), and Bitmap And/Or plan nodes. The current implementation suffers from a single leader process performing most of the work. The proposed solution involves each parallel worker building its own private `TIDBitmap`, followed by a repartitioning step among workers, and then each worker processes its private partition. This design deviates from the usual shared-state parallelism model to eliminate per-TID lock contention and simplify iteration. The proposer has already integrated a separate patch to enhance sort performance within each parallel worker. The design approach is open for discussion, and a new commenter has expressed interest in reviewing the patches.
47 parallel autovacuum: Propagate track_cost_delay_timing to parallel workers Committed 14 msgs Oct 5, 16:28
This thread discusses a small patch to correct an oversight in parallel autovacuum. The `track_cost_delay_timing` GUC, which enables reporting of cost-based vacuum delay time, was not being propagated to parallel autovacuum workers. This meant that if the GUC was enabled mid-flight, workers would not report their sleep time, leading to incomplete or inaccurate delay metrics. The proposer argues this impacts delay reporting and should be backported to PG19, where parallel autovacuum was introduced. Reviewers agreed that this is a bug and an oversight, supporting the backport. Discussions refined the patch to ensure any accumulated delay is reported even if the GUC is turned off before `parallel_vacuum_main()` completes, aligning with existing behavior in `track_wal_io_timing`. The naming of the shared parameter was also adjusted for consistency.
48 tuplesort_putdatum() does not account for tuple memory Committed 6 msgs Oct 5, 12:29
This thread discusses a memory accounting bug in `tuplesort_putdatum()`. The proposer identified that memory for copied pass-by-reference Datums was not properly accounted against `work_mem`, leading to inaccurate memory reporting and `work_mem` limits not being enforced. This could result in more data being held in memory than intended before spilling to disk. The bug, introduced in commit `6ed83d5fa55`, was confirmed by reviewers through `EXPLAIN ANALYZE` and additional queries, which now correctly spill to temporary files with the patch. The proposer explained the bug was discovered during AIO subsystem exploration with the help of an AI agent.
49 [PATCH] pg_walsummary: suppress limit output with --quiet Patch Review 4 msgs Oct 5, 09:29
This thread addresses a bug in `pg_walsummary` where it incorrectly printed 'limit' lines, such as relation creation or truncation messages, even when the `--quiet` option was enabled. The proposer submitted a patch to ensure these specific outputs are suppressed under `--quiet`. Reviewers suggested simplifying the test cases and improving code clarity by repositioning the print statement. The proposer incorporated all feedback, creating a revised patch that is now cleaner and confirmed to be applicable to relevant back-branches (v17 and v18). The patch is now awaiting committer review.
50 Persist slot invalidations before publishing them Patch Review 35 msgs Oct 5, 13:29
This thread discusses a race condition where a replication slot can be marked invalid in shared memory before its state is persisted to disk. If persistence fails, the slot is in an inconsistent state, potentially leading to data loss. The proposer introduced a patch series to ensure invalidation is persisted before being published in shared memory, using a new function `ReplicationSlotPersistInvalidation()`. Discussions revolved around the complexity of the serialization mechanism, the handling of `update_inactive_since` in `ReplicationSlotReleaseInternal()`, and the interface for `SaveSlotToPath()`. Reviewers suggested simplifying the release logic and wrapping `SaveSlotToPath()`. The proposer incorporated these suggestions, and the latest patches reflect these changes, advancing the review process.