infer_arbiter_indexes() classifies each inference element by its
const-simplified form: a plain Var is matched against the index's
plain attributes, and anything else against its expressions. Index
columns, however, were classified by indkey alone, so an expression
column stayed an expression even when const-simplification reduced it
to a plain Var. Since commit 10c4fe074, COALESCE on a NOT NULL column
is reduced to the column itself, so an inference element such as
COALESCE(b, '') became a plain Var that no longer matched the unique
index on that same expression, and the statement failed with "there is
no unique or exclusion constraint matching the ON CONFLICT
specification".
To fix, classify index columns the same way as inference elements: an
index expression that is simplified to a plain Var is treated as a
plain attribute. Also, teach infer_collation_opclass_match() to
compare such a column as an expression, so that an inference element
that specifies a collation or operator class can still match it.
As a consequence, an inference element naming the column directly now
matches such an index too, which is consistent with how an element
written as the expression already matches a plain index on the column.
Author: Shihao Zhong <zhong950419@gmail.com>
Reviewed-by: jian he <jian.universality@gmail.com>
Reviewed-by: Richard Guo <guofenglinux@gmail.com>
Discussion: https://postgr.es/m/CAGRkXqR7VXacmRP41UBaM5u4aseeCH5ECVmQgn_YS=L7=xdhTw@mail.gmail.com
Backpatch-through: 19
BackendWritebackContext is a purely per-process thing, so it feels
wrong to initialize it in the shmem init/attach functions. Move the
initialization to InitBufferManagerAccess() instead.
Backpatch to version 19, where the new shared memory callbacks system
was introduced. While this isn't new with that, it made the ShmemInit
functions more closely related to shared memory initialization than
before.
Reviewed-by: Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com>
Reviewed-by: Ashutosh Bapat <ashutosh.bapat.oss@gmail.com>
Discussion: https://www.postgresql.org/message-id/fb1bd612-4a58-4235-976f-2b2ef036bfff@iki.fi
Backpatch-through: 19
Converting xlog.c to the new shared memory allocation API left the
creation of the memory context used by wal_debug in the
XLOGShmemInit() callback, which now only runs in the postmaster or in
a standalone backend. Previously, XLOGShmemInit() ran in every child.
As a result, walDebugCtx was left uninitialized in child processes in
EXEC_BACKEND mode, and with wal_debug turned on, the backend crashed
on first WAL insertion.
Fix by moving the walDebugCtx initialization to a separate function
that is called in each backend that wishes to create WAL records. (It
cannot be created lazily on first WAL insertion, because it's done in
a critical section, and you cannot create a new memory context in a
critical section.)
Oversight in 9b5acad3f4.
Co-authored-by:: Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com>
Reviewed-by: Ashutosh Bapat <ashutosh.bapat.oss@gmail.com>
Discussion: https://www.postgresql.org/message-id/CALj2ACUNL4Xn3aBzvm4DTpdr_L0-AfL2agsghMRRy_ZTiKhxSA@mail.gmail.com
Backpatch-through: 19
When a table is partitioned, the previous code did not emit NO_GATHER
advice for the parent table, on the theory that doing this for the
child tables was good enough. However, it isn't. I believe what
happened here is that, during development, I was planning to enforce
NO_GATHER by clearing the consider_parallel flag for the children,
which would have been good enough to ensure that parallelism could
not be used at any higher level of the plan tree. Eventually, I
switched course, but failed to realize that this code needed to be
updated as a result. Do that, and add a regression test.
Backpatch-through: 19
Discussion: http://postgr.es/m/CA+TgmoYYHC4i-Gpzc9-JYHqih6pOK+6LTYFiNR-PioXG1nKkOQ@mail.gmail.com
pg_restore did not verify that zstd and LZ4 decompression completed
the final frame when reading custom and directory-format dumps. As a
result, it could accept a dump whose compressed data was truncated.
Track final-frame completion and reject incomplete compressed input.
For zstd, flush any internally buffered output before performing the
final check.
Add regression tests for truncated zstd and LZ4 data in custom and
directory-format dumps and also address a potentially empty error
message while in there.
Author: Chao Li <lic@highgo.com>
Co-authored-by: Daniel Gustafsson <daniel@yesql.se>
Reviewed-by: Daniel Gustafsson <daniel@yesql.se>
Discussion: https://postgr.es/m/5962B878-C43D-4EBC-9E95-1F945CE5E586@gmail.com
Backpatch-through: 19
pg_verifybackup failed to detect truncated compressed tar backups
for zstd, lz4 and gzip compression.
astreamer_zstd_decompressor checked whether ZSTD_decompressStream()
returned an error. However, a positive return value at end-of-input
means that the final zstd frame has not completed. Record the state
and verify at finalization that zstd completed the frame. Also flush
any pending internally buffered output before performing that check.
astreamer_gzip_decompressor did not check whether inflate() had
reached Z_STREAM_END before finalizing. Record whether inflate()
completed the gzip stream, and reject finalization if it did not.
astreamer_lz4_decompressor did not check whether LZ4F_decompress()
completed the final frame before finalizing. Record the status of
decompression and reject finalization unless the frame completed.
This prevents pg_verifybackup from accepting a truncated compressed
tar backup.
Author: Chao Li <lic@highgo.com>
Co-authored-by: Daniel Gustafsson <daniel@yesql.se>
Reviewed-by: Osama Abdul Qader <osamaabdulqader.cs@gmail.com>
Reviewed-by: Zsolt Parragi <zsolt.parragi@percona.com>
Reviewed-by: Japin Li <japinli@hotmail.com>
Discussion: https://postgr.es/m/5962B878-C43D-4EBC-9E95-1F945CE5E586@gmail.com
Backpatch-through: 19
The segment_close() callback is required to set ws_file to a negative
value after closing a WAL segment, as documented in xlogreader.h.
XLogReaderFree() only treated -1 as closed, so it could call
segment_close() again if another negative value was used.
This should not be an issue in practice, but all the other code paths in
the tree use 0 as comparison base when dealing with ws_file, except for
the line updated here. Let's be consistent, at least.
Author: Bertrand Drouvot <bertranddrouvot.pg@gmail.com>
Discussion: https://postgr.es/m/asNr203eue0R4zpZ@bdtpg
For run-time partition pruning running during execution (rather than at
executor startup), EvalPlanQualStart() reuses the es_part_prune_states
from the parent EState. This could cause an issue when the EPQ recheck is
finished, its EState is cleaned up and the es_query_cxt is deleted by
FreeExecutorState(). That cleanup could free memory that was allocated
again in InitExecPartitionPruneContexts() when it was called for the EPQ's
EState, and naturally, those fields got allocated in the EPQ's
es_query_cxt MemoryContext, which overwrote the ones for the parent
EState. If pruning occurs again after the EPQ's EState was cleaned up,
then we could have accessed free'd memory. This certainly caused issues in
debug builds because we clobber freed memory, but may not have in
production, depending on if the MemoryContext being deleted results in a
free(). It may not if the allocation was on the context's init block and
the context was pushed onto context_freelists[] for reuse.
Here we fix this by adding an "initialized" field to PartitionPruneState
and exit early in InitExecPartitionPruneContexts() when the state is
already initialized.
This was broken by bb3ec16e1, but the breakage there is slightly
different as that commit neglected to do what was fixed in 8741e48e5
(which did get backpatched to v18 in 9a82a64ed).
Reported-by: Vladimir Savin <vladimir@encord.com>
Author: David Rowley <dgrowleyml@gmail.com>
Reviewed-by: Andrey Rachitskiy <pl0h0yp1@gmail.com>
Discussion: https://postgr.es/m/CAP2G_gGpV=rKMLuET4RW-qO41GvBvi6Czsjsj16eT9hAwHxG6w@mail.gmail.com
Backpatch-through: 18
With WAL streaming, pg_basebackup -Ft -z writes pg_wal.tar.gz as a
series of gzip members. The decompressor did not reset the inflate
state when one ended, so inflate consumed nothing more and the loop
never finished. This caused pg_waldump to hang on such backups and
as a consequence pg_verifybackup (which use pg_waldump) would hang.
A test is added to pg_verifybackup for checking a gzip-compressed
tar backup, which in turn will verify that pg_waldump works.
Author: Shihao Zhong <zhong950419@gmail.com>
Reviewed-by: Daniel Gustafsson <daniel@yesql.se>
Reviewed-by: Chao Li <li.evan.chao@gmail.com>
Reviewed-by: Andrew Dunstan <andrew@dunslane.net>
Discussion: https://postgr.es/m/CAGRkXqTcyuog_MzKqQm+am2xMefLFJTSKaRevnfypDWirCFvdA@mail.gmail.com
Backpatch-through: 19
Commit 5cd72cc0c5 made the VACUUM VERBOSE and autovacuum logs report
buffer usage from the pgBufferUsage delta, which includes the parallel
workers' usage, but left the I/O timings on the leader's
pgStatBlockReadTime and pgStatBlockWriteTime. So the log counted the
blocks the workers read and dirtied but not the time they spent on
them.
Fix this by taking the I/O timings from the same BufferUsage delta.
This also removes a small error in the leader's own timings, since
pgStatBlock{Read,Write}Time accumulate whole microseconds per I/O.
Although 5cd72cc0c5 was backpatched to v15, BufferUsage on v16 doesn't
track the I/O time of temporary relations, so the same fix there would
drop that time from the log. We don't think a reporting issue is worth
v16-specific code, nor worth leaving v16 unfixed between v15 and v17,
so backpatch to v17 only.
Found by Bharath using AI assisted review with Claude.
Reported-by: Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com>
Author: Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com>
Reviewed-by: Sami Imseih <samimseih.pg@gmail.com>
Reviewed-by: Chao Li <li.evan.chao@gmail.com>
Reviewed-by: Masahiko Sawada <sawada.mshk@gmail.com>
Reviewed-by: Shihao Zhong <zhong950419@gmail.com>
Discussion: https://postgr.es/m/CALj2ACXrqaHrYmGdpAoGKXtiY9Xw4o1dxSC+EELxn47GDO6HAQ@mail.gmail.com
Backpatch-through: 17
_bt_pagedel only builds a search stack when it needs one to locate the
downlink of the page that it's about to mark half-dead (and when it
doesn't already have one from an earlier sibling deletion that took
place during the same _bt_pagedel call). _bt_pagedel won't build a
search stack for the page its caller passes whenever it happens to be
one that an earlier interrupted VACUUM left half-dead (since there is no
downlink to remove). We nevertheless asserted that page deletion could
only build a search stack for that initial caller-passed page.
To fix, remove the assertion, which spuriously failed when a half-dead
leaf page was passed by the _bt_pagedel caller and its right sibling
page was empty/had to be considered for deletion during the same call
(necessitating building a search stack to locate the right sibling's
downlink).
Oversight in commit cd8c73a38, which refactored nbtree page deletion's
INCOMPLETE_SPLIT check.
Author: Peter Geoghegan <pg@bowt.ie>
Reported-by: Iliia Khaprov <i.khaprov@gmail.com>
Discussion: https://postgr.es/m/CAFO+O_p5So4MXwQ1VEcL9+9vGgODu11TTWcYq9RDbxnira8o2w@mail.gmail.com
Backpatch-through: 14
Commit 0d4391b265 taught the planner
to store information about the RTIs of SubqueryScan, Append, and
MergeAppend node elided from the plan using a new ElidedNode data
structure; an hour later, 7358abcc60
taught to save the RTIs of an Append node into another Append
node using a new child_append_relid_sets field. Regrettably,
despite the fact that both commits were written by me and the
fact that both were part of the same patch set, the latter commit
failed to deal with the case where an Append node first acquires
child_append_relid_sets != NIL and then gets elided. I believe that
I thought that was impossible, but the included test case shows that
it isn't.
The proper fix involves updating the definition of ElidedNode to
carry child_append_relid_sets, so this commit does that, breaking
ABI compatibility. That's fine for the master branch, but it's a
bit unfortunate to be doing do that so close to the planned ship
date for v19. However, shipping v19 without fixing this appears
(at least to me) to be even more unfortunate.
Backpatch-through: 19
Discussion: http://postgr.es/m/CA+TgmoYYHC4i-Gpzc9-JYHqih6pOK+6LTYFiNR-PioXG1nKkOQ@mail.gmail.com
Follow-up to bca67e5a3. The test added there changed the value of
oauth_validator_libraries, waited for the postmaster to apply it, and
verified the change on an existing background connection. But there's no
guarantee that the SIGHUP will have arrived and completed its work on
existing backends by that time, so the final test flaked.
We only care about the postmaster's state here, since we're testing the
GUC's effects on newly forked backends, so remove the background check
entirely.
Per buildfarm member serinus.
Reported-by: Alexander Lakhin <exclusion@gmail.com>
Discussion: https://postgr.es/m/a37a3cea-5bd8-4385-a1d4-bb89394c079c%40gmail.com
Backpatch-through: 18
CopyToJsonOneRow() stamped the row datum with the query's descriptor
only when the slot's descriptor was of type RECORD. A scan that does
not project returns the table's row type, so the keys came from the
table, not the query, losing column aliases and mislabelling rows
from UNION ALL branches with different column orders.
Fix by always using the query's descriptor. For non-virtual slots,
heap_copy_tuple_as_datum() can just be passed that descriptor, so
there is no extra cost. This is safe because a scan skips projection
only when tlist_matches_tupdesc() holds, so the layouts match.
Bug introduced in 7dadd38cda.
Reported-by: Shihao Zhong <zhong950419@gmail.com>
Reviewed-by: Shihao Zhong <zhong950419@gmail.com>
Reviewed-by: João Marcelo Detomini <joao.detomini@enterprisedb.com>
Discussion: https://postgr.es/m/CAGRkXqQHjxgufHPz86+dxGaQZt37hikduUArYtfbn2_3mryZAw@mail.gmail.com
Backpatch-through: 19
Commit 1f2245371e added a test for the case where logical decoding
is disabled and re-enabled while synchronizing a replication slot. Its
cleanup assumed the slot re-created on retry would still be temporary,
and so would be gone once the corresponding remote slot was
dropped. That isn't guaranteed: the catalog_xmin reserved on the
standby is not necessarily ahead of the remote slot's. In that case
the slot is persisted on the first retry, and the subsequent logical
decoding deactivation invalidates it on the standby rather than
dropping it, so the check that no synced slot is left behind fails.
Stabilize the cleanup by calling an extra pg_sync_replication_slots()
after dropping the remote slot and before disabling logical decoding on
the primary.
Per buildfarm member skink.
Reported-by: Fujii Masao <masao.fujii@gmail.com>
Suggested-by: Fujii Masao <masao.fujii@gmail.com>
Reviewed-by: shveta malik <shveta.malik@gmail.com>
Discussion: https://postgr.es/m/CAHGQGwFWXASACRnhHJgEdk8nWyLw4bLLDhiEiKSqBqd+tzJ2Pg@mail.gmail.com
Backpatch-through: 19
Given an Append or MergeAppend node, pgpa_build_scan needs to call
pgpa_make_scan for the node's own apprelids, and also for each element
of child_append_relid_sets. It did that, but it overlooked the fact
that the scan type applicable to the child scans might differ from
the scan type applicable to the parent scan. The problem can only
occur in one direction: an RTE_SUBQUERY (Merge)Append can appear on
top of a RTE_RELATION (Merge)Append, but not the reverse, so the
only failure mode here is to classify a partitionwise scan of a child
appendrel as PGPA_SCAN_ORDINARY rather than PGPA_SCAN_PARTITIONWISE,
leading to failure to include PARTITIONWISE(something) in the
generated advice string.
Backpatch-through: 19
Discussion: http://postgr.es/m/CA+TgmoYYHC4i-Gpzc9-JYHqih6pOK+6LTYFiNR-PioXG1nKkOQ@mail.gmail.com
Commit 7358abcc60 taught the planner to
store information about Append node consolidation in the final plan,
but it forgot that child_append_relid_sets needs to be adjusted
at setrefs.c time just like every other place where we store RTIs
in a plan tree. Fix that.
Although this can cause incorrect plan advice to be emitted in cases
involving multi-level partitioning, it seems more appropriate to put
the test case for it in pg_overexplain, so this commit does that.
Backpatch-through: 19
Discussion: http://postgr.es/m/CA+TgmoYYHC4i-Gpzc9-JYHqih6pOK+6LTYFiNR-PioXG1nKkOQ@mail.gmail.com
Pasting together sentences with placeholders is not suitable for
translation. Write out both variants instead.
Fix comma splice in the detail message while we're here.
ri_LockPKTuple() passed table_tuple_lock() the current command ID, read
at lock time. ExecLockRows(), which locks the row for the SPI path's
SELECT ... FOR KEY SHARE, uses the estate's es_output_cid, fixed when
the query starts and equal to the command ID of the query's snapshot.
The two differ if user code run between taking the snapshot and locking
the row, such as the equality function called by the index scan,
advances the command counter. If that code also updated the referenced
row, the lock reports TM_Invisible, and the check fails with "attempted
to lock invisible tuple", where the SPI path gets TM_SelfModified and
reports a foreign key violation.
To fix, use the snapshot's command ID in ri_LockPKTuple() to mirror
what ExecLockRows() does.
This is mostly hardening. With FK values that need a cast now checked
through SPI, the only user code that can run there is the opclass's
equality function, and it would have to write to the referenced table.
That's unlikely, but the fast path should still behave like SPI.
Discussion: https://postgr.es/m/CA+HiwqG79XK1oObdZ2AwT660CeJ6s3Mn4LrFPCme-k4L2rF_ag@mail.gmail.com
Backpatch-through: 19
In most of the backend code, XLogReaders open a WAL segment file with
BasicOpenFile() (xlogutils.c, WAL summarizer and WAL sender). On ERROR,
the opened file descriptor could leak while reading WAL segments, piling
them over time if a session can spawn XLogReaders at will, leading to
EMFILE with various consequences, the worst being a backend-side PANIC.
The problem has been reported as reachable with pg_walinspect (EXECUTE
privilege revoked from PUBLIC by default), or replication slot functions
(REPLICATION privilege required).
XLogReaderState gains a memory context reset callback, registered if a
segment_open callback has the idea to open a WAL segment with
BasicOpenFile(). XLogReaderRegisterResetCallback() can be used by a
segment_open callback to ensure the cleanup of any opened file
descriptor.
No backpatch is done. This breaks the ABI of XLogReaderState (sizeof),
and MemoryContextUnregisterResetCallback() is new in v19. There may be
an argument for fixing this issue in v19, but the release is close
enough that this change does not seem worth taking a risk for.
Author: Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com>
Reviewed-by: Bertrand Drouvot <bertranddrouvot.pg@gmail.com>
Reviewed-by: Sami Imseih <samimseih.pg@gmail.com>
Reviewed-by: Chao Li <li.evan.chao@gmail.com>
Reviewed-by: Michael Paquier <michael@paquier.xyz>
Discussion: https://postgr.es/m/CALj2ACVwDuOXXDjj2cVdnTKoxsgTSLDin4XoL63AnM6aUgMQaA@mail.gmail.com
The SPI path checks a foreign key with SELECT ... FOR KEY SHARE, which
ExecCheckXactReadOnly() refuses in a read-only transaction unless the
referenced table is temporary. The fast path locks the referenced row
without that check, so for example a deferred check that fires at
COMMIT after SET TRANSACTION READ ONLY succeeded, having locked a row
in a read-only transaction, where the SPI path fails with "cannot
execute SELECT FOR KEY SHARE in a read-only transaction".
Fix the fast path to refuse it the same way so the behavior matches
with checks going through SPI.
Discussion: https://postgr.es/m/CA+HiwqG79XK1oObdZ2AwT660CeJ6s3Mn4LrFPCme-k4L2rF_ag@mail.gmail.com
Backpatch-through: 19
Restoring each zstd-compressed full-page image currently creates and
destroys a decompression context. One decompression context gets now
saved in a XLogReaderState on first use, reused for the decompression of
any follow-up zstd-compressed pages.
The decompression context is freed in XLogReaderFree(). In non-FRONTEND
builds, a memory context reset callback is registered to make sure that
the decompression context does not leak. Note that zstd has APIs to
either initialize a pre-allocated decompression context or assign custom
allocation and free routines, but none of these are available for
builds that dynamically link to the zstd library (ZSTDLIB_STATIC_API).
Tomas Vondra has done some benchmarking showing that for a FPI-heavy set
of WAL records, reusing the decompression context can nicely improve the
recovery time, bringing it close to lz4. In my own measurements, I have
seen up to a 4% faster recovery time when replaying records made mostly
of zstd-compressed full-page images between HEAD and this improvement.
Andrey has seen better numbers than that.
Author: Andrey Borodin <x4mmm@yandex-team.ru>
Reviewed-by: Michael Paquier <michael@paquier.xyz>
Discussion: https://www.postgresql.org/message-id/asRsOl73nf8GeBFd@paquier.xyz
Commit 6aba42c660 made slot synchronization skip persisting a new
slot if logical decoding got disabled after the remote slot
information was fetched. As noted in its XXX comment, the check
missed the case where the last logical slot on the primary is dropped
and re-created with the same name in the meantime. If the standby had
replayed both the deactivation and the re-activation by then, the
slot was persisted with a restart_lsn preceding the deactivation.
Subsequent synchronization cycles failed with "unexpected logical
decoding status change" when advancing the slot, which also stopped
the synchronization of all other failover slots, and the slot could
not be decoded after promotion.
Fix this by remembering the end LSN of the last replayed
XLOG_LOGICAL_DECODING_STATUS_CHANGE record that enables logical
decoding, and by requiring the remote restart_lsn to be at or after it
right after creating the local slot. The check is skipped until replay
reaches the remote restart_lsn, as we can't tell anything about WAL that
hasn't been replayed yet. Once the slot exists, a later deactivation
invalidates it, so remove the check before persisting the slot. We
compare the remote restart_lsn rather than the local one, so we may drop
a slot that would have been usable, but the retry in the next
synchronization cycle fetches fresh information.
Backpatch to v19, where commit 67c20979ce made logical decoding toggle
dynamically.
Reported-by: Nikolay Samokhvalov <nik@postgres.ai>
Reviewed-by: shveta malik <shveta.malik@gmail.com>
Reviewed-by: Nisha Moond <nisha.moond412@gmail.com>
Reviewed-by: Amit Kapila <amit.kapila16@gmail.com>
Reviewed-by: Zhijie Hou <houzhijie22@gmail.com>
Discussion: https://postgr.es/m/CAM527d_eV_BAYFiQnfZLSPfHoihye=nOi-OnAM_57pdH+F+gfA@mail.gmail.com
Backpatch-through: 19
LogicalDecodingCtl had no init callback so there is no guarantee that
its fields are initialized. Its startup function
StartupLogicalDecodingStatus() also returns without touching the
fields when wal_level is 'minimal'.
Initialize them in an init callback explicitly. This is not a bug fix
but we just shouldn't be depending on the allocator by accident.
Backpatch to v19, where logicalctl.c was introduced, since a
subsequent bug fix for slot synchronization adds a new field to
LogicalDecodingCtlData and relies on this callback to initialize it.
Reviewed-by: shveta malik <shveta.malik@gmail.com>
Reviewed-by: Nisha Moond <nisha.moond412@gmail.com>
Discussion: https://postgr.es/m/CAM527d_eV_BAYFiQnfZLSPfHoihye=nOi-OnAM_57pdH+F+gfA@mail.gmail.com
Backpatch-through: 19
Insist that the input lexemes be already sorted and non-duplicate.
There's no good reason for them not to be, since the required ordering
is simple and platform-independent. The previous coding was willing
to re-sort them, but that left a gap: it didn't de-duplicate them.
De-duplicating would potentially require merging position lists,
which would be painful; we can't easily re-use tsvectorin()'s logic
for that, since it uses a different temporary representation.
Let's just dodge the problem by insisting the lexeme list be fully
correct as-presented, as we were already doing for the position lists.
Also, there was an oversight in the position-list checking: it did
not verify that the first position isn't zero. tsvectorin() rejects
that, so this should too.
While these problems allow tsvectorrecv() to construct tsvectors
that would not be generated internally, the vectors aren't so broken
that we can't work with them. Therefore, the best course of action
seems to be to apply these tighter rules in master but not back-patch.
Back-patching might break application workflows that aren't causing
any serious problems. Any such users will need to clean their data
before loading it into PG >= v20, but they'd face that issue anyway,
and a major update is a better time for it than a minor update.
Bug: #19748
Reported-by: Ke <kehan5800@gmail.com>
Author: Tom Lane <tgl@sss.pgh.pa.us>
Discussion: https://postgr.es/m/19748-6075f0508fa1ffc0@postgresql.org
This backwards-compatibility hack was introduced twenty years ago (in
8.2), and has surely been of no value for at least the last ten years.
However, we shouldn't just leave it sit, because as was just pointed
out it creates a dump/reload hazard: if it's set in the receiving
environment then array contents will not be restored correctly.
Other applications would likely be broken by it too, so whatever
residual value it may have seems outweighed by the possibility of
misuse.
Our precedent for retiring some other comparable parameters, such as
standard_conforming_strings, has been to leave the parameter in
existence but force it to a fixed value. I judge however that
that's not worth the trouble here, because there's no reason to
believe that a non-negligible number of applications are either
setting or reading this value. Let's just delete it in the master
branch and be done with it.
Bug: #19747
Reported-by: Ke <kehan5800@gmail.com>
Author: Tom Lane <tgl@sss.pgh.pa.us>
Reviewed-by: Laurenz Albe <laurenz.albe@cybertec.at>
Discussion: https://postgr.es/m/19747-69b01e7fc190cd58@postgresql.org
The page a bit of a mess, with notes interpersed in the list of
parameters, and two "notes" subsections that weren't in the Notes
section.
Limit the commentary under "Description" and "Parameters" to a minimum;
move the existing text to appear in the "Notes" section. Both the
"Notes on Clustering" and "Notes on Resources" subsections, which were
under "Description", are moved to be under "Notes" instead. Add a new
"Notes on Concurrent Operation" subsection there, which now carries some
text that was in "Parameters", and gets some additional text to
(hopefully) explain resource consumption more clearly -- notably, the
hard limit of 104 million tuples that can be concurrently updated/deleted
before running into the palloc size limit.
Add appropriate cross-links.
Author: Radim Marek <radim@boringsql.com>
Author: Álvaro Herrera <alvherre@kurilemu.de>
Author: Shihao Zhong <zhong950419@gmail.com>
Backpatch-through: 19
Discussion: https://postgr.es/m/CAJgoLk+dodrwuwCuXERGYwgjQzSwrKd+xgitYrL32oWDt39zvA@mail.gmail.com
With synchronous_commit = remote_apply, a commit could remain blocked even
after the synchronous standby had applied it when
wal_receiver_status_interval was set to zero. With both
wal_sender_timeout and wal_receiver_timeout disabled, the wait could last
indefinitely.
Commit 400a790a48 introduced this regression while avoiding unnecessary
duplicate standby status replies. It changed apply notifications from forced
replies to replies that check for WAL position changes. However,
XLogWalRcvSendReply() still returned early for non-forced replies when
wal_receiver_status_interval was zero, before checking the apply position.
Fix this by allowing apply-notification requests to bypass the early return
while preserving the existing checks that suppress unnecessary duplicate
replies.
Author: Vaibhav Dalvi <vaibhav.dalvi@enterprisedb.com>
Reviewed-by: Ashutosh Sharma <ashu.coek88@gmail.com>
Reviewed-by: Fujii Masao <masao.fujii@gmail.com>
Discussion: https://postgr.es/m/CA+vB=AHKUhc9GCDwoLr=SshTBCQG09kp8TEXew50UtD5rYqowg@mail.gmail.com
Backpatch-through: 19
Commit e5d25959cf made REPACK null-out values of dropped columns in
the NEW tuple of an UPDATE. However, a concurrent INSERT can carry one
too, for example when a trigger returns a copy of an existing row.
REPACK (CONCURRENTLY) then kept the values for such columns, which is
wasteful -- or worse, failed with "row is too big" if the new heap
happens not to have a TOAST table.
Move their clearing from prepare_concurrent_update() to restore_tuple()
so that it affects all tuples processed, not just the UPDATE ones.
Author: Shihao Zhong <zhong950419@gmail.com>
Backpatch-through: 19
Discussion: https://postgr.es/m/CAJgoLkK2UBzB1J9buCsSUbjf7bOqz-o_0=CeiTruU789BhBw-Q@mail.gmail.com
unconstify_constexpr is like unconstify but for contexts where a
constant expression is required, such as for initializing global
variables. It provides the same level of checking as unconstify, but
the checking only works on GCC, otherwise it lets anything through.
Also, if the check fails, it gives a less clear error message. So it
should only be used when necessary.
(An unvolatize_constexpr doesn't seem necessary, but it could be added if
required.)
Reviewed-by: Rui Zhao <zhaorui126@gmail.com>
Discussion: https://www.postgresql.org/message-id/c76b72dd-5736-453a-b698-6968d42e4768%40eisentraut.org
When a join alias Var with nonempty varnullingrels expands to an
expression that cannot carry the nullingrels itself, such as the
RowExpr of a whole-row Var, add_nullingrels_if_needed() wraps it in a
PlaceHolderVar. The choice of the PHV's phrels had several problems.
First, phrels was taken from all Vars in the expression. If a LATERAL
subquery within the join has been pulled up, the expression can
contain lateral references to rels outside the join, so the PHV could
be evaluated above the outer join that is supposed to null it. This
gave wrong results, or "wrong phnullingrels" errors.
Second, for a variable-free expression we fell back to the join's
relids but removed the join's own outer-join relid. That leaves a set
spanning both sides of the outer join without the join itself, which
can make the PHV look nullable by that join and cause "wrong
phnullingrels" errors.
Third, that fallback raised "unsupported join alias expression" when
the Var is an outer reference from a subquery. The restriction is
unnecessary: every planner caller passes root->parse as the query, so
the Var always belongs to root->parse regardless of its varlevelsup.
To fix, restrict phrels to the rels within the join, and if nothing is
left, evaluate the PHV at the join including its outer-join relid, at
any varlevelsup. Add an assertion that the query is root->parse.
Author: Richard Guo <guofenglinux@gmail.com>
Discussion: https://postgr.es/m/CAMbWs49q_gvoeOoF0+KZ-DpQaDb=nGFfSgrz2V_n1_64L4ZNVQ@mail.gmail.com
Backpatch-through: 16
When a query has aggregates or GROUP BY, parseCheckAggregates()
flattens join alias Vars in the grouping expressions, target list and
HAVING clause so that aliased and unaliased references compare equal.
The parser has no PlannerInfo, so this relies on every join alias
expression being able to carry varnullingrels without a PHV. That
doesn't hold for a whole-row Var of a join on the nullable side of an
outer join: it expands to a RowExpr, and pushing the nulling bits down
into its fields would turn a NULL row into ROW(NULL, ...). Such
queries therefore failed with "unsupported join alias expression".
To fix, leave a nulled whole-row join Var unexpanded when no root is
available. The planner expands it later and wraps it in a
PlaceHolderVar as before. Nothing is lost for GROUP BY matching: a
nulled whole-row Var can only be equal to the same Var, never to an
expression over the join's columns.
This has been broken since the introduction of varnullingrels in v16.
Author: Richard Guo <guofenglinux@gmail.com>
Discussion: https://postgr.es/m/CAMbWs49q_gvoeOoF0+KZ-DpQaDb=nGFfSgrz2V_n1_64L4ZNVQ@mail.gmail.com
Backpatch-through: 16
reindexdb batches the REINDEX commands of all indexes attached to the
same table into a single multi-statement query when using multiple jobs,
as two steps with query "gen" and "run".
Such a query fails with REINDEX CONCURRENTLY, as this command cannot run
inside a transaction block. Batching cannot be avoided because the
indexes of one table have to be re-indexed one at a time to avoid
conflicts across jobs.
Let's error out cleanly on the reindexdb side if this combination of
options is used, instead of having the server complain about that. This
case has not been considered by 47f99a407d.
If there is interest in supporting this combination of options, the code
should be redesigned for this purpose with some better batching, rather
than try to use sets of "gen" and "run" routines. As currently written,
an index-level reindexdb uses a set of "gen" steps with one final "run"
step, which is nice code-wise. (Note: I doubt that that this would be
worth the effort.)
Author: Kirill Reshke <reshkekirill@gmail.com>
Discussion: https://postgr.es/m/CALdSSPiEkAmS+n7Ea9XDMrLjkbQk=CrB7zw1kcq9B47hfKUhXw@mail.gmail.com
Backpatch-through: 17
As of a32b8dc98 and friends, amcheck will complain about bpchar
indexes that claim to allow de-duplication. This causes our
cross-version-upgrade tests to fail when upgrading from a version
that lacks those commits, since there are some such indexes in the
core regression tests. Drop such indexes from the upgrade source
database. (oidvector indexes would be problematic too, but
there aren't any user-defined indexes on oidvector in our tests.)
Author: Peter Geoghegan <pg@bowt.ie>
Reviewed-by: Tom Lane <tgl@sss.pgh.pa.us>
Discussion: https://postgr.es/m/CAH2-Wzk-oO3HjLaByo8Vpse6Tpha1mqAW8DFShh-kS-Wo59ugA@mail.gmail.com
Backpatch-through: 14
A smart or fast shutdown during an early crash restart can wait forever
on the checkpointer and I/O workers. FatalError is still set, so
PM_STOP_BACKENDS adds them as processes to wait for, but these are
relaunched by the restart and ignore the SIGTERM that a shutdown sends.
AbortStartTime has been reset at that point, meaning that SIGKILL would
also not fire.
SIGQUIT is now sent instead of SIGTERM when FatalError is set, via
HandleFatalError(), arming AbortStartTime. Going through that routine
rather than signaling the children directly matters for the startup
process: TerminateChildren() marks the startup process as signaled, so
its exit is not later reported as a failure. Note that after sending
the SIGTERM, we check for any running processes, assuming that auxiliary
processes have been already SIGQUIT'd for an immediate shutdown or for a
FatalError.
A test case is added, able to stress the scenario of a shutdown
triggered during the early phases of crash recovery with a custom
restore command waiting for a shutdown by scanning the server logs. The
CI is stable with this test (several runs done); the buildfarm will be
the final judge regarding its stability.
No backpatch is done for now, to be on the conservative side. This
feels also a slightly risky change for v19, which is close to release,
and letting this change bake on HEAD a bit would not hurt.
This problem has been originally reported by Justin Pryzby back in 2023,
without being fixed, and still existed on HEAD.
Reported-by: Justin Pryzby <pryzby@telsasoft.com>
Author: Ayush Tiwari <ayushtiwari.slg01@gmail.com>
Co-authored-by: Michael Paquier <michael@paquier.xyz>
Discussion: https://postgr.es/m/CAJTYsWXCLi00cJ6-_OkMJB4UjyV6EZjc86QwmgE9=iH_C0R-Aw@mail.gmail.com
Discussion: https://postgr.es/m/ZWlrdQarrZvLsgIk@pryzbyj2023
bpchar equality ignores trailing spaces, so equal values in a column
with no length specifier can have different images. Similarly,
oidvector equality intentionally pays no attention to the array lower
bound field. We nevertheless allowed nbtree deduplication to be used
with indexes built with bpchar_ops, bpchar_pattern_ops, or
oidvector_ops. A posting list split could therefore place a TID in an
index tuple whose key was equal to, but not bytewise identical to, the
corresponding table value. This allowed index-only scans to return
subtly wrong results.
To fix, drop support function 4 from the affected opclasses (we can't
change catalog contents on branches 14 through 18, so we just disable
deduplication by adding hard-coding to the relevant support functions).
Deliberately leave the oidvector opclass alone on branches 14 through
18, though. Disallowing oidvector deduplication on these branches would
make pg_amcheck report that pg_proc_proname_args_nsp_index has a corrupt
metapage, needlessly alarming users.
System catalog oidvector values always have a lower bound of 0, so the
issue can't cause harm there; a user can only hit it by indexing an
oidvector column that stores the output of a function like trim_array().
It seems very unlikely that any user would ever do that.
Users should REINDEX any B-Tree index on a bpchar column.
Author: Shihao Zhong <zhong950419@gmail.com>
Co-authored-by: Peter Geoghegan <pg@bowt.ie>
Reported-by: Ke <kehan5800@gmail.com>
Reported-by: Tom Lane <tgl@sss.pgh.pa.us>
Reviewed-by: Tom Lane <tgl@sss.pgh.pa.us>
Bug: #19749
Discussion: https://postgr.es/m/19749-d46adf7c7d910b07@postgresql.org
Discussion: https://postgr.es/m/CAGRkXqQQ311Lg=62x5UdZtPuRFgpT6XpPMw4X9E3tgbTu162Mw@mail.gmail.com
Backpatch-through: 14
seg.sgml claims that a seg can store 7 significant digits, but this
is nonsense. The underlying float4 data type is not (quite) that
precise, and seg_out() intentionally rounds off to at most FLT_DIG
decimal places. With IEEE float arithmetic, FLT_DIG will be defined
as 6, or at least that's the case on all platforms I know of.
AFAICT this thinko goes clear back to the introduction of seg
(commit a3694b420), but it wasn't questioned until now.
Bug: #19744
Reported-by: Ke <kehan5800@gmail.com>
Author: Tom Lane <tgl@sss.pgh.pa.us>
Discussion: https://postgr.es/m/19744-8c700582bbb1d075@postgresql.org
Backpatch-through: 14
Previously, a backend with an assigned XID deferred the
XLogLogicalInfo update requested by a procsignal barrier, and we
applied it in AbortTransaction() on abort. However, a failed
transaction block keeps its XID until ROLLBACK, so a barrier absorbed
while idle in that state was deferred, and CleanupTransaction() did
not apply it. The next transaction then ran entirely with the stale
value. If logical decoding had just been enabled, that transaction's
changes were written without logical information and silently skipped
by decoding.
Fix by applying the pending update in CleanupTransaction() instead of
AbortTransaction(). We call it after resetting the top-level XID
because, unlike the commit and prepare paths, CleanupTransaction()
isn't always called with interrupts held, so a barrier absorbed before
the reset would be deferred again and carried into the next
transaction.
Patch by Sergei Patiakin, with tests added by me.
Oversight in commit 67c20979ce.
Reported-by: Sergei Patiakin <sergei.patiakin@enterprisedb.com>
Author: Sergei Patiakin <sergei.patiakin@enterprisedb.com>
Reviewed-by: Shlok Kyal <shlok.kyal.oss@gmail.com>
Reviewed-by: Hayato Kuroda <kuroda.hayato@fujitsu.com>
Reviewed-by: Masahiko Sawada <sawada.mshk@gmail.com>
Reviewed-by: Bertrand Drouvot <bertranddrouvot.pg@gmail.com>
Discussion: https://postgr.es/m/CANE55rApeNAFaqxLWrvm-NC0Y5gkrBFZFVaHVYP89AGS2SYMcA@mail.gmail.com
Backpatch-through: 19
COPY FROM runs its WHERE clause through eval_const_expressions(),
which among other things replaces NOT above an operator with the
operator's negator, if it has one (e.g., NOT (a > 0) becomes
a <= 0). The node it builds for the negator has no opfuncid, and
since COPY FROM never calls fix_opfuncids() on the clause, nothing
fills it in before the executor needs it. So
COPY tab FROM stdin WHERE NOT (a > 0)
fails with "cache lookup failed for function 0".
To fix, call fix_opfuncids() on the clause, as the relcache does
for index predicates.
Oversight in commit 31f3817402.
Reviewed-by: Tom Lane <tgl@sss.pgh.pa.us>
Discussion: https://postgr.es/m/asPZRTci4oBvxR9A%40nathan
Backpatch-through: 14
