Commit b53b88109f made the postmaster maintain reachedConsistency in
addition to the startup process. Since the startup process is forked
from the postmaster, it begins life holding whatever value the
postmaster last set.
On a crash reset the postmaster re-forks the startup process while its
own copy still says true: it clears that copy only on receipt of
PMSIGNAL_RECOVERY_STARTED, which the replacement process cannot send
before it exists. The replacement therefore starts out believing the
database is already consistent.
CheckRecoveryConsistency() then skips the minRecoveryPoint comparison
altogether, so hot standby is announced at redo start while replay may
be arbitrarily far behind minRecoveryPoint. Read-only connections are
accepted and answer from heap pages that were flushed ahead of the
replay position, returning wrong results with no error raised. The
same branch also runs XLogCheckInvalidPages() and
CheckTablespaceDirectory(), which are skipped as well, and
log_invalid_page() treats page references that are normal before
consistency as a PANIC.
Fix by clearing reachedConsistency in InitWalRecovery(), so that a
startup process never depends on the value it inherited. The
postmaster's own copy is deliberately left alone: forked backends read
it to choose the "not yet accepting connections" errdetail, and it
converges once the new startup process sends PMSIGNAL_RECOVERY_STARTED
and, on reaching minRecoveryPoint, PMSIGNAL_RECOVERY_CONSISTENT.
Successive crash resets alternate. A startup process that skips the
branch never sends PMSIGNAL_RECOVERY_CONSISTENT, so the postmaster's
copy stays false and the next reset forks a process holding the correct
value; that pass reaches consistency properly, which sets the
postmaster's copy back to true and re-arms the problem for the reset
after it. Roughly every other crash reset is therefore affected, not
just the first one. EXEC_BACKEND builds are unaffected, as
reachedConsistency is not carried in BackendParameters.
Backpatch to v18, where commit b53b88109f introduced this issue.
Reported-by: Eric Ridge <eebbrr@planetscale.com>
Author: Nikhil Sontakke <nikhil@planetscale.com>
Reviewed-by: Fujii Masao <masao.fujii@gmail.com>
Discussion: https://postgr.es/m/CA+UBoq2n2Zg9rKgMfUtUohzGssisF9cDeyjKqPrnRNFprEyX1Q@mail.gmail.com
Backpatch-through: 18
112c268380 restored failsafe vacuum's abandonment of a buffer
access strategy by clearing the ReadBuffersOperations' strategy
references. But it did so in lazy_scan_heap()'s main loop, meaning it
looped through all the ReadBuffersOperations once per block after
failsafe was engaged. Track it with a local flag and clear the strategy
only once.
Reported-by: Melanie Plageman <melanieplageman@gmail.com>
Discussion: https://postgr.es/m/CAAKRu_Zse14nSNeCgtnE1LUAH8Of7OmYR%2BCc3O_DAzxt3m6T-g%40mail.gmail.com
Backpatch-through: 18
The logic for asynchronous Append assumes that pending requests made for
subplans of an Append are drained during ExecReScanAppend. To ensure
that, commit 9e283fc85 modified postgresReScanForeignScan to drain such
a request if any, but failed to take into account that if such a request
was made for a subplan that is re-scanned with parameter changes or
pruned in the next round by runtime pruning, the postgres_fdw callback
function is called after ExecReScanAppend or never called, respectively.
This would cause such a request to remain even after ExecReScanAppend,
leading to incorrect results, an infinite loop, or an assertion failure.
To fix, modify ExecReScanAppend to, for each of the pending requests,
give the FDW a chance to drain that request using the existing
ForeignAsyncConfigureWait/ForeignAsyncNotify callback functions. This
makes the change made to postgresReScanForeignScan useless, so remove it
as well.
Back-patch to v14 where asynchronous Append was added.
Reported-by: Alexander Korotkov <aekorotkov@gmail.com>
Co-authored-by: Alexander Korotkov <aekorotkov@gmail.com>
Co-authored-by: Gleb Kashkin <g.kashkin@postgrespro.ru>
Co-authored-by: Etsuro Fujita <etsuro.fujita@gmail.com>
Reviewed-by: Alexander Pyhalov <a.pyhalov@postgrespro.ru>
Reviewed-by: Gleb Kashkin <g.kashkin@postgrespro.ru>
Discussion: https://postgr.es/m/CAPpHfduMOTnV5Zj2KGJ7zanL_10QvccZHtPUaDfJvBhsh9axnQ%40mail.gmail.com
Backpatch-through: 14
When pgstat_init_entry() fails due to an OOM in the DSA allocation,
pgstat_get_entry_ref() cleaned up the shared hashtable but forgot to
remove the local reference that pgstat_get_entry_ref_cached() had
already inserted into pgStatEntryRefHash.
Missing this cleanup would leave a backend with a stale local cache
entry whose entry_ref points to a NULL shared_stats. If
pgstat_gc_entry_refs() runs with this reference still around, it would
crash due to a pointer dereference.
The local reference is now removed before removing the shared entry,
the order being sensitive to pending interrupts.
Oversight in 8191e0c16a.
Author: Niall Newman <nn@turacolabs.com>
Discussion: https://postgr.es/m/2FDAA194-9CF3-4FD7-A450-F1A4BEB125F6@turacolabs.com
Backpatch-through: 15
VACUUM's wraparound failsafe mode exists to reclaim transaction IDs as
quickly as possible. 4830f10243 made the failsafe stop using the
BAS_VACUUM buffer access strategy so that the rest of the vacuum could
make use of all of shared buffers rather than being confined to the
small strategy ring.
However, when 9256822608 made vacuum's first heap pass use the read
stream, this was accidentally disabled. The read stream keeps its own
references to the buffer access strategy, so clearing vacrel->bstrategy
in lazy_check_wraparound_failsafe() no longer had any effect on the
reads issued by the first pass.
Fix this by adding clearing the BufferAccessStrategy reference actually
being used by the ongoing scan -- those in the ReadBuffersOperations
structs themselves.
Two things we accept rather than fix, as neither is worth the added
complexity given how rarely failsafe mode is reached:
- A small amount of read time for IOs that were already in progress when
the strategy was cleared may be attributed to IOCONTEXT_NORMAL instead
of IOCONTEXT_VACUUM. WaitReadBuffers() derives the IOContext from the
(now cleared) strategy, so the wait time of these in-flight IOs is
misattributed. This is bounded by the stream's look-ahead window and
happens at most once per vacuum, when the strategy is first cleared.
- The stream's buffer pin limit stays lower than it would have been had
no strategy been used at all. max_pinned_buffers is capped by the
strategy's pin limit when the stream is created and is not recomputed
when the strategy is cleared. Raising it would mean building a new,
larger ring, which would require first waiting for all in-progress IOs
to complete. That didn't seem worth it.
Reported-by: Jingtang Zhang <mrdrivingduck@gmail.com>
Discussion: https://postgr.es/m/CAPsk3_APRYVLhAJ5TMwdmpSx8W_%3DPHMm%3DPmKAvnC3gBrfNommQ%40mail.gmail.com
Backpatch-to: 18
Buildfarm member olingo intermittently failed this test, timing out while
waiting for the subscriber log to report an ERROR because
max_prepared_transactions is zero there. The test captured the log offset
only after issuing the publisher's
BEGIN/INSERT/PREPARE TRANSACTION/COMMIT PREPARED sequence.
Since streaming is enabled, the subscriber can receive and apply the
transaction, and log the expected ERROR, before that publisher SQL command
even returns, i.e. before the test captures the offset. The subsequent
wait_for_log() calls then searched only from a point after the message had
already been written, and timed out waiting for it.
Fix by moving the offset capture to before the publisher's transaction is
issued, ensuring it always precedes the point where the ERROR can appear
in the subscriber log.
Reported-by: Alexander Lakhin <exclusion@gmail.com>
Author: Zhijie Hou <houzj.fnst@fujitsu.com>
Reviewed-by: Amit Kapila <amit.kapila16@gmail.com>
Backpatch-through: 16, where test was introduced
Discussion: https://postgr.es/m/c43753d8-5265-4f77-83ff-9b1167276ec5@gmail.com
GetLocalPinLimit() and GetAdditionalLocalPinLimit(), currently in use
only by the read stream, previously allowed a backend to pin all
num_temp_buffers local buffers. This meant that the read stream could
use every available local buffer for read-ahead, leaving none for other
concurrent pin-holders.
In 18, this was reported as a sequential scan of a temporary table
failing with "no empty local buffer available": with
effective_io_concurrency >= 64 and default io_combine_limit and
temp_buffers, the scan's read stream budget,
(effective_io_concurrency + 1) * io_combine_limit, meets or exceeds
num_temp_buffers, so look-ahead could pin the entire local buffer pool.
Any additional pin request made while returning tuples -- such as
fetching TOASTed values from the table's (also temporary) TOAST table --
then found no unpinned buffer to evict.
Cap the local pin limit to num_temp_buffers / 4, providing some
headroom. This doesn't guarantee that all needed pins will be available
-- for example, a backend can still open more cursors than there are
buffers -- but it makes it less likely that read-ahead will exhaust the
pool.
This is a backpatch of commit da6874635d, which was applied to master
only during the 19 development cycle, where the issue surfaced as a
regression test failure after on-access pruning began setting the
visibility map. Reports of the failure above on 18 prompted
backpatching it now.
No backpatch to 17, even though its read stream also allows a single
stream to pin the whole pool in principle. There, sequential scans pass
READ_STREAM_SEQUENTIAL, which disables fadvise-based look-ahead, so the
look-ahead distance cannot grow past io_combine_limit (at most 32
buffers, versus a minimum temp_buffers of 100) and the reported failure
is unreachable. The remaining theoretical paths require adversarial
settings and have never been reported. Moreover, 17 predates these
functions; the fix would have to be reimplemented in
LimitAdditionalLocalPins(), making the change not a simple cherry-pick.
Reported-by: Induja Sreekanthan <indujas@google.com>
Reported-by: Eduard Stepanov <crtxcz@gmail.com>
Reported-by: Feike Steenbergen <feikesteenbergen@gmail.com>
Reported-by: Alexander Lakhin <exclusion@gmail.com>
Reviewed-by: Xuneng Zhou <xunengzhou@gmail.com>
Reviewed-by: Andres Freund <andres@anarazel.de>
Discussion: https://postgr.es/m/97529f5a-ec10-46b1-ab50-4653126c6889%40gmail.com
Discussion: https://postgr.es/m/flat/CAFMO8-rYPSJbXsDdWDzDdpNi-fQ%2B6bKvgbXwE%2BR%3DsGko4epq0Q%40mail.gmail.com
Backpatch-through: 18
seq_search_localized() returns the length of the matching prefix in
*len, but because it internally case-folds the inputs, it gets
confused on the length. The caller expects to get the length of the
prefix in the original string, but what it actually returns is the
length of the prefix after case-folding, which can be different if the
case-folded characters have different byte-length than the original,
or with ICU, if the case-folding changes the number of characters
(e.g. "ß", the German double s).
To fix, once we have determined that we have a match, work harder to
find the match's length in the original string. This adds some
overhead, but the strings are expected to be short.
The function does "case-folding" by converting a string to upper-case,
then to lower-case, which is a little ugly given that we have
dedicated functions for case-folding nowadays. But switching to that
doesn't seem appropriate to backpatch in a security fix, and that's
not available in older stable versions, anyway.
Author: Heikki Linnakangas <heikki.linnakangas@iki.fi>
Reported-by: Xint Code
Reviewed-by: Jeff Davis <pgsql@j-davis.com>
heap_force_common() tracked the current position in the caller-supplied
tid[] using OffsetNumber, which is only 16 bits wide, so when the array
held more than 65535 entries, the updated index wrapped around and the
outer loop never reached the exit condition. A SQL call with a
sufficiently large TID array would then run until interrupted.
Fix by tracking the tid[] position using int instead of OffsetNumber.
A regress case based on the report is included.
Author: Andrey Rachitskiy <pl0h0yp1@gmail.com>
Reviewed-by: Andrey Borodin <x4mmm@yandex-team.ru>
Reported-by: Yuelin Wang <1217816127@qq.com>
Backpatch-through: 14
Bug: #19607
Discussion: https://postgr.es/m/19607-2f256a66481c514b@postgresql.org
Commit f6f0542266 changed getNotify(),
getParameterStatus(), and related libpq message-processing paths to
abandon the connection on out-of-memory errors.
However, getCopyDataMessage() and pqFunctionCall3() did not handle
this new fatal-error state. Both can process asynchronous
NotificationResponse and ParameterStatus messages while waiting for
other responses. If one of those messages triggered a fatal error, these
loops continued processing instead of reporting it immediately.
Fix this by checking for a saved fatal error after processing an
asynchronous message. If the connection has been abandoned, return the
appropriate error immediately instead of continuing to parse input.
Backpatch to v18, where commit f6f0542266 introduced this issue.
Author: Anthonin Bonnefoy <anthonin.bonnefoy@datadoghq.com>
Reviewed-by: Ewan Young <kdbase.hack@gmail.com>
Reviewed-by: Fujii Masao <masao.fujii@gmail.com>
Discussion: https://postgr.es/m/CAO6_XqpGfm+XHE1OzS=_+jroeDOxhhGa11P3cbm9q2gT05yorA@mail.gmail.com
Backpatch-through: 18
Per commit 8bf6ec3ba3, a column can be GENERATED only if it is such in
the whole inheritance tree.
For this reason, ATPrepDropExpression refuses to be called with ONLY on
a partitioned table. To detect this, the current implementation checks
whether recurse is set to false and the rel has direct children.
Recursion is implemented with ATSimpleRecursion, which calls ATPrepCmd
with recurse = false for every node in the tree. Inner nodes (for
example a partition which itself has subpartitions) then fail the check,
accidentally preventing the command from working on inheritance trees of
depth > 2.
This commit fixes it by also checking that we're at the top level of the
recursive calls using the recursing parameter, which is always true when
called through ATSimpleRecursion, always false when invoked on the root
rel.
Also, remove a comment claiming that DROP EXPRESSION could be
implemented with some effort. It cannot, as the commit message for
8bf6ec3ba3 explains.
Author: Alberto Piai <alberto.piai@gmail.com>
Backpatch-through: 14
Discussion: https://postgr.es/m/DHMT78XOD8BK.341V3H87KZ7NO@gmail.com
Fix missing NULL check in MemoryContextAllocAligned(). The underlying
call to MemoryContextAllocExtended() could return NULL when
flags contains MCXT_ALLOC_NO_OOM and the underlying malloc fails.
There are no current callers using MemoryContextAllocAligned() that pass
the MCXT_ALLOC_NO_OOM in core, so no live bug fix in core here. However,
an extension might use this pattern, so we'd better fix.
Fix this so we correctly pass the NULL to the caller rather than trying
to write to a NULL memory address.
This also fixes the same bug in AlignedAllocRealloc(), which is also
unused in core.
Backpatch to v16, where these functions first appeared.
Author: Chao Li <li.evan.chao@gmail.com>
Discussion: https://postgr.es/m/07DAC4C3-120D-4F3C-8FEE-BA236F7E9C1D@gmail.com
Backpatch-through: 16
Commit 6566133c5f added a case for AuthMemRelationId in
AcquireDeletionLock(), but not ReleaseDeletionLock(). The fall-through
case would go to UnlockDatabaseObject(), which would raise a WARNING;
and the lock would be retained until the end of the transaction.
Add the missing branch.
Discussion: https://postgr.es/m/2487ddcd737d4fc8e408e87aa9ad4365eed3bbb3.camel@j-davis.com
Backpatch-through: 16
Logging connection information, even at DEBUG1, creates unnecessary
risks. Remove the entire log message because it had no other useful
content.
Addresses finding 14 in report from linked discussion.
Reported-by: Noah Misch <noah@leadboat.com>
Discussion: https://postgr.es/m/20260710195902.4f.noahmisch@microsoft.com
Backpatch-through: 14
In the not-too-likely scenario where we successfully created a hash
table entry for a TS dictionary, but then failed to make a small
memory context for it, we left the hash entry in existence but with
a garbage value for dictCtx. This confused the code the next time
through, leading to a crash. Rearrange things so that we leave
the hash entry in a well-defined state with dictCtx == NULL, and
then the next try knows it still needs to make a memory context.
Reported-by: Alexander Lakhin <exclusion@gmail.com>
Author: Tom Lane <tgl@sss.pgh.pa.us>
Discussion: https://postgr.es/m/0f3ddeb5-0dbd-479c-9d0e-ae254758e624@gmail.com
Backpatch-through: 14
Allocate CompoundAffix with room for its terminator, initialize the
old-format flag buffer before NIAddAffix(), and reject incomplete or
missing Hunspell AF aliases. None of these errors would be likely to
trigger on real dictionary files, accounting for the lack of previous
reports; but they're certainly bugs.
Bug: #19595
Reported-by: Michael Malis <michaelmalis2@gmail.com>
Author: Andrey Rachitskiy <pl0h0yp1@gmail.com>
Reviewed-by: Tom Lane <tgl@sss.pgh.pa.us>
Discussion: https://postgr.es/m/19595-7dc18b4e212c4757@postgresql.org
Backpatch-through: 14
Alberta (America/Edmonton) moved to permanent UTC-06 on
2026-06-18, which will affect their clocks beginning on 2026-11-01.
For lack of any clarity on the point, assume their TZ abbreviation
will be CST from that time forward.
Morocco (Africa/Casablanca) will move to permanent UTC+00,
without daylight saving time transitions, on 2026-09-20.
Backpatch-through: 14
The slow_down interval parsing code checks explicitly for overflow, but
since it does that after the signed overflow has already occurred, we
end up inviting undefined behavior from the compiler anyway.
Use checked arithmetic instead. set_timer() takes a long int in order to
interface nicely with libcurl, so use an int32 as the interval counter
and clamp to LONG_MAX during conversion to milliseconds.
Backpatch to 18, where libpq-oauth was introduced.
Reported-by: Andres Freund <andres@anarazel.de>
Reviewed-by: Daniel Gustafsson <daniel@yesql.se>
Discussion: https://postgr.es/m/qtclihmrkq67ach3xjxyi4qcksstin5qxwsnkqefkmotxwh4g6%40ae2bj6jvcmry
Backpatch-through: 18
(This is a late cherry-pick of 4e1e41733, now that the new suite has
proven its stability, to ensure coverage for this code in PG18 as the
later branches diverge.)
To better record the internal behaviors of oauth-curl.c, add a unit test
suite for the socket and timer handling code. This is all based on TAP
and driven by our existing Test::More infrastructure.
This commit is a replay of 1443b6c0e, which was reverted due to
buildfarm failures. Compared with that, this version protects the build
targets in the Makefile with a with_libcurl conditional, and it tweaks
the code style in 001_oauth.pl.
Reviewed-by: Dagfinn Ilmari Mannsåker <ilmari@ilmari.org>
Reviewed-by: Andrew Dunstan <andrew@dunslane.net>
Discussion: https://postgr.es/m/CAOYmi+nDZxJHaWj9_jRSyf8uMToCADAmOfJEggsKW-kY7aUwHA@mail.gmail.com
Discussion: https://postgr.es/m/CAOYmi+m=xY0P_uAzAP_884uF-GhQ3wrineGwc9AEnb6fYxVqVQ@mail.gmail.com
As previously coded, walsummarizer only wants to read WAL from a file
where the TimeLineID in the filename exactly matches the TimeLineID being
summarized. But in some cases, when a timeline switch occurs, the WAL file
from the old timeline is not archived, because it's never completely
filled, so the only way to obtain the contents of that last partial
segment is to read from the first segment on the new timeline. Teach
WAL summarizer to do that, and add a test case to make sure that it
works.
Reported-by: Nick Ivanov <nick.ivanov@enterprisedb.com>
Reviewed-by: Andrey Borodin <x4mmm@yandex-team.ru>
Tested-by: Amit Kapila <amit.kapila16@gmail.com>
Reviewed-by: Srinath Reddy Sadipiralla <srinath2133@gmail.com>
Reviewed-by: Zhijie Hou <houzj.fnst@fujitsu.com>
Reviewed-by: Thom Brown <thom@linux.com>
Discussion: http://postgr.es/m/CA+Tgmobr27GpKDZx3_ezW2+C5_g18i+jSK3sGF_cR-_ESv5N5A@mail.gmail.com
Backpatch-through: 17
adf97c156 allowed expression evaluation to perform hashing, and
subsequently 9ca67658d fixed a memory stomping bug in that commit
that caused unrelated-to-hashing expression op steps to stomp on the
intermediate hash value. The intermediate hash value needs to be
maintained when hashing multiple hash keys. 9ca67658d didn't quite get
things right when in "strict" mode when it aborted hashing early after
encountering a NULL hash key. What was meant to happen was that the
expression returns NULL directly to indicate to the caller the value
hashed to NULL. The problem was that any EEOP_HASHDATUM_FIRST_STRICT or
EEOP_HASHDATUM_NEXT32_STRICT op step that didn't belong to the final
key to be hashed would have its op->resnull and op->resvalue pointing to
the location to store the intermediate hash value. That's correct for
non-NULLs since we bit-rotate the intermediate value and continue hashing,
but with the strict case, when we get a NULL key, we immediately jump to
the "jumpdone" step. The problem is the jumpdone step expects the
ExprState resnull and resvalue fields to be set (as they would be if we
didn't abort hashing early due to the NULL), but when we aborted early,
the ExprState fields never got set. This would result in inserting
records into the hash table that would never match to any join partner,
which is a waste of CPU and memory.
Here we fix this by having EEOP_HASHDATUM_FIRST_STRICT and
EEOP_HASHDATUM_NEXT32_STRICT populate the ExprState resnull and resvalue
fields directly when the value to hash is NULL.
Although Hash Agg and Hashed Subplans do use hashing from ExprStates,
those were unaffected by this bug, as neither of those uses the STRICT op
steps.
Thanks to Tomas Vondra for finding the offending commit.
Reported-by: Dan Stefura <dstefura@bluecatnetworks.com>
Author: David Rowley <dgrowleyml@gmail.com>
Discussion: https://postgr.es/m/YQBPR0101MB89738FB972FBD02A3640C6D3D6C92@YQBPR0101MB8973.CANPRD01.PROD.OUTLOOK.COM
Backpatch-through: 18
Partition pruning for RANGE-partitioned tables could mistakenly prune
the DEFAULT partition in some cases when it was not valid to do so,
which could lead to rows missing from query results.
The only known cases where this could happen is when combining pruning
steps from an IS NOT NULL clause with other steps that matched to the
DEFAULT partition. This could occur due to RANGE partitioned tables
having two distinct internal representations for marking if the DEFAULT
partition should be scanned. The IS NOT NULL steps would mark the
"scan_default" boolean, but other steps created for different purposes
could mark a bound_offset Bitmapset, which would ultimately translate into
also scanning the default partition. This could all fail after multiple
steps were combined with a combine intersect operator, as that will
intersect the bound_offset bits and only set scan_default if all pruning
steps have that flag set. When both input steps to the intersect operator
had different representations of whether to scan the DEFAULT partition,
the resulting intersect step result would contain neither representation.
Here, we fix this by having the IS NOT NULL pruning result mark the
bound_offsets so that it uses both representations to mark that the
DEFAULT partition must be scanned.
Reported-by: Jacob Brazeal <jacob.brazeal@gmail.com>
Diagnosed-by: Jacob Brazeal <jacob.brazeal@gmail.com>
Author: David Rowley <dgrowleyml@gmail.com>
Discussion: https://postgr.es/m/CA+COZaDXrfTaBjLE=Z79MTaH6Xun1V4PeKxLvCNv8mXS8wn0rw@mail.gmail.com
Backpatch-through: 14
When restoring relation stats, pg_restore_relation_stats() rejected
calls with (reltuples < -1.0). But that is insufficient - Infinity and
NaN values both pass that check, and get stored in pg_class verbatim.
This can have various undesirable consequences.
Fixed by rejecting non-finite reltuple values, in the same non-fatal way
as for the existing checks (emit WARNING and skip the update). Adds a
regression test to stats_import for these non-finite values, and to
check the -1.0 special value is still accepted.
Backpatch to 18, where pg_restore_relation_stats() was introduced.
Patch by Jan Nidzwetzki, minor commit message tweaks by me.
Author: Jan Nidzwetzki <jan@planetscale.com>
Discussion: https://postgr.es/m/518BA772-8026-412A-AA8F-A7FE4C6B3717@planetscale.com
Backpatch-through: 18
Index builds update pg_class.reltuples for the table. In parallel GIN
builds, workers track the number of processed rows, and report it to
the leader, who then updates the pg_class with a total. However,
gin_parallel_build_main failed to initialize the bs_reltuples field,
leaving it set to whatever happens to be on the stack (which may be
bogus values like Infinity or NaN, or just impossibly high values).
If such values get reported to the leader and stored in pg_class, that
can have serious consequences. The pg_class.reltuples field is used to
decide when a table is due for autovacuum or autoanalyze, and if it
happens to be set to a bogus value, that may never happen. The field is
also used by the optimizer when calculating costs.
Fixed by initializing bs_reltuples together with the rest of the build
state. The bs_numtuples was initialized later, but it seems cleaner to
just initialize all the fields at once.
After a bogus value gets persisted in pg_class, affected systems are
unlikely to self-heal. That would require an ANALYZE, but preventing
that is one of the consequences. We have considered forcing autoanalyze
in these cases, but there's not a good way to reliably identify bogus
values (except for a small minority like Infitiny/NaN).
A manual ANALYZE on (possibly) affected tables is the only solution.
Backpatch to 18, where parallel GIN builds were introduced.
Reported-by: Jan Nidzwetzki <jan@planetscale.com>
Discussion: https://postgr.es/m/518BA772-8026-412A-AA8F-A7FE4C6B3717@planetscale.com
Backpatch-through: 18
pg_dump in --binary-upgrade mode emits "SUBSCRIPTION TABLE" TOC entries to
preserve pg_subscription_rel state across pg_upgrade. When such a dump
was restored with --no-subscriptions, _tocEntryRequired() skipped the
"SUBSCRIPTION" entry but not the associated "SUBSCRIPTION TABLE" entries,
so the restore would try to apply subscription-relation state for a
subscription that was never created.
Skip "SUBSCRIPTION TABLE" entries as well when no_subscriptions is set.
This can happen when pg_subscription_rel has entries, the dump is taken
with --binary-upgrade, and it is restored with --no-subscriptions.
Reported-by: Hayato Kuroda <kuroda.hayato@fujitsu.com>
Author: Hayato Kuroda <kuroda.hayato@fujitsu.com>
Reviewed-by: Shlok Kyal <shlok.kyal.oss@gmail.com>
Reviewed-by: Amit Kapila <amit.kapila16@gmail.com>
Backpatch-through: 17, where it was introduced
Discussion: https://postgr.es/m/OS9PR01MB121493DA4C1A7748B11A646D8F5C02@OS9PR01MB12149.jpnprd01.prod.outlook.com
A cascading standby could fail to reconnect to its upstream standby with
"requested starting point ... is ahead of the WAL flush position" after
falling back to archive recovery. This happened because archive
recovery processes whole segment files, so after replaying a segment the
cascade's next read position lands at the start of the following
segment, which is ahead of the upstream's flush position reported by
GetStandbyFlushRecPtr() (still inside the just-replayed segment).
Fix by having the walreceiver check the upstream's current WAL flush
position via IDENTIFY_SYSTEM before issuing START_REPLICATION.
IDENTIFY_SYSTEM already returns this position (as xlogpos), but
walrcv_identify_system() previously discarded it; now we have a use for
it. If the requested start point exceeds the upstream's flush position
on the same timeline, the walreceiver waits for
wal_retrieve_retry_interval and retries.
The wait is limited to gaps of at most one WAL segment, which is the
expected case from the segment-granularity of archive recovery. Larger
gaps indicate the upstream is genuinely behind, so START_REPLICATION is
allowed to proceed (and fail) normally, letting the startup process fall
back to other WAL sources. The first wait is logged at LOG level;
subsequent waits are demoted to DEBUG1 to avoid log noise. The
walreceiver honors wal_receiver_timeout during the wait, so it will exit
if the upstream doesn't catch up in time.
To preserve ABI compatibility on back branches, the flush position from
IDENTIFY_SYSTEM is communicated via a new global variable
(WalRcvIdentifySystemLsn) rather than changing the signature of
walrcv_identify_system().
The bug was introduced in Postgres 9.3 by commit abfd192b1b, which
added a flush-position check in StartReplication() that rejects requests
ahead of the upstream server's WAL flush position.
Author: Marco Nenciarini <marco.nenciarini@enterprisedb.com>
Reviewed-by: Xuneng Zhou <xunengzhou@gmail.com>
Backpatch-through: 14
Discussion: https://postgr.es/m/CA+nrD2cTuTkkX5WXVZengTYYZbAO6zV8K+Tri-R0fbLFuoyMBA@mail.gmail.com
find_nonnullable_rels and find_nonnullable_vars mistakenly treated a
ScalarArrayOpExpr that could return FALSE as strict, but that's okay
only at top level of a qual expression; further down, we've got to
insist on a guaranteed-NULL result. The result was that we could draw
mistaken conclusions about whether outer joins can be simplified, if
the decision hinged on a non-top-level ScalarArrayOpExpr with a
potentially-empty array argument.
I believe this error dates to commit 72a070a36, which taught
find_nonnullable_rels to descend into non-top-level parts of qual
expressions. is_strict_saop (added earlier by 72153c058) already had
enough intelligence to do the case correctly, but it wasn't passed the
proper flag, ie "top_level" needs to be passed for "falseOK".
e006a24ad copied that mistake into find_nonnullable_vars.
Later, over-eager refactoring in commit 2f153ddfd broke
contain_nonstrict_functions' handling of ScalarArrayOpExpr by treating
it as though it were no different from an OpExpr. It is, because
we must also prove the array is non-empty before concluding that the
expression is strict. This could result in misclassifying an
expression as strict when it is not, leading to assorted planning
mistakes such as inlining a SQL function that shouldn't be inlined.
We can almost fix this by just re-adding the previous handling of
ScalarArrayOpExpr in that function, but doing only that would lead to
also calling check_functions_in_node() and thus redundantly checking
the operator's strictness. Avoid that by turning the if-series into
an else-if chain, as it arguably should have been all along.
The reason these errors have escaped detection for decades is that
they are exposed only in arcane corner cases. ScalarArrayOpExpr with
an empty array isn't typical usage, and even when that's possible
several other conditions apply before the planner can reach a mistaken
conclusion. While it's possible to build test cases demonstrating
these mistakes, I (tgl) judged them too indirect and special-purpose
to justify consuming regression test cycles forevermore.
Author: Ayush Tiwari <ayushtiwari.slg01@gmail.com>
Reviewed-by: Tom Lane <tgl@sss.pgh.pa.us>
Discussion: https://postgr.es/m/CAJTYsWV3vqRJmST-gv1NsXEef-zOnjVJpYS910aBaiuMij4nFg@mail.gmail.com
Discussion: https://postgr.es/m/CAJTYsWWcLGmz0f8_QPP_Liq-fc7-geiFSCdqoq3XGeRHPPsWeA@mail.gmail.com
Backpatch-through: 14
A two-phase transaction that is assigned an XID but produces no change
to be decoded -- for example, one that only acquires row locks via
SELECT ... FOR SHARE -- has no base snapshot in the reorder
buffer. ReorderBufferReplay() already skips such a transaction at
PREPARE time and never invokes the begin_prepare/change/prepare
callbacks for it, but ReorderBufferFinishPrepared() still called the
commit_prepared (or rollback_prepared) callback. As a result a
spurious COMMIT/ROLLBACK PREPARED was sent to the output plugin with
no preceding PREPARE. For the built-in subscriber this breaks
replication (the apply worker fails to find the prepared transaction),
and test_decoding could even crash.
Fix this by detecting an empty transaction (base_snapshot == NULL) in
ReorderBufferFinishPrepared() and cleaning it up without invoking the
commit/rollback prepared callbacks, mirroring the existing empty
transaction handling in ReorderBufferReplay().
On v18 and newer versions, commit 072ee847ad changed
ReorderBufferPrepare() to send the prepare whenever it had not already
been sent, which also fires for empty transactions and emits a
spurious PREPARE. On those branches ReorderBufferPrepare() is
therefore additionally guarded with base_snapshot != NULL. This guard
and the Assert(!rbtxn_sent_prepare()) added in
ReorderBufferFinishPrepared(), are not necessary on v17 and older
versions: there ReorderBufferPrepare() only sends a prepare for
concurrently-aborted transactions (which never applies to an empty
transaction) and the RBTXN_SENT_PREPARE flag does not exist.
Back-patch to v14, where decoding of two-phase transactions was
introduced.
Bug: #19556
Reported-by: Alexander Kozhemyakin <a.kozhemyakin@postgrespro.ru>
Reviewed-by: Amit Kapila <amit.kapila16@gmail.com>
Discussion: https://postgr.es/m/19556-daa6d7ea65054d48@postgresql.org
Backpatch-through: 14
pg_get_publication_tables() collects the OIDs of the published tables
on its first call, without locking them, and then reopens each table
later, once per result row, to compute its column list and fetch its
row filter. The reopen used table_open(), which errors out with "could
not open relation with OID" if the table has been dropped in the
meantime. This could happen for any published table without an
explicit column list, which is every table in FOR ALL TABLES and FOR
TABLES IN SCHEMA publications, but also FOR TABLE entries without a
column list. The failure is common in environments where many tables
are created and dropped while publication tables are being queried,
e.g. by table synchronization on a subscriber.
Fix by opening every table with try_table_open(), which returns NULL
if the relation no longer exists, and skipping the table in that
case. Concurrently dropped tables are thus simply absent from the
result set, which is the expected point-in-time behavior.
As a side effect, tables with an explicit column list, which were
previously returned without being opened, are now also locked with
AccessShareLock, so the function can block behind concurrent DDL on
such tables where it previously did not.
Backpatch to v16, where we added the table_open() call in
pg_get_publication_tables().
Author: Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com>
Reviewed-by: Bertrand Drouvot <bertranddrouvot.pg@gmail.com>
Reviewed-by: shveta malik <shveta.malik@gmail.com>
Reviewed-by: Ajin Cherian <itsajin@gmail.com>
Reviewed-by: Masahiko Sawada <sawada.mshk@gmail.com>
Reviewed-by: Chao Li <li.evan.chao@gmail.com>
Discussion: https://www.postgresql.org/message-id/CALj2ACVYYooWH-5tJ6cPKkU%2BmutVxwb_z4S%2BqAi-zdrFqxXE2Q%40mail.gmail.com
Backpatch-through: 16
Commit fd83c83d09 turned the recursive posting-tree cleanup in
ginVacuumPostingTreeLeaves() into an iterative sweep that follows the
tree's leaf pages via their rightlinks. The recursive version called
vacuum_delay_point() while processing the tree, but that call was removed
and never re-added to the new loop. As that commit only set out to fix a
deadlock, the removal appears to have been unintentional.
Consequently the leaf-page sweep of a single posting tree runs with no
vacuum_delay_point(), and therefore no CHECK_FOR_INTERRUPTS(). A posting
tree stores all the TIDs for one indexed key, so for a frequently
occurring key it can span a large number of leaf pages. While such a
tree is being vacuumed the operation ignores vacuum_cost_delay and does
not respond to query cancellation or statement_timeout; an autovacuum
worker likewise cannot be interrupted mid-sweep when another backend
requests a conflicting lock.
Restore the call, placed after the current page has been unlocked and
released so that no buffer content lock is held across a potential delay
(cf. 21c27af65f). The sibling loops in ginbulkdelete() and
ginvacuumcleanup() already call vacuum_delay_point() once per page.
Author: Paul Kim <mok03127@gmail.com>
Co-authored-by: Alexander Korotkov <aekorotkov@gmail.com>
Reviewed-by: Michael Paquier <michael@paquier.xyz>
Reviewed-by: Andrey Borodin <x4mmm@yandex-team.ru>
Reviewed-by: solai v <solai.cdac@gmail.com>
Discussion: https://postgr.es/m/178447127453.110.12276981925360691905%40mail.gmail.com
Backpatch-through: 14
index_create_copy is used to create copy definitions of existing indexes.
Currently, it passes 0 as constr_flags to index_create(), which results
in the copied index to always be created as immediate (indimmediate set
to true). For deferrable unique constraints, it means that the
transient index used during the phase 2 of REINDEX CONCURRENTLY forces
immediate constraint checks on concurrent inserts, which can cause
unexpected constraint violations based on the definition of the parent
table, inconsistently set in the copied index.
To fix this without violating the contract of constr_flags (which should
only be used when creating constraints) and without relaxing the strict
assertion in index_create(), this introduces a new index creation flag:
INDEX_CREATE_DEFERRABLE. If set, a copied index's indimmediate is set
to false, meaning that unique constraints are not enforced immediately
on insertion, but at transaction commit time.
An isolation test for REINDEX CONCURRENTLY is added, based on an
injection point waiting after phase 1 of the operation, where an index
copy has been built and is able to accept DMLs for its validation in
phase 2. The test is tentatively backpatched down to v17.
INJECTION_POINT() is outside a transaction context, which should be fine
on HEAD since 8daeaa9b64 but I suspect may cause issues in v19 and
older branches due to the wait facility depending on condition variables
and a DSM setup, but let's see what the buildfarm tells.
Author: Nitin Motiani <nitinmotiani@google.com>
Discussion: https://postgr.es/m/CAH5HC97JmjPpgiQOqW9xm8qXhNiu7zZ1Qh+FfhEESJuDv69kuQ@mail.gmail.com
Backpatch-through: 14
Commit f9b7fc65 fixed a race when predicate-locking completely empty
btrees: without a buffer lock held, a matching key could be inserted
between _bt_search and the PredicateLockRelation call, so the scan would
miss concurrently inserted tuples while the writer wouldn't see the
reader's predicate lock. That commit only fixed _bt_first's _bt_search
path, though. Scans without useful insertion scan keys return early
from _bt_first via _bt_endpoint, which still didn't recheck if the
relation was empty.
To fix, add handling to _bt_endpoint that is analogous to the handling
added to _bt_search by commit f9b7fc65.
Author: Peter Geoghegan <pg@bowt.ie>
Discussion: https://postgr.es/m/CAH2-WzkNoTn3yXY0iGkSuavJ+sL8EROf+kitW+_2v2tJVWuKmA@mail.gmail.com
Backpatch-through: 14
pg_database_size() allows access to users who have either CONNECT
privilege on the target database or privileges of the pg_read_all_stats
role. However, previously, psql's \l+ checked only for CONNECT,
so users with privileges of pg_read_all_stats still saw "No Access" for
databases they could not connect to.
Fix this by making \l+ also check
pg_has_role('pg_read_all_stats', 'USAGE'), matching
pg_database_size()'s permission rules.
For back branches, emit the pg_read_all_stats check only when
connected to PostgreSQL 10 or later, since earlier releases do not have
that predefined role.
Backpatch to all supported versions.
Author: Christoph Berg <myon@debian.org>
Reviewed-by: Álvaro Herrera <alvherre@kurilemu.de>
Reviewed-by: Fujii Masao <masao.fujii@gmail.com>
Discussion: https://postgr.es/m/amCo6qRmnfPVk4-V@msg.df7cb.de
Backpatch-through: 14
injection_wait() only clears its slot in the waiter array after the
wait loop finishes. When the waiting query is canceled or the backend
is terminated (wait look has a CHECK_FOR_INTERRUPS), the slot leaks.
Later wakeups of the same point then bump the counter of the leaked slot
instead of the real waiter, that sleeps forever. Repeated leaks can
exhaust all the slots.
The code is changed so as the waiting loop is wrapped with
PG_ENSURE_ERROR_CLEANUP, so as the injection point slots, that are
shared resources, can be cleaned up on ERROR as much as a FATAL.
An isolation test is added: cancel one waiter, terminate another waiter,
then check that a later waiter still receives a wakeup. Without the
fixed code, the test would fail on timeout.
Author: Zsolt Parragi <zsolt.parragi@percona.com>
Discussion: https://postgr.es/m/CAN4CZFO+KF=cc0-iEg28RhqRBp_fTs6D4b8b7D7DB-pGYP3Ccg@mail.gmail.com
Backpatch-through: 17
SummarizeWAL documents that maximum_lsn should be passed as "the switch
point when reading a historic timeline, or the most-recently-measured end of
WAL when reading the current timeline." But the caller always passed the
most recently measured end-of-WAL even when reading from a historic
timeline, due to an oversight on my part. Fix that.
As far as I can determine, for this to become an issue in practice, it's
necessary to have a corrupted WAL file in the archive. SummarizeWAL checks
that every record it processes both starts and ends before switch_lsn; so if
all the WAL files in the archive are valid, SummarizeWAL will still discover
where it should stop summarizing and do the right thing. However, if
there's a corrupted file in the WAL archive, and if it is also the case that
the end of the current timeline has advanced past the switch point, then the
incorrect maximum_lsn value can result in trying to read an invalid record
and erroring out, which leads repeatedly retrying and failing with an error
every time.
One way this could occur is if a new primary is promoted and creates a
.partial file, and the user manually renames that file to remove the suffix,
and it is then archived. In that situation, the tail end of the file need
not be valid WAL, and that could lead to a stuck WAL summarizer.
Reported-by: Fabrice Chapuis <fabrice636861@gmail.com>
Analyzed-by: Thom Brown <thom@linux.com> (using claude)
Discussion: http://postgr.es/m/CAA5-nLDdvGMkN6Z-GaHGHG5T7QWEgv4YoHO7XvOJbeD00cghNg@mail.gmail.com
Backpatch-through: 17
It's always been the case that granting these privileges to users that
you don't fully trust was a bad idea, but it hasn't always been
obvious to people reading the documentation that this is the case.
To prevent confusion, and also repeated reports to pgsql-security,
mention it explicitly.
Discussion: http://postgr.es/m/CA+TgmobrjCHBuWHrvX3=2vndUCO2thUOdevrCcMDFW86cqCYvw@mail.gmail.com
Reviewed-by: Nathan Bossart <nathandbossart@gmail.com>
Backpatch-through: 14
Commit 1df9e8d96 made remove_useless_result_rtes() compute the set of
baserels in the jointree, to pass down to the find_dependent_phvs()
checks. But those checks are no-ops when the query contains no PHVs,
since find_dependent_phvs() and find_dependent_phvs_in_jointree() both
return early in that case. So we can avoid the
get_relids_in_jointree() scan altogether when root->glob->lastPHId is
zero, leaving baserels as NULL.
Author: Richard Guo <guofenglinux@gmail.com>
Reviewed-by: Tom Lane <tgl@sss.pgh.pa.us>
Discussion: https://postgr.es/m/CAMbWs49H275KzgZr3Cd1Hy+6Lmwp35bZ+5PrVc62k3HDLj6hNQ@mail.gmail.com
Backpatch-through: 16
find_dependent_phvs() and find_dependent_phvs_in_jointree() decide
whether a PlaceHolderVar depends on the RTE_RESULT rel we're
considering removing by comparing the PHV's phrels to a singleton set
containing that rel's RT index, reasoning that if phrels contains any
other relid bits then those define an appropriate place where we can
evaluate the PHV. But since this code was originally written, we've
redefined phrels to include outer-join relids, and that breaks this
logic, potentially allowing us to remove an RTE_RESULT that leaves no
valid place to evaluate the PHV. The planner doesn't throw an error
when that happens, but it does produce an incorrect plan that will not
replace the PHV's value with NULL when needed.
In the known test case for this bug, the "extra" OJ relid is one that
we've actually decided to remove but haven't yet cleaned out of the
query's PHVs. It's not entirely clear though that that would always
be the case. Let's restore this code to the way it was designed to
work, by considering only base relids within the PHV's phrels.
Bug: #19553
Reported-by: Viktor Leis <leis@in.tum.de>
Author: Matheus Alcantara <matheusssilv97@gmail.com>
Co-authored-by: Richard Guo <guofenglinux@gmail.com>
Reviewed-by: Tom Lane <tgl@sss.pgh.pa.us>
Discussion: https://postgr.es/m/19553-4561747f93f368a7@postgresql.org
Backpatch-through: 16
An index-only scan filled its result slot from the HeapTuple an index AM
returns in scan->xs_hitup by deforming it with the virtual slot's own
tuple descriptor (during GiST and SP-GiST index-only scans). But index
AMs form that heap tuple using their own descriptor, scan->xs_hitupdesc.
The AM's descriptor may disagree with the IoS virtual slot's descriptor
about each column's precise alignment, leading to "can't happen" errors
in certain rare edge cases. Hard crashes were possible but much less
likely.
To fix, deform the tuple with the descriptor it was formed with. This
is simpler, and makes xs_hitup handling (used by GiST and SP-GiST)
uniform with the nearby existing xs_itup handling (used by nbtree).
In practice this issue was very unlikely to be hit (it was found during
testing of a patch that will change the table AM API used during index
scans). The only currently affected core opclass is GiST's range_ops.
It was only possible for the datum to be accessed at an incorrectly
aligned offset when reading the second or subsequent column from a
multicolumn GiST index. This couldn't happen in the common case where
the datum used an unaligned short varlena header. Moreover, an earlier
column had to leave the range datum at an offset where the two
alignments actually disagree (e.g., an odd-length varlena datum).
Author: Peter Geoghegan <pg@bowt.ie>
Reviewed-by: Tomas Vondra <tomas@vondra.me>
Discussion: https://postgr.es/m/CAH2-WzkGXa2SKnebdW29RT1hCcQBo_p03v3iqif2u9bjzLB-aQ@mail.gmail.com
Backpatch-through: 14
The previous text claimed that once the slot is disassociated with
ALTER SUBSCRIPTION ... SET (slot_name = NONE), DROP SUBSCRIPTION "will no
longer attempt any actions on a remote host". That is inaccurate:
DROP SUBSCRIPTION may still connect to the publisher to drop
internally-created table synchronization slots when some table
synchronization is left unfinished. Reword to describe this, and note
that if the publisher is unreachable those slots (and the main slot, if
it still exists) must be dropped manually to avoid indefinitely reserving
WAL.
Reported-by: Jeff Davis <pgsql@j-davis.com>
Author: Amit Kapila <amit.kapila16@gmail.com>
Backpatch-through: 14
Discussion: https://postgr.es/m/CAA4eK1+tyYSpPxMBy1974kjivuGeR7YY=yopwRGrK3+vCTysdg@mail.gmail.com
Discussion: https://postgr.es/m/D908370F-2695-4231-851D-17179A6A6F2A@gmail.com
Commit c7aeb775 rewrote the HOT-chain offset sanity checks in three
places, but in heap_get_root_tuples it accidentally tested offnum -- the
outer loop variable, which is already bounded by the loop condition --
instead of nextoffnum, the offset actually passed to PageGetItemId. The
pre-c7aeb775 check tested nextoffnum.
With the check ineffective, a stale t_ctid could make PageGetItemId read
past the end of the line pointer array (which is data corruption that we
expect to be able to catch here).
Author: Peter Geoghegan <pg@bowt.ie>
Reported-by: Konstantin Knizhnik <knizhnik@garret.ru>
Discussion: https://postgr.es/m/87c7d8a4-3a82-4334-bee6-e8c2ad3f3293@garret.ru
Backpatch-through: 15
uuidv7(interval) shifts the current time by the given interval before
encoding it into the 48-bit Unix-millisecond timestamp field of the
generated UUID. Two cases were mishandled:
An infinite interval ('infinity' or '-infinity') produced an infinite
timestamp, which overflowed during the conversion to Unix-epoch
microseconds and yielded a garbage UUID. Reject infinite intervals up
front, before any timestamp arithmetic.
A shift that moved the timestamp outside the range representable by
the 48-bit field was silently accepted. Timestamps before the Unix
epoch wrapped when cast to unsigned, and timestamps beyond
approximately year 10889 overflowed the field; both produced UUIDs
with bogus timestamps that break sort ordering. Reject any shifted
timestamp outside the supported range.
Also document that infinite intervals and out-of-range shifts are
rejected.
Although raising a new error changes behavior in a stable branch, this
is back-patched to 18 (where uuidv7(interval) was introduced) because
the previous behavior can silently corrupt data. Failing loudly is far
safer than silently accepting the wraparound; otherwise users may not
discover that their UUIDv7 values are no longer sortable until years
later, when recovery is painful. It also matches how PostgreSQL
already handles timestamp + interval overflow, which raises an
error. The change only affects applications passing an interval large
enough to push the result outside the representable range.
Backpatch to 18, where uuidv7(interval) was introduced.
Reported-by: Christophe Pettus <xof@thebuild.com>
Author: Baji Shaik <baji.pgdev@gmail.com>
Reviewed-by: Masahiko Sawada <sawada.mshk@gmail.com>
Reviewed-by: Zsolt Parragi <zsolt.parragi@percona.com>
Reviewed-by: Tristan Partin <tristan@partin.io>
Reviewed-by: Kyotaro Horiguchi <horikyota.ntt@gmail.com>
Discussion: https://www.postgresql.org/message-id/799A70FA-6E5C-4118-99EB-2FBBE1CBAC54@thebuild.com
Backpatch-through: 18
