Previously, string_to_uuid() decoded one byte at a time, calling
isxdigit() twice and strtoul() once for every pair of hexadecimal
digits. Commit ec8719ccbf made hex_decode_safe() decode a run of
hexadecimal digits in bulk, so add a fast path for the two common
shapes: 32 contiguous hexadecimal digits, and the canonical
8x-4x-4x-4x-12x form, each optionally wrapped in braces.
Anything else, and any decoding error, falls back to the
byte-at-a-time parser, so the accepted grammar and the error messages
don't change. hex_decode_safe() also skips whitespace, which the UUID
grammar forbids, so we treat a short decode as an error too. Rejecting
an input that has a fast-path shape therefore costs a second
parse. But this only happens for syntax-error cases, so it's not a
problem in practice.
Reviewed-by: Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com>
Reviewed-by: Haibo Yan <tristan.yim@gmail.com>
Reviewed-by: Chao Li <li.evan.chao@gmail.com>
Reviewed-by: John Naylor <johncnaylorls@gmail.com>
Discussion: https://postgr.es/m/CAD21AoCqeR4UQU77Q_yOMNNzJ7AVeiO5QZT+4HnzPm4Wm-e02Q@mail.gmail.com
Otherwise, a read of blocks the stream is about to throw away can be
started and then waited for. read_stream_reset drains the stream by
calling read_stream_next_buffer, whose look-ahead treats the zeroed
distance as end of stream, leading to it spuriously performing the
pending read. This caused a pathological regression for a nested loop
antijoin query when run with the upcoming index prefetching patch series
applied.
This bug affects all stable branches with the read stream, which was
added to Postgres 17 by commit b5a9b18cd0. No backpatch for now,
though, since without index prefetching the added overhead for realistic
query plans is negligible.
Author: Tomas Vondra <tomas@vondra.me>
Discussion: https://postgr.es/m/CAH2-WzkKY8b42sZSPWJceT5Fn_THk3ZgcaM8ch3Hzw+XvhP0zQ@mail.gmail.com
Commit 0c5d626961 was confused about which indexes are potentially
problematic to rebuild: we thought only !indisready indexes could fail
to rebuild, and thus would attempt to rebuild !indisvalid ones that are
"ready" (that is, being maintained by DML but not yet completed).
However, that's wrong, because it could be that the validation phase of
such an index failed or was interrupted partway through. So, have
REPACK throw an error with all !indisvalid indexes, not just those
!indisready.
(!indisready implies !indisvalid, so this is an expansion of what
happened with the previous commit.)
Reported-by: shihao zhong <zhong950419@gmail.com>
Backpatch-through: 19
Discussion: https://postgr.es/m/CAGRkXqRA4YetT8Nu3+1-45+ffTwi2bd1d1eDFME39Y9OPRMytQ@mail.gmail.com
Add table_index_getnext_slot, a new table AM interface that serves as
the single entry point for both plain and index-only index scans that
use amgettuple. The table AM's index_scan_begin callback resolves which
implementation the scan will use and stores it in an IndexScanDesc
function pointer (xs_getnext_slot), which table_index_getnext_slot
dispatches through.
The interface intentionally avoids using fixed TableAmRoutine members,
allowing the table AM to dynamically register an implementation based on
conditions known at the start of each scan. That way a table AM can
provide specialized variants that help the compiler produce more
efficient code. heapam provides two implementations: one for plain
index scans, and one for index-only scans. An upcoming commit that adds
the amgetbatch interface will add two more. (Table AMs can opt to
provide a single generic implementation, or to provide many specialized
implementations, as requirements dictate.)
heapam's xs_getnext_slot implementations make aggressive use of forced
inlining to ensure that plain and index-only code paths are fully
specialized at compile time, without duplicating the code they have in
common. This specialization is necessary to keep icache and iTLB
pressure to a minimum.
The new interface moves visibility map checks required by index-only
scans out of the executor (and selfuncs.c) and into heapam, enabling
batching of visibility map lookups (though for now we continue to just
perform retail lookups). Using the new higher level slot-based
interface greatly simplifies nodeIndexonlyscan.c, which no longer has to
deal with the visibility map directly. More importantly, this is a
significant architectural improvement: table AMs can now implement
index-only scans that are not tied to heapam's visibility map.
Two users of the now removed table_index_fetch_tuple interface,
_bt_check_unique and unique_key_recheck, fundamentally need to pass a
TID to the table AM to perform constraint enforcement. Neither actually
performs an index scan (even though their TIDs are taken from an index),
so neither has any use for most of the index scan machinery. Switch
these callers over to fetch_tid, a new special-purpose table AM
interface for constraint enforcement code. All true index scan callers
now use the new slot-based interface. (Note that fetch_tid doesn't
perform on-access pruning, which matches how things worked prior to
Postgres 12 commits c2fe139c2 and 71bdc99d0.)
The VISITED_PAGES_LIMIT mechanism used by get_actual_variable_range to
cap scan overhead during planning is reworked to go through a new scan
descriptor field (xs_visited_pages_limit), rather than having selfuncs.c
count heap page fetches and terminate the scan itself. This is
necessary because callers that use the new slot-based interface no
longer have direct access to which heap blocks were fetched. Similarly,
nodeIndexonlyscan.c can no longer use InstrCountTuples2 to count heap
fetches during an EXPLAIN ANALYZE. EXPLAIN ANALYZE now obtains this
information from a new IndexScanInstrumentation field, which table AMs
are required to maintain during index-only scans.
Though independently useful, this commit is preparatory work for an
upcoming commit that will add an amgetbatch index AM interface, where
the table AM takes full responsibility for managing the progress of
index scans. The table AM determines when to request the next batch
from the index AM, at a time of its choosing. A follow-on commit will
rely on this to prefetch table blocks during index scans. Having a
single entry point for all index scan callers unambiguously puts the
table AM in control of the scan.
Author: Peter Geoghegan <pg@bowt.ie>
Reviewed-by: Andres Freund <andres@anarazel.de>
Reviewed-by: Tomas Vondra <tomas@vondra.me>
Reviewed-by: Rui Zhao <zhaorui126@gmail.com>
Discussion: https://postgr.es/m/CAH2-WzmYqhacBH161peAWb5eF=Ja7CFAQ+0jSEMq=qnfLVTOOg@mail.gmail.com
INSERT ... ON CONFLICT decides what to do based on the conflicting row
found by the arbiter index probe, but SSI never saw that read: the
probe runs with a dirty snapshot, which predicate locking ignores, and
the later fetch of the row uses SnapshotAny. When the statement then
writes nothing, as with DO NOTHING, DO UPDATE with a WHERE clause
rejecting the row, or DO SELECT, nothing records the read at all. A
concurrent writer of that row went unnoticed and write skew could
commit at SERIALIZABLE, even though the same schedule with a plain
SELECT of the row fails with a serialization error.
To fix, read the conflicting tuple again with the query snapshot,
right where the probe finds it. The table AM takes the SIREAD lock
and checks for a concurrent writer of the tuple as part of that read,
both under the buffer lock, so a writer either sees the lock or is
seen. A predicate lock by itself acquired separately after the probe
could not offer that: a writer passing its conflict check in between
would be missed. Doing this in the probe covers every conflict
action, including rows that the WHERE clause of DO UPDATE or DO SELECT
then rejects.
The DO NOTHING and DO UPDATE cases have been broken since ON CONFLICT
was added in 9.5; DO SELECT is new in v19. Backpatch to all supported
branches.
Author: Zsolt Parragi <zsolt.parragi@percona.com>
Author: Andrey Borodin <x4mmm@yandex-team.ru>
Reported-by: Andrey Borodin <x4mmm@yandex-team.ru>
Reported-by: Zsolt Parragi <zsolt.parragi@percona.com>
Discussion: https://postgr.es/m/787936C5-4155-4CF9-939D-39DC0EC1C892@yandex-team.ru
Discussion: https://postgr.es/m/CAN4CZFM1GkHJkpMeo4G5rxtacVsfeKCJYiik9E9AKX1E9VYQ1w@mail.gmail.com
Backpatch-through: 14
List of commits reverted:
8e72d914c5 Add UPDATE/DELETE FOR PORTION OF
b6ccd30d8f Add isolation tests for UPDATE/DELETE FOR PORTION OF
33db6c4baf Fix DELETE/UPDATE FOR PORTION OF with rules
7b22f15a01 Add psql tab completion for FOR PORTION OF clause
7ca8c94296 Fix FOR PORTION OF with non-updatable view columns
993a7aa0e4 Fix cross-leftover pollution in FOR PORTION OF insert triggers
7ac030d5b1 Require UPDATE permission on FOR PORTION OF column
f0aedc7cb0 Small terminology fixes in comments
7d13b03a2e Fix FOR PORTION OF for inheritance children
a272a58b94 Move FOR PORTION OF volatile check into planner
a40fdf6588 Reject child partition FDWs in FOR PORTION OF
bc3ae886a7 Forbid FOR PORTION OF with WHERE CURRENT OF
e994f956e4 Forbid generated columns in FOR PORTION OF
dfce19c230 Forbid FOR PORTION OF on views with INSTEAD OF triggers
20fe90bbb9 Fix RLS checks for FOR PORTION OF leftover rows
9170c8b716 Test what BEFORE UPDATE triggers do to FOR PORTION OF
7090c696cc Deparse FOR PORTION OF using the range column's current name.
62d1a5f8be Avoid RETURNING side effects for FOR PORTION OF leftovers.
9924e2d900 Enforce WITH CHECK OPTION on DELETE FOR PORTION OF leftovers
c16a1b4f2c Fix error code for null FOR PORTION OF target
610976420a Fix assertion failures in DELETE FOR PORTION OF tuple routing
438f027238 Resolve untyped parameters in FOR PORTION OF FROM/TO bounds
140fdfcdf1 Don't evaluate the FOR PORTION OF target under EXPLAIN
Discussion: https://www.postgresql.org/message-id/62bf70cc-dc33-4188-8070-a8c177bb61de%40eisentraut.org
Commit 0ee83dd4a9 added a call of assign_record_type_typmod() in
postgres_fdw.c, but relied on utils/typcache.h being included via
nodes/execnodes.h. That was put there as part of the FOR PORTION OF
feature, and if that were to change, there would be a possible
unrelated compilation failure. To fix, add the required include
directly.
A chunk_id type that does not match with one of the expected values (oid
or oid8) was missing a "return" clause. This caused the code to report
two corruptions instead of one.
Oversight in 6e58d6356f.
Discussion: https://postgr.es/m/aqju1K96ex28yz2X@paquier.xyz
A TOAST table whose chunk_id column is oid8 is now fed a 64-bit object
ID thanks to the introduction of a new vartag_external called
VARTAG_ONDISK_OID8, coupled with a new varatt_external_oid8. The only
difference between the existing varatt_external_oid and the new
varatt_external_oid8 is an extra 4 bytes to store 8 bytes worth an
object ID, spread across two uint32 to force alignement as memcmp may be
used for TOAST pointer comparison (line varatt_external_oid).
There is a one-one mapping between the vartag_external and the type of a
chunk_id:
- An oid8 chunk_id implies the use of VARTAG_ONDISK_OID8.
- An oid chunk_id implies the use of VARTAG_ONDISK_OID.
There are a couple of changes worth noting:
- In toast_tuple_init(), for the comparison of old and new external
values with a memcmp over VARSIZE_EXTERNAL(old_value). Now that
multiple types of external on-disk TOAST pointers are possible, we
need to add a check based on the vartag used by an old or new value.
One of the tests added by c68cba09dd cover this case.
- In toast_tuple_find_biggest_attribute(), the initial "biggest_size"
depends on the type of chunk_id. This needs a change in RelationData to
track the chunk_id type; this cached data is filled on demand.
- In detoast.h, toast_external_info_get() and toast_external_data act as
a translation layer to retrieve all the data associated to an Oid or an
Oid8 TOAST pointer, without the need to worry about the vartag dealt
with. The introduction of this function and structure has reduced a lot
of code churn that existed in the initial versions of this patch, as per
a suggestion from Bharath Rupireddy.
- amcheck is updated to check the vartag <-> chunk_id type mapping
requirements, handling both vartags depending on the underlying type of
chunk_id on the TOAST table.
Bump catalog version, due to at least the change of VARTAG_SIZE().
(This counts for the bump forgotten in a625fc570c22..)
Author: Michael Paquier <michael@paquier.xyz>
Reviewed-by: Greg Burd <greg@burd.me>
Reviewed-by: Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com>
Reviewed-by: Yugo Nagata <nagata@sraoss.co.jp>
Discussion: https://postgr.es/m/af19kUjwjhaoUTLn@paquier.xyz
The tests added by this commit cover a large ground in terms of the
introduction of oid8 in TOAST tables, with the following areas covered,
with all the gaps found during the development of the feature:
- Tables with oid8 TOAST tables in general, under strings.sql.
- ALTER TABLE with attribute manipulations in transactions and different
toast_table_value settings.
- Relation rewrites, not affecting the TOAST relation after initial
creation with VACUUM FULL, CLUSTER, REPACK.
- pg_column_compression().
- test_decoding, for external and extended TOAST data.
- amcheck, external and extended storage with heap checked.
- UPDATE with out-of-line datums that belongs to another TOAST table.
This offers coverage for toast_tuple_init(), where external vartags
could be mixed once multiple on-disk external TOAST pointers can
co-exist. This pattern can be triggered with asan/ubsan, with an
out-of-bound memcpy when unpatched.
These tests are added before the new vartag_external, mostly as a matter
of avoiding dead code in the tree.
Author: Michael Paquier <michael@paquier.xyz>
Reviewed-by: Greg Burd <greg@burd.me>
Reviewed-by: Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com>
Reviewed-by: Yugo Nagata <nagata@sraoss.co.jp>
Discussion: https://postgr.es/m/af19kUjwjhaoUTLn@paquier.xy
This commit adds the possibility to define TOAST tables with oid8 as
their chunk_id, based on the reloption toast_value_type. All the
external TOAST pointers still rely on varatt_external_oid and a single
vartag. The values inserted in the oid8 TOAST tables are fed from the
OID8 value generator, casted to OID for now, as we do not have a
vartag_external able to store Oid8 values yet.
An upcoming commit will add support for a new vartag_external and its
associated structures, with the code being able to use a different
on-disk external TOAST pointer depending on the attribute type of
chunk_id defined in TOAST relations.
All the changes done here are mechanical, mostly around the TOAST code
that needs to be able to do chunk_id lookups based on the two types now
supported.
Bump catalog version.
Author: Michael Paquier <michael@paquier.xyz>
Reviewed-by: Greg Burd <greg@burd.me>
Reviewed-by: Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com>
Reviewed-by: Yugo Nagata <nagata@sraoss.co.jp>
Discussion: https://postgr.es/m/af19kUjwjhaoUTLn@paquier.xy
d96f87332b folded COPY FREEZE visibility map updates into
XLOG_HEAP2_MULTI_INSERT records. Unlike the former XLOG_HEAP2_VISIBLE
redo path, multi-insert redo only updated the FSM when the record did
not contain a heap page FPI and the heap page had less than 20% free
space.
This can leave all-frozen pages with missing or stale FSM entries on a
standby. After promotion, vacuum will skip those pages, preventing
their free space from being discovered.
When a multi-insert record sets the page all-frozen, record the heap
page's free space unconditionally.
Reported-by: Melanie Plageman <melanieplageman@gmail.com>
Discussion: https://postgr.es/m/CAAKRu_bS3nfMJCF2fjE%2Bceb5-t%3D2p_VPMk2PWuVomrOFxg5DGQ%40mail.gmail.com
Backpatch-through: 19
add323da40 consolidated visibility map updates into XLOG_HEAP2_PRUNE
records. It assumed that a record setting the visibility map would
always modify the VM page during replay. That is not true when the
requested bits are already set on the standby.
Primary and standby visibility maps can diverge for several reasons.
This issue was exposed by a CREATE DATABASE ... STRATEGY WAL_LOG bug.
After a standby with an out-of-date VM is promoted, VACUUM may set bits
that were already present on the former primary. When the former primary
later replays that record as a standby, visibilitymap_set() is a no-op.
Restore visibilitymap_set()'s former API, which returns the state of the
VM bits before setting the requested flags. Use that result to set the
VM page LSN only when the visibilitymap_set() updates the page.
Reported-by: Rogers Wang <rogers.ww@qq.com>
Discussion: https://postgr.es/m/tencent_2E870046716FD94285045E96505A2D4E2908%40qq.com
Backpatch-through: 19
During a parallel GIN build, each key is serialized into a GinTuple, a
transient representation used only while sorting. _gin_build_tuple()
lays out the whole tuple (the palloc size, the key memcpy, and the
offset of the posting list) from a local variable holding the real key
length, but then stored that length in GinTuple.keylen, which was
uint16. For a key longer than 65535 bytes, the stored length was thus
silently truncated.
On read-back, GinTupleGetFirst() and _gin_parse_tuple_items()
recompute the posting-list offset from the truncated keylen and land
inside the key data, so ginPostingListDecodeAllSegments() decodes
garbage: an assertion failure with assertions enabled, and a read past
the end of the allocation without, which could in turn lead to a crash
or garbage being written into the index. Only parallel builds are
affected, because only they materialize a GinTuple; a serial build of
the same data succeeds, as index_form_tuple() compresses large keys
before the GinMaxItemSize check.
To fix, widen GinTuple.keylen to Size, which is what VARSIZE_ANY()
returns and what GinBuffer.keylen already uses, and use Size for the
local in _gin_build_tuple() too, which was an int that truncated
VARSIZE_ANY() the same way.
Widening keylen moves GinTuple.data. The key value is accessed in
place in data, so data must be MAXALIGN'ed; before, that was only true
by accident of the field layout. Make that explicit with
alignas(MAXIMUM_ALIGNOF) on data, so that the layout stays correct on
32-bit platforms, where Size is 4 bytes and would otherwise leave data
under-aligned when MAXIMUM_ALIGNOF is 8.
Bug: #19545
Author: Ewan Young <kdbase.hack@gmail.com>
Reported-by: Yuelin Wang <1217816127@qq.com>
Reviewed-by: Peter Eisentraut <peter@eisentraut.org>
Discussion: https://www.postgresql.org/message-id/flat/19545-0f25b7e47351e8fc%40postgresql.org
378a216187 set pd_prune_xid on inserts so that on-access pruning could
later set the pages all-visible. heap_multi_insert() skipped this only
when setting the page all-frozen, but it can insert frozen tuples even
when not setting the page all-frozen. Inserting frozen tuples should not
set pd_prune_xid as they introduce no work for on-access pruning. Fix by
only setting pd_prune_xid when inserting non-frozen tuples.
Reported-by: Melanie Plageman <melanieplageman@gmail.com>
Reviewed-by: Chao Li <li.evan.chao@gmail.com>
Discussion: https://postgr.es/m/CAAKRu_a02pu7U2S0qPZ5L%2BoWb2m2Zp965sOKCCLnKUAKNo-hfg%40mail.gmail.com
Backpatch-through: 19
Commit 977d865c36 established some rules for mentioning special
values (e.g., -1, 0, an empty string) in GUC descriptions. A few
parameters either neglect to mention their special values or
describe them in a way that doesn't follow those rules. Most of
these parameters were added after the aforementioned commit. This
commit fixes them.
Reviewed-by: Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com>
Reviewed-by: Fujii Masao <masao.fujii@gmail.com>
Reviewed-by: Chao Li <li.evan.chao@gmail.com>
Reviewed-by: Álvaro Herrera <alvherre@kurilemu.de>
Discussion: https://postgr.es/m/aqMkIpRr62cVqtCZ%40nathan
Backpatch-through: 19
The launcher_running flag was reset to false during shutdown, which
left a small window where a new launcher could be started when the
exit handler was still running. Fix by only updating the running
flag in shared memory in the exit handler if our local copy is true.
Also reset the aborted state if the launch_operation changes during
shutdown to allow scenarios like "start -> abort -> start" to work
even if they reach the second start before the abort is fully done.
Issues found by Noah and Robert using AI assisted review with Claude.
Backpatch to v19 where the online checksums feature was introduced.
Reported-by: Noah Misch <noah@leadboat.com>
Reported-by: Robert Haas <robertmhaas@gmail.com>
Discussion: https://postgr.es/m/E15AC050-C4B5-488D-BB2D-3C7AC9F89EA8@yesql.se
Backpatch-through: 19
When attempting to upgrade a cluster which is an inprogress state, the
same error message was used regardless of which state it was. Fix by
using different error messages for the different inprogress states.
Found by Noah using AI assisted review with Claude. Backpatch to v19
where the online checksums feature was introduced.
Reported-by: Noah Misch <noah@leadboat.com>
Discussion: https://postgr.es/m/E15AC050-C4B5-488D-BB2D-3C7AC9F89EA8@yesql.se
Backpatch-through: 19
When the data_checksums GUC was changed into an enum the enum values
were all incorrectly marked as hidden, which made pg_settings report
an empty array. Fix by setting all as visible as they should be.
Found by Noah using AI assisted review with GPT. Backpatch to v19
where the GUC was changed in the online checksums feature.
Reported-by: Noah Misch <noah@leadboat.com>
Discussion: https://postgr.es/m/E15AC050-C4B5-488D-BB2D-3C7AC9F89EA8@yesql.se
Backpatch-through: 19
A set of documentation updates for the online checksums work, found via
manual as well as LLM-guided review.
* The state diagram was re-done to be improve readability and be
more in line with the look and feel of other diagrams.
* Paragraph about mismatched states moved to its own sect2.
* The documentation for the functions to enable/ disable checksums
failed to mention that they are superuser only.
* The documentation for the checksums progress reporting had
accidentally omitted the work "fork" and for the blocks_* columns.
The values are per fork but the documentation made it seem they
were per relation.
* Add the state image to the meson buildfile
Reported-by: Heikki Linnakangas <hlinnaka@iki.fi>
Reported-by: Noah Misch <noah@leadboat.com>
Discussion: https://postgr.es/m/5b08b4d1-0982-4b77-bac5-3bdffc6583f5@iki.fi
Backpatch-through: 19
check_control_files() detected a chain whose backups were taken under
different data checksum states, warned, and proceeded. The output
directory keeps the last backup's control file, so a full backup taken
with checksums off combined with an incremental taken after an offline
enable produces a cluster whose control file says "on" while most of
its blocks carry no checksums; it starts, and then every connection
dies on the first unchecksummed catalog page. An offline enable
between two backups of a chain is all it takes, since it rewrites
every page without logging anything, so the incremental backup does
not re-ship the pages.
Turn the warning into an error, matching what pg_rewind does for the
equivalent combinations. The check stays asymmetric on purpose: when
the last backup was taken with checksums off, stale checksums from an
earlier backup are never verified, and that chain remains usable.
Author: Zsolt Parragi <zsolt.parragi@percona.com>
Reviewed-by: Bertrand Drouvot <bertranddrouvot.pg@gmail.com>
Reviewed-by: Daniel Gustafsson <daniel@yesql.se>
Discussion: https://postgr.es/m/anwm6UPxoVS41QA2@bdtpg
Backpatch-through: 19
pg_rewind installs the source's control file on the target, but every
block it does not copy keeps the target's content. With checksums
enabled on the target and disabled on the source, the rewound server
claims enabled checksums while the blocks copied from the source have
none, and fails checksum verification as soon as it reads them; in the
worst case every connection attempt dies on an unverifiable catalog
page.
Comparing the control files is not enough. Replay on the rewound
server resumes from the last common checkpoint and adopts the data
checksum state recorded there, so an offline disable on the source
after the divergence leaves both control files saying "off" while the
rewound server still resumes with verification enabled. Check the
state carried by the divergence checkpoint as well, which pg_rewind
already reads.
Refuse both cases, and an interrupted online transition on either
side, same as pg_checksums does. The opposite mismatch, checksums on
the source only, is allowed with a warning: recovery under the backup
label adopts the divergence state, so the rewound server keeps
checksums disabled (or converges through the replayed WAL if the
source enabled them online), and no page can fail verification.
Author: Zsolt Parragi <zsolt.parragi@percona.com>
Reviewed-by: Bertrand Drouvot <bertranddrouvot.pg@gmail.com>
Reviewed-by: Daniel Gustafsson <daniel@yesql.se>
Discussion: https://postgr.es/m/anwm6UPxoVS41QA2@bdtpg
Backpatch-through: 19
An inprogress-on or inprogress-off control file means an online
transition was cut short, running the offline tool on top of it mixes
two procedures. Refuse it in every mode, with a hint pointing at the
start-stop cycle (or, on a standby, at letting replication finish)
that resets the state. Also print that the change applies to one
data directory only, with standby-aware wording, since in a
replication setup the same change must be made on every node.
On a primary, inprogress-on cannot survive a graceful stop: the
checksums launcher resolves it from its exit cleanup, and a crashed
primary is already rejected by the existing "cluster must be shut
down" check. inprogress-off can, however, it is set by the backend
running pg_disable_data_checksums(), and a fast shutdown arriving
between its two barriers leaves it behind in a cleanly shut down
control file, where the next start-stop cycle resolves it at end of
recovery. A standby can be stopped with either state, since it has
no launcher and only carries forward whatever state the last replayed
WAL record left it in, with a restartpoint persisting that as-is.
The standby is also the deterministic way to reach the new guard,
which is why the test coverage uses one; the primary window would
need an injection point between the two barriers.
The pg_checksums page said the tool still processes all relation files
regardless of an interrupted online transition; describe the refusal
instead.
Author: Zsolt Parragi <zsolt.parragi@percona.com>
Reviewed-by: Bertrand Drouvot <bertranddrouvot.pg@gmail.com>
Reviewed-by: Daniel Gustafsson <daniel@yesql.se>
Discussion: https://postgr.es/m/anwm6UPxoVS41QA2@bdtpg
Backpatch-through: 19
Offline data checksum changes with pg_checksums are local to one node,
but replay adopted the data checksum state carried by checkpoint record
unconditionally. After enabling checksums offline on only the primary,
a standby thus started verifying checksums it does not have. After an
offline change on a standby, the next replayed checkpoint silently
reverted the state.
To fix, make the control file track this node's state alone, and have
replay cross-check the replayed state against it instead of adopting
it, warning once per divergent value and reporting when the states
agree again. pg_control gains a watermark, normally the end LSN of
the newest XLOG2_CHECKSUMS record the node has written or applied,
and a flag marking that the state was last written by pg_checksums.
Recovery must never overwrite this state with a replayed one. The
control file is updated when all pages are flushed. This requires
a PG_CONTROL_VERSION bump.
Recovery from a base backup may be an exception to not adopting: its
control file was copied at an arbitrary moment, so the state carried
by the starting checkpoint is the one the WAL from there on was
Also document the offline procedure for replication setups.
Author: Zsolt Parragi <zsolt.parragi@percona.com>
Author: Bertrand Drouvot <bertranddrouvot.pg@gmail.com>
Reported-by: Bertrand Drouvot <bertranddrouvot.pg@gmail.com>
Reviewed-by: Bertrand Drouvot <bertranddrouvot.pg@gmail.com>
Reviewed-by: Daniel Gustafsson <daniel@yesql.se>
Discussion: https://postgr.es/m/anwm6UPxoVS41QA2@bdtpg
Backpatch-through: 19
The command tag is WAIT, but the reference page called the command
"WAIT FOR" in its title, index entry, psql help name and throughout its
prose, and so did the error messages. FOR is a noise word that makes the
grammar read like English, exactly as INTO does for INSERT and FROM does
for DELETE; neither of those is part of the command's name. Settle on
WAIT everywhere the command is named, and keep WAIT FOR LSN wherever the
syntax itself is being shown.
Rename the reference page's id and file name to match, following
cb2c696b1d: v19 is not released, so the URL is not yet baked into a
supported version, and every other page's id matches its title.
Reported-by: Masahiko Sawada sawada.mshk@gmail.com
Author: Xuneng Zhou xunengzhou@gmail.com
Reviewed-by: Alexander Korotkov aekorotkov@gmail.com
Discussion: https://postgr.es/m/CAD21AoBdtiPTbm7T_aNeDON9JpFG9g%3DJDUP4WU-Y8t8_xqvR5Q%40mail.gmail.com
Backpatch-through: 19
Commit 447aae13b0 accepted TIMEOUT values up to INT64_MAX
milliseconds, but computing the deadline multiplies by 1000, which
caused an int64 overflow. And the command reported a timeout at once
instead of waiting.
Parse the value as an int instead, which simplifies the code and is
consistent with other timeout values such as the statement_timeout GUC.
The specified values are now rounded to the nearest millisecond, so a
positive timeout below half a millisecond becomes zero and waits
indefinitely.
Backpatch to v19, where the TIMEOUT option was introduced.
Reported-by: ChangAo Chen <cca5507@qq.com>
Author: ChangAo Chen <cca5507@qq.com>
Reviewed-by: Xuneng Zhou <xunengzhou@gmail.com>
Reviewed-by: Masahiko Sawada <sawada.mshk@gmail.com>
Discussion: https://postgr.es/m/tencent_86B83240785807077600A1778566B5C12908@qq.com
Backpatch-through: 19
To import attribute statistics, postgres_fdw sent a query wity COLLATE
"C" to remote servers without checking their version. Since COLLATE is
only supported since v9.1, this could break the import against older
servers; fix by modifying postgres_fdw to check their version and if
older, fall back to sampling. (This commit still allows importing only
relation statistics from such an older server.)
Also, expand the documentation to mention that attribute statistics are
stored into the local server without applying the foreign table columns'
n_distinct option.
Oversight in commit 28972b6fc.
Reported-by: Fujii Masao <masao.fujii@gmail.com>
Reported-by: Osama Abdul Qader <osamaabdulqader.cs@gmail.com>
Author: Etsuro Fujita <etsuro.fujita@gmail.com>
Reviewed-by: Corey Huinker <corey.huinker@gmail.com>
Discussion: https://postgr.es/m/CAHGQGwH8%2BuDTwfohpck9v_9L3nwSWACbmC36JYRyqHd5xQN_OA%40mail.gmail.com
Backpatch-through: 19
contain_context_dependent_node_walker() reports a CaseTestExpr as
context-dependent unless it sits under a simple CaseExpr or the
elemexpr of an ArrayCoerceExpr. A JsonConstructorExpr whose RETURNING
type requires a coercion also carries a CaseTestExpr placeholder in
that coercion, so any SQL function called with such a constructor as
an argument was refused inlining.
Teach the walker that a CaseTestExpr is expected within the coercion
of a JsonConstructorExpr, the same way it already handles the elemexpr
of an ArrayCoerceExpr. This is safe now that eval_const_expressions
no longer lets an enclosing simple CASE clobber that placeholder.
Author: Richard Guo <guofenglinux@gmail.com>
Discussion: https://postgr.es/m/CAMbWs48A=VCFbteTkuCoknO1_0-Cu0aMBT0M07dm7vj1QyixDg@mail.gmail.com
Backpatch-through: 19
A JSON constructor with a RETURNING clause uses a CaseTestExpr as the
placeholder for its result in the coercion expression. When such a
constructor appears in a WHEN clause of a simple CASE whose test
expression is a constant, eval_const_expressions substituted that
constant for the placeholder, so the coercion produced the CASE's test
value instead of the constructor's result. For instance,
CASE 'x' WHEN JSON_OBJECT('a': 'b' RETURNING text) THEN 1 ELSE 0 END
evaluated to 1.
To fix, keep case_val out of scope while simplifying the coercion, as
is already done for the elemexpr of an ArrayCoerceExpr.
Back-patch to v16, where the SQL/JSON constructor functions were
introduced.
Author: Richard Guo <guofenglinux@gmail.com>
Discussion: https://postgr.es/m/CAMbWs48A=VCFbteTkuCoknO1_0-Cu0aMBT0M07dm7vj1QyixDg@mail.gmail.com
Backpatch-through: 16
When a subquery references an output of another subquery that gets
pulled up, and that output must be wrapped in a PlaceHolderVar because
of an intermediate outer join, the PHV expression is pushed down into
the subquery. That copy is not preprocessed along with the outer
query's expressions, so the two copies can diverge. This used to be
harmless, but since commit 2ebf25e7d join removal edits the whole
query tree, walking into subqueries, and can trip an assert in
ChangeVarNodes if it removes a rel whose Var survives only in such a
copy.
To fix, preprocess these copies at their owning query level, early in
subquery_planner, before anything can consume them (in particular
before SubLinks are turned into SubPlans). This covers copies pushed
into both LATERAL subquery RTEs and SubLink subselects, and handles
nested copies innermost-first. extract_lateral_references no longer
preprocesses the copies it pulls out.
Correspondingly, the subquery's own processing must leave the contents
of an upper-level PHV alone, since the owning level has already
preprocessed them: eval_const_expressions returns such a PHV
unchanged, and flatten_join_alias_vars no longer recurses into 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/CAHewXN=kWGAXV537mKtSyBYobGdHhYJVDJJMXXZEmmPWE_zaPw@mail.gmail.com
Backpatch-through: 16
The counter reaches a standby through XLOG_NEXTOID and checkpoint
records. This commit adds a test to move the next OID past the 2^32
limit on a primary and read it back from the promoted standby. This
reuses the same trick as some of the previous tests, with a pg_resetwal
command setting the next OID to be larger than 2^32, combined with a
check based on pg_control_checkpoint().
This closes a testing gap of cb29861646 regarding the replay of
XLOG_NEXTOID, where this WAL record has been updated to pass down an
Oid8 value. By default, regression tests would use OIDs way below the
UINT32_MAX limit.
Author: Rui Zhao <zhaorui126@gmail.com>
Discussion: https://postgr.es/m/CAHWVJhGkb1n_QSS4M=PKJD5HHcwK3KXpFwVxoRVLzAG3j+2fhQ@mail.gmail.com
This is required for a follow-up commit that will add support for 8-byte
TOAST values, with this function being changed so as it is able to
support the largest TOAST value type available.
Most of the changes in this commit are in the regression tests that use
pg_column_toast_chunk_id(), that need to use some oid8-ish changes. The
change is surprisingly not invasive as casts from oid to oid8 are
possible (the opposite cast oid8->oid is not supported).
Bump catalog version.
Author: Michael Paquier <michael@paquier.xyz>
Reviewed-by: Greg Burd <greg@burd.me>
Reviewed-by: Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com>
Reviewed-by: Yugo Nagata <nagata@sraoss.co.jp>
Discussion: https://postgr.es/m/af19kUjwjhaoUTLn@paquier.xy
This is a mechanical update to change most of the TOAST code paths that
assume TOAST value IDs to be Oid variables to become Oid8 variables,
easing an the introduction of an upcoming patch to allow larger TOAST
values, at 8 bytes.
The areas touched by the change are related to table AM, amcheck and
logical decoding's reorder buffer. A good chunk of the changes involve
switching printf() markers from %u to OID8_FORMAT.
For the sake of the release notes in terms of compatibility (perhaps
worth mentioning, or not), note that this changes some callback
definitions in the table AM section, upgrading some TOAST callbacks to
use Oid8 instead of Oid when referring to a TOAST value.
Author: Michael Paquier <michael@paquier.xyz>
Reviewed-by: Greg Burd <greg@burd.me>
Reviewed-by: Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com>
Reviewed-by: Yugo Nagata <nagata@sraoss.co.jp>
Discussion: https://postgr.es/m/af19kUjwjhaoUTLn@paquier.xy
WaitForLSN() samples the current position before checking whether
recovery is still in progress. If recovery reaches the target and ends
between those operations, the promotion path can compare against the
stale sample and incorrectly return NOT_IN_RECOVERY.
Read the position again after observing that recovery has ended, before
deciding whether promotion reached the target.
Author: Xuneng Zhou <xunengzhou@gmail.com>
Discussion: https://postgr.es/m/CABPTF7U0gW5%2B-4oL7-qdML-yerZxUb7ku4QXp7JxCYo0qyJ_Tw%40mail.gmail.com
Reviewed-by: Alexander Korotkov <aekorotkov@gmail.com>
Backpatch-through: 19
XLogWrite() can advance the flush position when it finishes a WAL
segment, even if its caller requested only a write. When this happens
while AdvanceXLInsertBuffer() recycles a buffer, primary_flush waiters
are not notified and can sleep until an unrelated flush or checkpoint.
Track such progress with a process-local pending flag and perform the
wakeup after releasing WAL write and insertion locks, keeping
WaitLSNLock acquisition outside the contended WAL path. Add a
deterministic injection-point test that forces this path and inspects
the registration directly in shared memory.
Author: Xuneng Zhou <xunengzhou@gmail.com>
Discussion: https://postgr.es/m/CABPTF7U0gW5%2B-4oL7-qdML-yerZxUb7ku4QXp7JxCYo0qyJ_Tw%40mail.gmail.com
Reviewed-by: Alexander Korotkov <aekorotkov@gmail.com>
Backpatch-through: 19
A backend waiting for WAL replay can retain locks acquired by earlier
statements. If the startup process needs one of those locks, directly or
through another backend, before reaching the target LSN, a deadlock can
arise: startup waits for the backend to release the lock, while the backend
waits for startup to advance replay. The lock manager records the
backend's held locks, but WAIT FOR LSN does not register its dependency on
replay as a lock wait. The deadlock detector therefore cannot see the
complete cycle. With unlimited standby conflict delays and no other
timeout or cancellation, this deadlock can persist indefinitely.
Write and flush waits are restricted as well. Their positions are floored
by the replay position, so without an active walreceiver the startup
process can be their only source of progress. If a held lock blocks
replay, these waits can form the same cycle: the backend waits for replay
to advance, while replay waits for the backend to release the lock.
Streaming does advance them independently, but only while WAL keeps
arriving. If reception stops before the target is reached, a blocked
startup process cannot restart the walreceiver. It also cannot replay
newer checkpoint records needed to advance restartpoints and recycle WAL,
so continued reception can exhaust available space in pg_wal before the
target is reached. An active receiver at the start of the wait therefore
does not guarantee that the wait can finish while replay remains blocked.
Reject an unsatisfied standby_replay, standby_write, or standby_flush wait
while recovery is active when the backend already holds a granted
heavyweight lock. This conservative restriction covers direct
relation-lock cycles and indirect cycles involving advisory locks. It
also rejects some write and flush waits that an active receiver could
satisfy. Requests whose target is observed as already reached are exempt
from this check, as are primary_flush requests and requests issued after
recovery has ended. Existing snapshot and recovery-state checks still
apply.
Report one of the held locks so the user can find it, using the same
description the deadlock report uses.
Add replay-mode tests for relation and advisory locks and for the
already-reached case, and document the restriction along with the
recommended usage pattern.
Author: Xuneng Zhou <xunengzhou@gmail.com>
Discussion: https://postgr.es/m/CABPTF7U0gW5%2B-4oL7-qdML-yerZxUb7ku4QXp7JxCYo0qyJ_Tw%40mail.gmail.com
Reviewed-by: Alexander Korotkov <aekorotkov@gmail.com>
Backpatch-through: 19
This reverts commit 704ef84ef2.
The configure solution in that commit doesn't work if llvm-config
returns a system directory such as /usr/include. The resulting
-isystem /usr/include puts the C headers before the C++ headers in the
include file order, which is not supposed to be done and causes
failures. (Using -I /usr/include doesn't have that problem because it
is handled specially by the compiler.) Per buildfarm members
hippopotamus and jay.
The meson solution does work because it specifically filters out
system includes. This logic could theoretically be ported to
configure, but that would be a separate undertaking.
LLVM's headers produce warnings under some of the warning options we
might like to use. For example, -Wshadow=local reports several
warnings from LLVM's headers when compiling llvmjit_inline.cpp. To
work around that, add the include directories reported by llvm-config
with -isystem rather than -I, which makes the compiler treat them as
system headers and not report warnings from them. In meson, this is a
built-in facility of the dependency() function, in configure we
implement it ourselves.
An alternative solution would have been to use "#pragma GCC
system_header", as is already done elsewhere in the tree. But that
seems less elegant here. Either, we would have to potentially create
a separate wrapper for each LLVM header, or we would have to route all
LLVM includes through a common header, which would break modularity.
Reviewed-by: Peter Geoghegan <pg@bowt.ie>
Reviewed-by: Chao Li <li.evan.chao@gmail.com>
Discussion: https://www.postgresql.org/message-id/flat/84b6f128-91f5-480e-8a9e-7d0e8f538cea%40eisentraut.org
The in-tree getopt_long() moves each non-option to the end of argv,
which might put it right where an option's argument lookup expects
to find the argument. For example, "vacuumdb postgres --jobs"
takes "postgres" as the number of jobs instead of complaining that
--jobs is missing its argument. To fix, stop the argument lookups
at the start of the moved non-options, which we already track to
know when to stop scanning.
Oversight in commit 411b720343.
Author: Sehrope Sarkuni <sehrope@jackdb.com>
Reviewed-by: solai v <solai.cdac@gmail.com>
Discussion: https://postgr.es/m/CAH7T-arxDuVCSkorO%3Dk7%2BM-_JV0JFzMpN_EtKMyD2K0RDqZ2OA%40mail.gmail.com
Backpatch-through: 17
