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
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
A WAL sender tracks its WAL reader and logical decoding context as two
static variables, which were left set after being freed for a
START_REPLICATION command. The WAL reader is used for in a physical
replication command, both the WAL reader and the logical decoding
context are used for a logical replication command.
Both variables are now cleared once their contents are released at the
end of each replication command, to keep the release of the resources
and the maintenance of the pointers close to each other. Some more
cleanup is done by WalSndErrorCleanup(), as well, once the WAL reader is
clear.
The assertion added by 879ad9f90e is moved before the execution of a
replication command, checking now that both the WAL reader and the
logical decoding context are never set before the execution of a new
command.
Author: Ayush Tiwari <ayushtiwari.slg01@gmail.com>
Reviewed-by: Michael Paquier <michael@paquier.xyz>
Discussion: https://postgr.es/m/CAJTYsWW+hdmmVt_A4hvGv2_5RWK8-kaOf5ncT65=5i_hyD=BHg@mail.gmail.com
Backpatch-through: 14
CheckTablespaceDirectory() did not close the directory opened with
AllocateDir(), leaving its resources held during subsequent recovery,
potentially indefinitely on a standby. Add the missing FreeDir() call.
Backpatch to all supported versions.
Author: Chao Li <lic@highgo.com>
Reviewed-by: Fujii Masao <masao.fujii@gmail.com>
Discussion: https://postgr.es/m/66A784B7-55C1-4613-8C24-B4C2A8D75414@gmail.com
Backpatch-through: 14
Building extended statistics of type "ndistinct" calls
ndistinct_for_combination() once for every combination of columns. Each
call allocates arrays for the whole sample and never frees them, with
memory accumulating until the whole statistics object is built. The
sample has 300 multiplied by target rows. With large statistics targets
(default_statistics_target up to 10k), this can add up to gigabytes
worth of memory, useless once a combination is computed.
ndistinct_for_combination() is now called in a dedicated memory context,
reset after each combination is computed. We only need the estimated
number of distinct values, so there is no need to copy anything out of
the memory context.
Similar fixes have been done in 83772cc78e and ad8a166ca8 for other
areas of the extended stats computations.
Author: Ilia Evdokimov <ilya.evdokimov@tantorlabs.com>
Discussion: https://postgr.es/m/20d6689e-f490-4ca7-a92f-e92f691ba07e@tantorlabs.com
Backpatch-through: 14
Commit 2ebf25e7d left query_planner's qp_callback call where
it had been historically, but that turns out to be a bad idea.
It means that the code reached via qp_callback is subject to the
must-be-able-to-restart rule created by looping back after join
removal. We found one place that fails that requirement:
adjust_group_pathkeys_for_groupagg sets Aggref.aggpresorted flags,
and there's nothing that will reset those in case of a query_planner
restart, yet the selected set of Aggrefs can change. There might be
other places with similar issues.
Fortunately, qp_callback's main charter is to compute/update pathkey
values, and that is not something that join removal depends on or
should depend on. (Whether join removal is valid is surely not
dependent on what order the rel might get scanned in.) Nothing else
within the restart loop depends on it either --- in other words,
the existing placement was a bit random to begin with. So we can
fix this problem just by moving that call to below the restart loop.
That costs nothing, indeed it saves some cycles by not having to do
that processing more than once. And removing an unstated requirement
from code that's not directly called by query_planner should help
forestall future bugs.
While the known bug only exists in v18 and up (older branches lack the
aggpresorted optimization), I've chosen to back-patch the code change
to v16 like 2ebf25e7d was. This is the sort of API requirement that
we don't really want to be different across branches.
Reported-by: shihao zhong <zhong950419@gmail.com>
Author: Tom Lane <tgl@sss.pgh.pa.us>
Reviewed-by: shihao zhong <zhong950419@gmail.com>
Discussion: https://postgr.es/m/CAGRkXqSOAT1yop44B8jwacL_91rxK=QuoU8Of3NLMb9NgAO2NA@mail.gmail.com
Backpatch-through: 16
extract_jsp_bool_expr applied De Morgan's laws incorrectly when
considering an AND operator underneath a NOT. If one AND input
was implementable as a GIN indexqual while the other was not,
we derived an incomplete indexqual, leading to some rows not
being returned that should be.
extract_jsp_bool_expr also failed to check for methods being
applied to literals, so that for example '$.x == "42".double()'
was treated as '$.x == "42"' not '$.x == 42'. The indexqual
thus selected the wrong rows, all of which would fail qual
recheck, leading to no rows being returned.
While we're here, improve the comments a trifle.
Reported-by: Chinmay Kanchi <cgkanchi@gmail.com>
Author: Tom Lane <tgl@sss.pgh.pa.us>
Discussion: https://postgr.es/m/CAJqPDh9Cm=x8eYZ1Jf=_avd2WEqc=0t_f7_SO4igo-CKPi25_Q@mail.gmail.com
Discussion: https://postgr.es/m/CAJqPDh-tu5KY66KU8cvAEg3DQrZLzDZwjkLLa42zD+_Xm1WJAg@mail.gmail.com
Backpatch-through: 14
When a table's only GENERATED ALWAYS AS IDENTITY column had been
dropped, pg_dump --inserts and --column-inserts could still emit
OVERRIDING SYSTEM VALUE in INSERT statements. This clause has no effect
when restoring into PostgreSQL but introduces unnecessary differences
between dumps of otherwise equivalent tables.
ALTER TABLE ... DROP COLUMN leaves pg_attribute.attidentity unchanged.
getTableAttrs() used this field to set needs_override without checking
attisdropped, so a dropped identity column could trigger the clause even
when no remaining column required it.
Ignore dropped columns when determining whether to emit the clause, so
that the output reflects only the table's remaining columns.
Apply to master only, since the unnecessary clause does not affect
restoration into PostgreSQL and does not warrant backpatching.
Author: William Bernbaum <wbernbaum@dwdev.com>
Reviewed-by: Andreas Karlsson <andreas@proxel.se>
Reviewed-by: solai v <solai.cdac@gmail.com>
Reviewed-by: shihao zhong <zhong950419@gmail.com>
Reviewed-by: Fujii Masao <masao.fujii@gmail.com>
Discussion: https://postgr.es/m/PH0PR18MB44430C10F7431F8D163CDAFFA6202@PH0PR18MB4443.namprd18.prod.outlook.com
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
The nbtree README has long claimed that "the interlocking required to
avoid returning incorrect results from non-MVCC scans is not required on
standby nodes". A recent test case proved that the underlying reasoning
was faulty.
There are significant practical challenges around fixing this bug, and
no user has complained about it so far. The old "pin scan" mechanism
prevented the problem but performed very poorly. Since there seems to
be no appetite for resurrecting that mechanism, and since an acceptable
fix hasn't been forthcoming, make a note of the issue for later.
Discussion: https://postgr.es/m/CAH2-Wzkro678xEDOrsxrq-YDVP4w7nXybhVJV2MxrzTj-5OU4w@mail.gmail.com
A hash index scan whose target bucket is still being populated by a
split must read both buckets of the split: forward scans read the bucket
being populated (skipping the tuples that the split copied there) and
then the bucket being split, and backward scans read them in the
opposite order. _hash_readnext and _hash_readprev make the transition
from the first bucket to the second, but _hash_next did not: on reaching
the end of the first bucket, it ended the scan, so every matching tuple
in the second bucket was overlooked.
To fix, teach _hash_next to make the same transition, using helper
functions shared with _hash_readnext and _hash_readprev.
_hash_next only missed the transition when the last page that it read in
the first bucket had a match (otherwise _hash_readpage's loop made the
required transition). Getting wrong answers also needed a matching
tuple to be inserted into the bucket being populated _after_ the split
began. Forward scans then lost the older tuples in the bucket being
split. An incomplete split would keep the buckets in this state until
an inserter finished the split at some indeterminate later time. Note
that wrong answers didn't require an incomplete split.
Although this fix treats the bug as the omission of a required step in
_hash_next, the real problem is the way that page stepping works in
general. The logic for stepping to the next page appears both in
high-level routines like _hash_next and in low-level routines like
_hash_readnext and _hash_readprev. If anything, _hash_next already knew
too much about page stepping (cf. nbtree's _bt_next function). All of
the logic around stepping really belongs in one routine, modeled on
nbtree's _bt_steppage function, but that approach is much too invasive
for a backpatched fix.
Claude Code found this problem. The committed test case is a simplified
version of the one that it wrote to demonstrate this bug.
Oversight in commit 7c75ef57, which introduced page-at-a-time processing
of hash index scans.
Author: Peter Geoghegan <pg@bowt.ie>
Author: Amit Kapila <amit.kapila16@gmail.com>
Reviewed-by: Vignesh C <vignesh21@gmail.com>
Discussion: https://postgr.es/m/CAH2-Wz=4CJK9ysZzbzxBGGTkgAg9ib8cJyCx=9q_d1r8tf0ang@mail.gmail.com
Backpatch-through: 14
