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.

Friday, August 21, 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 split tablecmds.c Rejected 14 msgs Aug 21, 22:29
opened Dec 1, 17:25 ·last activity Aug 21, 22:29

The proposer initiated a discussion to split the tablecmds.c file, which is over 22,000 lines long and too large for GitHub's code search. The file originated in the 80s as command.c and grew significantly, especially due to recent additions like partitioning features. Several participants agreed with the idea, noting it's the largest source file and that partitioning code likely contributed a large portion of its growth. Initial suggestions for splitting included moving catalog-related changes to catalog/, tablespace functions to commands/tablespace.c, and notably, partitioning and inheritance code to a new file. The proposer then created a preliminary patch to move partitioning-related code to a new file, tablecmds_partition.c, and created tablecmds_internal.h for shared internal structures. Commenters suggested naming conventions like tablecmds_partition.c and introducing tablecmds_internal.h for clearer API boundaries, which the proposer incorporated in subsequent patch versions. However, the patch faced rebasing issues due to tablecmds.c receiving other modifications in the interim. The proposer attempted to fix headerscheck but eventually indicated uncertainty about when they would be able to return to the patch, effectively stalling its progress.

2 recent replies
Aug 21, 21:51 In the latest reply, the proposer thanks a reviewer for attempting to rebase the patch and acknowledges the ongoing rebase issues due to intervening changes in tablecmds.c. Critically, the proposer states uncertainty about when they will be able to return to work on this specific patch, indicating that the proposal is currently stalled without a clear timeline for completion and is therefore considered rejected at this time.
Aug 21, 21:19 The latest reply from commenter 8 indicates that the current patch (v3) requires a rebase. The commenter attempted to rebase it but encountered multiple failed hunks, suggesting that the proposer should handle the rebase due to the complexity of recent modifications to `tablecmds.c`. This highlights an integration challenge, currently blocking further progress on the patch.
archive ↗
02 Bypassing cursors in postgres_fdw to enable parallel plans Patch Review 48 msgs Aug 21, 10:29
opened Jan 6, 08:52 ·last activity Aug 21, 10:29

This thread focuses on enhancing `postgres_fdw` to support parallel query execution on remote servers by addressing its current limitation due to cursor usage. The proposer introduced a patch that allows bypassing cursors via a new GUC, `postgres_fdw.use_cursor`, and employs chunked data fetching. A key challenge involves correctly handling simultaneous queries, for which the patch uses a Tuplestore to aggregate results. Reviewer 1 provided extensive feedback, highlighting issues with `active_scan` pointer management, consistency in EXPLAIN output, and the design of `drain_other_active_scan` and `save_to_tuplestore` functions. The discussion progressed through multiple patch revisions, with the proposer iteratively addressing the raised concerns. Architectural considerations regarding connection state management and potential similarities between existing and new data handling mechanisms were also discussed.

Recent reply
Aug 21, 10:25 The latest reply from the proposer acknowledges the observed similarities between `process_pending_request()` and `save_to_tuplestore()`. They state that `pgfdw_exec_query()` has been refactored to be self-draining, actively handling pending streaming scans before executing new queries, and now requires a non-NULL state parameter. The proposer also mentioned simplifying the `save_to_tuplestore` function signature as previously requested.
archive ↗
03 [PATCH] Add pg_current_vxact_id() function to expose virtual transaction IDs Discussing 17 msgs Aug 21, 16:29
opened Dec 8, 12:09 ·last activity Aug 21, 16:29

This thread proposed adding a new SQL function, `pg_current_vxact_id()`, to directly expose a backend's virtual transaction ID (VXID). The proposer argued that VXIDs are crucial for tracking all transactions, including read-only ones, and for log correlation, but currently require inefficient `pg_locks` queries or log parsing. The patch aimed to provide an O(1) direct memory read, improving performance and API consistency. Initial reviews questioned the compelling use cases, suggesting `pg_locks` or PID for correlation. The proposer responded by emphasizing performance gains, semantic clarity (avoiding the `pg_locks` subsystem for a transaction ID), and alignment with PostgreSQL's pattern of providing direct accessor functions. The discussion also involved suggestions for OID range, format string macro `VXID_FMT`, and consistent documentation terminology (`localXID`). A reviewer noted the performance argument was theoretical and requested empirical evidence, leading the proposer to provide `pgbench` results showing significant speed improvements.

Recent reply
Aug 21, 15:31 The proposer presented `pgbench` performance results to empirically demonstrate the benefit of the proposed `pg_current_vxact_id()` function. The tests showed that the new function provides a substantial speedup, ranging from 2x with one client to over 6x with 32 clients, compared to the existing method of querying `pg_locks` to retrieve the virtual transaction ID. This data aimed to address previous concerns about the practical impact of the function.
archive ↗
04 Possible replace of strncpy on xactdesc.c Rejected 9 msgs Aug 21, 20:28
opened Jul 3, 03:47 ·last activity Aug 21, 20:28

The proposer initiated a discussion to replace a `strncpy()` call with `strlcpy()` in `xactdesc.c`, arguing that other functions in the same file already use `strlcpy()` and it's a generally better practice. A reviewer initially supported the general idea, and another reviewer confirmed the patch worked without regressions after local testing. However, a third reviewer questioned whether `strlcpy()` was appropriate given that the length of the string (GID) was already known, suggesting `memcpy()` might be more direct. A core developer responded, noting that `memcpy()` would relax a type check that `strlcpy()` implicitly provides. Ultimately, the proposer discovered that a similar change had already been committed by another developer (`dd50eb9145e`) while his patch was under review. Due to this independent commitment, the proposer decided to close their commitfest entry for this patch.

Recent reply
Aug 21, 19:56 The proposer informed the thread that the change proposed in their patch, which aims to replace `strncpy()` with `strlcpy()` in the two-phase transaction code, has already been committed by another developer. As a result, the proposer will close their submission in the commitfest, indicating that their specific patch is no longer needed.
archive ↗
05 [PATCH] Add support for SAOP in the optimizer for partial index paths Proposed 14 msgs Aug 21, 16:29
opened Dec 5, 14:59 ·last activity Aug 21, 16:29

The proposer introduced a patch to enhance PostgreSQL's optimizer, enabling it to utilize ScalarArrayOpExpr (SAOP) clauses (like ANY() and IN()) with partial index paths, a feature previously limited to BitmapOr paths. Initial review from a reviewer suggested several improvements, including pre-filtering suitable indexes, optimizing loops, consolidating test cases into bitmapopts.sql, and benchmarking performance overheads. The proposer implemented some of these, incorporating a bitmap for candidate indexes and a significant refactor to improve index selection logic. Further optimizations were added to handle cases where only synthesized equality clauses were matched. A new reviewer provided detailed feedback, noting whitespace errors, suggesting patch consolidation, dead code removal, and stylistic improvements. The proposer acknowledged these comments, noted that the patch had missed its commitfest deadline, and expressed intent to rebase and address the feedback in a future version.

Recent reply
Aug 21, 15:56 The proposer apologized for the long pause and acknowledged the reviewer's comments, stating an intention to incorporate the feedback and fix the merge request when an opportunity arises. The proposer also mentioned that the patch had missed its commitfest deadline, requiring a rebase, re-push, and re-opening of the submission. Additionally, the proposer inquired about the specific method used by the reviewer for applying the patches, as they did not encounter the reported whitespace errors during their own testing.
archive ↗
06 Credits For v19 Discussing 21 msgs Aug 21, 06:29
opened Aug 6, 07:09 ·last activity Aug 21, 06:29

A participant describes a new system for automatically generating PostgreSQL release credits for v19, using previous release data and mailing list emails to identify contributors. The system aims to streamline the process but has gaps for first-time contributors or initial typos. To address this, the participant published three lists: unverified names, emails without associated names, and preferred names with variations, requesting community review and corrections. Multiple community members responded, providing feedback on the spelling and order of various names, particularly Chinese, Indian, and other non-Western names. There was also discussion about how to handle names that appear multiple times or have different spellings (e.g., with or without umlauts), and a specific question arose about whether AI tools should be included in the credits if they contribute. The proposer continued to refine the lists based on feedback, including a patch to fix canonical sources, and asked for suggestions on how to distribute larger commit log files for crowdsourced review.

Recent reply
Aug 21, 05:52 The latest reply from a commenter provides a correction for their name in the `confident_names` file, stating their preferred full name and requesting it be merged and kept as "Mario González gonzalemario@gmail.com" to resolve multiple entries. They also thank the proposer for their work on the credit system.
archive ↗
07 hashjoins vs. Bloom filters (yet again) Patch Review 69 msgs Aug 21, 22:29
opened May 30, 00:55 ·last activity Aug 21, 22:29

The proposer initiated a discussion on reintroducing Bloom filters to hash joins, aiming for performance improvements in selective joins and situations with large, spilling hash tables. Earlier attempts faced challenges with cost estimation and filter sizing. Commenter 1 highlighted an issue with the current selectivity estimation, which uses ordinary join cardinality, potentially leading to pessimistic outcomes with join fanout, and suggested semi-join selectivity. The proposer agreed on the need for exact build_relids matches for correct estimates but acknowledged a potential trade-off in snowflake schemas. Commenter 2 provided a detailed survey of Bloom filter implementations in other analytical databases, noting common filter types (Bloom, IN-list, min/max range) but differences in planning approaches. Reviewer 1 offered a comprehensive review of a patch version, identifying issues like probe cost charging and dead code paths, while confirming correctness in testing. The proposer incorporated several fixes into a new patch version. There was a brief side discussion, where Commenter 2 inquired about combining indexes, which the proposer clarified as largely orthogonal to the thread's main topic of dynamic filter pushdown. The ongoing discussion revolves around fine-tuning the selectivity estimates and the impact of strict build_relids matching on various query patterns.

3 recent replies
Aug 21, 22:26 The proposer's latest reply continues a discussion with Reviewer 1 about the strict bms_equal requirement for build_relids and its implications for selectivity estimation and filter application in snowflake schemas. The proposer questions whether the observed behavior, where a filter on a smaller build side might be dropped in favor of a larger, equally selective one, is desirable or an unintended consequence, especially regarding WHERE clause placement. The message is truncated, indicating ongoing discussion about this specific planning challenge.
Aug 21, 18:52 Commenter 3 agrees with the proposer's reasoning regarding an assertion in `find_bloom_filter_recipient()`. The discussion then continues on the trade-off of requiring an exact match for `build_relids` in snowflake schemas, where it might lead to losing fact-table filtering for specific candidates. Commenter 3 seeks clarification on the exact snowflake query examples to better understand the implications.
Aug 21, 15:20 The latest reply from the proposer addresses commenter 1's feedback on the v9 patch. The proposer agrees that a `NULL` recipient for a Bloom filter should trigger an assertion, indicating a bug if it occurs. The proposer also confirms the need for an exact match between `f->build_relids` and `other_relids` for correct selectivity estimation, acknowledging a trade-off in snowflake schemas. The proposer further clarifies a correction to `try_partial_hashjoin_path` regarding parallel hash joins.
archive ↗
08 heapam_relation_toast_am() returns the wrong AM for a wrapped heap AM Rejected 4 msgs Aug 21, 19:28
opened Aug 21, 13:20 ·last activity Aug 21, 19:28

The thread discusses an issue where `heapam_relation_toast_am()` returned the OID of a wrapping table access method (AM) instead of the literal heap AM OID, causing problems for TOAST table creation and indexing when an AM reuses heap's `TableAmRoutine`. The proposer suggested a fix by explicitly returning `HEAP_TABLE_AM_OID`. However, two commenters argued against this, stating that the function is heap-specific and any wrapping AM should explicitly override the `relation_toast_am` callback if it wants heap-like TOAST behavior. They also raised concerns about testing and potential breakage of existing code. The proposer ultimately withdrew the proposal, acknowledging the validity of the objections.

3 recent replies
Aug 21, 19:11 The proposer acknowledges the feedback from earlier commenters, stating they were convinced that their initial approach was mistaken. The proposer accepts that a table AM that overrides parts of the heap routine should also explicitly handle the `relation_toast_am` callback. The proposal is effectively withdrawn.
Aug 21, 18:21 The latest reply from a commenter expresses strong reservations about the proposed fix. The commenter is concerned that the change would prevent testing of TOAST paths for non-default Access Methods that reuse the heap handler, as it would force TOAST tables to be plain heap AMs, thus negating testing for such AMs. The commenter also questions whether backpatching this change is appropriate, as it could potentially break currently working code.
Aug 21, 13:20 The latest message, from the proposer, describes a bug where `heapam_relation_toast_am()` returns an incorrect access method OID for custom AMs that wrap heap, causing TOAST table creation to fail. The proposer suggests a fix by ensuring the function always returns the literal `HEAP_TABLE_AM_OID`.
archive ↗
09 toast table corrupted by vacuum - missing chunk number 0 for toast value Discussing 8 msgs Aug 21, 17:29
opened Jul 12, 18:48 ·last activity Aug 21, 17:29

The original poster reported a rare corruption issue in PostgreSQL 14.20 where TOAST tables became corrupted, manifesting as 'missing chunk number 0 for toast value', immediately following a `VACUUM` operation. The corruption involved a TOAST page where pruning occurred, leaving only dead tuple pointers. Despite the corruption, the associated main table rows were not updated, and their `xmin`, `xmax` values matched those of the TOAST rows. The issue is highly infrequent and could not be reproduced on restored instances. Commenters suggested checking for fixes in later minor versions or investigating if an old row version was erroneously made visible after its TOAST data had been legitimately removed. However, further investigation is severely limited as the necessary historical WAL segments and backups are no longer available, making it challenging to pinpoint the root cause.

3 recent replies
Aug 21, 17:13 A commenter raises questions about the TOAST row's XID and flags, specifically how `HEAP_XMIN_COMMITTED` could be present when `VACUUM` marked all tuples as dead. The commenter inquires about the status of the transaction associated with the XID, requesting a `pg_xact/clog` dump and `RUNNING_XACTS` output from around the time of the `VACUUM` operation to help understand the state transitions.
Aug 21, 16:03 The proposer informed the reviewer that the backup and WAL logs required for deeper investigation are unfortunately no longer available, making it impossible to perform the requested analysis like restoring the cluster to a specific LSN or re-running pg_waldump on older segments. The proposer reiterated that pg_waldump had previously shown no prior changes to the page before the problematic PRUNE operation.
Aug 21, 11:01 The latest reply from commenter 3 outlines detailed diagnostic steps for the reported TOAST corruption, advising the use of `pg_waldump` to analyze historical page modifications and to restore the cluster to a state immediately preceding the corruption. The commenter also suggests attempting to manually reproduce the issue with `VACUUM` and gathering additional information about the cluster's history and installed extensions to aid in pinpointing the root cause.
archive ↗
10 missing possibility to use alternative translated month names in to_char function Patch Review 9 msgs Aug 21, 22:29
opened Aug 11, 15:06 ·last activity Aug 21, 22:29

The proposer identified an issue where to_char(current_date, 'tmmonth') often returns translated month names in the genitive case due to glibc behavior, which is problematic when the nominative case is needed. The proposer noted that glibc's strftime function offers an alternative format specifier (%OB) for nominative month names. A patch was proposed to add a new to_char modifier, TAMMONTH, to allow users to access these alternative nominative forms. Reviewer 1 provided feedback, confirming the patch's purpose, testing, and documentation. They identified an issue where the abbreviated form TAMMON fell back to English instead of being localized and questioned a redundant if condition and potential strftime_l compatibility issues. The proposer subsequently updated the patch to implement TAMMON (using %Ob) and removed the redundant if condition. Reviewer 1 gave a '+1 for Ready for Committer' after reviewing the updated patch, suggesting minor refactoring.

3 recent replies
Aug 21, 22:17 The latest message from a developer is a brief acknowledgment, confirming they have seen the previous discussions and the patch. This follows a 'Ready for Committer' assessment from Reviewer 1 in the previous message, indicating that the patch is progressing through the review process and awaiting final commit, possibly with minor adjustments suggested by Reviewer 1.
Aug 21, 16:56 The reviewer confirms that the updated patch, which now correctly implements both full `TAMMONTH` and abbreviated `TAMMON` forms, 'Looks Good To Me' and functions as expected. The reviewer commends the new `get_localized_*_months` functions but points out an unused `suffix_len` variable and an `if` statement in `DCH_to_char` that might need refactoring. The reviewer gives a '+1 for Ready for Committer' following these minor adjustments.
Aug 18, 18:47 In the latest reply, the proposer confirmed that the abbreviated forms for alternative month names have now been implemented as requested by the reviewer. They also stated that the previously identified redundant check for conflicting `TM` and `TAM` prefixes has been removed from the patch, indicating further refinement of the proposed feature.
archive ↗
11 [PATCH] doc: clarify AS requirement when VALUES used in a FROM clause Patch Review 7 msgs Aug 21, 14:29
opened Jul 24, 09:12 ·last activity Aug 21, 14:29

The thread discusses an outdated statement in the PostgreSQL documentation regarding the AS clause requirement when VALUES is used in a FROM clause. The original documentation incorrectly stated that an AS clause is required, which has not been true since PostgreSQL 16. The proposer submitted a patch to correct this. Commenter 1 suggested alternative wording to clarify that the AS clause itself is optional, not just the column names within it. The proposer refined the patch based on this feedback, and commenter 2 further suggested using wording similar to queries.sgml to distinguish between a table alias and column aliases. The proposer accepted this, and a revised patch was submitted. The discussion then moved to whether the patch should be backported to earlier versions (v14 and v15) and how to phrase the good practice recommendation for table aliases.

3 recent replies
Aug 21, 14:26 The latest reply from commenter 2 raises questions about the backporting scope of the documentation update, specifically if the change should also apply to PostgreSQL v14 and v15. The commenter also seeks clarification on whether to explicitly mention that writing a table alias is good practice, given it's already covered in another section of the documentation.
Aug 21, 10:06 The latest reply from the proposer agrees with a reviewer's suggested rephrasing for the documentation. The revised wording clarifies that when `VALUES` is used in a `FROM` clause, both a table alias and assigning alias names to columns are optional, but it remains good practice to specify them. A revised patch incorporating this change was attached.
Aug 21, 07:15 The latest reply from the proposer expresses agreement with the second reviewer's suggested alternative wording for the documentation. The proposer confirms that the new phrasing is "much better" as it accurately reflects the optionality of the AS keyword itself, in addition to column names.
archive ↗
12 Tracking role modification timestamps in pg_authid / pg_roles Discussing 6 msgs Aug 21, 13:28
opened Aug 21, 03:07 ·last activity Aug 21, 13:28

The proposer introduced a feature to add a `rollastupdated` `timestamptz` column to `pg_authid` (and its views). The goal is to provide a low-cost mechanism for declarative role management tools to detect changes in role definitions, thus enabling more efficient reconciliation by avoiding unnecessary `ALTER ROLE` commands. This timestamp would update upon role creation, alteration, renaming, and GUC setting changes. Initial feedback included a suggestion for a shorter column name. A reviewer questioned why this specific catalog was chosen, asking for clarification on the general applicability or specific need for `pg_authid` over other catalogs, prompting a discussion about the scope of such a feature.

3 recent replies
Aug 21, 12:29 In the latest reply, the proposer clarifies the scope of the `rollastupdated` column to global objects like roles, databases, and potentially tablespaces. The proposer explains this focus by noting that existing event triggers do not cover shared objects, thus justifying the proposed addition to fill this specific gap, and offers to provide a proof-of-concept for this refined scope.
Aug 21, 11:32 The latest reply from Reviewer 1 clarifies their concern, emphasizing that they are not advocating for a general implementation across all catalog objects. Instead, Reviewer 1 wants to understand why the proposed change is specifically limited to `pg_authid`, given that the underlying rationale for tracking modification times could apply to many other object types. The worry is that an isolated change for `pg_authid` might set an undesirable precedent, leading to a fragmented and less maintainable codebase if similar features are added incrementally for other catalogs.
Aug 21, 11:05 The latest reply from commenter 2 questions the scope of the proposed feature, asking why a modification timestamp is needed only for `pg_authid` and not across other catalog tables. This suggests a desire for a more generic mechanism to track object changes, acknowledging that such a broad implementation would be a significant undertaking for the project.
archive ↗
13 Possible race condition in pg_basebackup Discussing 3 msgs Aug 21, 18:28
opened Aug 21, 10:30 ·last activity Aug 21, 18:28

The proposer identified a race condition in `pg_basebackup` when multiple instances are run concurrently with `--wal-method=stream --create-slot`. The issue occurs because `BASE_BACKUP` (which triggers a checkpoint) is sent before `StartLogStreamer()` creates the slot. This timing gap allows a subsequent checkpoint (potentially from a concurrent backup) to recycle needed WAL segments before the slot can reserve them, leading to 'WAL segment already removed' errors. Reviewer 1 confirmed the analysis, noting a prior theoretical mention of this race, and suggested a design discussion, proposing the server itself should temporarily retain WAL from the REDO location until the WAL streamer connects. Reviewer 2 connected this problem to recent commits (PG14-19) that addressed similar race conditions in replication slot invalidation by acquiring exclusive locks during WAL reservation, providing relevant context for a comprehensive solution.

2 recent replies
Aug 21, 16:35 The latest reply from reviewer 2 connects the reported `pg_basebackup` race condition to previous discussions and commits concerning replication slot invalidation. Reviewer 2 highlights several recent commits across various PostgreSQL versions (14-19) that addressed a similar race where a newly created slot could be invalidated between WAL reservation and a checkpoint. These fixes involved acquiring exclusive locks to serialize WAL reservation and checkpoint's minimum restart_lsn computation, providing important context for resolving the current issue.
Aug 21, 11:28 The latest reply from Commenter 1 confirms the proposer's analysis of the race condition, providing a detailed step-by-step breakdown of the problematic sequence. Commenter 1 highlights that this scenario was previously discussed during the `--create-slot` review. Furthermore, Commenter 1 points out a discrepancy with the current documentation and explains why a simple client-side adjustment to create the slot earlier is not a complete solution, indicating that a more comprehensive design discussion, potentially involving server-side WAL retention, is necessary.
archive ↗
14 Reduce memory overheads for storing a Memoize tuple Patch Review 7 msgs Aug 21, 16:29
opened Aug 1, 08:35 ·last activity Aug 21, 16:29

The proposer identified an optimization to reduce memory overheads for storing `MemoizeTuple`s in `nodeMemoize.c`. By using `ExecCopySlotMinimalTupleExtra()` to store the `next` pointer within extra bytes allocated alongside the `MinimalTuple`, the patch saves 16 bytes per cached tuple and reduces `palloc` calls. An example demonstrated significant memory reduction. A reviewer suggested defining a macro for `MAXALIGN(sizeof(MinimalTuple))`, which the proposer incorporated. The patch was subsequently committed. However, post-commit, buildfarm failures emerged on 32-bit builds because the test case's `Memoize` cache entries fit within the minimum `work_mem`, preventing evictions and thus not exercising the intended behavior. A reviewer proposed a fix to modify the test to use `COUNT(t1)`, which would force `Memoize` to cache the entire inner tuple and correctly expose the memory saving scenario.

3 recent replies
Aug 21, 15:30 A reviewer confirmed a buildfarm failure for the committed patch on 32-bit builds. The reviewer identified that the test's `Memoize` cache entries were fitting entirely within the minimum `work_mem`, thus failing to trigger evictions and properly test the intended memory savings. The reviewer then proposed a small patch to rectify the test by modifying `COUNT(t1)` to force `Memoize` to cache the whole inner tuple, ensuring the test correctly simulates the scenario where memory optimization is relevant.
Aug 21, 07:20 The latest reply reports that the recently committed patch has caused build failures on 32-bit platforms, specifically on the CI and buildfarm. The commenter requests the proposer to fix this issue, highlighting the impact on 32-bit meson tasks.
Aug 21, 02:55 The proposer announced that the v2 patch, aimed at reducing memory overheads for storing `Memoize` tuples, has been pushed. The patch implements an optimization using `ExecCopySlotMinimalTupleExtra()` to save 16 bytes per cached tuple by embedding the "next tuple" pointer directly within the `MinimalTuple`'s allocation, avoiding a separate `MemoizeTuple` object. A reviewer had previously confirmed the memory savings and approved the v2 patch after a minor suggestion for a macro was incorporated.
archive ↗
15 Parallel Apply Patch Review 11 msgs Aug 21, 03:29
opened Apr 30, 14:39 ·last activity Aug 21, 03:29

The thread discusses the "Parallel Apply" feature, which aims to improve logical replication performance by applying changes in parallel. The proposer initially submitted a patch set (v17). Early discussions revolved around the choice of hash table implementations, their naming conventions, and minor code suggestions. A reviewer provided detailed feedback on patch003, focusing on comments, scope/lifespan of hash table entries, and commit message clarity. The proposer addressed these comments, leading to new patch versions (v19). Another reviewer provided further comments on v19-0001 and v19-0002, concerning struct placement, macro usage, and code style, which the proposer largely addressed in a subsequent version. The proposer then re-split patches, rewrote commit messages, and introduced a change-size-based memory limit control for dependency tracking. Subsequent updates also addressed issues with partitioned tables, unique key/foreign key dependency tracking, and a DML operation bug. The latest update (V21) fixed additional dependency checks and improved safety checks for concurrent schema changes.

Recent reply
Aug 21, 03:21 The proposer provided an updated patch set (V21), detailing significant refactoring, simplification, and enhanced comments to improve readability and understanding. Key changes include re-splitting patches for better modularity, moving the overall design summary to the first patch, and replacing a change-count-based cleanup with a change-size-based memory limit for dependency tracking. Bug fixes for DML operations and improved support for partitioned tables in unique key and foreign key dependency tracking were also highlighted.
archive ↗
16 Many of psql's describe functions bloat cache / waste mem Patch Review 3 msgs Aug 21, 14:29
opened Jul 22, 11:55 ·last activity Aug 21, 14:29

The thread originated from a report by an initial reporter about excessive memory usage (~7MB) by psql's \df command, even in databases without user-defined functions. This was attributed to the query populating catcaches for every system function. The proposer submitted a patch to optimize describeFunctions() by filtering functions in pg_catalog and information_schema by p.pronamespace *before* pg_function_is_visible() is evaluated, when system functions are not requested and no pattern is supplied. The patch significantly reduced CacheMemoryContext increase and execution time. A reviewer tested the patch, confirmed memory reduction, and analyzed the query plan. However, the reviewer identified a potential issue where the PostgreSQL planner could reorder the filter conditions, causing pg_function_is_visible() to still run for every catalog function, negating the optimization. The reviewer demonstrated this by altering the function cost.

Recent reply
Aug 21, 14:24 The latest reply from a reviewer points out a potential flaw in the current patch. While the query plan initially shows the intended filtering order, the reviewer demonstrated that the PostgreSQL planner could reorder the conditions if costs change. This could lead to pg_function_is_visible() being called for all catalog functions, negating the optimization and increasing memory usage.
archive ↗
17 Further cleanup related to statistics import support in postgres_fdw Patch Review 5 msgs Aug 21, 21:28
opened Aug 19, 11:44 ·last activity Aug 21, 21:28

This thread focuses on a cleanup patch for the `postgres_fdw` extension, specifically related to statistics import support. The proposer introduced changes aimed at improving code readability and consistency, including reordering struct definitions and function arguments, renaming variables, fixing typos, and clarifying comments. A reviewer provided detailed feedback, generally approving the changes but raising minor concerns about the long-term maintainability of strict ordering and suggesting adjustments to comment wording. The proposer integrated most of the feedback, pushing the typo fixes separately and incorporating other suggestions into a revised patch. The latest review confirms the updated patch looks good, with only a minor `pgindent` formatting issue remaining. The discussion also included a recommendation for backporting the changes to PG19 to minimize version differences.

3 recent replies
Aug 21, 20:40 The latest reply from reviewer 1 states that the updated patch looks good overall. The reviewer noted a minor `pgindent` formatting issue but otherwise had no functional concerns. The suggestion for backporting the changes to PG19 was reiterated to maintain minimal version differences, indicating readiness for commit pending resolution of the minor formatting or final approval.
Aug 21, 10:04 The latest reply from the proposer confirms that the typo fixes have been pushed and backported. They also state that the proposed changes from 'stats' to 'statistics' were removed for consistency, and the comment for `attribute_is_analyzable()` was modified to be more concise, as suggested by a reviewer. An updated patch, reflecting these changes, was attached.
Aug 20, 11:55 The latest message from the proposer accepts the reviewer's suggestions to clarify the hint in the error message, refine a descriptive phrase, and update a reference. The proposer also confirms that the MERGE command example should be explicitly MERGE PARTITIONS. The proposer has implemented these changes in the attached patch (v5).
archive ↗
18 Support EXCEPT for TABLES IN SCHEMA publications Discussing 81 msgs Aug 21, 18:28
opened Jul 10, 11:33 ·last activity Aug 21, 18:28

This lengthy thread discusses implementing `EXCEPT` clauses for `FOR TABLES IN SCHEMA` publications, primarily focusing on complex interactions with inheritance and cross-schema partitions. Debates centered on the level of protection against user misconfigurations versus simplifying rules and assuming user intent. Reviewer 1 consistently argued for erroring out on conflicting publication definitions. Commenter 1 (the original proposer) proposed a simpler 'fine-grained clauses take precedence' rule but later identified issues with it. The thread also explored whether existing `FOR TABLE` rules about implicit descendant inclusion apply to `FOR TABLES IN SCHEMA`. Multiple patch versions (v25-v29) were shared, with code complexity being a major concern. The latest message from a commenter proposes that DDL operations should not be restricted by publication metadata and that 'INCLUSION wins over EXCLUSION' in conflicting publication rules, drawing parallels with `pgoutput`'s behavior.

3 recent replies
Aug 21, 17:30 The latest reply from a commenter argues against restricting DDL operations based on publication metadata due to the complexity this would introduce. The commenter proposes a rule where 'INCLUSION wins over EXCLUSION' for conflicting publication clauses, citing existing `pgoutput` behavior. This suggests an approach where an explicitly included schema might override an `EXCEPT` clause for an inherited table, though complete exclusion of an inherited tree might require future enhancements to `EXCEPT` clauses.
Aug 21, 06:53 The latest reply from a reviewer clarifies the existing documented behavior concerning how FOR TABLE (without ONLY) implicitly includes descendant tables, even if they reside in different schemas, while FOR TABLES IN SCHEMA does *not* automatically pull in cross-schema descendants. The reviewer provides concrete examples to illustrate these distinct behaviors, aiming to resolve ambiguity raised by another reviewer regarding specific publication conflict scenarios.
Aug 21, 04:54 The latest reply from a reviewer identifies an ambiguous case regarding cross-schema inheritance in `TABLES IN SCHEMA` publications with `EXCEPT` clauses. The reviewer requests consistent error messages or clarification on behavior for specific scenarios where a table might be simultaneously included and excluded by different rules, indicating ongoing review and refinement.
archive ↗
19 MERGE/SPLIT PARTITIONS issues/questions Discussing 31 msgs Aug 21, 14:29
opened Jul 23, 11:59 ·last activity Aug 21, 14:29

This extensive thread discusses multiple issues and questions surrounding the MERGE PARTITIONS and SPLIT PARTITIONS commands. The initial proposer raised concerns about logical replication breaking due to plain heap inserts for moved rows, changes in generated columns, propagation to subscribers, and silent changes to table properties like RLS, replica identity, and publication membership. Early discussions revolved around how to handle divergences between partitions and the parent table, such as RLS policies and table access methods. A set of patches was proposed to address some of these issues, particularly by rejecting operations that would silently drop RLS policies. There's an ongoing debate about whether the feature should disallow merge/split operations if child partitions have *any* differences from the parent to ensure safety and simplify implementation for the upcoming release, or if more nuanced handling of various divergences is needed. One specific patch regarding table access methods was criticized for still allowing invisible changes.

3 recent replies
Aug 21, 14:17 The latest message from proposer 1 confirms the intention to disallow MERGE/SPLIT PARTITIONS operations entirely if a child partition has any differences from its parent. This is proposed as a way to simplify and stabilize the feature for PostgreSQL 19, addressing previous concerns about silent changes to table properties. The proposer plans to submit a patch reflecting this stricter approach soon.
Aug 21, 09:36 The latest reply from reviewer 3 argues that patch 0005 is incorrect because it still allows for the silent dropping of explicit table access methods for partitions, which can lead to critical data integrity issues like the silent removal of encryption. The reviewer reiterates the strong preference for rejecting operations that cause any such divergences, rather than silently changing them, to prevent dangerous and unintended side effects for users.
Aug 20, 11:46 The latest message from the proposer confirms incorporating the reviewer's suggestions regarding error messages and hints in the documentation. Specifically, the proposer changed "MERGE" to "MERGE PARTITIONS" and improved the hint for RLS errors to guide users more comprehensively on disabling RLS and dropping policies before performing partition operations.
archive ↗
20 Fix CPU cost of right-semi and right-anti hash joins Patch Review 12 msgs Aug 21, 17:29
opened Aug 17, 03:26 ·last activity Aug 21, 17:29

The proposer identified and provided a fix for a costing bug in `final_cost_hashjoin()` affecting `JOIN_RIGHT_SEMI` and `JOIN_RIGHT_ANTI` hash joins. The `cpu_tuple_cost` was being overestimated because it was charged on `hashjointuples` derived from the outer side, rather than the inner rows actually emitted by these join types. This led to the optimizer choosing slower query plans, with an example showing a 3x performance improvement after the fix. Initial patches suggested charging `cpu_tuple_cost` on the path's own row estimate. However, a reviewer pointed out this could undercharge for tuples examined but filtered. The proposer then refined the patch to compute `hashjointuples` more accurately for `right-semi` and `right-anti` joins, mirroring how `JOIN_SEMI` and `JOIN_ANTI` already handle the outer side. The latest version of the patch (v4) specifically addresses `JOIN_RIGHT_ANTI` for joinqual costing. The discussion indicates a thorough review process to ensure costing accuracy across different qual types.

3 recent replies
Aug 21, 17:15 The reviewer confirms the latest patch's handling of `RIGHT_ANTI` for `cpu_tuple_cost` and qual costs is correct, aligning with the executor's behavior. The reviewer suggests adding a test case that includes a join filter to fully validate the `RIGHT_ANTI` logic, and points out a minor issue in a test comment. The reviewer also highlights an `if` statement in `DCH_to_char` that might require refactoring, but overall, indicates the core fix is sound.
Aug 21, 07:55 The latest reply from a reviewer points out that `qp_qual_cost` combines two different executor qual classes with distinct evaluation populations. The reviewer questions if it would make sense to split these two costs before applying the right-anti candidate-pair multiplier to ensure more accurate costing for both parts.
Aug 21, 05:53 The proposer, in the latest reply, presents v4 of the patch, specifically fixing the `JOIN_RIGHT_ANTI` case. They clarify that `hashjointuples` for this join type correctly represents the number of tuples passing hashclauses, which is the appropriate count for charging `joinquals`. The proposer explains that while other semi/anti join types also have imprecise `joinqual` costing, their short-circuiting behavior makes them too complex to fix without further statistics, thus requiring a more targeted approach for now.
archive ↗
21 Introduce XID age based replication slot invalidation Patch Review 46 msgs Aug 21, 22:29
opened Sep 18, 17:20 ·last activity Aug 21, 22:29

The proposer restarted a discussion about implementing XID-age based replication slot invalidation to address problems like WAL accumulation and xmin/catalog_xmin holding back vacuuming, which can lead to transaction ID wraparound. The existing idle_replication_slot_timeout addresses inactivity but not necessarily high XID churn, prompting the need for max_slot_xid_age. The proposed solution introduces a max_slot_xid_age GUC, invalidating slots with XID age exceeding this limit during CHECKPOINT and VACUUM operations, prioritizing database availability. Early patches addressed error handling and spinlock issues. Commenter 1 raised concerns about how synced slots on standbys would interact with this new mechanism, especially regarding their xmin values potentially holding back the primary via hot_standby_feedback. The proposer investigated this, concluding that synced slots should indeed be invalidated by max_slot_xid_age on the standby if their catalog_xmin is aged and blocking the primary, proposing a TAP test to confirm this behavior. Reviewer 1 agreed with invalidating aged synced slots but questioned a specific scenario related to long hot_standby_feedback intervals. Commenter 1 also suggested adding alerts or a new column to pg_replication_slots to warn users about impending invalidation, which the proposer acknowledged as useful but proposed to discuss separately. The latest patch (v14) incorporates support for invalidating XID-aged synced replication slots on standbys and includes a new TAP test for this case.

3 recent replies
Aug 21, 22:05 The proposer's latest message announces the release of v14 patches. This version specifically addresses the earlier discussion regarding synced replication slots by adding support for their XID-age-based invalidation on standbys. It also includes a new TAP test (case 2) to verify this behavior, ensuring that aged synced slots are invalidated without affecting corresponding primary slots. The proposer previously committed to discussing early warnings for synced slots separately.
Aug 20, 01:05 The latest message is from the proposer, acknowledging a reviewer's point about adding early warnings for nearing invalidation of synced slots. The proposer agrees on the importance of such warnings, especially for synced slots which lack active consumers, but suggests discussing this feature separately as it could apply to synced slot invalidation in general, not just the XID-age based mechanism.
Aug 19, 11:09 The latest reply from a reviewer confirms the agreement to invalidate synced slots on standby due to `xid_age`. The reviewer reiterates the importance of logging/warning mechanisms for synced slots, noting their lack of direct consumers, and suggests a separate discussion for this feature. The reviewer accepts the proposer's idea for users to derive alerting information from existing `pg_replication_slots` columns.
archive ↗
22 Allow a prosupport function to be attached to an aggregate Patch Review 10 msgs Aug 21, 15:28
opened Aug 17, 06:13 ·last activity Aug 21, 15:28

The proposer identified a limitation where extensions cannot attach a planner support function to an aggregate, despite the existence of `SupportRequestSimplifyAggref` for the planner. This is due to `CREATE AGGREGATE` and `ALTER FUNCTION` not supporting the `SUPPORT` clause for aggregates. The proposer provided a patch to add `SUPPORT` to `CREATE AGGREGATE` and `ALTER AGGREGATE`. Commenter 1 initially suggested delaying this to v20, but commenter 2 insisted it was a "must-fix" for v19 due to the extensibility principle. Commenter 2 then submitted a revised patch focusing on adding `SUPPORT` to `CREATE AGGREGATE` and ensuring `pg_dump` compatibility, acknowledging that the initial approach of allowing `ALTER FUNCTION` on aggregates was non-orthogonal. The proposer appreciated this but pointed out that it didn't address the goal of optimizing built-in aggregates like `sum()` or `avg()` without redefining their entire structure, suggesting `ALTER AGGREGATE ... SUPPORT` for this use case. Commenter 2 firmly rejected this, citing concerns about modifying built-in aggregates via extensions and the complexity of such a change at this stage of the release cycle. The discussion indicates a path forward for user-defined aggregates via `CREATE AGGREGATE` but a clear rejection of modifying built-in aggregates through `ALTER AGGREGATE` for the current release.

3 recent replies
Aug 21, 15:20 In the latest reply, commenter 2 firmly rejects the proposer's request to add an `ALTER AGGREGATE ... SUPPORT` form specifically for modifying built-in aggregates. Commenter 2 argues that modifying built-in aggregates through extensions is fraught with issues, such as conflicts between multiple extensions, and considers it too risky and complex to implement in the current late stage of the release cycle. This clarifies the scope of the immediate patch to focus only on `CREATE AGGREGATE` for user-defined functions.
Aug 20, 20:06 The latest reply by reviewer 2 provides an updated version of the patch (v3). This revision specifically addresses a detail missed in the previous patch: ensuring that the superuser check for specifying a support function is correctly handled within `AggregateCreate`, rather than relying on `ProcedureCreate`. This refinement is part of the ongoing effort to properly integrate the `SUPPORT` option into `CREATE AGGREGATE`.
Aug 20, 07:23 The proposer acknowledged the reviewer's patch, deeming it suitable for v19. They compiled it into a two-part series. The first patch focuses on documenting SupportRequestSimplifyAggref, while the second incorporates the reviewer's changes, adding comprehensive documentation and regression tests. The proposer also included a rationale for future CREATE/ALTER AGGREGATE changes and sought discussion on pg_dump/upgrade support.
archive ↗
23 problems with toast.* reloptions Committed 39 msgs Aug 21, 22:29
opened Jun 19, 20:20 ·last activity Aug 21, 22:29

The proposer identified several bugs in how VACUUM and autovacuum handle toast.* reloptions, specifically concerning the inheritance of parameters from the main table to its TOAST table, which was not happening consistently as per documentation. Issues included vacuum_rel() not looking up main relation reloptions, autovacuum only inheriting if no TOAST reloptions were set, and autovacuum not resolving some parameters like vacuum_truncate independently. The proposer outlined a plan to fix these by teaching autovacuum and vacuum_rel() to properly combine reloptions (TOAST winning if set) and for autovacuum to resolve all parameters upfront. Through multiple patch versions, the solution involved introducing helper functions like merge_toast_reloptions() and get_effective_relopts() to consolidate the logic. Reviewer 1 suggested optimizations and encapsulation, which the proposer addressed, simplifying code and improving API contracts. Reviewer 2 provided further feedback. The proposer committed initial cleanup patches (0001-0003) and later committed the remaining patches, confirming the completion of the work.

3 recent replies
Aug 21, 22:26 The proposer's latest reply confirms that initial cleanup patches (0001-0003) have been committed. They also indicate a rebased patch set for the remaining two patches, addressing Reviewer 2's comments, including adding const markers for a better API contract in get_effective_relopts(). The proposer acknowledges that no better approach has been found for one of Reviewer 2's concerns about potential overcomplication, signaling ongoing refinement as the final pieces are prepared for commit. The patch set was fully committed shortly after this message.
Aug 19, 07:16 The latest reply from a reviewer acknowledged the improved API contract for `merge_toast_reloptions()` in the latest patch set. The reviewer also accepted the reasoning for the `ar_hasrelopts` logic but expressed a minor concern about the perceived complexity of `get_effective_relopts()`, without it being a blocking objection, and requested additional tests for the autovacuum case.
Aug 17, 20:08 The proposer confirmed the commitment of patches 0001-0003, which resolve initial issues in the `toast.*` reloptions functionality. A rebased patch set for the remaining changes was provided, indicating the proposer's intent to commit these further improvements soon. This marks a significant progression in addressing the identified problems with TOAST table reloption inheritance.
archive ↗
24 [PATCH] Fix NULL dereference in subscription REFRESH on concurrent DROP Committed 8 msgs Aug 21, 18:28
opened May 24, 07:57 ·last activity Aug 21, 18:28

This thread addresses a NULL dereference crash in `ALTER SUBSCRIPTION ... REFRESH PUBLICATION` when subscribed tables or sequences are concurrently dropped. The original issue was that `get_rel_name()` or `get_namespace_name()` could return NULL for dropped relations, which `quote_literal_cstr()` would then dereference, causing a segfault. The proposed fix, iterated through several versions, evolved from simple NULL checks to using `try_table_open()` and then back to simpler NULL checks, along with refactoring and adding a TAP test case. After much discussion and refinement, including feedback on logging policy (deciding to skip silently) and test placement, the patch reached version 4. The latest message confirms the patch is considered good and will be prepared for backbranching and pushed.

3 recent replies
Aug 21, 17:47 The latest reply from reviewer 2 confirms the v4 patch is considered good after incorporating all previous feedback. Reviewer 2 states the intention to prepare patches for backbranches based on this version and to push them early next week, indicating the fix is now ready for commitment across relevant PostgreSQL versions. The discussion has converged on a solution that addresses the NULL dereference while handling concurrent object drops gracefully.
Aug 21, 06:12 The latest reply from proposer 2 acknowledges and approves the v4 patch, which was implicitly created by reviewer 2's suggested changes. Proposer 2 agrees with moving the regression test to `100_bugs.pl` and making the `ALTER SUBSCRIPTION ... REFRESH` command complete without error, even if relations are dropped concurrently. They also confirm that adding a test for back branches (PG19) is acceptable.
Aug 21, 00:26 The latest reply from reviewer 2 provides further comments on the v3 patch. The reviewer suggests moving the newly added regression test from `001_rep_changes.pl` to `100_bugs.pl`, indicating that the latter is a more suitable location for bug-related tests. The reviewer also briefly mentions that while the background session survives with the fix, the `ALTER` command itself might still require further attention.
archive ↗
25 Allow table AMs to define their own reloptions Patch Review 16 msgs Aug 21, 19:28
opened Mar 2, 08:56 ·last activity Aug 21, 19:28

The thread proposes allowing table access methods (AMs) to define their own relation options (reloptions) through a new `relation_options` routine, similar to index AMs. This would enable AMs to manage their specific options, such as `fillfactor` or `toast_tuple_target`, independently from heap's default options. An initial patch was provided by the original proposer. Later, a different proposer introduced a new, competing patch for the same feature. Reviewers identified several issues with the latest patch, including incorrect handling of `reloptions` during `ALTER TABLE`, and bugs in the accompanying `dummy_table_am` test module related to `fillfactor` and support for text columns. The latest response from the second proposer confirms these bugs and details fixes implemented in an upcoming patch version. The discussion involves addressing the technical challenges of integrating custom reloptions while maintaining compatibility and correct behavior during AM changes.

3 recent replies
Aug 21, 19:09 The second proposer confirmed two critical bugs reported by a reviewer in the `dummy_table_am` test module: `fillfactor` being accepted but ignored, and a lack of support for text columns. The proposer explained the root causes (incorrect `RelationHasStdRdOptions` check and missing callback overrides) and stated these are fixed in a new `v5` patch version, with new tests to prevent regression.
Aug 17, 09:45 A commenter notes that the `dummy_table_am` module not handling `fillfactor` is acceptable since it's just a test module. The commenter also acknowledges the issue with `dummy_table_am` not supporting text columns, which contradicts its "behaves like heap" description.
Aug 14, 21:46 The reviewer provided detailed feedback on the `dummy_table_am` included in commenter 2's patch. They demonstrated that the `fillfactor` option was accepted but effectively ignored, causing significant discrepancies in table sizing compared to standard heap tables. Furthermore, the reviewer noted that the `dummy_table_am` failed to support text columns, which contradicted the stated goal of behaving like a heap table.
archive ↗
26 heapam_relation_toast_am() returns the wrong AM for a wrapped heap AM Rejected 1 msgs Aug 21, 19:28
opened Aug 21, 18:28 ·last activity Aug 21, 19:28

This thread is a follow-up to an earlier discussion regarding `heapam_relation_toast_am()`. The proposer, in response to feedback from a previous exchange, explicitly withdraws the patch proposal. The proposer acknowledges that any table access method (AM) that wraps heap's `TableAmRoutine` must also explicitly override the `relation_toast_am` callback, rather than expecting `heapam_relation_toast_am()` to return `HEAP_TABLE_AM_OID` for it. The proposer plans to document this implementation detail to prevent future issues.

Recent reply
Aug 21, 18:28 The proposer formally withdraws the patch proposal, accepting that the previous feedback was correct. They acknowledge that a table access method (AM) that reuses parts of the heap routine must also explicitly handle the `relation_toast_am` callback for TOAST table creation. The proposer indicates an intention to document this behavior.
archive ↗
27 CREATE OR REPLACE MATERIALIZED VIEW Discussing 17 msgs Aug 21, 13:28
opened Jan 12, 21:33 ·last activity Aug 21, 13:28

The thread focuses on implementing a `CREATE OR REPLACE MATERIALIZED VIEW` command. The proposer submitted multiple patch versions (v5-v8). Early discussions involved addressing issues with the `IF NOT EXISTS` clause (which was subsequently dropped) and ensuring the 'replace' semantics align with the idempotency principle observed in other `CREATE OR REPLACE` commands, particularly concerning tablespace and options. Reviewers noted limitations, such as inability to drop columns during replacement, and potential race conditions. Later, a core developer questioned the `CREATE OR REPLACE` approach for materialized views, suggesting an `ALTER MATERIALIZED VIEW` command might be more appropriate to modify only the query while preserving existing data and other properties. This shift in discussion indicates ongoing debate about the fundamental approach and syntax.

3 recent replies
Aug 21, 13:13 The latest reply from a commenter argues for a single idempotent command to either create or update a materialized view's query, even if it requires clarifying data retention. The commenter suggests that data might not be considered a 'property' in the context of `CREATE OR REPLACE` idempotency and proposes an optional 'WITH EXISTING DATA' clause.
Aug 14, 19:01 The latest reply from commenter 1 reiterates the core principle for `CREATE OR REPLACE` commands: they should yield an object identical to one created fresh. The commenter agrees with an earlier suggestion that an `ALTER` command would be more appropriate for modifying a materialized view's query while preserving existing data, distinguishing materialized views from functions in this regard.
Aug 14, 18:27 The commenter 3 argues for retaining the `CREATE OR REPLACE` syntax, emphasizing the practical need for a single command to either create a new materialized view or update an existing one's base query without requiring explicit conditional logic from the user. They point out that `CREATE OR REPLACE` for other object types like functions and views already has inherent limitations, suggesting similar constraints for materialized views are acceptable.
archive ↗
28 ProcArrayAdd/ProcArrayRemove in Prepared Transaction Discussing 3 msgs Aug 21, 11:28
opened Aug 21, 07:28 ·last activity Aug 21, 11:28

The proposer identified a performance bottleneck in distributed PostgreSQL environments related to `ProcArrayAdd` and `ProcArrayRemove` operations during prepared transaction management. These functions acquire an exclusive `ProcArrayLock` and perform `memmove` operations, which can cause contention. The proposer suggested an optimization: caching `PGPROC` structures for prepared transactions within `ProcArray` to eliminate the need for frequent `Add`/`Remove` calls. This approach raises questions regarding the careful management of cached `PGPROC` states, particularly how `databaseId` and `roleId` are bound. The proposer clarified that these fields could potentially be rewritten for dummy `PGPROC`s. A commenter noted that this proposal relates to an existing thread on `ProcArrayAdd`/`Remove` performance. The commenter also cautioned that the proposed caching could trade off `GetSnapshotData`'s predictable linear memory access for improved `PROC` registration efficiency, recommending benchmarks to validate any such tradeoff and suggesting that any unordered section of `PGPROC`s should remain very small.

3 recent replies
Aug 21, 10:58 The latest reply from commenter 1 brings up another ongoing discussion regarding `ProcArrayAdd`/`Remove` performance and suggests that the proposed caching mechanism might introduce a performance trade-off. While it could improve `PROC` registration, it might impact `GetSnapshotData`'s efficiency due to altered memory access patterns. The commenter advises conducting benchmarks to assess this trade-off and emphasizes keeping any unordered `PGPROC` section minimal.
Aug 21, 09:06 The proposer clarified a technical aspect of their initial proposal regarding the challenges of managing `PGPROC`s. They suggested that for the proposed cached 'dummy' `PGPROC`s, simply overwriting the `databaseId` and `roleId` fields should be an acceptable solution for managing their association with specific databases and users.
Aug 21, 07:28 The proposer highlights that `ProcArrayAdd` and `ProcArrayRemove` in prepared transaction handling involve `memmove` operations under a global lock, which could be a bottleneck. They propose caching `PGPROCs` for active prepared transactions to avoid this, but seek input on managing `PGPROC` states and their database/user bindings.
archive ↗
29 postgres_fdw: Fix flaky push down FUNCTION RTE test Patch Review 3 msgs Aug 21, 14:29
opened Aug 21, 03:34 ·last activity Aug 21, 14:29

The thread discusses a flaky test in postgres_fdw related to unnest function pushdown, which exhibited different query plans on different machines (Mac vs. Amazon Linux 2). The issue arose because the costs of the nested loop's inner and outer sides were too similar, leading to inconsistent plan choices. The proposer suggested stabilizing the test by changing a range predicate WHERE ((c3 < '00010')) to an equality predicate WHERE ((c3 = '00010')). This change restricts one side to a single row, making it consistently the outer side of the loop and thus producing a stable plan across environments. A reviewer confirmed the problem and the effectiveness of the proposed patch.

3 recent replies
Aug 21, 13:44 The latest message is a brief acknowledgement from the proposer, thanking the reviewer for confirming the issue and the effectiveness of the submitted patch. This indicates that the proposed fix is accepted and is likely ready to be applied.
Aug 21, 05:19 The latest reply from a reviewer confirms that the proposed patch correctly fixes the test flakiness on their system (Amazon Linux 2 x86_64) and states that the patch "looks good to me", indicating approval and readiness for further action.
Aug 21, 03:34 The latest and only reply in this thread proposes a fix for a flaky `postgres_fdw` test. The proposer suggests changing a `WHERE` clause from a range predicate to an equality predicate to stabilize query plans by ensuring one side of a nested loop is consistently identified as the outer side due to a significantly reduced row count.
archive ↗
30 Race conditions in logical decoding Patch Review 17 msgs Aug 21, 18:28
opened Jan 19, 16:29 ·last activity Aug 21, 18:28

The proposer identified a race condition in logical decoding where a COMMIT record could be processed and a snapshot built before the commit is fully recorded in CLOG, leading to incorrect visibility checks and data corruption. An initial fix involving waiting for CLOG updates caused deadlocks in synchronous replication. Commenters debated adding `TransactionIdIsInProgress()` checks, but this also led to deadlocks. A reviewer explained the complex layering causing these deadlocks, related to WAL writing, CLOG updates, synchronous replica ACKs, and procarray removal. The reviewer proposed two patches: 0001 (waiting for CLOG consistency) and 0002 (omitting still-running transactions), with 0002 failing tests. The issue was listed for backpatching to v19. The latest message refines patch 0001, providing a more robust waiting mechanism and a process-local cache to optimize `TransactionIdDidCommit()` calls, addressing the underlying `latestCompletedXid` problem.

2 recent replies
Aug 21, 18:16 The latest reply from a reviewer provides a refined approach for patch 0001, focusing on ensuring `TransactionIdDidCommit()` returns the correct value before the snapshot is returned. The reviewer explains that the problem stems from `latestCompletedXid` not being updated quickly enough, causing `TransactionIdIsInProgress()` to falsely report transactions as in progress. To optimize performance and reduce potential I/O for CLOG page access, a process-local cache is introduced to avoid repeatedly testing already committed transactions.
Aug 19, 13:15 The latest reply from the proposer acknowledges that the issue has been added to the PostgreSQL 19 Open Items list. The proposer has assigned ownership to a reviewer, noting that the reviewer's recent messages indicate a readiness to push a fix. This signals that the issue is being actively tracked for resolution in a specific release, with a potential commitment in the near future.
archive ↗
31 Thread-safe stringToNode() / pg_strtok() Patch Review 9 msgs Aug 21, 16:29
opened Aug 14, 14:08 ·last activity Aug 21, 16:29

This thread focuses on making `stringToNode()` and `pg_strtok()` thread-safe by replacing global variables with a `ReadNodeContext` struct passed through the call stack. Initial feedback was positive. The proposer updated the patch to fix non-`DEBUG_NODE_TESTS_ENABLED` builds and to ensure `_readExtensibleNode()` and `ExtensibleNodeMethods->nodeRead()` also receive `ReadNodeContext` to support extensions using `pg_strtok()`. The latest review acknowledged the improvements but suggested further structural changes. These included removing `readBitmapset()` and similar helper functions in favor of `readNode()` to simplify the API and eliminate a forward declaration of `ReadNodeContext` from `nodes.h`. The proposer is currently considering the implications of these proposed structural changes, particularly regarding potential overhead for `Bitmapset` serialization.

3 recent replies
Aug 21, 15:36 The proposer responded to a reviewer's feedback, confirming and agreeing to remove a stale comment. Regarding the suggestion to remove helper functions like `readBitmapset()` in favor of `readNode()`, the proposer expressed ambivalence. The proposer cited potential overhead for `Bitmapset` as it's a special `special_read_write` node type and questioned the suitability of other `special_read_write` node types for `ExtensibleNode` implementations. The proposer also sought clarification on a reviewer's suggestion for a "preliminary patch" to remove a forward declaration, given that `ReadNodeContext` is introduced in the current patchset.
Aug 21, 04:41 The latest reply from a core developer provides in-depth feedback on the v3 patch, expressing general approval but raising specific concerns. These include questioning the accuracy of a comment in `parseNodeString()`, highlighting a lack of test coverage for `RegisterExtensibleNodeMethods()`, and suggesting a structural refactoring to remove several `read*Cols` functions by encouraging the use of `readNode()` instead, which implies removing a forward declaration of `ReadNodeContext` from `nodes.h`.
Aug 19, 13:19 The latest reply from a reviewer expresses strong support for the overall goal of making the node infrastructure thread-safe and moving away from static variables. The reviewer views APIs like `pg_strtok()` as problematic, even without considering thread safety, and agrees that tackling these improvements separately makes sense.
archive ↗
32 [PATCH] Fix compilation of nodeMergejoin.c with EXEC_MERGEJOINDEBUG Committed 5 msgs Aug 21, 15:28
opened Aug 16, 06:27 ·last activity Aug 21, 15:28

The thread began with a bug report and a simple patch from the proposer to fix a compilation error in `nodeMergejoin.c` when the `EXEC_MERGEJOINDEBUG` macro was defined. The error was due to a missing include for `debugtup()`. Commenter 1 observed that this debug code had been broken and unnoticed for at least ten years, raising the question of its relevance. Commenter 2 strongly agreed, suggesting that `execdebug.h` and its associated debug support might not be useful and could be removed entirely. Following this, commenter 1 provided a patch to clean up `execdebug.h` and its APIs by removing the unused debug code. Commenter 2 approved this cleanup patch, indicating consensus on removing the largely unused and broken debugging infrastructure.

3 recent replies
Aug 21, 14:34 In the latest reply, a reviewer confirms approval for the patch provided by commenter 1, which aims to clean up `execdebug.h` and its associated APIs. The reviewer states "LGTM" (Looks Good To Me), indicating that the proposed solution to remove the unused debug code is acceptable, with only minor stylistic preferences noted.
Aug 21, 06:50 The latest reply from a reviewer provides a new patch aimed at cleaning up execdebug.h and its associated APIs, rather than fixing the original compilation issue. This change stems from the discussion questioning the usefulness and long-standing broken state of the EXEC_MERGEJOINDEBUG code, leading to a proposal for its removal due to lack of use.
Aug 16, 14:26 The latest reply from commenter 2 supports the idea of removing the debug code in question. The commenter also suggests a broader consideration of removing `execdebug.h` entirely, arguing that if this style of debugging were truly useful, it would have been integrated into more executor nodes by now.
archive ↗
33 Fix XLogFileReadAnyTLI silently applying divergent WAL from wrong timeline Patch Review 8 msgs Aug 21, 16:29
opened Feb 20, 13:09 ·last activity Aug 21, 16:29

The proposer identified a critical bug in archive recovery where a standby could silently apply divergent WAL from an incorrect timeline during an HA switchover. This happens because `XLogFileReadAnyTLI` could fall back to an older timeline's WAL segment that diverged after the switch point. A patch (0001) was proposed to prevent this, ensuring WAL is only read from the *owning* timeline for the requested LSN. Initial discussions confirmed this was a known issue. Reviewers suggested prioritizing stalling over silent corruption, clarifying comments, requesting positive test assertions, and improving diagnostics. An optimization (patch 0002) for `walreceiver` restarts was also proposed but later decided to be discussed separately. The latest patch (v3) addresses the feedback for the core issue, stopping after attempting the newest eligible timeline, explaining why parent copies cannot be used, adding `DEBUG1` messages for clarity, and enhancing tests for both negative and positive outcomes.

3 recent replies
Aug 21, 15:39 The proposer presented v3 of the patch, which incorporates feedback from a reviewer. The new version ensures that recovery stops after attempting the newest eligible timeline for a segment and includes a detailed comment explaining why a parent timeline's WAL segment cannot be fully utilized even if its prefix is valid. It also adds a `DEBUG1` message for clearer diagnostics when waiting for segments. The test cases have been improved to verify both that recovery reaches the missing child segment and rejects the parent copy, and that it successfully completes recovery on the correct timeline, verifying the absence of divergent rows. The proposer noted that the `walreceiver` state change (from patch 0002) was removed from this version to be discussed in a separate thread.
Aug 20, 18:48 The proposer clarified that the walreceiver's premature termination during timeline switching still occurs in a small window, even with previous fixes. The proposed `WALRCV_SWITCHING_TIMELINE` state prevents startup from killing walreceiver while it fetches history files, distinguishing this from `WALRCV_WAITING`. The proposer also indicated plans to address feedback on the main fix for divergent WAL and potentially separate the optimization patch.
Aug 17, 05:55 A reviewer critically questions the necessity and implementation of patch 0002, which introduces a new WALRCV_SWITCHING_TIMELINE state. The reviewer suggests that existing WALRCV_WAITING state might be sufficient and that the bug patch 0002 aims to fix might already be absent in later versions, raising concerns about adding a specific state without clearer justification.
archive ↗
34 walsummarizer can get stuck when switching timelines Patch Review 30 msgs Aug 21, 16:29
opened Jul 13, 20:39 ·last activity Aug 21, 16:29

The proposer identified a scenario where the WAL summarizer could get stuck during a timeline switch on a standby using archive recovery. This occurs if the WAL file containing the switchpoint is only archived on the new timeline. A patch was provided and later committed to resolve this. However, the newly introduced test case for this feature began failing on certain buildfarm environments, indicating a timing-sensitive issue. Reviewers pinpointed that the test's failure was related to the order of CHECKPOINT and pg_switch_wal() operations, and potentially due to bgwriter's LogStandbySnapshot activity. A patch was proposed to swap the order of these SQL statements in the test, with detailed comments justifying the change. Further buildfarm failures led to a discussion about refining the LSN recording and catch-up logic within the test to make it more robust against varying timings.

3 recent replies
Aug 21, 16:00 A reviewer is discussing the failing test with another reviewer, suggesting that using `pg_current_wal_flush_lsn()` instead of `pg_current_wal_insert_lsn()` for `node1_final_lsn` might resolve the observed test failures. The reviewer noted that this change seemed to fix the issue locally, but questioned if it was the correct approach from a test-design perspective.
Aug 20, 18:43 A commenter identified that recent buildfarm failures of the newly committed test module were due to `bgwriter`'s `LogStandbySnapshot` affecting LSN boundaries. This timing-dependent issue caused assertions about WAL summaries not to be met. The commenter proposed adjusting the test logic by obtaining `node1_final_lsn` immediately after the test's pre-promotion WAL generation, *before* waiting for replay catchup, to ensure a stable reference point.
Aug 17, 11:00 The latest reply from commenter 3 reports a new buildfarm failure for the committed test case. The issue appears to be caused by specific timings interacting with the `bgwriter` process, leading to incorrect WAL summary results. The commenter provides a detailed log snippet and a minimal code change to `bgwriter.c` that reproduces the failure, suggesting further adjustments might be needed to stabilize the test's reliability.
archive ↗
35 Row pattern recognition Patch Review 83 msgs Aug 21, 06:29
opened Jun 17, 13:13 ·last activity Aug 21, 06:29

The thread discusses various aspects of the Row Pattern Recognition (RPR) feature patch (v50). Early messages (before the provided snippet) highlighted a correctness problem where window function results could change based on other window functions in the query, suggesting the RPR match should happen upfront. Later messages focused on issues related to view definitions and `pg_dump`/`pg_restore` failures due to ambiguous column references in `DEFINE` clauses after schema changes. A proposer 1 added a warning to the documentation and explored a subquery workaround for ambiguity. A commenter then proposed refactoring parts of `nodeWindowAgg.c` and `execRPR.c` for readability and error handling. Reviewer 1 expressed concern about the rapid submission of new refactoring patches, urging a pause to allow the existing queue to be absorbed and verified, while still welcoming other forms of contributions.

3 recent replies
Aug 21, 06:06 The latest reply from proposer 1 refers to a previous discussion about using a subquery workaround to resolve ambiguous column names in the `DEFINE` clause of RPR views, illustrating how it can prevent `pg_dump | pg_restore` failures. The proposer confirms the workaround's effectiveness and shows how `pg_get_viewdef` reflects the subquery-based definition.
Aug 21, 04:52 The latest reply from a core developer expresses concern about the high volume of refactoring patches being submitted by a reviewer, noting that new patches are arriving faster than existing ones can be integrated. The core developer requests a temporary pause on new refactoring patches to allow the current backlog to be processed, while welcoming other contributions like standards analysis or bug fixes.
Aug 21, 03:42 The latest reply from the proposer clarifies a previous misunderstanding regarding the movement of local variables in the `nfa_update_absorption_flags()` function within the refactoring patch submitted by a commenter. This specific question about variable placement is now withdrawn, narrowing the focus of the ongoing patch review.
archive ↗
36 pgstat: Flush some statistics within running transactions, take 2 Patch Review 18 msgs Aug 21, 16:29
opened May 17, 14:34 ·last activity Aug 21, 16:29

This thread continues an earlier discussion on flushing statistics mid-transaction. The proposer introduced a patch providing SQL and C APIs to trigger on-demand stats flushing. It allows immediate flushing of non-transactional counters (scans, fetches, blocks) and all other pending stats (function, IO, WAL), while transactional write counters are deferred. Initial reviews focused on handling multi-level dependencies, preventing double-counting, and correcting interactions with `pgstat_report_analyze()` and truncate logic. The proposer revised the patch to re-queue partially flushed entries and improve time-based counter precision. Subsequent revisions addressed `pgStatFlushInProgress` safety during errors and refined lock acquisition efficiency for relation stats. This led to a proposed architectural change to split `PgStat_TableCounts` into transactional and non-transactional components. The latest patch (v7) incorporates these structural changes, re-factors table stats, and expands custom stats examples to reflect the new transactional split.

3 recent replies
Aug 21, 15:57 The proposer posted v7, which addresses previous reviewer points and the discussion about splitting table statistics counters. This version refactors table stats into transactional and non-transactional parts, with v7-0001 performing the refactoring and v7-0002 implementing the in-transaction flush change. The `pgStatFlushInProgress` flag now uses `PG_TRY/PG_FINALLY` for robust error handling, and the relation flush path now compares only non-transactional stats before acquiring locks for in-transaction flushes.
Aug 19, 04:58 The reviewer 2 agrees with the proposer's idea of separating transactional and non-transactional parts within `PgStat_TableCounts` for relation statistics, considering it a sensible refactoring. They suggest that two `memcmp()` calls, combined with a transactional flag for flush callbacks, would be more appropriate. The reviewer also questions the necessity of nested structs, proposing a dedicated data structure for transactional data within `PgStat_RelationStatus` instead.
Aug 19, 03:50 In the latest reply, the proposer addresses a reviewer's suggestion to use a helper for comparing flushable counters. The proposer expresses a preference for maintaining a single `memcmp()` operation over field-by-field comparisons in every flush callback. To achieve this while separating transactional and non-transactional counters, the proposer suggests structuring `PgStat_TableCounts` with two nested structs, defining the boundary within the type system itself. They are still awaiting input from another core developer on this design choice.
archive ↗
37 pg_dump: assert failure sorting casts/transforms Patch Review 5 msgs Aug 21, 10:29
opened Aug 20, 10:20 ·last activity Aug 21, 10:29

This thread addresses an assertion failure in `pg_dump` that occurs when sorting casts or transforms whose types share identical unqualified names across different schemas. The issue stems from casts and transforms lacking their own namespace for sorting. The proposer submitted a patch that adds tie-breakers by comparing the full natural keys, including schema names, for the referenced types. Reviewers provided feedback on the test coverage, suggesting additional symmetric test cases for source-type ambiguity and recommending clearer naming conventions within the tests. Minor refinements to code comments were also suggested. The proposer has provided multiple patch revisions to incorporate this feedback, aiming for a robust and clear solution.

3 recent replies
Aug 21, 09:59 The latest reply from the proposer provides a version 3 of the patch. This revision specifically addresses all the minor nit-picks raised by a reviewer in the previous message, aiming to improve the test completeness and code comments as requested.
Aug 21, 07:40 The latest reply from a reviewer confirms that the v2 patch effectively addresses the previous test coverage suggestion. It also offers minor suggestions for test completeness and readability, while affirming that the core code change is reasonable.
Aug 20, 11:40 The latest message from the proposer introduces a v2 version of the patch. This update specifically addresses the reviewer's suggestion to enhance test coverage by adding a symmetric test case for casts. The new test case now covers situations where two source types share the same type name across schemas and cast to the same target type, explicitly validating both castsource and casttarget tie-breakers.
archive ↗
38 [PATCH v4] Add pg_current_vxact_id() function Patch Review 1 msgs Aug 21, 16:29
opened Aug 21, 15:42 ·last activity Aug 21, 16:29

This thread proposes adding `pg_current_vxact_id()`, a new SQL function to expose the current backend's virtual transaction ID (VXID). VXIDs are consistently assigned to all backends, including read-only transactions, making them ideal for transaction tracking and log correlation. The function returns the VXID as "procNumber/localXID" text, aligning with PostgreSQL's internal formats. The proposer argues that a direct function is superior to the existing `pg_locks` workaround, which is inefficient (O(n) and involves lock acquisition). Performance tests using `pgbench` demonstrate significant speedups (2x to 7x) with the new O(1) function, especially under concurrency and high lock contention. The v4 patch, rebased to master, incorporates previous review feedback regarding a `VXID_FMT` macro and consistent documentation terminology ("localXID"), aiming for an efficient, semantically clear, and consistent API.

Recent reply
Aug 21, 15:42 The proposer presented v4 of the patch, which introduces the `pg_current_vxact_id()` function. The update includes detailed performance test results using `pgbench`, demonstrating that the new O(1) function is significantly faster (2x to 7x speedup, depending on concurrency and system load) compared to the existing `pg_locks` workaround. The patch has been rebased onto current master and addresses previous review comments, aiming to provide an efficient and semantically clear API for virtual transaction IDs.
archive ↗
39 Proposal: Conflict log history table for Logical Replication Patch Review 88 msgs Aug 21, 10:29
opened Jun 25, 13:00 ·last activity Aug 21, 10:29

This extensive thread focuses on the development and review of a proposal for a conflict log history table in Logical Replication. Initial discussions covered aspects like data type choices for tuple representation and documentation phrasing. As the patch evolved through many revisions, significant issues were discovered. These include race conditions leading to incorrect remote transaction IDs and commit timestamps in streaming mode, and a critical bug where large row conflicts cause string buffer overflows, thereby halting replication. Reviewers also proposed refactoring global flags for remote transaction information into a more consolidated context structure. The proposer has been actively integrating feedback and providing updated patch sets, while also clarifying design decisions regarding distinct transaction ID fields.

3 recent replies
Aug 21, 09:45 The latest reply from proposer 2 discusses the distinction between `stream_xid` and `ApplyRemoteCtx.remote_xid`, arguing against merging them due to their different scopes of validity. They also describe recent changes to the `0001` refactor patch, which include streamlining variable usage in `apply_error_callback` and refining various comments for clarity. They attached the updated patch for further review.
Aug 21, 06:43 The latest reply reports a critical bug where logical replication fails for large rows when conflicts are logged to a table (conflict_log_destination = 'table'). The issue arises because converting a large row's data into a string for logging exceeds PostgreSQL's maximum StringInfo buffer size, causing an error and halting replication. A detailed reproducer is provided, highlighting that this specific scenario works successfully when conflicts are logged to the server log instead.
Aug 20, 09:04 The committer announced that two patches from the series have been pushed to address identified bugs. The final patch, which incorporates per-subtransaction batch tracking and an invariant assertion, is scheduled for commitment the following day, signaling the near-completion of the resolution for the reported issues.
archive ↗
40 Apply extended statistics to join clause during parameterized path costing Discussing 1 msgs Aug 21, 15:28
opened Aug 21, 14:35 ·last activity Aug 21, 15:28

The proposer has introduced a new patch aimed at leveraging extended statistics, specifically `dependencies` statistics, to improve row cardinality estimates for join clauses during parameterized path costing. The proposed mechanism allows a clause to be considered compatible if it involves exactly two `varnos`, one being the relation for which the estimate is being made and the other from a single external relation, treating the latter as a pseudoconstant. The initial `EXPLAIN` output provided by the proposer shows a significant improvement in accuracy for the scan's row estimate when the extended statistics are applied. However, a reviewer immediately pointed out an inconsistency in the current implementation, where the improved scan estimate (e.g., `rows=200`) doesn't propagate correctly to the parent Nested Loop join's row estimate (e.g., `rows=98` instead of `50*200`). The reviewer also questioned why the solution only considers `dependencies` statistics and not `ndistinct` or MCV lists.

Recent reply
Aug 21, 14:35 In the latest reply, a reviewer acknowledges the merit of using extended statistics for join estimation but points out a key inconsistency in the current patch. The reviewer observes that while the scan's row estimate improves, the parent join node's cardinality is not correctly updated, leading to a mismatch. The reviewer also questions why `ndistinct` and MCV list statistics are not leveraged in this approach.
archive ↗
41 Orphaned Files in PostgreSQL Discussing 3 msgs Aug 21, 10:29
opened Aug 20, 09:10 ·last activity Aug 21, 10:29

This thread proposes a solution to the problem of orphaned relation files in PostgreSQL that can result from unclean shutdowns during transactions that create new relations. These orphaned files consume disk space and complicate backups. The proposer suggests implementing a durable 'relation-creation marker' stored in a new `pg_relcreate` directory. This marker would track each transactionally created permanent relation. A reviewer raised a critical concern regarding the proposed ordering of logging the `XLOG_SMGR_CREATE` record before the physical file creation, highlighting potential negative interactions with concurrent checkpoints. The proposer has acknowledged this concern and committed to exploring and incorporating a revised approach.

3 recent replies
Aug 21, 09:51 The latest reply from the proposer acknowledges the critical concern raised by a reviewer regarding the ordering of `XLOG_SMGR_CREATE` and `smgrcreate()`. They agree that logging a `PRECREATE` record (for marker intent) separately from the physical file creation record is a valid approach to maintain proper WAL ordering and will incorporate this into the next version of the patch.
Aug 21, 06:55 The latest reply from a reviewer acknowledges the proposer's work on orphaned files. The reviewer raises a concern about the proposed solution's interaction with concurrent checkpoints, specifically questioning the safety of logging XLOG_SMGR_CREATE *before* the physical creation of the relation. The reviewer suggests investigating the reuse of a PRECREATE record concept from a previous, related discussion.
Aug 20, 09:10 The proposer outlined a solution for orphaned relation files by introducing durable relation-creation markers in a new `pg_relcreate` directory. These markers would contain essential relation details and transaction ID, ensuring that incomplete relation creations can be identified and properly cleaned up even after an unclean server shutdown.
archive ↗
42 pg_upgrade --copy-file-range fails with EINVAL on Linux 4.19 Discussing 2 msgs Aug 21, 11:28
opened Aug 21, 06:19 ·last activity Aug 21, 11:28

The proposer reported a failure in `pg_upgrade --copy-file-range` when run on Linux kernel 4.19, resulting in an `EINVAL` error. The issue stems from a specific behavior of the `copy_file_range` syscall on this kernel version: it returns an invalid argument error when `NULL` is passed for `off_in` and `off_out` and the `len` argument (`SSIZE_MAX`) causes an internal overflow. While `pg_upgrade`'s initial probe for `copy_file_range` succeeds, the subsequent copy loop repeatedly calls the syscall with the problematic `NULL` arguments, leading to the failure and aborting the upgrade. The proposer suggested tightening the probe or fixing the copy loop. A commenter responded by pointing out that Linux 4.19 is an End-of-Life kernel and proposed a pragmatic solution: to conditionally undefine `HAVE_COPY_FILE_RANGE` in the build configuration for kernels older than version 5.3, thereby preventing `pg_upgrade` from attempting to use the problematic syscall on these older, unsupported systems.

2 recent replies
Aug 21, 11:00 The latest reply from commenter 1 offers a solution to the `pg_upgrade` failure on Linux 4.19. Given that kernel version is End-of-Life, the commenter suggests modifying the build system (e.g., `configure.ac`, `meson.build`) to `undef HAVE_COPY_FILE_RANGE` when compiling on kernels older than 5.3. This would disable the use of the problematic `copy_file_range` syscall on unsupported older systems, avoiding the reported `EINVAL` error.
Aug 21, 06:19 The proposer details a bug in `pg_upgrade --copy-file-range` on Linux 4.19, where it fails with an "Invalid argument" error. The `copy_file_range()` system call, when used with `NULL` offsets, implicitly uses the file descriptor's current position, leading to an overflow when `SSIZE_MAX` is added, causing an EINVAL error. The proposer seeks input on whether to tighten the initial probe or fix the main copy loop.
archive ↗
43 Proposal: Supporting URI SAN in Certificate Authentication Proposed 3 msgs Aug 21, 09:28
opened Mar 27, 13:20 ·last activity Aug 21, 09:28

The proposer introduced a feature request to add support for URI Subject Alternative Names (URI SAN) in PostgreSQL certificate authentication. Currently, PostgreSQL only uses the certificate's Subject (Common Name or Distinguished Name) for identity, which limits interoperability with modern URI-based workload identity systems like SPIFFE/SPIRE. The proposal suggests enabling URI SAN entries to serve as the client identity, possibly through a new `clientname=uri` option in `pg_hba.conf`. Commenter 1 confirmed community interest, citing a similar request from the pgBackRest project. Commenter 2 expressed strong support, emphasizing the growing adoption of SPIFFE/SPIRE in cloud-native environments and the benefits of native PostgreSQL support for zero-trust identity integration.

2 recent replies
Aug 21, 08:44 Commenter 2 strongly supported the proposal for URI SAN integration into PostgreSQL certificate authentication. They highlighted the feature's importance for modern workload identity management, especially with the increasing adoption of SPIFFE/SPIRE in cloud-native projects. Native support at the PostgreSQL level would significantly enhance zero-trust identity integration.
Aug 21, 01:35 The latest reply from a commenter confirms user interest in the proposed URI SAN feature, noting that a user of the pgBackRest project had also requested it but eventually fell back to using Common Names due to the lack of current PostgreSQL support.
archive ↗
44 [PATCH] Several refactorings for pg_dump Patch Review 6 msgs Aug 21, 08:28
opened Aug 18, 15:10 ·last activity Aug 21, 08:28

The proposer submitted a patch for `pg_dump` to perform several refactorings. Initially, this involved removing a write-only field `ArchiveHandle.lookaheadSize` and replacing the magic constant `512` with `TAR_BLOCK_SIZE`. A reviewer suggested that using `TAR_BLOCK_SIZE` might not be appropriate for a generic lookahead buffer, as TAR is only one of the archive options. The reviewer recommended instead initializing `lookaheadSize` and using that value where appropriate. The proposer then provided a v2 patch incorporating this feedback, which received a quick approval from another reviewer.

3 recent replies
Aug 21, 06:28 The latest reply from reviewer 1 acknowledges the v2 patch's improvements. The reviewer asks for clarification regarding whether any consumers of the lookahead buffer currently hardcode its size, suggesting they should instead use the `lookaheadSize` field, and indicates a more thorough review will follow.
Aug 20, 23:29 The latest reply is from reviewer 1, who provides a 'Looks Good To Me' (LGTM) for the v2 patch. This indicates that the proposed refactorings, including the adjustment to how the lookahead buffer size is handled based on earlier feedback, are now considered acceptable. The patch is deemed ready for further integration steps.
Aug 20, 12:53 The proposer provided a v2 patch in response to reviewer 2's feedback. Reviewer 2 had suggested against using `TAR_BLOCK_SIZE` for a general-purpose lookahead buffer and instead recommended initializing the `lookaheadSize` field and using that value. The new patch addresses this specific design point.
archive ↗
45 index prefetching Patch Review 80 msgs Aug 19, 21:28
opened Feb 18, 04:21 ·last activity Aug 19, 21:28

This extensive thread discusses and develops an index prefetching feature for PostgreSQL, primarily by introducing a new `amgetbatch` interface to replace `amgettuple`. The goal is to improve performance for index scans, particularly on higher-latency storage, by proactively fetching data blocks. Early discussions involved heuristics for when to start prefetching and how to measure "unconsumed I/O." The proposer has submitted numerous patch versions, evolving the design to simplify slot handling, integrate GiST and SP-GiST index types, and refine prefetching heuristics based on empirical testing. Recent work focuses on avoiding small regressions in workloads like pgbench SELECT, micro-optimizations for point lookups, and improving progress reporting for `EXPLAIN (ANALYZE, IO)`. The first patch, which revises the table AM to use a higher-level, slot-based interface, is intended for imminent commit, with subsequent patches addressing SP-GiST support and improved planning heuristics for bloated indexes.

Recent reply
Aug 19, 20:00 The proposer submitted v33, another revision primarily focused on fixing "bit rot" (ensuring the patch set applies cleanly against the master branch) without introducing substantive changes. This indicates ongoing maintenance and preparation for eventual commit of the feature.
archive ↗
46 Fix small psql slash option leaks Committed 12 msgs Aug 21, 05:28
opened Aug 12, 03:12 ·last activity Aug 21, 05:28

This thread began with the proposer identifying and patching several minor memory leaks in `psql`'s meta-commands, such as `\getresults` and `\gset` in pipeline mode, where `malloc`'d strings were not being freed. While not severe, these leaks could accumulate in long-running sessions. A reviewer then discovered a more critical issue: `psql_scan_slash_option()` and `psql_scan_slash_command()` could return a static `oom_buffer` on out-of-memory, potentially leading to invalid `free()` calls. The proposer provided additional patches to address this by having these functions return `NULL` and log an error on OOM. All these patches were reviewed, approved, and subsequently committed. A follow-up patch was also committed to fix an issue where an invalid `\getresults` argument could leave `psql` in an incorrect state.

3 recent replies
Aug 21, 04:45 The latest reply from a developer acknowledges a previous review of a patch (presumably the one fixing the `\getresults` state issue) and confirms that the patch has been pushed to the codebase. The replier also thanks the original proposer for the fix.
Aug 21, 03:38 The latest reply from the proposer announces that the patch addressing the stale `pset.send_mode` state caused by invalid ` getresults` commands has been pushed. This marks the successful resolution and commitment of the second identified issue in the `psql` client.
Aug 19, 06:50 The latest reply from a commenter reviewed the newly provided patch that addresses a bug in `\getresults` (where an invalid argument could leave `psql` in an inconsistent state, affecting subsequent commands) and found it to be good. This indicates that this specific follow-up fix is currently under review.
archive ↗
47 use of SPI by postgresImportForeignStatistics Discussing 57 msgs Aug 21, 07:29
opened Jun 15, 17:50 ·last activity Aug 21, 07:29

This long-running thread began with concerns about the postgresImportForeignStatistics function's use of SPI, citing potential issues with search_path safety, incorrect relation identification during concurrent renames, and overall design complexity. The initial message questioned the necessity of using SQL statements via FunctionCallInfo instead of a direct C interface. A later part of the thread, included here, focuses on two main aspects. First, a patch to rename the restore_stats option to import_stats was proposed and subsequently committed for v19. Second, for v20, a more significant refactoring was discussed, aiming to eliminate FCINFO construction and standardize parameter passing for statistics functions. The proposer revised the strategy, but a reviewer expressed strong reservations about the complexity, bug-proneness, and the patch's current state, suggesting a significant rework and a new discussion thread.

3 recent replies
Aug 21, 07:15 The latest reply from a reviewer criticizes the proposed v3 patch for significant refactoring intended for v20, particularly the expansion of functions with numerous arguments and the use of thin wrappers. The reviewer argues this approach is bug-prone and not a clear improvement, specifically rejecting the extensive argument list. They advise the proposer to rebase and refactor the patch, and to create a new thread for the proposed changes, as the current state is confusing and not clean.
Aug 20, 16:28 The commenter 2, in the final message of this thread, announced the creation of a new CommitFest entry for the ongoing architectural improvement work related to postgresImportForeignStatistics. This signals the formalization and tracking of the proposed changes for version 20, effectively moving the detailed patch review and further discussion of the core issues to a new, dedicated thread.
Aug 20, 11:43 The latest message from reviewer 1 approves the revised proposal for the v20 refactoring, which includes removing special handling for the version parameter and then refactoring how FCINFO is constructed by passing the latter portion of the NullableDatum array. The reviewer emphasizes fixing these issues with minimal changes to avoid making back-patching difficult.
archive ↗
48 doc: Reformat SELECT queries using GRAPH_TABLE Proposed 1 msgs Aug 21, 07:29
opened Aug 21, 07:05 ·last activity Aug 21, 07:29

This thread proposes a documentation patch to improve the readability of SELECT queries that use GRAPH_TABLE in ddl.sgml and queries.sgml. The proposer noted that these queries are currently written on single long lines, necessitating horizontal scrolling. The patch suggests breaking these queries across multiple lines, aligning with the formatting style of CREATE PROPERTY GRAPH statements already in the documentation, and referencing examples from SQL:2023 overviews for consistent indentation and line break placement.

Recent reply
Aug 21, 07:05 The proposer submits a patch to reformat SELECT queries utilizing GRAPH_TABLE in the documentation. The change aims to break long query lines into multiple, indented lines to enhance readability, consistent with existing CREATE PROPERTY GRAPH formatting and external SQL:2023 examples.
archive ↗
49 Report bytes and transactions actually sent downtream Patch Review 22 msgs Aug 20, 12:28
opened Jun 12, 06:59 ·last activity Aug 20, 12:28

The thread discusses adding a new statistic, output_bytes, to logical replication slots to report the amount of decoded data produced for the consumer by the output plugin. Initial confusion existed regarding whether to include protocol messages like keepalives or only the plugin's direct output. Participants agreed to exclude connection-management messages and focus on the data produced by the output plugin, including transaction changes and their delimiters. The naming sent_bytes was debated, with plugin_total_bytes and output_bytes suggested, settling on output_bytes for clarity. Reviewers provided feedback on the calculation, ensuring it only counts plugin-produced data, and on refactoring the statistics update mechanism. The proposer initially suggested splitting refactoring, but then provided a combined patch for review.

2 recent replies
Aug 20, 12:25 The latest message from the author acknowledges review comments, plans to rebase the patch, and clarifies the interpretation of output_bytes to include output-writer framing, not just plugin output. The author also indicates a plan to revisit and potentially add more specific test cases.
Aug 20, 03:01 The reviewer provided feedback on the latest patches, noting an issue with the refactoring patch application. They also identified that the `output_bytes` count in the WALsender path still includes an additional 25 bytes due to an uncounted header, suggesting a fix using an offset. Furthermore, the reviewer questioned the current test coverage, recommending more robust tests to assert `output_bytes` matches the expected number of bytes from the output plugin.
archive ↗
50 Report index currently being vacuumed in pg_stat_progress_vacuum Discussing 20 msgs Aug 21, 00:28
opened May 4, 02:00 ·last activity Aug 21, 00:28

This thread proposes enhancing `pg_stat_progress_vacuum` to report the OID of the index currently being vacuumed, aiding in debugging slow vacuum operations. The initial patch included arrays in the leader's row to track worker PIDs and index OIDs. Discussion evolved into a debate on the optimal design for parallel progress reporting. Reviewer 1 argued for a 'one row per worker' model for better granularity and extensibility, rather than arrays in the leader's row. Commenter 1 initially supported arrays but later suggested a separate `pg_stat_progress_vacuum_worker` view for per-worker details. The proposer then leaned towards a single 'one row per worker' view, but commenter 1 raised strong objections, citing a breach of existing design principles and potential ambiguity in interpreting merged statistics, advocating for separate views for aggregate and worker-level data.

3 recent replies
Aug 20, 23:58 The latest reply from commenter 1 expresses strong reservations about integrating worker progress into the main progress view. The commenter argues this approach deviates from the 'one row per command' design principle and creates ambiguity regarding the interpretation of aggregate statistics like `heap_blks_*`. The commenter suggests that separate views for worker-level details and high-level command information would be clearer and more manageable for monitoring tools and users.
Aug 20, 23:10 A reviewer concurred with the approach of having a single progress view that includes both the leader and its workers. They emphasized that this is a natural approach given that both the leader and workers can participate in the work, and stressed the importance of not restricting future implementation choices.
Aug 20, 02:37 The proposer reinforced their view on per-worker progress reporting, acknowledging that some index access methods might perform intra-parallel index vacuuming, allowing OIDs to repeat in the array-based approach. They also touched upon parallel heap vacuuming, suggesting that a "one row per worker" model would naturally accommodate reporting block counts per worker, instead of a single overall value. They reiterated that a new, worker-specific view might be better for detailed per-worker information, keeping the existing view high-level.
archive ↗
No threads match — try clearing filters.