Review/test: Add wait events for server logging destination writes (v4-0001)#45
Open
NikolayS wants to merge 66 commits into
Open
Review/test: Add wait events for server logging destination writes (v4-0001)#45NikolayS wants to merge 66 commits into
NikolayS wants to merge 66 commits into
Conversation
Owner
Author
|
Subject: Re: Add wait events for server logging destination writes Hi Seongjun, I tested v4-0001 on Linux and macOS. It applies and builds cleanly, and On Andrey's nesting concern: I checked the most likely case, One minor thing: a short comment noting that the write happens before I also exercised 0002 on Windows via CI (MSVC build, heavy logging into Nik |
Reported-by: Erik Rijkers <[email protected]> Discussion: https://www.postgresql.org/message-id/[email protected] Backpatch-through: 19
replace_property_refs() called expression_tree_mutator() with the root of the expression tree as the input node. But expression_tree_mutator() does not call the mutator function on the root node, so the root node remains unchanged. If the root node is a property reference or a lateral reference -- the two node kinds that replace_property_refs_mutator() rewrites -- it is returned unchanged. Modules after the rewriter do not know about property reference nodes, resulting in "ERROR: unrecognized node type: 63". Since varlevelsup of lateral references is not incremented, they are not resolved correctly in the planner, leading to many different symptoms. Fix this by calling replace_property_refs_mutator() directly from replace_property_refs(), similar to how other mutator functions do. The only case when a property reference or a lateral reference can be the root of a GRAPH_TABLE expression tree is when it is a bare property reference or a bare lateral reference in the WHERE clause. The COLUMNS clause is passed to replace_property_refs() as a targetlist. Every other expression has at least one expression node covering the property reference or a lateral reference in the expression tree. That explains why this bug was not seen so far. Author: Ashutosh Bapat <[email protected]> Reported-by: Noah Misch <[email protected]> Discussion: https://www.postgresql.org/message-id/flat/20260630173053.51.noahmisch%40microsoft.com
When a string literal is provided as a property expression, the data type of the property was set to "unknown", which may lead to various failures when the property is used in GRAPH_TABLE or when its data type is compared against other properties with the same name. To fix this, call resolveTargetListUnknowns() on the targetlist of new properties being added to resolve unknown type literals. Reported-by: Noah Misch <[email protected]> Author: Ashutosh Bapat <[email protected]> Discussion: https://www.postgresql.org/message-id/flat/20260630173053.51.noahmisch%40microsoft.com
The documentation previously said that pg_get_sequence_data() returns a row of NULL values if the sequence does not exist or if the current user lacks privileges on it. This was incomplete and could be misleading. A nonexistent relation name is rejected during regclass input conversion, while the function returns NULLs for a nonexistent relation OID and several other cases. This commit clarifies that the function returns NULLs when the specified relation OID does not exist, the relation is not a sequence, the current user lacks SELECT privilege on the sequence, the sequence belongs to another session's temporary schema, or it is an unlogged sequence on a standby server. Author: Amit Kapila <[email protected]> Reviewed-by: Fujii Masao <[email protected]> Discussion: https://postgr.es/m/CAA4eK1JOo0aJRhFHNWpj3hMwaTtNOopY34f1Lh_QD=z=+DrzWQ@mail.gmail.com Backpatch-through: 19
Sequence synchronization reports insufficient privileges on publisher and subscriber sequences, but the warnings do not indicate which role needs which privilege. This makes common configuration mistakes harder to diagnose. Add HINT messages for these warnings. Publisher-side warnings suggest granting SELECT to the role used for the replication connection. Subscriber-side warnings suggest granting UPDATE to the subscription owner when run_as_owner is enabled. Otherwise, the worker runs as the sequence owner, so no useful GRANT hint can be provided. Suggested-by : Amit Kapila <[email protected]> Author: Fujii Masao <[email protected]> Reviewed-by: Amit Kapila <[email protected]> Reviewed-by: Bharath Rupireddy <[email protected]> Discussion: https://postgr.es/m/CAA4eK1JOo0aJRhFHNWpj3hMwaTtNOopY34f1Lh_QD=z=+DrzWQ@mail.gmail.com Backpatch-through: 19
Effectively, a function to bitshift members by the specified number of bits. We have various fragments of code doing this manually with a bms_next_member() -> bms_add_member() loop. We can do this more efficiently in terms of CPU and memory allocation by making a new Bitmapset and bitshifting in the words of the old set to populate it. Author: David Rowley <[email protected]> Reviewed-by: Chao Li <[email protected]> Reviewed-by: Greg Burd <[email protected]> Discussion: https://postgr.es/m/CAApHDvq=eEdw2Qp+aSzSOtTSe+h0fnVQ55CcTNqBkLDYiRZmxw@mail.gmail.com
When pg_dump or pg_restore --jobs N is interrupted with Ctrl-C on Windows, we cancel all queries, but we don't want the cancellations to be reported as errors to the user in the short time before the whole process exits. That was previously achieved by calling TerminateThread() on each worker thread before sending the cancel message, but that doesn't appear to be 100% safe: the implementations of write() and the socket calls inside PQcancel() might acquire user space locks that were held by the terminated threads. (write() certainly does that.) Instead of silencing the threads in such a sketchy way this now sets a volatile flag before sending any cancel requests that tells the threads to not log errors anymore. (Instead of a volatile, it would be better to use an atomic operation here, but that has to wait until we add support for atomics on the frontend.) Note that this also stops using pg_fatal() and exit to exit() from workers on failure and instead use pg_log_error combined with exit_nicely. If a query fails in a worker we want it to kill the worker not the whole process. On Unix that's currently the same thing, but on Windows workers are threads. Author: Jelte Fennema-Nio <[email protected]> Reviewed-by: Bryan Green <[email protected]> Reviewed-by: Thomas Munro <[email protected]> Discussion: https://www.postgresql.org/message-id/[email protected]
This moves the responsibility of setting the ProcGlobal->walwriterProc and checkpointerProc fields into InitAuxiliaryProcess. Also switch to the same pattern to advertise the autovacuum launcher's ProcNumber, replacing the ad hoc av_launcherpid field in shared memory. This can easily be extended to other aux processes in the future, if other processes need to find them. Switch to pg_atomic_uint32 for the fields. Seems easier to reason about than volatile pointers. There was some precedence for that, as were already using pg_atomic_uint32 for the procArrayGroupFirst and clogGroupFirst fields, which also store ProcNumbers. Todo: We could also replace WalRecv->procno with this, but that's a little more code churn so I left that for the future. Reviewed-by: Andres Freund <[email protected]> Discussion: https://www.postgresql.org/message-id/[email protected]
When changing the expression of a generated column via ALTER TABLE ALTER COLUMN SET EXPRESSION, objects that depend on the column via indirect whole-row references (such as CHECK constraints, indexes) must be handled specially, because technically pg_depend does not contain such dependencies, see recordDependencyOnSingleRelExpr->find_expr_references_walker. This is a fix for commit f80bedd, "Allow ALTER COLUMN SET EXPRESSION on virtual columns with CHECK constraints". Author: jian he <[email protected]> Co-authored-by: Peter Eisentraut <[email protected]> Reported-by: Ayush Tiwari <[email protected]> Reviewed-by: Ayush Tiwari <[email protected]> Reviewed-by: solai v <[email protected]> Reviewed-by: Zsolt Parragi <[email protected]> Discussion: https://www.postgresql.org/message-id/flat/CAJTYsWXOkyeDVbzymWc9sKrq7Y_MUv6XJXN4H9GfsBOPd3NJ+w@mail.gmail.com
WAIT_FOR_WAL_FLUSH, WAIT_FOR_WAL_REPLAY, and WAIT_FOR_WAL_WRITE were placed in the WaitEventClient class. But WaitEventClient is about waiting for a socket to become readable or writable, while these events have other delay sources as well: local fsync and local replay, which may be disk- or CPU-bound. WaitEventIPC is a better fit, so move them there. Reported-by: Noah Misch <[email protected]> Discussion: https://postgr.es/m/[email protected] Backpatch-through: 19
When executing an UPDATE with a RETURNING clause on a table with a BEFORE UPDATE row trigger, the computation of the OLD values in the RETURNING list was incorrect if the target tuple was concurrently updated by another session, at isolation level READ COMMITTED. The problem was that the trigger code would lock the target tuple, waiting for the other session to commit, and then fetch the updated target tuple, but ExecUpdate() would not realise that the target tuple had changed, and use the outdated target tuple for computing OLD values. Fix by having ExecUpdate() check the TM_FailureData from trigger execution and re-fetch the target tuple if necessary. Re-fetching the target tuple like this is a little inefficient, but probably negligible compared to the trigger execution and update. A better long-term fix might be to move the EPQ code out of trigger.c, and let ExecUpdate() handle it, like ExecMergeMatched() does, but that would likely mean changing the trigger API, which seems a bit much for back-patching. Backpatch to v18, where support for RETURNING OLD/NEW was added. Bug: #19536 Reported-by: Jonas Boberg <[email protected]> Diagnosed-by: Bharath Rupireddy <[email protected]> Author: Dean Rasheed <[email protected]> Reviewed-by: Bharath Rupireddy <[email protected]> Discussion: https://postgr.es/m/[email protected] Backpatch-through: 18
refint was sample code from the pre-built-in-FK era and has long been documented as superseded by the built-in foreign key mechanism. Recent fixes (see commits b0b6196, 8cfbdf8, 260e977, 6117569, 1fbe206, and 1541d91) made it clear that the code has more issues than its sample-code value justifies. Author: Ayush Tiwari <[email protected]> Reviewed-by: solai v <[email protected]> Reviewed-by: Álvaro Herrera <[email protected]> Reviewed-by: Daniel Gustafsson <[email protected]> Reviewed-by: Fujii Masao <[email protected]> Discussion: https://postgr.es/m/CAJTYsWUHq8Ohc6-N-xamOPYz-q3qUYMtwQX-1%3DZi%3D5N1Q_GSEQ%40mail.gmail.com
The new partitions built for ALTER TABLE ... SPLIT PARTITION and ALTER TABLE ... MERGE PARTITIONS are created at the explicit request of the user, just like a plain CREATE TABLE. createPartitionTable() passes is_internal=true to heap_create_with_catalog(), while createTableConstraints() does the same to StoreAttrDefault() and AddRelationNewConstraints(). Pass is_internal=false in all these places instead, so that object-access hooks treat them as user-requested objects. The is_internal flag is intended for objects created as internal implementation details, such as a transient heap built during CLUSTER. While at it, pass 0 rather than PERFORM_DELETION_INTERNAL to the performDeletionCheck() calls that pre-check the drop eligibility of the old partitions, to match the subsequent performDeletion(). The flag has no functional effect on performDeletionCheck(), but change this for code consistency. Reported-by: Noah Misch <[email protected]> Discussion: https://postgr.es/m/[email protected] Backpatch-through: 19
I had previously made "seed" a uint64, as that's what pg_prng_seed() expects to get. However, GetCurrentTimestamp() and PG_GETARG_INT64() both return int64, so there was a mismatch in signedness. There are no bugs being fixed here, but it seems cleaner to make the variable int64 and cast to unsigned when passing to pg_prng_seed(). This means we no longer have to use the INT64_FORMAT specifier on the unsigned type to format the seed of the failing test correctly to allow a user to use the same seed from SQL when trying to recreate any failures manually. The most important thing to ensure is correct here is that users can specify the full range of seed values that could be automatically selected by GetCurrentTimestamp() so that we can recreate any failures from runs where the seed was auto-selected. This still works as expected when using the int64 type. In passing, adjust a comment that was a little misleading. Reported-by: Peter Eisentraut <[email protected]> Author: David Rowley <[email protected]> Discussion: https://postgr.es/m/[email protected]
Add pg_stat_progress_data_checksums to the progress reporting summary and the list of commands with progress reporting. Also clarify that the view reports both enabling and disabling data checksums, and correct the documented types of its progress counters to bigint. Author: Fujii Masao <[email protected]> Reviewed-by: Daniel Gustafsson <[email protected]> Discussion: https://postgr.es/m/CAHGQGwHJHJYAkYZBi3_O13np-Rou9UL637=hB3Y_-qdCgcZn-w@mail.gmail.com Backpatch-through: 19
We've long had enable_hashagg to discourage hashed aggregation, but there was no equivalent for grouping that works by reading presorted input. That's handy when investigating planner cost misestimates, and as an escape hatch when a sorted grouping plan is chosen over a much cheaper hashed one. enable_groupagg (on by default) covers the GroupAggregate and Group nodes, the sort-based Unique step used for DISTINCT and semijoin unique-ification, and the sorted mode of SetOp. It isn't a hard switch; it only bumps disabled_nodes, so plans with no other choice are still produced. Several regression and module tests had been setting enable_sort off, and one enable_indexscan off, only to force a hashed plan. Use enable_groupagg there instead, where that was the real intent. In union.sql this also lets us test the hashed UNION path, which we had no way to reach before. Author: Tatsuro Yamada <[email protected]> Co-authored-by: Richard Guo <[email protected]> Reviewed-by: Ashutosh Bapat <[email protected]> Reviewed-by: David Rowley <[email protected]> Discussion: https://postgr.es/m/CAOKkKFvYHSEsFazkrf9bRH14p-H27XMaqbZfRYjS6EHBruvZMQ@mail.gmail.com
Explain more accurately how REFRESH SEQUENCES differs from REFRESH PUBLICATION in ALTER SUBSCRIPTION, and note that CREATE SUBSCRIPTION uses copy_data = true (the default) to copy initial sequence values. Author: Peter Smith <[email protected]> Author: Amit Kapila <[email protected]> Backpatch-through: 19 Discussion: https://postgr.es/m/CAHut+PtFkGvZNihGRDoghWNKMfJufEpR9+thbG_8qPQ7RyVN4w@mail.gmail.com
pg_restore_attribute_stats() and pg_restore_extended_stats() (expressions) can handle a STATISTIC_KIND_BOUNDS_HISTOGRAM value, but did not check its shape when importing, especially regarding: - Empty ranges. - Unsorted elements. These properties are respected by ANALYZE in compute_range_stats(), when computing a histogram for a [multi]range, and by the planner when reading the data from the stats catalogs. The effects of importing data with these properties depend on the compilation options: - A assertion would be hit, if enabled. - A non-sensical value that would make the load of the stats fail. - A failure is hit if an empty range is loaded. While the owner of the table or the one with MAINTAIN rights is responsible for the stats data inserted, buggy data makes little sense if their load is going to fail. This commit adds a validation step to match what compute_range_stats() expects. This issue would be unlikely hit in practice, so no backpatch is done. Author: Ewan Young <[email protected]> Discussion: https://postgr.es/m/CAON2xHNM809WLR_g0ymKgU-tWxtbyH1Xvh4fqzRqy9YP2A59pg@mail.gmail.com
In transformLockingClause(), there was a comment that listed all the RTE kinds it did not want to process, but that list was already outdated about what RTE kinds actually exist. Rather than keeping that up-to-date, just say "all other". Discussion: https://www.postgresql.org/message-id/flat/CAHg%2BQDcE9wp6nOEC3SCRQ90nrCO%3DQF%2BOZq1MG8Qc6hnusmogqw%40mail.gmail.com
Specifying a locking clause (FOR UPDATE/SHARE) that names a GRAPH_TABLE alias currently results in an unhelpful "unrecognized RTE type: 8" error. This commit explicitly prohibits specifying a locking clause on a GRAPH_TABLE alias, raising a more user-friendly error instead. (Locking clause support for GRAPH_TABLE could be added as a separate feature in the future.) Author: SATYANARAYANA NARLAPURAM <[email protected]> Author: Ayush Tiwari <[email protected]> Reviewed-by: Ashutosh Bapat <[email protected]> Discussion: https://www.postgresql.org/message-id/flat/CAHg%2BQDcE9wp6nOEC3SCRQ90nrCO%3DQF%2BOZq1MG8Qc6hnusmogqw%40mail.gmail.com
This patch adds the PLAN clauses for JSON_TABLE, which allow the user to specify how data from nested paths are joined, allowing considerable freedom in shaping the tabular output of JSON_TABLE. PLAN DEFAULT allows the user to specify the global strategies when dealing with sibling or child nested paths. This is often sufficient to achieve the necessary goal, and is considerably simpler than the full PLAN clause, which allows the user to specify the strategy to be used for each named nested path. Path names may be attached to the row pattern and to each NESTED path using AS. Unlike the SQL/JSON standard, which requires a name for every NESTED path when a PLAN clause is present, PostgreSQL does not require them and generates a name for any path left unnamed; a specific PLAN() can only reference paths by name, so unnamed paths it must mention are reported as not covered by the plan. Author: Nikita Malakhov <[email protected]> Co-authored-by: Nikita Glukhov <[email protected]> Co-authored-by: Teodor Sigaev <[email protected]> Co-authored-by: Oleg Bartunov <[email protected]> Co-authored-by: Alexander Korotkov <[email protected]> Co-authored-by: Andrew Dunstan <[email protected]> Co-authored-by: Amit Langote <[email protected]> Co-authored-by: Anton Melnikov <[email protected]> Reviewed-by: Andres Freund <[email protected]> Reviewed-by: Pavel Stehule <[email protected]> Reviewed-by: Andrew Alsup <[email protected]> Reviewed-by: Erik Rijkers <[email protected]> Reviewed-by: Zhihong Yu <[email protected]> Reviewed-by: Himanshu Upadhyaya <[email protected]> Reviewed-by: Daniel Gustafsson <[email protected]> Reviewed-by: Justin Pryzby <[email protected]> Reviewed-by: Álvaro Herrera <[email protected]> Reviewed-by: jian he <[email protected]> Reviewed-by: Vladlen Popolitov <[email protected]> Discussion: https://postgr.es/m/[email protected] Discussion: https://postgr.es/m/[email protected] Discussion: https://postgr.es/m/abd9b83b-aa66-f230-3d6d-734817f0995d%40postgresql.org Discussion: https://postgr.es/m/CA+HiwqE4XTdfb1nW=Ojoy_tQSRhYt-q_kb6i5d4xcKyrLC1Nbg@mail.gmail.com Discussion: https://postgr.es/m/CAN-LCVP7HXmGu-WcinsHvdKqMGEdv=1Y67H4U58F6Y=Q0M5GyQ@mail.gmail.com
Autovacuum launcher is not an aux process, so it needs to be advertised in InitProcess() rather than InitAuxiliaryProcess() In the passing, also remove some now-unnecessary 'volatile' qualifiers left behind by the same commit. Reported-by: Thom Brown <[email protected]> Author: Fujii Masao <[email protected]> Author: Nathan Bossart <[email protected]> Discussion: https://www.postgresql.org/message-id/CAA-aLv6UHXubNxmAjNH8PnyKKkaXePO1ggB15=iidNW08T4hSg@mail.gmail.com
When compiling against OpenSSL, the <limits.h> header is indirectly included via openssl/ossl_typ.h from openssl/conf.h, but the LibreSSL version of ossl_typ.h does not include <limits.h> which cause compiler failure due to missing symbol (since ffd080d). Fix by explicitly including <limits.h>. Author: Daniel Gustafsson <[email protected]> Discussion: https://www.postgresql.org/message-id/[email protected] Backpatch-through: 14
Check explicitly for pqsecure_read() returning an error. It shouldn't fail, and we would've caught it in the check for a short read, but better to be explicit so that the error message is more informative. We also shouldn't update 'inEnd' when the read fails, although that too is just pro forma as we will bail out and close the connection on error. Reported-by: Peter Eisentraut <[email protected]> Discussion: https://www.postgresql.org/message-id/[email protected] Backpatch-through: 14
The spinlock is unnecessary because the counter is only ever accessed with WaitEventCustomLock held exclusively. This commit removes the struct definition and converts WaitEventCustomCounter to a pointer to an integer in shared memory. Reviewed-by: Michael Paquier <[email protected]> Reviewed-by: Fujii Masao <[email protected]> Discussion: https://postgr.es/m/ak8MeTS9QBmz6DKt%40nathan
86ab7f4 misses catversion bump as it adds new fields to the existing nodes. Reported-by: Thom Brown <[email protected]> Discussion: https://postgr.es/m/CAA-aLv7aZGSExnbjJRw8eKkoXbu34TdoKLLA2gPye3aHjO5OSA%40mail.gmail.com
log_statement_max_length limits the statement text logged by log_statement and duration logging. However, the prepared statement shown in the DETAIL message for EXECUTE was still logged in full, allowing very large prepared queries to bypass the limit. Apply the same truncation to the prepared statement text in errdetail_execute(), making the setting behave consistently across the main log message and its associated DETAIL output. Also append an ellipsis to truncated statement text so users can easily tell when truncation has occurred. With this change, a limit of zero logs only the ellipsis, indicating that the entire statement text was truncated. Finally, avoid scanning the entire query string just to determine whether truncation is needed. Use strnlen() with sufficient lookahead for multibyte character handling, then use pg_mbcliplen() to ensure truncation never splits a multibyte character. Author: Jim Jones <[email protected]> Reviewed-by: Fujii Masao <[email protected]> Discussion: https://postgr.es/m/CAHGQGwFOV+7nOdfoO=kfVH=-fRA9aQE1YcHHLYty3nfQ9rQ4RA@mail.gmail.com
truncate_query_log() returns NULL when statement truncation is disabled or the supplied query does not need to be truncated. Therefore, callers do not need to check log_statement_max_length before calling it. Remove the redundant checks and let truncate_query_log() make the decision in one place. This simplifies the statement and duration logging code without changing its behavior. Author: Jim Jones <[email protected]> Reviewed-by: Fujii Masao <[email protected]> Discussion: https://postgr.es/m/CAHGQGwFOV+7nOdfoO=kfVH=-fRA9aQE1YcHHLYty3nfQ9rQ4RA@mail.gmail.com
Previously, this function imported remote statistics by executing SQL functions like pg_restore_relation_stats and pg_restore_attribute_stats via SPI (in read-write mode). As the SQL functions take a schema name and a relation name as two separate arguments, rather than a single OID argument, if the containing schema was concurrently renamed, the callback function would throw an error like this: ERROR: schema "foo" does not exist To fix, 1) provide new interface functions to import remote statistics that are directly callable from FDWs and take a single OID, and 2) modify the callback function to use the interface functions instead when importing remote statistics. For #1, this commit does a bit of refactoring to relation_statistics_update and attribute_statistics_update, which are the workhorse functions for pg_restore_relation_stats and pg_restore_attribute_stats respectively: since they also take a schema name and a relation name, separate the guts of them into new functions so that they take a single OID and are callable not only from the workhorse functions but from the interface functions introduced by #1. Oversight in commit 28972b6. Reported-by: Robert Haas <[email protected]> Suggested-by: Robert Haas <[email protected]> Author: Corey Huinker <[email protected]> Co-authored-by: Etsuro Fujita <[email protected]> Discussion: https://postgr.es/m/CA%2BTgmoYqMtWb4zLUkT98oFnEkJ%3DWz0Pw-ggDJrp9wnSXPzUaeQ%40mail.gmail.com Backpatch-through: 19
The pg_attribute_always_inline macro name is so long it forces pgindent to format the code in strange ways. Which may incentivize patch authors to either structure the code in strange ways (e.g. reorder prototypes), use shorter names, etc. Neither is very desirable for code readability. This shortens the name by removing the _attribute_ part. It also makes it more consistent with pg_noinline, which does not have the _attribute_ part either. Backpatched to all supported branches, to prevent conflicts when backpatching other fixes. The backbranches however keep both the old and new macro name, so that existing code keeps working. Author: Andres Freund <[email protected]> Reviewed-by: Peter Geoghegan <[email protected]> Reviewed-by: Tomas Vondra <[email protected]> Discussion: https://postgr.es/m/bqqdehahpoa36igpictuqyn2s2mexk3t3ehidh2ffd2slb35e5@rzgksuiszgbg Backpatch-through: 14
…gment This test is able to trigger the following failure at the beginning of recovery, that was not covered yet: FATAL: could not locate required checkpoint record at %X/%X Note that the backup used for the node created has its pg_wal/ removed, which is why the segment expected is missing. Extracted from a larger patch by the same author. Author: Nitin Jadhav <[email protected]> Discussion: https://postgr.es/m/CAMm1aWZ9Tv=Wrx52_2Ppw+6ULf_twRZuQm=ZWLA_a-kXWykHkQ@mail.gmail.com
A table published both explicitly and through its schema (FOR TABLES IN SCHEMA) was listed twice by \dRp+ and \d. Since publishing a schema publishes its tables in full, the explicit entry's row filter has no effect; suppress it when the same publication also publishes the table's schema. Author: Peter Smith <[email protected]> Co-author: Jim Jones <[email protected]> Reviewed-by: Nisha Moond <[email protected]> Reviewed-by: vignesh C <[email protected]> Reviewed-by: Shlok Kyal <[email protected]> Discussion: https://postgr.es/m/CAHut%2BPvSOmRrQX%2BVrFYHtFipV9hM%3Dp99FeOwYCzkuU2BOaLu7Q%40mail.gmail.com
These are some cases in the vicinity of the patches to improve the size_t/ssize_t use with POSIX file system APIs. For example, the input buffers for write operations or the file names passed for error reporting can be const. Reviewed-by: Heikki Linnakangas <[email protected]> Discussion: https://www.postgresql.org/message-id/flat/[email protected]
It needs to be much earlier in win32_port.h so that declarations in that file can also use it. To be used by a subsequent patch. Reviewed-by: Heikki Linnakangas <[email protected]> Discussion: https://www.postgresql.org/message-id/flat/[email protected]
The return type of readlink() per POSIX is ssize_t, but most existing callers use int, so fix that. Also fix the return type of the Windows implementation to match. In _pglstat64(), we neglected to handle the case where the output buffer is not large enough and the result would be truncated. This case actually can't happen, because the Windows pgreadlink() implementation doesn't ever return that case, but adding this seems good for consistency with _pgstat64(), which already had this check, and in case pgreadlink() ever changes in this regard. Some callers of readlink(), in particular _pglstat64(), assume that errno == EINVAL means that the file was not a symlink. But Windows pgreadlink() also sets EINVAL in other cases, in particular if the buffer was too small. This could result in incorrect behavior, so pick a different errno. (There might be other cases where EINVAL is set inappropriately, but they are outside the theme of this patch.) Reviewed-by: Heikki Linnakangas <[email protected]> Discussion: https://www.postgresql.org/message-id/flat/[email protected]
The return type is ssize_t, not int. Fix that in one instance; the others were already okay. Reviewed-by: Heikki Linnakangas <[email protected]> Discussion: https://www.postgresql.org/message-id/flat/[email protected]
It was using int for the input and output length, where size_t or ssize_t would be more appropriate. It might not matter in practice, but it makes the APIs consistent at all levels. Reviewed-by: Heikki Linnakangas <[email protected]> Discussion: https://www.postgresql.org/message-id/flat/[email protected]
This commit adds a test case for early startup where a backup_label file uses a checkpoint LSN and a redo LSN located in two different segments, where the segment of the redo LSN is missing. This code has never been covered, and is complex enough that a test case is going to be useful in the long-term. Nitin has proposed a more complex approach than what is added by this commit, by forcing a reuse of the injection points to produce a split between redo and checkpoint. This commit relies on the checkpoint and redo LSNs generated by the first steps of the test, combined with a generated backup_label, making the whole cheaper. Author: Nitin Jadhav <[email protected]> Author: Michael Paquier <[email protected]> Discussion: https://postgr.es/m/CAMm1aWZ9Tv=Wrx52_2Ppw+6ULf_twRZuQm=ZWLA_a-kXWykHkQ@mail.gmail.com
_Generic is the C11 standard equivalent (and superset) of __builtin_types_compatible_p, so by replacing the latter with the former, we can now rely on this working on all compilers, instead of previously just with GCC-compatible ones. And we can drop a configure test. This affects StaticAssertVariableIsOfType() and StaticAssertVariableIsOfTypeMacro() and indirectly unconstify() and unvolatize(). Neither _Generic nor __builtin_types_compatible_p works in C++, so this does not change that, but this adds an explicit code comment about that. There are some subtle behavior changes, but these do not affect cases that are in use or likely to be useful. _Generic does lvalue conversion on the controlling expression, which means it drops top-level qualifiers and converts arrays to pointers. __builtin_types_compatible_p on the other hand, supports arrays and just ignores qualifiers. So before, StaticAssertVariableIsOfType(x, const int) might have worked, but now it does not. (But note that it would previously have succeeded even if x was a non-const int, so this usage would always have been dubious.) Also, StaticAssertVariableIsOfType(y, char[]) would have worked, but now you need to write char *. (But this is not backward compatible, because char * would previously not have succeeded.) Similarly, unconstify of non-pointers, like unconstify(const int, x) would previously have worked, but this was just by accident and never useful (you can just assign directly, without a cast), and the C++ implementation rejects non-pointers anyway. Some comments are added to explain this a bit. There are no current uses affected by this. Note that even though we have required C11 since f5e0186, we have not made use of _Generic until now (except in an MSVC-specific case in commit 59c2f03). This now raises the effective compiler requirement on the trailing edge slightly from GCC 4.8 to GCC 4.9. This in turn means that we effectively drop support for RHEL 7. Author: Peter Eisentraut <[email protected]> Co-authored-by: Thomas Munro <[email protected]> Co-authored-by: Jelte Fennema-Nio <[email protected]> Discussion: https://www.postgresql.org/message-id/flat/CA+hUKGL7trhWiJ4qxpksBztMMTWDyPnP1QN+Lq341V7QL775DA@mail.gmail.com
86ab7f4 commit made the table-level ON ERROR clause serve as the default ON ERROR for columns lacking their own, so that a top-level ERROR ON ERROR turned per-column evaluation errors into hard errors. The SQL standard does mandate this cascade, but introducing it should be a deliberate, separately-documented change, so restore the previous behavior for now. This also reverts the paired ruleutils.c logic that deparsed a column's behavior against an ERROR default: that dropped an explicit ERROR ON EMPTY from a dumped view, and otherwise emitted a redundant NULL ON EMPTY. Reported-by: Thom Brown <[email protected]> Discussion: https://postgr.es/m/CAA-aLv7aZGSExnbjJRw8eKkoXbu34TdoKLLA2gPye3aHjO5OSA@mail.gmail.com
get_json_table_plan() parenthesized the child of a parent/child (OUTER/INNER) plan only when that child was a sibling (UNION/CROSS) join, not when it was itself a parent/child join. A plan such as PLAN (p0 OUTER (p1 INNER p11)) was therefore deparsed as PLAN (p0 OUTER p1 INNER p11), which does not parse back to the same plan tree -- a dump/restore hazard. Parenthesize the child whenever it is not a bare path name, matching the logic already used for the operands of sibling joins. Reported-by: Thom Brown <[email protected]> Discussion: https://postgr.es/m/CAA-aLv7aZGSExnbjJRw8eKkoXbu34TdoKLLA2gPye3aHjO5OSA@mail.gmail.com
generateJsonTablePathName() produced names of the form
"json_table_path_N" in the same namespace as user-supplied path and
column names, without checking whether the name was already in use.
When an unnamed NESTED path's generated name happened to match a
user-supplied path name that a PLAN clause referenced, two sibling paths
matched the same plan entry and one of them, together with its columns,
was silently dropped from the output.
Bump the counter until the generated name is unused, so a generated name
can no longer coincide with a user-supplied one. The still-uncovered
path is then correctly reported as not found in the plan.
The row pattern (root) path is named before the user-supplied column and
path names are collected, so when it is left unnamed its generated name
could not avoid them either, and a user column or path named like a
generated name, e.g.
SELECT * FROM JSON_TABLE(jsonb '1', '$'
COLUMNS (json_table_path_0 int PATH '$')) jt;
was rejected with a bogus "duplicate JSON_TABLE column or path name"
error. Collect the user-supplied names first and generate the row
pattern path's name afterwards, so that it avoids all of them. An
explicit root path name is still seeded into the namespace, so a column
duplicating it is still correctly rejected.
Reported-by: Thom Brown <[email protected]>
Discussion: https://postgr.es/m/CAA-aLv7aZGSExnbjJRw8eKkoXbu34TdoKLLA2gPye3aHjO5OSA@mail.gmail.com
Discussion: https://postgr.es/m/CAA-aLv5U94KD4C%2BLhAPYcCeGvs1xBMngcS5oEkZHN9YWwXUHsA%40mail.gmail.com
Correct the JSON_TABLE synopsis to place the ON ERROR clause after the PLAN clause, matching the grammar (the previous order could not be typed). Replace a dangling reference to json_path_specification with path_expression, the term the synopsis actually defines. Mark up the PLAN DEFAULT keywords with <literal> and fix a couple of wording issues. Also list OUTER before INNER consistently, including in the parent/child join description, where OUTER is the default. Author: Thom Brown <[email protected]> Discussion: https://postgr.es/m/CAA-aLv5PGAFmAEpbhKQz8wppoOOTHfo-=LJb2sAB3974-9QtOw@mail.gmail.com
86ab7f4 makes JSON_TABLE with a NESTED PATH re-ran the nested path's jsonpath expression several times for each parent row. That makes such queries significantly slower as the nested arrays grew, even without a PLAN clause. Two leftovers from the plan/join executor rework were responsible. JsonTablePlanScanNextRow() still reset and advanced the nested plan itself, although JsonTablePlanNextRow() now does that; and JsonTableResetNestedPlan() eagerly called JsonTableResetRowPattern() (which evaluates the path) in addition to setting the reset flag that makes JsonTablePlanNextRow() evaluate it again. Together these caused the nested path to be evaluated multiple times per parent row. Reduce JsonTablePlanScanNextRow() to advancing its own row pattern iterator, and have JsonTableResetNestedPlan() only reset the transient scan state (so a not-yet-advanced sibling still reads as NULL) while deferring the actual path evaluation to the reset flag. The nested path is now evaluated exactly once per parent row, as before the PLAN clause feature; results are unchanged and are covered by the existing tests. Reported-by: Thom Brown <[email protected]> Discussion: https://postgr.es/m/CAA-aLv5U94KD4C%2BLhAPYcCeGvs1xBMngcS5oEkZHN9YWwXUHsA%40mail.gmail.com
transformJsonTableNestedColumns() looked up the nested column named by a JSON_TABLE PLAN node and raised "invalid JSON_TABLE plan clause / PATH name was %s not found in nested columns list" if none was found. That lookup cannot fail: validateJsonTableChildPlan() runs first, at every plan level, and already matches the plan's sibling path names one-to-one against the nested columns (reporting any uncovered nested path or any extra or duplicate sibling node). The check has been unreachable since the PLAN clause code was first written. Replace it with an Assert documenting the invariant, which also removes a user-facing message that could never be emitted. Reported-by: Thom Brown <[email protected]> Discussion: https://postgr.es/m/CAA-aLv5_9%3DzgA_Y7aoFp-%2BQSeh0kx4dfbAas9Wx%3DyrweQSqa6Q%40mail.gmail.com
Commit 62c3b4c taught postgres_fdw to push down ArrayCoerceExpr, but foreign_expr_walker() only recursed into the input array expression and never examined elemexpr, the per-element conversion that gives the coercion its semantics. deparseArrayCoerceExpr() then shipped a bare "arg::resulttype" cast, or nothing at all for an implicit-format coercion, leaving the remote server to re-resolve the element conversion against its own catalogs and session state. This produced wrong results or remote errors whenever the element conversion was not a plain relabeling, and it was inconsistent with how postgres_fdw treats the equivalent scalar coercions. An ArrayCoerceExpr was shipped even when its elemexpr was a cast function (whose shippability was never checked), a CoerceViaIO (e.g. float8out or byteaout, which depend on extra_float_digits / bytea_output that postgres_fdw sets differently on the remote session), or a CoerceToDomain (which pushes domain enforcement to the remote catalog). By contrast, a scalar CoerceViaIO is never shipped, and a scalar cast function is shipped only when it is shippable. Restrict pushdown to element coercions that are a plain relabeling, that is, elemexpr is a RelabelType or a bare CaseTestExpr. Any other element coercion is now evaluated locally. This keeps the common binary-coercible case pushed down, including "col = ANY($1)" with a varchar[]-to-text[] relabeling, which is the case 62c3b4c set out to optimize. Pushing down shippable element cast functions, to reach parity with the scalar case, is left out here for simplicity. Reported-by: Noah Misch <[email protected]> Discussion: https://postgr.es/m/20260711024234.43.noahmisch%40microsoft.com Backpatch-through: 19
This reverts commit ed823da, that has made pgstat_write_chunk() and pgstat_read_chunk() available for public use. These routines do not have a symmetric API definition across reads and writes, with the write part returning a void status, deferring an error detection once all the stats entries have been processed with an ferror(), and the read part returning a boolean status. These routines are just tiny wrappers around fread() and fwrite(), and extensions can just define they own routines instead of relying on the same facilities as the core pgstat.c. This commit removes their declaration from the public headers, to reduce the confusion. test_custom_stats is updated to use its own read/write routines. Perhaps something better could be designed in the future; trying to do so for v19 is not feasable during beta. Reported-by: Peter Eisentraut <[email protected]> Author: Sami Imseih <[email protected]> Discussion: https://postgr.es/m/[email protected] Backpatch-through: 19
Commit 9a60f29 stripped the stale PlaceHolderVars left behind by left-join removal from the surviving rels' baserestrictinfo and from EquivalenceClass member expressions, but it overlooked join clauses. A PlaceHolderVar embedded in a join clause can likewise retain the removed rel and join in its phrels, since remove_rel_from_query() fixes up the RestrictInfo's own relid sets but not the PHVs inside its expression. As before, this is normally harmless, because later processing consults those relid sets rather than the embedded PHVs. However, a restriction clause derived from such an OR join clause inherits the stale PlaceHolderVar, and when the derived clause is translated for an appendrel child, pull_varnos() recomputes its relids and folds the removed relation back in. The rebuilt clause then references a no-longer-existent relation, tripping an assertion during path generation. Fix by also stripping the removed relation from the PlaceHolderVars in the surviving rels' join clauses, including the sub-clauses of any OR clause. Like 9a60f29, this is only reachable on v18 and later, where match_index_to_operand() began ignoring PlaceHolderVars. Author: Arne Roland <[email protected]> Reviewed-by: Tender Wang <[email protected]> Reviewed-by: Richard Guo <[email protected]> Discussion: https://postgr.es/m/[email protected] Backpatch-through: 18
equalPolicy() is used in the relation cache to check if two policy definitions are equivalent, but missed to check for polpermissive. ALTER POLICY cannot switch a policy to be PERMISSIVE or RESTRICTIVE, so this would need a dropped and then re-created policy, which would trigger a relcache invalidation. Anyway, there is no harm in being consistent in the check, and if one decides to add an ALTER POLICY to switch PERMISSIVE or RESTRICTIVE, we would be silently in trouble. Author: Andreas Lind <[email protected]> Reviewed-by: Laurenz Albe <[email protected]> Discussion: https://postgr.es/m/CAMxA3rv1CS6R7JR5ojz-3CmCEnZEFrqu+XXTnGbLRWrjJRH7sA@mail.gmail.com Backpatch-through: 14
Contrary to the from_serialized_data callback used by the pgstats reads at startup, the to_serialized_data callback used for the pgstats writes matched with pgstat_write_statsfile(), by not returning a boolean status, expecting a ferror() failure to deal with the discard of the stats file should an error happen while writing the stats. This was slightly confusing designed this way. Things are changed in this commit with: - to_serialized_data now returns a boolean status on a write failure. pgstat_write_statsfile() detects that and switches to failure mode instead of continuing to process the entries to write, speeding up the shutdown. - pgstat_write_statsfile() now uses STATS_DISCARD if a failure happens, to let the registered callbacks directly know that something is wrong, and that things need to be cleaned up. This gives a better error path detection for custom stats kinds. For example, they do not have to rely solely on the expectation of an ferror() for an auxiliary file. This new set of behaviors matches with what is already done in pgstat_read_statsfile() for the finish() callback (DISCARD on failure, READ on success) and the from_serialized_data with a status returned. Author: Sami Imseih <[email protected]> Discussion: https://postgr.es/m/CAA5RZ0sMgOvuhpb2P=KSJOjgjC6AfUu+GYcu9mHar-y_Xtd=Pg@mail.gmail.com Backpatch-through: 19
The return type is ssize_t, not int, but some callers didn't handle this properly. The BIO callbacks are constrained by the OpenSSL API, so they take int for the length and return int. This is safe, since the return value can't be greater than the input length. To make it more clear that this is intentional, cast the result of the secure_read()/secure_write() call to int explicitly. Reviewed-by: Heikki Linnakangas <[email protected]> Discussion: https://www.postgresql.org/message-id/flat/[email protected]
and analogously for pg_pread() and FileRead() Be sure to store the return value in a variable of type ssize_t, not int. Also make the error messages for short reads consistent. They should always be like "read %zd of %zu". Appearance of other placeholders indicates the types are probably wrong (although in some cases some casts are added to make macros have the right type and keep the strings consistent, and it some cases it's left as "%zu of %zu", which is close enough). In several cases, the input length is derived from struct stat st_size, which has type off_t, which is neither size_t nor ssize_t. To keep the type handling clearer, this introduces intermediate variables in these cases. In SendTimeLineHistory() in walsender.c, we need to adjust the logic a bit to over underflow wrap if we end up reading more from the file than expected. This is believed to be a theoretical problem only. Alternatively, we could treat this as an error. Note that the previous code would have processed the extra data but only up to a full block, which seems wrong in any case. Reviewed-by: Heikki Linnakangas <[email protected]> Discussion: https://www.postgresql.org/message-id/flat/[email protected]
and analogously for pg_pwrite() and FileWrite() Be sure to store the return value in a variable of type ssize_t, not int. Some callers of FileWrite() did not have a separate error message for a short write. This is okay in practice because FileWriteV() sets ENOSPC for all non-error returns, so you'll get a reasonable error message either way. But callers handled this inconsistently, and this behavior isn't really prominently documented and commit 871fe49 seems to frown upon it, so it seems better to make all callers handle this consistently by adding the separate error message. Reviewed-by: Heikki Linnakangas <[email protected]> Discussion: https://www.postgresql.org/message-id/flat/[email protected]
The result from pg_preadv() or pg_pwritev(), which is of type ssize_t, is assigned to PgAioHandle.result, which is of type int. This should be ok because the maximum result is limited by PG_IOV_MAX times BLCKSZ. Add an assertion and a code comment to explain and check this. Reviewed-by: Heikki Linnakangas <[email protected]> Discussion: https://www.postgresql.org/message-id/flat/[email protected]
'ALTER SUBSCRIPTION ... REFRESH SEQUENCES' can race with an already running sequence synchronization worker. If a second refresh request resets the synchronization state while the worker has already fetched sequence values from the publisher but has not yet applied them to the subscriber, the worker can overwrite the subscriber with stale values and mark the synchronization as complete. Avoid this race by rejecting 'ALTER SUBSCRIPTION ... REFRESH SEQUENCES' when a sequence synchronization worker is already running for the subscription. The command reports an error asking the user to rerun it after the current synchronization completes. Also add a wait for the re-added 'regress_s4' sequence to finish synchronizing in 036_sequences.pl, so the subsequent test does not race against its sequencesync worker. Reported-by: Noah Misch <[email protected]> Author: vignesh C <[email protected]> Reviewed-by: Shveta Malik <[email protected]> Reviewed-by: Hayato Kuroda <[email protected]> Reviewed-by: Amit Kapila <[email protected]> Backpatch-through: 19, where it was introduced Discussion: https://postgr.es/m/[email protected]
pg_clear_attribute_stats() checks its required arguments manually
because the function is not strict. Previously, when schemaname or
relname was passed as NULL, the error incorrectly reported the
argument name as "relation" in both cases:
ERROR: argument "relation" must not be null
This was misleading, especially for schemaname, and inconsistent with
the function's SQL-visible argument names.
The cause is that cleararginfo[] in attribute_stats.c used
"relation" for both the schema-name and relation-name arguments.
This commit fixes the issue by using "schemaname" and "relname" instead,
matching the function's declared argument names so that the error reports
the correct argument name.
Backpatch to v18, where pg_clear_attribute_stats() was introduced.
Author: Ilia Evdokimov <[email protected]>
Reviewed-by: Fujii Masao <[email protected]>
Discussion: https://postgr.es/m/[email protected]
Backpatch-through: 18
When a backend writes server log output, the underlying call can
block: write(2) to the syslogger pipe or to stderr once the pipe
buffer fills up or the output device is slow, and syslog(3) when the
system logger is slow. These blocking calls were not instrumented, so
pg_stat_activity reported wait_event IS NULL during that time. Many
monitoring tools interpret NULL as on-CPU work, which made
heavy-logging stalls hard to attribute.
Add three new WaitEventIO events and report them around the relevant
calls:
IO / SysloggerWrite - write(2) to the syslogger pipe inside
write_pipe_chunks().
IO / StderrWrite - write(2) to stderr inside write_console().
IO / SyslogWrite - syslog(3) inside write_syslog().
The instrumentation is limited to the leaf write/syslog call. It uses
only the existing pgstat_report_wait_start()/end() inline helpers,
which are allocation-free and safe to call before MyProc is set up, so
this remains safe to invoke from within error reporting paths.
The call that actually blocks on a slow log device is
write_syslogger_file() in the syslogger process, which has no PGPROC
and so never appears in pg_stat_activity. Backends surface the stall
as the pipe fills and they block on the pipe write, so the
backend-side SysloggerWrite event is where it becomes visible.
Because wait events are single-slot (pgstat_report_wait_end() simply
writes 0), a log message emitted while an outer wait event is set will
briefly mask that event. This is a pre-existing property of the
mechanism rather than something specific to these events; a comment at
each wrapped site notes it.
NikolayS
force-pushed
the
review/logging-dest-wait-events
branch
from
July 15, 2026 14:52
73647c3 to
7bce6fb
Compare
Owner
Author
|
Updated to v5-0001 (posted to -hackers June 30), rebased onto current master (
Windows runtime re-check against v5 is running: https://github.com/NikolayS/postgres/actions/runs/29425495163 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
14 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What this is
Patch v4-0001 from Seongjun Shin's thread "Add wait events for server
logging destination writes", applied on top of current upstream
masterforreview and testing. Authorship is preserved (
git am).0002 (
WriteConsoleW+EventlogWrite) is not included here.The text below is my review, recorded here and intended to be sent to
pgsql-hackers.
Review
Hi Seongjun,
I reviewed and tested v4-0001 (the portable part). Short version: it applies
cleanly, builds without new warnings, and does exactly what it claims — log
write stalls that previously showed
wait_event IS NULL("on CPU") are nowattributable in
pg_stat_activity. +1 on the approach.Test setup
master(assert-enabled build); the patch also rebasescleanly onto the current tip.
Windows box for 0002).
RAISE LOGlines, while a server-side sampler snapshottedpg_stat_activityevery ~2 ms (~5000 samples).Results
logging_collector=onwrite_pipe_chunks()-> syslogger pipeIO / SysloggerWrite(33,575 samples)logging_collector=offwrite_console()-> stderrIO / StderrWrite(34,206 samples)On unpatched master the same workload shows
wait_event IS NULLthroughout.I did not exercise
IO / SyslogWrite(nosyslogdestination configured),but its instrumentation is structurally identical to the two paths above.
Code
write()sites inwrite_pipe_chunks()(the full-payload loop and thefinal chunk), the
write()inwrite_console(), and bothsyslog()branches in
write_syslog(). No Unix-side path is missed.pgstat_report_wait_start()writes to*my_wait_event_info, which defaults to a process-local variable(
local_my_wait_event_info) untilpgstat_set_wait_event_storage()runs atbackend startup. So invoking these before
MyProcis attached — or from thepostmaster — is safe; it simply won't be visible in
pg_stat_activity.That's the expected/acceptable behavior, but it might be worth a one-line
comment so readers don't expect postmaster-side log writes to surface.
error reporting.
Minor / open
pgstat_report_wait_start()site could help,though the event names are self-explanatory.
upthread.
(
WriteConsoleW/EventlogWrite) — already flagged in the thread.From my side the Unix portion is ready-for-committer; the outstanding item is
Windows verification of 0002.
Tested-by: Nikolay Samokhvalov [email protected]