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
The apply worker tracked the final LSN of the remote transaction being
applied in a separate global variable remote_final_lsn, even though
apply_error_callback_arg already tracked the same transaction's xid and
finish LSN for error context reporting, and both were updated together
at the same places.
Rename ApplyErrorCallbackArg to ApplyRemoteCtx and treat its
remote_xid/finish_lsn pair as the generic descriptor of the remote
transaction currently being applied, replacing all uses of
remote_final_lsn so the redundant global can be removed.
There is no behavior change.
Recording the remote transaction being applied in a single place should
also help the proposed conflict log table patch, which needs the same
information when logging a conflict.
Author: Zhijie Hou <houzj.fnst@fujitsu.com>
Author: Amit Kapila <amit.kapila16@gmail.com>
Reviewed-by: Hayato Kuroda <kuroda.hayato@fujitsu.com>
Reviewed-by: shveta malik <shveta.malik@gmail.com>
Reviewed-by: Dilip Kumar <dilipbalaut@gmail.com>
Discussion: https://postgr.es/m/TY4PR01MB177186ED4D38C5356294CC51C94A42@TY4PR01MB17718.jpnprd01.prod.outlook.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
Add a test for view containing labels which are shared by both vertex
and edge tables. When such a label is dropped from only vertex tables
or only edge tables, the view may be rendered invalid because it does
not find any elements associated with the label. Right now, you get
an error at run time. It would be correct per SQL standard to make
this a syntax error (parse time error), which would require some
restrict/cascade action when dropping labels. This is not a big
problem, because you still get a clean error, but it might be worth
improving sometime. Add a test case to record the current behavior.
Author: Ashutosh Bapat <ashutosh.bapat.oss@gmail.com>
Discussion: https://www.postgresql.org/message-id/flat/CAExHW5toFV8b984ipuBSAar-W50K%3D%2BXmPOcG5ZUZNP4jrf4zSg%40mail.gmail.com
This reverts commit 5e1e32c2c6.
It turns out that macOS still ships with an ancient libedit in the
base system that has the old definition of rl_readline_name.
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
It's reasonable to specify that the input string passed to these
functions isn't modified.
In each case, the input string is passed down to CStringGetDatum(),
which already takes a const char *, so this change just pushes that
API specification one level up.
A couple of callers benefit from this by no longer having to cast away
a const qualifier.
A few more functions that call input functions internally are changed
so that they can now offer a const qualified input string to their
callers.
Reviewed-by: Heikki Linnakangas <hlinnaka@iki.fi>
Discussion: https://www.postgresql.org/message-id/flat/459217ca-3840-4452-8cd1-8f9b47e8d479%40eisentraut.org
The pg_class fields relpages, relallvisible, and relallfrozen are
stored as int32 in the catalog but block numbers are internally of
type uint32. This all works correctly because the values are
implicitly converted back and forth according to C conversion rules.
But this was apparently never explicitly documented, so add this.
Reviewed-by: Chao Li <li.evan.chao@gmail.com>
Discussion: https://www.postgresql.org/message-id/flat/db993f34-e3e1-4e6e-85c3-4f248d2ff29b%40eisentraut.org
The pg_class fields relpages, relallvisible, and relallfrozen are of
type int32, but the statistics restoration code and some statistics
code in postgres_fdw internally dealt with them as uint32 for a little
bit, after which they would turn back into int32. This doesn't make
sense, so fix it to use int32 consistently throughout.
In passing, change a float to float4 to similarly make the C type
consistent with the SQL type.
Reviewed-by: Chao Li <li.evan.chao@gmail.com>
Discussion: https://www.postgresql.org/message-id/flat/db993f34-e3e1-4e6e-85c3-4f248d2ff29b%40eisentraut.org
final_cost_hashjoin() assumed that the rows a hash join produces come
from the outer side. That does not hold for JOIN_RIGHT_SEMI and
JOIN_RIGHT_ANTI, which produce rows from the inner side: matched inner
rows for the one, unmatched inner rows for the other. hashjointuples,
which carries both the cpu_tuple_cost charge and the remaining qual
costs, was therefore counting the wrong rows, and could be far off in
either direction.
Compute hashjointuples from the inner side for these two join types,
mirroring what JOIN_SEMI and JOIN_ANTI already do with the outer side.
The fraction we need is semifactors.outer_match_frac: it is derived
from the SpecialJoinInfo, so despite its name it always describes the
semijoin's left-hand side, which is the inner side here. Compute the
semijoin factors for these join types in all cases; previously that
happened only when the inner side was provably unique.
The same mix-up affects outer_matched_rows when the inner side is
known unique, where the outer row count was multiplied by the inner
side's match fraction. A unique inner side means each outer row has
at most one match, so use the number of matching pairs instead.
A right anti join evaluates the non-hashed joinquals once per tuple
passing the hash clauses, without any short-circuit. Charge the
remaining qual costs on that pair count, and only cpu_tuple_cost on
the emitted rows. A right semi join short-circuits already-matched
inner tuples and keeps the emitted-row charge, as JOIN_SEMI does.
Nestloop and mergejoin need no equivalent fix: neither supports
JOIN_RIGHT_SEMI, nestloop doesn't support JOIN_RIGHT_ANTI either, and
final_cost_mergejoin() takes its count from approx_tuple_count(),
which multiplies the two input sizes together and so gives the same
answer whichever side is on the outside.
No backpatch as this could result in plan changes.
Author: Richard Guo <guofenglinux@gmail.com>
Reviewed-by: Ayush Tiwari <ayushtiwari.slg01@gmail.com>
Reviewed-by: Haibo Yan <tristan.yim@gmail.com>
Reviewed-by: wenhui qiu <qiuwenhuifx@gmail.com>
Discussion: https://postgr.es/m/CAMbWs49XwhSC=e8_yeEaGKmKNyWR3DHH0p+e4k-bR_pgRiN8nQ@mail.gmail.com
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
Clearly define case mapping upper bounds that cover all providers,
locales, and encodings: PG_MAX_CASEMAP_MBCHARS,
PG_MAX_CASEMAP_EXPANSION, and PG_CASEMAP_BUFSZ.
Also redefine UTF8_CASEMAP_BUFSZ to include room for a NUL for
consistency with PG_CASEMAP_BUFSZ.
Use PG_CASEMAP_BUFSZ for the static buffer in ltree/crc32.c.
Remove macros UNICODE_CASEMAP_LEN (which was redundant with
UNICODE_MAX_CASEMAP_CODEPOINTS from unicode_limits.h), and
UNICODE_CASEMAP_BUFSZ which was not as descriptive or helpful.
Suggested-by: Heikki Linnakangas <hlinnaka@iki.fi>
Reviewed-by: Rithvika Devisetti <devisettirithvika@gmail.com>
Discussion: https://postgr.es/m/2cfbc37ae8deccd3825e36b7f31c391cbf8ff9b8.camel@j-davis.com
The previous commit made VACUUM apply a main table's storage
parameters to its TOAST table, as CREATE TABLE has long documented.
Autovacuum still gets this wrong in two ways. It falls back to the
main table's autovacuum parameters only when the TOAST table has no
reloptions at all, so setting a single toast.* parameter silently
stops the rest from being inherited. And it never consults the
main table for the non-autovacuum parameters, such as
vacuum_index_cleanup and vacuum_truncate.
To fix, combine the two sets with merge_toast_reloptions() rather
than choosing between them, and hand the main table's parameters
down to vacuum_rel() the way VACUUM now does.
pg_stat_autovacuum_scores uses the combined parameters for TOAST
tables as well; it has to collect the main relations' parameters
before it can do so, so it now makes a preliminary pass over
pg_class.
An existing shortcoming that this patch only makes worse is that
autovacuum remains oblivious to concurrent storage parameter
changes on the main table. That is, the main table's parameters
may be captured long before its TOAST table is processed, and a
user may very well have altered the settings in the meantime.
Fixing that would likely require additional pg_class lookups, and
it's not clear if it's worth the trouble.
While this is a bug fix, it's too intrusive for back-patching, but
the issue seems to have gone unnoticed for a very long time,
anyway.
Reviewed-by: Michael Paquier <michael@paquier.xyz>
Reviewed-by: Sami Imseih <samimseih@gmail.com>
Reviewed-by: Greg Burd <greg@burd.me>
Tested-by: solai v <solai.cdac@gmail.com>
Discussion: https://postgr.es/m/aFRxC1W_kZU9OjJ9%40nathan
Per the documentation for CREATE TABLE:
If a table parameter value is set and the equivalent toast.
parameter is not, the TOAST table will use the table's
parameter value.
Presently, VACUUM does no such thing. It reads the TOAST table's
own reloptions, which hold only what was set through toast.*, so
vacuum_index_cleanup, vacuum_max_eager_freeze_failure_rate, and
vacuum_truncate settings on the main table have no effect on its
TOAST table.
To fix, add merge_toast_reloptions(), which walks the parse table
for StdRdOptions and takes the main table's value for anything the
TOAST table left at its default. vacuum_rel() hands the main
table's parameters down when recursing to a TOAST table, and it
merges them into a copy of the TOAST table's parameters before the
values are used. This doesn't help VACUUM against a TOAST table
directly (e.g., "VACUUM pg_toast.pg_toast_5432"), but that's
probably okay because it's not the main supported way to vacuum a
TOAST table (see VACUUM's PROCESS_MAIN and PROCESS_TOAST options).
A follow-up commit will do the same for autovacuum. While this is
a bug fix, it's too intrusive for back-patching, but the issue
seems to have gone unnoticed for a very long time, anyway.
Reviewed-by: Michael Paquier <michael@paquier.xyz>
Reviewed-by: Sami Imseih <samimseih@gmail.com>
Reviewed-by: Greg Burd <greg@burd.me>
Tested-by: solai v <solai.cdac@gmail.com>
Discussion: https://postgr.es/m/aFRxC1W_kZU9OjJ9%40nathan
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
The unnest() test introduced in 0ee83dd4a9 did not produce a
deterministic EXPLAIN plan output.
The two sides of this test's nested loop join cost about the same, so
which one ended up as the outer wasn't stable and could flip between
platforms.
Use an equality predicate on ft1.c3 instead of a range one. That
restricts ft1 to a single row, so it's clearly the outer of the nested
loop and the Materialize on the inner side goes away, giving a stable
plan.
Author: Sami Imseih <samimseih@gmail.com>
Reviewed-by: Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com>
Discussion: https://postgr.es/m/CAA5RZ0uC%3DGOihOKcF7o3JVETF3e-Fkc4OUO5NbqMEKCTeSN-dQ%40mail.gmail.com
This calculation was failing to correctly account for the min_value when
calculating how many random members to add to the Bitmapset. What the
code was meant to do was add a random number of members between the
minimum and maximum values. The code failed to account for the minimum
value being non-zero, which could result in too many members being added
to the set, which could reduce the effectiveness of the test. In reality,
the only caller to this function did pass 0 as the min_value, so there is
no live bug being fixed here.
Fix this so that the Bitmapset is more sparsely populated with members
between the minimum and maximum values. In passing, swap the order of the
min_value and max_value parameters. Having the minimum value appear
before the maximum value makes more sense. Also align the names of both
parameters, and adjust the 'offset' calculation to widen the offset range
by 1 so that the possible range for that random value is -max_value to
max_value.
Author: David Rowley <dgrowleyml@gmail.com>
Reviewed-by: Greg Burd <greg@burd.me>
Discussion: https://postgr.es/m/CAApHDvpfZqdMRmR-6W9UXb+Tanex5wK6PsL=XzYCV4P3pD36Ng@mail.gmail.com
While stringToNode() was technically reentrant-safe, this was only for
single-threaded workloads: the string decoded was stored in a global
variable. This may be unsafe depending on the execution model, and
definitely unsafe in multi-threaded environments.
This patch moves the global variables into a struct (ReadNodeContext),
whose pointer is passed around on the stack. Support functions for node
read, extensible nodes (note: these have never been tested, should be
tested) and pg_strtok() are updated to handle the context data across
the call stack.
Code simplifications are mostly in stringToNodeInternal(), that can rely
on the context structure rather than a save-and-reassign logic based on
the global variables previously available.
Author: Matthias van de Meent <boekewurm+postgres@gmail.com>
Reviewed-by: Peter Eisentraut <peter@eisentraut.org>
Reviewed-by: Michael Paquier <michael@paquier.xyz>
Reviewed-by: Yuhang Qiu <iamqyh@gmail.com>
Discussion: https://postgr.es/m/CAEze2WjsGnOM4MB+n1LLdhhUg6r=hwWZRv+-heDE4UtzRdgjyg@mail.gmail.com
The functions moved in this commit have been exposed since c1ddd2361f,
and there still seem to be a couple of projects out there that require
them, as they can be more efficient than nodeRead(). Note that all
these functions are only called in readfuncs.c in terms of the core
code.
A follow-up patch plans to improve the in-core support of pg_strtok(),
making it thread-safe. This change removes the need to expose internals
specific to node reads in nodes.h, which is a header with a more general
purpose than readfuncs.h.
Based on a suggestion by me.
Author: Matthias van de Meent <boekewurm+postgres@gmail.com>
Discussion: https://postgr.es/m/aofW6ntfWJjk_yBL@paquier.xyz
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
execdebug.h includes "pretty old and crufty" code used to print
debugging information in the executor. As reported recently, this code
has not compiled in the last 10 years or so, and there are better
alternatives available, such as elog().
There is no indication that this facility is still being used, so remove
it along with its associated debugging macros:
EXEC_NESTLOOPDEBUG
EXEC_SORTDEBUG
EXEC_MERGEJOINDEBUG
Reviewed-by: Tom Lane <tgl@sss.pgh.pa.us>
Reviewed-by: Tatsuya Kawata <kawatatatsuya0913@gmail.com>
Discussion: https://postgr.es/m/aoFfAvp6xdk7Cdi6@paquier.xyz
609f969f6 reduced memory overheads for storing a tuple in Memoize. That
resulted in a Memoize eviction test no longer evicting anything from the
cache on 32-bit machines.
Here we fix that by adding another column to store in the cached
MinimalTuple.
Reported-by: Michael Paquier <michael@paquier.xyz>
Discussion: https://postgr.es/m/aof8KwoXaqOBFR_D@paquier.xyz
A follow-up commit will teach autovacuum and manual VACUUM to fill
in a TOAST table's unset storage parameters from its main table's.
To be able to determine whether a parameter is set, it must default
to a value that users cannot set. Of all the parameters that can
be set on TOAST tables, log_autovacuum_min_duration is the only one
that violates this rule. This commit changes its default to -2 so
that we can tell whether it is set. This shouldn't produce any
user-visible behavior changes.
While at it, add assertions to ensure all storage parameters that
can be set on TOAST tables continue to follow this rule.
Reviewed-by: Michael Paquier <michael@paquier.xyz>
Reviewed-by: Sami Imseih <samimseih@gmail.com>
Reviewed-by: Greg Burd <greg@burd.me>
Tested-by: solai v <solai.cdac@gmail.com>
Discussion: https://postgr.es/m/aFRxC1W_kZU9OjJ9%40nathan
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
Commit d6bf0ab170 introduced pg_parse_lsn() to validate LSNs given on
the command line of pg_waldump, pg_recvlogical, and pg_receivewal.
The remaining sscanf("%X/%08X") call sites under src/bin parse LSNs
that arrive in server responses, timeline history files, and
backup_label files. sscanf() accepts several forms that pg_lsn input
rejects and can silently continue with a different location than the
input text: a first component wider than eight hex digits wraps
around, a wider second component is truncated, and leading
whitespace, signs, "0x" prefixes, and trailing characters are
consumed or ignored.
Convert those call sites as well. The two call sites that read a
location out of a longer line isolate it by temporarily terminating
the string at the next whitespace character, so that they can use
pg_parse_lsn() like the others. Each tool keeps its existing error
message.
Malformed metadata now fails with each tool's existing error instead
of silently proceeding with a different location. Two error paths
shift: pg_rewind's history-file parser now rejects trailing
characters attached to a switchpoint, which used to be ignored, and a
malformed backup_label location now fails pg_combinebackup's "could
not parse" check rather than its "improper terminator" check.
Author: Zexin Li <lizi.openmind@gmail.com>
Reviewed-by: Fujii Masao <masao.fujii@gmail.com>
Discussion: https://postgr.es/m/CAAP6ZkS_3OH3yhhAGK6vu+2V1C2Hv4K6SpRuZL415R-gxjdTSg@mail.gmail.com
To prove that a forced-null Var is non-nullable in every matching row,
reduce_outer_joins() uses the strict quals that hold for every row a
subtree emits. The first pass currently gathers those quals into each
subtree's state, so that the second pass need not re-walk the jointree
at decision time.
That duplicates find_subquery_safe_quals(), which already performs
this traversal for query_outputs_are_not_nullable(), and it charges
every query containing an outer join for a list build at every
FromExpr and every inner or semi join, whether or not any upper qual
turns out to force a Var null. Since each level copies what its
children collected, the space required grows quadratically with the
depth of nested inner joins. What it buys is small even then: the
walk it avoids is cheap enough to be called on demand.
So rename find_subquery_safe_quals() to find_safe_quals(), export it,
and drop safe_quals from the first pass's state in favor of the
subtree's jointree node. The second pass calls find_safe_quals() at
the point where it needs the proof.
Also stop treating a semijoin there like a left join: semijoins are
reachable from the new caller, and a semijoin emits only matched
left-hand rows, so its ON quals do hold for every output row.
Author: Richard Guo <guofenglinux@gmail.com>
Discussion: https://postgr.es/m/CAMbWs4-6FazY7+vCOvR0-vqNuaXOZic8O4D4ZrwMm2woi+Q3uA@mail.gmail.com
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
This uses 16 fewer bytes per tuple stored in each MemoizeEntry. 24 bytes
can be saved by not doing palloc_object(MemoizeTuple), and instead we
can store the pointer to the next tuple by requesting that
ExecCopySlotMinimalTupleExtra() allocate an extra 8 bytes per
MinimalTuple, a net saving of 16 bytes per cached tuple, which equates
to 33% less memory in the most extreme case (assuming very few entries
with many tuples and the narrowest possible MinimalTuple). Real-world
cases will save less, as we're not saving anything on the MemoizeEntry
itself, only tuples stored within it.
Saving memory in Memoize allows more entries to exist concurrently and
reduces the chances that an entry will be evicted from the cache due to
memory pressure. That increases the chances of getting cache hits.
Author: David Rowley <dgrowleyml@gmail.com>
Reviewed-by: Chao Li <li.evan.chao@gmail.com>
Discussion: https://postgr.es/m/CAApHDvoNurQzpR2nZ4aLq1v_4gstsSdimZP-m34PMTBPptn+zQ@mail.gmail.com
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
