pgsql-hackers/digest / threads mailing list source generated · LLM-summarized

The pgsql-hackers
Weekly Digest

A reader's guide to the discussions, patches, and bickering on the primary development mailing list of PostgreSQL. Summaries are produced by a generative model — useful for orientation, not for citation.

Tuesday, October 6, 2026
50 hot threads
Note Thread statuses and summaries are generated by an LLM-based system and may contain inaccuracies. Always defer to the linked archive thread before quoting.

Hot Threads

showing 50 of 50 threads
01 [BUG?] check_exclusion_or_unique_constraint false negative Patch Review 29 msgs Oct 5, 23:28
opened Jan 20, 21:35 ·last activity 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.

Recent reply
Oct 5, 22:36 The latest reply, from the proposer, acknowledges a reviewer's feedback regarding a real behavior change where the patch causes `update_missing` to be reported sooner for in-flight `INSERT`s compared to master. The proposer confirms the commit message will be updated to reflect this, justifying the change by stating that master's current behavior in this specific race condition is not a guaranteed outcome.
archive ↗
02 Remove invalid SS2/SS3 handling from EUC-KR routines Patch Review 10 msgs Oct 6, 02:29
opened May 12, 06:09 ·last activity Oct 6, 02:29

This thread addresses inconsistencies in PostgreSQL's handling of EUC-KR encoding, specifically concerning the single-shift codes SS2 (0x8E) and SS3 (0x8F). The proposer pointed out that the KS X 2901 standard for EUC-KR only defines 1-byte (ASCII) and 2-byte (KS X 1001) sequences, making 3-byte sequences (involving SS2/SS3) invalid. However, PostgreSQL's documentation and some internal functions (pg_euckr_mblen()) incorrectly supported 3-byte sequences via shared helpers, despite pg_euckr_verifychar() already rejecting them. The proposed patch aims to correct this by implementing EUC-KR-specific multibyte functions, updating the maxmblen from 3 to 2, and fixing the documentation. Reviewer 1 provided a positive assessment. Reviewer 2 identified that the change impacts query planning (width calculations), TOAST table creation, and function behavior (e.g., lpad()). The proposer has since provided updated patches (v4) with regression tests and detailed explanations of the observed behavior changes, recommending that this change should be applied to master only.

Recent reply
Oct 6, 01:40 In the latest reply, the proposer submitted v4 of the patch, split into two commits: one for adding regression tests and another for the core fix. The proposer acknowledges that the change impacts query plan width calculations, TOAST table creation for new tables, and the behavior of functions like lpad()/rpad()/translate(). The proposer clarified that only the 0x8F (SS3) byte sequence behavior changes from 3 to 2 bytes, not 0x8E (SS2), and confirmed that all relevant tests pass. Due to the potential for query plan changes, the proposer recommends this patch for master only.
archive ↗
03 PSQL schema "describe" \dn is not escaping quotes Patch Review 17 msgs Oct 5, 18:29
opened Jul 28, 07:07 ·last activity 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.

Recent reply
Oct 5, 18:18 The latest reply from reviewer 5 provides a detailed review of patch v7, confirming the core quoting fix but highlighting new concerns. The reviewer suggests splitting the patch due to user-visible output changes affecting backporting. It also points out inconsistencies in `\dn`'s no-match behavior compared to `\dt`, including abortion with `ON_ERROR_STOP`, and notes issues with `\dn *` producing multiple headers in CSV output. Several minor code nits are also identified.
archive ↗
04 aio: worker: Free SMGR objects when idle Discussing 14 msgs Oct 5, 19:30
opened Sep 18, 07:28 ·last activity 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.

Recent reply
Oct 5, 18:43 The latest reply from commenter 1 acknowledges that `FileAccess()` already updates the LRU list, potentially reducing the overhead of an LRU-based solution. However, the commenter still believes the proposer's current v3 patch, which uses an entry cap and idle worker cleanup, is sufficient for the immediate problem, deferring the final decision to the proposer and reviewer 1.
archive ↗
05 [PATCH] pg_upgrade: add --initdb option to create the new cluster automatically Discussing 19 msgs Oct 5, 13:29
opened Jul 10, 10:41 ·last activity 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.

Recent reply
Oct 5, 12:49 The latest reply from a reviewer points out several critical issues with the current v6 patch for the `--initdb` option in `pg_upgrade`. The reviewer highlights that the `--check --initdb` dry run skips most essential compatibility checks, the utility fails to locate its binary without the `-B` option, and the design incorrectly disallows the `-O` option (for new cluster postmaster settings) when `--initdb` is used. These points indicate significant unresolved design and functional problems.
archive ↗
06 PG19: two RI fast-path issues found while testing the batching revert Discussing 18 msgs Oct 5, 13:29
opened Sep 10, 16:02 ·last activity 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.

Recent reply
Oct 5, 13:18 The latest reply by the reviewer reveals newly discovered issues with the RI fast path, specifically concerning the direct calling of cast functions which can lead to incorrect error handling (e.g., internal "function returned NULL" instead of an FK violation) or stale cast definitions. The reviewer proposes new patches to address these by selectively avoiding the fast path and fixing similar issues in older RI comparison paths.
archive ↗
07 UNDO with constant time recovery (CTR) Patch Review 10 msgs Oct 6, 02:29
opened Sep 28, 22:28 ·last activity Oct 6, 02:29

This thread discusses the implementation of UNDO logs in PostgreSQL to enable constant-time recovery (CTR) for file system operations. The proposer based this work on earlier efforts like ZHeap and Zedstore but diverged by initially focusing on FILEOPS without changing the HEAP access method. The initial patch set (v2) received significant review. Commenter 1 pointed out issues such as test dependencies, incomplete cleanup after `CREATE DATABASE` crashes, and server panics due to filesystem errors within critical sections. Reviewer 2 questioned the "constant-time recovery" claim, as measurements indicated linear time complexity for rollback and crash recovery, and raised concerns about UNDO operations outside transactional boundaries and their implications for unlogged relations. The proposer has acknowledged the feedback and is in the process of rebasing and reorganizing the patch series.

3 recent replies
Oct 6, 01:58 In the latest reply, the proposer acknowledges reviewer 2's detailed comments. The proposer agrees to address exceptions for file removal during rollback, noting that WAL log changes are not suitable for UNDO records. The proposer will also investigate the possibility and implications of using UNDO outside transactional boundaries, specifically for DDL and REINDEX CONCURRENTLY, and re-verify that UNDO is correctly disabled for unlogged relations.
Oct 5, 21:23 The latest messages show the proposer acknowledging specific issues identified by reviewers, including test dependencies, orphaned directories after crashes, and server panics due to filesystem errors. The proposer stated that these oversights are being corrected, and they are reorganizing the patch series. Another commenter provided detailed measurements on ROLLBACK latency and crash recovery duration, showing linear rather than constant-time performance, consistent with the README's sparse case assumption.
Oct 5, 19:53 In the latest replies, the proposer acknowledges the detailed feedback from reviewers and testers, confirming several identified issues such as incomplete directory cleanup and server panics. The proposer also engages with comments on the 'constant time recovery' claim, clarifying the intended scope of 'O(1) from the client's perspective' for sparse operations versus the total undo work. The proposer is working on reorganizing and correcting the patch series based on the review.
archive ↗
08 [PATCH] Fix segmentation fault caused by reentrancy in RI_Fkey_cascade_del (ri_triggers.c) Patch Review 12 msgs Oct 5, 15:29
opened May 29, 15:32 ·last activity 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.

Recent reply
Oct 5, 15:03 The latest reply is from the proposer, attaching a new v5 version of the patch. The proposer states that the patches have been rebased to apply cleanly on the current master branch. They welcome further reviews, indicating that the submission is ready for another round of checks, likely after the rebase.
archive ↗
09 [PG19]pg_verifybackup never finishes on a gzip-compressed tar backup Patch Review 3 msgs Oct 6, 02:29
opened Oct 6, 01:15 ·last activity Oct 6, 02:29

The proposer reported a bug in PostgreSQL 19 where pg_verifybackup fails to terminate (hangs indefinitely) when verifying a gzip-compressed tar-format backup generated by pg_basebackup with WAL streaming. This occurs because pg_basebackup produces a pg_wal.tar.gz file that is a concatenation of multiple gzip members, one for each tar header. The astreamer_gzip_decompressor_content() function within pg_waldump (used by pg_verifybackup) incorrectly loops infinitely after decompressing the first gzip member, as inflate() continuously returns Z_STREAM_END without processing subsequent members. The proposer provided a patch to resolve this by calling inflateReset() when Z_STREAM_END is encountered, allowing the decompression to continue to the next gzip member. A test case was also included. Commenter 1 inquired about a potential duplicate issue, but the proposer clarified it is a distinct problem.

Recent reply
Oct 6, 01:48 In the latest reply, the proposer clarified that the reported issue is not a duplicate of a previously identified truncated backup problem. The proposer explained that the fix for the truncated backup issue does not resolve the persistent hanging behavior experienced with gzip-compressed tar backups containing concatenated gzip members, confirming that the current patch addresses a distinct and separate bug.
archive ↗
10 Parallel autovacuum: DROP DATABASE WITH (FORCE) fails on the parallel workers Patch Review 7 msgs Oct 6, 02:29
opened Sep 28, 00:00 ·last activity Oct 6, 02:29

The proposer discovered a bug in parallel autovacuum (PG19+) causing DROP DATABASE WITH (FORCE) to fail with a permission denied error when parallel autovacuum workers are active in the target database. The problem arises because the autovacuum leader worker reports an InvalidOid for its role, while its parallel workers correctly report BOOTSTRAP_SUPERUSERID. This inconsistency causes TerminateOtherDBBackends() to incorrectly deny termination. Reviewer 1 suggested a broader fix to cover general parallel query workers spawned by background workers, advocating for the leader's roleId to be used for all lock group members. Subsequent patch versions (v2, v3) adopted this generalization. Reviewer 1 later identified a concurrency issue in v3 related to dereferencing `lockGroupLeader` without adequate locking. The latest patch (v4) from the proposer addresses this by acquiring the leader's partition lock and re-validating the leader before accessing its roleId, simplifying the check and making the fix robust. The fix is intended for backpatching to PG14.

3 recent replies
Oct 6, 01:00 In the latest reply, the proposer submitted v4 of the patch, addressing reviewer 1's concurrency concerns. The updated patch now acquires the partition lock and rechecks the leader's validity before accessing its roleId, enhancing safety. The databaseId test was removed as redundant due to the recheck. The proposer confirmed that the new test and role matrix still yield the expected results, indicating the robustness and correctness of the revised approach.
Oct 6, 00:27 The latest reply from reviewer 1 highlights a critical flaw in the current patch (v2/v3). The reviewer points out that the patch dereferences a `PGPROC` pointer for the `lockGroupLeader` without holding the appropriate lock. This creates a race condition where if the leader process exits and its `PGPROC` structure is reused, the patch could incorrectly access an unrelated backend's role ID, compromising the integrity of permission checks. The reviewer suggests acquiring the leader's partition lock and revalidating the leader before proceeding.
Oct 5, 03:35 The latest reply, from reviewer 2, confirms that the proposed v2 patch correctly addresses the issue, with the new test case passing. The reviewer also provides a rebased version of the patch and notes that the solution covers a superuser's parallel worker and seems to work for the background worker case as well.
archive ↗
11 Logical replication: lost updates/deletes and invalid log messages caused by SnapshotDirty + concurrent updates Discussing 4 msgs Oct 5, 23:28
opened Sep 10, 23:56 ·last activity 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.

Recent reply
Oct 5, 22:35 The latest reply, from the proposer, discusses a behavior change identified by a reviewer in a related thread concerning how the patch handles in-flight `INSERT`s during logical replication. The proposer explains that the patch causes an `update_missing` report sooner than master, arguing that master's current behavior is racy and not a guaranteed wait. The commit message for the fix has been updated to reflect this distinction.
archive ↗
12 pg_resetwal: refuse to run when backup_label exists Discussing 10 msgs Oct 6, 00:29
opened Oct 2, 03:35 ·last activity 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.

3 recent replies
Oct 5, 23:53 The latest reply from reviewer 3 announces that the commitfest entry for the `pg_resetwal` patch has been moved to "Ready for Committer". The reviewer confirmed that the code and tests are satisfactory from their perspective. Although one reviewer still expressed reservations about the patch's overall value, the core team decided to proceed. The specific question of whether the `-f` flag should override the `backup_label` check is left for the committer to make the final determination.
Oct 5, 22:32 The latest reply, from commenter 1, states that they don't see the value of the patch, implying their opinion might not be relevant for the decision on whether `-f` should override the `backup_label` check. The commenter also provides a general note about mailing list posting style.
Oct 5, 15:04 The reviewer 1 confirms that the latest patch version (v3) looks fine from their perspective and that they have no further changes to suggest. However, reviewer 1 explicitly states that the patch will be moved back to "Needs review" status, indicating that a decision is still pending from an earlier commenter regarding whether the `-f` flag should override the `backup_label` check.
archive ↗
13 [PG19] COPY (query) TO ... (FORMAT json) uses the table's column names Patch Review 4 msgs Oct 6, 02:29
opened Oct 5, 19:05 ·last activity Oct 6, 02:29

The proposer identified a bug in PostgreSQL 19's COPY (query) TO ... (FORMAT json) functionality, where the JSON keys were incorrectly derived from the underlying table's column names instead of the query's projected or aliased column names. This led to incorrect JSON output, particularly with UNION ALL queries or explicit column aliases. The root cause was traced to CopyToJsonOneRow() using the table's descriptor when a scan node did not project. The proposer suggested an initial fix, but a reviewer provided an optimized v2 patch. This v2 patch efficiently reuses the copied tuple and stamps it with the query's descriptor using heap_copy_tuple_as_datum(), avoiding a performance hit. Commenter 1 thoroughly tested v2, confirming it resolved the reported issues across various query patterns and even fixed an unmentioned edge case involving views and CTEs, with no performance regressions. The proposer also confirmed the efficacy and efficiency of v2.

3 recent replies
Oct 6, 01:04 In the latest reply, the proposer confirmed that the optimized v2 patch provided by the reviewer successfully fixes the reported bug. The proposer also validated that their previous test cases no longer expose the issue and acknowledged the performance benefits of the reviewer's approach over the proposer's initial suggestion, particularly regarding the avoidance of tuple re-formation per row.
Oct 5, 22:37 The latest reply, from reviewer 2, reports comprehensive testing of the developer's v2 patch. The reviewer confirms the patch fixes the original bug and an additional case involving views/CTEs with inherited tables, producing identical output to `row_to_json()` in all 37 tested scenarios without performance issues or asserts firing.
Oct 5, 21:05 A reviewer confirmed the reported bug where COPY ... FORMAT json uses table column names instead of query column names. The reviewer proposed a revised patch that avoids the performance regression of the original suggestion. This new patch uses heap_copy_tuple_as_datum() to correctly stamp the tuple with the query's descriptor, with timings indistinguishable from master. The reviewer asked the proposer to test this solution.
archive ↗
14 Parallel Apply Patch Review 12 msgs Oct 5, 13:29
opened Apr 30, 14:39 ·last activity 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.

Recent reply
Oct 5, 13:05 The latest reply from a reviewer states that the current patch set for Parallel Apply (v21) no longer applies cleanly to the codebase. The reviewer requests a rebased version to continue the review process, indicating ongoing work on the patch set.
archive ↗
15 pgstat: allow a stats kind to use its own dedicated dsa/dshash Patch Review 6 msgs Oct 6, 02:29
opened Jul 15, 17:16 ·last activity Oct 6, 02:29

This thread proposes a new feature for PostgreSQL statistics, allowing certain statistics kinds (e.g., pg_stat_statements) to use their own dedicated DSA area and dshash. This aims to improve performance by reducing lock contention, enabling independent sizing of shared memory, and optimizing iteration for kinds with numerous entries. The initial patch (v1) introduced an 'own_hash' option and new APIs to retrieve dedicated dshash and DSA pointers. Reviewer 1 provided comprehensive feedback, suggesting improvements such as better handling of out-of-memory scenarios during insertion, reconsidering startup failure semantics for persistent statistics, and exploring whether a dedicated hash necessarily implies a dedicated DSA. The proposer released v2, addressing several of these points, including using DSHASH_INSERT_NO_OOM and refining DSA retrieval. The decision to keep 'own_hash' tied to a dedicated DSA was maintained for isolation. Reviewer 2 provided further feedback on v2, raising concerns about potential duplication in the global stats structure and suggesting simplifications for hash table management.

3 recent replies
Oct 5, 23:44 In the latest reply, reviewer 2 provided further comments on v2 of the patch. The reviewer suggested that the `pgStatLocal` structure could be simplified by eliminating duplicated hash table pointers, proposing that `kind_dsa/kind_hash` could always be set for variable-sized stats and that decision-making could be driven by a `kind_hash_valid` flag, potentially making `pgstat_get_hash_for_kind()` unnecessary. The reviewer also emphasized the need to preserve documentation regarding locking in `pgstat_reset_matching_entries_in_hash()`.
Oct 5, 21:28 The latest substantial reply, from the proposer, provides a rebased v2 patch. It addresses several of the reviewer's points, including improving OOM handling, optimizing `pgstat_reset_entries_of_kind`, and updating API usage. The proposer indicates that certain design choices, such as the failure semantics for loading persisted stats and the explicit tying of `own_hash` to a dedicated DSA, remain as originally proposed and require further discussion.
Oct 5, 21:13 The proposer rebased the patch and addressed several reviewer comments. They updated insertion paths to use DSHASH_INSERT_NO_OOM and fixed pgstat_get_dsa_for_kind() to use dshash_get_dsa_area(). They also refined pgstat_reset_matching_entries() to scan specific hashes. The proposer deferred changes on startup failure semantics and kept own_hash tied to a dedicated DSA, stating they require more discussion on these points.
archive ↗
16 [PATCH v1] Fix races in Windows pthread emulation Discussing 6 msgs Oct 5, 20:28
opened Jul 23, 18:18 ·last activity 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.

2 recent replies
Oct 5, 20:25 The latest reply from the proposer acknowledges the reviewer's feedback and the additional reproductions of the hang on AMD64, clarifying that the issue is not exclusive to ARM64. The proposer suggests considering whether to separate the `pthread_once` change from the mutex fix, as its performance implications and backpatch compatibility are still under evaluation.
Oct 5, 18:00 The latest reply from commenter 2 confirms the broader scope of the reported race condition. The commenter states that the issue also occurs on AMD64 (x86_64) systems within the buildfarm and is reproducible locally on a Windows x86_64 VM, offering to assist with patch testing. This reinforces the necessity and impact of the proposed fix across different Windows architectures.
archive ↗
17 NOT NULL NOT ENFORCED Patch Review 18 msgs Oct 5, 07:29
opened Sep 4, 02:56 ·last activity 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.

Recent reply
Oct 5, 07:06 The reviewer acknowledges that the updated commit message for v6 of the patch is much clearer. They state their intention to compile the patch and perform additional syntax checks soon, indicating the ongoing nature of the review process for this feature.
archive ↗
18 stale comment in struct AlteredTableInfo Committed 2 msgs Oct 6, 02:29
opened Sep 23, 03:02 ·last activity Oct 6, 02:29

A commenter identified a stale and incomplete comment within the struct AlteredTableInfo definition. The existing comment, "/* Objects to rebuild after completing ALTER TYPE operations */", did not fully reflect the scope of the objects, as they also apply to "ALTER COLUMN SET EXPRESSION" operations. The commenter proposed an updated comment to accurately include both types of operations, enhancing code clarity.

Recent reply
Oct 6, 01:28 A committer applied the suggested correction to the comment in struct AlteredTableInfo, acknowledging the commenter's contribution to improving code readability and accuracy.
archive ↗
19 enhancing pg_basebackup speeds up to ~23Gbps (small fixes + io_uring/Direct I/O) Patch Review 6 msgs Oct 5, 16:28
opened Aug 13, 08:27 ·last activity 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.

Recent reply
Oct 5, 16:24 A reviewer starts examining patch 0004, which increases `SINK_BUFFER_LENGTH`. The reviewer suggests allocating the buffer with `palloc_aligned()` for potentially faster `pread()` operations. The reviewer also raises a concern about the logic for checksum verification, specifically questioning the assumption that the read count must be a multiple of `BLCKSZ` when using larger buffers.
archive ↗
20 amcheck: add index-all-keys-match verification for B-Tree Patch Review 13 msgs Oct 5, 09:29
opened Feb 17, 09:19 ·last activity 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.

Recent reply
Oct 5, 09:18 The latest reply, from reviewer 3, identifies a compilation issue in the `v6` patchset caused by recent changes in the `master` branch related to index prefetching. The reviewer provides a `v6fixup` patch to address this by adjusting calls to `table_fetch_tid()` and `table_tuple_fetch_row_version()`, and requests the proposer to verify the fix.
archive ↗
21 WAL segment file descriptor leak on read errors can PANIC the server Patch Review 14 msgs Oct 5, 22:29
opened Sep 21, 07:12 ·last activity 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.

3 recent replies
Oct 5, 22:27 The reviewer, Commenter 2, clarified that checking for a negative value for the file descriptor (ws_file) is likely sufficient for cleanup. The reviewer also requested a clearer rewording for the comment of the XLogReaderRegisterReset() function to specify its intended use for descriptors not managed by other cleanup mechanisms. The reviewer is awaiting feedback on their latest patch from the proposer.
Oct 5, 09:59 A commenter notes the difference between BasicOpenFile() and PathNameOpenFilePerm() regarding file descriptor tracking. They highlight that BasicOpenFile() can lead to indefinite accumulation of stale descriptors, reinforcing the need for the proposed fixes by relating it to previous discussions on file handling issues.
Oct 5, 09:20 The latest reply, from reviewer 3, provides feedback on the alternative patch proposed by reviewer 2. The reviewer confirms that the pattern of a callback calling another callback is acceptable in this context. They suggest refining the condition for checking an open file descriptor from `== -1` to `>= 0` and improving the comment for the `XLogReaderRegisterReset` function for better clarity regarding its intended use.
archive ↗
22 Vacuum statistics Patch Review 92 msgs Oct 4, 14:28
opened Dec 19, 10:37 ·last activity 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.

Recent reply
Oct 4, 14:07 The latest reply from a reviewer identifies two new issues in the proposed v44 patch set. Firstly, it notes that parallel VACUUM operations appear to double-count resource usage, as worker index usage is reported both separately and within the leader's table-level usage. Secondly, the reviewer points out that the extension's reset functions retain default EXECUTE privileges for PUBLIC, allowing non-superusers to clear accumulated statistics, which is inconsistent with core pg_stat_reset* functions that restrict access.
archive ↗
23 Proposal: SELECT * EXCLUDE (...) command Discussing 9 msgs Oct 5, 10:29
opened Jan 8, 10:27 ·last activity 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.

Recent reply
Oct 5, 09:39 The proposer acknowledges the reported bugs and confirms that the upcoming v3 patch will address these by switching to ColumnRef for column matching. They also state that v3 will include support for the new standard syntax with REPLACE and RENAME clauses, and is expected to be posted within the week.
archive ↗
24 pg_dump/restore failure (dependency?) on BF serinus Discussing 7 msgs Oct 5, 13:29
opened Apr 8, 03:41 ·last activity 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.

3 recent replies
Oct 5, 13:10 The latest reply from a reviewer simply states "[PATCH REMOVED]", indicating that a previously proposed patch (likely intended to fix the identified catalog check race) is no longer part of the active discussion, and the core issue remains open.
Oct 5, 11:48 A developer reports being able to reliably reproduce the `pg_restore` failure using an AI-generated script. The developer identifies the root cause as a server-side "catalog check race" where a partitioned primary key index is left permanently invalid due to concurrent `ATTACH` statements failing to observe the complete set of valid indexes. No fix has been proposed yet.
Oct 5, 11:20 The latest reply from commenter 2 confirmed the reproducibility of the failure and identified its root cause as a server-side catalog check race during concurrent `ATTACH` statements on 2-level partitioned tables. This race can leave a partitioned primary key index permanently invalid, causing the `pg_restore` error, and a fix is yet to be found.
archive ↗
25 Let an ordering index scan hand its ORDER BY value to the target list Discussing 5 msgs Oct 5, 18:29
opened Oct 1, 19:10 ·last activity 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.

3 recent replies
Oct 5, 18:14 The latest reply from the proposer further clarifies the intent behind the patch. The proposer reiterates that the aim is an optimization to prevent redundant value re-computation and to enable reporting scores directly from an index scan, without altering the fundamental meaning or behavior of the operator itself, ensuring its value remains consistent regardless of the execution plan.
Oct 5, 16:46 The latest reply from the proposer clarifies their motivation for the proposed change, acknowledging commenter 1's point that an operator's meaning should not be index-dependent. The proposer indicates that while the BM25 case initially highlighted the issue, the core motivation for the patch is the optimization of avoiding redundant computations rather than solely correcting potentially 'incorrect' results from specific operators.
Oct 5, 14:39 The reviewer expresses fundamental concerns about the proposal, particularly regarding the claim of "incorrect results," suggesting it might be an issue with the extension's definitions. The reviewer reiterates a core principle: an index's presence should not affect the row-data output of a query. The reviewer also asks for clarification on what "value" the proposer intends to substitute, implying a conceptual gap in the proposal.
archive ↗
26 issues with eager aggregation Committed 8 msgs Oct 6, 00:29
opened Sep 17, 12:12 ·last activity 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.

2 recent replies
Oct 5, 23:56 The latest reply from the primary developer confirms that the patches addressing the three identified bugs in eager aggregation have been successfully pushed and back-patched to v19. This marks the resolution and commitment of the core functional issues reported in the thread. The discussion regarding the division-by-zero behavioral change concluded with an agreement to update documentation, rather than modify the eager aggregation logic itself.
Oct 5, 01:44 The implementer provided three patches to address the identified bugs in eager aggregation. The first patch corrects incorrect grouping caused by type mismatches in `get_expression_sortgroupref()`. The second patch fixes an internal error by ensuring FDW fields are cleared when a join relation is copied. The third patch resolves an issue where extra grouping keys for partial aggregation were not properly handled for child relations. The implementer plans to commit these fixes and back-patch them to v19, pending any objections.
archive ↗
27 pg_*_advice: tsv load failure, etc. Committed 21 msgs Oct 5, 21:28
opened Aug 27, 17:18 ·last activity 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.

3 recent replies
Oct 5, 20:33 The latest message, a v7 patch set, was submitted by the main developer. It adds one more patch (0003) to fix a problem with child_append_relid_sets being lost when (Merge)Append nodes are elided, impacting multi-level partitioning. This brings the total fixes in this set to five, addressing deficiencies in child_append_relid_sets handling, NO_GATHER advice, and JOIN_ORDER feedback. The developer plans to commit these and back-patch to v19, acknowledging prior mistakes and remaining unfixed issues.
Oct 5, 17:01 The latest reply from the main committer provides an update on ongoing `pg_plan_advice` fixes. A new patch set is introduced, addressing several critical bugs related to `child_append_relid_sets`, missing `PARTITIONWISE` advice, and incorrect `NO_GATHER` emission, along with a fix for `JOIN_ORDER` feedback. The committer notes that challenges remain for set-operation advice and `NO_GATHER` on partition children, indicating these complex issues require further work.
Oct 4, 12:38 The latest reply provides a documentation patch for item 5, clarifying that `pg_plan_advice` and stash advice changes are silently ignored by already-cached generic plans. The commenter explains that `ANALYZE` is not a reliable method for invalidating these plans. The patch suggests updating the "Limitations" section to recommend `DISCARD PLANS` for the current session and to expect other sessions to re-plan, documenting the current behavior rather than changing it.
archive ↗
28 Limiting WAL retained for archiving, like max_slot_wal_keep_size Rejected 3 msgs Oct 5, 19:30
opened Oct 5, 15:13 ·last activity 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.

2 recent replies
Oct 5, 19:21 In the latest reply, the proposer acknowledged the commenter's concerns, agreeing that allowing the server to intentionally lose WAL critical for PITR is not an acceptable design. They confirmed that their archiving tool can independently handle this scenario by reporting success for segments it chooses to drop, thereby managing disk space outside of PostgreSQL's core logic.
Oct 5, 15:13 The latest and only reply is the initial proposal. The proposer suggests a new server setting similar to `max_slot_wal_keep_size` but for WAL archiving. This would prevent unarchived WAL from indefinitely filling disk space by allowing the server to remove old segments, while also logging this action to enable backup tools to detect gaps and prompt new base backups. The proposer is seeking initial feedback on this core feature idea.
archive ↗
29 Progress reporting: a debug trace and a test framework Patch Review 1 msgs Oct 6, 02:29
opened Oct 6, 01:29 ·last activity Oct 6, 02:29

The proposer submitted version 4 of a patch series focused on progress reporting, which includes a debug trace and a new test framework. During the rebase process, the test framework's completeness check successfully identified a deviation: a commit (1378aa13430) had introduced PROGRESS_VACUUM_CURRENT_INDEX_RELID without a corresponding description in the progress.h specification. The proposer clarified this counter represents the OID of the index currently being processed by a worker and updated the patch to correctly classify it, demonstrating the efficacy of the completeness check in maintaining specification integrity. Aside from this update during rebase, the four bug fixes (0001-0004) and the PROGRESS_DEBUG trace (0005) from the previous version remain unchanged.

Recent reply
Oct 6, 01:29 In the latest reply, the proposer submitted v4 of the patch, noting that a completeness check during rebase flagged an inconsistency where PROGRESS_VACUUM_CURRENT_INDEX_RELID was introduced without being described in the progress.h specification. The proposer classified this as the OID of the index a worker is currently processing, demonstrating the effectiveness of the test framework in identifying such drifts. No other changes were made from the previous version.
archive ↗
30 [PG19] eager aggregation gives wrong results because of bpchar_ops Patch Review 1 msgs Oct 6, 02:29
opened Oct 6, 02:19 ·last activity Oct 6, 02:29

The proposer identified a bug in PostgreSQL 19 where eager aggregation produces incorrect results for bpchar columns due to inconsistencies in how bpchar equality and the equalimage functions in bpchar_ops opclasses handle trailing spaces. This discrepancy leads to miscounts in queries involving bpchar joins and eager aggregation. The issue also affects nbtree deduplication. The proposer demonstrated the bug with a clear test case, showing a regression from PG18. A fix is proposed involving dropping a specific support function from the affected opclasses, similar to a prior fix for interval_ops. This solution, however, would also inadvertently end deduplication for char(n), which was previously considered safe. The proposer suggests treating this as an open item given its novelty in PG19.

Recent reply
Oct 6, 02:19 The proposer provided two patches to address the identified bug. The first patch aims to fix the eager aggregation issue by dropping support function 4 from both bpchar_ops and bpchar_pattern_ops opclasses, necessitating a catalog version bump. The second patch introduces a dedicated test case to validate the correctness of the fix and ensure the aggregation behaves as expected.
archive ↗
31 [PATCH v1] amcheck: Allow interrupting the child-level rightlink walk Committed 5 msgs Oct 5, 12:29
opened Jul 17, 15:14 ·last activity 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.

Recent reply
Oct 5, 10:39 The committer announces that the patch has been committed. This confirms the integration of the proposed change, which adds `CHECK_FOR_INTERRUPTS()` to a previously uninterruptible loop in `amcheck` to enhance its resilience when dealing with potentially corrupt indexes.
archive ↗
32 doc: Document Linux cgroup memory limits Patch Review 11 msgs Oct 5, 15:29
opened Oct 1, 22:19 ·last activity 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.

3 recent replies
Oct 5, 15:01 Reviewer 1 confirms that the latest patch version (v5) reads correctly, particularly regarding the softened sentence about memory usage and the recommendation to leave a safety margin, which aligns with existing documentation style. As only the last paragraph changed and the documentation still builds cleanly, reviewer 1 reaffirms that the patch is "Ready for Committer".
Oct 5, 12:18 The proposer submits v5, which addresses reviewer 2's broader concerns by softening the language to avoid implying that specific GUC settings alone are sufficient to prevent OOMs. The updated patch now recommends leaving a safety margin below the cgroup memory limit, aligning with existing documentation for `work_mem`.
Oct 5, 06:37 A commenter supports the documentation but expresses broader concerns about PostgreSQL's memory management, citing issues like memory leaks and `work_mem` not always functioning as expected. They suggest that running PostgreSQL on cgroups2 without specific `vm.overcommit_memory` settings or regular connection rotation could be risky and lead to outages.
archive ↗
33 COPY FROM ... WHERE fails for negated operators Patch Review 2 msgs Oct 5, 20:28
opened Oct 5, 17:07 ·last activity 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.

2 recent replies
Oct 5, 17:48 The latest reply from a core committer concurs with the proposed patch. The committer acknowledges that while the `fix_opfuncids()` mechanism itself is something they hope to remove in the long term, the patch provides a correct and necessary fix for the immediate bug.
Oct 5, 17:07 The latest reply proposes a patch to fix a bug in `COPY FROM stdin WHERE` statements involving negated operators. The proposer explains that `DoCopy()` omits a crucial call to `fix_opfuncids()`, leading to a "cache lookup failed" error when `negate_clause()` attempts to resolve the operator with a zero argument. The patch aims to rectify this oversight.
archive ↗
34 Reducing relcache memory usage 2: shrink sizeof(RelationData) Patch Review 4 msgs Oct 5, 12:29
opened Sep 10, 09:43 ·last activity 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.

Recent reply
Oct 5, 11:48 The proposer reports the results of `pgbench` tests for the patch (now 0006 in v3) that lazily allocates `RelationPartitionInfo`. Running with 1000 partitions, tests designed to put maximum pressure on planning did not show any measurable performance regressions in Transactions Per Second (TPS) compared to the master branch. The proposer also rebased the patch set to the latest master.
archive ↗
35 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
opened Oct 4, 16:24 ·last activity 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.

3 recent replies
Oct 5, 19:21 The latest reply from the reviewer expresses gratitude for the thorough review, affirming that the proposer's analysis accurately reflects the patch's intent and correctness, particularly regarding pointer aliasing and plain-OR-only guards. Acknowledging a minor cosmetic suggestion for the inner loop, the reviewer suggests incorporating it at commit time to avoid resetting the review process.
Oct 5, 08:10 The reviewer completed a detailed analysis of the proposed patch, confirming its correctness and effectiveness in resolving the quadratic planning time issue. The review highlighted that the patch's logic is sound, it offers substantial performance improvements, and the new test cases are appropriate. The reviewer plans to update the patch's status to 'Ready for Committer' as soon as their commitfest account is fully operational.
Oct 4, 23:27 Commenter 1 confirmed the registration of the proposed patch in the commitfest, responding to the proposer's offer to review it. This action moves the discussion into a formal review process within the PostgreSQL development cycle.
archive ↗
36 Limiting WAL retained for archiving, like max_slot_wal_keep_size Discussing 2 msgs Oct 5, 22:29
opened Oct 4, 21:36 ·last activity 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.

2 recent replies
Oct 5, 21:52 The commenter acknowledged the severity of a stalled archiving process filling the disk but voiced strong reservations about the server automatically discarding unarchived WAL. Key concerns included the potential for unnoticed data loss during recovery, misleading success reports for broken backups, the non-durability of 'pg_stat_archiver' for recording critical gaps, and confusion for the archiver daemon. The commenter suggested an alternative: enhancing 'pg_stat_archiver' with metrics to track archiving progress, enabling early alerts without the risk of automatic WAL deletion.
Oct 4, 21:36 The proposer introduces a new feature proposal to address uncontrolled WAL growth during archiving failures. They suggest a server setting akin to `max_slot_wal_keep_size` for archiving, which would limit retained WAL segments. This mechanism would prevent disk saturation by automatically dropping old unarchived WAL and recording the event, signaling the need for a new base backup.
archive ↗
37 Reduce SyncRepLock contention on the commit path Patch Review 3 msgs Oct 5, 13:29
opened Sep 17, 18:37 ·last activity 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.

Recent reply
Oct 5, 13:24 The latest reply from the reviewer confirms that the proposer's latest patch (v3) addresses previous concerns regarding a test case for quorum replication and agrees with placing certain regression tests behind a flag. The reviewer concludes the patch series is "ready for commit-fest".
archive ↗
38 Serverside SNI support in libpq Patch Review 75 msgs Oct 5, 15:29
opened Dec 11, 00:34 ·last activity 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.

3 recent replies
Oct 5, 15:15 The latest reply from reviewer 2 provides a second version of additional tests for the SNI support patches. This version reworks the previously submitted tests to use inline tests and an ordinary SKIP block, adhering to the project's Test::More minimum requirements. Reviewer 2 confirms that these updated tests continue to apply cleanly on top of the proposer's latest patch set (v5) and pass successfully.
Oct 5, 12:26 The proposer acknowledges the positive review of the v5 patch set and expresses intent to commit them shortly. However, the proposer points out that the additional test cases provided by reviewer 3 use unsupported subtests and would need to be rewritten as inline tests.
Oct 4, 15:50 Reviewer 3 expressed gratitude for the fixes in the proposer's v5 patch set for SSL reload and cleanup issues, noting independent work on similar problems. The reviewer confirmed the five patches were solid and found no further problems. Additionally, the reviewer contributed a new patch adding more extensive tests for specific SNI configuration reload scenarios, including encrypted per-host keys and handling missing certificate files, which passed successfully with the v5 patches.
archive ↗
39 Fix "unexpected logical decoding status change" error; from concurrent logical decoding activation Patch Review 41 msgs Oct 5, 22:29
opened Jul 10, 04:18 ·last activity 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.

3 recent replies
Oct 5, 22:26 The proposer confirmed that two suggestions for the TAP test, adding a 'sync_slot' condition to 'injection_points_attach()' and a 'note()' after 'wait_for_event()', were incorporated into the updated patch. Additionally, comments explaining the behavior of retaining slots when the standby is lagging were revised based on earlier discussions with other participants.
Oct 1, 11:08 The reviewer provides feedback on the TAP test for the patch, suggesting two minor improvements: adding a `sync_slot` condition to `injection_points_attach()` for more precise testing and including a `note()` after `wait_for_event()` for clearer test output. This indicates a focus on refining the test suite's robustness and clarity.
Sep 29, 00:35 In the latest exchange, commenter 1 further clarified why the system can safely proceed with slot synchronization even when a remote slot's restart_lsn is ahead of the standby's current replay_lsn. They explained that the process will wait for replay to catch up, and that mechanisms are in place to prevent a bad slot from being persisted if a deactivation record is replayed during this waiting period. They committed to improving the patch comments for better clarity.
archive ↗
40 REPACK (CONCURRENTLY) might keep dropped-column data Patch Review 10 msgs Oct 5, 16:28
opened Sep 30, 06:27 ·last activity 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.

3 recent replies
Oct 5, 15:46 A new contributor identifies additional scenarios where dropped column values can persist during a `REPACK (CONCURRENTLY)` operation, specifically with replayed `INSERT`s, `UPDATE`s that move rows between partitions, `COPY`, `MERGE`, and `INSERT ON CONFLICT`. The contributor proposes a patch that clears dropped columns in `restore_tuple()` to cover all these cases, eliminating the need for the previously added block in `prepare_concurrent_update()`. An additional concurrent `INSERT` test case is also suggested.
Oct 5, 11:35 A committer confirms that the patch addressing the `REPACK (CONCURRENTLY)` bug has been pushed to both relevant branches. The committer also details a post-commit internal refinement to prevent a potential race condition where `slot_getsomeattrs()` could inadvertently overwrite changes made to `tts_isnull[]` for dropped attributes.
Oct 5, 03:51 The latest message from a reviewer provides a patch to address minor cleanup issues in the test suite. Specifically, it adds `repack_dropped` to the Makefile, ensuring its proper inclusion in the build system, and adds a `DROP FUNCTION` statement for `injection_points` to the test teardown, improving test hygiene.
archive ↗
41 Fix reindexdb with parallel index-level conrurrent run Discussing 4 msgs Oct 5, 20:28
opened Oct 2, 17:03 ·last activity 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.

3 recent replies
Oct 5, 19:42 The latest reply agrees with the suggestion to make `reindexdb` return an error when parallel jobs are used with `REINDEX CONCURRENTLY` for multiple indexes on the same table. The commenter argues that a fix would only remove the error without providing actual speed benefits, as concurrent reindexing of multiple indexes on the same table cannot truly parallelize due to deadlock avoidance requirements.
Oct 5, 02:52 The latest reply from commenter 1 questions the overall utility of parallel index-level concurrent reindexing due to expected long wait phases. The commenter expresses concerns about code duplication in the proposed patch and suggests that rethinking the `gen/run` interfaces might be necessary, or perhaps this specific mode isn't worth the implementation effort, implying an error might be a more suitable outcome.
Oct 3, 01:48 The reviewer provided a detailed analysis of the proposed patch, confirming the original bug fix but highlighting three new issues: loss of parallelism for single-index tables, incomplete cancellation/failure handling, and back-patching incompatibilities with older PostgreSQL versions. The reviewer then presented an improved patch (v2) that addresses these identified regressions, including specific adaptations for REL_18 and REL_17, and shared performance measurements. An outstanding issue regarding multi-index table processing was also noted.
archive ↗
42 two small tab-completion patches (ALTER CONSTRAINT, CHECK modifiers) Proposed 1 msgs Oct 5, 21:28
opened Oct 5, 21:24 ·last activity 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.

Recent reply
Oct 5, 21:24 The proposer submitted two small patches to improve psql's tab-completion. The first patch adds options like DEFERRABLE/ENFORCED/INHERIT and their NOT/NO forms for ALTER CONSTRAINT. The second patch adds NOT ENFORCED, NOT VALID, NO INHERIT for ADD CHECK statements.
archive ↗
43 Direct TOAST v2, faster, smaller and no migration needed Discussing 28 msgs Oct 5, 16:28
opened Sep 7, 03:13 ·last activity 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.

3 recent replies
Oct 5, 15:37 The proposer clarifies that index maintenance is avoided for Direct TOAST if no "plain" TOAST is used in the same table, as VACUUM would collect zero dead tuple TIDs and thus skip index cleanup. The proposer also reiterates that the `ALTER TABLE ... SET (toast_flavour = 'direct')` operation does not perform a `CREATE INDEX`, but rather updates catalog tables directly, claiming it needs an exclusive lock for less than a millisecond. The proposer also engages with the point about independent repack-ability of TOAST tables, acknowledging it as an issue for cases where the main table is expensive to rebuild.
Oct 5, 13:17 The latest reply from a reviewer continues to challenge the proposer's claims about the extent of performance improvement and index maintenance avoidance for Direct TOAST. The reviewer reiterates concerns regarding the need for exclusive locks during index conversion and the loss of independent repack-ability for TOAST tables, emphasizing these as significant design drawbacks.
Oct 3, 16:26 The latest reply presents version 13 of the Direct TOAST patch series, rebased onto the current master branch. This update further refines the proposed index-free storage, enhancing its interoperability with 64-bit OID8 TOAST, incorporating self-pruning mechanisms, and improving vectored multi-block detoasting. It includes deterministic tests and benchmarks to demonstrate its performance advantages.
archive ↗
44 Report relation extension blockers within parallel lock groups Committed 12 msgs Oct 5, 20:28
opened Aug 31, 02:39 ·last activity 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.

3 recent replies
Oct 5, 19:55 The latest reply from a committer confirms that the patch has been "Committed." This indicates that the proposed fix for `pg_blocking_pids()` and its accompanying documentation improvements have been successfully integrated into the PostgreSQL codebase.
Oct 4, 18:49 The latest reply from reviewer 1 indicates full approval of the patch, including the refined documentation changes. The reviewer explicitly states that the patch is now considered 'LGTM' (Looks Good To Me) and has updated its status to 'Ready for Committer', signifying that it is prepared for integration into the PostgreSQL codebase.
Oct 4, 06:23 The proposer refined a previously submitted documentation patch, agreeing to reorganize the `pg_blocking_pids()` documentation by moving a sentence and creating a dedicated paragraph for parallel queries. The proposer suggested a slightly shorter version for the new paragraph to maintain conciseness.
archive ↗
45 examine_variable ignored CollateExpr Patch Review 6 msgs Oct 5, 05:29
opened Jan 13, 02:23 ·last activity 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.

Recent reply
Oct 5, 03:59 The latest message from a reviewer raises a new concern regarding the proposed patch (either v1 or v4). The reviewer demonstrates that the patch leads to worse query plan estimates (e.g., `rows=26` instead of `rows=100`) when a table lacks a statistics object but uses a `COLLATE` clause in a `WHERE` condition. The reviewer suggests a more conditional approach to stripping `COLLATE` clauses.
archive ↗
46 hashjoins vs. Bloom filters (yet again) Discussing 78 msgs Oct 5, 16:28
opened May 30, 00:55 ·last activity 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.

3 recent replies
Oct 5, 16:24 The proposer responds to a commenter who questioned the feasibility of estimating unmatched rows with current statistics. The proposer indicates that the current statistics might need to be improved, but it's not clear why the existing JOIN_SEMI estimates, as suggested by another reviewer, could not be used. The proposer also reiterates that the bottom-up approach, while more complex, offers significant benefits by optimizing join order, which the opportunistic approach cannot achieve.
Oct 5, 12:04 Reviewer 4 reiterates fundamental limitations in the current `eqjoinsel_semi()` for estimating unmatched rows, particularly for common cases lacking detailed statistics. The reviewer emphasizes that this difficulty is a strong argument for the "opportunistic approach" to Bloom filter implementation, which bypasses the need for such precise estimates.
Sep 29, 17:57 The proposer responds to concerns raised by reviewer 3 about the complexity of the bottom-up, cost-based Bloom filter approach. They argue against abandoning it, suggesting that existing JOIN_SEMI estimates might be sufficient for filter estimation or could be improved. The proposer views path changes from optimization as expected benefits, not issues, acknowledging the difficulty but expressing confidence in finding solutions.
archive ↗
47 Session in aborted transaction misses effective_wal_level change Patch Review 9 msgs Oct 5, 18:29
opened Sep 30, 17:18 ·last activity 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.

3 recent replies
Oct 5, 18:01 The latest reply from reviewer 1 (who also provided v2-v4 of the patch) agrees with previous reviewers' feedback regarding the most robust placement for the `AtEOXact_LogicalCtl()` call. The reviewer confirms that calling the function after resetting the top-level transaction ID is a more "bullet-proof" approach, thereby updating the patch to v4 to reflect this revised strategy.
Oct 5, 03:35 The latest message from a reviewer agrees with a previous commenter's concern about the placement of the `AtEOXact_LogicalCtl()` call. The reviewer argues that this function should be executed *after* `XactTopFullTransactionId` is reset, as arbitrary callbacks during memory cleanup could potentially consume a barrier if the transaction ID is still considered valid, which could lead to incorrect behavior.
Oct 4, 07:44 The latest message from reviewer 4 questions the placement of AtEOXact_LogicalCtl() in the updated patch. Specifically, they suggest it might be better to keep the function call after XactTopFullTransactionId is reset, as in an earlier patch version. The reviewer raises a concern that processing a barrier during AtCleanup_Memory() while the XID is still valid could potentially re-set XLogLogicalInfoUpdatePending after the call, implying the new placement might not fully close the gap the patch aims to fix.
archive ↗
48 Fix out-of-bounds array indexing in JsonValueList Patch Review 4 msgs Oct 5, 15:29
opened Sep 29, 04:04 ·last activity 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.

3 recent replies
Oct 5, 14:37 The reviewer expresses approval for the committer's proposed solution to fix the out-of-bounds array indexing. The reviewer states that the solution "seems to make the right trade-offs" and "add value where we can," also specifically noting approval of the use of a union in the fix. The reviewer provides a "+1" indicating their agreement with the approach.
Oct 5, 11:51 A committer (reviewer 1) provides an alternative and simpler solution to fix the out-of-bounds array indexing issue. This solution, offered after reviewing the proposer's more complex suggestion, is deemed sufficient and requires not exceeding `-fstrict-flex-arrays=1`, a constraint that is already met for other reasons.
Sep 29, 05:50 The proposer reported an out-of-bounds array indexing issue in JsonValueList when using -fsanitize=bounds and provided a fix by restructuring JsonValueList into two separate structs. Commenter 1 confirmed having encountered the same issue when compiling with -fstrict-flex-arrays=1, indicating a known problem that the proposed solution will now be evaluated against.
archive ↗
49 Fix apply worker crash when subscriber table has only a deferrable primary key Patch Review 22 msgs Oct 5, 09:29
opened Sep 28, 12:37 ·last activity 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.

3 recent replies
Oct 5, 08:57 The latest reply, from the proposer, agrees with previous suggestions to use the already computed `localindexoid` for the updatable check, ensuring a single source of truth. They confirm that this refined approach works and attach an updated patch (v5) which incorporates this change, along with updated comments and a new commit message. Separate patches are provided for different PostgreSQL versions (PG19/18 and PG17) due to compatibility requirements.
Oct 5, 07:38 A commenter confirmed that the rebased `v4` patch resolves a remaining issue in the thread. The commenter also suggested an additional top-up patch to further enhance the code by directly using the `localindexoid` for the updatable check. This proposal aims to simplify the code and ensure a single, consistent source of truth for index information, avoiding redundant lookups.
Oct 5, 05:53 Commenter 1 suggested an alternative approach for the rebased V4 patch. This idea involves moving `logicalrep_rel_mark_updatable()` after `FindLogicalRepLocalIndex()` and referring to already-found local index information, aiming to use a single source of truth and potentially simplify the code.
archive ↗
50 Per-thread leak in ECPG's memory.c Committed 1 msgs Oct 5, 19:30
opened Oct 5, 19:02 ·last activity 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.

Recent reply
Oct 5, 19:02 The latest reply confirms the v2 patch is correct and effectively resolves the double-free bug, applying cleanly and passing all ECPG tests. The reviewer also clarifies the rationale behind the fix, explaining that the `auto_mem` node is library bookkeeping while ownership of the pointed-to value is transferred to the caller upon statement success, differentiating this path from `ECPGfree_auto_mem()`.
archive ↗
No threads match — try clearing filters.