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
In most of the backend code, XLogReaders open a WAL segment file with
BasicOpenFile() (xlogutils.c, WAL summarizer and WAL sender). On ERROR,
the opened file descriptor could leak while reading WAL segments, piling
them over time if a session can spawn XLogReaders at will, leading to
EMFILE with various consequences, the worst being a backend-side PANIC.
The problem has been reported as reachable with pg_walinspect (EXECUTE
privilege revoked from PUBLIC by default), or replication slot functions
(REPLICATION privilege required).
XLogReaderState gains a memory context reset callback, registered if a
segment_open callback has the idea to open a WAL segment with
BasicOpenFile(). XLogReaderRegisterResetCallback() can be used by a
segment_open callback to ensure the cleanup of any opened file
descriptor.
No backpatch is done. This breaks the ABI of XLogReaderState (sizeof),
and MemoryContextUnregisterResetCallback() is new in v19. There may be
an argument for fixing this issue in v19, but the release is close
enough that this change does not seem worth taking a risk for.
Author: Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com>
Reviewed-by: Bertrand Drouvot <bertranddrouvot.pg@gmail.com>
Reviewed-by: Sami Imseih <samimseih.pg@gmail.com>
Reviewed-by: Chao Li <li.evan.chao@gmail.com>
Reviewed-by: Michael Paquier <michael@paquier.xyz>
Discussion: https://postgr.es/m/CALj2ACVwDuOXXDjj2cVdnTKoxsgTSLDin4XoL63AnM6aUgMQaA@mail.gmail.com
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
Restoring each zstd-compressed full-page image currently creates and
destroys a decompression context. One decompression context gets now
saved in a XLogReaderState on first use, reused for the decompression of
any follow-up zstd-compressed pages.
The decompression context is freed in XLogReaderFree(). In non-FRONTEND
builds, a memory context reset callback is registered to make sure that
the decompression context does not leak. Note that zstd has APIs to
either initialize a pre-allocated decompression context or assign custom
allocation and free routines, but none of these are available for
builds that dynamically link to the zstd library (ZSTDLIB_STATIC_API).
Tomas Vondra has done some benchmarking showing that for a FPI-heavy set
of WAL records, reusing the decompression context can nicely improve the
recovery time, bringing it close to lz4. In my own measurements, I have
seen up to a 4% faster recovery time when replaying records made mostly
of zstd-compressed full-page images between HEAD and this improvement.
Andrey has seen better numbers than that.
Author: Andrey Borodin <x4mmm@yandex-team.ru>
Reviewed-by: Michael Paquier <michael@paquier.xyz>
Discussion: https://www.postgresql.org/message-id/asRsOl73nf8GeBFd@paquier.xyz
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
Insist that the input lexemes be already sorted and non-duplicate.
There's no good reason for them not to be, since the required ordering
is simple and platform-independent. The previous coding was willing
to re-sort them, but that left a gap: it didn't de-duplicate them.
De-duplicating would potentially require merging position lists,
which would be painful; we can't easily re-use tsvectorin()'s logic
for that, since it uses a different temporary representation.
Let's just dodge the problem by insisting the lexeme list be fully
correct as-presented, as we were already doing for the position lists.
Also, there was an oversight in the position-list checking: it did
not verify that the first position isn't zero. tsvectorin() rejects
that, so this should too.
While these problems allow tsvectorrecv() to construct tsvectors
that would not be generated internally, the vectors aren't so broken
that we can't work with them. Therefore, the best course of action
seems to be to apply these tighter rules in master but not back-patch.
Back-patching might break application workflows that aren't causing
any serious problems. Any such users will need to clean their data
before loading it into PG >= v20, but they'd face that issue anyway,
and a major update is a better time for it than a minor update.
Bug: #19748
Reported-by: Ke <kehan5800@gmail.com>
Author: Tom Lane <tgl@sss.pgh.pa.us>
Discussion: https://postgr.es/m/19748-6075f0508fa1ffc0@postgresql.org
This backwards-compatibility hack was introduced twenty years ago (in
8.2), and has surely been of no value for at least the last ten years.
However, we shouldn't just leave it sit, because as was just pointed
out it creates a dump/reload hazard: if it's set in the receiving
environment then array contents will not be restored correctly.
Other applications would likely be broken by it too, so whatever
residual value it may have seems outweighed by the possibility of
misuse.
Our precedent for retiring some other comparable parameters, such as
standard_conforming_strings, has been to leave the parameter in
existence but force it to a fixed value. I judge however that
that's not worth the trouble here, because there's no reason to
believe that a non-negligible number of applications are either
setting or reading this value. Let's just delete it in the master
branch and be done with it.
Bug: #19747
Reported-by: Ke <kehan5800@gmail.com>
Author: Tom Lane <tgl@sss.pgh.pa.us>
Reviewed-by: Laurenz Albe <laurenz.albe@cybertec.at>
Discussion: https://postgr.es/m/19747-69b01e7fc190cd58@postgresql.org
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
unconstify_constexpr is like unconstify but for contexts where a
constant expression is required, such as for initializing global
variables. It provides the same level of checking as unconstify, but
the checking only works on GCC, otherwise it lets anything through.
Also, if the check fails, it gives a less clear error message. So it
should only be used when necessary.
(An unvolatize_constexpr doesn't seem necessary, but it could be added if
required.)
Reviewed-by: Rui Zhao <zhaorui126@gmail.com>
Discussion: https://www.postgresql.org/message-id/c76b72dd-5736-453a-b698-6968d42e4768%40eisentraut.org
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
A smart or fast shutdown during an early crash restart can wait forever
on the checkpointer and I/O workers. FatalError is still set, so
PM_STOP_BACKENDS adds them as processes to wait for, but these are
relaunched by the restart and ignore the SIGTERM that a shutdown sends.
AbortStartTime has been reset at that point, meaning that SIGKILL would
also not fire.
SIGQUIT is now sent instead of SIGTERM when FatalError is set, via
HandleFatalError(), arming AbortStartTime. Going through that routine
rather than signaling the children directly matters for the startup
process: TerminateChildren() marks the startup process as signaled, so
its exit is not later reported as a failure. Note that after sending
the SIGTERM, we check for any running processes, assuming that auxiliary
processes have been already SIGQUIT'd for an immediate shutdown or for a
FatalError.
A test case is added, able to stress the scenario of a shutdown
triggered during the early phases of crash recovery with a custom
restore command waiting for a shutdown by scanning the server logs. The
CI is stable with this test (several runs done); the buildfarm will be
the final judge regarding its stability.
No backpatch is done for now, to be on the conservative side. This
feels also a slightly risky change for v19, which is close to release,
and letting this change bake on HEAD a bit would not hurt.
This problem has been originally reported by Justin Pryzby back in 2023,
without being fixed, and still existed on HEAD.
Reported-by: Justin Pryzby <pryzby@telsasoft.com>
Author: Ayush Tiwari <ayushtiwari.slg01@gmail.com>
Co-authored-by: Michael Paquier <michael@paquier.xyz>
Discussion: https://postgr.es/m/CAJTYsWXCLi00cJ6-_OkMJB4UjyV6EZjc86QwmgE9=iH_C0R-Aw@mail.gmail.com
Discussion: https://postgr.es/m/ZWlrdQarrZvLsgIk@pryzbyj2023
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
pull_up_simple_subquery() rechecks is_simple_subquery() after pulling
up the subquery's own subqueries, but it flattened join alias Vars in
the subquery's targetlist only after that, and never in its quals. A
join alias Var within the subquery, such as a whole-row Var of a join,
can expand to an expression containing lateral references to the outer
query once the join's inputs have been pulled up, and until it is
flattened is_simple_subquery() cannot see them. We could thus pull up
a LATERAL subquery whose targetlist or quals reference rels outside
the lowest outer join above it. This leads to assertion failures in
distribute_qual_to_rels() or "wrong phnullingrels" errors, and in some
cases to wrong query results, where rows that should have been
preserved by an outer join above the subquery are discarded.
To fix, flatten join alias Vars in the targetlist, and in the jointree
quals if the subquery is LATERAL, before the recheck. If the recheck
fails, the flattened copy is discarded along with the rest of the
modified subquery, so no harm is done.
Back-patch to all supported branches, as this can lead to wrong query
results.
Author: Richard Guo <guofenglinux@gmail.com>
Reviewed-by: Tatsuya Kawata <kawatatatsuya0913@gmail.com>
Discussion: https://postgr.es/m/CAMbWs48GFZ=3Bjc1ug9JRsB0Qupg+CGcb9js5vmTM6kY2OGWOg@mail.gmail.com
Backpatch-through: 14
This commit addresses three gaps in the error reporting data, as of:
- In _gin_parallel_merge(), for a parallel GIN build, the last key was
counted twice in the progress report. Oversight in 8492feb98f.
- _gin_process_worker_data() forgot to increment bs_numtuples when
trimming a list of pointers. Oversight in b229c10164.
- blocks_done for an index build's heap scan was incorrect for its first
block. There was a one-off in heapam_scan_get_blocks_done(), causing
the routine to return the total number of blocks on the first block
scanned. Oversight in ab0dfc961b.
Manu has reported two issues. I have come across the one in
_gin_process_worker_data() while reviewing his patches.
All of these are mostly cosmetic, leading to incorrect data reported in
the progress views for a short period of time, so no backpatch is done.
Author: Manu <manuelreyesbravo@gmail.com>
Author: Michael Paquier <michael@paquier.xyz>
Discussion: https://postgr.es/m/179012373464.1732093.11171496412950256250@gmail.com
Commit e5d25959cf added the repack_dropped test to Meson but omitted it
from the Makefile, so make-based isolation runs did not execute it. The
test's teardown also left the injection_points extension installed,
relying on a later test to remove it.
Add repack_dropped to the Makefile's isolation tests and drop the
extension during teardown.
Author: Fujii Masao <masao.fujii@gmail.com>
Reviewed-by: Álvaro Herrera <alvherre@kurilemu.de>
Discussion: https://postgr.es/m/CAHGQGwG9bHxhs4Nj+9avSjSGh0vdYNQgEuz+pmJ-H5be6tEBTw@mail.gmail.com
Backpatch-through: 19
Previously, dropping a NOT NULL column, adding a new column with the same
name, and making it a primary key in a single ALTER TABLE command could
fail with "primary key column ... is not marked NOT NULL". This happened
because ALTER TABLE checked for an existing NOT NULL constraint before
dropping the old column. Finding the constraint caused it to skip adding
one for the new column, even though the old constraint would be removed
with the old column.
Defer this check until after column drops and additions have been
executed, so that it examines the columns that will actually form the
primary key. Add any required NOT NULL constraints before creating the
primary-key index.
Backpatch to v18, where this issue was introduced by commit a379061a22.
Reported-by: Ludvig Janiuk <ludvig.janiuk@proton.me>
Author: Fujii Masao <masao.fujii@gmail.com>
Co-authored-by: Manuel Reyes Bravo <manuelreyesbravo@gmail.com>
Discussion: https://postgr.es/m/0a9b4c44-cdff-4ec8-a8fb-b3a8b59e9ba9@proton.me
Backpatch-through: 18
Since commit e28cf96e9, the copies of a query level's PlaceHolderVars
that were pushed down into its subqueries are preprocessed by that
level before its SubLinks are turned into SubPlans, so such a copy may
already contain SubPlans by the time the subquery is planned. That
commit taught build_subplan not to process the SubLinks of a PHV that
is passed down as a SubPlan argument, but overlooked that such a copy
can also appear within the arguments of an outer-level Aggref,
GroupingFunc, or ReturningExpr. The SubLinks in those arguments are
still processed when build_subplan collects the node, and that
processing descended into the already-preprocessed PHV and tripped the
no-SubPlan assertion in process_sublinks_mutator.
To fix, don't descend into PHVs when processing the SubLinks in such a
node's arguments. Any PHV found there is a pushed-down copy that its
owning level has already preprocessed.
Back-patch to v16, as with commit e28cf96e9.
Author: Ayush Tiwari <ayushtiwari.slg01@gmail.com>
Reviewed-by: Richard Guo <guofenglinux@gmail.com>
Discussion: https://postgr.es/m/CAJTYsWW8THbrnsmYnDqnUgg0PjGrqw2vNOWb8qx7eu191KPW3g@mail.gmail.com
Backpatch-through: 16
