build_grouped_rel() makes a flat copy of the input relation, including
its FDW fields. As a result, when building a grouped join relation
for eager aggregation, add_paths_to_joinrel() called the FDW's
GetForeignJoinPaths for it. postgres_fdw then failed with "Aggref
found where not expected" on the partial Aggrefs in the target list,
and in any case it would have treated a partially aggregated input as
a plain foreign scan.
FDWs are not prepared to handle grouped relations, so clear serverid,
fdwroutine and fdw_private in build_grouped_rel().
Reported-by: Robert Haas <robertmhaas@gmail.com>
Author: Richard Guo <guofenglinux@gmail.com>
Discussion: https://postgr.es/m/CA+Tgmob7iSM9YkRM44VjUDuaCchW-fY54MV5njpTZTL9uNyV4w@mail.gmail.com
Backpatch-through: 19
When a Var of the relation to be partially aggregated is known equal
to a GROUP BY expression through an EC, get_expression_sortgroupref()
let it reuse that expression's SortGroupClause. If the two are of
different types, as with an int2 grouping key joined to an int4
column, the Var was then grouped using the other type's equality
operator, which could put unequal values into the same group and give
wrong results.
To fix, accept only EC members of the same type as the grouping
expression. A cross-type Var is then treated like any other join key
and gets a SortGroupClause built for its own type.
Reported-by: Robert Haas <robertmhaas@gmail.com>
Author: Richard Guo <guofenglinux@gmail.com>
Discussion: https://postgr.es/m/CA+Tgmob7iSM9YkRM44VjUDuaCchW-fY54MV5njpTZTL9uNyV4w@mail.gmail.com
Backpatch-through: 19
Commit 1ff3180ca0 made the leader of a parallel autovacuum propagate
changes to the cost-based delay parameters to its workers, but missed
track_cost_delay_timing. Workers therefore kept the value they had at
launch: turning the parameter on during the vacuum left
pg_stat_progress_vacuum.delay_time counting only the leader's naps,
and turning it off didn't stop workers from adding theirs. Throttling
itself was not affected.
Fix by adding track_cost_delay_timing to PVSharedCostParams. Since a
worker's setting can now change while it runs, we report its residual
delay time at exit whenever any has been accumulated, rather than only
if track_cost_delay_timing is still on.
Backpatch to v19, where parallel autovacuum was introduced.
Reported-by: Sami Imseih <samimseih.pg@gmail.com>
Author: Sami Imseih <samimseih.pg@gmail.com>
Reviewed-by: Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com>
Reviewed-by: Zsolt Parragi <zsolt.parragi@percona.com>
Reviewed-by: Masahiko Sawada <sawada.mshk@gmail.com>
Discussion: https://postgr.es/m/CAN12+YJZaALd3zMeKVVRTwcuY5_yDtR_vMPwWc8QNgeYxrn4Zg@mail.gmail.com
Backpatch-through: 19
Commit ce8d5fe0e failed to think hard enough about the case where
we're forced to replan the expression and it becomes no longer simple.
The code produced the right expression result, but its handling of
FOUND and ROW_COUNT was too simplistic since we might have obtained
zero rows. To fix, don't use exec_eval_expr, but go directly to
exec_eval_simple_expr, and if that reports failure then fall through
to the full SPI code path.
Another thinko in ce8d5fe0e is that it added
if (expr->plan == NULL)
exec_prepare_plan(estate, expr, 0);
which is quite useless since we cannot reach here without having
planned the expression at least once. (If the call were reachable
it'd likely be wrong, since it's not passing the same flags as
either the preceding call or the one inside exec_eval_expr.)
Author: shihao zhong <zhong950419@gmail.com>
Reviewed-by: Tom Lane <tgl@sss.pgh.pa.us>
Discussion: https://postgr.es/m/CAGRkXqQB_aqWU9OeVPGvATBVZ7c9EYrNXJyHG0jKXW0FGAy2-g@mail.gmail.com
Backpatch-through: 19
pg_blocking_pids() assumes that members of the same lock group
never block each other, but that hasn't been true for relation
extension locks since v13. This commit teaches the function to
report lock group members as blockers for relation extension locks.
Since parallel workers are reported with their leader's PID, the
result may now include the blocked session's own PID.
Oversight in commit 85f6b49c2c.
Author: Bertrand Drouvot <bertranddrouvot.pg@gmail.com>
Reviewed-by: Chao Li <li.evan.chao@gmail.com>
Reviewed-by: Kirill Reshke <reshkekirill@gmail.com>
Reviewed-by: Shihao Zhong <zhong950419@gmail.com>
Discussion: https://postgr.es/m/apTpV8h%2BCYHf%2B1PJ%40bdtpg
Backpatch-through: 14
When we converted these types' input functions to report errors
through the soft-error mechanism, we missed that stack-overflow
errors can be triggered through bad user input. Convert those
to be reported softly, too.
Bug: #19745
Reported-by: Ke <kehan5800@gmail.com>
Author: Tom Lane <tgl@sss.pgh.pa.us>
Discussion: https://postgr.es/m/19745-39cd7b93a505f9a9@postgresql.org
Backpatch-through: 16
create_append_path() has a rather questionable habit of overriding
the caller-supplied pathkeys when it sees that there is a single
child path, and applying the child's pathkeys instead. In most cases
we can get away with that, but it does not work for set-operation
AppendPaths. In set-operation nests, the append's result tlist will
contain "varno 0" Vars, which won't match the child's pathkeys,
leading to failure in create_plan().
We didn't have this problem before 03d40e4b5 allowed eliding
provably-empty child nodes of set-ops; there would never have been
a case with only one surviving child node, so create_append_path()
would always have accepted the specified NIL pathkeys.
As a band-aid fix, force the generated AppendPath's pathkeys to NIL
even if create_append_path() did something else, thus restoring the
status quo ante. This is demonstrably necessary for two of
prepunion.c's three calls; I did it at the third too, although
probably the pathkeys would already be NIL there.
There have been reasons to want to get rid of the "varno 0" hack
for a long time, and this is another one. But that will require
significant surgery in prepunion.c, and likely some changes in
parsetree representation, so we can't tackle it for v19. Hence,
we need a band-aid.
Bug: #19742
Reported-by: Junwen AN <feasiblechart@gmail.com>
Author: Tom Lane <tgl@sss.pgh.pa.us>
Co-authored-by: shihao zhong <zhong950419@gmail.com>
Discussion: https://postgr.es/m/19742-dc403ca277cad1d3@postgresql.org
Backpatch-through: 19
EXPLAIN failed to deal with varno-0 Vars that are made by prepunion.c
and can survive into a finished plan in the case where a provably
empty set operation is replaced by a dummy Result (which is possible
since 03d40e4b5). Commit 928df067d tried to fix this, but it was a
couple bricks shy of a load. First, it only dealt with varno-0 Vars
at the top level of the Result's tlist, but they could be buried
under coercion expressions. Fix that by doing a recursive mutation.
Second, it always replaced varno 0 with varno 1, but that's just
wrong: the Result might represent a group of setop leaf queries that
do not include the leftmost leaf. That led to displaying the wrong
variable(s) as outputs of the Result, risking confusion. Fortunately,
we can get the actual child relids from the recently-added
Result.relids field, and use that to discover the leftmost child
represented by the Result.
This was found while discussing bug #19742, but it's really an
independent issue.
Author: shihao zhong <zhong950419@gmail.com>
Co-authored-by: Tom Lane <tgl@sss.pgh.pa.us>
Discussion: https://postgr.es/m/CAGRkXqTFwygKmjLG_Y=kbXRHsePFmD6k+qrzVLP4_KrG+-=oRg@mail.gmail.com
Backpatch-through: 19
6ed83d5fa made it so tuplesorts could use a bump.c memory allocator for
non-bounded sorts in order to save memory and increase performance.
That commit didn't get the memory accounting correct for datum sorts and
mistakenly passed 0 bytes to tuplesort_puttuple_common(). That was
correct for byval types, but failed to take into account palloc'd memory
for byref types.
Fix by using datumGetSize() when using the bump allocator and by using
GetMemoryChunkSpace() when not.
Backpatch to v17, where 6ed83d5fa was added.
Author: Mario Karuza <mkaruza.pg@icloud.com>
Reviewed-by: David Rowley <dgrowleyml@gmail.com>
Reviewed-by: Joao Detomini <joao.detomini@enterprisedb.com>
Discussion: https://postgr.es/m/e9f9f54337cd86a982837763958518cdbf08863c.camel%40icloud.com
Backpatch-through: 17
pgindent descended into build directories located inside the source
tree and reindented the generated files there, which is wasteful and
often makes pg_bsd_indent fail outright. This change automatically
recognizes build directories and skips them. This works for both
Meson build directories and Autoconf VPATH build directories. A build
directory named directly on the command line is still processed, on
the assumption that this was asked for on purpose.
Reviewed-by: Tom Lane <tgl@sss.pgh.pa.us>
Reviewed-by: Nazir Bilal Yavuz <byavuz81@gmail.com>
Discussion: https://www.postgresql.org/message-id/flat/fc77b35a-34c4-4229-82f1-8cf0ce67f276%40eisentraut.org
bt_child_highkey_check() walks right along the child level following
btpo_next links, reading a page on each iteration, but its loop lacked a
CHECK_FOR_INTERRUPTS(). Every other page-traversal loop in
verify_nbtree.c already has one.
The loop's existing checks do not guarantee a timely exit on a corrupt
index. The in-loop cycle check only fires when a rightlink points back
through the block the walk started from, or to a page whose btpo_prev
points to itself, so a cycle further downstream can go undetected -- for
example a run of pages all flagged P_INCOMPLETE_SPLIT, which skips both
the high-key comparison and bt_downlink_missing_check() and so never
reaches an error. Such an index, or simply a very long rightlink chain,
makes the walk effectively uninterruptible. Since amcheck exists to be
run against possibly-corrupt indexes, make this walk respond to query
cancellation and shutdown requests like the sibling loops do.
Author: Paul Kim <mok03127@gmail.com>
Reviewed-by: shihao zhong <zhong950419@gmail.com>
Discussion: https://www.postgresql.org/message-id/20260717151437.10713-1-mok03127@gmail.com
It's a bad idea to sleep while holding locks, because you might block
another process that wants to acquire the same lock. Furthermore,
it's not safe to call ProcessConfigFile() in a critical section or
while holding locks. So avoid calling vacuum_delay_point() while
holding locks, and also make vacuum_delay_point() return quickly
without sleeping if it's called while interrupts cannot be processed.
To find the vacuum_delay_point() calls that were made while holding
locks, I added "Assert(INTERRUPTS_CAN_BE_PROCESSED())" in
vacuum_delay_point() and ran the regression tests. I didn't include
that Assert in this commit, because there might be more places that do
that that are not covered by the regression tests, including
extensions.
In ginInsertCleanup(), there was already another vacuum_delay_point()
call later in the loop, while not holding any locks, so just remove
the other call that was made while holding the lock.
In hashbucketcleanup(), move the vacuum_delay_point() call up the
stack to its caller. The call in hashbucketcleanup() was not able to
handle interrupts because it held a lock, and because there were no
vacuum_delay_point() or CHECK_FOR_INTERRUPTS() calls in the caller's
loop, the whole hash index vacuuming phase was uninterruptible by
pending shutdown or query cancel. Now it can be interrupted between
buckets. Unfortunately, hashbucketcleanup() has to process all the
bucket's pages in one go without pausing; fixing that would require
changing how hashbucketcleanup()'s lock chaining works.
In acquire_sample_rows(), move the vacuum_delay_point() call in the
loop to between pages, to a time where we're not holding the buffer
lock.
In master, also remove a misleading CHECK_FOR_INTERRUPTS() call from
pgstat_write_statsfile(). It was a no-op because we're always holding
a lock on the current dshash partition at that point. A call that
does nothing is harmless, but it makes you think that the loop is
interruptible when in reality it's not.
Author: Kevin Rocker <me@kevinrocker.com>
Author: Andrey Borodin <amborodin@acm.org>
Author: Mostafa <mostafa.nabil.nafie@gmail.com>
Reported-by: Greg Burd <greg@burd.me>
Reported-by: Sergei Kornilov <sk@zsrv.org>
Reviewed-by: Kirill Reshke <reshkekirill@gmail.com>
Reviewed-by: Neil Chen <carpenter.nail.cz@gmail.com>
Tested-by: Greg Burd <greg@burd.me>
Discussion: https://postgr.es/m/19628-c2b17d358181a1ea@postgresql.org
Discussion: https://postgr.es/m/492c6247-43d3-477b-8981-fb0c56767b38%40app.fastmail.com
Backpatch-through: 14
This commit addresses the following issues in commit de28140de:
* When the local transaction was DEFERRABLE, begin_remote_xact() started
a remote transaction in the same mode without checking the remote
server version. Since DEFERRABLE is supported only in v9.1 and later,
this would cause a syntax error on older remote servers. Fix this by
propagating the mode only to remote servers that support it.
* pgfdw_xact_callback() committed remote transactions without
synchronizing the read-only mode. If the local transaction had become
READ ONLY since the last remote operation, this would cause deferred
triggers on remote servers to still run in READ WRITE mode. Fix this
by synchronizing the mode at commit time.
* The documentation did not match the actual implementation. Update the
documentation to reflect the implementation.
Reported-by: Fujii Masao <masao.fujii@gmail.com>
Author: Etsuro Fujita <etsuro.fujita@gmail.com>
Reviewed-by: Matheus Alcantara <matheusssilv97@gmail.com>
Discussion: https://postgr.es/m/CAHGQGwE4UMaWGOZq1Z7YUTUSg-CgL4yAzO5YaGwTZe1-6Pjg2g%40mail.gmail.com
Backpatch-through: 19
When a LATERAL UNION ALL subquery is flattened into an appendrel, a
child EquivalenceMember's expression can contain lateral references to
other relations. create_join_clause() builds a clause from such a
member with make_restrictinfo(), which computes clause_relids from the
expression, and so includes the relids of the lateral references along
with those of any outer joins that null them. Commit 03107b4ed made
sure that clause_relids include the child's relids, but it only ever
added relids.
This is a problem when a lateral reference is nullable. The child is
parameterized by its lateral_relids, which do not include the outer
joins that null the lateral references, so the clause fails the
join_clause_is_movable_into() assertion that
get_baserel_parampathinfo() applies to EC-derived clauses.
To fix, set a child clause's clause_relids from its members' em_relids
rather than from their expressions. This matches how
adjust_appendrel_attrs() computes clause_relids for a child's other
join clauses. The lateral references need not be included, since
every path for the child is parameterized by at least its
lateral_relids. Also update num_base_rels to match, which commit
03107b4ed neglected to do.
Back-patch to v16, where outer-join relids were added to
clause_relids. Before that, the relids of a lateral reference are
always part of the child's parameterization, so the problem does not
arise.
Author: Richard Guo <guofenglinux@gmail.com>
Reviewed-by: wenhui qiu <qiuwenhuifx@gmail.com>
Discussion: https://postgr.es/m/CAMbWs4-cRxpQNyQJW=PkDagSDGQ=3Rp2t_fSAyROKsDStjKHUQ@mail.gmail.com
Backpatch-through: 16
Apple clang rejects taking the address of a flexible array member that
carries a counted_by attribute. A future patch wants to add that
attribute to two such places. To avoid problems, use the array itself
instead taking its address with &, which yields the same
address. (There are a few more, but they are not candidates for a
counted_by attribute, so they are left alone here.)
Discussion: https://www.postgresql.org/message-id/flat/DKBEBF32QLUA.3L2DJRPUYNKHZ%40partin.io
Buildfarm members adder and drongo failed to observe the expected relation
lock wait in the startup deadlock test. In adder's failure, autovacuum
pruned dead rows before the test's explicit VACUUM FREEZE, canceling the
standby backend too early and releasing the relation lock wait before the
test could observe it.
Fix this by disabling autovacuum for the test node, leaving the explicit
VACUUM commands in control of when pruning occurs. This also prevents
auto-analyze workers on other relations from holding back xmin and
interfering with the snapshot conflict test.
Backpatch to v17, where this test is enabled.
Reported-by: Alexander Lakhin <exclusion@gmail.com>
Author: Ayush Tiwari <ayushtiwari.slg01@gmail.com>
Reviewed-by: Fujii Masao <masao.fujii@gmail.com>
Discussion: https://postgr.es/m/421c0aee-84c8-4c07-b4b9-263095479755%40gmail.com
Discussion: https://postgr.es/m/714729ea-f536-0faf-353b-cbd5c8f241ea%40gmail.com
Backpatch-through: 17
Buildfarm members olingo and adder reported failures in
031_recovery_conflict.pl where a recovery conflict was counted twice.
olingo saw the snapshot conflict count reach 2 instead of 1, while adder
saw the same for the buffer pin conflict count.
The startup process can signal a standby backend again while it is
reporting a recovery-conflict FATAL error. Each processed conflict can
increment the corresponding statistic. Thus, even when the expected
conflict occurs, its counter can exceed 1. The existing poll for exactly 1
can then time out, and the exact check of the aggregate count can fail as
well.
Fix this by polling for a positive count for each conflict type and
accepting an aggregate count of at least the expected number.
Backpatch to v17, where this test was enabled.
Reported-by: Alexander Lakhin <exclusion@gmail.com>
Author: Ayush Tiwari <ayushtiwari.slg01@gmail.com>
Reviewed-by: Alexander Lakhin <exclusion@gmail.com>
Reviewed-by: Nazir Bilal Yavuz <byavuz81@gmail.com>
Reviewed-by: Fujii Masao <masao.fujii@gmail.com>
Discussion: https://postgr.es/m/421c0aee-84c8-4c07-b4b9-263095479755%40gmail.com
Backpatch-through: 17
The label is written as one line of the backup_label file. A label
with a newline added extra lines to the file, and recovery could read
those as other fields, failing on them. Characters could also be
injected to make recovery behave in inconsistent ways.
Trying to use such characters in label names is now rejected, for both
pg_backup_start() and BASE_BACKUP.
As this is arguably a behavior change, no backpatch is done. I also
seriously doubt that anybody is relying on the behavior of pushing some
arbitrary data to backup_label files, and even if they do, it would be a
very bad idea to contradict what the backend decides to generate.
Reported-by: Shallow <theshallow27@gmail.com>
Author: Shihao Zhong <zhong950419@gmail.com>
Discussion: https://postgr.es/m/19730-85a6044c9e72774a@postgresql.org
Clearing the tuplestore rather than ending it avoids the need for
various memory allocations, so is slightly more efficient. However, the
main reason to do this is to correctly track the maximum storage used by
the Table Function Scan node. 40708acd65 added additional EXPLAIN
output for Table Function Scan nodes and that output does claim to be
showing "Maximum Storage", which is not true, as the maximums could be
lost after tuplestore_end() is called during a rescan.
This causes misreporting when the final rescan of a Table Function Scan
node uses less storage than some previous rescan. One example is:
EXPLAIN ANALYZE
SELECT count(*)
FROM (VALUES ((SELECT jsonb_agg(g) FROM generate_series(1, 100000) g)),
('[1]'::jsonb)) v(j),
LATERAL JSON_TABLE(v.j, '$[*]' COLUMNS (x int PATH '$')) jt;
Without this fix, the reported Table Function Scan storage is for the
'[1]'::jsonb case, whereas it should consider how much was used with the
larger document and show the maximum of all rescans.
TableFuncNext() used a NULL tupstore to tell that the rows still had to
be fetched, so add a tupstore_filled flag for that, similar to what
908a96861 did for WindowAgg.
Technically, this issue does exist in v18, but no backpatch as this is a
fairly minor issue which was only noticed during a review of a similar
feature being proposed for Function Scans.
Author: Tatsuya Kawata <kawatatatsuya0913@gmail.com>
Reviewed-by: David Rowley <dgrowleyml@gmail.com>
Discussion: https://postgr.es/m/CAHza6qdXjcqXv2aGU7OCakG0OmEsy9-jfP0Gpe2EL+k-AidOhw@mail.gmail.com
On Windows, a backend exiting resets its connection, and the client
discards any data not yet read, including the FATAL message of a
terminated backend. isolationtester polls one connection at a time, so
when it waits on a different session the message can get lost, failing
the permutation and the tests that run after it.
wait_cleanup in injection_points (down to v17) and slot_creation_error
in test_decoding (down to v15) have this pattern, and have been failing
randomly in the buildfarm. Let's skip them on Windows.
The cause of the instabilities is proving to be tricky to deal with,
pointing for example at 29992a6a50. This will be tackled later,
perhaps with a backpatch (or perhaps with a HEAD-only solution), but it
is difficult to be sure at this stage of the work. In theory,
a8458f508a should make a revert of 29992a6a50 (revert of a revert!)
safe on WIN32.
Note that 278cdea6b9, that has introduced the isolation test
slot_creation_error in test_decoding, has mentioned the potential output
instability.
Author: Zsolt Parragi <zsolt.parragi@percona.com>
Author: Michael Paquier <michael@paquier.xyz>
Discussion: https://postgr.es/m/CAN4CZFOoiKa9E4Gnh_OzLk3S5Z6RFyg_oY1e_WddCVYhkpTVow@mail.gmail.com
Backpatch-through: 15
Clearing the tuplestore rather than ending it avoids the need for
various memory allocations, so is slightly more efficient. However, the
main reason to do this is to correctly track the maximum storage used by
the Material node. 1eff8279d added additional EXPLAIN output for Material
nodes and that output does claim to be showing "Maximum Storage", which is
not true, as the maximums could be lost after tuplestore_end() is called
during a rescan.
This causes misreporting when the final rescan of a Material node uses
less storage than some previous rescan. One example is:
EXPLAIN ANALYZE
SELECT count(*) FROM (VALUES(10000),(1)) v1(r1)
LEFT JOIN LATERAL (
SELECT * FROM generate_series(1, 2) gs0
LEFT JOIN LATERAL (
SELECT * FROM generate_series(1, v1.r1) gs1
FULL JOIN generate_series(1, v1.r1) gs4 ON false
) ss on true
) q on true;
Without this fix, the reported Material storage is for the v1.r1 = 1 case,
whereas it should consider how much was used with the v1.r1 = 10000 case
and show the maximum of each.
Technically this issue does exist in v18, but no backpatch here as
parameterized Material nodes are not that common and there have been no
bug reports about the incorrect memory reporting, so it's probably not
worth risking backpatching the change.
Author: David Rowley <dgrowleyml@gmail.com>
Reviewed-by: Tatsuo Ishii <ishii@postgresql.org>
Reviewed-by: Tatsuya Kawata <kawatatatsuya0913@gmail.com>
Discussion: https://postgr.es/m/CAApHDvoa55vcRth05Ozu5be4FawgTH-aCsZ5=Z+_UXTUzUxdQg@mail.gmail.com
In clang, -fsanitize=undefined includes -fsanitize=function, which
reports every call made through a function pointer whose type does not
exactly match the called function, so it fires all over the place on
expression tree walkers and mutators, as well as a few other places.
So -fsanitize=undefined hasn't been working cleanly under clang for a
while. (Before clang 17, it only applied to C++.)
This is the same issue that caused us to use
-Wno-cast-function-type-strict with clang. That warning applies at
the place where the mismatching function pointer is passed, so there
are potentially hundreds of sites. Therefore, a global disabling is
appropriate. The sanitizer, on the other hand, triggers where the
function is called, which are only about two dozen places, so it seems
possible to silence these checks individually and still main the check
for accidental violations elsewhere.
Add pg_attribute_no_sanitize_function() and place it on the functions
that make such calls.
Reviewed-by: Chao Li <li.evan.chao@gmail.com>
Discussion: https://www.postgresql.org/message-id/flat/2db401d9-ef16-430e-a42c-34477006a49f%40eisentraut.org
preprocess_expression skips flatten_join_alias_vars for TABLESAMPLE
clauses, on the assumption that they cannot contain Vars of the
current query level. That's not true once a LATERAL subquery
containing a sampled relation has been pulled up: the relation's
TABLESAMPLE clause then becomes a lateral reference of this level, and
if it referred to a join alias Var, that Var is left unexpanded.
extract_lateral_references then pulls the join alias Var out as a
lateral reference and fails with "no relation entry for relid N".
To fix, treat such clauses like lateral function and VALUES RTEs:
introduce EXPRKIND_TABLESAMPLE_LATERAL, used for the TABLESAMPLE
clause of a lateral relation RTE, and keep skipping the alias
expansion only for the non-lateral kind.
Back-patch to all supported branches, as this dates back to the
introduction of TABLESAMPLE.
Author: Richard Guo <guofenglinux@gmail.com>
Reviewed-by: Ewan Young <kdbase.hack@gmail.com>
Reviewed-by: Ayush Tiwari <ayushtiwari.slg01@gmail.com>
Discussion: https://postgr.es/m/CAMbWs4834TwOtyyycyvja3MuOBkfpXy3dFAL0zt_vcM1KtqV-Q@mail.gmail.com
Backpatch-through: 14
If a subquery has been flattened into its parent, the joinaliasvars
entries of a join above it can be arbitrary expressions rather than
plain Vars, so expanding a reference to such a join alias may insert a
SubLink into a lower-level subquery. flatten_join_alias_vars_mutator
detects that and sets the subquery's hasSubLinks flag, but only in the
single-column code path; the whole-row path just asserted in a comment
that its recursive call would handle this, which is true only when the
alias entry is itself a Var referencing another join.
Hence a whole-row reference to such a join appearing in a sub-select
left that sub-select's hasSubLinks false, so preprocess_expression
skipped SS_process_sublinks for it, and the unprocessed SubLink
reached code that is not prepared for one: this produced "cannot
handle unplanned sub-select" from cost_qual_eval, an assertion failure
in preprocess_aggrefs, or "unrecognized node type" at execution,
depending on where in the sub-select the SubLink ended up.
To fix, make the same check in the whole-row expansion path.
Back-patch to all supported branches.
Author: Richard Guo <guofenglinux@gmail.com>
Reviewed-by: Ayush Tiwari <ayushtiwari.slg01@gmail.com>
Discussion: https://postgr.es/m/CAMbWs49PgeNFhQTZ2gSzwatF0_LXyegMPGt++jSfxUN7NZuQ5Q@mail.gmail.com
Backpatch-through: 14
MSVC has no counted_by attribute, but its source annotation
language (SAL) has _Field_size_(), which states the same property: the
field is a buffer whose writable size in elements is given by the
named expression.
MSVC's static analyzer (/analyze) will make use of the information.
Author: Tristan Partin <tristan@partin.io>
Discussion: https://www.postgresql.org/message-id/flat/DKBEBF32QLUA.3L2DJRPUYNKHZ%40partin.io
In an in-tree build, "make coverage-html" invoked lcov with two
"-d ." options, because $(srcdir) is just ".". With lcov 2.0
(and perhaps earlier versions?) that results in scanning every
foo.gcda file twice and producing counts twice what they should
be. With later lcov versions (at least 2.3), it results in
"duplicate file" errors.
In VPATH builds we do need two -d options to cover both regular
and generated .c files. But in in-tree builds it suffices to
specify "-d .", so suppress the second switch.
Author: Narek Galstyan <narek.galstyan@enterprisedb.com>
Reviewed-by: Tom Lane <tgl@sss.pgh.pa.us>
Discussion: https://postgr.es/m/CAHsn6_xCDQWe8_vVFhtFk27_xTdyVV+Dr0yWzaooBZ6+-VH-5w@mail.gmail.com
Backpatch-through: 14
When repacking a table concurrently, some border conditions can cause
a concurrent update to propagate the original old tuple to the transient
table (such as a trigger that returns OLD). That's essentially just
wasted space, which REPACK is there to remove. Prevent that by setting
the isnull flag to true for any column cataloged as dropped in tuples
being "replayed" as concurrent updates.
Reported-by: Radim Marek <radim@boringsql.com>
Reviewed-by: Antonin Houska <ah@cybertec.at>
Backpatch: 19
Discussion: https://postgr.es/m/CAJgoLkK2UBzB1J9buCsSUbjf7bOqz-o_0=CeiTruU789BhBw-Q@mail.gmail.com
This removes the buffering of FK rows for batched PK index probes,
along with the after-trigger callback infrastructure used only by
batching. The per-row fast path, which probes the PK index directly
instead of going through SPI, remains. This is a cherry-pick of
commit 25649d6e79, which made the same change in REL_19_STABLE,
adjusted for some drift between the branches.
I (amitlan) initially thought we could keep improving the batching
path on master by fixing its remaining bugs and other issues as
they were found. However, we are still finding cases where delaying
checks to accumulate a batch changes their results, such as when other
AFTER ROW triggers modify the referenced table. Individual cases can
be addressed, but new ones keep turning up, which makes it hard to be
confident that fixing the known ones will be enough. It seems better
to rip batching out and reconsider how to introduce it, or at least
its useful pieces, while preserving the semantics of per-row checks.
Discussion: https://postgr.es/m/CA+HiwqFx4cjFtC3akr1DW-cPYw+B-EkCSffZPrkpTef8PNS6qA@mail.gmail.com
This is technically the more correct position according to C23,
although in practice compilers are relaxed about this. It also opens
us up to supporting MSVC's _Field_size_() in the future, which is a
similar construct.
This requires adding pg_attribute_counted_by to the list of
declaration attributes in pgindent, so that pgindent doesn't mess it
up.
Author: Tristan Partin <tristan@partin.io>
Discussion: https://www.postgresql.org/message-id/flat/DKBEBF32QLUA.3L2DJRPUYNKHZ%40partin.io
When the index cannot be used to find a recently deleted row,
RelationFindDeletedTupleInfoSeq() scans the table. It looked up the
relation's replica identity or primary key again to determine which
columns to compare, which could differ from the index selected when the
relation was opened. A concurrent DROP INDEX CONCURRENTLY could remove the
replica identity in between, so the search compared the whole row and
reported the conflict as update_deleted instead of update_missing.
Use the index recorded in the relation map instead. The relation map
already records the replica identity index, or the primary key if there is
no replica identity. It also avoids using a deferrable primary key, which
cannot serve as a replica identity; using its columns could incorrectly
report a deleted row with different values as update_deleted instead of
update_missing.
Author: Hayato Kuroda <kuroda.hayato@fujitsu.com>
Author: Nisha Moond <nisha.moond412@gmail.com>
Reviewed-by: Vignesh C <vignesh21@gmail.com>
Reviewed-by: Zhijie Hou <houzhijie22@gmail.com>
Reviewed-by: Amit Kapila <amit.kapila16@gmail.com>
Discussion: https://postgr.es/m/CABdArM5ydwdRrpaZyK1q2p3-vY_+pnBtTmkvg_pcM=gHwmH7Kg@mail.gmail.com
Discussion: https://postgr.es/m/OS7PR01MB1831779ED93F5470CA1D57605F58C2@OS7PR01MB18317.jpnprd01.prod.outlook.com
Backpatch-through: 19
pre_indent() hides each alignas(...) call from pg_bsd_indent behind
a plain identifier, because pg_bsd_indent has a fixed table of
declaration keywords and lexes anything else followed by "(" as the
start of an expression.
The placeholder was not the same width as the text it stood in for,
though, and pg_bsd_indent decides where to put a trailing comment from
the width of the code preceding it. A member declared with alignas()
and carrying a trailing comment therefore got the comment separated by
a tab where the real width calls for a single space, or the other way
round. Pad the placeholder out to the original width so that the
decision is made on the real width.
No member currently declared with alignas() has a trailing comment, so
this changes nothing in the tree today; it is a correctness fix for the
helper.
Author: Tristan Partin <tristan@partin.io>
Co-authored-by: Peter Eisentraut <peter@eisentraut.org>
Discussion: https://www.postgresql.org/message-id/flat/DKBEBF32QLUA.3L2DJRPUYNKHZ%40partin.io
dumpIndex() emits ALTER INDEX .. SET STATISTICS for per-column
statistics targets, but dumpConstraint() never did. Targets set on the
expression columns of an exclusion constraint's index were silently
lost on dump/restore and pg_upgrade.
The same code can be used in the index and constraint dump code to
generate these ALTER INDEX commands. The logic has been refactored so
that both share a new subroutine in charge of generating all these
custom SET STATISTICS, if any.
Oversight in e4fca461ab.
Author: Zsolt Parragi <zsolt.parragi@percona.com>
Reviewed-by: Chao Li <li.evan.chao@gmail.com>
Reviewed-by: Andrew Krylosov <krylosov.andrew@gmail.com>
Discussion: https://postgr.es/m/CAN4CZFMd9riOuV5LkM+uV=uF-HYp49C8Fh22xWyMxptGLBCZ3A@mail.gmail.com
Backpatch-through: 14
FetchStatementTargetList() dereferenced the portal named by a
FetchStmt without checking that it still exists. With the extended
query protocol, a client can sent a Parse "FETCH n FROM c" while cursor
"c" is open (which caches a result's TupleDesc), issue a CLOSE on cursor
"c", and then Describe the prepared statement or a portal bound to it.
Describe would re-resolve the target list through the named cursor, and
crash. In a non-assert builds, this causes a NULL pointer dereference.
An assertion was triggered in assert builds slightly before the pointer
dereference.
This commit changes the describe of a FetchStmt to match with the
ExecuteStmt case: if the named cursor's portal is invalid, fail rather
than assume that the portal should always exist.
A test case is added to libpq_pipeline, backpatchable all the way down.
Author: Dirkjan Bussink <d.bussink@gmail.com>
Reviewed-by: Tom Lane <tgl@sss.pgh.pa.us>
Reviewed-by: Michael Paquier <michael@paquier.xyz>
Discussion: https://postgr.es/m/7DE248EB-7509-4F8A-9F37-4694A40BBAB7@gmail.com
Backpatch-through: 14
pg_stat_progress_vacuum gains three fields, reflecting some activity
related to index cleanups during VACUUM, which is a phase that could
take a long time depending on the quantity of data to treat and the
number of indexes:
- current_index_relid, the index being processed.
- index_blks_total, total number of blocks to process.
- index_blks_done, number of blocks already processed.
There fields are new, and do not reuse the existing fields related to
heap as index cleanups may need multiple passes, where knowing both the
heap activity and the index activity could be relevant. This is
unlikely since tids are accumulated in the heap scanning phase using an
optimized radix tree, still possible. VACUUM can spawn parallel workers
to clean indexes (each worker cleans a different index); these should up
in the progress view in the shape of one extra row for each worker.
Bump catalog version.
Author: Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com>
Reviewed-by: Sami Imseih <samimseih@gmail.com>
Reviewed-by: Michael Paquier <michael@paquier.xyz>
Reviewed-by: Masahiko Sawada <sawada.mshk@gmail.com>
Reviewed-by: Shihao Zhong <zhong950419@gmail.com>
Discussion: https://postgr.es/m/CALj2ACUgwSchK6jQ2CdKLBWUADTOE_zKdTff2Zg3E6hOuXKv-w@mail.gmail.com
Commit 1ff3180ca0 made a parallel autovacuum leader propagate changes
to the cost-based delay parameters to its workers only from
vacuum_delay_point(), which it no longer reaches once it is waiting in
WaitForParallelWorkersToFinish(). While waiting, the leader neither
processed a config reload nor noticed a change in the number of
autovacuum workers sharing the cost limit, so its workers kept running
with stale parameters until they finished.
Fix by refreshing and propagating the parameters on every wakeup of
that wait loop, and by setting the latches of the balanced autovacuum
workers when their count changes so that a waiting leader wakes up.
A regression test will be added in a separate commit.
Backpatch to v19, where parallel autovacuum was introduced.
Reported-by: Nikolay Samokhvalov <nik@postgres.ai>
Author: Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com>
Reviewed-by: Manu <manuelreyesbravo@gmail.com>
Reviewed-by: Zsolt Parragi <zsolt.parragi@percona.com>
Reviewed-by: Daniel Gustafsson <daniel@yesql.se>
Reviewed-by: Masahiko Sawada <sawada.mshk@gmail.com>
Discussion: https://postgr.es/m/CAM527d-GL=Jp2EJXBSnVBGPK-4XEZwWof5Cv8P0hghS_og6oAg@mail.gmail.com
Backpatch-through: 19
Commit 7164dcb665 added the repack_decode isolation test, whose
s1_decode step counts every change decoded from the slot. A concurrent
autovacuum transaction that modifies only system catalogs is decoded
as an empty BEGIN/COMMIT pair, so the count could come out larger
than expected.
Fix by passing skip-empty-xacts to test_decoding. We pass it, along
with include-xids=0, to s1_decode_updates as well so that both steps
use the same options for consistency, though that step shows only
UPDATE lines and wasn't affected.
Per buildfarm animal skink.
Reported-by: Alexander Lakhin <exclusion@gmail.com>
Discussion: https://postgr.es/m/d704cde4-b1f2-48dc-8c5e-659d1128d286@gmail.com
Backpatch-through: 19
There have been several recent complaints that << and >> don't behave
the same way on all supported platforms. There is a risk that <<
overflows the signed integer type, which is undefined behavior, and there
are platform-dependent behaviors from shifting by numbers of bits >= the
bitwidth of the type, and also from shifting by negative values.
Document this to warn unsuspecting users of these facts.
Author: David Rowley <dgrowleyml@gmail.com>
Reviewed-by: Tom Lane <tgl@sss.pgh.pa.us>
Discussion: https://postgr.es/m/CAApHDvr0O6ZF8NO=Rq9Q+Sz=FHT=NZ1kkNTXZDcNph-OWuk7TQ@mail.gmail.com
Backpatch-through: 14
Canada's Northwest Territories will move to permanent UTC-06 on
2026-11-01. For now, assume their TZ abbreviation will be CST from
that time forward. This affects only America/Inuvik as the rest of
the territory is covered by America/Edmonton, for which the
equivalent change was released in 2026c.
Manitoba (America/Winnipeg) will move to permanent UTC-05 on
2026-10-31. For now, assume their TZ abbreviation will be EST
from that time forward.
(Upstream notes that more changes may be needed soon if smaller
jurisdictions choose to break away from what these have done,
but for now the plan is wait-and-see.)
Historical corrections for Colombia, Iran, and Ireland.
The backward-compatibility zone names EST5EDT, CST6CDT, MST7MDT, and
PST8PDT now conform better to POSIX. For example, EST5EDT now always
uses the abbreviation "EST" for standard time (now always 5 hours
behind UTC) and "EDT" for daylight saving time, whereas it formerly
had different UT offsets before standard time was introduced and
sometimes used abbreviations like "LMT", "EWT" and "EPT", all contrary
to POSIX. Also, though not specified by POSIX these names now use US
federal rules rather than rules of specific places like New York,
reverting to 2024a behavior. This change affects only timestamps
before 1966-10-30 at 01:00 standard time.
Backpatch-through: 14
Commit 0785d1b8b wrote
jsonapi_shlib.o: override CPPFLAGS += -DJSONAPI_USE_PQEXPBUFFER
forgetting that GNU make treats this change to CPPFLAGS as affecting
not only jsonapi_shlib.o, but anything that gets built in pursuit of
building jsonapi_shlib.o. Because of an even more ancient hack
(cf 7143b3e82), that will include jsonapi.o if it's not been built
already. If we do apply this -D switch to jsonapi.o, it gets built
with the wrong expansible-string infrastructure, which manifests
immediately as a link failure in pg_combinebackup and would misbehave
at runtime even if the build somehow completed.
Perhaps remarkably, we'd not seen field reports of build failures
till now, but it's pretty easy to reproduce a mis-build by trying
to build src/common/ with --shuffle and a high -j factor.
To fix, don't modify CPPFLAGS this way; instead make a dedicated
build rule for jsonapi_shlib.o that injects the additional switches.
There might be other places with similar problems, but some desultory
attempts with -j32 --shuffle on a 32-thread CPU didn't find any more
build failures. Also, it looks like the meson build infrastructure
lacks this problem (but I'm no meson expert).
Bug: #19727
Reported-by: Ross Burton <ross@burtonini.com>
Author: Tom Lane <tgl@sss.pgh.pa.us>
Reviewed-by: Ross Burton <ross@burtonini.com>
Discussion: https://postgr.es/m/19727-32d67d9dea410f38@postgresql.org
Backpatch-through: 18
The explanation of aggressive scans claimed that if the multixact
members grows to 2 billion entries, we launch autovacuum even if it's
nominally disabled. That's no longer true; anti-wraparound vacuum is
only triggered if we're in danger of running out of multixids, we no
longer launch it just to shrink the 'members' SLRU. There's no
wraparound danger with 'members' anymore, so it's no worse than if a
user table becomes very bloated.
We do still adjust the freeze threshold though, so that if autovacuum
runs, whether on schedule or as an emergency autovacuum to avoid
wraparound, it will freeze more aggressively if the members SLRU is
large.
I removed the mention of the 4 billion member threshold, even though
that still exists, because the explanation felt misleading. The
aggressiveness is scaled proportionally between 2 and 4 billion
members. At 4 billion it's as aggressive as it gets, but there's no
sudden cliff.
In the passing, fix some comments. Remove obsolete mention of safe
threshold for member storage triggering autovacuum. Secondly, the
freezing cutoff is adjusted to avoid the *members* SLRU from growing
too large.
Reported-by: Noah Misch <noah@leadboat.com>
Discussion: https://www.postgresql.org/message-id/20260827231757.78.noahmisch@microsoft.com
Backpatch-through: 19
ShmemRequestInternal() rejected SHMEM_ATTACH_UNKNOWN_SIZE whenever
IsUnderPostmaster was false. That also rejected an after-startup
attachment in a standalone backend, even if the area exists.
To fix, check whether the shared memory has been initialized
(ShmemIndex == NULL) rather than IsUnderPostmaster.
Refactor the tests, and add more test cases for after-startup
attachment, with SHMEM_ATTACH_UNKNOWN_SIZE and a mismatching size, and
for this single-user case.
Author: Ayush Tiwari <ayushtiwari.slg01@gmail.com>
Reviewed-by: Ashutosh Bapat <ashutosh.bapat.oss@gmail.com>
Discusssion: https://www.postgresql.org/message-id/CAJTYsWXREGXCnWdZJsXt8pT2UY_Y2GSx+HOUu2X3s8YDCLj0_A@mail.gmail.com
Backpatch-through: 19
ReplicationSlotRelease() copied MyProc->statusFlags unconditionally
when resetting PROC_IN_LOGICAL_DECODING, using its pgxactoff to decide
which area to update.
Slot invalidations can be done by the checkpointer or the startup
process, and ReplicationSlotRelease() ignored the fact that pgxactoff is
only meaningful for processes in the proc array, that auxiliary
processes never join. pgxactoff would remain at 0 for these, meaning
that an incorrect set of status flags would be updated.
The consequences of this issue could be various, depending on the
statsFlags lost in the proc slot incorrectly overwritten. Losing
PROC_AFFECTS_ALL_HORIZONS has been mentioned as the worse case, causing
VACUUM to remove rows a standby may needs even if standby feedback is
enabled. Assertions failures are another possibility.
Let's skip the update of statusFlags unless PROC_IN_LOGICAL_DECODING is
set. Nothing but StartupDecodingContext() sets it, and no auxiliary
processes call it.
Author: Vlad Lesin <vladlesin@gmail.com>
Reviewed-by: Hayato Kuroda <kuroda.hayato@fujitsu.com>
Reviewed-by: Michael Paquier <michael@paquier.xyz>
Discussion: https://postgr.es/m/889a06dd-45fc-423b-9dd2-87d5b5dc60c6@gmail.com
Backpatch-through: 14
SJE didn't consider the possibility that two RTEs referencing the
same table have different securityQuals, and might choose to merge
them anyway, thereby possibly losing quals that need to be enforced.
This is a regression introduced by 2ebf25e7d, since before that we
didn't re-generate baserestrictinfo lists from the RTEs' securityQuals
after performing SJE.
To defend against this, only consider SJE between RTEs with the same
checkAsUser values. That solves the problem because the set of
applicable RLS policies depends only on the role that is considered
to be accessing the table, so that the securityQuals must be equal
if the checkAsUser values are. It might also keep us from creating
similar bugs if we ever invent other features that depend on the
accessing role. And it's a lot cheaper than comparing securityQuals
trees themselves would be, not least because we can't sort them so
the preliminary sort step wouldn't help.
(The first proposed solution was to not perform SJE at all on RTEs
with nonempty securityQuals, but that seems rather sad, especially
since such cases worked before 2ebf25e7d. Making the restriction
depend on checkAsUser seems much less likely to interfere with SJE
unnecessarily, since in most cases that'll be the same for all
potentially-mergeable RTEs.)
The added test cases show that SJE is rejected when necessary, and
also demonstrate two quirks of this implementation. One is that if
a removed security qual includes an InitPlan, we'll still attach the
now-unused InitPlan to the plan. That's because SS_process_sublinks
runs before self-join elimination, so the extra InitPlan has already
been made. The other quirk is that we won't merge RTEs having zero
and nonzero checkAsUser fields, even if the calling user matches the
nonzero checkAsUser value so that there is no difference for this
user. That's intentional so that SJE doesn't require having to mark
the plan as caller-dependent.
Reported-by: Yonghwa Lee <underdog@theori.io>
Author: Tom Lane <tgl@sss.pgh.pa.us>
Reviewed-by: Ayush Tiwari <ayushtiwari.slg01@gmail.com>
Discussion: https://postgr.es/m/20260926174457.13.noahmisch@microsoft.com
Backpatch-through: 18
The help text for -c option documents that zero for either transaction
ID means to leave the corresponding value unchanged. Commit
978cf02bb8 replaced the separate checks for the two values with a
single flag indicating that the option was specified, causing zero to
be written to the control file instead.
Also parse the second transaction ID with strtouint32_strict(), like
the first one. Using strtoul() there was an oversight in commit
636c1914b4, which intended to reject negative and out-of-range
arguments.
Author: Chao Li <lic@highgo.com>
Discussion: https://www.postgresql.org/message-id/F61E486B-746B-42F3-A532-B8D4F1519583@gmail.com
Backpatch-through: 19
calcSumX2 is used to inform the aggregate transition function whether it
should include the square of the transition value in the aggregate
state. The deserialize functions for various numeric aggregates that do
use calcSumX2 weren't properly setting the property to true. This does
not matter in core as we never perform any aggregate transitions after a
deserialize operation.
This fix is only required for external code which plumbs aggregate paths
together in such a way that could result in aggregate transitions being
performed after states have been deserialized.
Author: Maxime Schoemans <maxime.schoemans@enterprisedb.com>
Reviewed-by: Andrey Borodin <x4mmm@yandex-team.ru>
Discussion: https://postgr.es/m/FEBF5C3D-CC53-455F-AA9D-4D377A2E957C@enterprisedb.com
Backpatch-through: 14
If no DSM segment can be created, a ParallelContext can still be
created with zero workers, but its seg pointer is NULL. Parallel
autovacuum registered a segment detach callback on that NULL pointer,
crashing the autovacuum worker.
Fix by setting up the shared cost parameters and the detach callback
only when the parallel context has workers, as there are no workers to
propagate the parameters to otherwise.
Oversight in commit 1ff3180ca0.
Found by Bharath using AI assisted review with Claude.
Reported-by: Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com>
Author: Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com>
Discussion: https://postgr.es/m/CALj2ACVjBN03Fq4XWgKH1zOfkapK6-P3ufNLHUHnZcW1zPm+Mg@mail.gmail.com
Backpatch-through: 19
Presently, s_lock.h insists that every platform provide TAS(), and
its header comment doesn't say which of the macros a new platform
actually needs to supply. The real contract is that a platform
must provide either S_LOCK() or a TAS() for the default S_LOCK() to
be built on.
To better express that, this commit moves the "no spinlock support"
error to where the default S_LOCK() is defined, compiles s_lock()
only when the default S_LOCK() is in use, and provides a default
TAS_SPIN() only when there is a TAS() to base it on. The direct
callers of s_lock() in the spinlock tests are adjusted to match.
The header comment is reworked accordingly, dropping the note about
pre-9.5 volatile requirements and the caution about TAS()
spuriously failing along the way.
Co-authored-by: Tom Lane <tgl@sss.pgh.pa.us>
Discussion: https://postgr.es/m/afkUeI7UhacZ5ZFm%40nathan
