In the perhaps-unlikely scenario where your session default settings
specify read-only, deferrable, serializable transactions, the implicit
transaction created by ProcessIncomingNotify() honored those settings
and would wait for a safe snapshot to be creatable; thereby getting
blocked by any long-running concurrent transaction. This is a bit
surprising, especially if you think you are committing a transaction
that doesn't have those properties.
There doesn't seem to be any good semantic reason for NOTIFY reporting
to wait for a safe snapshot, so create a code pathway that lets it
just get a plain snapshot, independently of transaction mode.
Given the lack of field complaints about such issues, I don't feel a
need to back-patch this.
Author: shihao zhong <zhong950419@gmail.com>
Co-authored-by: Tom Lane <tgl@sss.pgh.pa.us>
Discussion: https://postgr.es/m/CAGRkXqQ1bmVwBw2CXiyZeWWP=XkHPXLPcQ7BSg2Q5JKSvDAnTA@mail.gmail.com
get_param_path_clause_serials() handles join paths and Append paths
explicitly, and for anything else returns the ppi_serials of the
path's ParamPathInfo on the assumption that it is a baserel scan. But
Material, Memoize, Projection, Sort, IncrementalSort, Unique, Agg and
GroupingSets paths all inherit their subpath's ParamPathInfo, and that
assumption fails when the subpath is a join or Append.
This could drop a join clause for good. If a LATERAL UNION ALL
subquery references an outer relation in an expression that becomes an
EquivalenceClass member, the members get no child version of it, so
their parameterized scans cannot enforce the derived clause. The
appendrel's ParamPathInfo claims it nonetheless, and once a
Materialize sat on the Append, create_nestloop_path() dropped the
clause from the join above as already enforced, returning rows that
fail it.
To fix, make get_param_path_clause_serials() recurse through such
wrapper paths, since they enforce no clauses of their own. Also use
it in get_memoize_path(), which unsafely read ppi_serials directly for
its inner-unique check.
To try to clarify what's going on here, introduce a new function
get_wrapper_parampathinfo() that pathnode.c should use instead of
directly assuming that copying a wrapper path's subpath's param_info
is what to do. This is just cosmetic at present but might become
less so in future.
Back-patch to v16, where the serial-based detection of enforced
clauses was introduced.
Author: Richard Guo <guofenglinux@gmail.com>
Co-authored-by: Tom Lane <tgl@sss.pgh.pa.us>
Reviewed-by: shihao zhong <zhong950419@gmail.com>
Reviewed-by: Matheus Alcantara <matheusssilv97@gmail.com>
Discussion: https://postgr.es/m/CAMbWs49jVFRZ7oOgMK9zYt7d=SLxpiz=dN=QMu5iV8TS+9T0AA@mail.gmail.com
Backpatch-through: 16
Commit 1811f1af9 put outer tuples with null join keys into a
tuplestore and emitted them at the end of the batch, which risks
expensive spills to disk if there are a lot of them. It's difficult
to avoid the tuplestore in a multi-batch parallel join. But in the
non-parallel and single-batch cases we can just emit such tuples
immediately, at the tiny cost of propagating the "is-null" result
flag one call level further up. This avoids a performance regression
from v18 for such cases.
Author: shihao zhong <zhong950419@gmail.com>
Co-authored-by: Tom Lane <tgl@sss.pgh.pa.us>
Discussion: https://postgr.es/m/CAGRkXqTgugmYrtnP_11NjL_GCXwAfzVfm5WKUNFsfZOzxDF2rA@mail.gmail.com
Backpatch-through: 19
byteaout and pg_mcv_list_out depend on bytea_output GUC, while
float4out, float8out, and the geometric type output functions depend
on extra_float_digits GUC. The BRIN minmax-multi summary output
function also calls the summarized type's output function, so its result
can depend on the type's output settings.
Mark these functions STABLE instead of IMMUTABLE to reflect their
dependence on session settings. This prevents them from being used in
expressions that require immutability, such as expression indexes,
generated columns, and partition keys.
Do not backpatch, since this changes catalog entries and can break
existing schemas that rely on these output functions being immutable.
Bump catalog version.
Reported-by: 放空 <jj-zhang25@mails.tsinghua.edu.cn>
Author: Fujii Masao <masao.fujii@gmail.com>
Reviewed-by: Peter Eisentraut <peter@eisentraut.org>
Discussion: https://postgr.es/m/19644-d6db3fb5e5ad92f4@postgresql.org
This function attribute tells the compiler that the function will
never return NULL. This allows the compiler to do better static
analysis and lets it optimize the caller given the non-NULL guarantee.
The attribute is placed in front of the declaration, rather than after
it, so that it can also expand to the MSVC annotation _Ret_notnull_,
which is only valid in that position.
Only functions that raise an error on allocation failure are marked.
Functions that take a flags argument, such as palloc_extended() and
palloc_mul_extended(), can return NULL when MCXT_ALLOC_NO_OOM is
passed and are therefore not marked.
Author: Tristan Partin <tristan@partin.io>
Discussion: https://www.postgresql.org/message-id/flat/DJROV5LUO7B5.DIWXYC0101KD%40partin.io
Fix some issues involving volatile that would be triggered by
-Wcast-qual.
For PMSignalState, move the volatile qualifiers to the PMSignalData
fields that need it rather than making the whole structure have it.
For the MemSet() calls, do the initialization honestly with a loop
instead.
Note that the MemSet() calls would be normal "discards qualifier"
warnings if memset() (or another function with a prototype, not a
macro) were used.
Reviewed-by: Rui Zhao <zhaorui126@gmail.com>
Reviewed-by: Andres Freund <andres@anarazel.de> (older version)
Discussion: https://www.postgresql.org/message-id/c76b72dd-5736-453a-b698-6968d42e4768%40eisentraut.org
8741e48e5 made EvalPlanQualStart() borrow the parent EState's
es_part_prune_states. However, the ExprContext in each
PartitionPruneState belongs to the parent EState, so exec pruning during
an EPQ recheck used the parent's PARAM_EXEC values, i.e. those from the
old row version. This could prune away the partition the recheck
needed, causing the row to be silently skipped, e.g. with a correlated
subquery on a partitioned table in the WHERE clause of an UPDATE.
Here we fix this by giving the EPQ EState its own PartitionPruneStates,
while still borrowing the parent's es_part_prune_results so that the
same subplans are initialized. As the states are no longer shared, the
"initialized" field added by 1b5dd3a24 is no longer needed, so remove
it. This also undoes the ABI break that field caused in 18.
Backpatch to 18, where 8741e48e5 and 1b5dd3a24 were backpatched.
Author: Zsolt Parragi <zsolt.parragi@percona.com>
Reviewed-by: David Rowley <dgrowleyml@gmail.com>
Discussion: https://postgr.es/m/CAN4CZFPZ6qKnv6FPmq1RQZ49cHALCwLBu_NJdqUrkZNg0z81JQ%40mail.gmail.com
Backpatch-through: 18
When constructing a historic snapshot for logical decoding,
reorderbuffer needs to keep track of movement of tuples in catalog
tables. For the most part this works correctly, except that when a
subtransaction that wrote a catalog tuple aborts, we failed to "forget"
about that change. There's no known reports of anything actually
breaking except assertion failures, but it is likely possible to
construct cases where historical snapshots produce bogus visibility
answers, potentially causing incorrect decoding output.
To fix, have the decode of a subtransaction abort go over the TUPLECID
list that has been accumulated on the ReorderBufferTXN for its topmost
parent transaction, and remove all the entries that were marked as
originating from the aborted subtransaction or its children. There are
some little details in which various devils lie, which the added code
comments explain (we hope).
Add a couple of isolation test cases that trip the assertions with the
original coding. They're not ideal test design, because they pass in
non-assert builds without the fix. But we'd rather have the fix be
verified in that limited scenario than nowhere at all, and they are
cheap enough.
This bug is sibling to the one fixed in commit 8c67d29fd5 and, like
that one, it goes all the way back to the introduction of
reorderbuffers, so backpatch to all live branches.
While reviewing this, I (Álvaro) was bothered by the snapmgr.c
tuplecid_data file-scoped variable, as it's easily confused with a
function parameter used widely in reorderbuffer.c. In branches 19 and
master, rename that to avoid confusion.
Reported-by: Arseny Sher <a.sher@postgrespro.ru>
Reported-by: Mark Dilger <mark.dilger@enterprisedb.com>
Reported-by: Alexander Kozhemyakin <a.kozhemyakin@postgrespro.ru>
Author: Bingshuai Li <lucian1412@outlook.com>
Author: Hayato Kuroda <kuroda.hayato@fujitsu.com>
Author: Zhijie Hou <houzhijie22@gmail.com>
Backpatch-through: 14
Bug: #19555
Discussion: https://postgr.es/m/87r2c8xocq.fsf@ars-thinkpad
Discussion: https://postgr.es/m/CAHgHdKu5e3XY5e90Tuaxq_R4WrKxSV734Q%2BLwo5y39Omp2A-Gg@mail.gmail.com
Discussion: https://postgr.es/m/19555-3c700f40a13045cd@postgresql.org
The documentation described base backup progress in terms of data
streamed, which could be mistaken for the amount of data sent over the
connection when server-side compression is enabled. Clarify that the
counters measure archive data before server-side compression.
The progress report message was also documented as counting bytes from
the current tablespace, although it reports a cumulative count for the
whole backup. Correct the description to say that the count covers all
tablespaces since the backup began.
Backpatch to 15, where both the progress report message and server-side
compression were introduced.
Suggested-by: Surya Poondla <suryapoondla4@gmail.com>
Author: Daria Lepikhova <daria.n.lepikhova@gmail.com>
Reviewed-by: Surya Poondla <suryapoondla4@gmail.com>
Reviewed-by: Fujii Masao <masao.fujii@gmail.com>
Discussion: https://postgr.es/m/CAOVWO5pNe6XprMnt5OTT+-2Sm67z5GX4DzUNArEHubF0z9-PZQ@mail.gmail.com
Backpatch-through: 15
When pull_up_sublinks() converts a sublink to an antijoin, it builds a
JoinExpr with no RTE, on the grounds that nothing above the join can
reference its RHS. That overlooks PlaceHolderVars. A PHV with no
Vars of its own is evaluated at its phrels, and for the outputs of a
pulled-up subquery phrels is all the relids in the subquery's
jointree, including the RHS of such a join.
A qual using the PHV thus appears to reference the RHS rels, and can
be pushed down into the RHS as a parameterized join clause. There it
has the opposite of the intended effect: outer rows that fail the qual
find no match and are emitted, giving wrong results. For an outer
join, including an antijoin made by reducing a left join, this cannot
happen because the join's relid is among the qual's relids. Before
v16, the qual's nullable_relids prevented it.
To fix, make a join RTE for an antijoin built from a sublink, so that
it has a relid like any other outer join. Semijoins are left as they
are: a qual evaluated within a semijoin's RHS does not change the
result.
Back-patch to v16. Before that, the qual's nullable_relids included
the antijoin's RHS, which kept the qual from being moved there; that
protection went away when nullable_relids was replaced by outer-join
relids.
Author: Richard Guo <guofenglinux@gmail.com>
Reviewed-by: Tom Lane <tgl@sss.pgh.pa.us>
Reviewed-by: wenhui qiu <qiuwenhuifx@gmail.com>
Discussion: https://postgr.es/m/CAMbWs4_rEdcG89BgC-AO7b+Bf2z8w3976+4zP==Urt=CXB1hsg@mail.gmail.com
Backpatch-through: 16
Variable-numbered pgstats entries currently all live in one shared dshash
table. This works for stats kinds with each one having a rather "small"
number of entries, but there are limits if a stats kind wants to
allocate a large number of entries, especially if these need to be
scanned on a periodic basis.
This commit adds a new own_hash option for variable-numbered stats
kinds, allowing these to use an independent dshash table backed by its
own DSA area. This gives a way for stats kind to leverage their efforts
across more than one shared hashtable, splitting the load, with the same
guarantees as the main hashtable. Fixed-sized stats kind cannot use
this option. All the hashtables use the same key pattern,
PgStat_HashKey.
The motivation of this change is pg_stat_statements, to add it as a
custom stats kind. Out-of-core custom stats kind, or even built-in
stats kinds can also use this option.
At startup, each stats kind that opts in creates one initial DSA area in
shared memory at startup. At attach time, each registered
variable-numbered stats kind is mapped to either the shared hash and DSA
or its dedicated hash and DSA. The local references are extended to
track which hash table to use on a per-kind basis, with arrays based on
the kind ID.
The test module test_custom_stats is extended and refactored to provide
coverage for this new own_hash.
The idea of being able to use more than one hash table for pgstats has
been originally mentioned off-list during a conference by Robert Haas
(note: if my memory does not fail me here).
Author: Sami Imseih <samimseih@gmail.com>
Reviewed-by: Michael Paquier <michael@paquier.xyz>
Reviewed-by: Bertrand Drouvot <bertranddrouvot.pg@gmail.com>
Discussion: https://postgr.es/m/CAA5RZ0supQBxSkh=CWB39=j+cL3hHcLPki3tcBk0B1r4fesg_g@mail.gmail.com
1eff8279d did this for the leader, but neglected to consider that the
Material node may be below a Gather or Gather Merge and have parallel
workers also participate in the execution of the Material node.
Here we add EXPLAIN ANALYZE output to show Maximum Storage per worker
and all the required shared memory related code to get those details
back to the leader from each worker.
Because the prior code didn't have parallel worker processes report the
tuplestore usage details, this meant that the EXPLAIN ANALYZE output
didn't show the Maximum Storage when running debug_parallel_query =
regress. You could argue that fixing that should be a bug fix, but
fixing it would start reporting the Maximum Storage for Material nodes
for parallel workers during parallel queries, which feels more like a
new feature than a bug fix. The telemetery data going missing with
debug_parallel_query = regress isn't the end of the world, so let's just
fix this in master.
Author: David Rowley <dgrowleyml@gmail.com>
Reviewed-by: Tatsuya Kawata <kawatatatsuya0913@gmail.com>
Reviewed-by: Chao Li <li.evan.chao@gmail.com>
Discussion: https://postgr.es/m/CAApHDvoUWCms-b27MpDKd5_RUFtAnVrxPgN9u3QoQbBihk+S6g@mail.gmail.com
Previously, this returned a pointer to the string constant "Disk" or
"Memory" depending on whether the tuplestore spilled to disk or not. An
upcoming patch adds additional EXPLAIN ANALYZE output for Material nodes
for parallel workers. The API pointing to the string constant is not
great when we must transfer the storage method to the parallel leader, as
we cannot simply point to the string constant in the other process.
That's going to be mapped to a different memory address.
Here we adjust the tuplestore_get_stats() API to set "*used_disk" to
true when the tuplestore spilled to disk. This allows us to avoid a
pretty horrible: if (strcmp(..., "Disk") == 0) in the code to transfer
the information to the parallel leader.
Author: David Rowley <dgrowleyml@gmail.com>
Reviewed-by: Tatsuya Kawata <kawatatatsuya0913@gmail.com>
Reviewed-by: Chao Li <li.evan.chao@gmail.com>
Discussion: https://postgr.es/m/CAApHDvoUWCms-b27MpDKd5_RUFtAnVrxPgN9u3QoQbBihk+S6g@mail.gmail.com
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
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
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
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
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
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
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
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
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
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
Clearing the tuplestore rather than ending it avoids the need for
various memory allocations, so is slightly more efficient. However, the
main reason to do this is to correctly track the maximum storage used by
the Table Function Scan node. 40708acd65 added additional EXPLAIN
output for Table Function Scan nodes and that output does claim to be
showing "Maximum Storage", which is not true, as the maximums could be
lost after tuplestore_end() is called during a rescan.
This causes misreporting when the final rescan of a Table Function Scan
node uses less storage than some previous rescan. One example is:
EXPLAIN ANALYZE
SELECT count(*)
FROM (VALUES ((SELECT jsonb_agg(g) FROM generate_series(1, 100000) g)),
('[1]'::jsonb)) v(j),
LATERAL JSON_TABLE(v.j, '$[*]' COLUMNS (x int PATH '$')) jt;
Without this fix, the reported Table Function Scan storage is for the
'[1]'::jsonb case, whereas it should consider how much was used with the
larger document and show the maximum of all rescans.
TableFuncNext() used a NULL tupstore to tell that the rows still had to
be fetched, so add a tupstore_filled flag for that, similar to what
908a96861 did for WindowAgg.
Technically, this issue does exist in v18, but no backpatch as this is a
fairly minor issue which was only noticed during a review of a similar
feature being proposed for Function Scans.
Author: Tatsuya Kawata <kawatatatsuya0913@gmail.com>
Reviewed-by: David Rowley <dgrowleyml@gmail.com>
Discussion: https://postgr.es/m/CAHza6qdXjcqXv2aGU7OCakG0OmEsy9-jfP0Gpe2EL+k-AidOhw@mail.gmail.com
In clang, -fsanitize=undefined includes -fsanitize=function, which
reports every call made through a function pointer whose type does not
exactly match the called function, so it fires all over the place on
expression tree walkers and mutators, as well as a few other places.
So -fsanitize=undefined hasn't been working cleanly under clang for a
while. (Before clang 17, it only applied to C++.)
This is the same issue that caused us to use
-Wno-cast-function-type-strict with clang. That warning applies at
the place where the mismatching function pointer is passed, so there
are potentially hundreds of sites. Therefore, a global disabling is
appropriate. The sanitizer, on the other hand, triggers where the
function is called, which are only about two dozen places, so it seems
possible to silence these checks individually and still main the check
for accidental violations elsewhere.
Add pg_attribute_no_sanitize_function() and place it on the functions
that make such calls.
Reviewed-by: Chao Li <li.evan.chao@gmail.com>
Discussion: https://www.postgresql.org/message-id/flat/2db401d9-ef16-430e-a42c-34477006a49f%40eisentraut.org
MSVC has no counted_by attribute, but its source annotation
language (SAL) has _Field_size_(), which states the same property: the
field is a buffer whose writable size in elements is given by the
named expression.
MSVC's static analyzer (/analyze) will make use of the information.
Author: Tristan Partin <tristan@partin.io>
Discussion: https://www.postgresql.org/message-id/flat/DKBEBF32QLUA.3L2DJRPUYNKHZ%40partin.io
This removes the buffering of FK rows for batched PK index probes,
along with the after-trigger callback infrastructure used only by
batching. The per-row fast path, which probes the PK index directly
instead of going through SPI, remains. This is a cherry-pick of
commit 25649d6e79, which made the same change in REL_19_STABLE,
adjusted for some drift between the branches.
I (amitlan) initially thought we could keep improving the batching
path on master by fixing its remaining bugs and other issues as
they were found. However, we are still finding cases where delaying
checks to accumulate a batch changes their results, such as when other
AFTER ROW triggers modify the referenced table. Individual cases can
be addressed, but new ones keep turning up, which makes it hard to be
confident that fixing the known ones will be enough. It seems better
to rip batching out and reconsider how to introduce it, or at least
its useful pieces, while preserving the semantics of per-row checks.
Discussion: https://postgr.es/m/CA+HiwqFx4cjFtC3akr1DW-cPYw+B-EkCSffZPrkpTef8PNS6qA@mail.gmail.com
When the index cannot be used to find a recently deleted row,
RelationFindDeletedTupleInfoSeq() scans the table. It looked up the
relation's replica identity or primary key again to determine which
columns to compare, which could differ from the index selected when the
relation was opened. A concurrent DROP INDEX CONCURRENTLY could remove the
replica identity in between, so the search compared the whole row and
reported the conflict as update_deleted instead of update_missing.
Use the index recorded in the relation map instead. The relation map
already records the replica identity index, or the primary key if there is
no replica identity. It also avoids using a deferrable primary key, which
cannot serve as a replica identity; using its columns could incorrectly
report a deleted row with different values as update_deleted instead of
update_missing.
Author: Hayato Kuroda <kuroda.hayato@fujitsu.com>
Author: Nisha Moond <nisha.moond412@gmail.com>
Reviewed-by: Vignesh C <vignesh21@gmail.com>
Reviewed-by: Zhijie Hou <houzhijie22@gmail.com>
Reviewed-by: Amit Kapila <amit.kapila16@gmail.com>
Discussion: https://postgr.es/m/CABdArM5ydwdRrpaZyK1q2p3-vY_+pnBtTmkvg_pcM=gHwmH7Kg@mail.gmail.com
Discussion: https://postgr.es/m/OS7PR01MB1831779ED93F5470CA1D57605F58C2@OS7PR01MB18317.jpnprd01.prod.outlook.com
Backpatch-through: 19
pg_stat_progress_vacuum gains three fields, reflecting some activity
related to index cleanups during VACUUM, which is a phase that could
take a long time depending on the quantity of data to treat and the
number of indexes:
- current_index_relid, the index being processed.
- index_blks_total, total number of blocks to process.
- index_blks_done, number of blocks already processed.
There fields are new, and do not reuse the existing fields related to
heap as index cleanups may need multiple passes, where knowing both the
heap activity and the index activity could be relevant. This is
unlikely since tids are accumulated in the heap scanning phase using an
optimized radix tree, still possible. VACUUM can spawn parallel workers
to clean indexes (each worker cleans a different index); these should up
in the progress view in the shape of one extra row for each worker.
Bump catalog version.
Author: Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com>
Reviewed-by: Sami Imseih <samimseih@gmail.com>
Reviewed-by: Michael Paquier <michael@paquier.xyz>
Reviewed-by: Masahiko Sawada <sawada.mshk@gmail.com>
Reviewed-by: Shihao Zhong <zhong950419@gmail.com>
Discussion: https://postgr.es/m/CALj2ACUgwSchK6jQ2CdKLBWUADTOE_zKdTff2Zg3E6hOuXKv-w@mail.gmail.com
Commit 1ff3180ca0 made a parallel autovacuum leader propagate changes
to the cost-based delay parameters to its workers only from
vacuum_delay_point(), which it no longer reaches once it is waiting in
WaitForParallelWorkersToFinish(). While waiting, the leader neither
processed a config reload nor noticed a change in the number of
autovacuum workers sharing the cost limit, so its workers kept running
with stale parameters until they finished.
Fix by refreshing and propagating the parameters on every wakeup of
that wait loop, and by setting the latches of the balanced autovacuum
workers when their count changes so that a waiting leader wakes up.
A regression test will be added in a separate commit.
Backpatch to v19, where parallel autovacuum was introduced.
Reported-by: Nikolay Samokhvalov <nik@postgres.ai>
Author: Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com>
Reviewed-by: Manu <manuelreyesbravo@gmail.com>
Reviewed-by: Zsolt Parragi <zsolt.parragi@percona.com>
Reviewed-by: Daniel Gustafsson <daniel@yesql.se>
Reviewed-by: Masahiko Sawada <sawada.mshk@gmail.com>
Discussion: https://postgr.es/m/CAM527d-GL=Jp2EJXBSnVBGPK-4XEZwWof5Cv8P0hghS_og6oAg@mail.gmail.com
Backpatch-through: 19
Presently, s_lock.h insists that every platform provide TAS(), and
its header comment doesn't say which of the macros a new platform
actually needs to supply. The real contract is that a platform
must provide either S_LOCK() or a TAS() for the default S_LOCK() to
be built on.
To better express that, this commit moves the "no spinlock support"
error to where the default S_LOCK() is defined, compiles s_lock()
only when the default S_LOCK() is in use, and provides a default
TAS_SPIN() only when there is a TAS() to base it on. The direct
callers of s_lock() in the spinlock tests are adjusted to match.
The header comment is reworked accordingly, dropping the note about
pre-9.5 volatile requirements and the caution about TAS()
spuriously failing along the way.
Co-authored-by: Tom Lane <tgl@sss.pgh.pa.us>
Discussion: https://postgr.es/m/afkUeI7UhacZ5ZFm%40nathan
When searching the local relation by index, the apply worker can use the
first match only if the index is the relation's replica identity or
primary key. Otherwise, every match must be compared with the search slot.
Only the index OID was saved in the relation map, so this was determined
again during the search. Concurrent DROP INDEX or REINDEX can change the
index status between these two checks, causing the tuple search to fail
and the change to be silently skipped as an update_missing conflict.
Fix this by recording the answer as idxisreplident in the relation map
entry and avoid getting it second time in FindReplTupleInLocalRel() and
RelationFindReplTupleByIndex().
Oversight in 89e46da5e5.
Author: Mikhail Nikalayeu <mihailnikalayeu@gmail.com>
Author: vignesh C <vignesh21@gmail.com>
Reviewed-by: Amit Kapila <amit.kapila16@gmail.com>
Reviewed-by: Zhijie Hou <houzj.fnst@fujitsu.com>
Discussion: https://postgr.es/m/CADzfLwUJovFcnknCC9wjZKECX9xecgnGzC2r2TMV8h4QDD_jwQ@mail.gmail.com
Backpatch-through: 16, where it was introduced
Since 112faf1378, NOTICE, WARNING, and similar messages received through
dblink, postgres_fdw, and replication connections have been logged with
ereport(LOG). With the default log_min_error_statement setting, each
such log entry can also include the current local SQL statement. An active
error-context callback can add local CONTEXT information as well. Repeating
these fields for every remote message increases log volume and processing
overhead.
Use errhidestmt() and errhidecontext() when logging these messages to
prevent the local STATEMENT and CONTEXT fields, when present, from being
repeated for each remote message in the server log.
Reported-by: Merlin Moncure <mmoncure@gmail.com>
Author: Vignesh C <vignesh21@gmail.com
Reviewed-by: Merlin Moncure <mmoncure@gmail.com>
Reviewed-by: Fujii Masao <masao.fujii@gmail.com>
Discussion: https://postgr.es/m/CAHyXU0yDwOYkWpRi=gtpDwjBxVqeaezgkgZLRWm2p1_FgaiPFQ@mail.gmail.com
Backpatch-though: 19
The counted_by attribute allows specifying that a flexible array
member of a struct is "counted by" another member of the same struct.
The attribute makes arrays a little more self-documenting, but it is
also a hint to the compiler such that it can improve detection of
object size information and provide better results in compile-time
diagnostics and run-time features, such as the array bounds sanitizer.
Both GCC and clang warn when the attribute is used in C++, where it is
ignored, so define it away there.
In its current form, it is best used for flexible array members only.
Pointer array support may come later on.
This commit only contains a single use of the attribute for testing.
More will be added afterwards.
Author: Tristan Partin <tristan@partin.io>
Reviewed-by: Peter Eisentraut <peter@eisentraut.org>
Reviewed-by: Japin Li <japinli@hotmail.com>
Reviewed-by: Greg Burd <greg@burd.me>
Discussion: https://www.postgresql.org/message-id/flat/DKBEBF32QLUA.3L2DJRPUYNKHZ%40partin.io
This marker could break translated strings if used, so let's remove it
and encourage instead the use of PRIu64, something that xgettext is able
to digest.
This marker has been introduced in b139bd3b6e, down to v19 that has
not been officially released yet, so a removal is still possible without
breaking the ABI requirements.
Discussion: https://postgr.es/m/aqeO1OU7uZjhioAq@alvherre.pgsql
Backpatch-through: 19
pg_restore_attribute_stats() rejected range_length_histogram,
range_empty_frac and range_bounds_histogram for a column whose type is a
domain over a range or a multirange type. The same issue existed for
most_common_elems and elem_count_histogram for a domain over tsvector.
pg_restore_extended_stats() had the same set of problems for expressions
whose type is a domain over a range, a multirange type, or tsvector.
ANALYZE is able to generate such stats, incorporating the knowledge to
handle domains in the in-core typanalyze callbacks.
This problem is resolved by being more aggressive with the fetch of the
base type of a domain in the early phases of restore for attribute and
extended stats (special tip to Jian He for pointing out the unnecessary
typcache lookups), reflecting on the surrounding helper routines shared
by both code paths. The fix for v18 is local to attribute_stats.c, as
only attribute stats need to be touched.
Tests are included in a fashion consistent with the surroundings. The
consequence of this issue was the rejection of stats that ANALYZE was
able to build, which was not critical but annoying as it would lead to a
gap in the stats restored.
Reported-by: Qifan Liu <imchifan@163.com>
Author: Michael Paquier <michael@paquier.xyz>
Reviewed-by: Jian He <jian.universality@gmail.com>
Reviewed-by: Corey Huinker <corey.huinker@gmail.com>
Discussion: https://postgr.es/m/19715-b8be35083016f289@postgresql.org
Backpatch-through: 18
Variable-length catalog columns have been declared like
text attoptions[1];
but that "1" has always been a fiction. Before the use of #ifdef
CATALOG_VARLEN, these declarations were visible to the C compiler, and
this was also before flexible array members were universally
available, so this was just a convenient workaround to make this
compile. But these reasons are long gone, and the "1" is now just a
confusing relic. Change this to
text attoptions[];
which more intuitively reflects the actual nature of these
fields (while still being syntactically valid but semantically invalid
C code).
Catalog.pm could already parse both spellings, but no existing code
used bare []. To enforce future consistency, it is changed to no
longer permit digits between the brackets. But to help developers
transition, it still recognizes the old style and gives an explicit
error message. (Otherwise it would produce some garbage output and
give confusing errors later.)
Reviewed-by: Heikki Linnakangas <hlinnaka@iki.fi>
Reviewed-by: Daniel Gustafsson <daniel@yesql.se>
Reviewed-by: Chao Li <li.evan.chao@gmail.com>
Discussion: https://www.postgresql.org/message-id/flat/963b2109-833c-4e93-bdb1-546741d893f4%40eisentraut.org
toast_internals.h described the header of a compressed-in-line varlena
a second time, as toast_compress_header, with its own TOAST_COMPRESS_*
macros to read and write the tcinfo word. varatt.h already describes
the same bytes as varattrib_4b.va_compressed and provides equivalent
macros that act as setter and getter of this information, as of
VARDATA_COMPRESSED_GET_EXTSIZE() and GET_COMPRESS_METHOD().
Let's remove the duplicate definition, and switch the TOAST code to use
the macros from varatt.h. There should be no behavior change.
Note that toast_compression.h is removed from toast_internals.h, added
to the C files where its declarations are required.
Author: Nikhil Kumar Veldanda <veldanda.nikhilkumar17@gmail.com>
Author: Michael Paquier <michael@paquier.xyz>
Discussion: https://postgr.es/m/CAFAfj_FeJQZrKyVn7ur0aJx8iz8SR14NheVJNyc8JNoqtMX+og@mail.gmail.com
Autovacuum doesn't necessarily fully empty the GIN pending list. It
stops at the tail as of the start of cleanup, so that it doesn't keep
chasing pages appended by concurrent inserters. GIN decided this with
AmAutoVacuumWorkerProcess(), which returns false in parallel vacuum
workers launched by an autovacuum worker, so those workers tried to
fully empty the list as a manual VACUUM does.
Fix this by adding an is_autovacuum field to IndexVacuumInfo, set from
the shared parallel vacuum state in parallel workers, and using it in
GIN instead of checking the process type.
Oversight in commit 1ff3180ca0.
Reported-by: Jingtang Zhang <mrdrivingduck@gmail.com>
Author: Jingtang Zhang <mrdrivingduck@gmail.com>
Reviewed-by: Andrey Borodin <x4mmm@yandex-team.ru>
Reviewed-by: Yuhang Qiu <iamqyh@gmail.com>
Reviewed-by: Masahiko Sawada <sawada.mshk@gmail.com>
Discussion: https://postgr.es/m/CAPsk3_DiF2i1o=JRXRk1YPVMv1VzJaCAbfiEqgwCva4=ZewVAw@mail.gmail.com
Backpatch-through: 19
8191e0c16a made pgstat_init_entry() return NULL on DSA allocation
failure so callers could delete a just-inserted hash entry. That works
when dsa_allocate_extended(DSA_ALLOC_NO_OOM) returns InvalidDsaPointer,
but not when the creation of a new DSM segment raises ERROR.
Under this type of failure, pgstat_init_entry() would mark the hash
entry live before allocating the body and cause an inconsistent state in
shared memory. The next backend trying to acquire the existing,
partially-initialized entry in pgstat_acquire_entry_ref() would crash,
taking down the server. Some of the public reports mentioned ENOSPC on
POSIX shared memory as one initial cause.
This commit refactors the pgstats entry initialization to happen in two
phases:
- Allocate the DSA chunk.
- Insert the hash entry with a valid segment.
If a concurrent insert wins, the allocation of the first phase needs an
extra set of dsa_free() calls (pulling back a DSM into its free list).
As this changes the signature of pgstat_init_entry(), no backpatch is
done.
Author: Yuriy Grigoryev <ju.grigorev@ftdata.ru>
Author: Michael Paquier <michael@paquier.xyz>
Discussion: https://postgr.es/m/ddc3ecfb01ce4e9698b23cc59767f016@localhost.localdomain
Commit bad8829872 added an alignas specifier, and placed it in the
middle of the declaration, unlike most existing uses, possibly to
avoid the indentation problem recently fixed in commit 884f82ae26.
This placement is valid in C but not in C++, which requires it to be
at the beginning. This results in a warning in g++ and an error in
clang++. Fix by moving it accordingly.
Currently, this variable is an int protected by a spinlock. By
converting it to an atomic variable, we can remove the spinlock.
Detaching becomes an atomic subtract, and attaching becomes a
compare-and-exchange loop, since it must not resurrect a fileset
whose reference count has already reached zero.
Reviewed-by: Zsolt Parragi <zsolt.parragi@percona.com>
Tested-by: solai v <solai.cdac@gmail.com>
Discussion: https://postgr.es/m/alAJeRRzehDjLaF1%40nathan
Currently, this variable is a Buffer that is accessed via a
volatile pointer. By converting it to an atomic variable, we can
remove the volatile qualifiers. No barriers are needed because the
value is published before the backends that read it are signaled.
Reviewed-by: Peter Eisentraut <peter@eisentraut.org>
Reviewed-by: Andres Freund <andres@anarazel.de>
Reviewed-by: Zsolt Parragi <zsolt.parragi@percona.com>
Tested-by: solai v <solai.cdac@gmail.com>
Discussion: https://postgr.es/m/alAJeRRzehDjLaF1%40nathan
