If an expression containing an aggregate is evaluated above the plan
node that computes the aggregate, as happens with window functions or
with expressions postponed to above the final sort, setrefs.c replaces
the Aggref or WindowFunc with a Var referencing the lower node's
output. For SQL/JSON aggregates such as JSON_ARRAYAGG and
JSON_OBJECTAGG, deparsing the containing JsonConstructorExpr then
failed with "invalid JsonConstructorExpr underlying node type", since
get_json_agg_constructor() did not expect a Var there.
Fix by resolving the Var back to the underlying Aggref or WindowFunc
and deparsing the constructor as if the aggregate were computed at the
current node. The JsonConstructorExpr retains the RETURNING clause
and the ABSENT/NULL ON NULL and WITH UNIQUE options, and the arguments
come from the resolved aggregate, so the original JSON aggregate
syntax is reproduced in full. This mirrors how get_agg_expr() already
looks through such a Var when deparsing a combining aggregate.
Reported-by: Thom Brown <thom@linux.com>
Author: Richard Guo <guofenglinux@gmail.com>
Discussion: https://postgr.es/m/CAA-aLv5QYTaMOk=Qhv6cgwceeHETZV8YJvWZ_rH+yVZCuchATA@mail.gmail.com
Backpatch-through: 16
The previous commit strengthened a workaround for a hang when large
messages are split across TLS records/GSS tokens. Because that
workaround is implemented in libpq internals, it can only help us when
libpq itself is polling on the socket. In nonblocking situations,
where the client above libpq is expected to poll, the same bugs can
show up.
As a contrived example, consider a large protocol-2.0 error coming
back from a server during PQconnectPoll(), split in an odd way across
two records:
-- TLS record (8192-byte payload) --
EEEE[...repeated a total of 8192 times]
-- TLS record (8193-byte payload) --
EEEE[...repeated a total of 8192 times]\0
The first record will fill the first half of the libpq receive buffer,
which is 16k long by default. The second record completely fills the
last half with its first 8192 bytes, leaving the terminating NULL in
the OpenSSL buffer. Since we still haven't seen the terminator at our
level, PQconnectPoll() will return PGRES_POLLING_READING, expecting to
come back when the server has sent "the rest" of the data. But there
is nothing left to read from the socket; OpenSSL had to pull all of
the data in the 8193-byte record off of the wire to decrypt it.
A real server would probably not split up the records this way, nor
keep the connection open after sending a fatal connection error. But
servers that regularly use larger TLS records can get the libpq
receive buffer into the same state if DataRows are big enough, as
reported on the list. While the PostgreSQL server doesn't use larger
TLS records like that, other non-PostgreSQL servers that implement the
wire protocol are known to do that, as well as proxies that sit
between the server and the client
This is a layering violation. libpq makes decisions based on data in
the application buffer, above the transport buffer (whether SSL or
GSS), but clients are polling the socket below the transport buffer.
One way to fix this in a backportable way, without changing APIs too
much, is to ensure data never stays in the transport buffer. Then
pqReadData's postconditions will look similar for both raw sockets and
SSL/GSS: any available data is either in the application buffer, or
still on the socket.
Building on the prior commit, make pqReadData() to drain all pending
data from the transport layer into conn->inBuffer, expanding the
buffer as necessary. This is not particularly efficient from an
architectural perspective (the pqsecure_read() implementations take
care to fit their packets into the current buffer, and that effort is
now completely discarded), but it's hopefully easier to reason about
than a full rewrite would be for the back branches.
Author: Jacob Champion <jacob.champion@enterprisedb.com>
Reviewed-by: Mark Dilger <mark.dilger@enterprisedb.com>
Reviewed-by: solai v <solai.cdac@gmail.com>
Reported-by: Lars Kanis <lars@greiz-reinsdorf.de>
Discussion: https://postgr.es/m/2039ac58-d3e0-434b-ac1a-2a987f3b4cb1%40greiz-reinsdorf.de
Backpatch-through: 14
An extra check for pending bytes in the SSL layer has been part of
pqReadReady() for a very long time (79ff2e96d). But when GSS transport
encryption was added, it didn't receive the same treatment. (As
79ff2e96d notes, "The bug that I fixed in this patch is exceptionally
hard to reproduce reliably.")
Without that check, it's possible to hit a hang in gssencmode, if the
server splits a large libpq message such that the final message in a
streamed response is part of the same wrapped token as the split
message:
DataRowDataRowDataRowDataRowDataRowData
-- token boundary --
RowDataRowCommandCompleteReadyForQuery
If the split message takes up enough memory to nearly fill libpq's
receive buffer, libpq may return from pqReadData() before the later
messages are pulled out of the PqGSSRecvBuffer. Without additional
socket activity from the server, pqReadReady() (via pqSocketCheck())
will never again return true, hanging the connection.
Pull the pending-bytes check into the pqsecure API layer, where both
SSL and GSS now implement it.
Note that this does not fix the root problem! Third party clients of
libpq have no way to call pqsecure_read_is_pending() in their own
polling. This just brings the GSS implementation up to par with the
existing SSL workaround; a broader fix is left to a subsequent commit.
In preparation for the broader fix, this patch already changes the
*_read_pending() functions to return the number of bytes in the buffer
rather than just a boolean. The current callers don't need that, but
the subsequent fix will.
Author: Jacob Champion <jacob.champion@enterprisedb.com>
Discussion: https://postgr.es/m/CAOYmi%2BmpymrgZ76Jre2dx_PwRniS9YZojwH0rZnTuiGHCsj0rA%40mail.gmail.com
Backpatch-through: 14
ParallelBackupStart() stored _beginthreadex()'s return value as the
worker's thread handle without checking it. On failure that value is 0,
which would later reach WaitForMultipleObjects() as a null handle, caught
only by an Assert. The fork() path already calls pg_fatal() when it
fails; do the same for _beginthreadex(), as pgbench does.
Author: Bryan Green <dbryan.green@gmail.com>
Discussion: https://www.postgresql.org/message-id/8c712d76-ecf7-4749-a6d8-dddc01f298ec@gmail.com
Backpatch-through: 14
9d9c02ccd added code to allow the executor to stop early when processing
WindowAgg nodes where a monotonic window function starts producing
values that result in a pushed-down qual no longer matching, and will
never match again due to the window function's monotonic properties.
That commit requires a SupportRequestWFuncMonotonic to exist on the
window function and for it to detect when the function is monotonic. For
COUNT(ANY) and COUNT(*), the support function failed to consider some
cases where the WindowClause used EXCLUDE to exclude certain rows from
being aggregated. Some WindowClause definitions mean we aggregate rows
that come after the current row, and when processing those rows later,
if we EXCLUDE certain rows, the monotonic property can be broken.
Wrongly treating the COUNT(*) or COUNT(ANY) aggregate as monotonic could
lead to rows being filtered that should not be filtered from the result
set.
Another issue was that the support function for the COUNT aggregate
mistakenly thought that a WindowClause without an ORDER BY meant that
the results would be both monotonically increasing and decreasing, but
that's only true when in RANGE mode, where all rows are peers.
It is possible to support various cases that do have an EXCLUDE clause,
but getting the logic correct for the exact set of cases that are valid
is quite complex and would likely better be left for a future project.
Here, we mostly disable run condition pushdown when there is an EXCLUDE
clause unless the clause is for EXCLUDE CURRENT ROW, uses COUNT(*)
(rather than COUNT(ANY)), and the window aggregate has no FILTER clause.
Bug: #19533
Reported-by: Qifan Liu <imchifan@163.com>
Author: Chengpeng Yan <chengpeng_yan@outlook.com>
Author: David Rowley <dgrowleyml@gmail.com>
Reviewed-by: Richard Guo <guofenglinux@gmail.com>
Reviewed-by: John Naylor <johncnaylorls@gmail.com>
Discussion: https://postgr.es/m/19533-413a1014e5d0e766@postgresql.org
Backpatch-through: 15
transformJsonBehavior() coerced an ON EMPTY / ON ERROR DEFAULT
expression only when its type differed from the RETURNING type's OID.
When the base type matched but the RETURNING type carried a type
modifier (e.g. numeric(4,1) or varchar(3)), the coercion that enforces
the typmod was skipped, so the DEFAULT value could violate the
declared type:
SELECT JSON_VALUE(jsonb '{}', '$.a'
RETURNING numeric(4,1) DEFAULT 99999.999 ON EMPTY);
returned 99999.999, which 99999.999::numeric(4,1) would reject; the
value could even be stored into a numeric(4,1) column, as later
coercions trust its already-correct type label.
Fix by also coercing when the RETURNING type has a typmod, except for
a NULL constant. coerce_to_target_type() is a no-op when the typmod
already matches. The matching-OID short-circuit dates to 74c96699be.
Reported-by: Ewan Young <kdbase.hack@gmail.com>
Author: Ewan Young <kdbase.hack@gmail.com>
Discussion: https://postgr.es/m/CAON2xHPO9f4cAmyGn1mQ=VqoS7wN5rz4yOiqudxX78zninZpCw@mail.gmail.com
Backpatch-through: 17
Commit e7564ee8cd, which fixed base backup progress reporting on
backup failure, removed the external function basebackup_progress_done()
because it was no longer used in core. When that change was backpatched
to v15, it introduced an ABI break, which was reported by buildfarm
member crake.
This commit restores basebackup_progress_done() to preserve ABI
compatibility, even though it is no longer used in core, rather than
updating the .abi-compliance-history file. Because external backup
tools may still call this function.
Per buildfarm member crake.
Reported-by: Andrew Dunstan <andrew@dunslane.net>
Discussion: https://postgr.es/m/CAD5tBcJ+ktrEp=PT8Gq-f=8mA2cDtZMB-hDMV4mMJ+9V46qBeQ@mail.gmail.com
Backpatch-through: 15-18
ON SELECT rules must be named "_RETURN", while other kinds of rules
must not be; this ancient restriction is depended on by various client
code. We successfully enforced this convention in most places, but
ALTER RULE allowed renaming a non-SELECT rule to "_RETURN". Notably,
that would break dump/restore, since the eventual CREATE RULE command
would reject the name.
While at it, remove DefineQueryRewrite's hack to substitute "_RETURN"
for the convention that was used before 7.3. We dropped other
server-side code that supported restoring pre-7.3 dumps some time ago
(notably in e58a59975 and nearby commits), but this bit was missed.
Bug: #19543
Reported-by: Adam Pickering <adamkpickering@gmail.com>
Author: Tom Lane <tgl@sss.pgh.pa.us>
Discussion: https://postgr.es/m/19543-461228e77f3b32fc@postgresql.org
Backpatch-through: 14
gbt_var_consistent() handled the <> (BtreeGistNotEqual) strategy without
distinguishing leaf from internal pages, unlike every other strategy.
In particular, it tried to apply the datatype-specific f_eq method,
which is completely wrong since internal keys might not have the same
representation as leaf keys. This led to OOB reads and potentially
crashes, and most likely to wrong query results as well.
On leaf pages we can apply the inverse of what the Equal strategy does.
On internal pages, use a correct implementation of what the previous
code intended: we can descend if the query value equals both bounds,
*so long as the bounds aren't truncated*. With truncated bounds we
don't quite know the range of what's below, so we must always descend.
Adjust the code in gbt_num_consistent() to look similar, too. This
fixes a performance buglet in that there's no need to do two comparisons
on a leaf entry, but the main point is just to keep code consistency.
Reported-by: 王跃林 <violin0613@tju.edu.cn>
Author: Ayush Tiwari <ayushtiwari.slg01@gmail.com>
Reviewed-by: Tom Lane <tgl@sss.pgh.pa.us>
Discussion: https://postgr.es/m/AH*AvQCYKhQGVvPWi1GiU4oY.8.1781609375063.Hmail.3020001251@tju.edu.cn
Backpatch-through: 14
Commit ce146621 ensures that ERROR is raised if a session tries to read
pages of another session's temp table. But there is a corner case where
the other session's temp table is empty -- in this case the INSERT
command bypasses our checks and executes without any errors.
Such behavior is inconsistent and erroneous: it leaves an invalid buffer
in the temp buffers pool. Since the buffer was created for another
session's temp table, we get an error "no such file or directory" when
trying to flush it.
This commit fixes it by adding a RELATION_IS_OTHER_TEMP check in the
relation-extension path.
Backpatch to 16, because it is the first release after 31966b151e, which
introduced a separate local relation extension function
ExtendBufferedRelLocal(), which lacks of RELATION_IS_OTHER_TEMP() check.
As this fix introduces more checks to 013_temp_obj_multisession.pl, backpatch
the whole test script to 16.
Discussion: https://postgr.es/m/CAJDiXgiX2XZBHDNo%2BzBbvku%2BtchrUurvPRaN1_40mEQ1_sG90g%40mail.gmail.com
Author: Daniil Davydov <3danissimo@gmail.com>
Reviewed-by: Jim Jones <jim.jones@uni-muenster.de>
Reviewed-by: Imran Zaheer <imran.zhir@gmail.com>
Reviewed-by: ZizhuanLiu X-MAN <44973863@qq.com>
Backpatch-through: 16
Commit fb421231da extended \df to include procedures, but its tab
completion continued not to show procedures.
Update \df tab completion to include procedures as well.
Backpatch to all supported versions.
Author: Erik Wienhold <ewie@ewie.name>
Reviewed-by: Surya Poondla <suryapoondla4@gmail.com>
Reviewed-by: Fujii Masao <masao.fujii@gmail.com>
Discussion: https://postgr.es/m/10fbfdfe-80f6-4ef9-b8b3-f7be0eb53a50@ewie.name
Backpatch-through: 14
Previously, MultiXactId wraparound hints suggested dropping stale
replication slots. While that advice is appropriate for transaction ID
wraparound, where replication slots can hold back XID horizons,
it was misleading for MultiXactId wraparound. Following it could lead
users to drop replication slots unnecessarily without helping resolve
the MultiXactId wraparound condition.
MultiXact cleanup is not directly delayed by replication slots.
Instead, it depends on whether old MultiXactIds can still be seen
as live by running transactions.
This commit removes the replication slot advice from MultiXactId
wraparound hints, and documents that stale replication slots are
normally not relevant to resolving MultiXactId wraparound problems.
Backpatch to all supported branches.
BUG #18876
Reported-by: Haruka Takatsuka <harukat@sraoss.co.jp>
Author: Fujii Masao <masao.fujii@gmail.com>
Discussion: https://postgr.es/m/18876-0d0b53bad5a1f4c1@postgresql.org
Backpatch-through: 14
The jsonpath .decimal(precision[, scale]) method built its numeric
typmod by calling numerictypmodin() through DirectFunctionCall1(), which
can throw a hard error for an incorrect set of precision and/or scale
vaulues. This breaks the silent mode supported by this function, that
should not fail.
Most of the jsonpath code uses the soft error reporting to bypass
errors, which is what this fix does by avoiding a direct use of
numerictypmodin(). Its code is refactored to use a new routine called
make_numeric_typmod_safe(), able to take an error context in input.
numerictypmodin() sets no context, mapping to its previous behavior.
The jsonpath code sets or not a context depending on the use of the
silent mode. This result leads to some nice simplifications:
numerictypmodin() feeds on an array, we can now pass directly values for
the scale and precision.
Oversight in 66ea94e8e6.
Author: Ewan Young <kdbase.hack@gmail.com>
Discussion: https://postgr.es/m/CAON2xHMaigKABiyPBBq3Sjd3gp7uWMJXnnMHt=s85V1ij3KP1w@mail.gmail.com
Backpatch-through: 17
The float4 and float8 btree_gist opclasses compared keys with raw C
operators (==, <, >). IEEE 754 makes every comparison involving NaN
false, so GiST disagreed with the regular float comparison operators
and with the btree opclass, which uses float[4|8]_cmp_internal()
(so that all NaNs are equal and NaN sorts after every non-NaN value).
In addition, the penalty and distance functions were not careful
about NaNs, and the penalty functions could also misbehave for IEEE
infinities. Wrong answers from the penalty functions would probably
do no more than make the index non-optimal, but the distance mistakes
were visible from SQL.
To fix, make the comparison functions rely on the same NaN-aware
comparison functions the core code uses, and rewrite the penalty
and distance functions to follow the rules that NaNs are equal
but maximally far away from non-NaNs. The penalty_num() code was
formerly shared between integral and float cases, but I chose to make
two copies so that the integral cases are not saddled with the extra
logic for NaNs and infinities/overflows. I also rewrote it as static
inline functions instead of an unreadable and uncommented macro.
The float penalty functions were previously unreached by the
regression tests, so add new test cases to exercise them.
There's no on-disk format change, but users who have NaN entries
in a btree_gist index would be well advised to reindex it.
Bug: #19501
Bug: #19524
Reported-by: Man Zeng <zengman@halodbtech.com>
Reported-by: Yuelin Wang <3020001251@tju.edu.cn>
Author: Bill Kim <billkimjh@gmail.com>
Co-authored-by: Tom Lane <tgl@sss.pgh.pa.us>
Discussion: https://postgr.es/m/19501-3bff3bbc97f1e7c9@postgresql.org
Discussion: https://postgr.es/m/19524-9559d302c8455664@postgresql.org
Discussion: https://postgr.es/m/CAMQXxcgbtD2LXfX0tpgvOizxP-XxrCHV2ZDy4By_TZnJMsxXWQ@mail.gmail.com
Backpatch-through: 14
Previously, if a base backup failed after it had started streaming
files, pg_stat_progress_basebackup could continue to show a stale
progress entry even though the backup was no longer running. This could
be observed when the client kept the replication connection open after
the error. It is normally not observable when using pg_basebackup,
because the client disconnects after the error.
The problem was that progress reporting was cleared only after
successful completion.
This commit moves the progress reporting cleanup into the progress
sink's cleanup callback so that it is cleared after both successful
and failed backups.
Backpatch to v15. v14 has the same issue, but the fix does not apply
cleanly because it lacks the base backup sink infrastructure. Since
the bug does not affect the backup itself and is normally not
observable when using pg_basebackup, skip the v14 backpatch.
Author: Chao Li <lic@highgo.com>
Reviewed-by: Fujii Masao <masao.fujii@gmail.com>
Discussion: https://postgr.es/m/EA1A6CD2-EFA6-462B-9A02-03003555AB4A@gmail.com
Backpatch-through: 15
off_t is most likely a 64-bit integer, so casting it to a 32-bit type
for output could lose data. There are more issues like this in the
tree, but this is an instance where this could actually happen in
practice, since base backups are routinely larger than 4 GB. So this
is separated out as a bug fix.
Reviewed-by: Heikki Linnakangas <hlinnaka@iki.fi>
Discussion: https://www.postgresql.org/message-id/flat/20ce62fa-47fc-457b-b504-12f3c1651726%40eisentraut.org
Commit 4035cd5d4 added LZ4 compression for full-page writes in WAL, and
retained "on" as a backward-compatible way to specify the builtin PGLZ
method. Document this meaning of "on" and update postgresql.conf.sample
to make the equivalence clear.
Author: Christoph Berg <myon@debian.org>
Reviewed-by: Michael Paquier <michael@paquier.xyz>
Discussion: https://postgr.es/m/akJDHRtXwGLTppsQ@msg.df7cb.de
Backpatch-through: 15
Previously, if an unlogged sequence was created on the primary and
replicated to a standby, reading the sequence after promoting the
standby (for example, with nextval()) could trigger the following
assertion failure:
TRAP: failed Assert("((const PageHeaderData *) page)->pd_special >= SizeOfPageHeaderData")
In non-assert builds, the same operation could instead fail with an
error such as:
ERROR: bad magic number in sequence
The problem was that seq_redo() updated the init fork page in shared
buffers but did not flush it to disk. During promotion,
ResetUnloggedRelations() recreates the main fork of unlogged
relations by copying the init fork from disk, bypassing shared
buffers. As a result, the main fork could be recreated from a stale
init fork instead of the WAL-replayed page.
Fix this by introducing a helper to flush init fork buffers
immediately, and make seq_redo() use it. As a result, the main fork
of an unlogged sequence is recreated from the up-to-date init fork on
disk, allowing the unlogged sequence to be read successfully after
standby promotion.
Backpatch to v15, where unlogged sequences were introduced.
Author: Fujii Masao <masao.fujii@gmail.com>
Reviewed-by: vignesh C <vignesh21@gmail.com>
Discussion: https://postgr.es/m/CAHGQGwH1Ssze3XM6wjoTjSLVOR041c6xP+vsdLP951=w8oG8bA@mail.gmail.com
Backpatch-through: 15
Treat copy_file_range() return value of zero as an error: it indicates
that no bytes could be copied (perhaps the source file is shorter than
expected), and the existing retry loop would otherwise spin forever
since nwritten would never reach BLCKSZ.
The other uses of copy_file_range() in the tree don't have this
problem.
Reviewed-by: Nazir Bilal Yavuz <byavuz81@gmail.com>
Reviewed-by: Kyotaro Horiguchi <horikyota.ntt@gmail.com>
Reviewed-by: Yingying Chen <cyy9255@gmail.com>
Discussion: https://www.postgresql.org/message-id/flat/3208cf7a-c7f3-41eb-92f6-33cbeff4df40%40eisentraut.org
PL/Python and its hstore and jsonb transforms build SQL values from
Python containers by calling Python C API functions that can return
NULL, and in several places the result was used without first checking
it.
On the sequence side, PySequence_GetItem() is used when converting a
returned sequence into a SQL array or composite value, when reading
the argument list passed to plpy.execute() or plpy.cursor(), and when
reading the list of type names given to plpy.prepare(). On the
mapping side, the hstore and jsonb transforms call PyMapping_Size()
and PyMapping_Items() and then index the result with PyList_GetItem()
and PyTuple_GetItem().
All of these return NULL (or -1), with a Python exception set, for a
broken object: for example one whose __getitem__() or items() raises,
or which reports a length that disagrees with what it actually yields.
The unchecked result was then dereferenced, crashing the backend.
Fix this by checking the result of each call and reporting a regular
error if it failed, so that the underlying Python exception is
surfaced instead of taking down the session.
Author: Richard Guo <guofenglinux@gmail.com>
Reviewed-by: Ayush Tiwari <ayushtiwari.slg01@gmail.com>
Discussion: https://postgr.es/m/CAMbWs49BKM9wP6m8bCXEpHwQKp7usvOGV6Jf=J7FYr_BCpxLqg@mail.gmail.com
Backpatch-through: 14
In get_perl_array_ref(), for a PostgreSQL::InServer::ARRAY object, we
look up its "array" key with hv_fetch_string() and then inspect the
returned SV. However, hv_fetch_string() returns a NULL pointer when
the key is absent, and the code dereferenced that result without first
checking whether the pointer itself was NULL. As a result, a plperl
function returning a forged PostgreSQL::InServer::ARRAY object that
lacks the "array" key would crash the backend with a segmentation
fault.
Fix this by checking the pointer returned by hv_fetch_string() before
dereferencing it, matching how other callers in this file already
guard the result. With the check in place, such an object falls
through to the existing error report instead of crashing.
Author: Xing Guo <higuoxing@gmail.com>
Reviewed-by: Richard Guo <guofenglinux@gmail.com>
Discussion: https://postgr.es/m/CACpMh+DYgcnqZwQLXXuxQcehJTd7T8UmKWSLsK4mFBEp9G2ajA@mail.gmail.com
Backpatch-through: 14
When ALTER TABLE ... ATTACH PARTITION matches partition indexes to the
parent table's indexes, invalid indexes are skipped. This commit
improves the documentation to describe what e90e9275f5 has changed:
invalid indexes are skipped, and only valid indexes are considered for a
match.
Author: Mohamed Ali <moali.pg@gmail.com>
Reviewed-by: Sami Imseih <samimseih@gmail.com>
Discussion: https://postgr.es/m/CAGnOmWpAMaE-BOkpwM6mJnHcpS2QZ8yLSSaqmz+vryEsbCWWWA@mail.gmail.com
Backpatch-through: 14
This routine acts as a wrapper of a new pgstat_drop_entry_ext(), used in
the core code with a missing_ok argument.
This includes an update of .abi-compliance-history, removing the latest
entry that has documented the change of pgstat_drop_entry(). This
change is applied across v15~v18. HEAD keeps pgstat_drop_entry() as
single entry point, with the new missing_ok.
Per discussion with Álvaro Herrera and Lukas Fittl. This is a follow-up
of 850b9218c8.
Discussion: https://postgr.es/m/ajZz_sVJVX7pmPHo@alvherre.pgsql
Backpatch-through: 15-18
This function called the resource-kind-specific ReleaseResource()
method for each item before deleting that item from the resowner.
That's backwards from the ordering in ResourceOwnerReleaseAllOfKind,
and it's not very safe. If ReleaseResource throws an error then the
subsequent abort cleanup will come back here and try to release that
item again, possibly leading to a double-free or similar crash,
and in any case risking an infinite error cleanup loop. This mistake
explains why the pgcrypto bug just fixed in 80bb0ebcc led to a crash
rather than something more benign.
Remove the item from the resowner, then call ReleaseResource,
matching the way things were done before b8bff07da. If there
is a problem of this sort, we'd prefer to leak the item than
suffer the other likely consequences.
Per further analysis of bug #19527.
Author: Tom Lane <tgl@sss.pgh.pa.us>
Discussion: https://postgr.es/m/646741.1782157515@sss.pgh.pa.us
Backpatch-through: 17
Raising an error within a function using an OSSLCipher object led
to a complaint from ResourceOwnerForget and then a double-free crash,
because ResOwnerReleaseOSSLCipher forgot to unhook the OSSLCipher
object from its owner. (The sibling logic for OSSLDigest objects got
this right, as did every other ReleaseResource function AFAICS.)
Oversight in cd694f60d.
Bug: #19527
Reported-by: Yuelin Wang <3020001251@tju.edu.cn>
Author: Yuelin Wang <3020001251@tju.edu.cn>
Reviewed-by: Tom Lane <tgl@sss.pgh.pa.us>
Discussion: https://postgr.es/m/19527-6e7686960c6dce78@postgresql.org
Backpatch-through: 17
pg_mkdir_p creates each missing path component with a stat() followed
by mkdir(). If the stat() reports the component as absent but another
process creates it in the window before this process's mkdir(), mkdir()
fails with EEXIST and pg_mkdir_p treated that as a hard error -- unlike
"mkdir -p", which is meant to be idempotent and race-tolerant.
This shows up when several processes concurrently create paths that
share an ancestor directory: for example, parallel initdb runs whose
data directories live under a common temporary directory. One process
wins the race to create the shared ancestor and the others fail with
could not create directory "...": File exists
Fix this race condition by first trying mkdir() and only attempting
stat() if it fails with EEXIST.
On Windows, there's an additional problem: stat() opens a file handle
and participates in share-mode locking, which means it can transiently
fail on a directory another process is concurrently creating. Use
GetFileAttributes() instead: it requests only FILE_READ_ATTRIBUTES
and is exempt from share-mode denial, so it reliably sees a
concurrently-created directory.
I (tgl) also chose to back-patch 039f7ee0f's effects on this function,
so that pgmkdirp.c remains identical in all live branches.
Author: Andrew Dunstan <andrew@dunslane.net>
Co-authored-by: Tom Lane <tgl@sss.pgh.pa.us>
Discussion: https://postgr.es/m/3ca004de-e49b-4471-b8aa-fd656e70f68c@dunslane.net
Backpatch-through: 14
Newer gcc warns that this "actual_arg_types" variable may be used
uninitialized, but visual inspection indicates there's no bug. To
silence the warning, initialize the variable to zeros.
Bug: #19485
Reported-by: Hans Buschmann <buschmann@nidsa.net>
Tested-by: Erik Rijkers <er@xs4all.nl>
Tested-by: Hans Buschmann <buschmann@nidsa.net>
Reviewed-by: Tristan Partin <tristan@partin.io>
Reviewed-by: Álvaro Herrera <alvherre@kurilemu.de>
Discussion: https://postgr.es/m/19485-2b03231a775756f1%40postgresql.org
Discussion: https://postgr.es/m/6c52a1a6612948519468d46cb224a8c4%40nidsa.net
Add CHECK_FOR_INTERRUPTS() to the while loop in plperl_to_hstore()
that dereferences chains of Perl references, so that a circular
reference (e.g. $x = \$x) can be cancelled by the user instead of
spinning indefinitely. (We looked at detecting such circular
references, but it seems more trouble than it's worth.)
This is a follow-up to da82fbb8f, which fixed the same issue in
SV_to_JsonbValue() in jsonb_plperl.
Author: Aleksander Alekseev <aleksander@tigerdata.com>
Reviewed-by: Tom Lane <tgl@sss.pgh.pa.us>
Discussion: https://postgr.es/m/CAJ7c6TPbjkzUk4qJ5dHvDNEz0hBuFue3A-XWz_=897z+BC+z8A@mail.gmail.com
Backpatch-through: 14
Commit 6678b58d78 fixed a wrong "Prev" link by changing the link
generation code to use [position()=last()] instead of [last()] in
the predicate on the union of reverse axes. Unfortunately, that
caused documentation builds to take much longer. To fix, combine
the "preceding" and "ancestor" steps into one "preceding" step and
one "ancestor" step, and revert the predicate back to [last()].
The smaller union evades the libxml2 bug while avoiding the build
time regression.
Reported-by: Tom Lane <tgl@sss.pgh.pa.us>
Tested-by: Tom Lane <tgl@sss.pgh.pa.us>
Discussion: https://postgr.es/m/1132496.1781718007%40sss.pgh.pa.us
Backpatch-through: 14
As noted in the commit message of 850b9218c8, this function has gained
an extra called "missing_ok". All the callers of this routine should be
in core in the v15-v17 range. For v18, I have found one custom stats
kind that would be impacted by this change.
Discussion: https://postgr.es/m/ajOE3uRxVgSlPRcw@paquier.xyz
Backpatch-through: 15-18
drop_local_obsolete_slots() continued to dereference local_slot after
calling ReplicationSlotDropAcquired(). Once the slot is dropped, its
entry in the slot array can be reused by another backend, so later reads
of local_slot->data could observe a different slot's name or database
OID, leading to an incorrect unlock and log message.
Save the slot name and database OID before performing the drop, and use
the saved values for the subsequent UnlockSharedObject() call and the log
message. While at it, emit the "dropped replication slot" message only
when a slot was actually dropped, rather than unconditionally.
Author: Xuneng Zhou <xunengzhou@gmail.com>
Reviewed-by: Zhijie Hou <houzj.fnst@fujitsu.com>
Reviewed-by: Amit Kapila <amit.kapila16@gmail.com>
Reviewed-by: Fujii Masao <masao.fujii@gmail.com>
Backpatch-through: 17, where it was introduced
Discussion: https://postgr.es/m/TY4PR01MB177184FF9EE916F577E1F554194082@TY4PR01MB17718.jpnprd01.prod.outlook.com
pgstat_drop_entry_internal() generates an ERROR if facing a pgstats
entry already marked as dropped. With a workload doing a lot of
concurrent CALL and DROP/CREATE PROCEDURE, it could be possible for
AtEOXact_PgStat_DroppedStats(), that wants to do transactional drops, to
find entries that are already dropped, after a commit record has been
written. In this case, ERRORs are upgraded to PANIC, taking down the
server.
This issue is fixed by making pgstat_drop_entry() optionally more
tolerant to concurrent drops, adding to the routine a missing_ok option
to make some of its callers more tolerant (spoiler: some of the callers
want a strict behavior, like replication slots and backend stats).
pgstat_drop_entry_internal() cannot be called anymore for an entry
marked as dropped, hence its error is replaced by an assertion.
Functions are handled as a special case in core; this problem could also
apply to custom stats kinds depending on what an extension does.
track_functions is costly when enabled (disabled by default), which is
perhaps the main reason why this has not be found yet.
A similar version of this patch has been proposed by Sami Imseih on a
different thread for a feature in development. This version has tweaked
here by me for the sake of fixing this issue.
Reported-by: zhanglihui <zlh21343@163.com>
Author: Sami Imseih <samimseih@gmail.com>
Author: Michael Paquier <michael@paquier.xyz>
Reviewed-by: Ayush Tiwari <ayushtiwari.slg01@gmail.com>
Discussion: https://postgr.es/m/19520-73873648d44793cf@postgresql.org
Backpatch-through: 15
Add check_stack_depth() to Jsonb_to_SV, SV_to_JsonbValue,
PLyObject_FromJsonbContainer, and PLyObject_ToJsonbValue. Without
this, deeply nested JSONB values can crash the backend with SIGSEGV
instead of raising a proper error.
Also add CHECK_FOR_INTERRUPTS() to the while loop in SV_to_JsonbValue
that dereferences chains of Perl references, so that a circular
reference (e.g. $x = \$x) can be cancelled by the user instead of
spinning indefinitely. (We looked at detecting such circular
references, but it seems more trouble than it's worth.)
Author: Aleksander Alekseev <aleksander@tigerdata.com>
Reviewed-by: Tom Lane <tgl@sss.pgh.pa.us>
Discussion: https://postgr.es/m/CAJ7c6TPbjkzUk4qJ5dHvDNEz0hBuFue3A-XWz_=897z+BC+z8A@mail.gmail.com
Backpatch-through: 14
The test did not wait for the standby to be connected to the primary.
This breaks one assumption at the beginning of the test, where the
primary is stopped to ensure that all its records are flushed to both
standbys before moving on with its next steps.
If standby_1 finishes ahead of standby_2, the test would be able work
fine as the former waits for the latter. The opposite is not true,
standby_2 getting ahead of standby_1 would cause the test to fail on
timeout when standby_1 attempts to connect to standby_2.
This commit adds an additional polling query after the two standbys are
started, checking that both standbys are connected to the primary before
processing with the initial steps of the test.
Like 7185eddf05, backpatch down to v14.
Author: Sergey Tatarintsev <s.tatarintsev@postgrespro.ru>
Reviewed-by: Ewan Young <kdbase.hack@gmail.com>
Discussion: https://postgr.es/m/fea4190e-f8b5-4432-a52d-bcbee5f34366@postgrespro.ru
Backpatch-through: 14
The error path in ReorderBufferProcessTXN was not freeing
(reorderbuffer.c's representation of) a speculative insertion record
correctly. In assert-enabled builds, this leads to an assertion
failure. In production builds, I see no effect; there may be a small
transient leak, but in an improbable code path such as this, such a leak
is not of any significance. For users running with assertions enabled,
the crash is annoying.
Fix by having ReorderBufferProcessTXN() free the speculative insert
ahead of freeing the rest of the transaction, and no longer try to
handle that insert as a separate argument to ReorderBufferResetTXN().
This code came in with commit 7259736a6e (14-era). Backpatch all the
way back.
In branches 14-16, also backpatch the assertion that originally fails in
the problem scenario, which was added by dbed2e3662 (originally
backpatched to 17), that at the end of ReorderBufferReturnTXN() the
in-memory size of the transaction is zero.
Author: Vishal Prasanna <vishal.g@zohocorp.com>
Reviewed-by: Hayato Kuroda <kuroda.hayato@fujitsu.com>
Backpatch-through: 14
Discussion: https://postgr.es/m/19c7623e882.4080fd5426212.311756747309556767@zohocorp.com
The expression (len_diff * 10 * (an + 1)) used as the return value of
ltree_compare() is computed at int32 width. With LTREE_MAX_LEVELS =
65535, the product can exceed INT32_MAX once an ltree has more than
~14,653 levels, which causes the result to wrap and invert its sign.
That corrupts btree ordering as well as the "magnitude" consumed by
ltree_penalty() for GiST page splits.
To fix, split ltree_compare() into two functions. The new
ltree_compare_distance() function returns a float, which won't
overflow. It's used by the ltree_penalty() caller. All the other
callers only care about the sign of the return value, i.e. which of
the arguments is greater, so change ltree_compare() to not multiply
the result with (10 * (an + 1)), which avoids the overflow for those
callers.
Existing btree or GiST indexes on ltree columns containing values with
more than ~14,653 levels may be corrupt and should be REINDEXed.
Add a regression test based on the reporter's PoC.
Author: Ayush Tiwari <ayushtiwari.slg01@gmail.com>
Reported-by: 王跃林 <violin0613@tju.edu.cn>
Discussion: https://www.postgresql.org/message-id/AI6AnABgKW93Qbx1jVzi84r9.8.1781322625756.Hmail.3020001251%40tju.edu.cn
Backpatch-through: 14
Our handling of quoting within replication commands was pretty
sloppy, typically looking like
appendStringInfo(&cmd, " SLOT \"%s\"", options->slotname);
This is fine as long as options->slotname doesn't contain a double
quote mark, but what if it does? In principle this'd allow injection
of harmful options into replication commands, in the probably-unlikely
case that a slot name comes from untrustworthy input. We ought to
clean that up.
Moreover, even the places that were trying to be more careful
generally got it wrong, because they used quoting subroutines
intended for SQL commands rather than something that will work
with the replication-command scanner repl_scanner.l. For example,
several places naively use PQescapeLiteral() to quote option values
for replication commands. If the string contains a backslash,
PQescapeLiteral() will produce E'...' literal syntax, which
repl_scanner.l doesn't recognize. Another near miss was to use
quote_identifier() to quote identifiers. That function won't quote
valid lowercase identifiers unless they match SQL keywords ... but in
this context, replication keywords are what matter. Neither of these
errors seem to risk string injection, but they definitely can cause
syntax errors in replication commands that ought to be valid.
We can clean all this up by using simple quoting logic that just
doubles single or double quotes respectively.
Or at least, we could if repl_scanner.l handled doubled double quotes
in identifiers, but for some reason it doesn't! So the first step in
this fix has to be to fix that. (The fact that we'll later reject
slot names containing double quotes is very far short of justifying
this omission.)
Having done that, this patch runs around and applies correct
quoting in all places that generate replication commands containing
strings coming from outside the immediate context. Probably some
of these places are safe because of restrictions elsewhere, but it
seems best to just quote all the time.
This was originally reported as a security bug, which it could be
if replication slot names or parameters were to originate from
untrustworthy sources. But the security team concluded that that
was a very improbable situation, so we're just going to fix this
as a regular bug.
Reported-by: Team Dhiutsa
Author: Tom Lane <tgl@sss.pgh.pa.us>
Reviewed-by: Ayush Tiwari <ayushtiwari.slg01@gmail.com>
Discussion: https://postgr.es/m/1648659.1781287310@sss.pgh.pa.us
Backpatch-through: 14
Presently, the "Prev" link on the page for background workers sends
you to the middle of the previous chapter instead of the actual
previous page. This appears to be caused by a libxml2 bug, but
regardless, a minimal fix is to change the link generation code to
use [position()=last()] instead of [last()] in the predicate on the
union of reverse axes.
Reviewed-by: Ayush Tiwari <ayushtiwari.slg01@gmail.com>
Discussion: https://postgr.es/m/aim4AZorFKaC7Wrf%40nathan
Backpatch-through: 14
Late-model clang complains that these functions should be labeled
with "format(printf, 2, 3)", and it's right. But let's go a bit
further and also make use of varargs, to remove duplication and
allow these functions to be used with non-integer input values.
Since no good deed goes unpunished, I had to also adjust a couple
of call sites. They weren't wrong as-is, since the size_t-sized
arguments were coerced to int on the way into diag3(). But
without that, we have to adjust the format strings.
The point of this is to suppress compiler warnings, so back-patch
into branches containing pg_bsd_indent, even though there's no
functional change.
Author: Tom Lane <tgl@sss.pgh.pa.us>
Reviewed-by: Ayush Tiwari <ayushtiwari.slg01@gmail.com>
Discussion: https://postgr.es/m/1645041.1781283554@sss.pgh.pa.us
Backpatch-through: 16
If a query has more than 7498 params, the ParameterDescription message
exceeds the 30000 byte limit on messages that are not specifically
marked as possibly being longer than that (VALID_LONG_MESSAGE_TYPE).
To fix, add ParameterDescription to the list.
Author: Ning Sun <classicning@gmail.com>
Discussion: https://www.postgresql.org/message-id/dbfb4b65-0aa8-470a-8b87-b6496160b28a@gmail.com
Backpatch-through: 14
This commit reduces the number of expected output files for the "xml"
test from three to two (well, mostly one, see below for details).
xml_2.out existed to handle some differences in output due to libxml2
2.9.3, due to some error context missing (085423e3e3). This file is
removed, by tweaking the XML inputs to trigger the same error patterns
for the problematic 2.9.3 and other libxml2 versions. This part is
authored by Tom Lane.
xml_1.out (no libxml2 support) is reduced in size by adding an \if query
that exits the test early. This still checks NO_XML_SUPPORT() through
xmlin(). The rest of the test is skipped if XML input cannot be
handled by the backend. This part has been written by me.
Author: Tom Lane <tgl@sss.pgh.pa.us>
Author: Michael Paquier <michael@paquier.xyz>
Reviewed-by: Daniel Gustafsson <daniel@yesql.se>
Discussion: https://postgr.es/m/aiu6CXO67q-s70n5@paquier.xyz
Backpatch-through: 14
Commit 2f70fdb06 removed the deprecated containment operator
~(aclitem[],aclitem) from the catalogs, but missed removing its entry
from the documentation. (Arguably the blame should fall on c62dd80cd,
which added this entry in contravention of the longstanding policy
that we don't document deprecated aliases in the first place.)
Author: Shinya Kato <shinya11.kato@gmail.com>
Reviewed-by: Tom Lane <tgl@sss.pgh.pa.us>
Discussion: https://postgr.es/m/CAOzEurQSyR5psWukyhUz1LtxyO55C2Vfp0Fmt8w2jGKxhszQmQ@mail.gmail.com
Backpatch-through: 14
bt_normalize_tuple() uses VARSIZE() to get the size of varlena, even though
it's not yet known, that it has a 4-byte header. Fix this by replacing a
accessor with a universal VARSIZE_ANY().
Backpatch to all supported versions.
Reported-by: Andres Freund <andres@anarazel.de>
Discussion: https://postgr.es/m/7ckc7oka4bvafkf5bwlqs6ygrhlsbhz25ppozfch7zbuxcx3rf%40e4pr4oqenalc
Author: Andrey Borodin <x4mmm@yandex-team.ru>
Reviewed-by: Alexander Korotkov <aekorotkov@gmail.com>
Backpatch-through: 14
Commit 0e1f1ed157 taught seg_out() to print the certainty indicator
on an interval's upper boundary, but it was back-patched only as far
as v14. When upgrading from an older release, the old server prints
the one test_seg row exercising that case ('4.6 .. ~7.0') without the
indicator, so the pre- and post-upgrade dumps do not match. Make
AdjustUpgrade.pm delete just that row; seg's comparison function does
distinguish the certainty indicators, so the otherwise identical row
'4.6 .. 7.0' is unaffected.
Back-patch to all supported branches.
Per buildfarm members crake and fairywren.
Discussion: https://postgr.es/m/5ccbdbde-6467-4a10-bf4d-0be73a05ce8d@dunslane.net
OpenSSL 4.0.0 changed some parameters and returnvalues to const, so
we need to update our declarations and subsequently cast away const-
ness from a few callsites to make libpq build without warnings. This
is tested with OpenSSL 1.1.1 through 4.0.0 as well as with LibreSSL.
No functional change is introduced, this commit only allows postgres
to be compiled against OpenSSL 4.0.0 without warnings.
There is also an errormessage change in OpenSSL 4.0.0 which needed
to be covered by our testharness.
This will be backpatched to all supported branches since they are
all equally likely to be built against OpenSSL 4.0.0 as it becomes
available in distributions. Backpatching will be done once it has
been in master for a few days without issues.
Author: Daniel Gustafsson <daniel@yesql.se>
Reviewed-by: Michael Paquier <michael@paquier.xyz>
Discussion: https://postgr.es/m/066B07BB-85FA-487C-BE8C-40F791CFC3C4@yesql.se
Backpatch-through: 14
read_local_xlog_page_guts has the same race as logical_read_xlog_page:
RecoveryInProgress() can return true during promotion, impacting the
availability of the operations doing WAL page reads with this callback.
This problem is similar to eb4e7224a1 that has addressed the issue for
logical replication, impacting more areas of the code where this WAL
page callback can be used (same narrow window during promotion, same
availability issue):
- pg_walinspect.
- Slot advance (SQL function).
- Slot creation.
Repack workers (v19~) and 2PC files (since forever) can also use this
callback, but they are irrelevant as far as I know. A test is added
with the SQL lookup functions. This part relies on injection points,
and is backpatched down to v18, like the test added for eb4e7224a1.
This issue could probably be fixed as well in v14 and v15 for
pg_walinspect. However, I also feel that there is a conservative
argument about consistency here due to the support of logical decoding
on standbys, so let's limit ourselves to v16 for now. pg_walinspect is
used less in the field compared to the two other operations, making
addressing this problem less attractive in these two older branches.
Reported-by: Xuneng Zhou <xunengzhou@gmail.com>
Author: Bertrand Drouvot <bertranddrouvot.pg@gmail.com>
Reviewed-by: Xuneng Zhou <xunengzhou@gmail.com>
Reviewed-by: Hayato Kuroda <kuroda.hayato@fujitsu.com>
Discussion: https://postgr.es/m/7daef094-abf3-4672-bc23-3df4763b16a3%40gmail.com
Backpatch-through: 16
