When a Gather or Gather Merge node is rescanned, it starts a new set of
workers, which write their Memoize stats into the same shared memory
slots as the previous worker. ExecEndMemoize() used memcpy() for this,
so EXPLAIN ANALYZE showed only the stats for the final scan.
Here we adjust ExecEndMemoize() to accumulate the counters so that we
gather up the totals over all rescans.
Author: Tatsuya Kawata <kawatatatsuya0913@gmail.com>
Reviewed-by: David Rowley <dgrowleyml@gmail.com>
Discussion: https://postgr.es/m/CAHza6qeA-4vB355co-2yAxDMRTpEneY0rg9CD2zMLYscYvv2UA@mail.gmail.com
Backpatch-through: 14
match_clause_to_partition_key() tells whether a comparison value can
be evaluated during executor-startup pruning by checking that it has
no Vars, no volatile functions and no PARAM_EXEC Params. A SubPlan
normally fails the last test, since the testexpr of an ANY/ALL SubPlan
references the subplan's output Params. But if the left-hand side is
a NULL constant, the strict comparison operator in the testexpr is
const-folded to a NULL Const, and an uncorrelated SubPlan has no
parParam or args either, so nothing is left for pull_exec_paramids()
to find. The expression then reaches initial_pruning_steps, where
ExecDoInitialPruning() must evaluate it before any PlanState exists,
and we fail with "SubPlan found with no parent plan".
To fix, don't allow such an expression to be used for startup-time
pruning. It's still fine for per-scan pruning, which has a PlanState
to evaluate it with. Expressions containing exec Params are
unaffected.
This has been broken since d47cbf474, which moved initial pruning out
of ExecInitNode(); before that, pruning always had the parent node's
PlanState available. Back-patch to v18.
Author: Richard Guo <guofenglinux@gmail.com>
Reviewed-by: Amit Langote <amitlangote09@gmail.com>
Discussion: https://postgr.es/m/CAMbWs48nYDu7joYeyMKYdX8kHoL_ASEax2fqZH6L+HHqO2nkXA@mail.gmail.com
Backpatch-through: 18
If a proposed join has lateral references to rels outside it,
join_is_legal() checks that those rels can still be joined to from
outside. To do that it collects the rels that must end up on the
inner side of an outer join with the proposed join, directly or
indirectly, and rejects the join if any of its lateral references
point to one of them.
That search followed only outer joins, from min_lefthand to
min_righthand. It failed to notice that a rel that laterally
references the proposed join, or any rel found by the search, cannot
supply a parameter to the proposed join either, and that the same
holds for rels on the inner side of an outer join with such a rel. As
a result, we could approve a join whose lateral references could never
be satisfied.
Such a join can never appear in a complete plan. Worse, as the
existing comment warns, its apparent legality could mislead the
clauseless-join heuristics into thinking that some other join rel need
not be formed. In particular, have_join_order_restriction() would not
force a clauseless outer join if one of its input rels appeared to
have a legal clause join elsewhere, and if that outer join was
required, we would fail with "failed to build any N-way joins".
To fix, also include in the search any input rel of the current join
search that laterally references a rel already found. The input rels
are taken from root->initial_rels rather than from the query's base
rels, because an input can be the joinrel of a sub-joinlist, as
happens for a full join or when from_collapse_limit or
join_collapse_limit prevents flattening. Such a joinrel is joined as
a unit, so it has to be included as a whole.
Back-patch to all supported branches.
Author: Richard Guo <guofenglinux@gmail.com>
Reviewed-by: Tender Wang <tndrwang@gmail.com>
Reviewed-by: wenhui qiu <qiuwenhuifx@gmail.com>
Discussion: https://postgr.es/m/CAMbWs4_p6iWbsjNcYpd6-6mWFnog3UJ2fZKrxWAQTsyNoyeQTw@mail.gmail.com
Backpatch-through: 14
When pull_up_sublinks() converts a sublink to an antijoin, it builds a
JoinExpr with no RTE, on the grounds that nothing above the join can
reference its RHS. That overlooks PlaceHolderVars. A PHV with no
Vars of its own is evaluated at its phrels, and for the outputs of a
pulled-up subquery phrels is all the relids in the subquery's
jointree, including the RHS of such a join.
A qual using the PHV thus appears to reference the RHS rels, and can
be pushed down into the RHS as a parameterized join clause. There it
has the opposite of the intended effect: outer rows that fail the qual
find no match and are emitted, giving wrong results. For an outer
join, including an antijoin made by reducing a left join, this cannot
happen because the join's relid is among the qual's relids. Before
v16, the qual's nullable_relids prevented it.
To fix, make a join RTE for an antijoin built from a sublink, so that
it has a relid like any other outer join. Semijoins are left as they
are: a qual evaluated within a semijoin's RHS does not change the
result.
Back-patch to v16. Before that, the qual's nullable_relids included
the antijoin's RHS, which kept the qual from being moved there; that
protection went away when nullable_relids was replaced by outer-join
relids.
Author: Richard Guo <guofenglinux@gmail.com>
Reviewed-by: Tom Lane <tgl@sss.pgh.pa.us>
Reviewed-by: wenhui qiu <qiuwenhuifx@gmail.com>
Discussion: https://postgr.es/m/CAMbWs4_rEdcG89BgC-AO7b+Bf2z8w3976+4zP==Urt=CXB1hsg@mail.gmail.com
Backpatch-through: 16
A WAL sender tracks its WAL reader and logical decoding context as two
static variables, which were left set after being freed for a
START_REPLICATION command. The WAL reader is used for in a physical
replication command, both the WAL reader and the logical decoding
context are used for a logical replication command.
Both variables are now cleared once their contents are released at the
end of each replication command, to keep the release of the resources
and the maintenance of the pointers close to each other. Some more
cleanup is done by WalSndErrorCleanup(), as well, once the WAL reader is
clear.
The assertion added by 879ad9f90e is moved before the execution of a
replication command, checking now that both the WAL reader and the
logical decoding context are never set before the execution of a new
command.
Author: Ayush Tiwari <ayushtiwari.slg01@gmail.com>
Reviewed-by: Michael Paquier <michael@paquier.xyz>
Discussion: https://postgr.es/m/CAJTYsWW+hdmmVt_A4hvGv2_5RWK8-kaOf5ncT65=5i_hyD=BHg@mail.gmail.com
Backpatch-through: 14
CheckTablespaceDirectory() did not close the directory opened with
AllocateDir(), leaving its resources held during subsequent recovery,
potentially indefinitely on a standby. Add the missing FreeDir() call.
Backpatch to all supported versions.
Author: Chao Li <lic@highgo.com>
Reviewed-by: Fujii Masao <masao.fujii@gmail.com>
Discussion: https://postgr.es/m/66A784B7-55C1-4613-8C24-B4C2A8D75414@gmail.com
Backpatch-through: 14
Building extended statistics of type "ndistinct" calls
ndistinct_for_combination() once for every combination of columns. Each
call allocates arrays for the whole sample and never frees them, with
memory accumulating until the whole statistics object is built. The
sample has 300 multiplied by target rows. With large statistics targets
(default_statistics_target up to 10k), this can add up to gigabytes
worth of memory, useless once a combination is computed.
ndistinct_for_combination() is now called in a dedicated memory context,
reset after each combination is computed. We only need the estimated
number of distinct values, so there is no need to copy anything out of
the memory context.
Similar fixes have been done in 83772cc78e and ad8a166ca8 for other
areas of the extended stats computations.
Author: Ilia Evdokimov <ilya.evdokimov@tantorlabs.com>
Discussion: https://postgr.es/m/20d6689e-f490-4ca7-a92f-e92f691ba07e@tantorlabs.com
Backpatch-through: 14
Commit 2ebf25e7d left query_planner's qp_callback call where
it had been historically, but that turns out to be a bad idea.
It means that the code reached via qp_callback is subject to the
must-be-able-to-restart rule created by looping back after join
removal. We found one place that fails that requirement:
adjust_group_pathkeys_for_groupagg sets Aggref.aggpresorted flags,
and there's nothing that will reset those in case of a query_planner
restart, yet the selected set of Aggrefs can change. There might be
other places with similar issues.
Fortunately, qp_callback's main charter is to compute/update pathkey
values, and that is not something that join removal depends on or
should depend on. (Whether join removal is valid is surely not
dependent on what order the rel might get scanned in.) Nothing else
within the restart loop depends on it either --- in other words,
the existing placement was a bit random to begin with. So we can
fix this problem just by moving that call to below the restart loop.
That costs nothing, indeed it saves some cycles by not having to do
that processing more than once. And removing an unstated requirement
from code that's not directly called by query_planner should help
forestall future bugs.
While the known bug only exists in v18 and up (older branches lack the
aggpresorted optimization), I've chosen to back-patch the code change
to v16 like 2ebf25e7d was. This is the sort of API requirement that
we don't really want to be different across branches.
Reported-by: shihao zhong <zhong950419@gmail.com>
Author: Tom Lane <tgl@sss.pgh.pa.us>
Reviewed-by: shihao zhong <zhong950419@gmail.com>
Discussion: https://postgr.es/m/CAGRkXqSOAT1yop44B8jwacL_91rxK=QuoU8Of3NLMb9NgAO2NA@mail.gmail.com
Backpatch-through: 16
extract_jsp_bool_expr applied De Morgan's laws incorrectly when
considering an AND operator underneath a NOT. If one AND input
was implementable as a GIN indexqual while the other was not,
we derived an incomplete indexqual, leading to some rows not
being returned that should be.
extract_jsp_bool_expr also failed to check for methods being
applied to literals, so that for example '$.x == "42".double()'
was treated as '$.x == "42"' not '$.x == 42'. The indexqual
thus selected the wrong rows, all of which would fail qual
recheck, leading to no rows being returned.
While we're here, improve the comments a trifle.
Reported-by: Chinmay Kanchi <cgkanchi@gmail.com>
Author: Tom Lane <tgl@sss.pgh.pa.us>
Discussion: https://postgr.es/m/CAJqPDh9Cm=x8eYZ1Jf=_avd2WEqc=0t_f7_SO4igo-CKPi25_Q@mail.gmail.com
Discussion: https://postgr.es/m/CAJqPDh-tu5KY66KU8cvAEg3DQrZLzDZwjkLLa42zD+_Xm1WJAg@mail.gmail.com
Backpatch-through: 14
A hash index scan whose target bucket is still being populated by a
split must read both buckets of the split: forward scans read the bucket
being populated (skipping the tuples that the split copied there) and
then the bucket being split, and backward scans read them in the
opposite order. _hash_readnext and _hash_readprev make the transition
from the first bucket to the second, but _hash_next did not: on reaching
the end of the first bucket, it ended the scan, so every matching tuple
in the second bucket was overlooked.
To fix, teach _hash_next to make the same transition, using helper
functions shared with _hash_readnext and _hash_readprev.
_hash_next only missed the transition when the last page that it read in
the first bucket had a match (otherwise _hash_readpage's loop made the
required transition). Getting wrong answers also needed a matching
tuple to be inserted into the bucket being populated _after_ the split
began. Forward scans then lost the older tuples in the bucket being
split. An incomplete split would keep the buckets in this state until
an inserter finished the split at some indeterminate later time. Note
that wrong answers didn't require an incomplete split.
Although this fix treats the bug as the omission of a required step in
_hash_next, the real problem is the way that page stepping works in
general. The logic for stepping to the next page appears both in
high-level routines like _hash_next and in low-level routines like
_hash_readnext and _hash_readprev. If anything, _hash_next already knew
too much about page stepping (cf. nbtree's _bt_next function). All of
the logic around stepping really belongs in one routine, modeled on
nbtree's _bt_steppage function, but that approach is much too invasive
for a backpatched fix.
Claude Code found this problem. The committed test case is a simplified
version of the one that it wrote to demonstrate this bug.
Oversight in commit 7c75ef57, which introduced page-at-a-time processing
of hash index scans.
Author: Peter Geoghegan <pg@bowt.ie>
Author: Amit Kapila <amit.kapila16@gmail.com>
Reviewed-by: Vignesh C <vignesh21@gmail.com>
Discussion: https://postgr.es/m/CAH2-Wz=4CJK9ysZzbzxBGGTkgAg9ib8cJyCx=9q_d1r8tf0ang@mail.gmail.com
Backpatch-through: 14
Commit c8d2f68cc taught expr_is_nonnullable() to treat a DistinctExpr
as never returning NULL. That is wrong: when neither input is NULL,
the result is NULL if the underlying equality operator returns NULL,
which nothing forbids. This could lead to wrong query results.
To fix, remove the DistinctExpr case, along with its test case.
While at it, test the relkind before calling has_subclass() in
var_is_nonnullable(), to avoid a useless syscache lookup for
partitioned tables.
Reported-by: Robert Haas <robertmhaas@gmail.com>
Author: Richard Guo <guofenglinux@gmail.com>
Discussion: https://postgr.es/m/CA+TgmoaLcsmc9weJr1yo1X1SwZJX1t4XXHj0SJzH8XztmN9Hdg@mail.gmail.com
Backpatch-through: 19
To avoid warnings and lines that extend beyond the right page
margin in the PDF documentation, we have zero-width spaces in
the source at strategic places. They are not visible to the
naked eye, but allow FOP to break the lines in sane places.
However, some of these zero-width spaces were in the middle of
text that people might copy and paste from the HTML documentation,
which leads to mysterious syntax errors.
Scanning the documentation for &zwsp; identified four places
that this patch improves:
- an ISO 8601 interval literal didn't actually need a line break
in either the US letter or A4 format, so simply remove that
zero-width space.
- three v18-era examples for uuid_extract_timestamp() and
uuid_extract_version() had zero-width spaces in the middle of UUID
literals. Instead, add regular spaces around the "::" cast symbols,
allowing FOP to break the lines there. This solution isn't
invisible in the HTML output, but it doesn't look bad.
Reported-by: Y. Saburov <y.saburov@gmail.com>
Author: Laurenz Albe <laurenz.albe@cybertec.at>
Reviewed-by: Tom Lane <tgl@sss.pgh.pa.us>
Discussion: https://postgr.es/m/179143326813.1061054.14714458907175925072@wrigleys.postgresql.org
Backpatch-through: 18
Commit 383eb21eb taught the planner to convert a NOT IN sublink to an
anti-join when the sub-select's outputs are provably non-nullable.
Since the sub-select has not been through any planner processing at
that point, a reference to a virtual generated column is still a plain
Var, and query_outputs_are_not_nullable() relied on the column's NOT
NULL constraint to prove it non-nullable.
That is not safe for a partitioned table. A partition can have a
generation expression different from its parent's, and it enforces the
NOT NULL constraint against its own expression, whereas a query on the
parent computes the column using the parent's expression. So the
column can be NULL when read through the parent despite the
constraint, and the anti-join would then produce wrong results, as it
returns rows that NOT IN should have rejected.
To fix, make var_is_nonnullable() ignore the NOT NULL constraint on a
virtual generated column of a partitioned table when consulting the
catalogs. The other sources of NOT NULL information are only used
after virtual generated columns have been expanded, so they are not
affected.
Author: Shihao Zhong <zhong950419@gmail.com>
Reviewed-by: Richard Guo <guofenglinux@gmail.com>
Discussion: https://postgr.es/m/CAGRkXqR7VXacmRP41UBaM5u4aseeCH5ECVmQgn_YS=L7=xdhTw@mail.gmail.com
Backpatch-through: 19
eval_const_expressions() can reduce a COALESCE, CASE, or NULLIF
expression to one of its inputs. The input, however, does not
necessarily expose the same typmod and collation as the expression it
replaces. In particular, an explicit COLLATE clause on one input
determines the collation of the whole expression, while the other
inputs keep their own collations. If the expression was reduced to
one of those other inputs, anything that relies on exprCollation(),
such as sorting or grouping on the expression, would use the wrong
collation and could produce wrong results.
For CASE and NULLIF this is a long-standing oversight. For COALESCE
it has become much easier to hit since commits 931766aae and
10c4fe074, which reduce a COALESCE with a single surviving argument,
or with a leading non-nullable argument, to that argument.
To fix, apply applyRelabelType() to the replacement in these cases, so
that it exposes the type, typmod, and collation of the expression it
replaces.
Back-patch to v19 only. The CASE and NULLIF cases exist in older
branches too, but they are not back-patched further, given the lack of
field reports and the fact that the fix visibly changes the typmod
exposed by a reduced CASE expression.
Reported-by: Shihao Zhong <zhong950419@gmail.com>
Author: Shihao Zhong <zhong950419@gmail.com>
Author: Richard Guo <guofenglinux@gmail.com>
Discussion: https://postgr.es/m/CAGRkXqR7VXacmRP41UBaM5u4aseeCH5ECVmQgn_YS=L7=xdhTw@mail.gmail.com
Backpatch-through: 19
infer_arbiter_indexes() classifies each inference element by its
const-simplified form: a plain Var is matched against the index's
plain attributes, and anything else against its expressions. Index
columns, however, were classified by indkey alone, so an expression
column stayed an expression even when const-simplification reduced it
to a plain Var. Since commit 10c4fe074, COALESCE on a NOT NULL column
is reduced to the column itself, so an inference element such as
COALESCE(b, '') became a plain Var that no longer matched the unique
index on that same expression, and the statement failed with "there is
no unique or exclusion constraint matching the ON CONFLICT
specification".
To fix, classify index columns the same way as inference elements: an
index expression that is simplified to a plain Var is treated as a
plain attribute. Also, teach infer_collation_opclass_match() to
compare such a column as an expression, so that an inference element
that specifies a collation or operator class can still match it.
As a consequence, an inference element naming the column directly now
matches such an index too, which is consistent with how an element
written as the expression already matches a plain index on the column.
Author: Shihao Zhong <zhong950419@gmail.com>
Reviewed-by: jian he <jian.universality@gmail.com>
Reviewed-by: Richard Guo <guofenglinux@gmail.com>
Discussion: https://postgr.es/m/CAGRkXqR7VXacmRP41UBaM5u4aseeCH5ECVmQgn_YS=L7=xdhTw@mail.gmail.com
Backpatch-through: 19
BackendWritebackContext is a purely per-process thing, so it feels
wrong to initialize it in the shmem init/attach functions. Move the
initialization to InitBufferManagerAccess() instead.
Backpatch to version 19, where the new shared memory callbacks system
was introduced. While this isn't new with that, it made the ShmemInit
functions more closely related to shared memory initialization than
before.
Reviewed-by: Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com>
Reviewed-by: Ashutosh Bapat <ashutosh.bapat.oss@gmail.com>
Discussion: https://www.postgresql.org/message-id/fb1bd612-4a58-4235-976f-2b2ef036bfff@iki.fi
Backpatch-through: 19
Converting xlog.c to the new shared memory allocation API left the
creation of the memory context used by wal_debug in the
XLOGShmemInit() callback, which now only runs in the postmaster or in
a standalone backend. Previously, XLOGShmemInit() ran in every child.
As a result, walDebugCtx was left uninitialized in child processes in
EXEC_BACKEND mode, and with wal_debug turned on, the backend crashed
on first WAL insertion.
Fix by moving the walDebugCtx initialization to a separate function
that is called in each backend that wishes to create WAL records. (It
cannot be created lazily on first WAL insertion, because it's done in
a critical section, and you cannot create a new memory context in a
critical section.)
Oversight in 9b5acad3f4.
Co-authored-by:: Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com>
Reviewed-by: Ashutosh Bapat <ashutosh.bapat.oss@gmail.com>
Discussion: https://www.postgresql.org/message-id/CALj2ACUNL4Xn3aBzvm4DTpdr_L0-AfL2agsghMRRy_ZTiKhxSA@mail.gmail.com
Backpatch-through: 19
When a table is partitioned, the previous code did not emit NO_GATHER
advice for the parent table, on the theory that doing this for the
child tables was good enough. However, it isn't. I believe what
happened here is that, during development, I was planning to enforce
NO_GATHER by clearing the consider_parallel flag for the children,
which would have been good enough to ensure that parallelism could
not be used at any higher level of the plan tree. Eventually, I
switched course, but failed to realize that this code needed to be
updated as a result. Do that, and add a regression test.
Backpatch-through: 19
Discussion: http://postgr.es/m/CA+TgmoYYHC4i-Gpzc9-JYHqih6pOK+6LTYFiNR-PioXG1nKkOQ@mail.gmail.com
pg_restore did not verify that zstd and LZ4 decompression completed
the final frame when reading custom and directory-format dumps. As a
result, it could accept a dump whose compressed data was truncated.
Track final-frame completion and reject incomplete compressed input.
For zstd, flush any internally buffered output before performing the
final check.
Add regression tests for truncated zstd and LZ4 data in custom and
directory-format dumps and also address a potentially empty error
message while in there.
Author: Chao Li <lic@highgo.com>
Co-authored-by: Daniel Gustafsson <daniel@yesql.se>
Reviewed-by: Daniel Gustafsson <daniel@yesql.se>
Discussion: https://postgr.es/m/5962B878-C43D-4EBC-9E95-1F945CE5E586@gmail.com
Backpatch-through: 19
pg_verifybackup failed to detect truncated compressed tar backups
for zstd, lz4 and gzip compression.
astreamer_zstd_decompressor checked whether ZSTD_decompressStream()
returned an error. However, a positive return value at end-of-input
means that the final zstd frame has not completed. Record the state
and verify at finalization that zstd completed the frame. Also flush
any pending internally buffered output before performing that check.
astreamer_gzip_decompressor did not check whether inflate() had
reached Z_STREAM_END before finalizing. Record whether inflate()
completed the gzip stream, and reject finalization if it did not.
astreamer_lz4_decompressor did not check whether LZ4F_decompress()
completed the final frame before finalizing. Record the status of
decompression and reject finalization unless the frame completed.
This prevents pg_verifybackup from accepting a truncated compressed
tar backup.
Author: Chao Li <lic@highgo.com>
Co-authored-by: Daniel Gustafsson <daniel@yesql.se>
Reviewed-by: Osama Abdul Qader <osamaabdulqader.cs@gmail.com>
Reviewed-by: Zsolt Parragi <zsolt.parragi@percona.com>
Reviewed-by: Japin Li <japinli@hotmail.com>
Discussion: https://postgr.es/m/5962B878-C43D-4EBC-9E95-1F945CE5E586@gmail.com
Backpatch-through: 19
For run-time partition pruning running during execution (rather than at
executor startup), EvalPlanQualStart() reuses the es_part_prune_states
from the parent EState. This could cause an issue when the EPQ recheck is
finished, its EState is cleaned up and the es_query_cxt is deleted by
FreeExecutorState(). That cleanup could free memory that was allocated
again in InitExecPartitionPruneContexts() when it was called for the EPQ's
EState, and naturally, those fields got allocated in the EPQ's
es_query_cxt MemoryContext, which overwrote the ones for the parent
EState. If pruning occurs again after the EPQ's EState was cleaned up,
then we could have accessed free'd memory. This certainly caused issues in
debug builds because we clobber freed memory, but may not have in
production, depending on if the MemoryContext being deleted results in a
free(). It may not if the allocation was on the context's init block and
the context was pushed onto context_freelists[] for reuse.
Here we fix this by adding an "initialized" field to PartitionPruneState
and exit early in InitExecPartitionPruneContexts() when the state is
already initialized.
This was broken by bb3ec16e1, but the breakage there is slightly
different as that commit neglected to do what was fixed in 8741e48e5
(which did get backpatched to v18 in 9a82a64ed).
Reported-by: Vladimir Savin <vladimir@encord.com>
Author: David Rowley <dgrowleyml@gmail.com>
Reviewed-by: Andrey Rachitskiy <pl0h0yp1@gmail.com>
Discussion: https://postgr.es/m/CAP2G_gGpV=rKMLuET4RW-qO41GvBvi6Czsjsj16eT9hAwHxG6w@mail.gmail.com
Backpatch-through: 18
With WAL streaming, pg_basebackup -Ft -z writes pg_wal.tar.gz as a
series of gzip members. The decompressor did not reset the inflate
state when one ended, so inflate consumed nothing more and the loop
never finished. This caused pg_waldump to hang on such backups and
as a consequence pg_verifybackup (which use pg_waldump) would hang.
A test is added to pg_verifybackup for checking a gzip-compressed
tar backup, which in turn will verify that pg_waldump works.
Author: Shihao Zhong <zhong950419@gmail.com>
Reviewed-by: Daniel Gustafsson <daniel@yesql.se>
Reviewed-by: Chao Li <li.evan.chao@gmail.com>
Reviewed-by: Andrew Dunstan <andrew@dunslane.net>
Discussion: https://postgr.es/m/CAGRkXqTcyuog_MzKqQm+am2xMefLFJTSKaRevnfypDWirCFvdA@mail.gmail.com
Backpatch-through: 19
Commit 5cd72cc0c5 made the VACUUM VERBOSE and autovacuum logs report
buffer usage from the pgBufferUsage delta, which includes the parallel
workers' usage, but left the I/O timings on the leader's
pgStatBlockReadTime and pgStatBlockWriteTime. So the log counted the
blocks the workers read and dirtied but not the time they spent on
them.
Fix this by taking the I/O timings from the same BufferUsage delta.
This also removes a small error in the leader's own timings, since
pgStatBlock{Read,Write}Time accumulate whole microseconds per I/O.
Although 5cd72cc0c5 was backpatched to v15, BufferUsage on v16 doesn't
track the I/O time of temporary relations, so the same fix there would
drop that time from the log. We don't think a reporting issue is worth
v16-specific code, nor worth leaving v16 unfixed between v15 and v17,
so backpatch to v17 only.
Found by Bharath using AI assisted review with Claude.
Reported-by: Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com>
Author: Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com>
Reviewed-by: Sami Imseih <samimseih.pg@gmail.com>
Reviewed-by: Chao Li <li.evan.chao@gmail.com>
Reviewed-by: Masahiko Sawada <sawada.mshk@gmail.com>
Reviewed-by: Shihao Zhong <zhong950419@gmail.com>
Discussion: https://postgr.es/m/CALj2ACXrqaHrYmGdpAoGKXtiY9Xw4o1dxSC+EELxn47GDO6HAQ@mail.gmail.com
Backpatch-through: 17
_bt_pagedel only builds a search stack when it needs one to locate the
downlink of the page that it's about to mark half-dead (and when it
doesn't already have one from an earlier sibling deletion that took
place during the same _bt_pagedel call). _bt_pagedel won't build a
search stack for the page its caller passes whenever it happens to be
one that an earlier interrupted VACUUM left half-dead (since there is no
downlink to remove). We nevertheless asserted that page deletion could
only build a search stack for that initial caller-passed page.
To fix, remove the assertion, which spuriously failed when a half-dead
leaf page was passed by the _bt_pagedel caller and its right sibling
page was empty/had to be considered for deletion during the same call
(necessitating building a search stack to locate the right sibling's
downlink).
Oversight in commit cd8c73a38, which refactored nbtree page deletion's
INCOMPLETE_SPLIT check.
Author: Peter Geoghegan <pg@bowt.ie>
Reported-by: Iliia Khaprov <i.khaprov@gmail.com>
Discussion: https://postgr.es/m/CAFO+O_p5So4MXwQ1VEcL9+9vGgODu11TTWcYq9RDbxnira8o2w@mail.gmail.com
Backpatch-through: 14
Commit 0d4391b265 taught the planner
to store information about the RTIs of SubqueryScan, Append, and
MergeAppend node elided from the plan using a new ElidedNode data
structure; an hour later, 7358abcc60
taught to save the RTIs of an Append node into another Append
node using a new child_append_relid_sets field. Regrettably,
despite the fact that both commits were written by me and the
fact that both were part of the same patch set, the latter commit
failed to deal with the case where an Append node first acquires
child_append_relid_sets != NIL and then gets elided. I believe that
I thought that was impossible, but the included test case shows that
it isn't.
The proper fix involves updating the definition of ElidedNode to
carry child_append_relid_sets, so this commit does that, breaking
ABI compatibility. That's fine for the master branch, but it's a
bit unfortunate to be doing do that so close to the planned ship
date for v19. However, shipping v19 without fixing this appears
(at least to me) to be even more unfortunate.
Backpatch-through: 19
Discussion: http://postgr.es/m/CA+TgmoYYHC4i-Gpzc9-JYHqih6pOK+6LTYFiNR-PioXG1nKkOQ@mail.gmail.com
Follow-up to bca67e5a3. The test added there changed the value of
oauth_validator_libraries, waited for the postmaster to apply it, and
verified the change on an existing background connection. But there's no
guarantee that the SIGHUP will have arrived and completed its work on
existing backends by that time, so the final test flaked.
We only care about the postmaster's state here, since we're testing the
GUC's effects on newly forked backends, so remove the background check
entirely.
Per buildfarm member serinus.
Reported-by: Alexander Lakhin <exclusion@gmail.com>
Discussion: https://postgr.es/m/a37a3cea-5bd8-4385-a1d4-bb89394c079c%40gmail.com
Backpatch-through: 18
CopyToJsonOneRow() stamped the row datum with the query's descriptor
only when the slot's descriptor was of type RECORD. A scan that does
not project returns the table's row type, so the keys came from the
table, not the query, losing column aliases and mislabelling rows
from UNION ALL branches with different column orders.
Fix by always using the query's descriptor. For non-virtual slots,
heap_copy_tuple_as_datum() can just be passed that descriptor, so
there is no extra cost. This is safe because a scan skips projection
only when tlist_matches_tupdesc() holds, so the layouts match.
Bug introduced in 7dadd38cda.
Reported-by: Shihao Zhong <zhong950419@gmail.com>
Reviewed-by: Shihao Zhong <zhong950419@gmail.com>
Reviewed-by: João Marcelo Detomini <joao.detomini@enterprisedb.com>
Discussion: https://postgr.es/m/CAGRkXqQHjxgufHPz86+dxGaQZt37hikduUArYtfbn2_3mryZAw@mail.gmail.com
Backpatch-through: 19
Commit 1f2245371e added a test for the case where logical decoding
is disabled and re-enabled while synchronizing a replication slot. Its
cleanup assumed the slot re-created on retry would still be temporary,
and so would be gone once the corresponding remote slot was
dropped. That isn't guaranteed: the catalog_xmin reserved on the
standby is not necessarily ahead of the remote slot's. In that case
the slot is persisted on the first retry, and the subsequent logical
decoding deactivation invalidates it on the standby rather than
dropping it, so the check that no synced slot is left behind fails.
Stabilize the cleanup by calling an extra pg_sync_replication_slots()
after dropping the remote slot and before disabling logical decoding on
the primary.
Per buildfarm member skink.
Reported-by: Fujii Masao <masao.fujii@gmail.com>
Suggested-by: Fujii Masao <masao.fujii@gmail.com>
Reviewed-by: shveta malik <shveta.malik@gmail.com>
Discussion: https://postgr.es/m/CAHGQGwFWXASACRnhHJgEdk8nWyLw4bLLDhiEiKSqBqd+tzJ2Pg@mail.gmail.com
Backpatch-through: 19
Given an Append or MergeAppend node, pgpa_build_scan needs to call
pgpa_make_scan for the node's own apprelids, and also for each element
of child_append_relid_sets. It did that, but it overlooked the fact
that the scan type applicable to the child scans might differ from
the scan type applicable to the parent scan. The problem can only
occur in one direction: an RTE_SUBQUERY (Merge)Append can appear on
top of a RTE_RELATION (Merge)Append, but not the reverse, so the
only failure mode here is to classify a partitionwise scan of a child
appendrel as PGPA_SCAN_ORDINARY rather than PGPA_SCAN_PARTITIONWISE,
leading to failure to include PARTITIONWISE(something) in the
generated advice string.
Backpatch-through: 19
Discussion: http://postgr.es/m/CA+TgmoYYHC4i-Gpzc9-JYHqih6pOK+6LTYFiNR-PioXG1nKkOQ@mail.gmail.com
Commit 7358abcc60 taught the planner to
store information about Append node consolidation in the final plan,
but it forgot that child_append_relid_sets needs to be adjusted
at setrefs.c time just like every other place where we store RTIs
in a plan tree. Fix that.
Although this can cause incorrect plan advice to be emitted in cases
involving multi-level partitioning, it seems more appropriate to put
the test case for it in pg_overexplain, so this commit does that.
Backpatch-through: 19
Discussion: http://postgr.es/m/CA+TgmoYYHC4i-Gpzc9-JYHqih6pOK+6LTYFiNR-PioXG1nKkOQ@mail.gmail.com
Pasting together sentences with placeholders is not suitable for
translation. Write out both variants instead.
Fix comma splice in the detail message while we're here.
ri_LockPKTuple() passed table_tuple_lock() the current command ID, read
at lock time. ExecLockRows(), which locks the row for the SPI path's
SELECT ... FOR KEY SHARE, uses the estate's es_output_cid, fixed when
the query starts and equal to the command ID of the query's snapshot.
The two differ if user code run between taking the snapshot and locking
the row, such as the equality function called by the index scan,
advances the command counter. If that code also updated the referenced
row, the lock reports TM_Invisible, and the check fails with "attempted
to lock invisible tuple", where the SPI path gets TM_SelfModified and
reports a foreign key violation.
To fix, use the snapshot's command ID in ri_LockPKTuple() to mirror
what ExecLockRows() does.
This is mostly hardening. With FK values that need a cast now checked
through SPI, the only user code that can run there is the opclass's
equality function, and it would have to write to the referenced table.
That's unlikely, but the fast path should still behave like SPI.
Discussion: https://postgr.es/m/CA+HiwqG79XK1oObdZ2AwT660CeJ6s3Mn4LrFPCme-k4L2rF_ag@mail.gmail.com
Backpatch-through: 19
The SPI path checks a foreign key with SELECT ... FOR KEY SHARE, which
ExecCheckXactReadOnly() refuses in a read-only transaction unless the
referenced table is temporary. The fast path locks the referenced row
without that check, so for example a deferred check that fires at
COMMIT after SET TRANSACTION READ ONLY succeeded, having locked a row
in a read-only transaction, where the SPI path fails with "cannot
execute SELECT FOR KEY SHARE in a read-only transaction".
Fix the fast path to refuse it the same way so the behavior matches
with checks going through SPI.
Discussion: https://postgr.es/m/CA+HiwqG79XK1oObdZ2AwT660CeJ6s3Mn4LrFPCme-k4L2rF_ag@mail.gmail.com
Backpatch-through: 19
Commit 6aba42c660 made slot synchronization skip persisting a new
slot if logical decoding got disabled after the remote slot
information was fetched. As noted in its XXX comment, the check
missed the case where the last logical slot on the primary is dropped
and re-created with the same name in the meantime. If the standby had
replayed both the deactivation and the re-activation by then, the
slot was persisted with a restart_lsn preceding the deactivation.
Subsequent synchronization cycles failed with "unexpected logical
decoding status change" when advancing the slot, which also stopped
the synchronization of all other failover slots, and the slot could
not be decoded after promotion.
Fix this by remembering the end LSN of the last replayed
XLOG_LOGICAL_DECODING_STATUS_CHANGE record that enables logical
decoding, and by requiring the remote restart_lsn to be at or after it
right after creating the local slot. The check is skipped until replay
reaches the remote restart_lsn, as we can't tell anything about WAL that
hasn't been replayed yet. Once the slot exists, a later deactivation
invalidates it, so remove the check before persisting the slot. We
compare the remote restart_lsn rather than the local one, so we may drop
a slot that would have been usable, but the retry in the next
synchronization cycle fetches fresh information.
Backpatch to v19, where commit 67c20979ce made logical decoding toggle
dynamically.
Reported-by: Nikolay Samokhvalov <nik@postgres.ai>
Reviewed-by: shveta malik <shveta.malik@gmail.com>
Reviewed-by: Nisha Moond <nisha.moond412@gmail.com>
Reviewed-by: Amit Kapila <amit.kapila16@gmail.com>
Reviewed-by: Zhijie Hou <houzhijie22@gmail.com>
Discussion: https://postgr.es/m/CAM527d_eV_BAYFiQnfZLSPfHoihye=nOi-OnAM_57pdH+F+gfA@mail.gmail.com
Backpatch-through: 19
LogicalDecodingCtl had no init callback so there is no guarantee that
its fields are initialized. Its startup function
StartupLogicalDecodingStatus() also returns without touching the
fields when wal_level is 'minimal'.
Initialize them in an init callback explicitly. This is not a bug fix
but we just shouldn't be depending on the allocator by accident.
Backpatch to v19, where logicalctl.c was introduced, since a
subsequent bug fix for slot synchronization adds a new field to
LogicalDecodingCtlData and relies on this callback to initialize it.
Reviewed-by: shveta malik <shveta.malik@gmail.com>
Reviewed-by: Nisha Moond <nisha.moond412@gmail.com>
Discussion: https://postgr.es/m/CAM527d_eV_BAYFiQnfZLSPfHoihye=nOi-OnAM_57pdH+F+gfA@mail.gmail.com
Backpatch-through: 19
The page a bit of a mess, with notes interpersed in the list of
parameters, and two "notes" subsections that weren't in the Notes
section.
Limit the commentary under "Description" and "Parameters" to a minimum;
move the existing text to appear in the "Notes" section. Both the
"Notes on Clustering" and "Notes on Resources" subsections, which were
under "Description", are moved to be under "Notes" instead. Add a new
"Notes on Concurrent Operation" subsection there, which now carries some
text that was in "Parameters", and gets some additional text to
(hopefully) explain resource consumption more clearly -- notably, the
hard limit of 104 million tuples that can be concurrently updated/deleted
before running into the palloc size limit.
Add appropriate cross-links.
Author: Radim Marek <radim@boringsql.com>
Author: Álvaro Herrera <alvherre@kurilemu.de>
Author: Shihao Zhong <zhong950419@gmail.com>
Backpatch-through: 19
Discussion: https://postgr.es/m/CAJgoLk+dodrwuwCuXERGYwgjQzSwrKd+xgitYrL32oWDt39zvA@mail.gmail.com
With synchronous_commit = remote_apply, a commit could remain blocked even
after the synchronous standby had applied it when
wal_receiver_status_interval was set to zero. With both
wal_sender_timeout and wal_receiver_timeout disabled, the wait could last
indefinitely.
Commit 400a790a48 introduced this regression while avoiding unnecessary
duplicate standby status replies. It changed apply notifications from forced
replies to replies that check for WAL position changes. However,
XLogWalRcvSendReply() still returned early for non-forced replies when
wal_receiver_status_interval was zero, before checking the apply position.
Fix this by allowing apply-notification requests to bypass the early return
while preserving the existing checks that suppress unnecessary duplicate
replies.
Author: Vaibhav Dalvi <vaibhav.dalvi@enterprisedb.com>
Reviewed-by: Ashutosh Sharma <ashu.coek88@gmail.com>
Reviewed-by: Fujii Masao <masao.fujii@gmail.com>
Discussion: https://postgr.es/m/CA+vB=AHKUhc9GCDwoLr=SshTBCQG09kp8TEXew50UtD5rYqowg@mail.gmail.com
Backpatch-through: 19
Commit e5d25959cf made REPACK null-out values of dropped columns in
the NEW tuple of an UPDATE. However, a concurrent INSERT can carry one
too, for example when a trigger returns a copy of an existing row.
REPACK (CONCURRENTLY) then kept the values for such columns, which is
wasteful -- or worse, failed with "row is too big" if the new heap
happens not to have a TOAST table.
Move their clearing from prepare_concurrent_update() to restore_tuple()
so that it affects all tuples processed, not just the UPDATE ones.
Author: Shihao Zhong <zhong950419@gmail.com>
Backpatch-through: 19
Discussion: https://postgr.es/m/CAJgoLkK2UBzB1J9buCsSUbjf7bOqz-o_0=CeiTruU789BhBw-Q@mail.gmail.com
When a join alias Var with nonempty varnullingrels expands to an
expression that cannot carry the nullingrels itself, such as the
RowExpr of a whole-row Var, add_nullingrels_if_needed() wraps it in a
PlaceHolderVar. The choice of the PHV's phrels had several problems.
First, phrels was taken from all Vars in the expression. If a LATERAL
subquery within the join has been pulled up, the expression can
contain lateral references to rels outside the join, so the PHV could
be evaluated above the outer join that is supposed to null it. This
gave wrong results, or "wrong phnullingrels" errors.
Second, for a variable-free expression we fell back to the join's
relids but removed the join's own outer-join relid. That leaves a set
spanning both sides of the outer join without the join itself, which
can make the PHV look nullable by that join and cause "wrong
phnullingrels" errors.
Third, that fallback raised "unsupported join alias expression" when
the Var is an outer reference from a subquery. The restriction is
unnecessary: every planner caller passes root->parse as the query, so
the Var always belongs to root->parse regardless of its varlevelsup.
To fix, restrict phrels to the rels within the join, and if nothing is
left, evaluate the PHV at the join including its outer-join relid, at
any varlevelsup. Add an assertion that the query is root->parse.
Author: Richard Guo <guofenglinux@gmail.com>
Discussion: https://postgr.es/m/CAMbWs49q_gvoeOoF0+KZ-DpQaDb=nGFfSgrz2V_n1_64L4ZNVQ@mail.gmail.com
Backpatch-through: 16
When a query has aggregates or GROUP BY, parseCheckAggregates()
flattens join alias Vars in the grouping expressions, target list and
HAVING clause so that aliased and unaliased references compare equal.
The parser has no PlannerInfo, so this relies on every join alias
expression being able to carry varnullingrels without a PHV. That
doesn't hold for a whole-row Var of a join on the nullable side of an
outer join: it expands to a RowExpr, and pushing the nulling bits down
into its fields would turn a NULL row into ROW(NULL, ...). Such
queries therefore failed with "unsupported join alias expression".
To fix, leave a nulled whole-row join Var unexpanded when no root is
available. The planner expands it later and wraps it in a
PlaceHolderVar as before. Nothing is lost for GROUP BY matching: a
nulled whole-row Var can only be equal to the same Var, never to an
expression over the join's columns.
This has been broken since the introduction of varnullingrels in v16.
Author: Richard Guo <guofenglinux@gmail.com>
Discussion: https://postgr.es/m/CAMbWs49q_gvoeOoF0+KZ-DpQaDb=nGFfSgrz2V_n1_64L4ZNVQ@mail.gmail.com
Backpatch-through: 16
