Commit 767c59895 made set_append_rel_size() skip join quals that came
from above an outer join that can null the parent rel. Such quals are
of no use while generating paths for the child, and skipping them also
keeps them away from adjust_appendrel_attrs(), which cannot apply
nullingrels to a non-Var translation.
However, it identified such quals by looking at their clause_relids.
That misses a reference to the parent rel that is nulled by such an
outer join but appears only within a PlaceHolderVar, because
pull_varnos() reports only where the PHV is evaluated. Such a qual
could then reach adjust_appendrel_attrs(), which failed with "failed
to apply nullingrels to a non-Var" if the referenced output of a UNION
ALL member was not a plain Var.
To fix, also skip quals whose clause_relids overlap the parent rel's
lateral_referencers. A reference to the parent rel within a PHV that
is evaluated elsewhere is necessarily a lateral reference, so this
catches all such quals. These quals are no use for the child either,
since they can never be enforced by a scan of the parent rel; this is
the same reasoning join_clause_is_movable_to() uses.
Back-patch to v16, where commit 767c59895 was introduced.
Author: Richard Guo <guofenglinux@gmail.com>
Reviewed-by: wenhui qiu <qiuwenhuifx@gmail.com>
Discussion: https://postgr.es/m/CAMbWs4_fzrrN9G6-wLwwrdM-n=fwj3Uhr1zkgj3o91EEC-CuEA@mail.gmail.com
Backpatch-through: 16
Previously, REPACK and CLUSTER silently skipped foreign-table partitions
when processing a partitioned table, leaving users unaware that some
partitions had not been processed. In contrast, VACUUM FULL warns when
skipping such partitions.
Emit a warning for each skipped foreign-table partition. Inspect the
table's partition tree even when an index is specified, because foreign
tables have no indexes and therefore do not appear in the index's
partition tree. Check privileges before issuing the warning, so that
users without the required privileges receive a permission warning
instead, as with VACUUM FULL.
Note that database-wide REPACK and CLUSTER continue to skip foreign
tables silently, consistent with database-wide VACUUM FULL.
Author: Zsolt Parragi <zsolt.parragi@percona.com>
Reviewed-by: Antonin Houska <ah@cybertec.at>
Reviewed-by: Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com>
Reviewed-by: shihao zhong <zhong950419@gmail.com>
Reviewed-by: Surya Poondla <suryapoondla4@gmail.com>
Reviewed-by: solai v <solai.cdac@gmail.com>
Reviewed-by: Fujii Masao <masao.fujii@gmail.com>
Discussion: https://postgr.es/m/CAN4CZFNXFdkmOCnt8m=K_A4i3LQC+59nx59Y=5bsH4vehQHTTw@mail.gmail.com
Buildfarm member skink reported failures in
053_standby_login_event_trigger.pl. The checks that dathasloginevt was
cleared on the primary and that the cleared flag reached the standby
both found the flag still set.
The test assumed that the first login after dropping the trigger would
clear the flag. However, EventTriggerOnLogin() acquires the database
lock conditionally to avoid delaying connections for this cleanup.
Concurrent connection startup, including that of an autovacuum worker,
can hold a conflicting lock and cause the cleanup to be skipped. The
standby correctly retained the flag because it had not been cleared
on the primary.
Fix this by using poll_query_until() to retry with fresh connections
to the test database until the flag is cleared, giving the login cleanup
repeated opportunities to acquire the lock.
Backpatch to v17, like commit bec61f5935, which added this test.
Reported-by: Alexander Lakhin <exclusion@gmail.com>
Author: Ayush Tiwari <ayushtiwari.slg01@gmail.com>
Reviewed-by: Fujii Masao <masao.fujii@gmail.com>
Discussion: https://postgr.es/m/09561429-50c1-4d06-a262-300c7a845175@gmail.com
Backpatch-through: 17
Commit 67c20979ce made end-of-recovery processing emit a process signal
barrier and wait for all processes to refresh their cached logical WAL
logging state. This can hold up promotion indefinitely if a backend
cannot process the barrier, for example while blocked sending data to
a client that has stopped reading.
At wal_level=logical, logical WAL logging is enabled regardless of the
cached state, so no cache synchronization is needed. Skip emitting and
waiting for the barrier in this case.
When logical decoding will be disabled after recovery, stale caches
can only cause extra logical information to be written to WAL, which is
safe. Still emit the barrier so processes update their caches
asynchronously, but no longer wait for it to complete.
These changes allow promotion to finish without waiting for unresponsive
backends to process this barrier in either case. Retain the wait in
other cases, even if the shared state has not changed, to prevent
processes with stale caches from omitting required logical information
after recovery.
Reported-by: shihao zhong <zhong950419@gmail.com>
Author: Fujii Masao <masao.fujii@gmail.com>
Author: Manuel Reyes Bravo <manuelreyesbravo@gmail.com>
Reviewed-by: Masahiko Sawada <sawada.mshk@gmail.com>
Discussion: https://postgr.es/m/CAGRkXqToBk9c2sPAgha-kCgxJhVwn90j1sCyUXrvydq51LKAqA@mail.gmail.com
Backpatch-through: 19
On v18 and below, if the newest multixid's members end exactly at the
the 32-bit offset wraparound point, the next offset in the control
file is 0. GetOldMultiXactIdSingleMember() treated that as an invalid
entry and dropped the multixid's members, so after upgrade accessing
the tuple failed with "MultiXact N has invalid offset".
To fix, only check for zero offset when it was read from the next
multixid's slot, like GetMultiXactIdMembers() did in v18. This is
unlikely to happen in practice, but better to get it right.
The bug came with commit bd8d9c9bdf in v19.
Author: Zsolt Parragi <zsolt.parragi@percona.com>
Discussion: https://www.postgresql.org/message-id/CAN4CZFM8BCazGz6s1Vn5tMEXYiXE2fi8T2rwmwXSty5gD+h9rA@mail.gmail.com
Backpatch-through: 19
We must tell init about each role name we plan to connect as,
else SSPI auth fails. Similar to previous patches such as
e3fbc9a8d.
Oversight in a39e53a70, per buildfarm member unicorn.
The documentation stated that existing connections would continue to
function normally after the OOM killer terminated the postmaster.
In fact, those connections eventually terminate when server processes
detect the postmaster's death.
Clarify that the OOM killer can terminate either the postmaster or a
child process. If the postmaster is killed, the server must be restarted.
If a child process is killed, PostgreSQL typically terminates the
remaining connections and either recovers automatically or shuts down,
depending on restart_after_crash.
Backpatch to all supported versions.
Author: Takeshi Ideriha <iderihatakeshi@gmail.com>
Reviewed-by: Manuel Reyes Bravo <manuelreyesbravo@gmail.com>
Reviewed-by: Fujii Masao <masao.fujii@gmail.com>
Discussion: https://postgr.es/m/ce269321-a7f4-48f4-a9ca-a4a4983b9fd6@gmail.com
Backpatch-through: 14
The description of autovacuum_max_parallel_workers did not specify
where it can be set or how it relates to the per-table
autovacuum_parallel_workers storage parameter.
Clarify that it can be set only in postgresql.conf or on the server
command line, and that the limit can be reduced for individual tables.
Add a link to the autovacuum_parallel_workers storage parameter.
Backpatch to v19, where parallel autovacuum was introduced.
Author: Baji Shaik <baji.pgdev@gmail.com>
Reviewed-by: Sami Imseih <samimseih.pg@gmail.com>
Reviewed-by: Chao Li <li.evan.chao@gmail.com>
Reviewed-by: Fujii Masao <masao.fujii@gmail.com>
Discussion: https://postgr.es/m/CA+fm-RMmbUUPWJShxkhoB7SeiQ7XWw3YsVnHpMzp1Y2i_gtwig@mail.gmail.com
Backpatch-through: 19
All other callers of sentinel_ok() pass the palloc'd address to the
function and the requested size of the allocation as the 2nd parameter.
Here we adjust 3 locations that were passing the MemoryChunk and adding on
sizeof(MemoryChunk) onto the byte count. Effectively, it's the same byte,
but a strange way to calculate it.
Author: David Rowley <dgrowleyml@gmail.com>
Reviewed-by: shihao zhong <zhong950419@gmail.com>
Discussion: https://postgr.es/m/CAApHDvq7HyLWX27XCSf5BmijeObV-QUF6CCbKAW00LxFPc71Ag@mail.gmail.com
find_var_for_subquery_tle() searches a subquery rel's targetlist for a
Var referencing a given subquery output column, and asserted that any
Var it finds there belongs to that rel. That does not hold for an
appendrel child. set_append_rel_size() builds the child's targetlist
by translating the parent's, and as noted there the result can contain
arbitrary expressions. In particular, when a member of a LATERAL UNION
ALL subquery is pulled up and its output is a lateral reference, the
translated expression is a Var of the laterally referenced rel. If the
child is itself a subquery with sorted paths, convert_subquery_pathkeys()
would trip the Assert.
In non-assert builds, the function went on to compare such a Var's
varattno with the subquery column's resno, and so could match a Var of
an unrelated rel.
To fix, ignore Vars that do not belong to the subquery rel, the same
way set_rel_width() already does. Also correct the adjacent comment,
which claimed that the only non-Var entries are placeholders.
Back-patch to all supported branches.
Author: Richard Guo <guofenglinux@gmail.com>
Reviewed-by: shihao zhong <zhong950419@gmail.com>
Discussion: https://postgr.es/m/CAMbWs48fFiPi7Q5yJCoM_nAs47zd5XgvPHd9nZyM_ewq+k_jpQ@mail.gmail.com
Backpatch-through: 14
Effectively an oversight in 29f6a959cf. The code went to the trouble of
doing set_sentinel() but no code ever checked it was still set during
BumpCheck(). Since bump context types are used in a handful of places,
it very much makes sense to backpatch this to where bump.c was added so we
can more easily discover any memory stomping bugs.
Reported-by: shihao zhong <zhong950419@gmail.com>
Author: David Rowley <dgrowleyml@gmail.com>
Discussion: https://postgr.es/m/CAGRkXqQxTCthDEnw_10OEGLVNMUeyE4So-K29jmn9nAkxtr1ow@mail.gmail.com
Backpatch-through: 17
0ca3b1697 added support for parallel TID Range Scans. The range of
blocks to scan is stored in the shared phs_startblock and phs_numblock
fields by the first participant to start the scan, but
table_block_parallelscan_reinitialize() didn't reset these, so every
rescan of the Gather reused the block range of the first execution.
This caused rows to be silently missed when a rescan used a different
TID range, e.g:
SELECT r.lo, (SELECT count(*) FROM t WHERE ctid >= (SELECT r.lo))
FROM (VALUES ('(30,0)'::tid), ('(10,0)'), ('(0,0)')) r(lo);
Here we fix this by resetting both fields in
table_block_parallelscan_reinitialize(). As a side effect, a rescanned
Parallel Seq Scan using synchronized scans now gets a new start
location, which is what a new scan would do anyway.
The existing rescan test didn't exercise this, so add one that does.
This was introduced in 0ca3b1697, so backpatch to v19.
Reported-by: Zsolt Parragi <zsolt.parragi@percona.com>
Author: Zsolt Parragi <zsolt.parragi@percona.com>
Author: David Rowley <dgrowleyml@gmail.com>
Reviewed-by: David Rowley <dgrowleyml@gmail.com>
Reviewed-by: shihao zhong <zhong950419@gmail.com>
Discussion: https://postgr.es/m/CAN4CZFOoyCQE483YVGg07_Vr2MjFCTiCLUHUAFCwKqJAE=PN6w@mail.gmail.com
Backpatch-through: 19
Callers of WALReadFromBuffers() read data from the WAL buffers
(currently only WAL senders). This commit makes now available the
number of pages read from the WAL uffers in pg_stat_io.hits under object
"wal" and context "normal".
A read from a WAL file is counted in pg_stat_io.reads and read_bytes.
This new information can be useful for the tuning of wal_buffers, where
the number of hits matches with the number of pages found as available
in the buffers.
Note that WALReadFromBuffers() may be called for an amount of data worth
less than one WAL page, and that a partial page read is counted as one
hit.
Author: Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com>
Reviewed-by: Michael Paquier <michael@paquier.xyz>
Discussion: https://postgr.es/m/CALj2ACVfF2Uj9NoFy-5m98HNtjHpuD17EDE9twVeJng-jTAe7A@mail.gmail.com
When a parallel vacuum failed, the leader's VacuumSharedCostBalance
and VacuumActiveNWorkers kept pointing into the parallel vacuum's DSM
segment, which is gone after the abort, until the next vacuum in the
same process reset them. Nothing in the current code reads them in the
meantime, but it's better not to leave them dangling. Reset them at
the end of vacuum(), as we already do at its start.
No backpatch, as this doesn't fix any live problem.
Author: Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com>
Reviewed-by: Masahiko Sawada <sawada.mshk@gmail.com>
Discussion: https://postgr.es/m/CALj2ACW3OY7Ex-2-aits7yhRciH20r12DHcCNf+exuweR1FcMw@mail.gmail.com
Commit 41d69e6dc added labels to psql's internally-generated queries,
which are sent to the server and possibly recorded in the server log.
In that commit we always attempted to translate the labels per psql's
NLS environment, but that is hazardous: non-ASCII labels might fail
to be translatable to the server's encoding, rendering some or all of
these operations unusable since the server will throw an error and
not execute the query. Hence, we need some gating checks to decide
whether it's safe to translate a label.
To allow that to be managed in just one place, invent a new function
addQueryComment() that is responsible for outputting these labels.
(Probably it should have been done that way to start with, but better
late than never.)
This commit just does the heavy lifting of creating that function and
fixing all the call sites. The actual choice of whether to translate
is stubbed out, so that this commit in itself changes no behavior.
Once we've settled on the correct gating checks to use, a much smaller
follow-on commit will install them.
Author: shihao zhong <zhong950419@gmail.com>
Reviewed-by: Fujii Masao <masao.fujii@gmail.com>
Reviewed-by: Tom Lane <tgl@sss.pgh.pa.us>
Discussion: https://postgr.es/m/CAGRkXqRv7yhBOhF0YvSr6ZB7rvsPCjnXSPj8=3bKcSJuYvNrBA@mail.gmail.com
Backpatch-through: 19
An autovacuum worker advertises no role in its PGPROC, but its
parallel workers advertise the bootstrap superuser. So DROP DATABASE
... FORCE run by a non-superuser with pg_signal_backend failed with
"permission denied to terminate process" while a parallel autovacuum
was running in the database, although the same user can terminate the
autovacuum worker itself, and DROP DATABASE without FORCE succeeds.
Fix by checking a parallel worker against its leader's role when the
leader advertises no role. The leader is in the same database and is
checked too, so this doesn't let anyone terminate a process they
couldn't already terminate through its leader.
Parallel workers launched by a background worker connected without a
user name have had the same problem in older branches, but we haven't
received any report of it, and the next minor release is the final one
for v14, leaving no chance to fix a mistake there. So backpatch only
to v19, where parallel autovacuum made the problem easy to hit. If we
get a report, we can backpatch it further.
Found by Bharath using AI assisted review.
Reported-by: Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com>
Author: Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com>
Reviewed-by: Masahiko Sawada <sawada.mshk@gmail.com>
Reviewed-by: shihao zhong <zhong950419@gmail.com>
Discussion: https://postgr.es/m/CALj2ACWK6PkOk5MTfibtJyTYpb=Aq_8=YaexnPKhOj11bOCROQ@mail.gmail.com
Backpatch-through: 19
In match_unique_clauses(), use equal() to compare complete restriction
clauses after substituting the relation ID, instead of comparing their
operands separately. This ensures that the operator and input
collation also match, preventing incorrect self-join elimination when
restrictions have identical operands but different semantics.
The former code apparently intended to support matching clauses to
commuted versions of those clauses, but the amount of complication
that added seems far out of proportion to the value, so just drop
that goal. (If we wanted to reinstate it, something based on
applying CommuteOpExpr or equivalent logic would be better.)
Author: Tender Wang <tndrwang@gmail.com>
Reviewed-by: Tom Lane <tgl@sss.pgh.pa.us>
Discussion: https://postgr.es/m/CAHewXNmwYrp0DTuisCyfXXjVa_irUdDFApUzLVDg3J4MAR6gEg@mail.gmail.com
Backpatch-through: 18
Backwards scans that reach a half-dead leftmost leaf page step over the
page without calling _bt_readpage. This happens whenever the most
recent _bt_lock_and_validate_left call returned a half-dead leftmost
page. An assertion within _bt_readnextpage failed to consider this,
causing it to fail spuriously.
To fix, make the assertion accept the case where currPage doesn't match
lastcurrblkno as long as blkno is P_NONE (it's always P_NONE when the
last leaf page examined by _bt_readnextpage is either the leftmost or
rightmost page).
Oversight in commit caca6d8d27.
Author: Peter Geoghegan <pg@bowt.ie>
Backpatch-through: 18
The libxml2 versions I tested all pass through non-ASCII data
in tag attributes; but it appears that late-model versions
will hexify that data too. Per buildfarm.
For log(base, +Infinity) where 0 < base < 1, the correct output
is -Infinity, not +Infinity as we had been giving.
log(1, x) is erroneous regardless of x. Formerly we returned
+Infinity when x is +Infinity, and threw a rather confusing
"division by zero" error for finite-positive x. Change both
cases to report "cannot take logarithm using base one".
While the cases with infinite inputs are clearly wrong, it's
not clear that people would appreciate such a behavior change
in stable branches, so fix master only.
Author: Chao Li <li.evan.chao@gmail.com>
Reviewed-by: Matthias van de Meent <boekewurm+postgres@gmail.com>
Reviewed-by: Tom Lane <tgl@sss.pgh.pa.us>
Discussion: https://postgr.es/m/08270B8B-634F-4381-9D6F-9BDB18DA82A0@gmail.com
The main goal of this patch is to get rid of our use of
xmlKeepBlanksDefault, which libxml2 has deprecated.
To do that, we want to use xmlParseInNodeContext instead of
xmlParseBalancedChunkMemory, so that we can pass an options
mask that includes XML_PARSE_NOBLANKS.
xmlParseInNodeContext wants an xmlNode rather than an xmlDoc as
context. This gives us an opportunity for simplification though:
we can make that node be a dummy root node for xml_parse's result
document, and then attach the content node(s) as children of the root,
essentially moving the data structure hack in xmltotext_with_options
into xml_parse. The main benefit of this is that callers no longer
need to free the content node list separately; freeing the document is
sufficient. (There were error paths in xmltotext_with_options that
I think would have leaked that list, though they could only be
reached in OOM or similar conditions.)
We also do some fooling with doc->encoding, to be bug-compatible
with our previous behavior on as many libxml2 versions as practical.
Some small discrepancies with previous behavior are known with older
libxml2 versions. Therefore, it seems inadvisable to back-patch this,
at least not before it's gotten a good deal of field experience.
Author: Tom Lane <tgl@sss.pgh.pa.us>
Fix-suggested-by: Andrew Dunstan <andrew@dunslane.net>
Reviewed-by: Rui Zhao <zhaorui126@gmail.com>
Discussion: https://postgr.es/m/ZR2PPFF1830B8772632FA23847D7482168FD2C62@ZR2PPFF1830B877.CHEP278.PROD.OUTLOOK.COM
START_TIMER launches a new thread, but never closed the handle to it.
We don't need the handle after starting the thread, so just close it
right away. The program creates a timer for each benchmark phase, so
this isn't a big leak at the moment, but it could become more serious
if someone added START_TIMER to a loop. I didn't bother backpatching
though.
Author: Harrison Booth <harrisontbooth@gmail.com>
Discussion: https://www.postgresql.org/message-id/CAAH-eSVFy2GySdYEAtd9Wp=Swka0TzZGkw2VrO8GTaq3ndcikQ@mail.gmail.com
As mentioned in the comments in TransactionIdDidAbort() and in
heapam_visibility.c, the usual procedure for checking whether a
transaction aborted is to first call TransactionIdIsInProgress(), and
then !TransactionIdDidCommit(). A couple places didn't get the memo,
and used TransactionIdDidAbort() incorrectly.
In DoesMultiXactIdConflict(), the consequence was that if a
conflicting transaction was just aborting, and had already marked the
transaction as aborted in pg_xact, but had not yet removed itself from
ProcArray, the function would return false meaning that it does not
conflict, even though it was still considered as running by
TransactionIdIsInProgress(). That could cause trouble later. One
known consequence is that if you then went ahead with updating the row
again and called MultiXactIdExpand(), MultiXactIdExpand() would see
both the old aborting transaction and the new updater as still
running, and throw an ERROR: new multixact has more than one updating
member. The multixact-aborted-updater.spec included in this commit
exercises that scenario.
Another place that got this wrong was in heap_update(), where we check
if the updating transaction that was part of a multixid committed or
aborted. Because it used TransactionIdDidAbort(), if the updating
transaction crashed, it was considered as committed and heap_update()
incorrectly returned TM_Updated indicating a conflict, even though the
row could be updated. This would only happen if the multixid as whole
was still considered running, i.e. if the tuple was still locked by
other transactions while the updating transaction crashed. That can
happen if the locker is a prepared transaction. The included
060_multixact_crashed_updater.pl test tests that scenario.
Author: Chee Wooson <wuqi@vastdata.com.cn>
Tested-by: Lionel Hamayon <lionel.hamayon@evolution-digitale.fr>
Discussion: https://www.postgresql.org/message-id/5778D6297D3CCF21+202609151836512678486@vastdata.com.cn
Backpatch-through: 14
The previous commit stopped using the fast path when FK values need a
cast function, which leaves its code for calling them unused. So
remove the following: the cached FmgrInfos for the cast and equality
functions and the memory context they used, the cast step in
build_index_scankeys(), the EXECUTE privilege check on the cast
function, and the unused forward declaration of RI_CompareHashEntry.
Also revert the changes made to ri_HashCompareOp() for the fast path's
cross-type operators and domain-typed keys in commits 2da86c1ef9 and
68ace967c1. Fast-path code no longer uses the function.
Reviewed-by: Matheus Alcantara <matheusssilv97@gmail.com>
Discussion: https://postgr.es/m/CAM527d9BgPjeOOYmbCBTd57R145qHCk-dzw9qNq+nOrDq1j__A@mail.gmail.com
Backpatch-through: 19
When the FK column's type needs a cast function to reach the equality
operator's input type, the RI fast path calls that function directly.
The SPI path instead applies the cast as an expression in its query,
which takes care of the function's declared arguments, its input
collation, a NULL result, volatility, and replanning when the function
changes. The direct call doesn't cover all of that; for example, a
NULL result raises an internal error rather than a foreign key
violation, and if the cast's function is dropped and replaced, later
checks fail with "cache lookup failed for function". Coercions that
aren't a call to one function, array coercion and coercion via I/O,
fail outright with "no conversion function".
Rather than replicate expression evaluation, leave FK values that
aren't binary-coercible to the operator's input type to SPI. The fast
path keeps handling cross-type operators, domains and binary-compatible
types. As eligibility now depends on pg_cast, invalidate cached
constraint information on pg_cast changes too.
Because the check needs catalog lookups, it is made in
ri_check_fastpath_index(), which caches its result in fastpath_state
until the constraint's cached information is invalidated, rather than
for every row in ri_fastpath_is_applicable().
The two tests that used a cast function to exercise fast-path code that
still exists (metadata invalidation and the active snapshot) now use
the equality function of a custom opclass, the only user code a
fast-path check still runs.
The fast-path code that calls cast functions is now unused; the next
commit removes it.
Reported-by: Nikolay Samokhvalov <nik@postgres.ai>
Reported-by: Amit Langote <amitlangote09@gmail.com>
Reviewed-by: Matheus Alcantara <matheusssilv97@gmail.com>
Discussion: https://postgr.es/m/CAM527d9BgPjeOOYmbCBTd57R145qHCk-dzw9qNq+nOrDq1j__A@mail.gmail.com
Backpatch-through: 19
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
The pgstats callbacks to_serialized_name() and from_serialized_name()
are documented as being for variable-sized stats only, as they map an
entry hash key to a name written to the on-disk stats file.
pgstat_register_kind() did not enforce this policy for custom stats
kinds. Enforcing this behavior is more interesting since 44539786b8,
where a stats kind can begin pointing at its own hash table, offering as
guarantee that a PGSTAT_FILE_ENTRY_NAME will never match a fixed-sized
stats kind at startup, should the same kind ID be reused across restart
with a different kind info.
See also 0d110901f0 that has dealt with a similar issue, for
PGSTAT_FILE_ENTRY_HASH.
Author: Sami Imseih <samimseih.pg@gmail.com>
Discussion: https://postgr.es/m/CAN4CZFMxuOOuYV14+i3YcPG-16q02eBtSVa82J111=ZnDwSicA@mail.gmail.com
pgstat_read_statsfile() has never checked that the kind of a hash entry
is variable-sized. Since 44539786b8, that has begun tracking which
hash table a kind ID should use, it is possible to finish with a NULL
pointer dereference for a PGSTAT_FILE_ENTRY_HASH. Fixed-sized stats
kinds have no hash table and no DSA area, hence their dsa references in
pgStatLocal.kind_hash[] are NULL.
This case can occur when reading statistics files if the same kind ID is
reused across server restarts, with shared_preload_libraries loading
different stats kind information each time: stats written with a set of
PGSTAT_FILE_ENTRY_HASH, then read back with the same kind ID pointing to
a custom fixed-sized kind info.
Oversight in 44539786b8.
Reported-by: Zsolt Parragi <zsolt.parragi@percona.com>
Author: Sami Imseih <samimseih.pg@gmail.com>
Author: Zsolt Parragi <zsolt.parragi@percona.com>
Discussion: https://postgr.es/m/CAN4CZFMxuOOuYV14+i3YcPG-16q02eBtSVa82J111=ZnDwSicA@mail.gmail.com
To perform cross-type index scans with a timestamp index and a
timestamptz comparison value, btree_gin must convert the timestamptz
to timestamp to establish the starting point for the indexscan. This
is hazardous because timestamp_cmp_timestamptz is actually defined to
upconvert the timestamp to timestamptz and then compare timestamptz's.
When dealing with ambiguous timestamps near a fall-back DST transition,
the value may not round-trip, leading to selecting a scan start point
that misses some index entries that should match.
To fix that, move the converted value back an hour at a time until
it's clearly <= the given comparison value. (We could try to be
smarter, but this way seems more certain to work across all possible
DST behaviors.)
Another hazard is that for timestamptz's that are finite but close to
the upper range limit, the conversion may produce timestamp +Infinity,
again comparing greater than the given value. It's less clear whether
this can lead to missing matching values in practice, but it would at
least risk making the aforesaid correction loop very slow. So replace
+Infinity by the maximum finite timestamp.
The reverse case of timestamptz index and timestamp comparison value
is not at risk because upconverting the timestamp stays consistent
with what timestamptz_cmp_timestamp will do. Also, while
date-vs-timestamptz could theoretically be at risk, the conversion
error would have to be a full day which seems not to occur in reality.
Author: shihao zhong <zhong950419@gmail.com>
Reviewed-by: Tom Lane <tgl@sss.pgh.pa.us>
Discussion: https://postgr.es/m/CAGRkXqQio=7PWgTfKvjyQY9-7A_0A36E3put1gxJeDypgqFSzw@mail.gmail.com
Backpatch-through: 19
Initial array-specific preprocessing in _bt_preprocess_array_keys
transforms inequality strategy SK_SEARCHARRAY scan keys into simple
scalar scan keys, whose sk_argument contains the array's most
restrictive element. The transformed keys nevertheless retained their
SK_SEARCHARRAY markings. This approach left it up to everybody else to
determine whether any given SK_SEARCHARRAY-marked key was an equality
strategy key (which actually has an array) or an inequality key (which
no longer has one).
Simplify things by teaching preprocessing to unset SK_SEARCHARRAY from
the flags for inequality scan keys, at the point where it already sets
the key's sk_argument to a simple scalar datum. This makes it safe for
code that runs after array preprocessing to assume that any key marked
SK_SEARCHARRAY must be an equality key.
This also fixes a minor inconsistency: _bt_advance_array_keys's precheck
of the least significant key was skipped when that key came from a SAOP
inequality, even though we were willing to apply that optimization given
an inequality that had a scalar argument before preprocessing.
Author: Peter Geoghegan <pg@bowt.ie>
Discussion: https://postgr.es/m/CAH2-WznnuUH-fH84d60UHmDo4rO0Mio4_8DGvhxBo1yL0EwPng@mail.gmail.com
Discussion: https://postgr.es/m/CAH2-Wznvj74CAwVydAs_Lh1hR3PGi_aN1s7BiRKE0iJYNdoixQ%40mail.gmail.com
Self-join elimination failed to check for inheritance cases,
which has two big problems. First, a unique index on the parent
table isn't sufficient to prove uniqueness across its children too.
Second, since we matched RTEs by OID without considering rte->inh,
we could try to merge "mytable" with "mytable ONLY", which doesn't
even represent the same set of rows.
We can fix both issues by just skipping RTEs that have inh set.
preprocess_relation_rtes already cleared that flag for tables known
not to have inheritance children, so this doesn't break SJE for the
cases where it's valid.
Author: Tender Wang <tndrwang@gmail.com>
Reviewed-by: Tom Lane <tgl@sss.pgh.pa.us>
Discussion: https://postgr.es/m/CAHewXNn4EjCQEvanN_FPKQ3OJWaBXq8Pxcj-DZYbeQ6_4xSFgw@mail.gmail.com
Backpatch-through: 18
The tablesample and securityQuals subtrees of a RangeTblEntry can
include Vars, which will cause trouble if they point at a removed
rel and are visited during later processing. Hence, clear those
fields during join removal, as we already do for the subquery field.
remove_useless_outer_joins got this right for securityQuals but
not tablesample, while remove_self_join_rel missed doing it for
securityQuals. (It escapes trouble for tablesample only because we
made a policy decision to never touch sampled relations for SJE.)
Oversight in 2ebf25e7d. Back-patch to v16, like that commit
(but the SJE issue only goes as far back as v18).
Author: shihao zhong <zhong950419@gmail.com>
Reviewed-by: Tom Lane <tgl@sss.pgh.pa.us>
Discussion: https://postgr.es/m/CAGRkXqSOAT1yop44B8jwacL_91rxK=QuoU8Of3NLMb9NgAO2NA@mail.gmail.com
Backpatch-through: 16
The process by which a hash index inserter finishes an incomplete bucket
split involves a walk of the new bucket (the bucket "being populated")
to collect the TIDs that the interrupted split already moved. This
process keeps a pin on the new bucket's primary page, and then takes
conditional cleanup locks on both buckets. When either lock could not
be acquired, we gave up, but neglected to release the new bucket's pin.
To fix, release the buffer pin on failure.
Oversight in commit 6d46f4783, which improved hash index bucket split
behavior by switching it from using a heavyweight lock to using these
cleanup locks.
Author: Peter Geoghegan <pg@bowt.ie>
Reviewed-by: Ayush Tiwari <ayushtiwari.slg01@gmail.com>
Discussion: https://postgr.es/m/CAH2-Wz=b9sSBv15HsgZAuazjJu8-YRGHOpLjvp+dJgEDaBx0Ng@mail.gmail.com
Backpatch-through: 14
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
When being used as a window function, these aggregates must run the
finalization step repeatedly on their accumulated state. During that,
discarding null-valued pairs isn't okay if we want to reject duplicate
keys, since per SQL spec a null-valued pair still participates in
duplicate key checking. Hence, when both those options are selected
(jsonb_object_agg_unique_strict) run uniqueifyJsonbObject on a copy of
the pairs array, keeping the original data structure unmodified.
We don't want to do that when unique_keys isn't specified, because
repeat de-duplication can be very expensive when there are many
duplicates, but fortunately we don't have to.
uniqueifyJsonbObject did not renumber the pairs it kept, so a pair
added later could lose to an older one with the same key. Renumber
them to ensure that we preserve last-one-wins behavior across
multiple finalizations. Also improve related comments.
While adding test cases to cover these mistakes, I chose to sync up
all the json[b]_object_agg tests in json.sql and jsonb.sql. This
exposed that json_object_agg() fails to de-duplicate keys in its
output, which seems like a pre-existing bug; but for this commit
let's just memorialize its behavior.
Author: shihao zhong <zhong950419@gmail.com>
Reviewed-by: Tom Lane <tgl@sss.pgh.pa.us>
Discussion: https://postgr.es/m/CAGRkXqTCfSu=aEUCvKDTGroEWOcyz3MCv4Xi8x1PZ1T1rEmG=Q@mail.gmail.com
Backpatch-through: 19
If you called pg_upgrade with a --new-datadir that points to a
configuration-only dir and no --new-bindir option, it would try to
execute "postgres -C data_directory" using the new binary before
resolving the correct path to it.
To fix, check and resolve the binary dirs first, before calling
adjust_data_dir(). For clarity, rename the setup() function to
check_clusters_stopped() to better reflect what it does now.
One consequence of this is that if the binaries are not found, you now
get an error earlier than before, before the pg_upgrade_output.d
directory and the log files have been created. That's what
necessitated the test change in this commit. The log files are
important for things like listing unsupported objects in the source
database, but missing binaries feels like a more permanent error,
similar to invalid options or incorrect directory permissions, and it
seems OK to just print the error to stderr for those.
This has been broken forever, but I chose to not backpatch since no
one else has complained, it's a pretty minor issue, and it's easy to
work around by passing the binary dir explicitly. Maybe later, if
someone in the field runs into this.
Reviewed-by: Chao Li <li.evan.chao@gmail.com>
Reviewed-by: Bohyun Lee <bohyun.lee@databricks.com>
Discussion: https://www.postgresql.org/message-id/6894f355-10e4-4756-89ad-ae5987f416a6@iki.fi
getQuadrant() places a point on each axis with the fuzzy comparisons
FPlt(), FPgt() and FPeq(), which do not cover every case. FPeq()
compares the rounded difference against EPSILON, while FPlt() and FPgt()
round the sum of one operand and EPSILON. When the centroid plus
EPSILON rounds up, a point equal to that rounded sum is more than
EPSILON above the centroid, so FPeq() is false. FPgt() compares it with
the same rounded sum, so it is false too, and FPlt() is false because
the point is above the centroid. The same gap exists below the
centroid, when the centroid minus EPSILON rounds down. No quadrant
matches, and index builds, inserts and searches fail with
"getQuadrant: impossible case".
To fix, fall back to deciding each axis separately: Use fuzzy tests
first and then tiebreak with exact comparisons. That gives every finite
value a quadrant (NaN coordinates are still an error). Points that
matched a fuzzy test before are routed as before, so existing indexes
remain valid.
Bug: #19597
Reported-by: Michael Malis <malis@pgrust.com>
Author: Ayush Tiwari <ayushtiwari.slg01@gmail.com>
Reviewed-by: Andrey Rachitskiy <pl0h0yp1@gmail.com>
Reviewed-by: Pierre Forstmann <pierre.forstmann@gmail.com>
Discussion: https://postgr.es/m/19597-39c532e61d78dff6@postgresql.org
Backpatch-through: 14
Progress reporting in pg_basebackup and pg_stat_progress_basebackup used
the estimated size of a full backup even when taking an incremental backup.
The size estimation pass did not account for incremental files and counted
every file at its full size. Progress could therefore remain at only a few
percent even after the incremental backup completed successfully.
Fix this by making the size estimation pass account for incremental files,
counting them at the size they will occupy in the archive.
Backpatch to 17, where incremental backup was introduced.
Author: Daria Lepikhova <daria.n.lepikhova@gmail.com>
Reviewed-by: Manuel Reyes Bravo <manuelreyesbravo@gmail.com>
Reviewed-by: Zsolt Parragi <zsolt.parragi@percona.com>
Reviewed-by: Surya Poondla <suryapoondla4@gmail.com>
Reviewed-by: Fujii Masao <masao.fujii@gmail.com>
Discussion: https://postgr.es/m/CAK49S1ASwk7L-7LYUUz1MU_+uQnZ75srva2XRR4r-w0HCVZeUQ@mail.gmail.com
Backpatch-through: 17
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 a Gather or Gather Merge node is rescanned, it starts a new set of
workers, which write their Memoize stats into the same shared memory
slots as the previous worker. ExecEndMemoize() used memcpy() for this,
so EXPLAIN ANALYZE showed only the stats for the final scan.
Here we adjust ExecEndMemoize() to accumulate the counters so that we
gather up the totals over all rescans.
Author: Tatsuya Kawata <kawatatatsuya0913@gmail.com>
Reviewed-by: David Rowley <dgrowleyml@gmail.com>
Discussion: https://postgr.es/m/CAHza6qeA-4vB355co-2yAxDMRTpEneY0rg9CD2zMLYscYvv2UA@mail.gmail.com
Backpatch-through: 14
When wal_compression=zstd, the compression of a full page image involves
ZSTD_compress(), that creates and destroys a zstd compression context
for each page processed.
Tests with FPI-heavy workloads have proven that this can be costly, and
a compression context can be reused across multiple pages. This commit
switches to use ZSTD_compressCCtx() for the compression of each page,
with ZSTD_CCtx allocated once in each backend, allocation happening the
first time it is required. If the allocation fails, the code
temporarily fails back to no compression, as a soft failure mode while
being inside a critical section.
Some tests have proved that this simple change can lead to a substantial
performance gain. For example, I have measured up to 6% faster runtime
and 8% less user CPU time for a VACUUM FULL generating WAL made mostly
of zstd-compressed FPIs.
ce3387b74b has done something similar for the decompression of pages
at replay.
Author: Andrey Borodin <x4mmm@yandex-team.ru>
Reviewed-by: Anthonin Bonnefoy <anthonin.bonnefoy@datadoghq.com>
Reviewed-by: Michael Paquier <michael@paquier.xyz>
Discussion: https://www.postgresql.org/message-id/asRsOl73nf8GeBFd@paquier.xyz
match_clause_to_partition_key() tells whether a comparison value can
be evaluated during executor-startup pruning by checking that it has
no Vars, no volatile functions and no PARAM_EXEC Params. A SubPlan
normally fails the last test, since the testexpr of an ANY/ALL SubPlan
references the subplan's output Params. But if the left-hand side is
a NULL constant, the strict comparison operator in the testexpr is
const-folded to a NULL Const, and an uncorrelated SubPlan has no
parParam or args either, so nothing is left for pull_exec_paramids()
to find. The expression then reaches initial_pruning_steps, where
ExecDoInitialPruning() must evaluate it before any PlanState exists,
and we fail with "SubPlan found with no parent plan".
To fix, don't allow such an expression to be used for startup-time
pruning. It's still fine for per-scan pruning, which has a PlanState
to evaluate it with. Expressions containing exec Params are
unaffected.
This has been broken since d47cbf474, which moved initial pruning out
of ExecInitNode(); before that, pruning always had the parent node's
PlanState available. Back-patch to v18.
Author: Richard Guo <guofenglinux@gmail.com>
Reviewed-by: Amit Langote <amitlangote09@gmail.com>
Discussion: https://postgr.es/m/CAMbWs48nYDu7joYeyMKYdX8kHoL_ASEax2fqZH6L+HHqO2nkXA@mail.gmail.com
Backpatch-through: 18
If a proposed join has lateral references to rels outside it,
join_is_legal() checks that those rels can still be joined to from
outside. To do that it collects the rels that must end up on the
inner side of an outer join with the proposed join, directly or
indirectly, and rejects the join if any of its lateral references
point to one of them.
That search followed only outer joins, from min_lefthand to
min_righthand. It failed to notice that a rel that laterally
references the proposed join, or any rel found by the search, cannot
supply a parameter to the proposed join either, and that the same
holds for rels on the inner side of an outer join with such a rel. As
a result, we could approve a join whose lateral references could never
be satisfied.
Such a join can never appear in a complete plan. Worse, as the
existing comment warns, its apparent legality could mislead the
clauseless-join heuristics into thinking that some other join rel need
not be formed. In particular, have_join_order_restriction() would not
force a clauseless outer join if one of its input rels appeared to
have a legal clause join elsewhere, and if that outer join was
required, we would fail with "failed to build any N-way joins".
To fix, also include in the search any input rel of the current join
search that laterally references a rel already found. The input rels
are taken from root->initial_rels rather than from the query's base
rels, because an input can be the joinrel of a sub-joinlist, as
happens for a full join or when from_collapse_limit or
join_collapse_limit prevents flattening. Such a joinrel is joined as
a unit, so it has to be included as a whole.
Back-patch to all supported branches.
Author: Richard Guo <guofenglinux@gmail.com>
Reviewed-by: Tender Wang <tndrwang@gmail.com>
Reviewed-by: wenhui qiu <qiuwenhuifx@gmail.com>
Discussion: https://postgr.es/m/CAMbWs4_p6iWbsjNcYpd6-6mWFnog3UJ2fZKrxWAQTsyNoyeQTw@mail.gmail.com
Backpatch-through: 14
