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

The pgsql-hackers
Weekly Digest

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

Tuesday, July 7, 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 Report oldest xmin source when autovacuum cannot remove tuples Discussing 44 msgs Jul 7, 18:28
opened Oct 31, 06:31 ·last activity Jul 7, 18:28

The proposer initiated a discussion to enhance VACUUM logs by including the reason for the oldest xmin, which prevents dead tuples from being removed. This aims to help diagnose and prevent table bloat by providing actionable information that is currently hard to capture retrospectively from volatile views. The initial patch proposed logging the reason and, if applicable, the backend PID. This sparked extensive discussion, including concerns about the overhead of an additional ProcArray scan, the timing of the report (after vacuum completion vs. earlier), and the accuracy of identifying the *highest-priority* blocker. Through several iterations, the patch evolved to address these concerns, adopting a priority scan to correctly identify the blocker, refining the information captured (e.g., GID for prepared transactions, application_name for standbys), and improving memory allocation. The latest versions (v7, v8, v9) also introduced significant correctness fixes, such as scoping the blocker scan to the relation's horizon kind, comparing against effective xmin for replication slots, covering all slots, and classifying backend states more accurately. A helper function was factored out to prevent logic drift between ComputeXidHorizons() and the new function. The recovery case was explicitly removed from GetXidHorizonBlockers()'s scope.

2 recent replies
Jul 7, 16:34 The proposer rebased the v8 patches to align with the latest master branch, resulting in v9, which is now attached. This was a technical update to ensure the patch applies cleanly after recent master changes. The proposer also clarified a missing reference from an earlier reply.
Jul 7, 16:16 The proposer addressed concerns regarding ProcArrayLock contention and the timing of blocker reporting. They reiterated that integrating blocker capture directly into ComputeXidHorizons() was previously found to be incorrect. The proposer also argued that the additional lock acquisition is infrequent and should have minimal performance impact, while a separate live view would be more suitable for real-time monitoring.
archive ↗
02 Fix gistkillitems & add regression test to microvacuum Committed 12 msgs Jul 7, 18:28
opened Jan 15, 07:00 ·last activity Jul 7, 18:28

The proposer identified a bug in GiST where XLOG_GIST_DELETE records, related to the microvacuum feature, were not properly covered, specifically when all dead tuples reside on the root page. This meant gistkillitems was not called for the root page, making small GiST indexes with deleted items problematic for microvacuum. The proposer provided a patch with a regression test to address this. Initial feedback pointed out an existing, failing isolation test, which the proposer then adapted to. Reviewer 1 confirmed the bug, suggested the patch was suitable for master but not necessarily for backpatching, and proposed minor comment improvements. Reviewer 2 validated the fix by reproducing the issue and confirming correct handling of the root page. Although XLOG_GIST_DELETE records were not observed in one test case, the fix for curBlkno initialization was deemed correct. The patch was ultimately committed by a committer.

Recent reply
Jul 7, 18:22 The committer confirmed the proposed solution, which involves renaming a function and adding an assertion, makes sense. The committer applied the patch to the master branch. They decided against backpatching the fix for now, considering the bug relatively harmless.
archive ↗
03 Bump soft open file limit (RLIMIT_NOFILE) to hard limit on startup Patch Review 31 msgs Jul 7, 08:29
opened Feb 11, 18:52 ·last activity Jul 7, 08:29

This thread proposes to increase PostgreSQL's soft open file limit (`RLIMIT_NOFILE`) to the hard limit during startup, addressing frequent 'too many open files' errors and aligning with modern system configurations. This change is particularly beneficial for I/O-intensive workloads and common user issues. The discussion involved several patch iterations. Initial feedback led to refinements, including renaming helper functions and relocating code. A significant update introduced custom `pg_system()` and `pg_popen()` functions designed to restore the original, lower file limits in child processes immediately after forking but before `exec`, while maintaining the higher limit for the main PostgreSQL process. This ensures compatibility with older applications using `select(2)` and prepares for future multi-threading. A part of the proposed change for `pgbench` was already committed in PostgreSQL 18.

Recent reply
Jul 7, 08:02 The proposer submitted another rebased version of the patch. This indicates ongoing refinement to ensure the proposed changes remain current with the development branch and are ready for continued review and integration. The core logic for handling file limits in child processes is maintained.
archive ↗
04 Refactoring postmaster's code to cleanup after child exit Discussing 22 msgs Jul 7, 09:29
opened Dec 8, 23:12 ·last activity Jul 7, 09:29

This thread, initially about refactoring postmaster's cleanup code, has largely shifted to addressing persistent test failures, particularly in `001_connection_limits.pl` under Valgrind and `002_connection_limits.pl` on Hurd systems. Early discussions focused on improving error reporting in TAP tests by capturing `stderr` and `stdout` for better debuggability. Patches were proposed and well-received to fix a race condition and enhance error messages in connection limit tests. Later, a specific issue on Hurd was identified, where `IO::Socket::UNIX`'s `getpeername()` call failed due to an unimplemented system call. A temporary `SKIP` patch for this test on Hurd was suggested and discussed, with the latest messages confirming its successful implementation as a workaround in Debian packages.

Recent reply
Jul 7, 08:39 The latest commenter reports that a previously discussed temporary patch to skip a problematic test on Hurd systems has been successfully integrated into the Debian package. This workaround effectively prevents failures caused by an unimplemented `getpeername()` system call, thus ensuring that the test suite runs without issues on those specific architectures.
archive ↗
05 Bug: mdunlinkfiletag unlinks mainfork seg.0 instead of indicated fork+segment Committed 8 msgs Jul 7, 18:28
opened Dec 20, 22:41 ·last activity Jul 7, 18:28

The proposer identified a bug in PostgreSQL's mdunlinkfiletag function within the MD smgr internals. This bug caused unlink requests for specific forks or segments to incorrectly target segment 0 of the main fork, rather than the requested fork and segment. This behavior, present since PG16 for forks and PG12 for segments other than 0, could be problematic during recovery or for extensions expecting precise unlinking. Initially, the proposer offered a patch to fix the incorrect unlinking. However, a reviewer suggested an alternative direction: since the mechanism is primarily used for "tombstone" files (main fork, segment 0) and is not intended for general-purpose unreliable segment unlinking by external code, it would be better to rename register_unlink_segment() to register_unlink_tombstone() and add Assert() statements to ensure it's only used for tombstone files. The proposer agreed with this revised approach, submitting a new patch. Subsequent reviewers confirmed the correctness and clarity of this new direction. The committer agreed with the renaming and assertion, and committed the changes to master.

2 recent replies
Jul 7, 17:46 The committer affirmed that renaming the function and adding an assertion to enforce its specific use for tombstone files is a sensible approach. They committed the changes to the master branch.
Jul 7, 17:08 The committer acknowledges the proposer's agreement on the revised approach. They confirm that renaming the function and adding an assertion to clarify its specific purpose for tombstone files is a sensible change. The committer then states that the patch has been committed to the master branch.
archive ↗
06 injection_points: Switch wait/wakeup to use atomics rather than latches Patch Review 18 msgs Jul 7, 10:29
opened May 28, 02:43 ·last activity Jul 7, 10:29

This thread discusses a proposal to switch the `injection_points` facility from using latches and condition variables to atomic counters for wait/wakeup mechanisms. The core committer (proposer 1) identified limitations with the current latch-based approach, especially for scenarios where processes lack assigned latches (e.g., in postmaster context or during backend termination), which hinders testing. The proposed patch uses atomic counters and a periodic polling loop with escalating delays (10us to 100ms), arguing it provides a good balance of responsiveness and portability. A reviewer initially expressed concern about reduced responsiveness compared to instant wakeups, but another committer (commenter 1) clarified that latches only work after a `PGPROC` slot is available, which is not always the case (e.g., authentication, postmaster). A third committer (commenter 2) confirmed the need for this change for corruption-seeking tests and suggested improving how wait points are identified. Further discussion explored methods for tests without `PGPROC` access, including printing LOG entries or using memory-mapped files. Proposer 1 rejected the file-mapping approach as too invasive, suggesting a simpler file-based marker system. Commenter 2 then submitted a new patch based on the simpler file-based approach, rebased on the atomics commit.

Recent reply
Jul 7, 09:51 The latest reply from commenter 2 presents a new patch that adopts a file-system-based approach for managing `injection_points` wait states, as suggested by proposer 1. This new design avoids shared memory mapping for out-of-process control and instead uses control directories and files in the data directory to arm, monitor, and wake up wait points, addressing observability concerns for tests without SQL or `PGPROC` access.
archive ↗
07 BF mamba failure Discussing 14 msgs Jul 7, 10:29
opened Aug 4, 10:00 ·last activity Jul 7, 10:29

This thread reports a recurring FATAL error "trying to drop stats entry already dropped" leading to replica shutdowns, specifically on cascading streaming replicas. The issue has been observed across multiple PostgreSQL versions (15.4, 17.3, 17.6) and persists despite upgrades. The reporter notes a heavy workload with frequent creation and dropping of tables (high OID consumption). A core committer suggests the problem might be related to transaction ID wraparound and race conditions in standbys, particularly with how `pgstats` entries are dropped during WAL redo. Initial investigations included adding "generation" to the error log for better debugging (committed to 17.6). The committer is attempting to reproduce the issue with specific test scenarios but has so far been unsuccessful. The latest update confirms the issue still occurs in 17.6, highlighting the difficulty in tracking down this elusive bug.

Recent reply
Jul 7, 10:13 The latest reply from the reporter confirms that the "FATAL: trying to drop stats entry already dropped" error continues to occur, even on PostgreSQL 17.6, which includes the additional "generation" information in the error message. The reporter asks if future PostgreSQL versions might address the issue, indicating the problem remains unsolved.
archive ↗
08 Add enable_groupagg GUC parameter to control GroupAggregate usage Patch Review 14 msgs Jul 7, 07:29
opened Jun 6, 07:59 ·last activity Jul 7, 07:29

This thread proposes adding a new GUC parameter, enable_groupagg, to control the use of GroupAggregate plans. The proposer demonstrated a significant performance improvement (35x faster in a specific parallel query scenario) by disabling GroupAggregate and forcing HashAggregate. Reviewers supported the idea, noting that existing GUCs like enable_sort are too broad for specific aggregate control. The discussion expanded the scope of enable_groupagg to cover other sort-based operations like SetOp and Unique. A committer took over the patch, incorporating these broader changes (v4) and refining the cost model for AGG_MIXED strategies. The latest discussions focused on documentation clarity, with the committer preferring to maintain conceptual consistency with existing GUCs rather than enumerating specific plan node types. The committer expressed intent to push the patch soon.

Recent reply
Jul 7, 06:53 The latest reply from the committer expresses the intention to push the patch soon. The rationale is to allow newly added test cases to directly use enable_groupagg, thus avoiding the need for continuous rebasing to convert tests that currently use enable_sort to force hashed grouping.
archive ↗
09 Bug in ALTER SUBSCRIPTION ... SERVER / ... CONNECTION with broken old server Committed 25 msgs Jul 7, 01:29
opened Apr 22, 01:51 ·last activity Jul 7, 01:29

The proposer reported a bug where `ALTER SUBSCRIPTION ... SERVER` or `... CONNECTION` fails if the *old* foreign server or its user mapping is broken, contradicting documentation that suggests ACL checks should be skipped. The root cause was `GetSubscription()` calling `ForeignServerConnectionString()` even when ACL checks were explicitly set to `false`, attempting to connect to a potentially broken server. The proposed fix involved storing the server OID in the `Subscription` object and lazily calling `ForeignServerConnectionString()` only when `sub->conninfo` is actually needed. Discussions also covered related issues, such as `DROP SUBSCRIPTION` behavior when `SLOT_NAME=NONE` is used but tablesync slots still exist. Parts of the initial patch (0001 and 0002) have been committed to address the primary bug. Further discussion on a third patch (0003) involved using subtransactions to capture errors during connection attempts for `DROP SUBSCRIPTION`, but the proposer ultimately opted against this complexity, favoring a simpler "escape hatch" for users to force `DROP SUBSCRIPTION` without remote publisher contact, acknowledging a pre-existing bug related to documentation deviation.

Recent reply
Jul 6, 23:46 The proposer reiterates the importance of providing an "escape hatch" in `DROP SUBSCRIPTION` that would completely prevent any attempt to connect to the remote publisher, aligning with existing documentation. This mechanism is crucial for handling edge cases where connection errors occur or when users prefer to drop a subscription without network side-effects. The proposer emphasizes that `walrcv_connect()` can also throw errors and that a network connection itself might be undesirable, highlighting the need for a robust way to force disconnection.
archive ↗
10 Do not lock tables in get_tables_to_repack Patch Review 3 msgs Jul 7, 17:28
opened Jun 16, 07:34 ·last activity Jul 7, 17:28

The proposer raised a concern regarding the `REPACK` process, specifically how tables are locked during the initial list building. The existing mechanism is flawed because concurrent table drops can still occur after the list is built (due to per-table transactions), and `ConditionalLockRelationOid()` can lead to unexpected `SKIP_LOCKED` behavior. The proposer suggests removing these locks and ensuring `repack_is_permitted_for_relation()` robustly handles concurrent drops. A commenter initially questioned the urgency for `REPACK` compared to `VACUUM` but another core developer supported the proposal, highlighting inconsistencies in `REPACK`'s current table list acquisition. This core developer agreed with the proposed direction and attached an alternative patch for review, aiming for a more defensive implementation.

Recent reply
Jul 7, 16:53 A core developer agrees with the proposer that the existing locking strategy during `REPACK`'s table list generation is problematic and inconsistent. The developer supports the idea of correctly handling concurrent drops without explicit locks and has attached an alternative patch for consideration, aiming for a more robust solution.
archive ↗
11 Add malloc attribute to memory allocation functions Discussing 7 msgs Jul 7, 16:29
opened Jul 1, 17:51 ·last activity Jul 7, 16:29

The proposer suggested adding the `malloc` attribute to memory allocation functions to leverage compiler features for error detection, code coverage, and optimization. While initial testing showed no regressions, a core reviewer raised significant concerns that GCC's static analyzer, lacking an understanding of PostgreSQL's memory contexts, would generate a large volume of false-positive memory leak warnings. The proposer confirmed that such warnings increased dramatically from 62 to 598. Commenters then discussed the broader benefits of the attribute beyond leak detection, particularly for frontend tools without memory contexts. The proposer proposed suppressing the leak warnings for backend code, leaving the path open for continued discussion.

3 recent replies
Jul 7, 15:54 The proposer suggested adding `-Wno-analyzer-malloc-leak` to backend code to suppress the numerous false-positive memory leak warnings while still benefiting from other checks provided by the `malloc` attribute.
Jul 6, 16:34 The proposer confirms the committer's concern regarding excessive warnings. With the patch applied, the number of `-Wanalyzer-malloc-leak` warnings significantly escalates from 62 to 598. This observation validates the committer's skepticism, indicating that the proposed `malloc` attribute is problematic in the context of PostgreSQL's custom memory management.
Jul 6, 04:26 The commenter expressed strong skepticism regarding the proposal, arguing that adding the `malloc` attribute would likely result in numerous bogus `-Wanalyzer-malloc-leak` warnings. The commenter's concern stems from the compiler's inability to understand PostgreSQL's custom memory contexts, suggesting that such annotations would only "sort-of match our semantics" and lead to more problems than benefits.
archive ↗
12 plpython: NULL pointer dereference on broken sequence objects Patch Review 6 msgs Jul 7, 10:29
opened Jun 25, 08:49 ·last activity Jul 7, 10:29

This thread discusses a NULL pointer dereference vulnerability in PL/Python, similar to a recently fixed issue in PL/Perl. The proposer identified that `PySequence_GetItem()` and related functions in PL/Python (like `PyMapping_Items()` and `PyList_GetItem()`) did not check for NULL return values, allowing a maliciously crafted or broken Python object (e.g., one that claims a length but fails to deliver items) to crash the backend. A patch was submitted to add NULL checks and raise proper errors. A reviewer confirmed the fix and suggested extending it to mapping-related functions, which the proposer promptly addressed in a v2 patch. The v2 patch was reviewed, approved, and then committed and back-patched to v14. After the commit, another commenter identified a similar remaining issue in `PLySequence_ToJsonbValue()` where `PySequence_Size()` returns -1 for unresolvable lengths, leading to silent empty array output and a poisoned Python error indicator, proposing a further patch.

Recent reply
Jul 7, 10:08 The latest reply identifies a new, related issue in `PLySequence_ToJsonbValue()`. It notes that `PySequence_Size()` can return -1, which is not properly handled, causing empty JSONB arrays to be silently generated and the Python error indicator to be left set. A patch is attached to correctly check for this negative return and surface the underlying Python exception.
archive ↗
13 Mark class_descr strings for translation Discussing 5 msgs Jul 7, 18:28
opened Jul 3, 06:37 ·last activity Jul 7, 18:28

The proposer identified an issue with message translation in pg_depend.c, where get_object_class_descr() returns a string (intended for internal messages) that is embedded in user-facing error messages. This results in unnatural translations. The proposer proposed a patch to mark these class_descr strings for translation using gettext_noop() at their definition and _() at their usage. A reviewer approved this approach, confirming that both halves of the patch (marking at array definition and translating at consumption) are necessary and correctly implemented. However, a committer raised a concern, suggesting that class_descr strings are *meant* for internal messages and their use in user-facing messages is a bug at the caller's site. The committer expressed hesitation about adding many new translation strings that only differ by object type, suggesting alternative solutions might be preferred.

3 recent replies
Jul 7, 16:50 The committer stated that class_descr strings are intended solely for internal messages. They argued that their current use in user-facing messages is a bug at the call site which should be fixed. The committer expressed reluctance to introduce numerous new translation strings that would only vary by object type.
Jul 7, 12:39 A reviewer expressed strong support for the proposed direction, confirming that the mechanism of marking static strings for translation with `gettext_noop()` and translating them at consumption with `_()` is correct. The reviewer noted that both halves of the patch are necessary and highlighted a minor issue with CRLF line endings in the submitted patch, suggesting a resend with LF endings.
Jul 3, 08:19 The latest reply, from a commenter, provides a positive review of the proposed patch. They confirm that the mechanism for marking and translating the `class_descr` strings is correct and that both halves of the patch are needed and correctly paired. The reviewer also noted a minor issue with CRLF line endings in the attached patch, suggesting a resend with LF endings for easier application.
archive ↗
14 Implement waiting for wal lsn replay: reloaded Patch Review 50 msgs Jul 7, 15:29
opened Jan 27, 01:14 ·last activity Jul 7, 15:29

This thread continues the discussion and refinement of a patchset aimed at improving the waiting for WAL LSN replay mechanism. Initially, the proposer submitted a patch to fix a misplaced wake-up call in PerformWalRecovery to prevent missed wake-ups during recovery pauses or promotions. Commenter 1 (an early reviewer) had pushed an earlier version of the patchset but then discovered new test failures related to the wait_for_catchup() implementation, which required adjustments to test scripts. The proposer provided a revised patch to simplify the fix by directly calling wait_for_catchup() and accepting a return to polling in some cases. Commenter 1 approved this, noting the clarity gained. Later, commenter 3 provided a detailed review of the current implementation, raising concerns about reading XLogRecoveryCtl->lastReplayedEndRecPtr without locks, the performance impact of three extra function calls on a hot path for every WAL record, and suggesting moving the WaitLSNWakeup() calls inside ApplyWalRecord() for better code structure. The proposer agreed to test the performance impact and supported moving the calls.

3 recent replies
Jul 7, 15:25 Commenter 1 is responding to commenter 3's detailed review of the WAL redo loop implementation. Commenter 3 raised concerns about reading shared variables without locks, potential performance overhead from multiple WaitLSNWakeup calls on a hot path, and suggested refactoring to move these calls within ApplyWalRecord(). Commenter 1 is acknowledging these points, indicating ongoing engagement with the proposed improvements.
Jul 6, 14:17 The latest reply from the proposer reaffirms their agreement with a reviewer's suggestion to move WaitLSNWakeup calls inside ApplyWalRecord for improved code safety and readability. The proposer also commits to conducting performance testing to measure the impact of these calls, which operate on a very hot path during WAL record application. This indicates active work towards refining the recently committed feature.
Jul 6, 11:04 The latest reply from a reviewer examines the newly placed WaitLSNWakeup calls within the main WAL redo loop. The reviewer questions the lack of locking/atomics when reading lastReplayedEndRecPtr, expresses concern about the performance impact of three WaitLSNWakeup calls per WAL record in a hot path, and suggests moving these calls inside ApplyWalRecord() for better consolidation.
archive ↗
15 Proposal: new file format for hba/ident/hosts configuration? Proposed 2 msgs Jul 7, 17:28
opened Jul 7, 17:00 ·last activity Jul 7, 17:28

The proposer highlights several issues with PostgreSQL's current hba/ident/hosts configuration file formats, including their PostgreSQL-specific nature, limited extensibility, and inconsistencies in order-sensitivity. The proposer suggests introducing a new, standard configuration format while retaining the existing formats for backward compatibility, with new features exclusively implemented in the new format. After evaluating INI, XML, JSON, YAML, and TOML, the proposer expresses a preference for TOML, finding JSON too verbose despite having an existing parser. The proposer notes that TOML was suggested by others and appears to offer a good balance of precise specification and human readability. A commenter agrees with the assessment of existing formats and the potential benefits of TOML or JSON5, also emphasizing the value of JSON Schema for validation.

Recent reply
Jul 7, 17:17 The commenter expresses dissatisfaction with the current configuration style and supports the proposer's dismissal of INI and XML. They agree that JSON, YAML, and TOML are good options, highlighting the utility of JSON Schema. The commenter concurs that YAML is too complex and JSON's boilerplate can hinder human readability, ultimately favoring TOML and mentioning KDL and JSON5 as other possibilities.
archive ↗
16 truncating casts of pgoff_t Committed 4 msgs Jul 7, 10:29
opened Jun 22, 07:56 ·last activity Jul 7, 10:29

The proposer identified instances where `pgoff_t` values, typically 64-bit integers, were cast to smaller types for output, potentially leading to truncation in error messages. This was deemed mostly cosmetic but could have practical implications in `basebackup_server.c`. The initial patch aimed to fix this by consistently using the `%lld` print format and casting to `long long int`, aligning with existing practices and improving robustness against negative values. Reviewers agreed with the general approach and suggested a more robust error handling (`elog(ERROR)`) instead of an `Assert` for `histfilelen` in `SendTimeLineHistory()`. They also noted a related issue of reading beyond the original file length if the file grows during reading, which is being addressed in a separate thread. The proposer committed the patch with the suggested `elog` change.

Recent reply
Jul 7, 09:59 The latest reply confirms that the patch has been committed, incorporating the suggestion from a reviewer to use `elog(ERROR)` instead of `Assert` when checking for potential `histfilelen` truncation. The proposer also notes that a related issue regarding file growth during reading is being handled in a separate discussion.
archive ↗
17 wait_event_type for WAIT FOR LSN Discussing 5 msgs Jul 7, 16:29
opened Jul 6, 01:26 ·last activity Jul 7, 16:29

The proposer suggested reclassifying several `WAIT_FOR_WAL_*` events from the `WaitEventClient` category to `WaitEventIPC` in `wait_event_names.txt`. The rationale is that these waits are not solely related to client socket I/O but can also stem from internal delays like local fsync or WAL replay, making them more akin to inter-process communication. Commenters generally agreed with this move. A discussion also ensued regarding `WAIT_FOR_STANDBY_CONFIRMATION`, with the proposer explaining the arguments for and against reclassifying it. Ultimately, the proposer decided against moving `WAIT_FOR_STANDBY_CONFIRMATION` due to the higher standard for reclassifying events that have already been released in a previous version, and this decision was accepted by other participants.

3 recent replies
Jul 7, 14:38 The commenter agreed with the proposer's analysis and the decision to reclassify the `WAIT_FOR_WAL_*` events to `WaitEventIPC`, while keeping `WAIT_FOR_STANDBY_CONFIRMATION` in its current category due to the higher standard for reclassification of existing events.
Jul 7, 14:08 The latest reply from a reviewer, though truncated, continues the discussion regarding the appropriate classification of `WAIT_FOR_STANDBY_CONFIRMATION`. The reviewer is likely responding to the proposer's analysis of why this event might or might not fit into the `WaitEventIPC` category, indicating ongoing deliberation.
Jul 6, 20:29 The latest reply from the proposer addresses the question of reclassifying `WAIT_FOR_STANDBY_CONFIRMATION`. The proposer notes it shares characteristics with `WaitEventIPC` events, being a condition variable wait, but also points out that its trigger is a socket read and that there is a higher bar for changing the classification of an event post-release to preserve comparability across versions.
archive ↗
18 Don't use the deprecated and insecure PQcancel in our frontend tools anymore Patch Review 22 msgs Jul 7, 15:29
opened Dec 14, 14:40 ·last activity Jul 7, 15:29

This thread focuses on a significant refactoring effort to remove the insecure PQcancel function from PostgreSQL's frontend tools (like psql) in favor of newer, more secure cancellation APIs introduced in a recent commit. The initial proposer outlined the changes, which involve moving cancellation logic to a dedicated thread on non-Windows platforms to align with existing Windows behavior and avoid signal-safety issues. The proposer also considered backporting the change to recent PostgreSQL versions. Discussions included addressing complexities in psql's cancel handling, identifying redundant global variables, and potential follow-up work like using threads for pg_dump on Unix. One reviewer committed a documentation-only patch related to current cancel handling. Later in the thread, a race condition with select() and signal handling was identified, leading to a new patch proposal that uses a 1-second poll as a temporary fix. The reviewer then explored more robust long-term solutions, suggesting various alternatives like pselect(), ppoll(), or custom helper functions, indicating ongoing deeper architectural discussions.

3 recent replies
Jul 7, 14:52 The reviewer acknowledged the proposer's updated patch for the select() race condition but expressed dissatisfaction with the polling fallback. The reviewer then suggested several more robust alternatives for implementing asynchronous I/O and cancellation handling, including using pselect(), ppoll(), or a custom poll() implementation for Windows, to avoid polling and improve consistency across platforms.
Jul 7, 07:54 The proposer submitted a new patch version addressing a critical concern regarding the reliance on signals to interrupt `select()`, which is not universally guaranteed or race-condition safe. The updated patch now implements a robust, consistent 1-second polling mechanism across all supported platforms and includes improved documentation for clarity.
Jul 7, 06:14 The latest reply from a committer provides historical context regarding a previous concern about the --single-step flag's interaction with PQcancel(). The committer recalls that the issue likely stemmed from CancelRequest not being set when PQcancel() failed in older versions, a behavior since changed. This clarifies why the current patch's behavior with --single-step is consistent.
archive ↗
19 Propagate stadistinct through GROUP BY/DISTINCT in subqueries and CTEs Patch Review 5 msgs Jul 7, 07:29
opened Apr 13, 01:18 ·last activity Jul 7, 07:29

This thread addresses a critical performance issue in PostgreSQL's query planner, where cardinality estimates for GROUP BY or DISTINCT clauses within subqueries and CTEs were often inaccurate, leading to inefficient execution plans. The proposer demonstrated cases where queries ran hundreds or thousands of times slower due to the planner falling back on 1/DEFAULT_NUM_DISTINCT for selectivity. The proposed solution is a patch that propagates stadistinct (number of distinct values) from base tables to GROUP BY or DISTINCT key columns, allowing for more accurate 1/ndistinct estimates. The latest version of the patch (v2) also accounts for how stanullfrac (null fraction) should be adjusted when grouping collapses NULL values.

Recent reply
Jul 7, 05:31 The latest reply from the proposer introduces v2 of the patch, which addresses a self-identified issue: the stanullfrac (null fraction) was not correctly adjusted after grouping. The updated patch now accounts for NULL values collapsing during grouping, setting stanullfrac to 1 / (ndistinct + 1) for single grouping keys and approximating it as zero for multiple keys to maintain optimistic hash-join-favoring estimates.
archive ↗
20 Schema-qualify the equality operator when deparsing NULLIF/IS DISTINCT FROM Proposed 4 msgs Jul 7, 14:28
opened Jul 3, 12:56 ·last activity Jul 7, 14:28

This thread addresses a long-standing issue where views using NULLIF or IS [NOT] DISTINCT FROM with equality operators not present in the search_path (e.g., hstore) fail to dump and restore correctly due to the deparser emitting unqualified operator names. The proposer initially suggested rewriting these constructs into CASE expressions with schema-qualified operators (v1), but a reviewer raised concerns about performance and the need for a more comprehensive solution for similar cases like JOIN USING. The proposer acknowledged these concerns and withdrew v1. A revised approach (v2) was then proposed, which involves modifying the deparser to add explicit OPERATOR() decoration to all constructs that resolve operators by unqualified names, covering a broader range of affected statements. This new approach aims to preserve the original expression tree, maintaining performance and optimizations. The reviewer found this direction promising.

3 recent replies
Jul 7, 14:27 The reviewer acknowledged the second version of the patch, finding its broader approach promising as it addresses a wider array of related issues, including JOIN USING. The reviewer also suggested that the proposer register the patch for the upcoming commitfest.
Jul 3, 14:11 The latest reply from a committer expresses strong disapproval of the proposed CASE statement rewrite. The committer cites concerns about potential performance impacts, the non-comprehensive nature of the fix (not addressing other related issues like JOIN USING), and the need for new syntax in those cases, effectively rejecting the current approach.
Jul 3, 12:56 The proposer identified a problem where views using `NULLIF` or `IS DISTINCT FROM` fail to restore if their equality operator isn't in the `search_path`. The proposed solution transforms these constructs into `CASE` expressions that allow for schema-qualification of the equality operator, ensuring dump/restore compatibility. A limitation exists for volatile inputs, where the original syntax is kept.
archive ↗
21 Add missing CustomPathMethods on typedefs.list Rejected 2 msgs Jul 7, 14:28
opened Jul 7, 13:25 ·last activity Jul 7, 14:28

This thread concerns a cosmetic issue where `CustomPathMethods` was not correctly indented in `src/include/nodes/extensible.h` because its declaration was missing from `typedefs.list`. The proposer submitted a patch to add it. However, a reviewer explained that `typedefs.list` is a mechanically generated file. Changes to it will not persist if the typedef is not actively used in the code to declare objects (variables, fields, parameters, etc.). Since `CustomPath` references it using `const struct CustomPathMethods *methods;` rather than the typedef, the proposed fix was deemed non-persistent. The reviewer suggested that a persistent fix would require modifying the usage or duplicating the typedef declaration in `pathnodes.h`.

2 recent replies
Jul 7, 14:17 The reviewer clarified that the `typedefs.list` file is automatically generated and changes will not be permanent if the type is not used to declare objects. The reviewer explained that `CustomPathMethods` is not directly used in object declarations, suggesting an alternative approach for a lasting fix.
Jul 7, 13:25 The proposer identified a formatting error in `src/include/nodes/extensible.h` and proposed a patch to correct the indentation of `CustomPathMethods` by adding its declaration to `typedefs.list`.
archive ↗
22 generic plans and "initial" pruning Patch Review 59 msgs Jul 6, 09:29
opened Dec 9, 07:10 ·last activity Jul 6, 09:29

This thread focuses on optimizing lock acquisition for generic plans, particularly in the context of partition pruning. The proposer redesigned the executor's startup process, moving execution lock acquisition out of GetCachedPlan() to callers, allowing for more precise, pruning-aware locking. The patch series (v13) introduced functions to handle validity checks, retries, and to lock only the relevant, unpruned partitions. Reviewer 2 confirmed that SELECT queries with reused generic plans successfully acquired locks only on active partitions. However, UPDATE statements still locked all partitions, which the proposer clarified is outside the scope of this specific optimization for now. A critical resource ownership bug in the PortalLockCachedPlan() retry path was identified by Reviewer 2 on a Windows build and confirmed by the proposer, indicating the need for further refinement before commitment.

2 recent replies
Jul 6, 09:09 Reviewer 2 identified a critical resource ownership bug in the PortalLockCachedPlan() retry path, which causes a "plancache reference is not owned by resource owner Portal" error. The issue stems from an inconsistency where the cached plan is initially acquired without a specific owner, but is later released and reacquired with portal->resowner. The proposer confirmed the diagnosis and acknowledged the need to fix this ownership mismatch.
Jul 6, 08:14 The reviewer provided a detailed test report for v13, confirming that SELECT statements with reused generic plans correctly acquire locks only on matching child partitions. However, the reviewer observed that UPDATE statements still held locks on all child partitions. The reviewer also identified a potential ownership mismatch bug related to ReleaseCachedPlan() in the portal replan path, suggesting an inconsistency in how the portal->cplan reference is acquired and released.
archive ↗
23 postgres_fdw: fix cumulative stats after imported foreign-table stats Committed 12 msgs Jul 7, 14:28
opened Jun 12, 03:48 ·last activity Jul 7, 14:28

The thread identifies and resolves a bug in `postgres_fdw` where cumulative statistics were not updated for foreign tables when `ANALYZE` imported remote statistics using the `restore_stats = true` option. The proposer demonstrated that `pg_stat_get_analyze_count` remained zero and `pg_stat_get_last_analyze_time` was null for such tables. The root cause was identified as `do_analyze_rel()` being skipped during stats import, which is the only function that calls `pgstat_report_analyze()`. Initially, the proposer suggested modifying the `ImportForeignStatistics` API. However, a reviewer proposed an alternative: calling `pgstat_report_analyze()` directly within the `postgresImportForeignStatistics()` FDW callback. This alternative was adopted, refined, and subsequently committed and back-patched, resolving the issue.

3 recent replies
Jul 7, 14:22 The proposer acknowledged and thanked the reviewer for committing and back-patching the fix, indicating the successful resolution of the reported issue and the conclusion of the discussion.
Jul 7, 09:50 The latest message from the reviewer confirms that the patch has been updated to address the previously identified leftover code. Following this confirmation, the reviewer states that the patch has been "Committed and back-patched," indicating its successful integration into the codebase and application to relevant older branches to fix the cumulative statistics issue.
Jul 6, 14:51 The proposer released a v3 patch that addresses a minor oversight identified by Reviewer 1: a redundant `if (!stats_imported)` conditional. This refinement further cleans up the code, bringing the patch closer to completion and ensuring the correct updating of cumulative statistics after importing foreign table data in `postgres_fdw`.
archive ↗
24 Support EXCEPT for TABLES IN SCHEMA publications Patch Review 93 msgs Jul 7, 11:28
opened Apr 14, 06:30 ·last activity Jul 7, 11:28

This thread discusses extending the `EXCEPT` clause functionality to `TABLES IN SCHEMA` publications, building on previous work for `FOR ALL TABLES`. The goal is to allow users to exclude specific tables from schema-level publications, which currently publish all tables without exclusion. The proposer outlined a new syntax for `CREATE PUBLICATION`, `ALTER PUBLICATION ADD`, and `ALTER PUBLICATION SET` with the `EXCEPT` clause, requiring schema-qualified table names. Key rules for combining `TABLES IN SCHEMA` with `FOR TABLE` and handling conflicting definitions were detailed, along with consistency requirements for partitioned tables (e.g., excluding a root table also excludes its partitions). The discussion has involved multiple patches and detailed review, focusing on refining error messages, clarifying the behavior of `ADD/DROP TABLE` for excluded tables, and ensuring consistent handling of partition hierarchies. A recent bug related to conflicting exclusion lists was also reported and is under discussion.

3 recent replies
Jul 7, 10:43 The latest message from commenter 2 raises a new design question regarding conflicting entries during `CREATE PUBLICATION`. Specifically, it questions whether explicitly including a partition while its root table is excluded within a `TABLES IN SCHEMA EXCEPT` clause should be blocked at creation time, as `ALTER` operations already prevent such conflicts. The commenter also seeks clarification on the scope of the ancestor check in `check_publication_add_relation()`.
Jul 7, 09:01 The latest commenter queries whether a specific scenario, where a publication explicitly includes a partition while its root is excluded via an `EXCEPT` clause, should also be restricted. The commenter suggests this situation represents another form of conflicting entries that might need to be disallowed, akin to earlier identified issues with clashing schema exclusions. An example is provided to illustrate the problematic behavior.
Jul 7, 03:09 The latest reply from reviewer 2 confirms a bug reported by reviewer 1. It agrees that the current patch allows conflicting exclusion lists when the same schema is specified multiple times with different `EXCEPT` clauses, and this behavior should be disallowed to prevent logical inconsistencies, mirroring existing restrictions for table column lists.
archive ↗
25 implement CAST(expr AS type FORMAT 'template') Discussing 53 msgs Jul 7, 12:28
opened Jul 27, 15:43 ·last activity Jul 7, 12:28

The thread discusses the implementation of `CAST(expr AS type FORMAT 'template')` to align with the SQL standard. The initial patch introduced this syntax, primarily transforming it into existing `to_` formatting functions. A reviewer questioned the design's coherence, particularly how it relates to standard casts and suggested the existing `to_char` functions might suffice. Another commenter, who also contributed patches, expanded on potential use cases beyond date/time, envisioning a generic mechanism for conversions between types and external representations (text/bytea), including support for extension types. However, the reviewer maintained skepticism about the feature's fundamental value and its broadness. In the latest messages, the commenter who proposed the broader scope acknowledged that their previous patch was too expansive, realizing that the SQL standard's T839 feature is specifically for formatted casts between datetime types and character strings. This commenter suggests narrowing the scope to this specific standard feature and proposes renaming the thread to reflect this focus, while the reviewer continues to question the overall necessity of the feature.

3 recent replies
Jul 7, 12:07 The reviewer responds to the suggestion of re-scoping the feature. They state that the previous patch accurately implemented the feature for arbitrary types, if that were the desired scope. However, the reviewer reiterates that the primary unresolved issue is whether the feature itself is wanted at all, regardless of its specific scope.
Jul 7, 03:56 The latest reply from a reviewer concludes that the previous patch for CAST(... FORMAT ...) was too broad. Based on a closer look at the SQL standard and feedback, the reviewer proposes narrowing the scope of the next version to specifically support SQL feature T839 (formatted casts between datetime types and character strings), avoiding arbitrary source/target pairs and ad-hoc parser rewrites to specific function names.
Jul 6, 20:27 The latest message from a reviewer questions the suggested limitation of formatted conversions to only data types and external representations like text or bytea. They point out that if the SQL standard allows arbitrary type pairs for such casts, then the design should align with that broader scope.
archive ↗
26 ci: namespace ccache by PostgreSQL major version Rejected 4 msgs Jul 7, 18:28
opened Jul 3, 14:26 ·last activity Jul 7, 18:28

The proposer observed that CI jobs fail with "incompatible library ... version mismatch" errors when the PostgreSQL major version is updated. This occurs because GitHub Actions ccache reuses old object files, leading to libraries compiled against a previous version. To address this, the proposer suggested namespacing the ccache key by the PostgreSQL major version, by adding a PG_MAJOR_VERSION variable to the CI file, updated by version_stamp.pl. A reviewer expressed suspicion, arguing that major version is only a partial indicator of ABI compatibility, and that ccache should inherently detect such changes if used correctly (e.g., in "depend mode" or "direct mode"). The proposer acknowledged this feedback, realizing their initial approach was flawed. Another commenter explicitly stated this was not the right answer and pointed to a different thread and a more suitable technical solution.

3 recent replies
Jul 7, 17:35 The commenter echoed earlier concerns that namespacing ccache by major version is insufficient for ABI compatibility. The commenter pointed to a related discussion thread, reiterating that the underlying issue is ccache's handling of precompiled headers and suggested adding the -fpch-deps flag to GCC builds as a more appropriate fix.
Jul 6, 08:05 The proposer acknowledged the feedback from the commenter, admitting that their initial research was insufficient and the proposed solution, which involved namespacing ccache by PostgreSQL major version, was not the correct direction to address ABI compatibility issues.
Jul 3, 14:26 The proposer highlights a CI failure where ccache reuses objects from a previous PostgreSQL major version, causing version mismatch errors. They propose namespacing the ccache key with 'PG_MAJOR_VERSION', automatically updated by 'version_stamp.pl', to prevent cross-version object reuse.
archive ↗
27 pg_stat_io_histogram Patch Review 33 msgs Jul 6, 12:29
opened Jan 26, 09:40 ·last activity Jul 6, 12:29

The proposer introduces `pg_stat_io_histogram` to track and display I/O latency profiles, aiming to help identify I/O outliers and "stuck" PostgreSQL instances. The initial proposal focuses on a global histogram for various backend types, objects, contexts, and I/O types. Early discussions center on the overhead and memory footprint, particularly for `pgStatLocal` and `PendingIOStats` in each backend. A core committer expresses concern about storing "lots of values that are never anything but zero." The proposer explores several strategies to reduce memory usage, including dynamically allocating memory and using indirect offsets for histograms in both backend-local and shared memory. Multiple patch versions are shared, with `v9b` and `v10` significantly reducing shared memory footprint. Discussions also cover the histogram's range and bucket granularity, with suggestions to prioritize higher latencies for outlier detection.

Recent reply
Jul 6, 11:41 The latest reply from the proposer introduces `v10`, which continues the effort to reduce shared memory footprint by implementing indirect offsets. This version focuses on making shared memory sized and allocated on startup by the postmaster. This iteration aims to address previous memory consumption concerns while maintaining the core functionality of tracking I/O latencies.
archive ↗
28 MinGW ccache snafu on CI Discussing 2 msgs Jul 7, 18:28
opened Jul 4, 04:44 ·last activity Jul 7, 18:28

The proposer reported a recurring issue in the MinGW CI job where ccache gets confused, leading to "incorrect version" errors (e.g., vacuumdb (PostgreSQL) 19beta1 found, 20devel expected). This suggests ccache fails to recompile correctly, producing stale object files. The proposer noted that the MinGW job uses precompiled headers and wondered if incorrect ccache switches might be a factor, contrasting it with the MSVC job which also uses PCH but doesn't exhibit the same problems. A commenter pointed to a related thread, suggesting that adding -fpch-deps for gcc (until ccache or meson handles it automatically) might be the solution.

2 recent replies
Jul 7, 17:37 The commenter referenced another thread discussing the same issue and reiterated a previously suggested solution. The solution involves adding the -fpch-deps flag for GCC builds to ensure correct dependency tracking with precompiled headers, suggesting it should be added when supported by the compiler, regardless of whether PCH is enabled.
Jul 4, 04:44 The latest (and only) message is the initial report from the proposer, detailing a problem with the MinGW CI job. The proposer explains that `ccache` is producing stale object files, leading to incorrect version detection errors during the build process. The proposer suggests this might be related to how precompiled headers are handled or incorrect `ccache` settings.
archive ↗
29 Adding a stored generated column without long-lived locks Discussing 9 msgs Jul 7, 18:28
opened Mar 17, 10:31 ·last activity Jul 7, 18:28

The proposer introduced a feature to add a stored generated column to a large table without requiring long-lived AccessExclusiveLock for a full table rewrite. The initial proposal involved an ALTER TABLE ... ALTER COLUMN ... ADD GENERATED ALWAYS AS (expr) STORED command, which would skip the rewrite if a CHECK (c = expr) constraint already existed and was valid. Discussion ensued about standard SQL syntax compliance, whether changing the expression should also avoid a rewrite, and existing "tricks" for online migrations. A subsequent design iteration (v2) refined the approach to explicitly use a constraint: ALTER TABLE ... ALTER COLUMN ... ADD GENERATED ALWAYS AS (expr) STORED USING CONSTRAINT check_name, failing if no suitable constraint was found, and changing (c = expr) to (c IS NOT DISTINCT FROM expr) to handle NULLs. Further refinement (v3) suggested dropping the explicit (expr) from the syntax, deriving it directly from the constraint, and dropping the constraint automatically. However, a reviewer preferred not dropping the constraint, aligning with existing behavior. The current patch (v6) adheres to iteration 3, does not drop the constraint, and includes documentation and tab completion.

3 recent replies
Jul 7, 18:16 The reviewer approved the idea of inferring the generation expression directly from the constraint, simplifying the syntax and reducing potential errors. However, the reviewer found the proposed syntax "ADD GENERATED ALWAYS STORED USING CONSTRAINT" ugly and suggested alternatives like "ADD GENERATED ALWAYS USING CONSTRAINT ... STORED" or "ADD GENERATED USING CONSTRAINT". The reviewer also provided feedback on the documentation, suggesting a clearer explanation of where the "generation expression" comes from in the updated syntax.
Jul 3, 06:42 The latest message provides a `v6` patch, which primarily fixes a brittle test from `v5` that relied on a `DEBUG` message for verification. The proposer replaced this with an `injection_point` test to enhance reliability and reduce dependencies on logging configurations, seeking feedback on this new testing approach.
Jun 30, 13:44 The latest reply from the proposer presents `v5` of the patch, which implements 'Design Iteration 3'. This version simplifies the syntax by deriving the generated expression directly from the specified constraint, removes the automatic dropping of the constraint, and addresses previous reviewer comments regarding documentation, psql tab completion, and proper handling of identity columns and sequences.
archive ↗
30 Function scan FDW pushdown Patch Review 16 msgs Jul 7, 10:29
opened Mar 18, 12:08 ·last activity Jul 7, 10:29

This thread discusses the development and refinement of a patch to enable Foreign Data Wrappers (FDWs) to push down function scans, allowing functions like `generate_series` to be executed remotely. The initial patch faced issues with `rte->functions` handling and unfinished design aspects, leading to it being returned for feedback. Subsequent versions addressed these concerns, including stricter checks for function types (disallowing set-returning complex types or functions with parameters in arguments) and moving function information to foreign scan private data. A key challenge was correctly handling `rti` mappings after `setrefs` and ensuring proper deparsing of `ColumnRef` for whole-row variables, especially with nullable outer sides. Discussions also revolved around the cost model, explaining why local joins might still be preferred over pushed-down ones due to cost estimations. The latest iterations focus on refining the handling of whole-row variables in update/returning clauses and classifying `baserestrictinfo` correctly by passing `fpinfo` down to `foreign_expr_walker`. The thread is actively addressing review comments and identified issues, with a focus on robustness and correct behavior across various query types.

2 recent replies
Jul 7, 10:25 The latest reply from the original proposer suggests re-including `funcid` in the per-RTE metadata. This change would allow the `EXPLAIN` output to display the actual function name instead of just an alias, improving readability and consistency with how relation names are shown, a minor refinement for user experience.
Jul 6, 14:11 The latest reply from a reviewer agrees with the proposer's approach of passing fpinfo down to foreign_expr_walker() to address issues with RTE RelOptInfo missing fdw_private during condition classification. The reviewer clarifies that get_relation_column_alias_ids() is not a concern for the current scope of inner join pushdown. They also confirm that using parameters in function arguments, as observed in a lateral join example, does not introduce correctness problems.
archive ↗
31 [PATCH] Add pg_get_table_ddl() to reconstruct CREATE TABLE statements Patch Review 51 msgs Jul 7, 17:28
opened Jun 3, 12:58 ·last activity Jul 7, 17:28

The proposer introduced a patch to add `pg_get_table_ddl()`, a function to reconstruct `CREATE TABLE` statements and related DDL for indexes, constraints, rules, and statistics. The function aims to support various table and column features, offering options to fine-tune the output. Initial feedback included concerns about overlap with `pg_dump` and suggestions for option handling. Several issues were identified through review, such as incorrect clause order for `TABLESPACE` and `ON COMMIT`, missing `ONLY` for inherited default overrides, and erroneous duplicate unique indexes on partitioned tables, which the proposer addressed in subsequent patch versions. The latest review of v17 confirmed previous fixes but highlighted new problems: `serial` columns produce non-replayable DDL because sequences are not created, and partition/inheritance children lose specific properties like dropped defaults or custom constraint names upon DDL replay.

3 recent replies
Jul 7, 16:53 A reviewer re-tested v17 and confirmed previous fixes but identified new issues preventing full DDL round-tripping. Key problems include `serial` columns generating DDL that fails to replay due to missing sequence creation, and partition/inheritance children losing custom properties like dropped defaults or constraint names after DDL reconstruction, resulting in semantic differences or errors.
Jul 6, 10:57 The latest reply from the proposer acknowledges and confirms that all issues raised by the previous reviewer, including clause order corrections (TABLESPACE vs ON COMMIT), proper use of ALTER TABLE ONLY for inherited defaults, and other partition-related fixes, have been addressed in the newly submitted v17 patch.
Jul 4, 16:35 The latest reply, from reviewer 2, provides a detailed review of patch v16. They confirm that several previous issues are fixed but identify new problems. These include an incorrect clause order (`TABLESPACE` before `ON COMMIT`), a missing `ONLY` keyword when emitting child-default overrides (leading to unintended recursion), and redundant `SET DEFAULT` statements for partitioned tables. The reviewer points out that the patch generally fails to emit `ONLY` for `ALTER TABLE` statements.
archive ↗
32 Increased SECURITY RISCS from omitting some compikler options when building PG with meson; e.g. -fcf-protection=full Discussing 2 msgs Jul 7, 12:28
opened Jul 7, 09:59 ·last activity Jul 7, 12:28

A proposer highlighted that self-compiled PostgreSQL instances using Meson on Fedora 44 lack default security compiler options, such as Shadow Stack (SHSTK) and Indirect Branch Tracking (IBT), unlike distribution or PGDG provided packages. This discrepancy could lead to reduced security. The proposer suggested enabling these compiler options by default in both autoconfigure and Meson build systems to ensure consistent security across different operating systems, PostgreSQL versions, architectures, and compilers, and to improve build reproducibility. A commenter disagreed, arguing that distributions are better equipped to decide on specific security protections. The commenter stated it would be unreasonable and overly complex for PostgreSQL to track and align with the diverse and evolving compiler flags adopted by various operating systems.

2 recent replies
Jul 7, 12:25 The commenter expresses disagreement with the proposal. They argue that distributions are better placed to decide when and how to activate specific security protections. The commenter further suggests it would be unreasonable for PostgreSQL to track and sync with the varied compiler flags used by different operating systems, citing the complexity of such an endeavor.
Jul 7, 09:59 The proposer raises a security concern that self-compiled PostgreSQL versions might lack compiler protections like Shadow Stack and Indirect Branch Tracking, which are present in distribution-provided binaries. They propose that all security-related compiler options used in official releases should be enabled by default in PostgreSQL's build systems (autoconf and Meson) and documented for better security and reproducibility.
archive ↗
33 Use HostsFileName everywhere Patch Review 4 msgs Jul 7, 08:29
opened Jun 18, 06:10 ·last activity Jul 7, 08:29

The proposer identified a bug where three specific PostgreSQL error messages hardcoded 'pg_hosts.conf' instead of referencing the configurable `hosts_file` GUC. This inconsistency could lead to confusion when a custom hosts file is in use. A patch was submitted to update these error messages to dynamically use the `HostsFileName` variable. Reviewers confirmed the bug's validity and the patch's correctness, considering it a useful cosmetic improvement. A minor typo in the commit message was noted. A potential NULL-safety edge case, if `hosts_file` were unset, was investigated but deemed non-fatal as configuration loading at startup prevents a NULL value in typical operation.

3 recent replies
Jul 7, 07:34 A reviewer has acknowledged the patch and its preceding discussion, confirming the proposed fix is sound. They stated their intention to commit the changes shortly after returning from vacation, indicating a high likelihood of the patch being integrated soon.
Jul 7, 06:36 The latest reply from a committer acknowledges that the bug addressed by the patch, an oversight from a previous commit, is not critical but offers a "nice life improvement" for debugging. The committer confirms that the NULL-safety concern raised by another reviewer is not an issue, as the relevant GUCs are always loaded before authentication. The committer seeks final confirmation from another expert ("Daniel?").
Jul 6, 23:43 The reviewer affirms the correctness and practical utility of the proposed patch, stating it will significantly improve debugging when the `hosts_file` GUC has been customized. The reviewer also notes a minor typo in the commit message and provides an analysis of a related NULL-safety concern, concluding that while passing a NULL `HostsFileName` could lead to a misleading `(null)` in the error message, it would not result in a crash or segfault.
archive ↗
34 remove unnecessary volatile qualifiers Committed 4 msgs Jul 7, 16:29
opened Jun 30, 21:47 ·last activity Jul 7, 16:29

The proposer initiated a thread to remove `volatile` qualifiers deemed unnecessary from the PostgreSQL codebase, aiming to improve code clarity and potentially enable compiler optimizations. A reviewer provided feedback, identifying one instance where `volatile` was still necessary due to usage within `PG_TRY/CATCH` blocks and suggesting a minor refactoring. The proposer promptly incorporated the feedback, reverting the problematic change and implementing the suggested refactoring. The revised patch was then committed to the repository.

3 recent replies
Jul 7, 16:03 The proposer announced that the patch, which removes unnecessary `volatile` qualifiers after incorporating review feedback, has been committed to the codebase.
Jul 6, 15:47 The proposer accepted the reviewer's feedback, removing the local `procglobal` variable as suggested. The proposer also reverted the change for the `flags` variable, acknowledging that its usage within a `PG_TRY/CATCH` block indeed made the `volatile` qualifier necessary to ensure correct behavior.
Jul 6, 11:58 The latest reply from the reviewer raises a concern about one specific `volatile` removal in `ProcessProcSignalBarrier()`. The reviewer points out that the `flags` variable is modified within a `PG_TRY()` block and subsequently read in the `PG_CATCH()` block, which might require the `volatile` qualifier to ensure correct behavior.
archive ↗
35 [PATCH] Don't call ereport(ERROR) from recovery target GUC assign hooks Patch Review 32 msgs Jul 7, 07:29
opened Apr 13, 08:21 ·last activity Jul 7, 07:29

This thread discusses a patch aimed at fixing a long-standing design issue in PostgreSQL where recovery target GUC assign hooks incorrectly raised errors, causing guc.c's internal state to become inconsistent. The proposer's solution involves removing conflict checks from these assign hooks and centralizing conflict detection in validateRecoveryParameters(), which runs safely after all GUCs are loaded. This changes when the error occurs (from postmaster startup to the startup process) and results in conflicting settings being silently ignored if recovery is not requested. Discussions covered the wording of error messages, the utility of hinting pg_settings during a failed startup, and the clarity of regression test comments. The patch has undergone several revisions, with the latest focusing on refining error messages and test descriptions.

3 recent replies
Jul 7, 06:59 The latest reply from a reviewer suggests further simplification of the error message for multiple recovery targets, proposing a more concise errmsg and errdetail. The reviewer also suggests trimming down the comments in the TAP script, noting some seem verbose or "AI set of comments." Finally, the reviewer recommends adding a test case to check conflicts with the immediate recovery target.
Jul 7, 03:01 The latest reply from the proposer announces the submission of v8 of the patch. This version incorporates the previously discussed change to remove the `errhint` from the error message, addressing the feedback that suggesting `pg_settings` might be unhelpful when a server fails to start.
Jul 6, 22:30 A reviewer confirmed agreement to remove the `errhint()` from the error message. This decision was made because the hint, which suggested checking `pg_settings`, would be unhelpful in a scenario where the server fails to start due to conflicting recovery targets. The consensus is that no hint is better than a potentially misleading one in such a critical startup failure.
archive ↗
36 Row pattern recognition Discussing 50 msgs Jul 7, 07:29
opened Jun 17, 13:13 ·last activity Jul 7, 07:29

This thread is an extensive discussion on a patch for Row Pattern Recognition (RPR). Initially, a correctness problem was identified where window function results could change based on unrelated window functions, leading to incorrect pattern matching advancement. This highlighted a need for the RPR window to perform matching upfront. Subsequent discussions addressed changes in the planner's cost model, specifically how it charges for DEFINE expressions, with a reviewer clarifying that the change was a simplification and did not alter the cost. The thread also delved into advanced design considerations for CLASSIFIER and row pattern variable storage, and a bug was found related to context-absorption optimization causing incorrect match counts in specific queries. The participants discussed whether to integrate major refactoring changes immediately or defer them to follow-up patches to keep the current commit small.

3 recent replies
Jul 7, 07:07 The latest reply from a reviewer identifies a bug in the context-absorption optimization of the RPR patch, which leads to incorrect match results for certain queries. The reviewer explains that an absorbing context can incorrectly discard a previously recorded match from a non-absorbable branch. A fix is proposed to ensure absorption only occurs when the absorbing context also covers the recorded match.
Jul 6, 06:02 The latest reply from a developer agrees with a previous suggestion to defer the implementation of short-circuit or lazy evaluation of DEFINE predicates to a separate patch series. This consensus allows the current RPR patch set to maintain a smaller scope and focus on committed changes, with the more complex optimization to be tackled in future work, potentially driven by the original proposer of that idea.
Jul 4, 16:39 The latest message is a design note from the proposer focusing on data structures for `CLASSIFIER` and row pattern variables, intended for the next round of RPR development. The proposer reflects on an incorrect premise about keeping full match history during matching, dissolving the problem into smaller, bounded storage pieces. The note details what to store per case and how, while inviting discussion on the matching engine's aggregate accumulator lifecycle.
archive ↗
37 ri_Fast* crash w/ nullable UNIQUE constraint Discussing 3 msgs Jul 7, 16:29
opened Jul 5, 21:05 ·last activity Jul 7, 16:29

The proposer initiated a review of the `ri_Fast*` family of commits, identifying three significant issues. The first is a crash occurring with foreign key constraints that reference nullable unique constraints, due to an incorrect assertion. The second is a session-lifespan memory leak originating from functions called within a `TopMemoryContext` that should use a shorter-lived context. The third raises concerns about whether newly queued deferred triggers might be lost if they are created during the firing of fast-path batch callbacks. The implementer acknowledged these findings and committed to addressing them after resolving a related issue.

3 recent replies
Jul 7, 13:44 The implementer acknowledged the proposer's detailed review of the `ri_Fast*` commits, including the identified crash, memory leak, and trigger queuing concerns, and stated they would address these issues once another related task is completed.
Jul 5, 21:47 The latest message provides a forgotten attachment, which is a demo patch illustrating a crash when a foreign key references a nullable UNIQUE constraint. This attachment substantiates the first reported critical issue, highlighting how an `Assert(!isnull[i])` fails because the code incorrectly assumes referenced columns cannot be NULL, leading to a system crash.
Jul 5, 21:05 The proposer reviewed the `ri_Fast*` commit family and reported three critical issues. These include a crash due to an assertion failure when foreign keys reference nullable unique constraints, a memory leak in `record_eq()` due to incorrect memory context usage, and a potential integrity hole where deferred triggers queued by fast-path callbacks might be missed. The proposer provided a crash reproduction and detailed explanations for all findings.
archive ↗
38 libpq: Process buffered SSL read bytes to support records >8kB on async API Committed 26 msgs Jul 7, 16:29
opened Mar 16, 13:13 ·last activity Jul 7, 16:29

The thread discusses a critical issue in libpq's asynchronous API when using SSL, where records larger than 8KB could cause deadlocks or hangs due to the library failing to fully drain OpenSSL's internal buffer. The proposer provided patches to address this by ensuring pqReadData() processes all buffered SSL read bytes. Early discussions involved concerns about the "tight coupling" between libpq and OpenSSL's buffering mechanisms and potential impacts on future architectural changes, which the proposer clarified were not being precluded. Multiple reviewers provided feedback, identifying a layering violation and a bug where buffer pointers were not correctly advanced. These issues were resolved, a reproducer script was shared, and the patch was successfully tested by a third party. The final version was committed and backpatched to all supported PostgreSQL versions.

2 recent replies
Jul 7, 15:58 The reviewer announced that the patch, which addresses the issue of libpq's async API not fully processing buffered SSL read bytes, has been committed and backpatched to all supported versions of PostgreSQL after extensive review and testing.
Jul 3, 09:25 The latest reply from commenter 2 provides an updated patch version (v4), incorporating minor changes to comments and commit messages, and a refined split of the changes into two patches. Commenter 2 thanks the reviewer for testing and indicates an intention to commit the patch soon, while also seeking further input on whether GSSAPI should adopt a similar `pqReadData` return behavior to SSL for consistency.
archive ↗
39 RFC: Logging plan of the running query Discussing 62 msgs Jul 7, 15:29
opened Feb 3, 12:46 ·last activity Jul 7, 15:29

This "Request for Comments" thread discusses a proposed feature to log the execution plan of a running query, accessible via a pg_log_query_plan() function. Early discussions focused on the naming of the function and the asynchronous nature of the operation, with an agreement to document potential delays. The proposer addressed initial feedback by fixing specific code issues (e.g., extern keywords, duplicate definitions, boolean parameters, injection points) and updating documentation regarding asynchronicity and innermost query plan logging. A reviewer provided detailed architectural feedback, suggesting the use of a ExecProcNode() hook for better extensibility and safety instead of the current ExecSetExecProcNodeRecurse() approach, and raising concerns about QueryDesc pointer safety and predictability for automation. The proposer responded by reiterating performance concerns about hooks and clarifying the behavior with nested queries, explaining that the current mechanism would log the plan for deeper queries. The discussion continues regarding implementation robustness and guarantees for users.

3 recent replies
Jul 7, 14:46 The proposer clarified a previous misunderstanding regarding the behavior of the pg_log_query_plan() feature with nested queries. The proposer explained that if execution moves to an external or deeper QueryDesc after plan nodes are wrapped, the LogQueryPlanPending flag remains set, ensuring that the plan tree for that external or deeper QueryDesc will also be wrapped and the plan will be logged. This addresses a concern raised by the reviewer about the feature's predictability.
Jul 6, 13:05 A reviewer expresses concerns about the ExecSetExecProcNodeRecurse() approach, suggesting it's fragile and might require updates when new plan nodes are added. The reviewer proposes using a global hook in ExecProcNode() instead for better robustness and extensibility, which could also serve other extension developers. The reviewer also questions the safety of storing QueryDesc pointers during interrupts and points out potential issues if execution moves to an external or deeper query after plan nodes are wrapped.
Jul 6, 06:21 The latest reply from the proposer addresses feedback regarding the proposed `ExecSetExecProcNodeRecurse` and `QueryDesc` pointer handling, acknowledging the concerns about fragility and safety. The proposer reiterates performance concerns about a global hook approach and suggests that specific delays in plan logging might be an unavoidable consequence of the asynchronous design, which should be clearly documented. The proposer also commits to adding documentation that clarifies only the innermost nested query's plan is logged.
archive ↗
40 pg_dump: use threads for parallel workers on all platforms Discussing 2 msgs Jul 7, 15:29
opened Jul 5, 23:21 ·last activity Jul 7, 15:29

This thread discusses a proposal to standardize pg_dump's parallel worker implementation across all platforms by using threads instead of processes, coordinated via an in-process work queue. The proposer has already developed the Windows half of this change. A reviewer provided feedback, agreeing with the direction but raising concerns about testing Windows-only code and inquiring about thread-safety hazards for setFmtEncoding() and quote_all_identifiers. The reviewer also noted the introduction of _Thread_local and suggested removing an unused hook. A specific bug fix related to an unchecked _beginthreadex return (patch 0001) was committed and backpatched by the reviewer, with the understanding that the larger thread-based model unification remains an ongoing discussion.

2 recent replies
Jul 7, 15:25 The reviewer announced the commitment and backport of patch 0001, which fixed an unchecked _beginthreadex return for the Windows-specific part of the code. This resolves a specific bug while indicating that the larger proposal for unifying pg_dump's parallel worker model using threads on all platforms is still pending further development and review.
Jul 5, 23:21 The commenter 1 acknowledges the proposer's approach but expresses reservations about the difficulty of testing Windows-specific code without an appropriate environment. Commenter 1 also provides detailed technical feedback on the proposed changes, including the use of `_Thread_local`, the `getLocalPQExpBuffer` hook, and potential thread-safety issues with `setFmtEncoding()` and `quote_all_identifiers`.
archive ↗
41 Global temporary tables Patch Review 32 msgs Jul 7, 10:29
opened Jun 21, 19:08 ·last activity Jul 7, 10:29

This extensive thread details the development of a patch series implementing global temporary tables (GTTs), where definitions are permanent and shared, but data is session-local and temporary. The proposer aims to provide a new design addressing issues in older attempts. The patch series introduces a `RELPERSISTENCE_GLOBAL_TEMP` type, manages local storage and buffers for each session, and tracks GTTs across backends using a shared hash table to prevent concurrent DDL issues. Key aspects include handling indexes, sequences, and introducing `pg_temp_class` for per-session overrides. Recent discussions focused on `relfrozenxid` handling for GTTs, which was revised to set `pg_class.relfrozenxid` to 0 and update only `pg_temp_class` values. However, a reviewer uncovered a critical bug where `pg_temp_class` tuples could be lost during savepoint rollbacks, leading to unvacuumable and undroppable tables. The proposer submitted v7 to address this by implementing sub-XID tracking for `pg_temp_class` tuples. Another commenter reported that sequences attached to GTTs with `ON COMMIT DELETE ROWS` were not reset, which the proposer indicated was the expected behavior consistent with local temp tables. A different commenter reported issues with `pg_dump` not preserving the `ON COMMIT` clause, aborted transactions pinning `frozenxid`, and potential NULL pointer crashes with OOM in `dshash_find`. The latest comment identifies an infinite loop in `gtr_process_invalidated_gtrs()` when `debug_discard_caches` is enabled due to whole-cache invalidation.

3 recent replies
Jul 7, 09:06 The latest reply from a reviewer reports a new critical bug: setting `debug_discard_caches = 1` and creating a global temporary table leads to an infinite loop. This issue is traced to `gtr_process_invalidated_gtrs()` and suggests a problem with how whole-cache invalidation interacts with GTTs, requiring further investigation into cache handling.
Jul 7, 01:09 Reviewer 2 agrees with the proposer's rationale that sequences associated with global temporary tables, even with `ON COMMIT DELETE ROWS`, should not reset after a transaction commit, aligning with the behavior of local temporary tables. The reviewer also supports setting `pg_class.reloncommit` to `NULL` for non-table objects and prefers the proposer's current method of handling the on-commit state over an alternative reloption-based approach.
Jul 6, 21:53 Commenter 4 provided a multi-point review of the patch. Key issues highlighted include pg_dump not preserving the ON COMMIT clause for GTTs, aborted transactions pinning frozenxid which prevents datfrozenxid advancement, and several potential code defects such as missing return statements in cleanup functions and a possible null pointer access due to dshash_find_or_insert failing with OOM.
archive ↗
42 Fix duplicate detection for null-treatment window functions Proposed 1 msgs Jul 7, 15:29
opened Jul 7, 15:14 ·last activity Jul 7, 15:29

This thread identifies a bug in ExecInitWindowAgg() where duplicate detection for window functions with IGNORE NULLS or explicit RESPECT NULLS options fails. The proposer explains that the ignore_nulls flag is not copied when function information is appended to perfuncstate data, leading to wfunc->ignore_nulls == perfunc[i].ignore_nulls always evaluating to false for these cases. A minimal one-line fix is proposed to copy the ignore_nulls value, along with a clear reproduction case and debug logs illustrating the "cache miss" before the fix and "cache hit" after. The proposer included temporary debug logs in the patch to aid reviewers, noting they should be removed before commitment.

Recent reply
Jul 7, 15:14 The proposer has submitted a new patch to address a bug in ExecInitWindowAgg() where duplicate detection for window functions with null treatment options (IGNORE NULLS/RESPECT NULLS) was failing due to the ignore_nulls flag not being copied during function setup. The patch includes a one-line fix and a clear reproduction example.
archive ↗
43 support create index on virtual generated column. Patch Review 21 msgs Jul 7, 14:28
opened Mar 26, 07:15 ·last activity Jul 7, 14:28

This thread focuses on implementing support for creating indexes on virtual generated columns (VGCs). Early proposals involved transforming VGC indexes into expression indexes, with various restrictions and challenges related to dependency tracking and `ALTER TABLE` operations. An early patch (v1) was found to cause a bootstrap crash, which was subsequently fixed. A reviewer suggested a simpler approach by only supporting VGCs in expression indexes and limiting `SET EXPRESSION` functionality. The original proposer debated the implications of not adding new catalog columns for VGC indexes. A new proposer (proposer 2) took over, outlining a v2 patch that expands VGCs into expressions during index creation, storing them in `pg_index.indexprs`, and adding a new `indisvirtual` catalog column for clarity. The latest update, v4, revises `generated_virtual.sql` and related output, covering index constraints, regular indexes, and foreign keys, with all tests passing.

3 recent replies
Jul 7, 13:40 The latest reply from proposer 2 introduces a V4 patch. This version includes revised `generated_virtual.sql` and output files, specifically addressing index constraints, regular indexes, and foreign keys. The proposer states that all existing and newly added test cases are successfully passing with this updated patch.
Jul 6, 16:09 A commenter reported that while basic index scans now work with virtual generated columns, minor planner inconsistencies persist. The commenter provided an example demonstrating that a query using a virtual generated column in its WHERE clause still results in a bitmap heap scan with a recheck condition, unlike a similar query on a stored generated column, indicating further optimization is needed to fully leverage the index.
Jul 5, 09:00 The latest reply from commenter 3 indicates that the CFbot test results for the current patches show failures primarily concentrated in the `generated_virtual.sql` file and its associated test cases. This feedback highlights ongoing issues identified during automated testing of the proposed feature.
archive ↗
44 Reject ill-formed range bounds histograms in pg_restore_attribute_stats() Patch Review 2 msgs Jul 7, 08:29
opened Jul 7, 08:13 ·last activity Jul 7, 08:29

The proposer identified a bug in `pg_restore_attribute_stats()` and `pg_set_attribute_stats()` where these functions accept malformed range bounds histograms. These histograms, if not properly sorted or containing empty ranges, cause internal assertions to fail (crashing the backend) or lead to incorrect selectivity estimates in the planner. The issue is analogous to a previous bug with oversized MCV lists. The proposer submitted a patch that adds validation to the bounds histogram during import, rejecting ill-formed input with a `WARNING` to prevent such runtime failures, aligning with existing error handling practices for other inconsistent statistics inputs.

Recent reply
Jul 7, 08:27 A reviewer has acknowledged the bug report and the attached patch, indicating their intention to review and check the proposed solution the following day. This confirms the issue is under active consideration.
archive ↗
45 CAST(... ON DEFAULT) - WIP build on top of Error-Safe User Functions Discussing 82 msgs Jul 7, 08:29
opened Jul 22, 01:59 ·last activity Jul 7, 08:29

This thread details the implementation of `CAST(... ON CONVERSION ERROR)` for error-safe type conversions. The proposer identified several consistency issues, such as differing error propagation for operand errors (e.g., division by zero versus numeric overflow) and inconsistent behavior between plan-time constant folding and run-time evaluation. Debate also centered on how to handle user-defined cast functions—either disallowing them, permitting them with strict caveats, or limiting them to C/Internal language functions. A reviewer suggested that error-safeness should be a function property (via `CREATE FUNCTION`) rather than a cast property. The reviewer also detailed 'clear defects' in operand error swallowing and problematic interactions with nested `DEFAULT` clauses. The proposer has responded to some of these points and, in the latest messages, proposed a new `ERROR { SAFE | UNSAFE }` attribute for `CREATE FUNCTION` to explicitly mark error-safe functions, to be stored in `pg_proc`.

3 recent replies
Jul 7, 07:49 The proposer summarized the challenges for `CAST(... ON CONVERSION ERROR)`, noting that default expressions lack error-safe handling and that full bulletproof safety is infeasible. To address the need for clearly identifying error-safe functions, they proposed a new `ERROR { SAFE | UNSAFE }` keyword for `CREATE FUNCTION`. This property would be stored in a `proerrorsafe` column in `pg_proc`, set to true for built-in cast functions and defaulting to false for user-defined ones.
Jul 7, 06:12 The latest reply from the proposer addresses a reviewer's example involving a 'hardtype' cast that produces an error despite ON CONVERSION ERROR. The proposer clarifies that the 'hardtype' input function is deliberately designed to hard-error for testing purposes, meaning it is expected to bypass the error-safe mechanism. The proposer also reiterates that not all functions can be made error-safe.
Jul 2, 15:38 The proposer responds to specific "clear defects" raised by a reviewer. Regarding nested `DEFAULT` clauses silently converting hard errors, the proposer explains that if the top-level expression is error-safe, the entire expression compiles to support error-safe evaluation for its subexpressions, suggesting this is an intended consequence of the error-safe context implementation. For another point concerning `DEFAULT` policy, the proposer refers to a previous message for clarification.
archive ↗
46 Proposal: Conflict log history table for Logical Replication Discussing 54 msgs Jul 7, 08:29
opened Jun 25, 13:00 ·last activity Jul 7, 08:29

This thread concerns the design and implementation of a conflict log history table for Logical Replication. Following an initial commit of a preparatory part of the feature, discussions ensued regarding documentation, code comments, and minor stylistic improvements. However, a reviewer raised significant architectural concerns about the error handling strategy in the committed code. The reviewer criticized the approach of temporarily setting aside an error, invoking `AbortOutOfAnyTransaction()`, and then attempting to insert a conflict tuple before re-throwing the original error, citing potential unsafety for nested transaction blocks and risks of data inconsistency if the conflict logging itself fails. The original proposer defended the design, explaining that `AbortOutOfAnyTransaction()` operates at the outermost catch block and that `replorigin_xact_clear()` prevents replication origin advancement if conflict logging fails, ensuring consistency.

3 recent replies
Jul 7, 08:24 The original proposer addressed a reviewer's fundamental design concerns, clarifying that the `AbortOutOfAnyTransaction()` call is executed within the outermost `PG_CATCH` block, ensuring it does not interfere with nested transaction handlers. They further explained that if conflict log insertion fails, `replorigin_xact_clear()` prevents the replication origin from advancing, guaranteeing that the transaction and its associated conflict logging will be re-attempted upon replay.
Jul 6, 18:40 A core developer expresses serious architectural concerns regarding a recently committed preparatory patch and the overall design. The developer criticizes the method of error handling within a try/catch block and the lack of robust mechanisms to ensure the conflict log table's consistency. The developer suggests the current approach is fundamentally flawed and calls for a reversion of the committed changes and a complete redesign of the feature.
Jul 6, 10:18 The proposer responded to a reviewer's query about code comment changes. They indicated that those specific suggestions had not yet been addressed, as they believed no conclusion was reached on that particular set of modifications. This suggests the discussion on code comment style is still open for future resolution.
archive ↗
47 [RFC PATCH v0 0/7] Add EXPLAIN ANALYZE wait event reporting Discussing 8 msgs Jul 6, 18:29
opened May 8, 23:22 ·last activity Jul 6, 18:29

This RFC thread proposes adding `EXPLAIN (ANALYZE, WAITS)` to report completed wait events, both at the statement and plan-node levels. The proposer outlined an inclusive attribution semantic for plan nodes, fixed-size accumulators with overflow handling, and allocation-free wait-end accounting. Key review questions included the option name (`WAITS` vs. `WAIT_EVENTS`), the inclusive per-node attribution, the fixed accumulator limit, and the hot-path overhead. A core reviewer expressed significant skepticism regarding the proposed direct measurement approach, particularly concerns about adding cycles to the `pgstat_report_wait_start/end()` functions and the potential for measurement overhead to heavily distort results. The reviewer and another commenter strongly advocated for a sampling-based approach instead, noting that event counts are less useful than durations. The discussion is now exploring the complexities of implementing either self-sampling with timer interrupts or external sampling, focusing on how to minimize overhead and effectively attribute wait events to plan nodes.

Recent reply
Jul 6, 17:50 The reviewer discussed the complexities of implementing a sampling approach for wait event reporting. For self-sampling, the challenge is minimizing work in the interrupt handler due to the unbounded number of distinct wait events. For external sampling, the issue is how the external process identifies the active plan node or query, as direct memory pointers are not valid. Potential solutions include sharing `plan_node_id` or query strings, or using shared memory (DSA) or temporary files for data transfer back to the originating process.
archive ↗
48 [PATCH] Fix for bug #19474: LIKE fails to match literal backslashes with nondeterministic collations Committed 11 msgs Jul 7, 09:29
opened May 14, 11:12 ·last activity Jul 7, 09:29

This thread addresses Bug #19474, concerning `LIKE` operator failures to match literal backslashes with non-deterministic collations. The proposer identified an incorrect escaping mechanism as the root cause and submitted an initial patch. A reviewer suggested improving code clarity and robustness, leading the proposer to revise the patch to use an 'afterescape' flag and handle multibyte characters. A second reviewer confirmed the fix for both false-negative and false-positive cases. However, a core developer interjected, deeming the multibyte handling unnecessary and over-engineered for backend-safe encodings. Consequently, the core developer simplified and committed a patch that solely addressed the consecutive backslash issue, eliminating the extraneous multibyte logic.

3 recent replies
Jul 7, 08:52 The proposer, in the final message, acknowledged that a core developer had simplified and pushed the fix. The proposer expressed gratitude for the core developer's action, indicating that the bug has been resolved and the necessary code changes have been integrated into the project.
Jul 6, 18:55 The committer informs the proposer that they have already simplified the patch and pushed it. The simplification involved removing the logic for multibyte character checks, which the committer deemed unnecessary given the nature of PostgreSQL's backend-safe encodings. This message marks the resolution of the bug with a committed fix.
Jul 3, 21:33 The committer rejects the latest patch, stating it's not ready and introduces unnecessary complexity. The committer explains that the concern about multibyte characters matching '\' is a "nonexistent problem" due to backend-safe encoding. While acknowledging the core bug with consecutive backslashes, the committer believes the patch adds complexity to fix something that isn't broken, advocating for a simpler solution.
archive ↗
49 Add per-backend AIO statistics Proposed 1 msgs Jul 7, 11:28
opened Jul 7, 11:02 ·last activity Jul 7, 11:28

The proposer introduces a patch to enhance AIO (Asynchronous I/O) monitoring by adding per-backend statistics. Currently, AIO insights are limited to real-time handle usage or total I/O volume, lacking details on synchronous fallbacks, handle exhaustion, or how I/O completions are distributed across backends. The proposed patch introduces cumulative counters such as `started`, `executed_sync`, `executed_async`, `completed_self`, `completed_other`, `handle_waits`, and `submitted`. These statistics are designed to help users understand and tune AIO behavior by revealing metrics like the ratio of synchronous execution, instances of handle exhaustion, and patterns of cross-backend I/O completion. The data will be accessible through a new system function, `pg_stat_get_backend_aio()`, and integrates with existing backend statistics infrastructure with minimal performance overhead. The proposer suggests starting with a per-backend view before considering a global one.

Recent reply
Jul 7, 11:02 The proposer introduces a patch to add cumulative per-backend AIO statistics, including counters for started, synchronous/asynchronous execution, self/other completions, handle waits, and submitted operations. This new functionality, exposed via `pg_stat_get_backend_aio()`, aims to provide deeper insights into AIO behavior, help diagnose performance issues, and aid in tuning. It details the specific counters and their utility for analysis.
archive ↗
50 [PATCH] Limit PL/Perl scalar copies to work_mem Rejected 2 msgs Jul 7, 07:29
opened Jul 7, 01:56 ·last activity Jul 7, 07:29

This thread discusses a proposed patch to limit the size of PL/Perl scalar copies to prevent out-of-memory (OOM) killer invocations when large text values are returned. The proposer aimed to reject Perl strings larger than work_mem * 1024 bytes. However, a committer quickly rejected the patch, pointing out that memory overcommitment is a system-wide issue in PostgreSQL, work_mem is intended for memory-to-disk spilling rather than hard query failures, and PL/Perl can bypass such limits by allocating memory internally. The proposer acknowledged and accepted the feedback, concluding that the approach was inappropriate.

2 recent replies
Jul 7, 05:44 The latest reply from the proposer accepts the feedback from the committer. The proposer acknowledges that using work_mem for a hard failure is unsuitable and agrees that the problem of large memory allocations in PL/Perl is broader than what a simple boundary check can solve. The proposer commits to discussing ideas on the mailing list before submitting patches in the future.
Jul 7, 01:56 The reviewer's latest reply clarifies that the memory over-allocation issue is common across many PostgreSQL operations, not just PL/Perl, and suggests disabling memory overcommit as the general solution. The reviewer also states that the proposed use of `work_mem` as a hard limit for rejecting strings is inappropriate, as `work_mem` is typically for soft limits that trigger spill-to-disk rather than outright query failure. Therefore, the proposed approach is deemed unsuitable.
archive ↗
No threads match — try clearing filters.