pgoutput records the toplevel XID in the relation's streamed_txns
list, allocated in CacheMemoryContext, when it sends a relation's
schema during a streamed transaction. We clean up that list at stream
commit and stream abort, but missed stream prepare, so the XIDs stay
around until a relcache invalidation for the relation or until the
walsender exits.
Fix by cleaning up the cache after writing the stream prepare message,
marking the schema as sent as we do at stream commit. The subscriber
applies the spooled relation messages at PREPARE time and its relation
map isn't rolled back by ROLLBACK PREPARED, so we don't need to send
the schema again even if the prepared transaction eventually aborts.
Oversight in commit 63cf61cdeb.
Author: Ayush Tiwari <ayushtiwari.slg01@gmail.com>
Reviewed-by: Hayato Kuroda <kuroda.hayato@fujitsu.com>
Reviewed-by: Masahiko Sawada <sawada.mshk@gmail.com>
Discussion: https://postgr.es/m/CAJTYsWWt5aaKHCnV=jNOwABzspd-WbnhWLndDqtESDQRQ-BkKQ@mail.gmail.com
Backpatch-through: 15
We unintentionally copied the original query's limitOption field while
building the modified subquery. If it was LIMIT_OPTION_WITH_TIES,
that behavior would propagate through to execution, potentially
causing a "more than one row returned by a subquery used as an
expression" error. Fix by resetting to the default LIMIT_OPTION_COUNT.
This bug dates back to the introduction of WITH TIES.
Bug: #19694
Reported-by: Chaitanya Choudhary <chaitanyyachoudhary@gmail.com>
Author: Chaitanya Choudhary <chaitanyyachoudhary@gmail.com>
Reviewed-by: Tom Lane <tgl@sss.pgh.pa.us>
Discussion: https://postgr.es/m/19694-4e61bd0475398eef@postgresql.org
Backpatch-through: 14
Commits 2bc7e886fc and 90eae926ab taught ON CONFLICT to include
indexes matching an already selected arbiter, so that an index left
behind by REINDEX CONCURRENTLY continues to arbitrate together with its
replacement. Both checks were too permissive:
a) infer_arbiter_indexes() compared a candidate with a named
constraint's index using only attributes, expressions and predicate, but
ignored collation, NULLS NOT DISTINCT setting or deferrability. As a
result, an index with a difference in these settings could be accepted
even though it did not identify the same conflicts. Also, a deferrable
index that otherwise matches an arbiter index would also cause ON
CONFLICT to fail with "ON CONFLICT does not support deferrable unique
constraints/exclusion constraints as arbiters".
b) ExecInitPartitionInfo() also failed due to the failure to compare
deferrability, so partition-local deferrable indexes would be considered
and break inserts routed to that partition with the error mentioned
above.
Fix by making IsIndexCompatibleAsArbiter() compare those properties, and
changing infer_arbiter_indexes() to use that routine instead of open
coding equivalent logic. Also handle the named-constraint case in
infer_arbiter_indexes() separately instead of passing the constraint
index through the regular clause-inference matching. That function is
not static anymore, so move it to index.c, and also reimplement it to
use the Relation from the indexes only, no longer receiving the
IndexInfo (which wasn't really necessary); also, use equal() to compare
the expression and predicate lists rather than list_difference().
Add tests for ON CONFLICT ON CONSTRAINT with deferrable, NULLS NOT
DISTINCT, and different-collation sibling indexes, preserving the
behavior of released pre-19 versions. Also test routed inserts with a
partition-local deferrable unique constraint.
Author: Zsolt Parragi <zsolt.parragi@percona.com>
Author: Mihail Nikalayeu <mihailnikalayeu@gmail.com>
Reported-by: Zsolt Parragi <zsolt.parragi@percona.com>
Reviewed-by: Michael Paquier <michael@paquier.xyz>
Backpatch-through: 19
Discussion: https://postgr.es/m/CAN4CZFPEYXeYFTxHpoPujfVFb+1Tx1jnXVboDMBg-ZhpgpQ-_g@mail.gmail.com
SHMEM_ATTACH_UNKNOWN_SIZE can be passed as argument to
ShmemRequestStruct() when the caller wants to attach to an existing
shared memory structure whose size it doesn't know. If the shared
memory structure doesn't exist, the request should fail, but instead,
ProcessShmemRequestsAfterStartup() tried to create the structure with
size = -1. That led to integer overflow in ShmemAllocRaw() and memory
corruption.
Fix by rejecting requests with SHMEM_ATTACH_UNKNOWN_SIZE when the
structure doesn't exist. Also add an integer overflow check in
ShmemAllocRaw(), to protect from this kind of confusion or simply too
large requests. Also document SHMEM_ATTACH_UNKNOWN_SIZE.
Author: Ashutosh Bapat <ashutosh.bapat.oss@gmail.com>
Discussion: https://www.postgresql.org/message-id/CAExHW5u_fTsOAS85kG981Vu6eR1GV-344rup6zYew7xMjEDREw@mail.gmail.com
Backpatch-through: 19
The query to get attribute statistics from a remote server applied the
"C" collation to a column of type name without checking the server's
version. Since the name type was made collatable in Postgres 12, this
could break attribute statistics import if the server was older. Fix by
casting the column value to text before collating it if the server is
older.
Also, fix a typo in the if-test in the same function.
Oversights in commit 28972b6fc; the former was reported by Fujii Masao.
Reported-by: Fujii Masao <masao.fujii@gmail.com>
Author: Etsuro Fujita <etsuro.fujita@gmail.com>
Reviewed-by: Corey Huinker <corey.huinker@gmail.com>
Discussion: https://postgr.es/m/CAHGQGwGvA_3q7Wg7GXQkuugGZj3%3DkxDRjDsKgVvK1uwAxHTopQ%40mail.gmail.com
Backpatch-through: 19
The reference page said WAIT "cannot be used in contexts where such a
snapshot must remain active, including transactions running at isolation
levels higher than READ COMMITTED", which reads as a blanket ban at those
levels. It is not one. The rule the code enforces is that the current
transaction must hold no snapshot, and a transaction-snapshot-mode
transaction has no snapshot until its first query: WaitStmt is exempt in
PlannedStmtRequiresSnapshot(), so the command itself does not take one.
WAIT therefore works before that first query, which is the order an
application wants anyway, and it is what 447aae13b0 set out to allow by
keeping the wait snapshot-free.
State the rule in terms of the snapshot rather than the isolation level,
say that WAIT acquires none of its own and name the other ways to hold
one, and give the consequence of holding one: it can delay replay, which
standby_replay waits for and which the other standby modes ultimately
depend on as well.
The error for a held snapshot named the internal snapshot states instead
of saying what the user did, and attached the isolation-level
explanation as DETAIL unconditionally, so a READ COMMITTED session with a
cursor was told its isolation level was too high. Report what is held
rather than how it is held, and emit the DETAIL only when the isolation
level is in fact the reason.
Reported-by: Sami Imseih <samimseih.pg@gmail.com>
Reported-by: Xuneng Zhou <xunengzhou@gmail.com>
Reviewed-by: Sami Imseih <samimseih.pg@gmail.com>
Reviewed-by: Xuneng Zhou <xunengzhou@gmail.com>
Discussion: https://postgr.es/m/CAN12%2BYJddgAZzA36JZFX%2BYrnNR7Uubp9qWFz6ASs3v0kL30AMA%40mail.gmail.com
Backpatch-through: 19
Commit a49b9cfd72 began reporting the EXCEPT clause relation with a
schema-qualified name that quotes identifiers when needed, inside a
message that already puts the name in quotes. So a schema or relation name
that needs quoting came out quoted twice.
Fix by building the qualified name as "%s.%s" without identifier quoting,
so it renders like the qualified names in other backend error messages.
The message text, and so its translations, is unchanged.
RelationGetQualifiedRelationName() has no callers left, so remove it.
Author: Manuel Reyes Bravo <manuelreyesbravo@gmail.com>
Reviewed-by: shveta malik <shveta.malik@gmail.com>
Reviewed-by: Vignesh C <vignesh21@gmail.com>
Reviewed-by: Peter Smith <smithpb2250@gmail.com>
Reviewed-by: Zhijie Hou <houzj.fnst@fujitsu.com>
Reviewed-by: Amit Kapila <amit.kapila16@gmail.com>
Discussion: https://postgr.es/m/CAHGQGwHfESBexa7fq99EvFCf31av=O9h9udnw22ymxmm6LMZzw@mail.gmail.com
Backpatch-through: 19, where it was introduced
Some code added in 489247b0e tried to prune the DEFAULT partition when
the next partition had a MINVALUE clause and likewise when the final
partition to scan had a MAXVALUE clause for the final partition key in
the pruning step. This code was incorrect as it could prune the default
partition incorrectly in cases such as:
p: PARTITION BY RANGE (a, b)
p1: FOR VALUES FROM (13, 0) TO (19, MAXVALUE)
pd: DEFAULT
SELECT * FROM t WHERE a = 32 AND b >= -7;
Here the pruning step for a = 32 and b >= -7 would see that only the
default partition needs to be scan, but it would then see that the
partition prior to the default had a MAXVALUE bound then prune away the
default thinking that it needn't be scanned. This could result in
incorrect results.
Fix this by moving the code that looks for the MAXVALUE bound into the
code handling BTEqualStrategyNumber so that when we're pruning with a
prefix of the partition keys, we check if the bound for the offset we've
calculated lands on a partition where the next partition key is bounded
with MAXVALUE. If so we don't include the default partition.
This leaves only the case of the first partition key. We handle that by
modifying the existing code that was checking the last key covered by
the given values.
Author: Ewan Young <kdbase.hack@gmail.com>
Reviewed-by: Tender Wang <tndrwang@gmail.com>
Reviewed-by: David Rowley <dgrowleyml@gmail.com>
Discussion: https://postgr.es/m/CAON2xHO=sqdqp=z8zWkybnWp0AuvefnAi2ez2vOYWrhXB6hHWQ@mail.gmail.com
Backpatch-through: 14
Building the partition pruning steps for excessively complex queries
can take some time. Add CHECK_FOR_INTERRUPTS() to
get_steps_using_prefix_recurse() to allow more timely processing of
interruptions.
Bug: #19692
Reported-by: Qifan Liu <imchifan@163.com>
Discussion: https://postgr.es/m/19692-dacfc57cda5035fc@postgresql.org
Backpatch-through: 14
This commit fixes several related problems. All of these problems
escaped testing for the same reason: they do not occur when only
generated advice is supplied, and therefore test_plan_advice was
incapable of detecting these shortcomings.
First, prior to this commit, the advice feedback code never regarded
an unordered sublist of a JOIN_ORDER specification as matching an
unrolled join. As a result, when JOIN_ORDER() advice contained
unordered sublists, the advice feedback tended to be "matched, failed"
even when the advice worked exactly as intended. Only the case where
an unordered sublist was implemented by a plan shape not subject to
unrolling, such as a partitionwise join, worked properly.
Second, prior to this commit, a join order sublist in the initial
position wasn't properly handled. Note that this overlaps with the
problem described in the previous paragraph; JOIN_ORDER({a b} c) was
broken both because of the unordered sublist and because of the
sublist being in the initial position. However, there's more to this
case: JOIN_ORDER((a b) c) means the same as JOIN_ORDER(a b c), but the
advice feedback code didn't know that, and would generate "matched,
failed" for the former case even when everything was working.
Finally, prior to this commit, advice feedback didn't correctly handle
single-element sublists. Those are pretty nonsensical, since by
definition a join involves at least 2 tables, so we could just ban
that case. Instead, at least for now, I've chosen to make advice
feedback handle such cases in the same way that advice enforcement
already does: the extra grouping levels are simply disregarded, so
that JOIN_ORDER({a} ((b))) is enforced in the same way as, and also
gets the same advice feedback as, JOIN_ORDER(a b).
Reported-by: Noah Misch <noah@leadboat.com>
Backpatch-through: 19
Discussion: https://postgr.es/m/CA+TgmoYmXy-jiP5qDhqNEiYFEBzQsArO6O2d9E8szNZqi1bePQ@mail.gmail.com
Reviewed-by: Ayush Tiwari <ayushtiwari.slg01@gmail.com>
Prior to v18, the total cost of a plan was just one number, a
floating-point value. Commit e222534679
introduced a second component, the number of disabled_nodes at or
below that level of the plan tree. Unfortunately, I (rhaas) failed
to realize that GEQO extracts the cost from each candidate plan and
compares it directly as a measure of fitness.
This means that, at least in principle, it's possible for GEQO to
prefer a plan with more disabled_nodes and lower cost over one with
fewer disabled_nodes and higher cost, which is incorrect. It may be
possible for that to become a practical issue in v18, but it's much
more likely to become an issue in v19 due to pg_plan_advice. Hence,
since the straightforward fix breaks ABI compatibility, back-patch
only to v19.
Backpatch-through: 19
Discussion: https://postgr.es/m/CA+TgmoYmXy-jiP5qDhqNEiYFEBzQsArO6O2d9E8szNZqi1bePQ@mail.gmail.com
The check_oauth_validator check for an empty validator list didn't test
for a string with only whitespace, which would cause the postmaster to
crash. Instead of testing for the empty string cases, pass the config
value to SplitDirectoriesString unconditionally. If an empty list is
returned then the input string was empty. Since pstrdup cannot handle
NULL, assert that the string ie set. While users cannot set the string
to NULL, it is initialized to NULL so guard against programmer error.
Check the parsed list instead. Assert that the GUC string is non-NULL
before pstrdup(); users cannot set it to NULL, but the C variable is
initialized that way.
This also adds a TAP test that reloads a whitespace-only setting after
pg_hba_file_rules and waits until the existing backend sees the restored
GUC.
Author: Grigorev Jurij <ju.grigorev@ftdata.ru>
Reviewed-by: Jacob Champion <jacob.champion@enterprisedb.com>
Reviewed-by: Daniel Gustafsson <daniel@yesql.se>
Discussion: https://postgr.es/m/04fa84f6ebbe400f940e179ebe1070e9@localhost.localdomain
Backpatch-through: 18
The worker connects as the table owner in a new session, so a role- or
database-level lock_timeout and transaction_timeout unhelpfully apply to
it, possibly canceling the wait for older transactions. This cannot be
overridden by the REPACK calling session. Fix by passing the timeout
values from the steering backend via the DecodingWorkerShared struct and
adopting these values in the worker; this way, values from session-local
SET reach the worker too.
In passing, remove a pointless member from the same struct.
Author: Shihao Zhong <zhong950419@gmail.com>
Backpatch-through: 19
Discussion: https://postgr.es/m/CAGRkXqTYaBjFvjtjPb1+=sZWvt93=2c472bmM+xdG7w9ZvAydA@mail.gmail.com
pgstat_get_entry_ref_cached() inserts a local entry_ref with
shmem-related fields set to NULL, expecting pgstat_get_entry_ref() (its
sole caller) to fill them up before returning. If an ERROR happens
while pgstat_get_entry_ref() runs, it could be possible to finish with a
local pgstats entry partially filled.
This could lead to a crash of pgstat_gc_entry_refs(), which tolerates a
NULL shared_stats in an assertion but unconditionally dereferenced its
"dropped" and "generation" fields.
This extends 4069df21be, being a cheap insurance against NULL pointer
dereference, if some code paths of pgstat_get_entry_ref() are not able
to perform any cleanup actions (for example after a dsm_create()
throwing an ERROR).
Reviewed-by: Grigorev Jurij <ju.grigorev@ftdata.ru>
Discussion: https://postgr.es/m/aqtiKTvl519bu8-V@paquier.xyz
Backpatch-through: 15
A REPACK background worker can fail to start for various reasons, but
the REPACK steering process was not handling them correctly. For
instance, the steering process waits on a condition variable, but it
would simply hang if the worker doesn't send the signal. (A signal
would be sent by postmaster, but it would wake up the process only to be
ignored and continue sleeping.) Fix this by splitting the wait in two:
first wait for the worker to attach to the error queue, and then wait on
the condition variable as before.
The shutdown sequence can also get stuck, if the message queue gets full
while trying to shut down (this can apparently happen if the log level
is set to very noisy). Previously the worker would send a signal and
then detach; but the steering process can receive that signal, act on
it, then fail to wake up to handle the detach, sleeping indefinitely.
Fix this by having the worker send a Terminate message, which the
steering process can react to.
Author: Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com>
Author: Shihao Zhong <zhong950419@gmail.com>
Author: Álvaro Herrera <alvherre@kurilemu.de>
Reviewed-by: Antonin Houska <ah@cybertec.at>
Reviewed-by: Masahiko Sawada <sawada.mshk@gmail.com>
Reviewed-by: Shihao Zhong <zhong950419@gmail.com>
Reported-by: Nathan Bossart <nathandbossart@gmail.com>
Reported-by: Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com>
Reported-by: Nikolay Samokhvalov <nik@postgres.ai>
Backpatch-through: 19
Discussion: https://postgr.es/m/CALj2ACVAxA9HxvFe8HSspTJ-UO4Aoz%3DkuQdZBeLrod0gqUxH3g%40mail.gmail.com
Discussion: https://postgr.es/m/apBpOVZOyqrakEr_@nathan
These errors have been reported to the mailing lists in the last few
weeks. Some of the spots fixed have been noticed by myself during
independent code reviews, but not all.
The reference page and the read-your-writes section both presented
WAIT in standby_replay mode as enough for read-your-writes, without
saying which LSN to wait for. Read as written, an LSN taken after the
last statement but before COMMIT would do, and it would not: the standby
position WAIT compares against is the end of the last replayed record, so
a target that falls inside the COMMIT record is satisfied while that
record is still unapplied.
Say that the target must be at or after the end of the write
transaction's COMMIT record, and note in the example that autocommit is
what puts the UPDATE's commit before the call that reads the position.
Author: Sami Imseih <samimseih.pg@gmail.com>
Reviewed-by: Xuneng Zhou <xunengzhou@gmail.com>
Reviewed-by: Alexander Korotkov <aekorotkov@gmail.com>
Discussion: https://postgr.es/m/CAN12%2BYKN7gXb1o9bDWgfA%3D6wjDrqNEgt%2BrXJiAWXgPCXQ5TqpA%40mail.gmail.com
Backpatch-through: 19
The description of NO_THROW covered what the option does but not when an
application would want it. Explain that it suits callers that treat
timeout or not in recovery as expected results to be handled as part of
ordinary control flow, note the obligation to check the returned status,
and give the reason to omit the option.
Be precise about the option's scope: it changes only how those two wait
outcomes are reported. Soften the description's claim that the command
"returns a status string instead of throwing errors" accordingly, and say
that invalid input still raises an error, as does any condition checked
before the wait begins. Name two of those conditions as examples rather
than listing them, since the set grows.
Author: Rithvika Devisetti <devisettirithvika@gmail.com>
Reported-by: Peter Eisentraut <peter@eisentraut.org>
Reviewed-by: Kiran Kaki <itskkpg@gmail.com>
Reviewed-by: Xuneng Zhou <xunengzhou@gmail.com>
Reviewed-by: Satyanarayana Narlapuram <satyanarlapuram@gmail.com>
Reviewed-by: Alexander Korotkov <aekorotkov@gmail.com>
Discussion: https://postgr.es/m/14415b05-16d0-446f-ba7d-c891db8b3406%40eisentraut.org
Backpatch-through: 19
Previously, aborting an internal subtransaction during COMMIT or
PREPARE TRANSACTION could cause the following assertion failure.
This could happen, for example, when a deferred constraint trigger fired
at COMMIT and its PL/pgSQL exception block caught an error raised
while executing the trigger function.
TRAP: failed Assert("s->blockState == TBLOCK_SUBINPROGRESS || s->blockState
== TBLOCK_INPROGRESS || s->blockState == TBLOCK_IMPLICIT_INPROGRESS ||
s->blockState == TBLOCK_PARALLEL_INPROGRESS || s->blockState ==
TBLOCK_STARTED"), File: "xact.c", Line: 4851, PID: 73455
An internal subtransaction should be able to be aborted while the parent
transaction is in the COMMIT or PREPARE TRANSACTION phase. However,
RollbackAndReleaseCurrentSubTransaction()'s assertion check previously
did not allow TBLOCK_END and TBLOCK_PREPARE as parent transaction
states, causing the assertion failure.
This commit fixes the assertion check by allowing those two parent
transaction states.
Backpatch to all supported versions.
Reported-by: Fabrízio de Royes Mello <fabrizio@planetscale.com>
Author: Patrick Reynolds <piki@planetscale.com>
Reviewed-by: Fujii Masao <masao.fujii@gmail.com>
Reviewed-by: Chao Li <li.evan.chao@gmail.com>
Reviewed-by: Fabrízio de Royes Mello <fabrizio@planetscale.com>
Discussion: https://postgr.es/m/CABo-N97AeMbWuYTWg-3%3D2DkTR3EkvS%2BFt%3DyEaWB181STsR1mBg%40mail.gmail.com
Backpatch-through: 14
A pg_publication_rel entry can represent either an explicitly published
relation or a table excluded from a FOR ALL TABLES publication, but the
object address code treated every entry as a published relation without
checking prexcept. EXCEPT entries thus appeared as ordinary
published-table mappings in dependency messages, pg_identify_object(),
and pg_identify_object_as_address().
Report such entries as "publication excluded relation" instead, with
object identities indicating that the table is excluded from the
publication. pg_get_object_address() also accepts this new object type,
so that the output of pg_identify_object_as_address() can still be
passed back to it to look up the same object. "publication relation"
now resolves only non-EXCEPT entries, and "publication excluded
relation" only EXCEPT entries.
Oversight in commit fd366065e0.
Author: Fujii Masao <masao.fujii@gmail.com>
Author: Vignesh C <vignesh21@gmail.com>
Reviewed-by: shveta malik <shveta.malik@gmail.com>
Reviewed-by: Peter Smith <smithpb2250@gmail.com>
Reviewed-by: Zhijie Hou <houzj.fnst@fujitsu.com>
Reviewed-by: Nisha Moond <nisha.moond412@gmail.com>
Reviewed-by: Amit Kapila <amit.kapila16@gmail.com>
Reviewed-by: Chao Li <li.evan.chao@gmail.com>
Reviewed-by: Manuel Reyes Bravo <manuelreyesbravo@gmail.com>
Discussion: https://postgr.es/m/CAHGQGwHfESBexa7fq99EvFCf31av=O9h9udnw22ymxmm6LMZzw@mail.gmail.com
Backpatch-through: 19, where it was introduced
Up until now, pg_plan_advice has had a feature that allows an advice
target to mention a partition name but omit the partition schema;
that is, something like SEQ_SCAN(foo/bar) forces a sequential scan on
every child of table foo whose partition name is bar, regardless of
the schema in which bar appears. However, that feature turns out to
have a nasty design flaw: while advice enforcement handles this case
just fine, the advice feedback code doesn't know about it and will
mark such advice as "matched, failed" even when everything worked
perfectly. Unfortunately, there seems to be no simple code fix for
this problem.
Since the release of PostgreSQL 19 is imminent, take the conservative
course and revert this feature of pg_plan_advice. In other words,
require the partition schema whenever the partition name is present.
You must now write e.g. SEQ_SCAN(foo/public.bar) rather than just
SEQ_SCAN(foo/bar). This doesn't affect any cases where automatically
generated advice is supplied, since generated advice has always
included the partition schema anyway. Manually written advice will
have to conform to the new, stricter rule. For a later release, we
can consider whether to again relax this restriction in some way,
but now is not the time to design new things.
Reported-by: Noah Misch <noah@leadboat.com>
Backpatch-through: 19
Discussion: https://postgr.es/m/CA+TgmoYmXy-jiP5qDhqNEiYFEBzQsArO6O2d9E8szNZqi1bePQ@mail.gmail.com
Like in PerformOffsetsTruncation() and e.g. in TruncateSUBTRANS(),
need to step back to previous page at page boundary, to avoid passing
a cutoff page to SimpleLruTruncate() that hasn't been created
yet. Otherwise you can get a bogus 'LOG: could not truncate directory
"pg_multixact/members": apparent wraparound' message in the log.
Reported-by: Noah Misch <noah@leadboat.com>
Discussion: https://www.postgresql.org/message-id/20260827231757.78.noahmisch@microsoft.com
Backpatch-through: 19
The Recovery section said we would "operate correctly without the full page
images that MarkBufferDirtyHint() provides", offering them only as a way to
lose less slot knowledge to RBM_ZERO_ON_ERROR. That is true of freespace.c,
which passes RBM_ZERO_ON_ERROR on every read, and false of the system around
it, which reads the same files and does verify checksums. Reading it as a
licence to dirty FSM pages without a full page image is what led to
c06d1a4ba6, since reverted.
Say instead that RBM_ZERO_ON_ERROR is a property of this directory rather
than of the fork, and name the readers that do not share it:
RelationCopyStorage(), RelationCopyStorageUsingBuffer(), base backups and
pg_checksums. Rather than presenting today's behavior as the only one
possible, note that the alternative is for all of that code to skip checksum
validation of the FSM.
Reported-by: Noah Misch <noah@leadboat.com>
Reviewed-by: Noah Misch <noah@leadboat.com>
Reviewed-by: Andrey Borodin <x4mmm@yandex-team.ru>
Discussion: https://postgr.es/m/20260901211837.f6.noahmisch%40microsoft.com
A semijoin whose RHS is unique-ified groups the RHS on the expressions in
SpecialJoinInfo.semi_rhs_exprs. Those were recorded with whatever collation
the RHS expression itself exposes, which need not be the collation the join
compares with. Neither SortGroupClause nor the pathkey machinery carries a
collation of its own, so both Unique-over-Sort and HashAggregate then grouped
by the wrong equality: values the join considers equal survived, and the
following inner join emitted the outer row once per survivor.
With a non-deterministic collation on one side, "SELECT count(*) FROM t WHERE
c IN (SELECT c0 FROM t2)" therefore counted more rows than the same predicate
reports for the rows of t.
Label each RHS expression with the operator's input collation, the same
treatment process_equivalence() gives to equivalence class members. Every
consumer of semi_rhs_exprs reads the collation off the expression, so this
fixes the sort-based and hash-based paths together; in the branches where
create_unique_path() also passes these expressions to
relation_has_unique_index_for(), it likewise stops a unique index built with a
different collation from being taken as proof that unique-ification can be
skipped. The same goes for a subquery's DISTINCT computed under a different
collation, since translate_sub_tlist() punts on the relabeled expressions.
Reported-by: Suyang Zhong <syzhong16@gmail.com>
Author: Andrey Rachitskiy <pl0h0yp1@gmail.com>
Reviewed-by: Tender Wang <tndrwang@gmail.com>
Reviewed-by: Richard Guo <guofenglinux@gmail.com>
Reviewed-by: Alexander Korotkov <aekorotkov@gmail.com>
Discussion: https://postgr.es/m/19633-647cd4c73a84b085%40postgresql.org
Backpatch-through: 14
Previously, during REPACK (CONCURRENTLY), index_rebuild_count in
pg_stat_progress_repack and pg_stat_progress_cluster did not advance
as indexes were rebuilt. This made it impossible for users monitoring the
operation to tell how many indexes had been completed.
Fix this by incrementing the count after each index is built on the new
heap, so that both views report the number of completed index builds.
Author: Fujii Masao <masao.fujii@gmail.com>
Reviewed-by: Álvaro Herrera <alvherre@kurilemu.de>
Discussion: https://postgr.es/m/CAHGQGwFUsrBvTurkSU8TEc=DZTunqteUXrasseqNMT7AMBZKRw@mail.gmail.com
Backpatch-through: 19
Previously, when REPACK, CLUSTER, or VACUUM FULL created a transient TOAST
table, index_rebuild_count in pg_stat_progress_repack and
pg_stat_progress_cluster reported 2 before any indexes on the main table
had been rebuilt. This could mislead users monitoring the operation.
This happened because creating the TOAST index writes the CREATE INDEX phase
to the same progress slot used for the index rebuild count of the enclosing
command.
Fix this by passing INDEX_CREATE_SUPPRESS_PROGRESS when creating a TOAST
index, preserving the progress information of the enclosing command and
keeping the count at zero until the main-table indexes are rebuilt.
Author: Adam Lee <adam8157@gmail.com>
Reviewed-by: Fujii Masao <masao.fujii@gmail.com>
Reviewed-by: Álvaro Herrera <alvherre@kurilemu.de>
Discussion: https://postgr.es/m/aj4gJQMba0kClQmj@Mac
Backpatch-through: 19
Commit 5f3a49abf7 changed the primary status update message to report the
end of the last inserted WAL record instead of the WAL write position.
However, the protocol documentation still described the WAL position field as
the latest WAL write position, which no longer matches the message sent by
walsender.
Update the description of the WAL position field to indicate the end of the
last inserted WAL record, matching the implementation.
Author: Fujii Masao <masao.fujii@gmail.com>
Reviewed-by: Ayush Tiwari <ayushtiwari.slg01@gmail.com>
Discussion: https://postgr.es/m/CAHGQGwFn2QfpHDR6cWxd98tqNb5vyguEis8rtqnqidFR36p4QQ@mail.gmail.com
Backpatch-through: 19
pg_bsd_indent doesn't know about alignas(), so a struct member
declared with it is misindented unless it's the first member.
Disguise each alignas(...) call as a plain identifier while
indenting, restoring it afterward. Stash the call's original text
by index rather than embedding it in the placeholder: embedding only
works for a single-token argument, and fails to match at all against
nested parens (e.g. alignas(sizeof(x))) or a line break, silently
skipping the disguise.
There is currently no code affected by this, but there are some
pending patches that would be.
Co-authored-by: Peter Eisentraut <peter@eisentraut.org>
Co-authored-by: Andrew Dunstan <andrew@dunslane.net>
Discussion: https://postgr.es/m/94a128da-bf3e-46bd-9e2d-609573c484da@eisentraut.org
The test already used a pg_resetwal to bump the initial OID to be higher
than UINT32_MAX after the main regression test suite has been run, to
check that pg_upgrade was able to carry an 8-byte wide value. This now
happens set much earlier in the script, before running the main
regression test suite.
A small oid8-related check is added, while on it. Perhaps this could be
expanded to more objects and/or patterns later on. It should be good
enough as a starting point.
Author: Michael Paquier <michael@paquier.xyz>
Reviewed-by: Greg Burd <greg@burd.me>
Reviewed-by: Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com>
Discussion: https://postgr.es/m/aqd_gYs35v--sVlR@paquier.xyz
Some of the tests introduced by c68cba09dd in these two modules want
to rely on out-of-line values (compressed and not compressed), but the
values inserted that should be compressed were not (STORAGE EXTENDED).
The tests are updated to do what they should, reusing a cheap trick to
ensure out-of-line values coming from a different test in test_decoding.
Some checks are added to ensure that the expected set of values is
inserted.
Reported-by: Rui Zhao <zhaorui126@gmail.com>
Discussion: https://postgr.es/m/CAHWVJhGxVc4CpDd+mb=6YbeL2KHDUtsUC=R68y7NeMg1cO8QMg@mail.gmail.com
Previously, string_to_uuid() decoded one byte at a time, calling
isxdigit() twice and strtoul() once for every pair of hexadecimal
digits. Commit ec8719ccbf made hex_decode_safe() decode a run of
hexadecimal digits in bulk, so add a fast path for the two common
shapes: 32 contiguous hexadecimal digits, and the canonical
8x-4x-4x-4x-12x form, each optionally wrapped in braces.
Anything else, and any decoding error, falls back to the
byte-at-a-time parser, so the accepted grammar and the error messages
don't change. hex_decode_safe() also skips whitespace, which the UUID
grammar forbids, so we treat a short decode as an error too. Rejecting
an input that has a fast-path shape therefore costs a second
parse. But this only happens for syntax-error cases, so it's not a
problem in practice.
Reviewed-by: Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com>
Reviewed-by: Haibo Yan <tristan.yim@gmail.com>
Reviewed-by: Chao Li <li.evan.chao@gmail.com>
Reviewed-by: John Naylor <johncnaylorls@gmail.com>
Discussion: https://postgr.es/m/CAD21AoCqeR4UQU77Q_yOMNNzJ7AVeiO5QZT+4HnzPm4Wm-e02Q@mail.gmail.com
Otherwise, a read of blocks the stream is about to throw away can be
started and then waited for. read_stream_reset drains the stream by
calling read_stream_next_buffer, whose look-ahead treats the zeroed
distance as end of stream, leading to it spuriously performing the
pending read. This caused a pathological regression for a nested loop
antijoin query when run with the upcoming index prefetching patch series
applied.
This bug affects all stable branches with the read stream, which was
added to Postgres 17 by commit b5a9b18cd0. No backpatch for now,
though, since without index prefetching the added overhead for realistic
query plans is negligible.
Author: Tomas Vondra <tomas@vondra.me>
Discussion: https://postgr.es/m/CAH2-WzkKY8b42sZSPWJceT5Fn_THk3ZgcaM8ch3Hzw+XvhP0zQ@mail.gmail.com
Commit 0c5d626961 was confused about which indexes are potentially
problematic to rebuild: we thought only !indisready indexes could fail
to rebuild, and thus would attempt to rebuild !indisvalid ones that are
"ready" (that is, being maintained by DML but not yet completed).
However, that's wrong, because it could be that the validation phase of
such an index failed or was interrupted partway through. So, have
REPACK throw an error with all !indisvalid indexes, not just those
!indisready.
(!indisready implies !indisvalid, so this is an expansion of what
happened with the previous commit.)
Reported-by: shihao zhong <zhong950419@gmail.com>
Backpatch-through: 19
Discussion: https://postgr.es/m/CAGRkXqRA4YetT8Nu3+1-45+ffTwi2bd1d1eDFME39Y9OPRMytQ@mail.gmail.com
Add table_index_getnext_slot, a new table AM interface that serves as
the single entry point for both plain and index-only index scans that
use amgettuple. The table AM's index_scan_begin callback resolves which
implementation the scan will use and stores it in an IndexScanDesc
function pointer (xs_getnext_slot), which table_index_getnext_slot
dispatches through.
The interface intentionally avoids using fixed TableAmRoutine members,
allowing the table AM to dynamically register an implementation based on
conditions known at the start of each scan. That way a table AM can
provide specialized variants that help the compiler produce more
efficient code. heapam provides two implementations: one for plain
index scans, and one for index-only scans. An upcoming commit that adds
the amgetbatch interface will add two more. (Table AMs can opt to
provide a single generic implementation, or to provide many specialized
implementations, as requirements dictate.)
heapam's xs_getnext_slot implementations make aggressive use of forced
inlining to ensure that plain and index-only code paths are fully
specialized at compile time, without duplicating the code they have in
common. This specialization is necessary to keep icache and iTLB
pressure to a minimum.
The new interface moves visibility map checks required by index-only
scans out of the executor (and selfuncs.c) and into heapam, enabling
batching of visibility map lookups (though for now we continue to just
perform retail lookups). Using the new higher level slot-based
interface greatly simplifies nodeIndexonlyscan.c, which no longer has to
deal with the visibility map directly. More importantly, this is a
significant architectural improvement: table AMs can now implement
index-only scans that are not tied to heapam's visibility map.
Two users of the now removed table_index_fetch_tuple interface,
_bt_check_unique and unique_key_recheck, fundamentally need to pass a
TID to the table AM to perform constraint enforcement. Neither actually
performs an index scan (even though their TIDs are taken from an index),
so neither has any use for most of the index scan machinery. Switch
these callers over to fetch_tid, a new special-purpose table AM
interface for constraint enforcement code. All true index scan callers
now use the new slot-based interface. (Note that fetch_tid doesn't
perform on-access pruning, which matches how things worked prior to
Postgres 12 commits c2fe139c2 and 71bdc99d0.)
The VISITED_PAGES_LIMIT mechanism used by get_actual_variable_range to
cap scan overhead during planning is reworked to go through a new scan
descriptor field (xs_visited_pages_limit), rather than having selfuncs.c
count heap page fetches and terminate the scan itself. This is
necessary because callers that use the new slot-based interface no
longer have direct access to which heap blocks were fetched. Similarly,
nodeIndexonlyscan.c can no longer use InstrCountTuples2 to count heap
fetches during an EXPLAIN ANALYZE. EXPLAIN ANALYZE now obtains this
information from a new IndexScanInstrumentation field, which table AMs
are required to maintain during index-only scans.
Though independently useful, this commit is preparatory work for an
upcoming commit that will add an amgetbatch index AM interface, where
the table AM takes full responsibility for managing the progress of
index scans. The table AM determines when to request the next batch
from the index AM, at a time of its choosing. A follow-on commit will
rely on this to prefetch table blocks during index scans. Having a
single entry point for all index scan callers unambiguously puts the
table AM in control of the scan.
Author: Peter Geoghegan <pg@bowt.ie>
Reviewed-by: Andres Freund <andres@anarazel.de>
Reviewed-by: Tomas Vondra <tomas@vondra.me>
Reviewed-by: Rui Zhao <zhaorui126@gmail.com>
Discussion: https://postgr.es/m/CAH2-WzmYqhacBH161peAWb5eF=Ja7CFAQ+0jSEMq=qnfLVTOOg@mail.gmail.com
INSERT ... ON CONFLICT decides what to do based on the conflicting row
found by the arbiter index probe, but SSI never saw that read: the
probe runs with a dirty snapshot, which predicate locking ignores, and
the later fetch of the row uses SnapshotAny. When the statement then
writes nothing, as with DO NOTHING, DO UPDATE with a WHERE clause
rejecting the row, or DO SELECT, nothing records the read at all. A
concurrent writer of that row went unnoticed and write skew could
commit at SERIALIZABLE, even though the same schedule with a plain
SELECT of the row fails with a serialization error.
To fix, read the conflicting tuple again with the query snapshot,
right where the probe finds it. The table AM takes the SIREAD lock
and checks for a concurrent writer of the tuple as part of that read,
both under the buffer lock, so a writer either sees the lock or is
seen. A predicate lock by itself acquired separately after the probe
could not offer that: a writer passing its conflict check in between
would be missed. Doing this in the probe covers every conflict
action, including rows that the WHERE clause of DO UPDATE or DO SELECT
then rejects.
The DO NOTHING and DO UPDATE cases have been broken since ON CONFLICT
was added in 9.5; DO SELECT is new in v19. Backpatch to all supported
branches.
Author: Zsolt Parragi <zsolt.parragi@percona.com>
Author: Andrey Borodin <x4mmm@yandex-team.ru>
Reported-by: Andrey Borodin <x4mmm@yandex-team.ru>
Reported-by: Zsolt Parragi <zsolt.parragi@percona.com>
Discussion: https://postgr.es/m/787936C5-4155-4CF9-939D-39DC0EC1C892@yandex-team.ru
Discussion: https://postgr.es/m/CAN4CZFM1GkHJkpMeo4G5rxtacVsfeKCJYiik9E9AKX1E9VYQ1w@mail.gmail.com
Backpatch-through: 14
List of commits reverted:
8e72d914c5 Add UPDATE/DELETE FOR PORTION OF
b6ccd30d8f Add isolation tests for UPDATE/DELETE FOR PORTION OF
33db6c4baf Fix DELETE/UPDATE FOR PORTION OF with rules
7b22f15a01 Add psql tab completion for FOR PORTION OF clause
7ca8c94296 Fix FOR PORTION OF with non-updatable view columns
993a7aa0e4 Fix cross-leftover pollution in FOR PORTION OF insert triggers
7ac030d5b1 Require UPDATE permission on FOR PORTION OF column
f0aedc7cb0 Small terminology fixes in comments
7d13b03a2e Fix FOR PORTION OF for inheritance children
a272a58b94 Move FOR PORTION OF volatile check into planner
a40fdf6588 Reject child partition FDWs in FOR PORTION OF
bc3ae886a7 Forbid FOR PORTION OF with WHERE CURRENT OF
e994f956e4 Forbid generated columns in FOR PORTION OF
dfce19c230 Forbid FOR PORTION OF on views with INSTEAD OF triggers
20fe90bbb9 Fix RLS checks for FOR PORTION OF leftover rows
9170c8b716 Test what BEFORE UPDATE triggers do to FOR PORTION OF
7090c696cc Deparse FOR PORTION OF using the range column's current name.
62d1a5f8be Avoid RETURNING side effects for FOR PORTION OF leftovers.
9924e2d900 Enforce WITH CHECK OPTION on DELETE FOR PORTION OF leftovers
c16a1b4f2c Fix error code for null FOR PORTION OF target
610976420a Fix assertion failures in DELETE FOR PORTION OF tuple routing
438f027238 Resolve untyped parameters in FOR PORTION OF FROM/TO bounds
140fdfcdf1 Don't evaluate the FOR PORTION OF target under EXPLAIN
Discussion: https://www.postgresql.org/message-id/62bf70cc-dc33-4188-8070-a8c177bb61de%40eisentraut.org
Commit 0ee83dd4a9 added a call of assign_record_type_typmod() in
postgres_fdw.c, but relied on utils/typcache.h being included via
nodes/execnodes.h. That was put there as part of the FOR PORTION OF
feature, and if that were to change, there would be a possible
unrelated compilation failure. To fix, add the required include
directly.
A chunk_id type that does not match with one of the expected values (oid
or oid8) was missing a "return" clause. This caused the code to report
two corruptions instead of one.
Oversight in 6e58d6356f.
Discussion: https://postgr.es/m/aqju1K96ex28yz2X@paquier.xyz
A TOAST table whose chunk_id column is oid8 is now fed a 64-bit object
ID thanks to the introduction of a new vartag_external called
VARTAG_ONDISK_OID8, coupled with a new varatt_external_oid8. The only
difference between the existing varatt_external_oid and the new
varatt_external_oid8 is an extra 4 bytes to store 8 bytes worth an
object ID, spread across two uint32 to force alignement as memcmp may be
used for TOAST pointer comparison (line varatt_external_oid).
There is a one-one mapping between the vartag_external and the type of a
chunk_id:
- An oid8 chunk_id implies the use of VARTAG_ONDISK_OID8.
- An oid chunk_id implies the use of VARTAG_ONDISK_OID.
There are a couple of changes worth noting:
- In toast_tuple_init(), for the comparison of old and new external
values with a memcmp over VARSIZE_EXTERNAL(old_value). Now that
multiple types of external on-disk TOAST pointers are possible, we
need to add a check based on the vartag used by an old or new value.
One of the tests added by c68cba09dd cover this case.
- In toast_tuple_find_biggest_attribute(), the initial "biggest_size"
depends on the type of chunk_id. This needs a change in RelationData to
track the chunk_id type; this cached data is filled on demand.
- In detoast.h, toast_external_info_get() and toast_external_data act as
a translation layer to retrieve all the data associated to an Oid or an
Oid8 TOAST pointer, without the need to worry about the vartag dealt
with. The introduction of this function and structure has reduced a lot
of code churn that existed in the initial versions of this patch, as per
a suggestion from Bharath Rupireddy.
- amcheck is updated to check the vartag <-> chunk_id type mapping
requirements, handling both vartags depending on the underlying type of
chunk_id on the TOAST table.
Bump catalog version, due to at least the change of VARTAG_SIZE().
(This counts for the bump forgotten in a625fc570c22..)
Author: Michael Paquier <michael@paquier.xyz>
Reviewed-by: Greg Burd <greg@burd.me>
Reviewed-by: Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com>
Reviewed-by: Yugo Nagata <nagata@sraoss.co.jp>
Discussion: https://postgr.es/m/af19kUjwjhaoUTLn@paquier.xyz
The tests added by this commit cover a large ground in terms of the
introduction of oid8 in TOAST tables, with the following areas covered,
with all the gaps found during the development of the feature:
- Tables with oid8 TOAST tables in general, under strings.sql.
- ALTER TABLE with attribute manipulations in transactions and different
toast_table_value settings.
- Relation rewrites, not affecting the TOAST relation after initial
creation with VACUUM FULL, CLUSTER, REPACK.
- pg_column_compression().
- test_decoding, for external and extended TOAST data.
- amcheck, external and extended storage with heap checked.
- UPDATE with out-of-line datums that belongs to another TOAST table.
This offers coverage for toast_tuple_init(), where external vartags
could be mixed once multiple on-disk external TOAST pointers can
co-exist. This pattern can be triggered with asan/ubsan, with an
out-of-bound memcpy when unpatched.
These tests are added before the new vartag_external, mostly as a matter
of avoiding dead code in the tree.
Author: Michael Paquier <michael@paquier.xyz>
Reviewed-by: Greg Burd <greg@burd.me>
Reviewed-by: Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com>
Reviewed-by: Yugo Nagata <nagata@sraoss.co.jp>
Discussion: https://postgr.es/m/af19kUjwjhaoUTLn@paquier.xy
This commit adds the possibility to define TOAST tables with oid8 as
their chunk_id, based on the reloption toast_value_type. All the
external TOAST pointers still rely on varatt_external_oid and a single
vartag. The values inserted in the oid8 TOAST tables are fed from the
OID8 value generator, casted to OID for now, as we do not have a
vartag_external able to store Oid8 values yet.
An upcoming commit will add support for a new vartag_external and its
associated structures, with the code being able to use a different
on-disk external TOAST pointer depending on the attribute type of
chunk_id defined in TOAST relations.
All the changes done here are mechanical, mostly around the TOAST code
that needs to be able to do chunk_id lookups based on the two types now
supported.
Bump catalog version.
Author: Michael Paquier <michael@paquier.xyz>
Reviewed-by: Greg Burd <greg@burd.me>
Reviewed-by: Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com>
Reviewed-by: Yugo Nagata <nagata@sraoss.co.jp>
Discussion: https://postgr.es/m/af19kUjwjhaoUTLn@paquier.xy
d96f87332b folded COPY FREEZE visibility map updates into
XLOG_HEAP2_MULTI_INSERT records. Unlike the former XLOG_HEAP2_VISIBLE
redo path, multi-insert redo only updated the FSM when the record did
not contain a heap page FPI and the heap page had less than 20% free
space.
This can leave all-frozen pages with missing or stale FSM entries on a
standby. After promotion, vacuum will skip those pages, preventing
their free space from being discovered.
When a multi-insert record sets the page all-frozen, record the heap
page's free space unconditionally.
Reported-by: Melanie Plageman <melanieplageman@gmail.com>
Discussion: https://postgr.es/m/CAAKRu_bS3nfMJCF2fjE%2Bceb5-t%3D2p_VPMk2PWuVomrOFxg5DGQ%40mail.gmail.com
Backpatch-through: 19
add323da40 consolidated visibility map updates into XLOG_HEAP2_PRUNE
records. It assumed that a record setting the visibility map would
always modify the VM page during replay. That is not true when the
requested bits are already set on the standby.
Primary and standby visibility maps can diverge for several reasons.
This issue was exposed by a CREATE DATABASE ... STRATEGY WAL_LOG bug.
After a standby with an out-of-date VM is promoted, VACUUM may set bits
that were already present on the former primary. When the former primary
later replays that record as a standby, visibilitymap_set() is a no-op.
Restore visibilitymap_set()'s former API, which returns the state of the
VM bits before setting the requested flags. Use that result to set the
VM page LSN only when the visibilitymap_set() updates the page.
Reported-by: Rogers Wang <rogers.ww@qq.com>
Discussion: https://postgr.es/m/tencent_2E870046716FD94285045E96505A2D4E2908%40qq.com
Backpatch-through: 19
