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
reindexdb batches the REINDEX commands of all indexes attached to the
same table into a single multi-statement query when using multiple jobs,
as two steps with query "gen" and "run".
Such a query fails with REINDEX CONCURRENTLY, as this command cannot run
inside a transaction block. Batching cannot be avoided because the
indexes of one table have to be re-indexed one at a time to avoid
conflicts across jobs.
Let's error out cleanly on the reindexdb side if this combination of
options is used, instead of having the server complain about that. This
case has not been considered by 47f99a407d.
If there is interest in supporting this combination of options, the code
should be redesigned for this purpose with some better batching, rather
than try to use sets of "gen" and "run" routines. As currently written,
an index-level reindexdb uses a set of "gen" steps with one final "run"
step, which is nice code-wise. (Note: I doubt that that this would be
worth the effort.)
Author: Kirill Reshke <reshkekirill@gmail.com>
Discussion: https://postgr.es/m/CALdSSPiEkAmS+n7Ea9XDMrLjkbQk=CrB7zw1kcq9B47hfKUhXw@mail.gmail.com
Backpatch-through: 17
As of a32b8dc98 and friends, amcheck will complain about bpchar
indexes that claim to allow de-duplication. This causes our
cross-version-upgrade tests to fail when upgrading from a version
that lacks those commits, since there are some such indexes in the
core regression tests. Drop such indexes from the upgrade source
database. (oidvector indexes would be problematic too, but
there aren't any user-defined indexes on oidvector in our tests.)
Author: Peter Geoghegan <pg@bowt.ie>
Reviewed-by: Tom Lane <tgl@sss.pgh.pa.us>
Discussion: https://postgr.es/m/CAH2-Wzk-oO3HjLaByo8Vpse6Tpha1mqAW8DFShh-kS-Wo59ugA@mail.gmail.com
Backpatch-through: 14
bpchar equality ignores trailing spaces, so equal values in a column
with no length specifier can have different images. Similarly,
oidvector equality intentionally pays no attention to the array lower
bound field. We nevertheless allowed nbtree deduplication to be used
with indexes built with bpchar_ops, bpchar_pattern_ops, or
oidvector_ops. A posting list split could therefore place a TID in an
index tuple whose key was equal to, but not bytewise identical to, the
corresponding table value. This allowed index-only scans to return
subtly wrong results.
To fix, drop support function 4 from the affected opclasses (we can't
change catalog contents on branches 14 through 18, so we just disable
deduplication by adding hard-coding to the relevant support functions).
Deliberately leave the oidvector opclass alone on branches 14 through
18, though. Disallowing oidvector deduplication on these branches would
make pg_amcheck report that pg_proc_proname_args_nsp_index has a corrupt
metapage, needlessly alarming users.
System catalog oidvector values always have a lower bound of 0, so the
issue can't cause harm there; a user can only hit it by indexing an
oidvector column that stores the output of a function like trim_array().
It seems very unlikely that any user would ever do that.
Users should REINDEX any B-Tree index on a bpchar column.
Author: Shihao Zhong <zhong950419@gmail.com>
Co-authored-by: Peter Geoghegan <pg@bowt.ie>
Reported-by: Ke <kehan5800@gmail.com>
Reported-by: Tom Lane <tgl@sss.pgh.pa.us>
Reviewed-by: Tom Lane <tgl@sss.pgh.pa.us>
Bug: #19749
Discussion: https://postgr.es/m/19749-d46adf7c7d910b07@postgresql.org
Discussion: https://postgr.es/m/CAGRkXqQQ311Lg=62x5UdZtPuRFgpT6XpPMw4X9E3tgbTu162Mw@mail.gmail.com
Backpatch-through: 14
seg.sgml claims that a seg can store 7 significant digits, but this
is nonsense. The underlying float4 data type is not (quite) that
precise, and seg_out() intentionally rounds off to at most FLT_DIG
decimal places. With IEEE float arithmetic, FLT_DIG will be defined
as 6, or at least that's the case on all platforms I know of.
AFAICT this thinko goes clear back to the introduction of seg
(commit a3694b420), but it wasn't questioned until now.
Bug: #19744
Reported-by: Ke <kehan5800@gmail.com>
Author: Tom Lane <tgl@sss.pgh.pa.us>
Discussion: https://postgr.es/m/19744-8c700582bbb1d075@postgresql.org
Backpatch-through: 14
Previously, a backend with an assigned XID deferred the
XLogLogicalInfo update requested by a procsignal barrier, and we
applied it in AbortTransaction() on abort. However, a failed
transaction block keeps its XID until ROLLBACK, so a barrier absorbed
while idle in that state was deferred, and CleanupTransaction() did
not apply it. The next transaction then ran entirely with the stale
value. If logical decoding had just been enabled, that transaction's
changes were written without logical information and silently skipped
by decoding.
Fix by applying the pending update in CleanupTransaction() instead of
AbortTransaction(). We call it after resetting the top-level XID
because, unlike the commit and prepare paths, CleanupTransaction()
isn't always called with interrupts held, so a barrier absorbed before
the reset would be deferred again and carried into the next
transaction.
Patch by Sergei Patiakin, with tests added by me.
Oversight in commit 67c20979ce.
Reported-by: Sergei Patiakin <sergei.patiakin@enterprisedb.com>
Author: Sergei Patiakin <sergei.patiakin@enterprisedb.com>
Reviewed-by: Shlok Kyal <shlok.kyal.oss@gmail.com>
Reviewed-by: Hayato Kuroda <kuroda.hayato@fujitsu.com>
Reviewed-by: Masahiko Sawada <sawada.mshk@gmail.com>
Reviewed-by: Bertrand Drouvot <bertranddrouvot.pg@gmail.com>
Discussion: https://postgr.es/m/CANE55rApeNAFaqxLWrvm-NC0Y5gkrBFZFVaHVYP89AGS2SYMcA@mail.gmail.com
Backpatch-through: 19
COPY FROM runs its WHERE clause through eval_const_expressions(),
which among other things replaces NOT above an operator with the
operator's negator, if it has one (e.g., NOT (a > 0) becomes
a <= 0). The node it builds for the negator has no opfuncid, and
since COPY FROM never calls fix_opfuncids() on the clause, nothing
fills it in before the executor needs it. So
COPY tab FROM stdin WHERE NOT (a > 0)
fails with "cache lookup failed for function 0".
To fix, call fix_opfuncids() on the clause, as the relcache does
for index predicates.
Oversight in commit 31f3817402.
Reviewed-by: Tom Lane <tgl@sss.pgh.pa.us>
Discussion: https://postgr.es/m/asPZRTci4oBvxR9A%40nathan
Backpatch-through: 14
When reloading the SSL configuration fails, the existing config is
left in place to keep the server from being inaccessible. The GUC
backing SNI, ssl_sni, was however changed. This could lead to new
connections trying to access non-existing configurations. Fix by
storing the ssl_sni setting at the time of initialization in the
hosts context and only use the stored value during the handshake.
The patch submitted by the reporter has been significantly reworked
and expanded. Additional test cases written by Rui Zhao.
Author: Anthropic OSS program
Co-authored-by: Daniel Gustafsson <daniel@yesql.se>
Co-authored-by: Rui Zhao <zhaorui126@gmail.com>
Reported-by: Anthropic OSS program
Reviewed-by: Zsolt Parragi <zsolt.parragi@percona.com>
Reviewed-by: Jacob Champion <jacob.champion@enterprisedb.com>
Reviewed-by: Rui Zhao <zhaorui126@gmail.com>
Discussion: https://postgr.es/m/20260920211619.9c.noahmisch@microsoft.com
Backpatch-through: 19
