pg_database_size() allows access to users who have either CONNECT
privilege on the target database or privileges of the pg_read_all_stats
role. However, previously, psql's \l+ checked only for CONNECT,
so users with privileges of pg_read_all_stats still saw "No Access" for
databases they could not connect to.
Fix this by making \l+ also check
pg_has_role('pg_read_all_stats', 'USAGE'), matching
pg_database_size()'s permission rules.
For back branches, emit the pg_read_all_stats check only when
connected to PostgreSQL 10 or later, since earlier releases do not have
that predefined role.
Backpatch to all supported versions.
Author: Christoph Berg <myon@debian.org>
Reviewed-by: Álvaro Herrera <alvherre@kurilemu.de>
Reviewed-by: Fujii Masao <masao.fujii@gmail.com>
Discussion: https://postgr.es/m/amCo6qRmnfPVk4-V@msg.df7cb.de
Backpatch-through: 14
It's always been the case that granting these privileges to users that
you don't fully trust was a bad idea, but it hasn't always been
obvious to people reading the documentation that this is the case.
To prevent confusion, and also repeated reports to pgsql-security,
mention it explicitly.
Discussion: http://postgr.es/m/CA+TgmobrjCHBuWHrvX3=2vndUCO2thUOdevrCcMDFW86cqCYvw@mail.gmail.com
Reviewed-by: Nathan Bossart <nathandbossart@gmail.com>
Backpatch-through: 14
An index-only scan filled its result slot from the HeapTuple an index AM
returns in scan->xs_hitup by deforming it with the virtual slot's own
tuple descriptor (during GiST and SP-GiST index-only scans). But index
AMs form that heap tuple using their own descriptor, scan->xs_hitupdesc.
The AM's descriptor may disagree with the IoS virtual slot's descriptor
about each column's precise alignment, leading to "can't happen" errors
in certain rare edge cases. Hard crashes were possible but much less
likely.
To fix, deform the tuple with the descriptor it was formed with. This
is simpler, and makes xs_hitup handling (used by GiST and SP-GiST)
uniform with the nearby existing xs_itup handling (used by nbtree).
In practice this issue was very unlikely to be hit (it was found during
testing of a patch that will change the table AM API used during index
scans). The only currently affected core opclass is GiST's range_ops.
It was only possible for the datum to be accessed at an incorrectly
aligned offset when reading the second or subsequent column from a
multicolumn GiST index. This couldn't happen in the common case where
the datum used an unaligned short varlena header. Moreover, an earlier
column had to leave the range datum at an offset where the two
alignments actually disagree (e.g., an odd-length varlena datum).
Author: Peter Geoghegan <pg@bowt.ie>
Reviewed-by: Tomas Vondra <tomas@vondra.me>
Discussion: https://postgr.es/m/CAH2-WzkGXa2SKnebdW29RT1hCcQBo_p03v3iqif2u9bjzLB-aQ@mail.gmail.com
Backpatch-through: 14
The previous text claimed that once the slot is disassociated with
ALTER SUBSCRIPTION ... SET (slot_name = NONE), DROP SUBSCRIPTION "will no
longer attempt any actions on a remote host". That is inaccurate:
DROP SUBSCRIPTION may still connect to the publisher to drop
internally-created table synchronization slots when some table
synchronization is left unfinished. Reword to describe this, and note
that if the publisher is unreachable those slots (and the main slot, if
it still exists) must be dropped manually to avoid indefinitely reserving
WAL.
Reported-by: Jeff Davis <pgsql@j-davis.com>
Author: Amit Kapila <amit.kapila16@gmail.com>
Backpatch-through: 14
Discussion: https://postgr.es/m/CAA4eK1+tyYSpPxMBy1974kjivuGeR7YY=yopwRGrK3+vCTysdg@mail.gmail.com
Discussion: https://postgr.es/m/D908370F-2695-4231-851D-17179A6A6F2A@gmail.com
Previously, libpqrcv_create_slot() checked only that
CREATE_REPLICATION_SLOT returned PGRES_TUPLES_OK before reading
values from the first row. If the server unexpectedly returned an
invalid result, such as zero rows, PQgetvalue() could return NULL,
leading to a crash while parsing the LSN.
Other replication commands, such as IDENTIFY_SYSTEM, already validate
the response shape before accessing result values, but
CREATE_REPLICATION_SLOT did not.
Fix this by verifying that CREATE_REPLICATION_SLOT response contains
exactly one row with four fields, and report a protocol violation otherwise.
Backpatch to all supported versions.
Bug: #19547
Reported-by: Yuelin Wang <1217816127@qq.com>
Author: Kenny Chen <kennychen851228@gmail.com>
Reviewed-by: Hayato Kuroda <kuroda.hayato@fujitsu.com>
Reviewed-by: Fujii Masao <masao.fujii@gmail.com>
Discussion: https://postgr.es/m/19547-f7986f668f71e788@postgresql.org
Discussion: https://postgr.es/m/CAPXstDtW2iqe+DJAOTQTX+rRziJp2UhZSo1+HRj1COAtbu+nKw@mail.gmail.com
Backpatch-through: 14
The documentation for log_parameter_max_length said it affects messages
generated by log_duration. However, log_duration alone does not log bind
parameter values, so this is misleading.
This commit updates the documentation to reference log_min_duration_statement,
which can log bind parameters, to better reflect actual behavior.
Backpatch to all supported versions.
Author: Fujii Masao <masao.fujii@gmail.com>
Reviewed-by: Surya Poondla <suryapoondla4@gmail.com>
Discussion: https://postgr.es/m/CAHGQGwGnCVMVz8-LU9F8Sh57bkQX3jMZzx7age7M0LFEz5=Fog@mail.gmail.com
Backpatch-through: 14
equalPolicy() is used in the relation cache to check if two policy
definitions are equivalent, but missed to check for polpermissive.
ALTER POLICY cannot switch a policy to be PERMISSIVE or RESTRICTIVE, so
this would need a dropped and then re-created policy, which would
trigger a relcache invalidation. Anyway, there is no harm in being
consistent in the check, and if one decides to add an ALTER POLICY to
switch PERMISSIVE or RESTRICTIVE, we would be silently in trouble.
Author: Andreas Lind <andreaslindpetersen@gmail.com>
Reviewed-by: Laurenz Albe <laurenz.albe@cybertec.at>
Discussion: https://postgr.es/m/CAMxA3rv1CS6R7JR5ojz-3CmCEnZEFrqu+XXTnGbLRWrjJRH7sA@mail.gmail.com
Backpatch-through: 14
The pg_attribute_always_inline macro name is so long it forces pgindent
to format the code in strange ways. Which may incentivize patch authors
to either structure the code in strange ways (e.g. reorder prototypes),
use shorter names, etc. Neither is very desirable for code readability.
This shortens the name by removing the _attribute_ part. It also makes
it more consistent with pg_noinline, which does not have the _attribute_
part either.
Backpatched to all supported branches, to prevent conflicts when
backpatching other fixes. The backbranches however keep both the old and
new macro name, so that existing code keeps working.
Author: Andres Freund <andres@anarazel.de>
Reviewed-by: Peter Geoghegan <pg@bowt.ie>
Reviewed-by: Tomas Vondra <tomas@vondra.me>
Discussion: https://postgr.es/m/bqqdehahpoa36igpictuqyn2s2mexk3t3ehidh2ffd2slb35e5@rzgksuiszgbg
Backpatch-through: 14
Check explicitly for pqsecure_read() returning an error. It shouldn't
fail, and we would've caught it in the check for a short read, but
better to be explicit so that the error message is more informative.
We also shouldn't update 'inEnd' when the read fails, although that
too is just pro forma as we will bail out and close the connection on
error.
Reported-by: Peter Eisentraut <peter@eisentraut.org>
Discussion: https://www.postgresql.org/message-id/34844e8c-267c-4daf-b1e0-f26059a4a7d3@eisentraut.org
Backpatch-through: 14
Commit aa606b9316 disallowed generated columns in COPY FROM WHERE
expressions, and commit 21c69dc73f disallowed system columns.
However, the COPY reference page still mentions only the restriction
on subqueries.
Update the documentation to also list generated columns and system
columns as unsupported in COPY FROM WHERE expressions.
Backpatch the generated-column documentation change to all supported
versions. Backpatch the system-column documentation change to v19,
where that restriction was introduced.
Author: Fujii Masao <masao.fujii@gmail.com>
Reviewed-by: Ayush Tiwari <ayushtiwari.slg01@gmail.com>
Discussion: https://postgr.es/m/CAHGQGwEgxErc54yVOAVWCsr1O=8pgw4oKRPuEQ9mfhkoYGR_XA@mail.gmail.com
Backpatch-through: 14
Commit dcb0049523 accidentally changed the final expanded query's
condition to > 2 while rewriting the example into SQL operator notation.
The original query and the preceding rewritten forms all use >= 2,
and view expansion should preserve that qualification. This commit
changes the final condition from > 2 to >= 2.
Backpatch to all supported versions.
Reported-by: Yaroslav Saburov <y.saburov@gmail.com>
Author: Fujii Masao <masao.fujii@gmail.com>
Reviewed-by: Daniel Gustafsson <daniel@yesql.se>
Discussion: https://postgr.es/m/178248467618.108999.9966122434342474006@wrigleys.postgresql.org
Backpatch-through: 14
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
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
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 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
Commit 0b7719f74 added regression tests that spell the language name
as plpython3u. That works on the master branch, but on v14 the
plpython tests must use the unversioned name plpythonu: the plpython3u
extension is built only in Python 3 builds, and for those builds
regress-python3-mangle.mk rewrites plpythonu to plpython3u in the test
files. In a Python 2 build the new tests instead failed with
"language \"plpython3u\" does not exist".
Spell the language as plpythonu in the new tests, matching every other
plpython test in this branch, so they work in both Python 2 and Python
3 builds. This is needed only in v14; later branches no longer
support Python 2 and have dropped the mangling step.
Per buildfarm member hippopotamus.
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
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
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
The two-argument jsonb @? and @@ operators invoke the jsonpath executor
with no variable set. In that case getJsonPathVariable() treated any
"$name" reference as JSON null and continued evaluating, instead of
reporting the variable as undefined.
This produced incorrect results -- for example '42'::jsonb @? '$"x"'
returned true -- and, for some malformed or hostile jsonpath expressions
with deeply nested predicates, allowed essentially unbounded memory
consumption that could get the backend killed by the OOM killer.
Report the undefined variable as an error in this case as well, reusing
the message already emitted when a variable is not found among supplied
variables. This matches the behavior of v17 and later, where the
jsonpath executor was reorganized. Stopping at the first undefined
variable reference also resolves the reported memory-growth case.
Note this is a user-visible change in the back branches: a jsonpath
expression that references a variable while no variables are supplied now
raises an error rather than silently evaluating it as NULL. The previous
behavior was incorrect, so the change is judged worthwhile.
Bug: #19458
Reported-by: Andrey Rachitskiy <pl0h0yp1@gmail.com>
Author: Andrey Rachitskiy <pl0h0yp1@gmail.com>
Reviewed-by: Andrey Borodin <x4mmm@yandex-team.ru>
Reviewed-by: Nikita Malakhov <hukutoc@gmail.com>
Reviewed-by: Amit Langote <amitlangote09@gmail.com>
Discussion: https://postgr.es/m/19458-a69c98bc498333ba@postgresql.org
Backpatch-through: 14-16
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
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 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
Commit ca9e9b08e4 wrongly tried to import devnull from File::Spec, but
it's not exported, you just call the method via the class. This was
harmless until modern perls complained, so stop doing that.
Per buildfarm failures.
Backpatch 14 thru 16
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
Commit a70bce43fb added instructions on how to recover if PostgreSQL
refuses to issue new transaction IDs because of imminent wraparound,
but when describing how to find replication slots that should be dropped,
it referred to pg_stat_replication where it should have referenced
pg_replication_slots.
In passing, decorate references to views with <structname> tags.
Backpatch to all supported versions.
Reported-By: Sanjaya Waruna <sanjaya.waruna@gmail.com>
Author: Laurenz Albe <laurenz.albe@cybertec.at>
Reviewed-by: Robert Treat <rob@xzilla.net>
Reviewed-by: Fujii Masao <masao.fujii@gmail.com>
Discussion: https://postgr.es/m/176767268098.1084085.10345048667224193115@wrigleys.postgresql.org
Backpatch-through: 14
xpath() attempted to call xmlCopyNode() and xmlNodeDump() on a
XML_NAMESPACE_DECL, finishing with a confusing error:
=# SELECT xpath('//namespace::foo', '<root xmlns:foo="http://127.0.0.1"/>');
ERROR: 53200: could not copy node
CONTEXT: SQL function "xpath" statement 1
xpath() is changed so as it goes through xmlXPathCastNodeToString()
instead, that is able to handle namespace nodes. xml2 uses the same
solution. This issue has been discovered while digging into
9d33a5a804.
Author: Michael Paquier <michael@paquier.xyz>
Discussion: https://postgr.es/m/aioT7ui_ZJ9RMlfM@paquier.xyz
Backpatch-through: 14
When printing the upper boundary of a seg interval, seg_out() decided
whether to emit the certainty indicator ('<', '>' or '~') by testing the
upper indicator (u_ext) for '<' and '>', but mistakenly tested the lower
indicator (l_ext) for '~'. This is a copy-and-paste slip from the
symmetric code that prints the lower boundary a few lines above.
The consequences for valid input were:
* A '~' on the upper boundary was dropped on output, e.g.
'1.5 .. ~2.5'::seg printed as '1.5 .. 2.5'.
* When the lower boundary carried '~' but the upper boundary had no
indicator, the wrong test matched and sprintf(p, "%c", seg->u_ext)
wrote a NUL byte (u_ext == '\0'), which truncated the result string
and silently lost the entire upper boundary, e.g.
'~6.5 .. 8.5'::seg printed as '~6.5 .. '.
Certainty indicators are documented to be preserved on output (they are
ignored by the operators, but kept as comments), so this broke the
input/output round-trip for the affected values.
The bug has existed since seg was added. It went unnoticed because the
existing regression tests only exercised certainty indicators on
single-point segs, which are printed by a different branch of seg_out().
Add tests that place indicators on both boundaries of an interval.
Author: Ewan Young <kdbase.hack@gmail.com>
Discussion: https://www.postgresql.org/message-id/CAON2xHPYeRRCEVAv8XfE18KsEsEHCiYcJ5fOsoxFuMEfpxF1=g@mail.gmail.com
Backpatch-through: 14
pgxmlNodeSetToText() passed nodeTab[i]->doc to xmlNodeDump() without
checking the node type, which could cause a crash as a
XML_NAMESPACE_DECL maps to a xmlNs struct. The passed-in code would
then be dereferenced in xmlNodeDump().
This commit switches the code to render XML_NAMESPACE_DECL nodes with
xmlXPathCastNodeToString(), like xpath_table(). Some tests are added,
written by me.
Author: Andrey Chernyy <andrey.cherny@tantorlabs.com>
Co-authored-by: Michael Paquier <michael@paquier.xyz>
Discussion: https://postgr.es/m/20260611031436.5afde3cb@andrnote
Backpatch-through: 14
Previously, outlen was miscalculated if case_sensitive was false and
str_tolower() changed the byte length of the string. If outlen was too
large, pnstrdup() would stop at the NUL terminator, preventing
overrun. But if outlen was too small, it would cause truncation.
Fix by just removing outlen. It was only used in a single site, which
could just as well use pstrdup().
Discussion: https://postgre.es/m/1101e1a3afbbabb503317069c40374b82e6f4cac.camel@j-davis.com
Reviewed-by: Tristan Partin <tristan@partin.io>
Backpatch-through: 14
The operators for array_eq, record_eq, range_eq, and multirange_eq
are all marked oprcanhash, but there's a pitfall: their hash functions
can fail at runtime if the contained type(s) are not hashable.
Therefore, the planner has to check hashability of the contained types
before deciding it can use hashing in these cases. Not every place
had gotten this memo, and noplace at all had considered the issue
for ranges or multiranges. In particular we could attempt to use
hashing for a ScalarArrayOpExpr on a container type when it won't
actually work, leading to "could not identify a hash function ..."
runtime failures.
For the most part we should fix this in the lookup functions provided
by lsyscache.c, to wit get_op_hash_functions and op_hashjoinable.
But there's a problem: get_op_hash_functions is not passed the input
data type it would need to check. We mustn't change the API of that
exported function in a back-patched fix, and even if we wanted to,
its call sites in the executor mostly don't have easy access to the
required data type OID. Fortunately, the executor call sites don't
actually need fixing, because it's expected that the planner verified
hashability before building a plan that requires it. Therefore,
leave get_op_hash_functions as-is and invent a wrapper function
get_op_hash_functions_ext that does the additional checking needed
in the planner's uses.
We also need to fix hash_ok_operator (extending the fix in 647889667).
While at it, neaten up a couple of places in lookup_type_cache where
relevant code for multirange cases was written differently from the
code for other container types.
Note: while this touches pg_operator.dat, it's only to add oid_symbol
macros. So there's no on-disk data change and no need for a
catversion bump.
Reported-by: Andrei Lepikhov <lepihov@gmail.com>
Author: Andrei Lepikhov <lepihov@gmail.com>
Co-authored-by: Tom Lane <tgl@sss.pgh.pa.us>
Discussion: https://postgr.es/m/ed221f95-f09b-4a9c-b05b-e1fed621ec87@gmail.com
Backpatch-through: 14
