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
When reloading the SSL configuration fails, the existing config is
left in place to keep the server from being inaccessible. The GUC
backing SNI, ssl_sni, was however changed. This could lead to new
connections trying to access non-existing configurations. Fix by
storing the ssl_sni setting at the time of initialization in the
hosts context and only use the stored value during the handshake.
The patch submitted by the reporter has been significantly reworked
and expanded. Additional test cases written by Rui Zhao.
Author: Anthropic OSS program
Co-authored-by: Daniel Gustafsson <daniel@yesql.se>
Co-authored-by: Rui Zhao <zhaorui126@gmail.com>
Reported-by: Anthropic OSS program
Reviewed-by: Zsolt Parragi <zsolt.parragi@percona.com>
Reviewed-by: Jacob Champion <jacob.champion@enterprisedb.com>
Reviewed-by: Rui Zhao <zhaorui126@gmail.com>
Discussion: https://postgr.es/m/20260920211619.9c.noahmisch@microsoft.com
Backpatch-through: 19
pull_up_simple_subquery() rechecks is_simple_subquery() after pulling
up the subquery's own subqueries, but it flattened join alias Vars in
the subquery's targetlist only after that, and never in its quals. A
join alias Var within the subquery, such as a whole-row Var of a join,
can expand to an expression containing lateral references to the outer
query once the join's inputs have been pulled up, and until it is
flattened is_simple_subquery() cannot see them. We could thus pull up
a LATERAL subquery whose targetlist or quals reference rels outside
the lowest outer join above it. This leads to assertion failures in
distribute_qual_to_rels() or "wrong phnullingrels" errors, and in some
cases to wrong query results, where rows that should have been
preserved by an outer join above the subquery are discarded.
To fix, flatten join alias Vars in the targetlist, and in the jointree
quals if the subquery is LATERAL, before the recheck. If the recheck
fails, the flattened copy is discarded along with the rest of the
modified subquery, so no harm is done.
Back-patch to all supported branches, as this can lead to wrong query
results.
Author: Richard Guo <guofenglinux@gmail.com>
Reviewed-by: Tatsuya Kawata <kawatatatsuya0913@gmail.com>
Discussion: https://postgr.es/m/CAMbWs48GFZ=3Bjc1ug9JRsB0Qupg+CGcb9js5vmTM6kY2OGWOg@mail.gmail.com
Backpatch-through: 14
This commit addresses three gaps in the error reporting data, as of:
- In _gin_parallel_merge(), for a parallel GIN build, the last key was
counted twice in the progress report. Oversight in 8492feb98f.
- _gin_process_worker_data() forgot to increment bs_numtuples when
trimming a list of pointers. Oversight in b229c10164.
- blocks_done for an index build's heap scan was incorrect for its first
block. There was a one-off in heapam_scan_get_blocks_done(), causing
the routine to return the total number of blocks on the first block
scanned. Oversight in ab0dfc961b.
Manu has reported two issues. I have come across the one in
_gin_process_worker_data() while reviewing his patches.
All of these are mostly cosmetic, leading to incorrect data reported in
the progress views for a short period of time, so no backpatch is done.
Author: Manu <manuelreyesbravo@gmail.com>
Author: Michael Paquier <michael@paquier.xyz>
Discussion: https://postgr.es/m/179012373464.1732093.11171496412950256250@gmail.com
Commit e5d25959cf added the repack_dropped test to Meson but omitted it
from the Makefile, so make-based isolation runs did not execute it. The
test's teardown also left the injection_points extension installed,
relying on a later test to remove it.
Add repack_dropped to the Makefile's isolation tests and drop the
extension during teardown.
Author: Fujii Masao <masao.fujii@gmail.com>
Reviewed-by: Álvaro Herrera <alvherre@kurilemu.de>
Discussion: https://postgr.es/m/CAHGQGwG9bHxhs4Nj+9avSjSGh0vdYNQgEuz+pmJ-H5be6tEBTw@mail.gmail.com
Backpatch-through: 19
Previously, dropping a NOT NULL column, adding a new column with the same
name, and making it a primary key in a single ALTER TABLE command could
fail with "primary key column ... is not marked NOT NULL". This happened
because ALTER TABLE checked for an existing NOT NULL constraint before
dropping the old column. Finding the constraint caused it to skip adding
one for the new column, even though the old constraint would be removed
with the old column.
Defer this check until after column drops and additions have been
executed, so that it examines the columns that will actually form the
primary key. Add any required NOT NULL constraints before creating the
primary-key index.
Backpatch to v18, where this issue was introduced by commit a379061a22.
Reported-by: Ludvig Janiuk <ludvig.janiuk@proton.me>
Author: Fujii Masao <masao.fujii@gmail.com>
Co-authored-by: Manuel Reyes Bravo <manuelreyesbravo@gmail.com>
Discussion: https://postgr.es/m/0a9b4c44-cdff-4ec8-a8fb-b3a8b59e9ba9@proton.me
Backpatch-through: 18
Since commit e28cf96e9, the copies of a query level's PlaceHolderVars
that were pushed down into its subqueries are preprocessed by that
level before its SubLinks are turned into SubPlans, so such a copy may
already contain SubPlans by the time the subquery is planned. That
commit taught build_subplan not to process the SubLinks of a PHV that
is passed down as a SubPlan argument, but overlooked that such a copy
can also appear within the arguments of an outer-level Aggref,
GroupingFunc, or ReturningExpr. The SubLinks in those arguments are
still processed when build_subplan collects the node, and that
processing descended into the already-preprocessed PHV and tripped the
no-SubPlan assertion in process_sublinks_mutator.
To fix, don't descend into PHVs when processing the SubLinks in such a
node's arguments. Any PHV found there is a pushed-down copy that its
owning level has already preprocessed.
Back-patch to v16, as with commit e28cf96e9.
Author: Ayush Tiwari <ayushtiwari.slg01@gmail.com>
Reviewed-by: Richard Guo <guofenglinux@gmail.com>
Discussion: https://postgr.es/m/CAJTYsWW8THbrnsmYnDqnUgg0PjGrqw2vNOWb8qx7eu191KPW3g@mail.gmail.com
Backpatch-through: 16
ri_FastPathCheck() read riinfo->fpmeta again after
ri_CheckFunctionPermissions(), which looks up catalog entries and so
can process invalidation messages. If one of them reaches
InvalidateConstraintCacheCallBack() for the constraint, as a pg_amop
change or a cache reset would, the callback detaches the metadata and
sets riinfo->fpmeta to NULL, which build_index_scankeys() then
dereferences.
The callback already defers freeing detached metadata until
AtEOXact_RI(), and its comment assumes callers keep their own pointer
to it, so use a local pointer instead of riinfo->fpmeta.
There is no test, as hitting this needs an invalidation to arrive
during those catalog lookups.
Discussion: https://postgr.es/m/CA+HiwqFaipkMsZ8XP-9sMk21h0Q7rih=NpJQXsBAaD39VtnrOA@mail.gmail.com
A Var that is needed by a join but is not a GROUP BY expression
becomes an extra grouping key for partial aggregation. The
EquivalenceClass for sorting on it is created when the grouped paths
are generated, which is after the children of appendrels and
partitionwise joins have received their EC members, so it has no child
members. Sort-based partial aggregation on a child relation then
failed with "could not find pathkey item to sort", as with a
partitionwise join whose join clause includes a non-equality
condition.
To fix, add the missing child members when the grouping pathkeys of a
child relation are built. This is done by the new function
add_child_rel_pathkey_equivalences(), which handles both child base
relations and child joins, including keys that are nullable by an
outer join within the child join.
Reported-by: Robert Haas <robertmhaas@gmail.com>
Author: Richard Guo <guofenglinux@gmail.com>
Discussion: https://postgr.es/m/CA+Tgmob7iSM9YkRM44VjUDuaCchW-fY54MV5njpTZTL9uNyV4w@mail.gmail.com
Backpatch-through: 19
build_grouped_rel() makes a flat copy of the input relation, including
its FDW fields. As a result, when building a grouped join relation
for eager aggregation, add_paths_to_joinrel() called the FDW's
GetForeignJoinPaths for it. postgres_fdw then failed with "Aggref
found where not expected" on the partial Aggrefs in the target list,
and in any case it would have treated a partially aggregated input as
a plain foreign scan.
FDWs are not prepared to handle grouped relations, so clear serverid,
fdwroutine and fdw_private in build_grouped_rel().
Reported-by: Robert Haas <robertmhaas@gmail.com>
Author: Richard Guo <guofenglinux@gmail.com>
Discussion: https://postgr.es/m/CA+Tgmob7iSM9YkRM44VjUDuaCchW-fY54MV5njpTZTL9uNyV4w@mail.gmail.com
Backpatch-through: 19
When a Var of the relation to be partially aggregated is known equal
to a GROUP BY expression through an EC, get_expression_sortgroupref()
let it reuse that expression's SortGroupClause. If the two are of
different types, as with an int2 grouping key joined to an int4
column, the Var was then grouped using the other type's equality
operator, which could put unequal values into the same group and give
wrong results.
To fix, accept only EC members of the same type as the grouping
expression. A cross-type Var is then treated like any other join key
and gets a SortGroupClause built for its own type.
Reported-by: Robert Haas <robertmhaas@gmail.com>
Author: Richard Guo <guofenglinux@gmail.com>
Discussion: https://postgr.es/m/CA+Tgmob7iSM9YkRM44VjUDuaCchW-fY54MV5njpTZTL9uNyV4w@mail.gmail.com
Backpatch-through: 19
Commit 1ff3180ca0 made the leader of a parallel autovacuum propagate
changes to the cost-based delay parameters to its workers, but missed
track_cost_delay_timing. Workers therefore kept the value they had at
launch: turning the parameter on during the vacuum left
pg_stat_progress_vacuum.delay_time counting only the leader's naps,
and turning it off didn't stop workers from adding theirs. Throttling
itself was not affected.
Fix by adding track_cost_delay_timing to PVSharedCostParams. Since a
worker's setting can now change while it runs, we report its residual
delay time at exit whenever any has been accumulated, rather than only
if track_cost_delay_timing is still on.
Backpatch to v19, where parallel autovacuum was introduced.
Reported-by: Sami Imseih <samimseih.pg@gmail.com>
Author: Sami Imseih <samimseih.pg@gmail.com>
Reviewed-by: Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com>
Reviewed-by: Zsolt Parragi <zsolt.parragi@percona.com>
Reviewed-by: Masahiko Sawada <sawada.mshk@gmail.com>
Discussion: https://postgr.es/m/CAN12+YJZaALd3zMeKVVRTwcuY5_yDtR_vMPwWc8QNgeYxrn4Zg@mail.gmail.com
Backpatch-through: 19
Commit ce8d5fe0e failed to think hard enough about the case where
we're forced to replan the expression and it becomes no longer simple.
The code produced the right expression result, but its handling of
FOUND and ROW_COUNT was too simplistic since we might have obtained
zero rows. To fix, don't use exec_eval_expr, but go directly to
exec_eval_simple_expr, and if that reports failure then fall through
to the full SPI code path.
Another thinko in ce8d5fe0e is that it added
if (expr->plan == NULL)
exec_prepare_plan(estate, expr, 0);
which is quite useless since we cannot reach here without having
planned the expression at least once. (If the call were reachable
it'd likely be wrong, since it's not passing the same flags as
either the preceding call or the one inside exec_eval_expr.)
Author: shihao zhong <zhong950419@gmail.com>
Reviewed-by: Tom Lane <tgl@sss.pgh.pa.us>
Discussion: https://postgr.es/m/CAGRkXqQB_aqWU9OeVPGvATBVZ7c9EYrNXJyHG0jKXW0FGAy2-g@mail.gmail.com
Backpatch-through: 19
pg_blocking_pids() assumes that members of the same lock group
never block each other, but that hasn't been true for relation
extension locks since v13. This commit teaches the function to
report lock group members as blockers for relation extension locks.
Since parallel workers are reported with their leader's PID, the
result may now include the blocked session's own PID.
Oversight in commit 85f6b49c2c.
Author: Bertrand Drouvot <bertranddrouvot.pg@gmail.com>
Reviewed-by: Chao Li <li.evan.chao@gmail.com>
Reviewed-by: Kirill Reshke <reshkekirill@gmail.com>
Reviewed-by: Shihao Zhong <zhong950419@gmail.com>
Discussion: https://postgr.es/m/apTpV8h%2BCYHf%2B1PJ%40bdtpg
Backpatch-through: 14
When we converted these types' input functions to report errors
through the soft-error mechanism, we missed that stack-overflow
errors can be triggered through bad user input. Convert those
to be reported softly, too.
Bug: #19745
Reported-by: Ke <kehan5800@gmail.com>
Author: Tom Lane <tgl@sss.pgh.pa.us>
Discussion: https://postgr.es/m/19745-39cd7b93a505f9a9@postgresql.org
Backpatch-through: 16
