Unlogged tables have no data on a standby, so reading one there must
fail. SELECT and COPY (SELECT ...) TO already report a clear error,
but COPY with a table name read the table directly and failed with a
low-level error instead:
ERROR: could not open file "base/5/16384": No such file or directory
Check for this case in COPY TO and report the same error as SELECT. On
v19 and later, also check each partition when copying from a
partitioned table, so that an unlogged partition is reported too.
Backpatch to all supported versions, since the low-level error looks
like data corruption.
Author: Jim Jones <jim.jones@uni-muenster.de>
Reviewed-by: Zsolt Parragi <zsolt.parragi@percona.com>
Reviewed-by: Masahiko Sawada <sawada.mshk@gmail.com>
Discussion: https://postgr.es/m/b6d71815-d362-4579-bf07-73eca01aee6d@uni-muenster.de
Backpatch-through: 14
The main portion of findtimezone.c's win32_tzmap[] is now directly
generated from the CLDR project's current windowsZones.xml file (using
a script available in the discussion thread, but not yet merged into
our tree). Windows zone names that are no longer present in CLDR are
kept, but are moved to a separate list at the end of the array to
simplify future management.
The important changes that this brings are inclusion of new zone names
for Canada's upcoming timekeeping changes. In addition, this tracks a
few minor changes that CLDR has made in other entries:
"Central Asia Standard Time" now maps to Asia/Bishkek, and
"Mountain Standard Time (Mexico)" now maps to America/Mazatlan.
Reported-by: CyberNative AI <ceo@cybernative.ai>
Author: Tom Lane <tgl@sss.pgh.pa.us>
Discussion: https://postgr.es/m/179219370621.3670768.16110484606338109870@cybernative.ai
Backpatch-through: 14
Commit 00f4c2959d added the x modifier to psql's list commands, making
\d+x display sequence descriptions in expanded mode. However, the
documentation and \? help indicate that x applies only without a pattern.
Clarify that \d+x also uses expanded mode when describing sequences
matched by a name or pattern, and update the command help accordingly.
Also clarify that the same restriction applies to \x on and \x auto.
Backpatch to v18, where the x modifier was introduced.
Reported-by: George MacKerron <george.mackerron@databricks.com>
Author: Fujii Masao <masao.fujii@gmail.com>
Reviewed-by: Shihao Zhong <zhong950419@gmail.com>
Discussion: https://postgr.es/m/FEA91EA9-CF32-4920-9674-F2C3E01C3807@databricks.com
Backpatch-through: 18
This completes the project begun in commit 294d2196c of making
commit 41d69e6dc's hidden query labels safe against unexpected
encoding-related failures. At that point we were still uncertain
exactly what rule to use to decide that a translated label is safe.
The rule adopted here is: translate the label only when the client
and server encodings match and the translated bytes pass the same
encoding verification check the server will apply. This is enough
to ensure that no error will be thrown, and it avoids the difficult
task of identifying what encoding gettext() thinks it's using.
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
When prepare_sort_from_pathkeys() cannot find a pathkey's expression
in the input plan's targetlist, it adds a resjunk entry computing an
EquivalenceClass member's expression. It copied that expression
as-is, without replacing lateral references with nestloop Params the
way build_path_tlist() does for the rest of the targetlist.
This matters when the sort key contains a PlaceHolderVar whose
expression has a lateral reference, and the sort is placed directly
atop the plan node that evaluates the PlaceHolderVar. The targetlist
already contains the PlaceHolderVar, so the sort key is considered
computable, but the entry added for it still has the outer relation's
Var inside the PlaceHolderVar. If the node is a join, setrefs.c fails
with "variable not found in subplan target lists". If it is a scan,
setrefs.c leaves the Var in place, and the executor fetches it from
the scan tuple, leading to wrong results.
To fix, pass root to prepare_sort_from_pathkeys() and apply
replace_nestloop_params() to the expression before adding it to the
targetlist.
Back-patch to all supported branches.
Author: Richard Guo <guofenglinux@gmail.com>
Discussion: https://postgr.es/m/CAMbWs49LWNm7_OxuW1V71wLi+Ev3KbfY1bMY36uydNKwSPnbtQ@mail.gmail.com
Backpatch-through: 14
When building sort keys for a child relation, such as the per-child
sort columns of a MergeAppend over partitionwise child joins,
prepare_sort_from_pathkeys() may need to compute an EquivalenceClass
member from the child's targetlist. find_computable_ec_member()
accepts a member if all the Vars and quasi-Vars it contains appear in
that targetlist. It ignores child members that don't belong to the
given relids, but it can still choose a parent member. Usually that
is harmless, since a parent member's Vars never appear in a child's
targetlist. PlaceHolderVars are different: equal() ignores their
phexpr and phrels, so a parent PlaceHolderVar matches its translated
copy in the child's targetlist. As a result, an untranslated parent
expression could be added to the child plan's targetlist, leading to
"variable not found in subplan target lists" errors from setrefs.c.
To fix, when relids is given, also ignore parent members that don't
belong to it. This changes nothing for a non-child relation, because
any member computable from its targetlist belongs to it anyway. When
relids is NULL, only child members are filtered, as before.
Also update the comments for prepare_sort_from_pathkeys() to reflect
that 'relids' can identify a non-child relation being sorted, which
has long been the case.
Back-patch to all supported branches.
Author: Richard Guo <guofenglinux@gmail.com>
Reviewed-by: wenhui qiu <qiuwenhuifx@gmail.com>
Discussion: https://postgr.es/m/CAMbWs48bLLEGy6YC+n01w6=YR=28cj_kpoKcAgOHQR7jbtLb6w@mail.gmail.com
Backpatch-through: 14
When a stats entry is reinitialized and dropped again while a backend
still holds a reference from an older generation, the backend releasing
the last reference found a generation mismatch and left behind an entry
marked as dropped with a refcount of 0. Nothing would free it, and the
checkpointer would trigger an assertion at shutdown when flushing the
stats.
Looking more at the issue, using some hardcoded sleeps before the
dshash_find() that takes an exclusive lock when releasing an entry with
some concurrent activity, there was more going on:
- A missing entry could be reported as an error at backend exit, error
that should not be hit even if the entry was reinitialized and dropped
concurrently.
- The generation check was not correct for the case of 0 due to the
first issue. A generation of 0 refers to an entry that has been freshly
created.
This commit reworks the entry release to be safer concurrent-wise when
an entry targetted for release is found as already existing:
- If marked as dropped without refount at 0, free it.
- In other cases, ignore it. The entry has been reinitialized or
created again with the same key, and is still in use.
Sami has posted a basic test case (equivalent added in this commit) that
gives a way to reproduce the assertion failure on backend exit, which is
a consequence of the incorrect handling of entry releases.
Reported-by: Sami Imseih <samimseih.pg@gmail.com>
Reviewed-by: Sami Imseih <samimseih.pg@gmail.com>
Discussion: https://postgr.es/m/CAN12+YJXE+rEOVo74uM5Ofbj_t_m0DHvd_Mo4ppeHV=aDvvXoQ@mail.gmail.com
Backpatch-through: 15
Dumps taken from unpatched versions (pre-v14) will not have the AS
keyword that ruleutils.c now prints, causing a harmless diff in dump
output. Adjust the cross-version upgrade's old dump to make it match.
While I'm here, move the stanza that I hastily put in a non-optimal
place in commit f8ab6c622. It's worth fixing because it messed up
the logical structure of adjust_database_contents, making it
un-obvious where to put future additions.
Per buildfarm.
set_deparse_context_plan() set the aliases for OLD and NEW in
RETURNING when EXPLAIN printed a ModifyTable, but that's the
wrong place: they should be set in set_deparse_plan() like other
plan-node-specific fields. So they were not changed when
push_ancestor_plan() moved to the ancestor ModifyTable to print a
Param of a SubPlan in its RETURNING list, and the aliases of some
previously-printed ModifyTable could be used instead. It seems
possible that we'd also fail to print an alias at all, since
get_variable() is unfazed by obtaining a NULL refname. That
shouldn't happen for RETURNING variables, so complain.
Author: Henson Choi <assam258@gmail.com>
Reviewed-by: Tom Lane <tgl@sss.pgh.pa.us>
Discussion: https://postgr.es/m/CAAAe_zAUb_XydapJqUk7O8o2crJa7xbxX=KvSi3nY9h98OcN7g@mail.gmail.com
Backpatch-through: 18
Previously we called strtol() with base 0, so that a leading zero
digit caused the number to be read as octal. This is not documented
because it was not the original intent: before commit 7ccb6dc2d, this
code used atoi() which would have read the number as decimal always.
Use base 10 instead to restore the original behavior.
Also reject a bare minus sign consistently. Per POSIX, strtol()
"may" set errno for that case but doesn't have to, and if it doesn't
then we'd not have noticed the bad syntax but read the value as 0.
Although this change restores old behavior, 7ccb6dc2d was a long
time ago, so it's at least possible that someone out there is
expecting the octal behavior. So it doesn't seem like a great
idea to put this into stable branches. But I (tgl) judge it
not too late for v19.
Author: Marko Grujic <marko.grujic@enterprisedb.com>
Reviewed-by: Manu <manuelreyesbravo@gmail.com>
Reviewed-by: Tom Lane <tgl@sss.pgh.pa.us>
Discussion: https://postgr.es/m/CAOvwyF3=brZoxwcwHRQqvLjuqSqxiTN7RXpKMNHXJ1WAS5GQ2Q@mail.gmail.com
Backpatch-through: 19
isSimpleNode() and related code treated every XmlExpr as
self-delimiting. Most of them are, because they look like function
calls with some extra decoration. But "x IS DOCUMENT" is not: it is
a postfix test, like IS NULL. Printed as the operand of another
operator, or as the operand of a postfix test, it was therefore not
parenthesized and could not be parsed back, or was parsed as something
unintended. Under-parenthesization occurred even in non-pretty-print
cases, so that this could break dump/restore workflows.
To fix, treat IS DOCUMENT as neither simple nor function-like, and
give it the same treatment as other postfix tests.
Also pass the single argument of IS DOCUMENT, not the argument list,
to get_rule_expr_paren(). This avoids unnecessary parenthesization
of the argument.
Author: Henson Choi <assam258@gmail.com>
Reviewed-by: Prashant Chinnam <crprashant@hotmail.com>
Reviewed-by: Tom Lane <tgl@sss.pgh.pa.us>
Discussion: https://postgr.es/m/CAAAe_zDuPSkc8ihCctV67Lxbqock+S4wqCHHo8xxZ1t4teXo0g@mail.gmail.com
Backpatch-through: 14
When a number format has a decimal point with no digit positions
after it (e.g. '9.' or 'FM999.PL'), NUM_numpart_to_char() copied the
terminating '\0' of the number string into the output as if it were a
digit, resulting in apparent truncation of the output; for example,
a sign or literal text following the decimal point would disappear.
Fix it by not copying the '\0'.
This is clearly a bug, but given the lack of field complaints,
people who have run into it may think it's expected behavior,
and may even have written applications that depend on it.
Therefore it seems unwise to change it in stable branches.
I (tgl) think it's not too late to fix it in v19 though.
Bug: #19771
Author: Gowtham Reddy <gowthamdoma18@gmail.com>
Reviewed-by: Tom Lane <tgl@sss.pgh.pa.us>
Discussion: https://postgr.es/m/19771-4f173b891525020c@postgresql.org
Backpatch-through: 19
If the target relation of UPDATE, DELETE, or MERGE has an alias,
we printed that "bare" (without AS or forced quoting), the same
as we do in FROM. However, if that alias is "set", there's an
ambiguity with the SET keyword that must follow the relation
specification. The parser would choose the wrong interpretation,
so that the dumped query would fail to reload. The least surprising
and most future-proof fix seems to be to always print AS in these
contexts, as we already did in INSERT.
Also, we were insufficiently careful about parenthesizing the
left-hand argument of a SubLink (IN, op ANY/ALL, or row comparison),
so that the printed expression might not parse back with the intended
meaning depending on operator precedence. This is only a problem in
"pretty-print" mode, since non-pretty mode fully parenthesizes every
operator. So it isn't a hazard for pg_dump, but it still seems like
something to fix. To do so, switch to using get_rule_expr_paren.
Presently that will add parens in some cases where it wouldn't really
be necessary, but that's better than the reverse, and it arguably
is helpful for readability anyway.
Author: Henson Choi <assam258@gmail.com>
Reviewed-by: Tom Lane <tgl@sss.pgh.pa.us>
Discussion: https://postgr.es/m/CAAAe_zDqXin5NeB5atRCbZqUdcMOeyXRqhan+tVp0F30OCURnw@mail.gmail.com
Discussion: https://postgr.es/m/CAAAe_zCy+D45Qd_br484F+V34y0BVA=hp_7cb=u6HysyMkE3UQ@mail.gmail.com
Backpatch-through: 14
The pattern seems to be to EXPLAIN the query to ensure we get the
expected partition-wise aggregate plan, then run the same query to check
the results are what we expect. A couple of places didn't use the same
query between the EXPLAIN and the checking of the results, which
effectively voided the plan check.
Additionally, a comment claimed array_agg() didn't support partial mode,
but that was made not true in 16fd03e95.
Author: Matthias van de Meent <boekewurm@gmail.com>
Reviewed-by: Pierre Forstmann <pierre.forstmann@gmail.com>
Reviewed-by: David Rowley <dgrowleyml@gmail.com>
Discussion: https://postgr.es/m/CAEze2WgjJUiqV-VZVrWqYz+ETLtd4oKk2gbvBWxf1Fo0is5XVQ@mail.gmail.com
Backpatch-through: 14
AllocSet contexts with different maxBlockSize values can have the same
freeListIndex. When reusing a context from one of the context_freelists,
we updated maxBlockSize, but we failed to adjust allocChunkLimit, even
though that ordinarily depends on maxBlockSize. This could result in
some allocations being put on an external block when they shouldn't have
been, resulting in excessive malloc() calls, or if the allocChunkLimit was
left set higher than it should be, allocations could have resulted in
memory not being free()'d after pfree() due to the chunk having been put
on a multi-chunk block (these would still later be cleaned up when the
context is reset, so no memory leaks).
The repercussions of this are only inefficient use of memory and perhaps
some performance instability depending on which context happened to be
top of the freelist when a new AllocSet context was created.
To fix, we move the allocChunkLimit calculation code into a helper
function and call that to reset the allocChunkLimit when reusing a
context.
Author: Chao Li <li.evan.chao@gmail.com>
Reviewed-by: Donghang Lin <donghanglin@gmail.com>
Reviewed-by: David Rowley <dgrowleyml@gmail.com>
Discussion: https://postgr.es/m/1D54961E-6BEF-411A-BFAD-610C3B18D7E7@gmail.com
Backpatch-through: 14
Commit a83586b554 decided to scribble on the caller's fcinfo but failed
to take into account that const arguments are written once at expression
init and assumed never to change.
The use_json_null branch overwrote fcinfo->args[2] with a jsonb null
built in the per-tuple expression context. In later rows,
args[2].isnull is still false and args[2].value still holds that
pointer, but the per-tuple context has been reset. Later rows find
args[2].isnull = false, short-circuit past the NULL handling at the top
of the function, and jump straight to jsonb_set with the stale pointer
regardless of their own null_value_treatment argument, which is never
read.
To fix, call jsonb_set through its own call frame instead. Arguments 0,
1 and 3 were already checked non-NULL above, so we can use
DirectFunctionCall4.
Add regression tests covering use_json_null followed by delete_key and
return_target, and a multi-row default-treatment query.
Reported-by: Evan Si <evsi@amazon.com>
Discussion: https://postgr.es/m/CANWCAZY-PS=ev8TiiV1j=NoR2xLopEL8fDq0fyQZahHiP5b+Pw@mail.gmail.com
Backpatch-through: 14
When remove_useless_result_rtes() removes an RTE_RESULT, any
PlaceHolderVars that were to be evaluated there must be evaluated
somewhere else. If the RTE_RESULT is the nullable side of an outer
join, there is no other place that would preserve the PHVs' nulling
behavior, so find_dependent_phvs() refuses the removal if any such PHV
exists anywhere in the query. If the RTE_RESULT is a child of an
inner join, its PHVs can instead be evaluated at the join of all its
sibling rels, unless a sibling needs them before that join is formed.
find_dependent_phvs_in_jointree() checks for that by looking for such
PHVs in the join quals and LATERAL RTEs of the siblings.
That misses a PHV that is evaluated at some but not all of the sibling
rels and contains one of the RTE_RESULT's PHVs. Such a PHV can appear
anywhere in the query, such as in the targetlist. This can happen
when pulled-up expressions are wrapped in PHVs because of outer joins
or grouping sets. Once the RTE_RESULT is removed, the outer PHV needs
a value that is only available at a join above its own evaluation
level. The outer PHV's rel thus becomes laterally dependent on the
other sibling rels. If one of those rels laterally references it in
turn, we end up with a rel that is laterally dependent on itself,
causing an assertion failure in create_lateral_join_info().
To fix, also search the whole query for PHVs that are evaluated at
some but not all of the sibling rels, and refuse to remove the
RTE_RESULT if any of them contains one of its PHVs. An outer PHV that
is evaluated at all of the sibling rels is not a problem, since the
moved PHV is available there, so we don't block removal in that common
case.
Back-patch to all supported branches.
Author: Richard Guo <guofenglinux@gmail.com>
Reviewed-by: Ayush Tiwari <ayushtiwari.slg01@gmail.com>
Discussion: https://postgr.es/m/CAMbWs4_hLOsW8_SePp_3EmYTLX8Oc4zEZEvypyPRszpoMhDdGQ@mail.gmail.com
Backpatch-through: 14
The following commits are reverted:
* 092f3c63ef, libpq: Add "servicefile" connection option
* 6b1c4d326b, psql: Add variable SERVICEFILE
* 90354030b8, libpq: Fix "servicefile" after fallback to system
service file
After more review, PGSERVICEFILE is sufficient for the purpose of
specifying a per-session service file. Some details around this feature
will need more attention before being actually usable, if we ever get
down to it.
One independent piece that has been committed during the v19 development
cycle is df286a5b83, which is kept in the tree as that was an
unrelated change.
Reported-by: Sehrope Sarkuni <sehrope@jackdb.com>
Discussion: https://postgr.es/m/CAH7T-aq+U6GDPO8mr3Gga3KzxGpAE=BSLb38x+R8fvWjzF-H8w@mail.gmail.com
Backpatch-through: 19
An assertion within nbtree preprocessing's _bt_mark_scankey_required
function expects the first member key of a row compare to have the same
strategy as its header key, which its caller passes. But the first row
member key won't necessarily have been commuted in cases where the row
compare is unsatisfiable due to its first member key having a NULL
argument. This led to spurious assertion failures in the single key
fast path call to _bt_mark_scankey_required.
To fix, always commute the row members, even when the row compare is
unsatisfiable. That way the header and its members will always agree on
strategy.
Oversight in commit ec986020, which improved nbtree's unsatisfiable
RowCompare detection.
Author: Peter Geoghegan <pg@bowt.ie>
Backpatch-through: 18
make_outerjoininfo() decides whether outer join identity 3 applies
between an outer join and a lower one in its LHS by checking that the
join clause is strict for the lower join's RHS. It relied on the
clause then necessarily referencing that RHS. That is not so when the
strictness comes through a PlaceHolderVar: find_nonnullable_rels()
looks inside the PHV's expression and reports rels it references
laterally, while pull_varnos() reports only where the PHV is
evaluated.
In that case we marked the two joins as commutable although the clause
does not use the lower join's RHS at all. Joinrels for the same set
of base rels could then be built both with and without the upper
join's relid, leading to an assertion failure in
standard_join_search(). The bogus commutation also caused an extra
clone of the upper join's clause to be generated, which could be
applied at a higher join in addition to the clone already applied
below it. In non-assert builds, this could lead to planner errors
such as "variable not found in subplan target lists".
To fix, ignore rels that the clause is strict for but does not
reference, consistent with clause_relids treating a PHV as computed at
its ph_eval_at.
Back-patch to v16, where this commutation logic was introduced.
Author: Richard Guo <guofenglinux@gmail.com>
Reviewed-by: Tender Wang <tndrwang@gmail.com>
Discussion: https://postgr.es/m/CAMbWs49xvh4-uPv3rxWkk_rtMT1ic+psHvEAxaFEXu2Rf-32Tw@mail.gmail.com
Backpatch-through: 16
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
Commit 67c20979ce made end-of-recovery processing emit a process signal
barrier and wait for all processes to refresh their cached logical WAL
logging state. This can hold up promotion indefinitely if a backend
cannot process the barrier, for example while blocked sending data to
a client that has stopped reading.
At wal_level=logical, logical WAL logging is enabled regardless of the
cached state, so no cache synchronization is needed. Skip emitting and
waiting for the barrier in this case.
When logical decoding will be disabled after recovery, stale caches
can only cause extra logical information to be written to WAL, which is
safe. Still emit the barrier so processes update their caches
asynchronously, but no longer wait for it to complete.
These changes allow promotion to finish without waiting for unresponsive
backends to process this barrier in either case. Retain the wait in
other cases, even if the shared state has not changed, to prevent
processes with stale caches from omitting required logical information
after recovery.
Reported-by: shihao zhong <zhong950419@gmail.com>
Author: Fujii Masao <masao.fujii@gmail.com>
Author: Manuel Reyes Bravo <manuelreyesbravo@gmail.com>
Reviewed-by: Masahiko Sawada <sawada.mshk@gmail.com>
Discussion: https://postgr.es/m/CAGRkXqToBk9c2sPAgha-kCgxJhVwn90j1sCyUXrvydq51LKAqA@mail.gmail.com
Backpatch-through: 19
On v18 and below, if the newest multixid's members end exactly at the
the 32-bit offset wraparound point, the next offset in the control
file is 0. GetOldMultiXactIdSingleMember() treated that as an invalid
entry and dropped the multixid's members, so after upgrade accessing
the tuple failed with "MultiXact N has invalid offset".
To fix, only check for zero offset when it was read from the next
multixid's slot, like GetMultiXactIdMembers() did in v18. This is
unlikely to happen in practice, but better to get it right.
The bug came with commit bd8d9c9bdf in v19.
Author: Zsolt Parragi <zsolt.parragi@percona.com>
Discussion: https://www.postgresql.org/message-id/CAN4CZFM8BCazGz6s1Vn5tMEXYiXE2fi8T2rwmwXSty5gD+h9rA@mail.gmail.com
Backpatch-through: 19
We must tell init about each role name we plan to connect as,
else SSPI auth fails. Similar to previous patches such as
e3fbc9a8d.
Oversight in a39e53a70, per buildfarm member unicorn.
The documentation stated that existing connections would continue to
function normally after the OOM killer terminated the postmaster.
In fact, those connections eventually terminate when server processes
detect the postmaster's death.
Clarify that the OOM killer can terminate either the postmaster or a
child process. If the postmaster is killed, the server must be restarted.
If a child process is killed, PostgreSQL typically terminates the
remaining connections and either recovers automatically or shuts down,
depending on restart_after_crash.
Backpatch to all supported versions.
Author: Takeshi Ideriha <iderihatakeshi@gmail.com>
Reviewed-by: Manuel Reyes Bravo <manuelreyesbravo@gmail.com>
Reviewed-by: Fujii Masao <masao.fujii@gmail.com>
Discussion: https://postgr.es/m/ce269321-a7f4-48f4-a9ca-a4a4983b9fd6@gmail.com
Backpatch-through: 14
The description of autovacuum_max_parallel_workers did not specify
where it can be set or how it relates to the per-table
autovacuum_parallel_workers storage parameter.
Clarify that it can be set only in postgresql.conf or on the server
command line, and that the limit can be reduced for individual tables.
Add a link to the autovacuum_parallel_workers storage parameter.
Backpatch to v19, where parallel autovacuum was introduced.
Author: Baji Shaik <baji.pgdev@gmail.com>
Reviewed-by: Sami Imseih <samimseih.pg@gmail.com>
Reviewed-by: Chao Li <li.evan.chao@gmail.com>
Reviewed-by: Fujii Masao <masao.fujii@gmail.com>
Discussion: https://postgr.es/m/CA+fm-RMmbUUPWJShxkhoB7SeiQ7XWw3YsVnHpMzp1Y2i_gtwig@mail.gmail.com
Backpatch-through: 19
find_var_for_subquery_tle() searches a subquery rel's targetlist for a
Var referencing a given subquery output column, and asserted that any
Var it finds there belongs to that rel. That does not hold for an
appendrel child. set_append_rel_size() builds the child's targetlist
by translating the parent's, and as noted there the result can contain
arbitrary expressions. In particular, when a member of a LATERAL UNION
ALL subquery is pulled up and its output is a lateral reference, the
translated expression is a Var of the laterally referenced rel. If the
child is itself a subquery with sorted paths, convert_subquery_pathkeys()
would trip the Assert.
In non-assert builds, the function went on to compare such a Var's
varattno with the subquery column's resno, and so could match a Var of
an unrelated rel.
To fix, ignore Vars that do not belong to the subquery rel, the same
way set_rel_width() already does. Also correct the adjacent comment,
which claimed that the only non-Var entries are placeholders.
Back-patch to all supported branches.
Author: Richard Guo <guofenglinux@gmail.com>
Reviewed-by: shihao zhong <zhong950419@gmail.com>
Discussion: https://postgr.es/m/CAMbWs48fFiPi7Q5yJCoM_nAs47zd5XgvPHd9nZyM_ewq+k_jpQ@mail.gmail.com
Backpatch-through: 14
Effectively an oversight in 29f6a959cf. The code went to the trouble of
doing set_sentinel() but no code ever checked it was still set during
BumpCheck(). Since bump context types are used in a handful of places,
it very much makes sense to backpatch this to where bump.c was added so we
can more easily discover any memory stomping bugs.
Reported-by: shihao zhong <zhong950419@gmail.com>
Author: David Rowley <dgrowleyml@gmail.com>
Discussion: https://postgr.es/m/CAGRkXqQxTCthDEnw_10OEGLVNMUeyE4So-K29jmn9nAkxtr1ow@mail.gmail.com
Backpatch-through: 17
0ca3b1697 added support for parallel TID Range Scans. The range of
blocks to scan is stored in the shared phs_startblock and phs_numblock
fields by the first participant to start the scan, but
table_block_parallelscan_reinitialize() didn't reset these, so every
rescan of the Gather reused the block range of the first execution.
This caused rows to be silently missed when a rescan used a different
TID range, e.g:
SELECT r.lo, (SELECT count(*) FROM t WHERE ctid >= (SELECT r.lo))
FROM (VALUES ('(30,0)'::tid), ('(10,0)'), ('(0,0)')) r(lo);
Here we fix this by resetting both fields in
table_block_parallelscan_reinitialize(). As a side effect, a rescanned
Parallel Seq Scan using synchronized scans now gets a new start
location, which is what a new scan would do anyway.
The existing rescan test didn't exercise this, so add one that does.
This was introduced in 0ca3b1697, so backpatch to v19.
Reported-by: Zsolt Parragi <zsolt.parragi@percona.com>
Author: Zsolt Parragi <zsolt.parragi@percona.com>
Author: David Rowley <dgrowleyml@gmail.com>
Reviewed-by: David Rowley <dgrowleyml@gmail.com>
Reviewed-by: shihao zhong <zhong950419@gmail.com>
Discussion: https://postgr.es/m/CAN4CZFOoyCQE483YVGg07_Vr2MjFCTiCLUHUAFCwKqJAE=PN6w@mail.gmail.com
Backpatch-through: 19
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
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
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
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
