When neither BKPIMAGE_HAS_HOLE nor BKPIMAGE_COMPRESSED is set,
DecodeXLogRecord() checks that bimg_len needs to be equal to BLCKSZ.
However, the associated error message reported the data length
associated to a block (DecodedBkpBlock.data_len), and not the length of
the block (DecodedBkpBlock.bimg_len).
Oversight in 57aa5b2bb1, probably due to some copy-paste from the
surroundings.
Author: Ayush Tiwari <ayushtiwari.slg01@gmail.com>
Discussion: https://postgr.es/m/CAJTYsWVz9ymE7aMt2ZiQvs1bxNvhjfTf+XaJEsdFw7MT5dvtYQ@mail.gmail.com
Backpatch-through: 14
When adding support for OpenSSL 4, one occurrence of OpenSSL error
message matching was missed in the backpatches to PostgreSQL 17 and
16.
Reported-by: Christoph Berg <cb@df7cb.de>
Discussion: https://postgr.es/m/apky-gqiLVtFkR-w@msg.df7cb.de
Backpatch-through: 16,17
Previously, a session that had created temporary objects could hang during
exit if its default transaction mode was SERIALIZABLE READ ONLY DEFERRABLE
and a concurrent serializable transaction was prepared. During backend
exit, temporary-relation cleanup pushed a regular transaction snapshot,
which honored the session's default settings and could wait for a safe
serializable snapshot. So, if the conflicting transaction was prepared,
this wait could last indefinitely.
The wait occurred while the backend was already exiting and interrupts
were held off, so even pg_terminate_backend() could not cancel it. The
backend therefore remained until the prepared transaction was resolved.
Temporary-relation cleanup only needs an active MVCC snapshot for fetching
TOAST data from catalog tuples while dropping temporary objects. It does
not need a transaction snapshot affected by the user's default isolation
settings. So, use a catalog snapshot instead, which provides the needed
protection without entering the deferrable safe-snapshot wait.
Backpatch to all supported versions.
Bug: #19441
Reported-by: Alexander Lakhin <exclusion@gmail.com>
Author: Andrey Rachitskiy <pl0h0yp1@gmail.com>
Reviewed-by: Andrey Borodin <x4mmm@yandex-team.ru>
Reviewed-by: Fujii Masao <masao.fujii@gmail.com>
Discussion: https://postgr.es/m/19441-ec29f3b1363b4a68@postgresql.org
Backpatch-through: 14
When recovery starts from a backup without a signal file, pg_subtrans
is not started at the beginning of recovery and remains unstarted
throughout recovery. However, previously, a restartpoint run by
the checkpointer during recovery nevertheless tried to truncate
pg_subtrans, triggering the assertion failure:
TRAP: failed Assert("TransactionIdIsValid(initial)")
This commit fixes this by tracking whether pg_subtrans has been
started during recovery, and have the checkpointer check this flag
before truncating pg_subtrans at restartpoints.
Backpatch to all supported versions.
Reported-by: Imran Zaheer <imran.zhir@gmail.com>
Author: Imran Zaheer <imran.zhir@gmail.com>
Author: Fujii Masao <masao.fujii@gmail.com>
Reviewed-by: Michael Paquier <michael@paquier.xyz>
Discussion: https://postgr.es/m/CA+UBfamzfEReT0VOGRUW_=_AecCYQ-CNKLR_T4Q+YEmJAH3fdg@mail.gmail.com
Backpatch-through: 14
When reduce_outer_joins reduces an outer join to an antijoin, any IS
NULL qual on a Var from the antijoin's nullable side is necessarily
true. Previously, such quals were discarded later in
distribute_qual_to_rels, mainly to avoid bogus selectivity estimates.
But that discard was incomplete: the qual remained in the jointree,
while its Vars were not counted in attr_needed. Since commit
2ebf25e7d, join removal edits the jointree and expects it to contain
no other references to a removed rel, so it could remove a rel that
such a discarded qual still references, and then trip an assertion on
the qual's stale Var.
To fix, move this processing to an earlier phase: such quals are now
removed from the jointree by reduce_outer_joins itself. This way
later phases see a consistent query tree, and
check_redundant_nullability_qual is no longer needed, so remove it.
Back-patch to v16, as with commit 2ebf25e7d.
Reported-by: Tender Wang <tndrwang@gmail.com>
Author: Richard Guo <guofenglinux@gmail.com>
Reviewed-by: Tom Lane <tgl@sss.pgh.pa.us>
Discussion: https://postgr.es/m/CAHewXNk8b0TsSy4dL=CO7FXL2W3WBm0BcdP-zwNJePa-Qj4HzA@mail.gmail.com
Backpatch-through: 16
The VALUES reference page said that an AS clause was required when
VALUES is used in a FROM clause. This was imprecise because AS is
optional when specifying an alias.
In v14 and v15, a table alias is still required in this case, so state
that explicitly instead. In v16 and later, commit bcedd8f5fc made table
aliases for subqueries in FROM clauses optional, so state that the
table alias is optional there. In all branches, continue to recommend
explicit column aliases as good practice.
Backpatch to all supported versions.
Author: Ian Barwick <barwick@gmail.com>
Reviewed-by: Laurenz Albe <laurenz.albe@cybertec.at>
Reviewed-by: Fujii Masao <masao.fujii@gmail.com>
Discussion: https://postgr.es/m/CAB8KJ=jrvZdNLLhYSiCgbjLTz2LKEjYw+-HVQkaF+kgk61KtHA@mail.gmail.com
Backpatch-through: 14
A negative n means "return all but the first |n| characters", so
text_right() negates n before clipping. Negating PG_INT32_MIN
overflows; with -fwrapv the result is PG_INT32_MIN again, still
negative, and pg_mbcharcliplen() then returns an offset of zero,
so the whole string is returned where the correct answer is an
empty string:
SELECT right('abcdef', (-2147483648)::int4); -- 'abcdef', want ''
SELECT right('abcdef', -2147483647); -- '', correct
Clamp to PG_INT32_MAX instead. Any n whose absolute value is at
least the string's length skips all of it, and a text value cannot
be longer than PG_INT32_MAX, so this gives the same answer for every
other input. Erroring out, as text_format_string_conversion() does
for a width of INT_MIN, would not be correct here: unlike a format
width, an out-of-range skip count has a well-defined result.
Using pg_neg_s32_overflow() would be a slightly more optimal fix but
as it's only available in PostgreSQL 18 and later the decision was
taken to apply the same fix to all backbranches.
Backpatch to all supported versions.
Author: Ewan Young <kdbase.hack@gmail.com>
Reviewed-by: Daniel Gustafsson <daniel@yesql.se>
Reviewed-by: Dagfinn Ilmari Mannsåker <ilmari@ilmari.org>
Reviewed-by: David Rowley <dgrowleyml@gmail.com>
Reviewed-by: Chao Li <li.evan.chao@gmail.com>
Discussion: https://postgr.es/m/CAON2xHNnBz-AcPJgDmd5_39+8qR5AUKEZk4X3ZM-0zdsATn8kQ@mail.gmail.com
Backpatch-through: 14
When to_char() formatted an integer value with a V pattern, it could
return an incorrect result instead of reporting an overflow. V shifts
the decimal point by multiplying the input value by a power of ten before
formatting it, so, for example,
to_char(3, '9V999999999')
requires computing 3 * 10^9. This result does not fit in int4, but
the integer variant of to_char() performed the multiplication using a
plain int32 expression. The intermediate result could therefore
overflow, causing the function to output incorrect digits instead of
raising "integer out of range".
Use dtoi4() and int4mul() for this calculation so that both an
out-of-range multiplier and an out-of-range product are detected, as
with ordinary integer arithmetic. This also matches the existing int8
implementation, which uses dtoi8() and int8mul() for the same
operation.
After this change, to_char() with V format either returns the
correctly formatted result when the scaled value fits in int4, or
raises "integer out of range" when it does not.
Backpatch to all supported versions.
Reported-by: Andrey Rachitskiy <pl0h0yp1@gmail.com>
Author: Andrey Rachitskiy <pl0h0yp1@gmail.com>
Reviewed-by: Miłosz Bieniek <bieniek.milosz@proton.me>
Reviewed-by: Fujii Masao <masao.fujii@gmail.com>
Discussion: https://postgr.es/m/CAB8bMivEfqZxOVdzc3kZDN++XshmkEz2t7dfGBU8+oUm864EZg@mail.gmail.com
Backpatch-through: 14
The compound flags collected from COMPOUNDFLAG and friends are stored in
either the string or the integer member of a union, according to the
flag mode that the affix file's FLAG line declares. NIImportOOAffixes()
converted each flag as soon as it read it, using the mode in effect at
that point, and recorded that mode in the entry. Since FLAG may appear
anywhere in the file, including after the compound flags, entries written
before and after it could disagree about which member of the union holds
the flag.
In assert-enabled builds, this would result in an assertion failure.
Otherwise, cmpcmdflag() takes the mode from its first argument and
applies it to both, so it can read an integer as a char pointer and pass
that to strcmp(). Depending on which way the mismatch goes, the result
is a segfault while sorting the array, a segfault in the bsearch() that
later looks flags up (the lookup key is built with the final mode, so
this happens even when the array itself is consistent), or, when both
members happen to be readable, no crash at all and a compound flag that
is never found, which silently disables compound word splitting. This
isn't a security bug because we consider dictionary files to be trusted
data, but it's still worth fixing. (In practice, dictionary files
usually put the FLAG line first, which is why this went unreported for
so long.)
Fix by keeping the flags as strings while the file is read and converting
them once it has been read in full, when the mode is final. This also
makes the position of the FLAG line irrelevant, which is how the flags on
AF, SFX and PFX lines are already treated: those are parsed in a second
pass and so always use the final mode. That precedent is reason for
behaving this way rather than throwing an error.
The old ispell file format reaches addCompoundAffixFlagValue() too,
from NIImportAffixes(), and returns without entering NIImportOOAffixes(),
so it needs the conversion step as well.
While we're here, also fix some integer width mismatches: store the
result of strtol() into a "long", and cast to int only after we've done
range checks. Typically a value too wide for int would fail the range
checks anyway, but in some cases it would be silently accepted after
truncation to int.
Author: Ewan Young <kdbase.hack@gmail.com>
Reviewed-by: Tom Lane <tgl@sss.pgh.pa.us>
Discussion: https://postgr.es/m/CAON2xHN3QmsaySM6DGWa1gttcbJoFh0wjAE-_ZpSPo=LKN1hYw@mail.gmail.com
Backpatch-through: 14
analyzejoins.c decided which joins could be dropped by consulting the
planner's derived data structures, but then implemented the removal
by updating those structures in-place. That is a lot of fiddly work,
and nothing keeps it in step with the rest of the planner:
remove_leftjoinrel_from_query only bothered to update "parts of the
planner's data structures that will actually be consulted later", with
no good way to know what those are. Bug #19560 is one consequence.
In that report, removing a join leaves an EquivalenceClass that now
gives rise to a base restriction clause, but base restriction clauses
have already been generated and nothing reconsiders them, so the WHERE
condition disappears from the plan and we return wrong answers.
The self-join elimination code has the same design and the same type
of hazard. We have seen many related bugs over the years too, so it's
time to do something drastic.
To fix, do the removals by editing root->parse->jointree (which is a
far simpler and more stable representation than the derived data),
and then have query_planner() discard everything it computed from the
jointree and derive it over again. This requires quite a bit less
code, and doesn't require touching analyzejoins.c every time we change
the data derived by query_planner(). For typical cases it can actually
save a bit of planning time, though in cases where we have to iterate
the derivation loop many times it does add some time.
reduce_unique_semijoins() gets the same treatment: rather than deleting
the semijoin's SpecialJoinInfo and relying on the jointree not being
consulted again, it now changes the JoinExpr's jointype to JOIN_INNER
and recalculates everything.
Some plans change in the join regression test. Qual evaluation order
shifts in a few cases, because the conditions now reach later planning
in jointree order rather than in whatever order the removal code
re-distributed them. A few plans improve, since the rebuilt relation
targetlists no longer carry columns that only a removed join needed.
We also detect a constant-false filter condition whose test used to
carry a FIXME label. One plan gets marginally worse, because the old
code recomputed attr_needed from equivalence classes after a join
removal; that is more accurate than what deconstruct_jointree()
derives from the original clauses, but we no longer do that. Making
that recomputation happen anyway could be worth doing, but it should
be considered independently and perhaps implemented differently.
Back-patch to v16, on the grounds that the introduction of
varnullingrels in v16 made the old approach significantly more complex
and bug-prone; notably, bug #19560 does not manifest before v16.
In released branches, do not remove externally-visible fixup
functions such as remove_join_clause_from_rels, in case any
extensions are relying on them; but they're no longer used by core
code. But we must nonetheless break API/ABI for remove_useless_joins,
reduce_unique_semijoins, and remove_useless_self_joins, as those now
have different outputs and very different behavior than before.
It seems unlikely that any extensions are calling those; but just in
case, make the breakage more obvious by renaming remove_useless_joins
to remove_useless_outer_joins, which is a more sensible name for it
anyway since the addition of remove_useless_self_joins.
Full disclosure: initial drafts of this patch were made with
Claude Opus 4.8.
Bug: #19560
Reported-by: Orestis Markou <orestis@orestis.gr>
Author: Tom Lane <tgl@sss.pgh.pa.us>
Reviewed-by: Richard Guo <guofenglinux@gmail.com>
Reviewed-by: Thom Brown <thom@linux.com>
Reviewed-by: Jacob Brazeal <jacob.brazeal@gmail.com>
Discussion: https://postgr.es/m/1186816.1784573544@sss.pgh.pa.us
Backpatch-through: 16
collate.linux.utf8 skips itself unless version() matches "linux-gnu",
and infinite_recurse skips itself when version() matches
"powerpc64[^,]*-linux-gnu". configure substitutes the GNU host triplet
into that string, but the meson build composes it from
host_machine.cpu_family() and host_system, which never carries the ABI
suffix. So ever since meson support arrived in 16, collate.linux.utf8
has not run at all on a meson build, and infinite_recurse has been
running on ppc64 Linux the very case it means to stay away from.
Meson documentation says it reports 'ppc64' instead of 'powerpc64'.
Fix by matching "-linux[-,]" and "p(ower)?pc64[^,]*-linux", which match
all spellings. Keeping the punctuation on either side confines the
match to the platform field. Neither pattern excludes musl, but
collate.linux.utf8's other conditions already require a set of glibc
locales to be present.
Backpatch to 16, where the meson build was introduced.
Discussion: https://postgr.es/m/a40b19da-9a02-47b4-8afd-2bbbde8db1e8@dunslane.net
Reviewed-By: Jonathan Gonzalez V. <jonathan@abdiel.eu>
Reviewed-By: Nazir Bilal Yavuz <byavuz81@gmail.com>
When performing partition pruning with a RANGE partitioned table where
the pruning quals are only present for a leading prefix of the partition
key, it was possible that partition pruning would accidentally prune away
some partitions which shouldn't be pruned and include some partitions that
were not needed.
This happened due to an incorrectly coded loop bound which was
terminating the loop when the bound reached the first or last element in
the partition bound array. This resulted in those end elements not being
checked in cases where they should be checked. It appears that it might
have been coded this way to avoid stepping off the array, but that was
done incorrectly as it failed to take into account the direction of travel
through the array (the loop can go forwards or backwards). I.e., it's
valid to loop when 'off' is the last element if we're going backwards
through the array, and valid to loop if 'off' is 0 and we're looping
forward through the array, but the code as it was didn't allow that.
Here we fix this by moving the loop condition check to after we've
calculated the array element to process, and break from the loop if that
element is beyond either end of the array.
Example of accidentally pruned partition:
p: partition by range (a, b);
p1: for values from (1, 4) to (1, 7);
p2: for values from (1, 7) to (3, 8);
p3: for values from (4, 8) to (6, 9);
def: default;
select * from p where a <= 1;
Here p2 was pruned by mistake.
Example of accidentally not pruning a partition:
p: partition by range (a, b);
p1: for values from (7, 2) to (7, 7);
def: default;
select * from p where a > 7;
No partitions would be pruned in this case, despite it being impossible
for matching rows to exist in p1.
Author: David Rowley <dgrowleyml@gmail.com>
Reviewed-by: Ayush Tiwari <ayushtiwari.slg01@gmail.com>
Reviewed-by: Tender Wang <tndrwang@gmail.com>
Discussion: https://postgr.es/m/CAApHDvp5ne9AWaH-tG1Lke-USLz3NwWLWTUdP5NT7ypKtcFqcg@mail.gmail.com
Backpatch-through: 14
Previously, when wal_receiver_create_temp_slot was enabled, a timeline
switch could cause the walreceiver to try to create the same temporary
replication slot again on the same connection. The slot had already been
created before the first streaming attempt and still existed, so the
second creation attempt failed with a FATAL error such as
"could not create replication slot ...".
The walreceiver would later be restarted and streaming replication could
continue, so this did not permanently break replication. Nevertheless,
the unexpected failure is a bug and should be fixed.
Fix this by tracking whether the temporary replication slot has already
been created for the lifetime of the walreceiver and skipping subsequent
creation attempts. Also copy the retained slot name to shared memory on
each streaming attempt, since RequestXLogStreaming() clears it when
streaming is restarted without a configured primary slot. This also
keeps pg_stat_wal_receiver.slot_name populated after timeline switches.
Backpatch to all supported versions.
Author: ChangAo Chen <cca5507@qq.com>
Reviewed-by: Quan Zongliang <quanzongliang@yeah.net>
Reviewed-by: Fujii Masao <masao.fujii@gmail.com>
Discussion: https://postgr.es/m/tencent_628FDAF814231923BC8E8357BBBC50F94207@qq.com
Backpatch-through: 14
Refactor the checks in the function to move all the conditions for
when to attempt creating a shell type into one place. Add a check for
the array syntax.
In addition to rejecting array syntax, another user-visible effect is
that the error message is now different if the type specified a
typmod. You now get "type does not exist" instead of the more
specific "type modifier cannot be specified for shell type". That
seems better; the implicit shell type creation exists only for
backwards compatibility, and it never worked with type modifiers, so
if there's a type modifier it's most likely not because the user tried
to create a shell type,
Add test for the array syntax, the type modifier, and some other cases
for which we don't create shell types.
Discussion: https://www.postgresql.org/message-id/de673feb-41b4-4685-b24b-6408b95e58ab@iki.fi
Backpatch-through: 14
For a REPLICA IDENTITY FULL remote relation whose local counterpart has
no primary key or replica identity, FindUsableIndexForReplicaIdentityFull()
chooses the first index of a suitable shape from RelationGetIndexList().
That list excludes only indexes that are not indislive, so an invalid
index left behind by a failed CREATE INDEX CONCURRENTLY can be selected.
Such an index need not contain every row. Consequently, changes for rows
that it fails to find can be silently dropped as missing-tuple conflicts.
If the index contains no rows at all, the scan can instead error out and
cause the apply worker to exit.
Skip invalid indexes, as the planner does.
Author: Mikhail Nikalayeu <mihailnikalayeu@gmail.com>
Reviewed-by: Miłosz Bieniek <bieniek.milosz@proton.me>
Reviewed-by: Amit Kapila <amit.kapila16@gmail.com>
Reviewed-by: Shlok Kyal <shlok.kyal.oss@gmail.com>
Reviewed-by: Vignesh C <vignesh21@gmail.com>
Reviewed-by: Ajin Cherian <itsajin@gmail.com>
Discussion: https://postgr.es/m/CADzfLwWuubcbJBDRZ_J1SSqHDNjNmUYSAgf5y=17LxmP401xbw@mail.gmail.com
Backpatch-through: 16, where it was introduced
test_regex_utf8 decides whether to run by looking at the database
encoding alone, but two of its cases, [[:graph:]] and [[:print:]] over
E'xᔀሷ', depend on the ctype as well. In a database with encoding
UTF8 and locale C they match just the x, because isgraph() and isprint()
are false for anything outside ASCII, and the file fails.
No buildfarm animal builds such a cluster, which is why this went
unnoticed. A pending buildfarm client change will let an
animal be configured that way.
Fix by providing a second expected file holding the C-ctype answers, in
the manner of json_encoding.sql, which carries expected files for UTF8
and for SQL_ASCII. The .sql needs no change.
This applies to releases 15 and 16 only. A better solution is available
for release 17 and up.
Backpatch to v15, v16 only.
A snapshot taken during recovery stores all of its in-progress XIDs in
subxip, every running top-level XID included, leaving xip empty. Unlike
with other snapshots, its suboverflowed flag does not mean that subxip
is redundant. We nevertheless treated it that way during snapshot
export, so an importing session could see in-progress transactions as
aborted. This misbehavior could also lead to hint bits being
incorrectly set on the standby; affected tuples then wrongly appeared
visible or invisible to sessions that never imported the snapshot.
To fix, teach snapshot export to include the subxip[] array regardless
of the overflow flag when the snapshot is taken during recovery. This
is in line with how CopySnapshot() and SerializeSnapshot() already
handle the same issue.
Claude Code diagnosed this problem. The committed TAP test is a
simplified version of the one that it wrote to demonstrate this bug.
Oversight in commit 6c2003f8a, which enabled snapshot export and import
during recovery.
Author: Peter Geoghegan <pg@bowt.ie>
Author: Bertrand Drouvot <bertranddrouvot.pg@gmail.com>
Bug: #17846
Discussion: https://postgr.es/m/CAH2-WzmHVeYY%3Dpjz9x8DhhxVjXHX0pvoQ-MdiB1Tt6%3Do2GTiKg%40mail.gmail.com
Discussion: https://postgr.es/m/17846-1a0e5ce976f4c01a@postgresql.org
Backpatch-through: 14
Commit 46b4f5c11b made the logical replication origin checks quote
the schema and relation names they interpolate into the query sent to
the publisher. Those names can be NULL, which that commit overlooked.
AlterSubscription_refresh() collects the OIDs of the relations already
present in pg_subscription_rel and hands them to
check_publications_origin_tables() and
check_publications_origin_sequences(), which append the
schema-qualified name of each one to the query so that
already-subscribed relations are excluded from the check. The
relations are never locked, so one of them can be dropped concurrently
before its name is read, and get_rel_name() and get_namespace_name()
return NULL. quote_literal_cstr() dereferences it and crashes the
backend.
This commit fixes this by skipping a relation whose name is no longer
available. A dropped relation is not synchronized anyway, and the
appended clauses only exclude relations from a check whose sole effect
is a WARNING, so omitting one can at most produce a spurious WARNING.
The window is reachable from ALTER SUBSCRIPTION ... REFRESH
PUBLICATION and from SET, ADD and DROP PUBLICATION, which refresh by
default, but only when copy_data is true and origin is none. Backpatch
to v16 as commit 46b4f5c11b was back-patched that far. The sequence
path exists only in v19 and later.
The test is applied to v19 and later only. Adding it to v17 and v18
would require enabling injection point support in
src/test/subscription there, and v16 predates injection points
entirely. That is more test infrastructure churn on stable branches
than this fix warrants.
Reported-by: SATYANARAYANA NARLAPURAM <satyanarlapuram@gmail.com>
Author: SATYANARAYANA NARLAPURAM <satyanarlapuram@gmail.com>
Co-authored-by: Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com>
Reviewed-by: Ajin Cherian <itsajin@gmail.com>
Reviewed-by: Masahiko Sawada <sawada.mshk@gmail.com>
Discussion: https://postgr.es/m/CAHg+QDcd_o3707Ey8c8b7HkE-t14g8c0tk8ME3ctywDsh3ut8g@mail.gmail.com
Backpatch-through: 16
39dcfda2d fixed an incorrect reuse of ResultRelInfos for AFTER triggers
when the ResultRelInfo needed to have a different ri_RootResultRelInfo.
That caused an issue in logical replication apply workers as
finish_edata() neglects to call ExecCloseResultRelations() and instead
relies on ExecCleanupTupleRouting() to close relations opened during
partitioning's tuple routing. Since 39dcfda2d, because we may have done
some additional table_opens() calls due to having to create an additional
ResultRelInfo because of requirements to have a different
ri_RootResultRelInfo, we should now be explicitly closing any relations
opened on ResultRelInfos in EState's es_trig_target_relations.
Since finish_edate() seems to want to avoid calling
ExecCloseResultRelations(), add a new external function named
ExecCloseTrigTargetRelations().
Reported-by: Hayato Kuroda (Fujitsu) <kuroda.hayato@fujitsu.com>
Author: Hayato Kuroda (Fujitsu) <kuroda.hayato@fujitsu.com>
Author: David Rowley <dgrowleyml@gmail.com>
Reviewed-by: Zhijie Hou (Fujitsu) <houzj.fnst@fujitsu.com>
Discussion: https://postgr.es/m/OS9PR01MB121491E7E05950D108AF9A6D8F5A72@OS9PR01MB12149.jpnprd01.prod.outlook.com
Backpatch-through: 15
query_is_distinct_for() treated a subquery's DISTINCT ON clause as
proof that its output is unique over the DISTINCT ON columns, even if
the targetlist contains set-returning functions. That's not true:
when the query has an ORDER BY, the planner postpones evaluation of
SRFs that are not DISTINCT ON or ORDER BY columns until after the
Unique step, so the subquery can produce duplicates of the DISTINCT ON
columns. Relying on this bogus uniqueness proof allowed join removal
and unique-inner joins to produce wrong results.
Plain DISTINCT is not affected, since all tlist columns are DISTINCT
columns there, and so any SRFs get expanded before the Unique step.
To fix, make query_supports_distinctness() and query_is_distinct_for()
refuse to prove distinctness via DISTINCT ON if the targetlist
contains any SRFs. This is more conservative than necessary, since
the SRFs are only postponed when there is an ORDER BY and none of them
appear in a sort/group column, but it doesn't seem worth the trouble
to check that precisely.
Author: Richard Guo <guofenglinux@gmail.com>
Reviewed-by: Tom Lane <tgl@sss.pgh.pa.us>
Discussion: https://postgr.es/m/CAMbWs4-hfd1Pyy_zBejsVUSy-3dx16rz2hgUakkKnAg3qg2q=Q@mail.gmail.com
Backpatch-through: 14
GIN posting tree page deletion failed to consider whether the target
page's left sibling page, or the deletion target itself, was marked as
incompletely split. Page deletion finds the target page's left sibling
by walking the parent's downlinks, but an incompletely split page's new
right half is part of the sibling chain despite having no downlink.
Deletion could therefore overwrite the rightlink of the wrong page,
disconnecting the split's still-live right half from the sibling chain.
Scans would then silently miss tuples from that page.
To fix, teach the relevant page deletion path to avoid deleting a
posting tree page whose left sibling is marked incompletely split (and
to avoid doing so when the target page itself is so marked). This is
essentially the same approach used by nbtree page deletion.
Claude Code found this problem. The committed test case is a simplified
version of the one that it wrote to demonstrate this bug.
Author: Peter Geoghegan <pg@bowt.ie>
Reviewed-by: Andrey Borodin <x4mmm@yandex-team.ru>
Discussion: https://postgr.es/m/CAH2-Wz=sKJcn+OtfVN9rdg+Ps9e4cuQWNP-9t12UE2d8nEG90Q@mail.gmail.com
Backpatch-through: 14
The test that checks the expected plan for this self-join test has been
known to have failed in the past due to badly timed VACUUMs causing
small variations in row estimates on one of the tables, resulting in a
swapped join order. Currently, failures have only been seen in v14, and
seemingly due to 74388a1ac and 4496020e6 the failures have not been seen
in more recent versions.
Here we shrink down the number of matching rows on one side of the join
to make the alternative join order's costs more expensive relative to
the cheapest join order. Previously the alternative order had the same
cost.
We do this in all supported versions to reduce the chances of future
changes reintroducing stability issues with these queries.
Reported-by: Alexander Lakhin <exclusion@gmail.com>
Author: David Rowley <dgrowleyml@gmail.com>
Discussion: https://postgr.es/m/f5d1f4c2-6224-4797-be17-c86e77f96c9c@gmail.com
Backpatch-through: 14
ProcArrayInstallImportedXmin verifies that the source transaction (the
transaction whose snapshot we're importing) is still running, and then
installs the caller's imported xmin. These steps have to be atomic.
But it was just about possible for VACUUM to fail to observe the
imported xmin in either the source proc or the importing one. This
could result in VACUUM pruning away deleted tuples that were still
visible to the imported snapshot.
To fix, take ProcArrayLock in exclusive mode while importing an exported
snapshot's xmin within ProcArrayInstallImportedXmin. That guarantees
that a concurrent VACUUM's OldestXmin cannot advance past the xmin (one
proc or the other always advertises an xmin that holds it back).
Author: Chee Wooson <chee.wooson@gmail.com>
Reviewed-by: Peter Geoghegan <pg@bowt.ie>
Discussion: https://postgr.es/m/20260730042128.714201-1-chee.wooson@gmail.com
Backpatch-through: 14
Previously, in the path for non-deterministic collations, the code
assumed that bsize==rsize. That assumption seems to be true for ICU,
and all non-deterministic collations are ICU, so it's not known to be
an actual bug.
The only known place where bsize may not equal rsize is in the libc
provider, where strxfrm() can return an upper bound of the size needed
to store the result. That means the initial call to determine the
buffer size (with dest==NULL, n==0) could return a larger number than
the actual call with an adequate dest buffer. That's OK, because libc
locales are always deterministic.
Commit 679c5084cf partially fixed the assumption, but missed this
part. Fix it, and add a more prominent documentation note.
Reviewed-by: Haibo Yan <tristan.yim@gmail.com>
Discussion: https://postgr.es/m/CABXr29Hb31nkj1g2Jmk+1BhAm=3ecGs_pWy4tU++j8CQBnbMxQ@mail.gmail.com
Backpatch-through: 16
A sequence has no rowtype. We called
find_composite_type_dependencies() with InvalidOid, which is harmless
but pointless. Skip it.
This started to happen with commit 344d62fb9a in v15, which added
the ALTER SEQUENCE ... SET LOGGED/UNLOGGED subcommand. Before that,
sequences were never rewritten. While this is harmless, backpatch to
keep the code the same on all branches, to make backpatching future
patches a little easier.
Backpatch-through: 15
The number of trigger arguments is stored as a smallint, but there was
no check that the number of arguments fits with the catalog data type.
This could result in an invalid negative value being stored once one
defined more than INT16_MAX arguments, with an overflowed value stored
in the catalogs.
Looking at other catalogs that store a number of arguments, we have
similar protections already in place (aggregates, functions, etc.).
Reported-by: Xingwang Xiang <v3rdant.xiang@gmail.com>
Author: Kyotaro Horiguchi <horikyota.ntt@gmail.com>
Discussion: https://postgr.es/m/19627-5b72a57e332e2b3f@postgresql.org
Backpatch-through: 14
GiST neglected to invalidate its killedItems[] array on a rescan. As a
result, it was just about possible for the wrong tuples from the wrong
index page to be LP_DEAD-marked on a rescan. The scan mistakenly
believed that the previous rescan's killedItems[] were for this rescan's
curBlkno, causing index corruption.
To fix, bring GiST in line with nbtree and hash: call gistkillitems from
both gistrescan and gistendscan (the existing gistgettuple caller still
handles the common case where we need to LP_DEAD-mark before moving on
to the next page). That way the scan's pending killedItems[] are passed
to gistkillitems while they still describe items from curBlkno. When
gistkillitems runs, it'll invalidate the array in passing (and won't
needlessly miss out on an opportunity to LP_DEAD-mark eligible index
tuples). Back branches just get minimal hardening: we invalidate
killedItems[] at the places where the master branch gets new calls to
gistkillitems (and we invalidate curBlkno and curPageLSN on a rescan).
The test that proved corruption on master didn't result in corruption on
any stable branch, though only because, without commit 9c9ddf109, we'd
clobber curPageLSN without also updating curBlkno -- which accidentally
prevented it. Relying on gistkillitems to not LP_DEAD-mark by passing
it a curBlkno whose curPageLSN was taken from an entirely different page
seems like a very bad idea, which is why this issue is being treated as
a bug affecting all stable branches.
Author: Peter Geoghegan <pg@bowt.ie>
Reviewed-By: Andrey Borodin <x4mmm@yandex-team.ru>
Discussion: https://postgr.es/m/CAH2-WzmwEThnQf17Ju+t0N9_KJLsEQSXzYrFnaS2=s4KnGGrqw@mail.gmail.com
Backpatch-through: 14
ginbulkdelete performs pending list cleanup before it searches the entry
tree (and any posting trees) for dead TIDs. This is necessary to avoid
leaving behind dangling TID references that index vacuuming is required
to remove; nothing prevents recently inserted pending list tuples from
containing TIDs that VACUUM already considers dead.
However, ginbulkdelete neglected to perform pending list cleanup on
VACUUM's second or subsequent call. It was therefore possible for a
VACUUM that requires multiple rounds of index vacuuming to leave behind
dangling references.
To fix, teach ginbulkdelete to perform pending list cleanup during every
call. In passing, tweak some related comments in the pending list
cleanup path to make it clear why it's safe for VACUUM to not _fully_
empty an index's pending list.
This was arguably an oversight in commit e2c79e14, which fixed a similar
issue where pending list cleanup by VACUUM could end early, but missed
this closely related problem.
Author: Peter Geoghegan <pg@bowt.ie>
Reviewed-by: Andrey Borodin <x4mmm@yandex-team.ru>
Discussion: https://postgr.es/m/CAH2-Wzmsa-RPA2Ko8A5LaGOnmbpimJ--71xkiBqwgjk3Fq8YEg@mail.gmail.com
Backpatch-through: 14
ginVacuumPostingTreeLeaves swaps a shared buffer lock for an exclusive
one when it encounters a leaf page. It neglected to re-verify whether a
page that was initially a leaf root page became an internal page due to
a concurrent root page split (during the window when no lock was held).
It was therefore possible for GIN VACUUM to spuriously treat an internal
page as a leaf page, leading to data corruption. VACUUM could miss dead
TIDs that it was required to remove, leaving behind dangling references
in the index.
To fix, re-verify that a leaf page is still a leaf page after an
exclusive lock is acquired. If it isn't, drop our exclusive lock and
acquire a shared lock so that the non-leaf root page gets processed in
the usual way.
Oversight in commit fd83c83d, which fixed a deadlock bug in GIN posting
tree vacuuming.
Author: Peter Geoghegan <pg@bowt.ie>
Reviewed-by: Andrey Borodin <x4mmm@yandex-team.ru>
Discussion: https://postgr.es/m/CAH2-Wz=RBpJTQgvOxr6C=J04dExmFSt1E3F-r+cRTQ56hEotkg@mail.gmail.com
Backpatch-through: 14
psql_scan_slash_option() returns a malloc'd string, but \getresults,
\gset in pipeline mode, \restrict, and \unrestrict did not free it
after consuming or copying the value.
Free these option strings after use.
Backpatch to all supported versions. In v17 and older, only \restrict
and \unrestrict are affected, so those branches need only that part of
the fix.
Author: Fujii Masao <masao.fujii@gmail.com>
Reviewed-by: Chao Li <li.evan.chao@gmail.com>
Discussion: https://postgr.es/m/CAHGQGwEh3R3=1tx_a5=fTDJ+ycuwxWMEn6bG_Yt4B5P+hE7AVw@mail.gmail.com
Backpatch-through: 14
psql_scan_slash_option() builds option text in a local PQExpBufferData
and returns the buffer's data pointer to its caller. If either the initial
allocation or a later enlargement failed, that data pointer could be
the static PQExpBuffer OOM buffer rather than malloc-owned storage.
The callers could then eventually pass it to free(), causing undefined
behavior.
Detect a broken option buffer before returning it, report OOM, and return
NULL instead. Also avoid evaluating a backtick substitution when the option
buffer is already broken, since doing so could otherwise touch the static
OOM buffer.
This keeps the existing NULL-return convention for slash options. Callers
are not generally changed to distinguish OOM from no option.
Backpatch to all supported versions.
Reported-by: Junwang Zhao <zhjwpku@gmail.com>
Author: Fujii Masao <masao.fujii@gmail.com>
Reviewed-by: Chao Li <li.evan.chao@gmail.com>
Reviewed-by: Junwang Zhao <zhjwpku@gmail.com>
Discussion: https://postgr.es/m/CAHGQGwEh3R3=1tx_a5=fTDJ+ycuwxWMEn6bG_Yt4B5P+hE7AVw@mail.gmail.com
Backpatch-through: 14
psql_scan_slash_command() builds the command name in a local
PQExpBufferData and returns the buffer's data pointer to its caller. If
either the initial allocation or a later enlargement failed, that data
pointer could be the static PQExpBuffer OOM buffer rather than malloc-owned
storage. HandleSlashCmds() could then eventually pass it to free(), causing
undefined behavior.
Detect a broken command-name buffer before returning it, report OOM, and
return NULL instead. Teach HandleSlashCmds() to treat a NULL command name
as a command error before trying to compare or dispatch it.
Backpatch to all supported versions.
Reported-by: Junwang Zhao <zhjwpku@gmail.com>
Author: Fujii Masao <masao.fujii@gmail.com>
Reviewed-by: Chao Li <li.evan.chao@gmail.com>
Reviewed-by: Junwang Zhao <zhjwpku@gmail.com>
Discussion: https://postgr.es/m/CAHGQGwEh3R3=1tx_a5=fTDJ+ycuwxWMEn6bG_Yt4B5P+hE7AVw@mail.gmail.com
Backpatch-through: 14
ReorderBufferProcessTXN() opens every relation referenced by a
TRUNCATE change. When RelationIsLogicallyLogged() returns false,
it skips the relation without releasing the reference acquired
by RelationIdGetRelation().
Looking at the in-core code paths building XLOG_HEAP_TRUNCATE records,
no relation OIDs would be included if they do not satisfy
RelationIsLogicallyLogged(). One pattern that could go through is if a
table is switched to SET UNLOGGED, but that would not be reachable in
practice as the decoding happens after a historical snapshot is taken,
so the relation should still be valid.
This is a defense-in-depth measure in practice, and we tend to be
careful about how Relations are handled when sending changes to output
plugins, so backpatch all the way down.
Author: Chao Li <li.evan.chao@gmail.com>
Reviewed-by: Xuneng Zhou <xunengzhou@gmail.com>
Discussion: https://postgr.es/m/7DD65D03-3B5A-43B2-99AD-8E6AF5372BAB@gmail.com
Backpatch-through: 14
Base backups reported checksum failures to pg_stat_database only for
files with more than one failing page. Commit 6b9e875f72 placed the
report inside the block emitting the per-file summary WARNING, which
was skipped for a single failure. As a result, a backup failing on
files with one corrupted page each left checksum_failures untouched.
To fix, emit the per-file summary and the pgstat report for any
non-zero failure count. The end-of-backup total WARNING had the same
off-by-one and is now also emitted for a single failure.
Author: Zsolt Parragi <zsolt.parragi@percona.com>
Reviewed-by: Nazir Bilal Yavuz <byavuz81@gmail.com>
Discussion: https://postgr.es/m/CAN4CZFN+Bi6XmaH8zOdMWjoycYFx9nKtOr+dzQf0o-UQ+Rdqmw@mail.gmail.com
Backpatch-through: 14
Tied hashes, and probably tied arrays, are capable of returning Perl
value pointers that are actually NULL, not the usual pointer to an
undef SV. We were not defending against that everywhere, leading
to possible SIGSEGV. Fix the code to consistently treat a null
pointer returned from hv_iternext or av_fetch like a !SvOK one.
(Note that the large diff in SV_to_JsonbValue is actually quite
trivial, but it required reindenting a chunk of existing code.)
Claude Code found the instance in hstore_plperl, and I found the
others by code auditing. Perhaps the other instances aren't
actually reachable, but I see little reason to assume that.
The known test cases for these errors require perl's Tie modules,
which may not be present, so it doesn't seem worth the trouble
to create regression test cases that would cover them.
Reported-by: Claude Code (via Noah Misch)
Author: Tom Lane <tgl@sss.pgh.pa.us>
Discussion: https://postgr.es/m/569769.1786901901@sss.pgh.pa.us
Backpatch-through: 14
textsearch.sgml neglected to mention that the MAXSTRPOS total-length
limit applies to tsquery as well as tsvector. It also claims that
there is a 32K limit on the total number of nodes in a tsquery, which
is wrong. (I suspect that QueryOperator.left may once have been
int16, which would give rise to such a limit. But it's uint32 now,
so you'd hit the 1GB varlena limit well before overflowing that.)
While at it, re-order the bullet points into an order that makes
more sense, to me anyway.
Reported-by: Claude Code (via Noah Misch)
Author: Tom Lane <tgl@sss.pgh.pa.us>
Reviewed-by: Chao Li <li.evan.chao@gmail.com>
Discussion: https://postgr.es/m/455079.1786897319@sss.pgh.pa.us
Backpatch-through: 14
tsqueryrecv() accepted zero-length lexemes, which tsqueryin() doesn't.
It also accepted phrase distance values larger than MAXENTRYPOS,
which tsqueryin() doesn't. While neither of these omissions are
very harmful in themselves, they do allow accepting tsquery values
that will fail in a subsequent textual dump/reload.
Commit 23d9ad771 performed similar tightening of tsvectorrecv(),
but I left off these changes at the time because they didn't seem
to have security implications.
Reported-by: Claude Code (via Noah Misch)
Author: Tom Lane <tgl@sss.pgh.pa.us>
Reviewed-by: Chao Li <li.evan.chao@gmail.com>
Discussion: https://postgr.es/m/455079.1786897319@sss.pgh.pa.us
Backpatch-through: 14
This flag hasn't been useful since the removal of old-style VACUUM FULL,
so remove it now (this includes removing vestigial code that
unnecessarily set the flag).
Also add test coverage of gistprunepage(). Had that test case been
available before now, the issue fixed by this commit would have been
detected by wal_consistency_checking buildfarm animals.
Author: Peter Geoghegan <pg@bowt.ie>
Reviewed-by: Michael Paquiër <michael@paquier.xyz>
Discussion: https://postgr.es/m/CAH2-WznbTsQCrjmd=eSawfPqcxCjSFUkk6Qzd3z+gpNte5i03Q@mail.gmail.com
Backpatch-through: 14
plperl_func_handler()'s stanza for handling an arrayref result in
a SETOF function could loop forever (or at least till OOM) when
given a tied array, since av_fetch won't necessarily ever return
a null pointer in that case. Be consistent with the other places
where we traverse a perl array: call av_len() once and use len+1
as the loop limit, silently ignoring any null pointers we get back
from that range of subscripts.
But actually, Perl's preferred locution for this seems to be to
use av_count() not av_len()+1. av_count() seems better since
there's less risk of forgetting to add 1. Also, both of those
functions return Size_t (or SSize_t) not int, creating at least
a theoretical overflow hazard. While we're modernizing this,
let's use the correct variable type where we can, and include an
overflow check where we can't.
Reported-by: Claude Code (via Noah Misch)
Author: Tom Lane <tgl@sss.pgh.pa.us>
Reviewed-by: Andrey Rachitskiy <pl0h0yp1@gmail.com>
Discussion: https://postgr.es/m/569769.1786901901@sss.pgh.pa.us
Backpatch-through: 14
Presently, the documentation for logging_collector says that the
collector "is designed to never lose messages," which reads as a
stronger promise than we actually make. The collector does not
fsync the log file or retry failed writes, so log messages can go
missing after an operating system crash, power loss, or a write
error. Reword that sentence and add a note about what is not
guaranteed.
Author: Daniel Bauman <danielbaniel@gmail.com>
Reviewed-by: Fujii Masao <masao.fujii@gmail.com>
Reviewed-by: Zhenwei Shang <a934172442@gmail.com>
Reviewed-by: Robert Treat <rob@xzilla.net>
Discussion: https://postgr.es/m/CAMtj0_a86DdDKkW-ReVpQpqjndVS6GMrwXVpQY4G3-SGY7saMQ%40mail.gmail.com
Backpatch-through: 14
When SendQuery() is not told how many COPY FROM STDIN commands the
query string contains (as for -c, \gexec, and \watch), it scans the
string to count them itself. But it called psql_scan() only once,
which stops at the first semicolon, so any COPY FROM STDIN past the
first sub-command was not counted, causing failure of cases that used
to work. Oversight in commit 3045a25ba.
Author: Zsolt Parragi <zsolt.parragi@percona.com>
Reviewed-by: Tom Lane <tgl@sss.pgh.pa.us>
Discussion: https://postgr.es/m/CAN4CZFPqa6c+u4uX5jJ8LANHTQ4dxM3m4_8G9WmX_A4-2wuv2A@mail.gmail.com
Backpatch-through: 14
Presently, check_role_membership_authorization() decides whether
the current user may grant or revoke membership in a role by
calling is_admin_of_role(), which recurses through all grants,
while it chooses the grantor to record for the resulting entry by
calling select_best_admin(), which recurses only through inherited
grants. When the two disagree, the permission check passes and the
grantor lookup then comes up empty, so the user sees an internal
"no possible grantors" error. ALTER GROUP ... ADD USER reaches the
same error through the separate check in AlterRole().
To fix, teach both checks to search the same way
select_best_admin() does via a new has_admin_privs_of_role(). The
new check passes exactly when the grantor lookup was going to
succeed, so nothing that works today starts failing; the internal
error simply becomes a proper permission error. Note that this
leaves the other callers of is_admin_of_role() alone, so a role
reachable only through a non-inherited grant can still be dropped,
renamed, or altered. Whether that ought to change as well is left
as a future exercise.
Oversight in commit ce6b672e44.
Reported-by: ChangAo Chen <cca5507@qq.com>
Author: ChangAo Chen <cca5507@qq.com>
Reviewed-by: Chao Li <li.evan.chao@gmail.com>
Reviewed-by: Pretham <prezza672@gmail.com>
Reviewed-by: Robert Haas <robertmhaas@gmail.com>
Reviewed-by: Jacob Champion <jacob.champion@enterprisedb.com>
Discussion: https://postgr.es/m/tencent_ADCE2B34B230A9B631854806104FEF40C105%40qq.com
Discussion: https://postgr.es/m/CAJUn_kN%2BMhbb8fYP5xxQCq1KEziOinM6HgYx4ts_pPDnQ2y1nQ%40mail.gmail.com
Backpatch-through: 16
Some places rejected individual tokens longer than MAXSTRLEN, while
others rejected ones longer than MAXSTRLEN-1. The data structure is
perfectly capable of handling MAXSTRLEN, so there's nothing wrong
with using the looser bound. Moreover, as things stand there is a
dump/reload hazard: some code paths permit construction of a tsvector
or tsquery that would later be rejected by tsvectorin or tsqueryin.
So standardize on using MAXSTRLEN.
Identical remarks apply to MAXSTRPOS (the total data length),
so fix that too.
Back-patch, in hopes of avoiding cases where a value acceptable to
one supported release is not acceptable to another.
Author: Tom Lane <tgl@sss.pgh.pa.us>
Reviewed-by: Zsolt Parragi <zsolt.parragi@percona.com>
Discussion: https://postgr.es/m/CAN4CZFNYQo4zfbRR435uD0vSfuy5y7dnFOXDfKr9zYoL1JnAxA@mail.gmail.com
Backpatch-through: 14
On timeout, poll_query_until() would fail to print its report when
"$query" was undefined. Instead of the expected report, this prevented
the contents of stdout and stderr from being shown.
Note that some of the in-core tests use an undefined query for
connection-only polls.
Author: Bryan Green <dbryan.green@gmail.com>
Reviewed-by: Jonathan Gonzalez V. <jonathan.abdiel@gmail.com>
Discussion: https://postgr.es/m/0f515c6d-6032-4c5f-80ac-5c78faae9522@gmail.com
Backpatch-through: 14
By default, allocate this via malloc, as we've been doing since commit
62c8421e8. Commit aeb07c55f adopted upstream tzdb's default of
allocating it on the stack, but that still doesn't seem like a good
idea for the reasons given in 62c8421e8 (and now memorialized in a
comment, in hopes that we don't make the same mistake again).
However, under USE_VALGRIND, put it on the stack as upstream does.
This accidentally prevents a crash when Python 3.14 is used under
Valgrind. The reasons for that are obscure, and it's most likely
not our bug, and even if we figured it out it'd be nice to have a fix
for buildfarm member skink now rather than after persuading the guilty
party to fix it.
In the normal non-USE_VALGRIND case, this has no effect on the logic
in released branches, and it reverts master to match them.
Reported-by: Alexander Lakhin <exclusion@gmail.com>
Author: Tom Lane <tgl@sss.pgh.pa.us>
Discussion: https://postgr.es/m/f071e691-5930-4738-9dbd-43ed2da367fe@gmail.com
Backpatch-through: 14
