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
The REPACK grammar accepts ONLY before the table name and * after
the table name, but that's neither documented nor handled in the
code. Perhaps REPACK should support that syntax, but for now let's
just bring it in line with its documentation.
Oversight in commit ac58465e06.
Reviewed-by: Antonin Houska <ah@cybertec.at>
Discussion: https://postgr.es/m/apBTsWGwkLXVh8Ow%40nathan
Backpatch-through: 19
AutoVacuumUpdateCostLimit() runs after each nap in vacuum_delay_point()
and follows av_nworkersForBalance, but the new limit never reached the
shared cost params in the vacuum DSM: propagation only required config
reload. Parallel workers computed their delays from the stale limit,
so a parallel autovacuum could run at up to twice the configured budget
(or half of it) until the next SIGHUP, contradicting the propagation
promise in maintenance.sgml.
Call parallel_vacuum_propagate_shared_delay_params() after rebalancing.
Gated on the leader: parallel workers take the same nap path and must
not overwrite the shared parameters.
This also adds a test to validate the behaviour: pause the leader at the
existing injection point, start a second autovacuum worker and hold it at
a new injection point placed after it joined the balance, then check the
first parameter load of the parallel workers reports the balanced limit.
The hold is needed because a second worker left running can finish its own
vacuum before the leader resumes, which puts the balance back where it
started. Autovacuum is disabled for everything but the two test tables
via thresholds, as a worker spawned by catalog churn would get trapped at
the hold point and starve the test of its second worker slot.
Backpatch to v19 where autovacuum gained the ability to use parallel
vacuum workers.
Author: Zsolt Parragi <zsolt.parragi@percona.com>
Reviewed-by: Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com>
Reviewed-by: Masahiko Sawada <sawada.mshk@gmail.com>
Reviewed-by: Daniel Gustafsson <daniel@yesql.se>
Discussion: https://postgr.es/m/CAN4CZFOZtEPwGQ6oa9LvHvN522zEp8h_dW9hHExSh7pVXofoKQ@mail.gmail.com
Backpatch-through: 19
In v19, pg_stat_autovacuum_scores computes a TOAST table's scores
from its own storage parameters alone. On the other hand,
autovacuum falls back to the main table's parameters when the TOAST
table has none. This means that the view may report scores that
don't match what autovacuum would calculate. This contradicts the
documented promise that the view generates its results the same way
autovacuum workers do. To fix, teach the view to do the same
fallback. As in do_autovacuum(), we must make a preliminary pass
over pg_class to collect the main tables' parameters, since the
pg_class scan may see TOAST tables before their main tables.
Commit fad70a09ff for v20 improved autovacuum's handling of TOAST
storage parameters and adjusted the view to match, but it was
deemed too intrusive to back-patch. This fix is for v19 only.
Oversight in commit 87f61f0c82.
Reported-by: Masahiko Sawada <sawada.mshk@gmail.com>
Reviewed-by: Masahiko Sawada <sawada.mshk@gmail.com>
Discussion: https://postgr.es/m/CAD21AoB1CJRVfCDh8qYuD3eueiygXxk7F3nybgjN0RZXSD-QUw%40mail.gmail.com
Backpatch-through: 19 only
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
If SHMEM_CALLBACKS_ALLOW_AFTER_STARTUP is used to allocate shared
memory after startup, but the initialization fails half-way through,
the shmem area is left in an indeterminate state. Furthermore, if
multiple shmem areas are registered in one RegisterShmemCallbacks()
call, some might be allocated while others are not.
This commit adds an explicit 'initialized' flag to each shmem area.
We still leave behind an uninitialized area on error, but at least
they are now clearly marked, and you get a slightly nicer error
message if you try to re-register them. It'd be nice to clean up more
thoroughly and support actually retrying the allocations, but in
practice, the most likely reason for a shmem allocation or
initialization to fail is that you are out of shared memory and
retrying wouldn't help with that.
This isn't exactly a new problem, the old ShmemInitStruct() interface
had similar issues if the initialization code failed, or if you
allocated multiple structs and some allocations failed. It was just
left to the calling code to deal with it.
Author: Ayush Tiwari <ayushtiwari.slg01@gmail.com>
Reviewed-by: Ashutosh Bapat <ashutosh.bapat.oss@gmail.com>
Discussion: https://www.postgresql.org/message-id/CAJTYsWVRRWH48=PcuAo_2Y4Ap6M0QRmzxgUfFkNRtdWK74LjBQ@mail.gmail.com
Backpatch-through: 19
RegisterShmemCallbacks() left the backend in a bad state, if an error
occurred in the callbacks or if an allocation failed. Firstly,
'shmem_request_state' was left in wrong state, causing a subsequent
call to RegisterShmemCallbacks() to wrongly take the postmaster
startup codepath or assertion failures in some other functions.
Secondly, the 'pending_shmem_requests' list was not properly cleaned
up, causing a subsequent RegisterShmemCallbacks() to try to process
the stale, already-freed requests.
To fix, add a PG_TRY() block to clean those things up on error.
Author: Ayush Tiwari <ayushtiwari.slg01@gmail.com>
Reviewed-by: Ashutosh Bapat <ashutosh.bapat.oss@gmail.com>
Discussion: https://www.postgresql.org/message-id/CAJTYsWVRRWH48=PcuAo_2Y4Ap6M0QRmzxgUfFkNRtdWK74LjBQ@mail.gmail.com
Backpatch-through: 19
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>
create_append_path() skips cost_append() when an Append has exactly
one child whose parallel awareness matches its own, since setrefs.c
strips such an Append out entirely. In that case it copies the
child's rowcount and costs directly, but it failed to copy
disabled_nodes. An Append over a disabled child therefore claimed to
contain no disabled nodes, letting a disabled path win over one that
is not disabled.
This is a regression in v18; before e22253467, disable_cost was folded
into a path's startup and total costs, so it rode along in the fields
this shortcut already copies.
Back-patch to v18. This can change plans in stable branches, but only
for installations that have explicitly disabled a node type, and only
to stop using the node they asked us to avoid.
Reported-by: Man Zeng <zengman@halodbtech.com>
Author: Tender Wang <tndrwang@gmail.com>
Reviewed-by: Richard Guo <guofenglinux@gmail.com>
Reviewed-by: David Rowley <dgrowleyml@gmail.com>
Discussion: https://postgr.es/m/CAHewXNm_Zx5EDoaD7wo7bq6cfRroNznS+RBCzT_p2-CWQXpgSw@mail.gmail.com
Backpatch-through: 18
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
Commit bd8d9c9bdf widened MultiXactOffset to 64 bits, but
pg_control_checkpoint() still handle checkPointCopy.nextMultiOffset as
xid type and declared next_multi_offset column as xid. Since xid is 32
bits wide, an offset above 2^32 was reported truncated, while
pg_controldata printed the full value of the same field.
This commit reports the column as bigint instead. That matches
pg_get_multixact_stats(), which already reports num_members and
members_size, both derived from these same offsets, as int8.
Backpatch to v19, where MultiXactOffset was widened.
Bump catalog version.
Reviewed-by: Heikki Linnakangas <hlinnaka@iki.fi>
Reviewed-by: Chao Li <li.evan.chao@gmail.com>
Discussion: https://postgr.es/m/CAD21AoCvzerscfU8o4ARQ793yAGHpQ72r2x5apeC_W2-k=SLCQ@mail.gmail.com
Backpatch-through: 19
Commit bd8d9c9bdf widened MultiXactOffset to 64 bits and widened
ControlData.chkpnt_nxtmxoff accordingly, but get_control_data() still
read the "Latest checkpoint's NextMultiOffset" line with str2uint(),
which returns unsigned int.
This commit adds str2uint64(), mirroring the str2uint() helper used
for the other control file fields, and reads the offset with it.
Backpatch to v19, where MultiXactOffset was widened.
Reviewed-by: Heikki Linnakangas <hlinnaka@iki.fi>
Reviewed-by: Chao Li <li.evan.chao@gmail.com>
Discussion: https://postgr.es/m/CAD21AoCvzerscfU8o4ARQ793yAGHpQ72r2x5apeC_W2-k=SLCQ@mail.gmail.com
Backpatch-through: 19
FOREIGN_JOIN target sublists must contain at least two relation
identifiers. However, the parser checked only for sublists with
exactly one identifier, so FOREIGN_JOIN(()) was accepted.
Reject sublists with fewer than two relation identifiers, and add
regression coverage.
Author: Chao Li <lic@highgo.com>
Discussion: http://postgr.es/m/BEDC04E0-6732-4310-95BA-6EC34BC1442C@gmail.com
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
The pg_restore_*_stats() functions could report SQLSTATE XX000 for
invalid variadic arguments, such as an unmatched name/value pair, a NULL
argument name, or a non-text argument name. Attribute and extended
statistics restores could also report XX000 when the supplied statistics
exceed the number of slots PostgreSQL can store.
These are not internal errors. They result from invalid caller input or
a PostgreSQL implementation limit, but the lack of specific SQLSTATEs
made clients treat them as internal errors.
Assign appropriate SQLSTATEs to these errors so that applications and
tests can classify them correctly.
Backpatch to v19, but no further; changing ERRCODE assignments in
released stable branches doesn't seem like a good idea.
Bug: #19629
Reported-by: Zheng Wang <hackerzheng666@gmail.com>
Reported-by: Yanjie Zhao
Reported-by: Yiyang Liu
Author: Fujii Masao <masao.fujii@gmail.com>
Discussion: https://postgr.es/m/19629-76babc04b683594d@postgresql.org
Backpatch-through: 19
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, and why the to_date() crash in 18.5 went undetected for want
of exactly this coverage. A pending buildfarm client change will let an
animal be configured that way.
Fix by giving the two cases an explicit collation, so that they exercise
a fixed Unicode ctype instead of whatever the database happened to be
initialized with. test_regex() already passes its input collation down
to the regex compiler. The expected results are unchanged; only the
echoed queries differ.
Backpatch-through: 17 (15 and 16 get a different fix)
Reviewed-by: Jonathan Gonzalez V. <jonathan@abdiel.eu>
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
GetSubscriptionRelations() can see a pg_subscription_rel row for a
relation whose pg_class row was removed by a concurrent DROP. In this
case, get_rel_relkind() returns '\0'. Skip such relations since they no
longer exist and do not need synchronization.
Replace the assertion on relkind with an error. Once the dropped
relation case is handled, any other unexpected relkind indicates a
relation kind that cannot be part of a subscription and should be
reported rather than ignored.
Author: Vignesh C <vignesh21@gmail.com>
Reviewed-by: Amit Kapila <amit.kapila16@gmail.com>
Reviewed-by: Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com>
Reviewed-by: Hayato Kuroda <kuroda.hayato@fujitsu.com>
Reviewed-by: Kyotaro Horiguchi <horikyota.ntt@gmail.com>
Backpatch-through: 19
Discussion: https://postgr.es/m/CALDaNm3eeKocRtQyUPdg2kaxJ-VAo5pwcNytDKNT1Yc0t2V_ug%40mail.gmail.com
Adding REPACK introduced table-rewrite behavior that overlaps with
VACUUM FULL and CLUSTER, but some documentation still mentioned only one
of these commands or described the progress and locking behavior
imprecisely.
Clarify that table-rewriting operations, including REPACK, can change
CTIDs. Also describe VACUUM FULL, CLUSTER, and REPACK consistently as
table-rewriting operations for disk-space recovery where appropriate,
document the pg_stat_progress_repack and compatibility
pg_stat_progress_cluster views consistently, and clarify the ACCESS
EXCLUSIVE locking behavior of REPACK, including CONCURRENTLY.
Backpatch to v19, where REPACK was introduced.
Author: Marcos Pegoraro <marcos@f10.com.br>
Author: Fujii Masao <masao.fujii@gmail.com>
Discussion: https://postgr.es/m/CAB-JLwaLcHyf=OnLnBsJEbdd57WjWPpK5T7iGFJvAQphjTZj_A@mail.gmail.com
Backpatch-through: 19
When a hash table grows large enough we try to expand the directory,
but we shouldn't try to do that if the hash table is marked as
HASH_FIXED_SIZE. Before commit 9fe9ecd516 that was harmless,
although it was a little weird to expand the directory when allocating
the new element was doomed to fail later anyway. But the new
allocator function added in that commit for shared memory hash tables
did not expect to be called after hash table creation at all and would
just crash. Fix by ensuring that the allocator is not called after
hash table creation for HASH_FIXED_SIZE tables.
The crash started with commit 9fe9ecd516 in v19, so backpatch to v19.
To trigger the crash, you needed a shared memory hash table of just
the right size so that we would attempt to resize it: power-of-2 and
at least HASH_SEGSIZE (256) elements. With any other size, you would
run out of allocated elements first.
Author: Konstantin Knizhnik <knizhnik@garret.ru>
Reviewed-by: Rahila Syed <rahilasyed90@gmail.com>
Discussion: https://www.postgresql.org/message-id/d59221f2-b3d2-41ad-8bf0-d581b42e4cba@garret.ru
Backpatch-through: 19
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
53a981ed5 adjusted test_random_offset_operations() so that the minimum
value parameter came before the maximum value parameter. Here we adjust
test_random_operations() so that it accepts absolute numbers for the
minimum and maximum values. Previously the maximum value to put in the
random Bitmapset was the minimum value plus the range. Also align the
parameter order with test_random_offset_operations() to make these
functions consistent with each other.
Backpatch to v19 as test_random_operations() is new there.
Author: David Rowley <dgrowleyml@gmail.com>
Reviewed-by: Greg Burd <greg@burd.me>
Discussion: https://postgr.es/m/66d041b4-32f4-4097-bb2c-0e7147337a5e@app.fastmail.com
Backpatch-through: 19
Explicitly make space for the NUL when casefolding.
No known bug in the previous code, because the previous buffer (size
12) was more than large enough for folding any codepoint with enough
room left for a NUL. The builtin provider's limit is 7; ICU's limit
seems to be 7 also; and libc always does 1:1 mappings so the real
limit is MAX_MULTIBYTE_CHAR_LEN + 1 (size 5).
Reviewed-by: Heikki Linnakangas <hlinnaka@iki.fi>
Reviewed-by: Rithvika Devisetti <devisettirithvika@gmail.com>
Discussion: https://postgr.es/m/2cfbc37ae8deccd3825e36b7f31c391cbf8ff9b8.camel@j-davis.com
Backpatch-through: 18
Test the API independently to account for fallback paths that aren't
adequately tested from SQL.
The backport to 18 also tests the previously-supported behavior where
a size of -1 meant that the string was NUL-terminated. That behavior
was later removed in 19.
Reviewed-by: Andres Freund <andres@anarazel.de>
Discussion: https://postgr.es/m/v36ssaygf7grb3qzfsjhtdzi7kqd45ds56nyuf7gi5qjml4qbb@ezmfqzmhlrs2
Backpatch-through: 18
Commit 42473b3b3 implemented SupportRequestSimplifyAggref, but failed
to think about what infrastructure would be required for an extension
to use that: there is no SQL-level mechanism for attaching a planner
support function to an aggregate. That seems pretty critical for a
feature that's primarily intended to be used by extensions.
To fix, add a SUPPORT clause to CREATE AGGREGATE, and teach pg_dump
(and thereby pg_upgrade) about dumping this aggregate property.
We don't need to touch ALTER AGGREGATE, because it's already the case
that you're supposed to use CREATE OR REPLACE AGGREGATE if you want
to alter any aggregate-specific properties set by CREATE AGGREGATE.
(Maybe at some point we'll think that that policy ought to change,
but I don't think this one feature moves the needle enough.)
Per report from Andrei Lepikhov, who also provided some of the
new documentation text.
Reported-by: Andrei Lepikhov <lepihov@gmail.com>
Author: Tom Lane <tgl@sss.pgh.pa.us>
Reviewed-by: Andrei Lepikhov <lepihov@gmail.com>
Discussion: https://postgr.es/m/8f58c96d-d3c7-4c0f-9898-116f00eeaff6@gmail.com
Backpatch-through: 19
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
Commit 4113873 confined RI fast-path batching to the top transaction
level to avoid mishandling the batch cache on subtransaction abort.
That disabled batching for a foreign-key load wrapped in a savepoint,
such as:
BEGIN; SAVEPOINT s; COPY fk_table FROM ...
This was a surprising performance cliff and departed from the usual
per-subtransaction resource handling.
Track cache entries per subtransaction instead. Add AtEOSubXact_RI(),
called from CommitSubTransaction() and AbortSubTransaction() after
ResourceOwnerRelease(). On abort, it removes only entries opened by
the ending subtransaction, whose resources have just been released,
while leaving entries opened by an outer level intact. Thus, an inner
subtransaction abort during outer-level trigger firing does not
discard the outer statement's batch. On commit, no matching entry is
expected because its batch should already have been flushed at
statement end. Each entry records the subtransaction that opened its
resources.
After an abort, the remaining slot storage and per-entry flush contexts
are reclaimed when TopTransactionContext is reset at top-level
transaction end.
A fast-path batch is filled and flushed within a single trigger-firing
cycle, so every row added to an entry must come from the subtransaction
that created it. AtEOSubXact_RI() relies on this invariant to identify
an aborting subtransaction's entries by the subid stamped at entry
creation. Assert the invariant in ri_FastPathBatchAdd().
Add regression coverage for batching during nested firing inside a
subtransaction, both with different constraints and with the same
constraint at the inner and outer firing levels.
Reported-by: Noah Misch <noah@leadboat.com>
Reported-by: Nikolay Samokhvalov <nik@postgres.ai>
Discussion: https://postgr.es/m/20260705222115.be.noahmisch@microsoft.com
Backpatch-through: 19
When the target expression of FOR PORTION OF (...) evaluated to NULL,
ExecInitModifyTable raised an error without an errcode, so clients got
the internal error code XX000 for a user-reachable condition.
Oversight in commit 8e72d914c5.
To fix, report ERRCODE_NULL_VALUE_NOT_ALLOWED, and reword the message
to "FOR PORTION OF target must not be null", matching similar executor
messages such as "frame starting offset must not be null".
Bug: #19630
Reported-by: Zheng Wang <hackerzheng666@gmail.com>
Reported-by: Yanjie Zhao
Reported-by: Yiyang Liu
Author: Zsolt Parragi <zsolt.parragi@percona.com>
Discussion: https://www.postgresql.org/message-id/flat/19630-9f10ca28426295fa%40postgresql.org
In pipeline mode, an invalid \getresults argument could previously
affect the next SQL command in the same pipeline. For example, after
reporting an error for \getresults -1, psql could treat the following
SQL command as a request to read pending pipeline results instead of
sending it to the server, making the command appear to be skipped or
causing missing results.
This happened because psql marked \getresults as a request to read
pipeline results before validating its optional argument. When
validation failed, psql reported the error without running the normal
cleanup path that clears the request.
Fix this by validating the \getresults argument before marking the
command as a request to read pipeline results. After an invalid
argument, psql now reports the error and sends the following SQL command
normally.
Backpatch to v18, where psql pipeline meta-commands were introduced.
Author: Fujii Masao <masao.fujii@gmail.com>
Reviewed-by: Anthonin Bonnefoy <anthonin.bonnefoy@datadoghq.com>
Discussion: https://postgr.es/m/CAHGQGwGkgM2HKZeig5hobEgUCjn7MMJux4UCcN=OfOzOEvRT0A@mail.gmail.com
Backpatch-through: 18
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
The three error recovery checks in `test_handle()` used "qr/^|ok$/" to
look for the marker "ok" in psql's output. '^' matches every string, so
the assertions passed no matter what psql printed.
Spelling the regex correctly as "qr/^ok\|$/" exposed that the explicit
xact case was actually failing, reporting an incorrect "current
transaction is aborted" instead of showing that an AIO handle can be
acquired again after an error. This is rewritten with a ROLLBACK,
similarly to the subxact counterpart.
While on it, the subxact case had no marker column in its query, so add
one there for consistency, and reformat to use same pattern.
Author: Jelte Fennema-Nio <me@jeltef.nl>
Discussion: https://postgr.es/m/DKSU6GI1YLG5.3VF6M4IRKQ7XE@jeltef.nl
Backpatch-through: 18
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
