libpqwalreceiver parses the WAL flush location returned by
IDENTIFY_SYSTEM using sscanf("%X/%X") into uint32 values. This
accepts overlong components and trailing characters, potentially
producing a different LSN from the one supplied by the upstream server.
Use pg_parse_lsn() instead, requiring one to eight hexadecimal digits
per component and no trailing characters, as for pg_lsn input. Reject
malformed locations with the existing protocol violation error.
Suggested-by: Ayush Tiwari <ayushtiwari.slg01@gmail.com>
Author: Hayato Kuroda <kuroda.hayato@fujitsu.com>
Reviewed-by: Jan Nidzwetzki <jan@planetscale.com>
Reviewed-by: Fujii Masao <masao.fujii@gmail.com>
Discussion: https://postgr.es/m/tencent_C6ED19DF1759CDD37E786F7EE0B5207D0009@qq.com
The replication command scanner accepts arbitrarily long hexadecimal
LSN components, but converts them with sscanf("%X/%08X") into uint32
values. An overlong high component can overflow, while an overlong low
component is truncated to its first eight digits. This can cause
START_REPLICATION to use a different WAL location from the one requested.
Use pg_parse_lsn() to validate and convert the complete LSN token,
requiring one to eight hexadecimal digits per component, as for pg_lsn
input. This applies to both physical and logical replication.
Reported-by: ♂π≌26218 <1991230470@qq.com>
Author: Ayush Tiwari <ayushtiwari.slg01@gmail.com>
Reviewed-by: Hayato Kuroda <kuroda.hayato@fujitsu.com>
Reviewed-by: Jan Nidzwetzki <jan@planetscale.com>
Reviewed-by: Fujii Masao <masao.fujii@gmail.com>
Discussion: https://postgr.es/m/tencent_C6ED19DF1759CDD37E786F7EE0B5207D0009@qq.com
The function is declared in pg_proc.dat a bigint as its second input
argument, but the function's code used PG_GETARG_INT32().
This is an issue in 32-bit architectures in stable branches (up to v18)
where bigint is still passed by reference, reporting bogus numbers.
Since 2a600a93c7 (v19 and HEAD), this is less of a problem as Datums
became 8-byte wide everywhere.
Oversight in ab0dfc961b.
Author: Markos Fountoulakis <markos@planetscale.com>
Reviewed-by: Chao Li <li.evan.chao@gmail.com>
Discussion: https://postgr.es/m/e791bbb7-af3a-4a91-a5bb-c744e5c3ef54@planetscale.com
Backpatch-through: 14
An unhandled ERROR while synchronizing a sequence (for example, when
SwitchToUntrustedUser() fails because the subscription owner cannot SET
ROLE to the sequence owner) makes the sequencesync worker exit and be
restarted, repeatedly, until the user resolves the problem, as with the
apply worker. The error message does not identify the sequence or
subscription responsible.
Add an error context callback around per-sequence validation and
synchronization in copy_sequences() so that such errors report the
schema-qualified sequence name and subscription name in CONTEXT.
Reported-by: Zsolt Parragi <zsolt.parragi@percona.com>
Author: Zhijie Hou <houzhijie22@gmail.com>
Reviewed-by: Ajin Cherian <itsajin@gmail.com>
Reviewed-by: Hayato Kuroda <kuroda.hayato@fujitsu.com>
Reviewed-by: vignesh C <vignesh21@gmail.com>
Reviewed-by: Zsolt Parragi <zsolt.parragi@percona.com>
Discussion: https://postgr.es/m/CAN4CZFMGiyksxVD1M7cc7_5E+evfi51xmDxw4uqdmvoeH53Ugw@mail.gmail.com
Backpatch-through: 19
The DROP DATABASE test in 035_standby_logical_decoding.pl created
slots with the prefix "drop_db_" but checked for their removal using
"drop_db". Sined names never existed, the checks passed
even if the actual slots remained.
Fix this by using the correct prefix when checking for slot removal.
Backpatch to v16, where this testce the check was introduced.
Author: Fujii Masao <masao.fujii@gmail.com>
Reviewed-by: Hayato Kuroda <kuroda.hayato@fujitsu.com>
Discussion: https://postgr.es/m/CAHGQGwEnuLZnFRF5CzK47n_hXT-iCXnzirxG2v_V-=QNMLkpEA@mail.gmail.com
Backpatch-through: 16
Buildfarm animals francolin and kestrel reported intermittent failures
in 035_standby_logical_decoding.pl.
The test checks that replaying DROP DATABASE on a standby terminates
pg_recvlogical with the expected recovery-conflict message and removes
the database's replication slots. To exercise this while pg_recvlogical
is streaming, the test must wait for streaming to start before dropping
the database on the primary.
Previously, the test treated an active slot in pg_replication_slots as
confirmation that pg_recvlogical was streaming. However, the slot can
become active before pg_recvlogical has entered streaming mode. If
DROP DATABASE is replayed during this window, pg_recvlogical can report
a connection-closed error instead of the recovery-conflict message,
causing the test to fail.
Run pg_recvlogical with --verbose and wait for "streaming initiated"
instead of checking whether the slot is active. This ensures that
pg_recvlogical has entered streaming mode before the database is
dropped on the primary.
Back-patch to v16, where this test was introduced.
Reported-by: Alexander Lakhin <exclusion@gmail.com>
Author: Hayato Kuroda <kuroda.hayato@fujitsu.com>
Reviewed-by: Alexander Lakhin <exclusion@gmail.com>
Reviewed-by: Bertrand Drouvot <bertranddrouvot.pg@gmail.com>
Reviewed-by: Fujii Masao <masao.fujii@gmail.com>
Discussion: https://postgr.es/m/a50ff89b-fb2c-4912-8f89-50dd0bd0916b@gmail.com
Backpatch-through: 16
Commit 1e707d3256 made COPY TO report a proper error for unlogged
tables during recovery. currtid2() has the same problem: it reads the
table directly without going through the planner, so on a standby it
failed with a low-level error for an unlogged table:
ERROR: could not open file "base/5/16384": No such file or directory
Check for this case and report the same error as SELECT. No test is
added, as currtid2() is an undocumented internal function.
Backpatch to all supported versions, like commit 1e707d3256.
Reported-by: Masahiko Sawada <sawada.mshk@gmail.com>
Author: Jim Jones <jim.jones@uni-muenster.de>
Discussion: https://postgr.es/m/CAD21AoDNa7pL760_23AN_wYPMkWXCt+Kw0_vJAcci4R5DoCQGg@mail.gmail.com
Backpatch-through: 14
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
When all shared buffers examined are pinned, a query can fail with
"no unpinned buffers available". This can happen when shared_buffers
is too small for the workload, but the error was reported as an internal
error, making a resource shortage look like a server bug.
Replace elog() with ereport() to report this condition with SQLSTATE
53000 (insufficient_resources) instead of XX000 (internal_error),
matching the corresponding local-buffer error. Reword both messages
to clarify whether shared or local buffers are involved.
Author: Ma Xueting <ma@sraoss.co.jp>
Reviewed-by: Pierre Forstmann <pierre.forstmann@gmail.com>
Reviewed-by: Matthias van de Meent <boekewurm+postgres@gmail.com>
Reviewed-by: solai v <solai.cdac@gmail.com>
Reviewed-by: Fujii Masao <masao.fujii@gmail.com>
Discussion: https://postgr.es/m/TYWP286MB2571C92032EE9D1B8F2BFACFEF812@TYWP286MB2571.JPNP286.PROD.OUTLOOK.COM
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
... in fe_memutils.c's pnstrdup().
strnlen() does return size_t, so it seems better to use the Size input
parameter to store the return value rather than demote it to int and
risk larger values being truncated.
Pushing only to master as this doesn't seem like an actual problem.
Fixing will hopefully prevent future LLM-triggered reports about this.
Author: Chao Li <li.evan.chao@gmail.com>
Discussion: https://postgr.es/m/0E717E09-809F-4BC4-B3B1-36BE729DBFAE@gmail.com
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
byteaout and pg_mcv_list_out depend on bytea_output GUC, while
float4out, float8out, and the geometric type output functions depend
on extra_float_digits GUC. The BRIN minmax-multi summary output
function also calls the summarized type's output function, so its result
can depend on the type's output settings.
Mark these functions STABLE instead of IMMUTABLE to reflect their
dependence on session settings. This prevents them from being used in
expressions that require immutability, such as expression indexes,
generated columns, and partition keys.
Do not backpatch, since this changes catalog entries and can break
existing schemas that rely on these output functions being immutable.
Bump catalog version.
Reported-by: 放空 <jj-zhang25@mails.tsinghua.edu.cn>
Author: Fujii Masao <masao.fujii@gmail.com>
Reviewed-by: Peter Eisentraut <peter@eisentraut.org>
Discussion: https://postgr.es/m/19644-d6db3fb5e5ad92f4@postgresql.org
This has no effect on normal use of this repository. The scanner clones
this repository and locates this file. While we could store it in the
scanner configuration, storing it here simplifies keeping CI and this in
sync. References to postgresql/project.yaml are forward-looking; see
github.com/nmisch/oss-scanner/blob/enroll-postgresql/projects/postgresql/project.yaml
for the planned future content. Claude wrote some of this.
Reviewed-by: Sehrope Sarkuni <sehrope@jackdb.com>
Reviewed-by: Ayush Tiwari <ayushtiwari.slg01@gmail.com>
Reviewed-by: Andres Freund <andres@anarazel.de>
Discussion: https://postgr.es/m/20261017032215.26.noahmisch@microsoft.com
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
These checks compile headers, so all the headers they might include
need to have been generated. The makefile target does that, but the
meson implementation didn't do that. (Arguably a bit of a bug in
commit e82fc27e095.) Add the required dependencies so that meson
compile headerscheck works in an unbuilt tree as well (as is
documented).
Reviewed-by: Nazir Bilal Yavuz <byavuz81@gmail.com>
Discussion: https://www.postgresql.org/message-id/flat/e52e2c29-4834-4c64-a200-eddcba38fb4d%40eisentraut.org
This function attribute tells the compiler that the function will
never return NULL. This allows the compiler to do better static
analysis and lets it optimize the caller given the non-NULL guarantee.
The attribute is placed in front of the declaration, rather than after
it, so that it can also expand to the MSVC annotation _Ret_notnull_,
which is only valid in that position.
Only functions that raise an error on allocation failure are marked.
Functions that take a flags argument, such as palloc_extended() and
palloc_mul_extended(), can return NULL when MCXT_ALLOC_NO_OOM is
passed and are therefore not marked.
Author: Tristan Partin <tristan@partin.io>
Discussion: https://www.postgresql.org/message-id/flat/DJROV5LUO7B5.DIWXYC0101KD%40partin.io
Fix some issues involving volatile that would be triggered by
-Wcast-qual.
For PMSignalState, move the volatile qualifiers to the PMSignalData
fields that need it rather than making the whole structure have it.
For the MemSet() calls, do the initialization honestly with a loop
instead.
Note that the MemSet() calls would be normal "discards qualifier"
warnings if memset() (or another function with a prototype, not a
macro) were used.
Reviewed-by: Rui Zhao <zhaorui126@gmail.com>
Reviewed-by: Andres Freund <andres@anarazel.de> (older version)
Discussion: https://www.postgresql.org/message-id/c76b72dd-5736-453a-b698-6968d42e4768%40eisentraut.org
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
After 44539786b8, variable-sized stats updates are batched in one
backend. A later safe_psql() can check the counters before that backend
has finished flushing its pending stats during shutdown, so force a
flush before checking them.
This is only needed before this report check because
test_custom_stats_var_update() uses pgstat_prep_pending_entry(), leaving
counter updates as backend-local pending stats.
Per buildfarm member mamba. This could be reproduced locally by slowing
down pgstat_shutdown_hook().
Reported-by: Tom Lane <tgl@sss.pgh.pa.us>
Author: Sami Imseih <samimseih.pg@gmail.com>
Discussion: https://postgr.es/m/193326.1792282194@sss.pgh.pa.us
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
Previously, REPACK and CLUSTER silently skipped foreign-table partitions
when processing a partitioned table, leaving users unaware that some
partitions had not been processed. In contrast, VACUUM FULL warns when
skipping such partitions.
Emit a warning for each skipped foreign-table partition. Inspect the
table's partition tree even when an index is specified, because foreign
tables have no indexes and therefore do not appear in the index's
partition tree. Check privileges before issuing the warning, so that
users without the required privileges receive a permission warning
instead, as with VACUUM FULL.
Note that database-wide REPACK and CLUSTER continue to skip foreign
tables silently, consistent with database-wide VACUUM FULL.
Author: Zsolt Parragi <zsolt.parragi@percona.com>
Reviewed-by: Antonin Houska <ah@cybertec.at>
Reviewed-by: Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com>
Reviewed-by: shihao zhong <zhong950419@gmail.com>
Reviewed-by: Surya Poondla <suryapoondla4@gmail.com>
Reviewed-by: solai v <solai.cdac@gmail.com>
Reviewed-by: Fujii Masao <masao.fujii@gmail.com>
Discussion: https://postgr.es/m/CAN4CZFNXFdkmOCnt8m=K_A4i3LQC+59nx59Y=5bsH4vehQHTTw@mail.gmail.com
Buildfarm member skink reported failures in
053_standby_login_event_trigger.pl. The checks that dathasloginevt was
cleared on the primary and that the cleared flag reached the standby
both found the flag still set.
The test assumed that the first login after dropping the trigger would
clear the flag. However, EventTriggerOnLogin() acquires the database
lock conditionally to avoid delaying connections for this cleanup.
Concurrent connection startup, including that of an autovacuum worker,
can hold a conflicting lock and cause the cleanup to be skipped. The
standby correctly retained the flag because it had not been cleared
on the primary.
Fix this by using poll_query_until() to retry with fresh connections
to the test database until the flag is cleared, giving the login cleanup
repeated opportunities to acquire the lock.
Backpatch to v17, like commit bec61f5935, which added this test.
Reported-by: Alexander Lakhin <exclusion@gmail.com>
Author: Ayush Tiwari <ayushtiwari.slg01@gmail.com>
Reviewed-by: Fujii Masao <masao.fujii@gmail.com>
Discussion: https://postgr.es/m/09561429-50c1-4d06-a262-300c7a845175@gmail.com
Backpatch-through: 17
Commit 67c20979ce made end-of-recovery processing emit a process signal
barrier and wait for all processes to refresh their cached logical WAL
logging state. This can hold up promotion indefinitely if a backend
cannot process the barrier, for example while blocked sending data to
a client that has stopped reading.
At wal_level=logical, logical WAL logging is enabled regardless of the
cached state, so no cache synchronization is needed. Skip emitting and
waiting for the barrier in this case.
When logical decoding will be disabled after recovery, stale caches
can only cause extra logical information to be written to WAL, which is
safe. Still emit the barrier so processes update their caches
asynchronously, but no longer wait for it to complete.
These changes allow promotion to finish without waiting for unresponsive
backends to process this barrier in either case. Retain the wait in
other cases, even if the shared state has not changed, to prevent
processes with stale caches from omitting required logical information
after recovery.
Reported-by: shihao zhong <zhong950419@gmail.com>
Author: Fujii Masao <masao.fujii@gmail.com>
Author: Manuel Reyes Bravo <manuelreyesbravo@gmail.com>
Reviewed-by: Masahiko Sawada <sawada.mshk@gmail.com>
Discussion: https://postgr.es/m/CAGRkXqToBk9c2sPAgha-kCgxJhVwn90j1sCyUXrvydq51LKAqA@mail.gmail.com
Backpatch-through: 19
On v18 and below, if the newest multixid's members end exactly at the
the 32-bit offset wraparound point, the next offset in the control
file is 0. GetOldMultiXactIdSingleMember() treated that as an invalid
entry and dropped the multixid's members, so after upgrade accessing
the tuple failed with "MultiXact N has invalid offset".
To fix, only check for zero offset when it was read from the next
multixid's slot, like GetMultiXactIdMembers() did in v18. This is
unlikely to happen in practice, but better to get it right.
The bug came with commit bd8d9c9bdf in v19.
Author: Zsolt Parragi <zsolt.parragi@percona.com>
Discussion: https://www.postgresql.org/message-id/CAN4CZFM8BCazGz6s1Vn5tMEXYiXE2fi8T2rwmwXSty5gD+h9rA@mail.gmail.com
Backpatch-through: 19
We must tell init about each role name we plan to connect as,
else SSPI auth fails. Similar to previous patches such as
e3fbc9a8d.
Oversight in a39e53a70, per buildfarm member unicorn.
The documentation stated that existing connections would continue to
function normally after the OOM killer terminated the postmaster.
In fact, those connections eventually terminate when server processes
detect the postmaster's death.
Clarify that the OOM killer can terminate either the postmaster or a
child process. If the postmaster is killed, the server must be restarted.
If a child process is killed, PostgreSQL typically terminates the
remaining connections and either recovers automatically or shuts down,
depending on restart_after_crash.
Backpatch to all supported versions.
Author: Takeshi Ideriha <iderihatakeshi@gmail.com>
Reviewed-by: Manuel Reyes Bravo <manuelreyesbravo@gmail.com>
Reviewed-by: Fujii Masao <masao.fujii@gmail.com>
Discussion: https://postgr.es/m/ce269321-a7f4-48f4-a9ca-a4a4983b9fd6@gmail.com
Backpatch-through: 14
The description of autovacuum_max_parallel_workers did not specify
where it can be set or how it relates to the per-table
autovacuum_parallel_workers storage parameter.
Clarify that it can be set only in postgresql.conf or on the server
command line, and that the limit can be reduced for individual tables.
Add a link to the autovacuum_parallel_workers storage parameter.
Backpatch to v19, where parallel autovacuum was introduced.
Author: Baji Shaik <baji.pgdev@gmail.com>
Reviewed-by: Sami Imseih <samimseih.pg@gmail.com>
Reviewed-by: Chao Li <li.evan.chao@gmail.com>
Reviewed-by: Fujii Masao <masao.fujii@gmail.com>
Discussion: https://postgr.es/m/CA+fm-RMmbUUPWJShxkhoB7SeiQ7XWw3YsVnHpMzp1Y2i_gtwig@mail.gmail.com
Backpatch-through: 19
All other callers of sentinel_ok() pass the palloc'd address to the
function and the requested size of the allocation as the 2nd parameter.
Here we adjust 3 locations that were passing the MemoryChunk and adding on
sizeof(MemoryChunk) onto the byte count. Effectively, it's the same byte,
but a strange way to calculate it.
Author: David Rowley <dgrowleyml@gmail.com>
Reviewed-by: shihao zhong <zhong950419@gmail.com>
Discussion: https://postgr.es/m/CAApHDvq7HyLWX27XCSf5BmijeObV-QUF6CCbKAW00LxFPc71Ag@mail.gmail.com
