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
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
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
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
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
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
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
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
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
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
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
To avoid warnings and lines that extend beyond the right page
margin in the PDF documentation, we have zero-width spaces in
the source at strategic places. They are not visible to the
naked eye, but allow FOP to break the lines in sane places.
However, some of these zero-width spaces were in the middle of
text that people might copy and paste from the HTML documentation,
which leads to mysterious syntax errors.
Scanning the documentation for &zwsp; identified four places
that this patch improves:
- an ISO 8601 interval literal didn't actually need a line break
in either the US letter or A4 format, so simply remove that
zero-width space.
- three v18-era examples for uuid_extract_timestamp() and
uuid_extract_version() had zero-width spaces in the middle of UUID
literals. Instead, add regular spaces around the "::" cast symbols,
allowing FOP to break the lines there. This solution isn't
invisible in the HTML output, but it doesn't look bad.
Reported-by: Y. Saburov <y.saburov@gmail.com>
Author: Laurenz Albe <laurenz.albe@cybertec.at>
Reviewed-by: Tom Lane <tgl@sss.pgh.pa.us>
Discussion: https://postgr.es/m/179143326813.1061054.14714458907175925072@wrigleys.postgresql.org
Backpatch-through: 18
For run-time partition pruning running during execution (rather than at
executor startup), EvalPlanQualStart() reuses the es_part_prune_states
from the parent EState. This could cause an issue when the EPQ recheck is
finished, its EState is cleaned up and the es_query_cxt is deleted by
FreeExecutorState(). That cleanup could free memory that was allocated
again in InitExecPartitionPruneContexts() when it was called for the EPQ's
EState, and naturally, those fields got allocated in the EPQ's
es_query_cxt MemoryContext, which overwrote the ones for the parent
EState. If pruning occurs again after the EPQ's EState was cleaned up,
then we could have accessed free'd memory. This certainly caused issues in
debug builds because we clobber freed memory, but may not have in
production, depending on if the MemoryContext being deleted results in a
free(). It may not if the allocation was on the context's init block and
the context was pushed onto context_freelists[] for reuse.
Here we fix this by adding an "initialized" field to PartitionPruneState
and exit early in InitExecPartitionPruneContexts() when the state is
already initialized.
This was broken by bb3ec16e1, but the breakage there is slightly
different as that commit neglected to do what was fixed in 8741e48e5
(which did get backpatched to v18 in 9a82a64ed).
Reported-by: Vladimir Savin <vladimir@encord.com>
Author: David Rowley <dgrowleyml@gmail.com>
Reviewed-by: Andrey Rachitskiy <pl0h0yp1@gmail.com>
Discussion: https://postgr.es/m/CAP2G_gGpV=rKMLuET4RW-qO41GvBvi6Czsjsj16eT9hAwHxG6w@mail.gmail.com
Backpatch-through: 18
Commit 5cd72cc0c5 made the VACUUM VERBOSE and autovacuum logs report
buffer usage from the pgBufferUsage delta, which includes the parallel
workers' usage, but left the I/O timings on the leader's
pgStatBlockReadTime and pgStatBlockWriteTime. So the log counted the
blocks the workers read and dirtied but not the time they spent on
them.
Fix this by taking the I/O timings from the same BufferUsage delta.
This also removes a small error in the leader's own timings, since
pgStatBlock{Read,Write}Time accumulate whole microseconds per I/O.
Although 5cd72cc0c5 was backpatched to v15, BufferUsage on v16 doesn't
track the I/O time of temporary relations, so the same fix there would
drop that time from the log. We don't think a reporting issue is worth
v16-specific code, nor worth leaving v16 unfixed between v15 and v17,
so backpatch to v17 only.
Found by Bharath using AI assisted review with Claude.
Reported-by: Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com>
Author: Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com>
Reviewed-by: Sami Imseih <samimseih.pg@gmail.com>
Reviewed-by: Chao Li <li.evan.chao@gmail.com>
Reviewed-by: Masahiko Sawada <sawada.mshk@gmail.com>
Reviewed-by: Shihao Zhong <zhong950419@gmail.com>
Discussion: https://postgr.es/m/CALj2ACXrqaHrYmGdpAoGKXtiY9Xw4o1dxSC+EELxn47GDO6HAQ@mail.gmail.com
Backpatch-through: 17
_bt_pagedel only builds a search stack when it needs one to locate the
downlink of the page that it's about to mark half-dead (and when it
doesn't already have one from an earlier sibling deletion that took
place during the same _bt_pagedel call). _bt_pagedel won't build a
search stack for the page its caller passes whenever it happens to be
one that an earlier interrupted VACUUM left half-dead (since there is no
downlink to remove). We nevertheless asserted that page deletion could
only build a search stack for that initial caller-passed page.
To fix, remove the assertion, which spuriously failed when a half-dead
leaf page was passed by the _bt_pagedel caller and its right sibling
page was empty/had to be considered for deletion during the same call
(necessitating building a search stack to locate the right sibling's
downlink).
Oversight in commit cd8c73a38, which refactored nbtree page deletion's
INCOMPLETE_SPLIT check.
Author: Peter Geoghegan <pg@bowt.ie>
Reported-by: Iliia Khaprov <i.khaprov@gmail.com>
Discussion: https://postgr.es/m/CAFO+O_p5So4MXwQ1VEcL9+9vGgODu11TTWcYq9RDbxnira8o2w@mail.gmail.com
Backpatch-through: 14
Follow-up to bca67e5a3. The test added there changed the value of
oauth_validator_libraries, waited for the postmaster to apply it, and
verified the change on an existing background connection. But there's no
guarantee that the SIGHUP will have arrived and completed its work on
existing backends by that time, so the final test flaked.
We only care about the postmaster's state here, since we're testing the
GUC's effects on newly forked backends, so remove the background check
entirely.
Per buildfarm member serinus.
Reported-by: Alexander Lakhin <exclusion@gmail.com>
Discussion: https://postgr.es/m/a37a3cea-5bd8-4385-a1d4-bb89394c079c%40gmail.com
Backpatch-through: 18
When a join alias Var with nonempty varnullingrels expands to an
expression that cannot carry the nullingrels itself, such as the
RowExpr of a whole-row Var, add_nullingrels_if_needed() wraps it in a
PlaceHolderVar. The choice of the PHV's phrels had several problems.
First, phrels was taken from all Vars in the expression. If a LATERAL
subquery within the join has been pulled up, the expression can
contain lateral references to rels outside the join, so the PHV could
be evaluated above the outer join that is supposed to null it. This
gave wrong results, or "wrong phnullingrels" errors.
Second, for a variable-free expression we fell back to the join's
relids but removed the join's own outer-join relid. That leaves a set
spanning both sides of the outer join without the join itself, which
can make the PHV look nullable by that join and cause "wrong
phnullingrels" errors.
Third, that fallback raised "unsupported join alias expression" when
the Var is an outer reference from a subquery. The restriction is
unnecessary: every planner caller passes root->parse as the query, so
the Var always belongs to root->parse regardless of its varlevelsup.
To fix, restrict phrels to the rels within the join, and if nothing is
left, evaluate the PHV at the join including its outer-join relid, at
any varlevelsup. Add an assertion that the query is root->parse.
Author: Richard Guo <guofenglinux@gmail.com>
Discussion: https://postgr.es/m/CAMbWs49q_gvoeOoF0+KZ-DpQaDb=nGFfSgrz2V_n1_64L4ZNVQ@mail.gmail.com
Backpatch-through: 16
When a query has aggregates or GROUP BY, parseCheckAggregates()
flattens join alias Vars in the grouping expressions, target list and
HAVING clause so that aliased and unaliased references compare equal.
The parser has no PlannerInfo, so this relies on every join alias
expression being able to carry varnullingrels without a PHV. That
doesn't hold for a whole-row Var of a join on the nullable side of an
outer join: it expands to a RowExpr, and pushing the nulling bits down
into its fields would turn a NULL row into ROW(NULL, ...). Such
queries therefore failed with "unsupported join alias expression".
To fix, leave a nulled whole-row join Var unexpanded when no root is
available. The planner expands it later and wraps it in a
PlaceHolderVar as before. Nothing is lost for GROUP BY matching: a
nulled whole-row Var can only be equal to the same Var, never to an
expression over the join's columns.
This has been broken since the introduction of varnullingrels in v16.
Author: Richard Guo <guofenglinux@gmail.com>
Discussion: https://postgr.es/m/CAMbWs49q_gvoeOoF0+KZ-DpQaDb=nGFfSgrz2V_n1_64L4ZNVQ@mail.gmail.com
Backpatch-through: 16
reindexdb batches the REINDEX commands of all indexes attached to the
same table into a single multi-statement query when using multiple jobs,
as two steps with query "gen" and "run".
Such a query fails with REINDEX CONCURRENTLY, as this command cannot run
inside a transaction block. Batching cannot be avoided because the
indexes of one table have to be re-indexed one at a time to avoid
conflicts across jobs.
Let's error out cleanly on the reindexdb side if this combination of
options is used, instead of having the server complain about that. This
case has not been considered by 47f99a407d.
If there is interest in supporting this combination of options, the code
should be redesigned for this purpose with some better batching, rather
than try to use sets of "gen" and "run" routines. As currently written,
an index-level reindexdb uses a set of "gen" steps with one final "run"
step, which is nice code-wise. (Note: I doubt that that this would be
worth the effort.)
Author: Kirill Reshke <reshkekirill@gmail.com>
Discussion: https://postgr.es/m/CALdSSPiEkAmS+n7Ea9XDMrLjkbQk=CrB7zw1kcq9B47hfKUhXw@mail.gmail.com
Backpatch-through: 17
As of a32b8dc98 and friends, amcheck will complain about bpchar
indexes that claim to allow de-duplication. This causes our
cross-version-upgrade tests to fail when upgrading from a version
that lacks those commits, since there are some such indexes in the
core regression tests. Drop such indexes from the upgrade source
database. (oidvector indexes would be problematic too, but
there aren't any user-defined indexes on oidvector in our tests.)
Author: Peter Geoghegan <pg@bowt.ie>
Reviewed-by: Tom Lane <tgl@sss.pgh.pa.us>
Discussion: https://postgr.es/m/CAH2-Wzk-oO3HjLaByo8Vpse6Tpha1mqAW8DFShh-kS-Wo59ugA@mail.gmail.com
Backpatch-through: 14
bpchar equality ignores trailing spaces, so equal values in a column
with no length specifier can have different images. Similarly,
oidvector equality intentionally pays no attention to the array lower
bound field. We nevertheless allowed nbtree deduplication to be used
with indexes built with bpchar_ops, bpchar_pattern_ops, or
oidvector_ops. A posting list split could therefore place a TID in an
index tuple whose key was equal to, but not bytewise identical to, the
corresponding table value. This allowed index-only scans to return
subtly wrong results.
To fix, drop support function 4 from the affected opclasses (we can't
change catalog contents on branches 14 through 18, so we just disable
deduplication by adding hard-coding to the relevant support functions).
Deliberately leave the oidvector opclass alone on branches 14 through
18, though. Disallowing oidvector deduplication on these branches would
make pg_amcheck report that pg_proc_proname_args_nsp_index has a corrupt
metapage, needlessly alarming users.
System catalog oidvector values always have a lower bound of 0, so the
issue can't cause harm there; a user can only hit it by indexing an
oidvector column that stores the output of a function like trim_array().
It seems very unlikely that any user would ever do that.
Users should REINDEX any B-Tree index on a bpchar column.
Author: Shihao Zhong <zhong950419@gmail.com>
Co-authored-by: Peter Geoghegan <pg@bowt.ie>
Reported-by: Ke <kehan5800@gmail.com>
Reported-by: Tom Lane <tgl@sss.pgh.pa.us>
Reviewed-by: Tom Lane <tgl@sss.pgh.pa.us>
Bug: #19749
Discussion: https://postgr.es/m/19749-d46adf7c7d910b07@postgresql.org
Discussion: https://postgr.es/m/CAGRkXqQQ311Lg=62x5UdZtPuRFgpT6XpPMw4X9E3tgbTu162Mw@mail.gmail.com
Backpatch-through: 14
seg.sgml claims that a seg can store 7 significant digits, but this
is nonsense. The underlying float4 data type is not (quite) that
precise, and seg_out() intentionally rounds off to at most FLT_DIG
decimal places. With IEEE float arithmetic, FLT_DIG will be defined
as 6, or at least that's the case on all platforms I know of.
AFAICT this thinko goes clear back to the introduction of seg
(commit a3694b420), but it wasn't questioned until now.
Bug: #19744
Reported-by: Ke <kehan5800@gmail.com>
Author: Tom Lane <tgl@sss.pgh.pa.us>
Discussion: https://postgr.es/m/19744-8c700582bbb1d075@postgresql.org
Backpatch-through: 14
COPY FROM runs its WHERE clause through eval_const_expressions(),
which among other things replaces NOT above an operator with the
operator's negator, if it has one (e.g., NOT (a > 0) becomes
a <= 0). The node it builds for the negator has no opfuncid, and
since COPY FROM never calls fix_opfuncids() on the clause, nothing
fills it in before the executor needs it. So
COPY tab FROM stdin WHERE NOT (a > 0)
fails with "cache lookup failed for function 0".
To fix, call fix_opfuncids() on the clause, as the relcache does
for index predicates.
Oversight in commit 31f3817402.
Reviewed-by: Tom Lane <tgl@sss.pgh.pa.us>
Discussion: https://postgr.es/m/asPZRTci4oBvxR9A%40nathan
Backpatch-through: 14
pull_up_simple_subquery() rechecks is_simple_subquery() after pulling
up the subquery's own subqueries, but it flattened join alias Vars in
the subquery's targetlist only after that, and never in its quals. A
join alias Var within the subquery, such as a whole-row Var of a join,
can expand to an expression containing lateral references to the outer
query once the join's inputs have been pulled up, and until it is
flattened is_simple_subquery() cannot see them. We could thus pull up
a LATERAL subquery whose targetlist or quals reference rels outside
the lowest outer join above it. This leads to assertion failures in
distribute_qual_to_rels() or "wrong phnullingrels" errors, and in some
cases to wrong query results, where rows that should have been
preserved by an outer join above the subquery are discarded.
To fix, flatten join alias Vars in the targetlist, and in the jointree
quals if the subquery is LATERAL, before the recheck. If the recheck
fails, the flattened copy is discarded along with the rest of the
modified subquery, so no harm is done.
Back-patch to all supported branches, as this can lead to wrong query
results.
Author: Richard Guo <guofenglinux@gmail.com>
Reviewed-by: Tatsuya Kawata <kawatatatsuya0913@gmail.com>
Discussion: https://postgr.es/m/CAMbWs48GFZ=3Bjc1ug9JRsB0Qupg+CGcb9js5vmTM6kY2OGWOg@mail.gmail.com
Backpatch-through: 14
Previously, dropping a NOT NULL column, adding a new column with the same
name, and making it a primary key in a single ALTER TABLE command could
fail with "primary key column ... is not marked NOT NULL". This happened
because ALTER TABLE checked for an existing NOT NULL constraint before
dropping the old column. Finding the constraint caused it to skip adding
one for the new column, even though the old constraint would be removed
with the old column.
Defer this check until after column drops and additions have been
executed, so that it examines the columns that will actually form the
primary key. Add any required NOT NULL constraints before creating the
primary-key index.
Backpatch to v18, where this issue was introduced by commit a379061a22.
Reported-by: Ludvig Janiuk <ludvig.janiuk@proton.me>
Author: Fujii Masao <masao.fujii@gmail.com>
Co-authored-by: Manuel Reyes Bravo <manuelreyesbravo@gmail.com>
Discussion: https://postgr.es/m/0a9b4c44-cdff-4ec8-a8fb-b3a8b59e9ba9@proton.me
Backpatch-through: 18
Since commit e28cf96e9, the copies of a query level's PlaceHolderVars
that were pushed down into its subqueries are preprocessed by that
level before its SubLinks are turned into SubPlans, so such a copy may
already contain SubPlans by the time the subquery is planned. That
commit taught build_subplan not to process the SubLinks of a PHV that
is passed down as a SubPlan argument, but overlooked that such a copy
can also appear within the arguments of an outer-level Aggref,
GroupingFunc, or ReturningExpr. The SubLinks in those arguments are
still processed when build_subplan collects the node, and that
processing descended into the already-preprocessed PHV and tripped the
no-SubPlan assertion in process_sublinks_mutator.
To fix, don't descend into PHVs when processing the SubLinks in such a
node's arguments. Any PHV found there is a pushed-down copy that its
owning level has already preprocessed.
Back-patch to v16, as with commit e28cf96e9.
Author: Ayush Tiwari <ayushtiwari.slg01@gmail.com>
Reviewed-by: Richard Guo <guofenglinux@gmail.com>
Discussion: https://postgr.es/m/CAJTYsWW8THbrnsmYnDqnUgg0PjGrqw2vNOWb8qx7eu191KPW3g@mail.gmail.com
Backpatch-through: 16
pg_blocking_pids() assumes that members of the same lock group
never block each other, but that hasn't been true for relation
extension locks since v13. This commit teaches the function to
report lock group members as blockers for relation extension locks.
Since parallel workers are reported with their leader's PID, the
result may now include the blocked session's own PID.
Oversight in commit 85f6b49c2c.
Author: Bertrand Drouvot <bertranddrouvot.pg@gmail.com>
Reviewed-by: Chao Li <li.evan.chao@gmail.com>
Reviewed-by: Kirill Reshke <reshkekirill@gmail.com>
Reviewed-by: Shihao Zhong <zhong950419@gmail.com>
Discussion: https://postgr.es/m/apTpV8h%2BCYHf%2B1PJ%40bdtpg
Backpatch-through: 14
When we converted these types' input functions to report errors
through the soft-error mechanism, we missed that stack-overflow
errors can be triggered through bad user input. Convert those
to be reported softly, too.
Bug: #19745
Reported-by: Ke <kehan5800@gmail.com>
Author: Tom Lane <tgl@sss.pgh.pa.us>
Discussion: https://postgr.es/m/19745-39cd7b93a505f9a9@postgresql.org
Backpatch-through: 16
