Tom Lane [Sat, 5 Apr 2025 19:01:33 +0000 (15:01 -0400)]
Fix parse_cte.c's failure to examine sub-WITHs in DML statements.
makeDependencyGraphWalker thought that only SelectStmt nodes could
contain a WithClause. Which was true in our original implementation
of WITH, but astonishingly we missed updating this code when we added
the ability to attach WITH to INSERT/UPDATE/DELETE (and later MERGE).
Moreover, since it was coded to deliberately block recursion to a
WithClause, even updating raw_expression_tree_walker didn't save it.
The upshot of this was that we didn't see references to outer CTE
names appearing within an inner WITH, and would neither complain about
disallowed recursion nor account for such references when sorting CTEs
into a usable order. The lack of complaints about this is perhaps not
so surprising, because typical usage of WITH wouldn't hit either case.
Still, it's pretty broken; failing to detect recursion here leads to
assert failures or worse later on.
Fix by factoring out the processing of sub-WITHs into a new function
WalkInnerWith, and invoking that for all the statement types that
can have WITH.
Bug: #18878 Reported-by: Yu Liang <luy70@psu.edu>
Author: Tom Lane <tgl@sss.pgh.pa.us>
Discussion: https://postgr.es/m/18878-a26fa5ab6be2f2cf@postgresql.org
Backpatch-through: 13
Tom Lane [Sat, 5 Apr 2025 16:13:35 +0000 (12:13 -0400)]
Avoid double transformation of json_array()'s subquery.
transformJsonArrayQueryConstructor() applied transformStmt() to
the same subquery tree twice. While this causes no issue in many
cases, there are some where it causes a coredump, thanks to the
parser's habit of scribbling on its input.
Fix by making a copy before the first transformation (compare 0f43083d1). This is quite brute-force, but then so is the
whole business of transforming the input twice. Per discussion
in the bug thread, this implementation of json_array() parsing
should be replaced completely. But that will take some work
and will surely not be back-patchable, so for the moment let's
take the easy way out.
Oversight in 7081ac46a. Back-patch to v16 where that came in.
Bug: #18877 Reported-by: Yu Liang <luy70@psu.edu>
Author: Tom Lane <tgl@sss.pgh.pa.us>
Discussion: https://postgr.es/m/18877-c3c3ad75845833bb@postgresql.org
Backpatch-through: 16
Tom Lane [Sat, 5 Apr 2025 00:11:48 +0000 (20:11 -0400)]
Repair misbehavior with duplicate entries in FK SET column lists.
Since v15 we've had an option to apply a foreign key constraint's
ON DELETE SET DEFAULT or SET NULL action to just some of the
referencing columns. There was not a check for duplicate entries in
the list of columns-to-set, though. That caused a potential memory
stomp in CreateConstraintEntry(), which incautiously assumed that
the list of columns-to-set couldn't be longer than the number of key
columns. Even after fixing that, the case doesn't work because you
get an error like "multiple assignments to same column" from the SQL
command that is generated to do the update.
We could either raise an error for duplicate columns or silently
suppress the dups, and after a bit of thought I chose to do the
latter. This is motivated by the fact that duplicates in the FK
column list are legal, so it's not real clear why duplicates
in the columns-to-set list shouldn't be. Of course there's no
need to actually set the column more than once.
I left in the fix in CreateConstraintEntry() too, just because
it didn't seem like such low-level code ought to be making
assumptions about what it's handed.
Bug: #18879 Reported-by: Yu Liang <luy70@psu.edu>
Author: Tom Lane <tgl@sss.pgh.pa.us>
Discussion: https://postgr.es/m/18879-259fc59d072bd4d7@postgresql.org
Backpatch-through: 15
Commit 28d3c2ddcf introduced an assertion that if the memorized
downlink location in the insertion stack isn't valid, the parent's
LSN should've changed too. Turns out that was too strict. In
gistFindCorrectParent(), if we walk right, we update the parent's
block number and clear its memorized 'downlinkoffnum'. That triggered
the assertion on next call to gistFindCorrectParent(), if the parent
needed to be split too. Relax the assertion, so that it's OK if
downlinkOffnum is InvalidOffsetNumber.
Backpatch to v13-, all supported versions. The assertion was added in
commit 28d3c2ddcf in v12.
Reported-by: Alexander Lakhin <exclusion@gmail.com> Reviewed-by: Tender Wang <tndrwang@gmail.com>
Discussion: https://www.postgresql.org/message-id/18396-03cac9beb2f7aac3@postgresql.org
The documentation for the special value "system" for sslrootcert could
be misinterpreted to mean the default operating system CA store, which
it may be, but it's defined to be the default CA store of the SSL lib
used.
Backpatch down to v16 where support for the system value was added.
Author: Daniel Gustafsson <daniel@yesql.se> Reviewed-by: George MacKerron <george@mackerron.co.uk>
Discussion: https://postgr.es/m/B3CBBAA3-6EA3-4AB7-8619-4BBFAB93DDB4@yesql.se
Backpatch-through: 16
Fix logical decoding test to correctly check slot removal on standby.
The regression test for logical decoding verifies whether a logical slot
is correctly dropped on a standby when its associated database is dropped.
However, the test mistakenly retrieved slot information from the primary
instead of the standby, causing incorrect behavior.
This commit fixes the issue by ensuring the test correctly checks the slot
on the standby.
Fix logical decoding regression tests to correctly check slot existence.
The regression tests for logical decoding verify whether a logical slot
exists or has been dropped. Previously, these tests attempted to
retrieve "slot_name" from the result of slot(), but since "slot_name" was
not included in the result, slot()->{'slot_name'} always returned undef,
leading to incorrect behavior.
This commit fixes the issue by checking the "plugin" field in the result
of slot() instead, ensuring the tests properly verify slot existence.
Restrict copying of invalidated replication slots.
Previously, invalidated logical and physical replication slots could
be copied using the pg_copy_logical_replication_slot and
pg_copy_physical_replication_slot functions. Replication slots that
were invalidated for reasons other than WAL removal retained their
restart_lsn. This meant that a new slot copied from an invalidated
slot could have a restart_lsn pointing to a WAL segment that might
have already been removed.
This commit restricts the copying of invalidated replication slots.
Backpatch to v16, where slots could retain their restart_lsn when
invalidated for reasons other than WAL removal.
For v15 and earlier, this check is not required since slots can only
be invalidated due to WAL removal, and existing checks already handle
this issue.
Author: Shlok Kyal <shlok.kyal.oss@gmail.com> Reviewed-by: vignesh C <vignesh21@gmail.com> Reviewed-by: Zhijie Hou <houzj.fnst@fujitsu.com> Reviewed-by: Peter Smith <smithpb2250@gmail.com> Reviewed-by: Masahiko Sawada <sawada.mshk@gmail.com> Reviewed-by: Amit Kapila <amit.kapila16@gmail.com>
Discussion: https://postgr.es/m/CANhcyEU65aH0VYnLiu%3DOhNNxhnhNhwcXBeT-jvRe1OiJTo_Ayg%40mail.gmail.com
Backpatch-through: 16
Noah Misch [Mon, 8 Jan 2024 19:39:56 +0000 (11:39 -0800)]
Make dblink interruptible, via new libpqsrv APIs.
This replaces dblink's blocking libpq calls, allowing cancellation and
allowing DROP DATABASE (of a database not involved in the query). Apart
from explicit dblink_cancel_query() calls, dblink still doesn't cancel
the remote side. The replacement for the blocking calls consists of
new, general-purpose query execution wrappers in the libpqsrv facility.
Out-of-tree extensions should adopt these.
The original commit d3c5f37dd543498cc7c678815d3921823beec9e9 did not
back-patch. Back-patch now to v16-v13, bringing coverage to all supported
versions. This back-patch omits the orignal's refactoring in postgres_fdw.
Tom Lane [Wed, 2 Apr 2025 20:17:43 +0000 (16:17 -0400)]
Remove unnecessary type violation in tsvectorrecv().
compareentry() is declared to work on WordEntryIN structs, but
tsvectorrecv() is using it in two places to work on WordEntry
structs. This is almost okay, since WordEntry is the first
field of WordEntryIN. But on machines with 8-byte pointers,
WordEntryIN will have a larger alignment spec than WordEntry,
and it's at least theoretically possible that the compiler
could generate code that depends on the larger alignment.
Given the lack of field reports, this may be just a hypothetical bug
that upsets nothing except sanitizer tools. Or it may be real on
certain hardware but nobody's tried to use tsvectorrecv() on such
hardware. In any case we should fix it, and the fix is trivial:
just change compareentry() so that it works on WordEntry without any
mention of WordEntryIN. We can also get rid of the quite-useless
intermediate function WordEntryCMP.
Bug: #18875 Reported-by: Alexander Lakhin <exclusion@gmail.com>
Author: Tom Lane <tgl@sss.pgh.pa.us>
Discussion: https://postgr.es/m/18875-07a29c49c825a608@postgresql.org
Backpatch-through: 13
Andres Freund [Wed, 2 Apr 2025 18:25:17 +0000 (14:25 -0400)]
Remove HeapBitmapScan's skip_fetch optimization
The optimization does not take the removal of TIDs by a concurrent vacuum into
account. The concurrent vacuum can remove dead TIDs and make pages ALL_VISIBLE
while those dead TIDs are referenced in the bitmap. This can lead to a
skip_fetch scan returning too many tuples.
It likely would be possible to implement this optimization safely, but we
don't have the necessary infrastructure in place. Nor is it clear that it's
worth building that infrastructure, given how limited the skip_fetch
optimization is.
In the backbranches we just disable the optimization by always passing
need_tuples=true to table_beginscan_bm(). We can't perform API/ABI changes in
the backbranches and we want to make the change as minimal as possible.
Author: Matthias van de Meent <boekewurm+postgres@gmail.com> Reported-By: Konstantin Knizhnik <knizhnik@garret.ru>
Discussion: https://postgr.es/m/CAEze2Wg3gXXZTr6_rwC+s4-o2ZVFB5F985uUSgJTsECx6AmGcQ@mail.gmail.com
Backpatch-through: 13
Tom Lane [Wed, 2 Apr 2025 15:13:01 +0000 (11:13 -0400)]
Need to do CommandCounterIncrement after StoreAttrMissingVal.
Without this, an additional change to the same pg_attribute row
within the same command will fail. This is possible at least with
ALTER TABLE ADD COLUMN on a multiple-inheritance-pathway structure.
(Another potential hazard is that immediately-following operations
might not see the missingval.)
Introduced by 95f650674, which split the former coding that
used a single pg_attribute update to change both atthasdef and
atthasmissing/attmissingval into two updates, but missed that
this should entail two CommandCounterIncrements as well. Like
that fix, back-patch through v13.
Reported-by: Alexander Lakhin <exclusion@gmail.com>
Author: Tender Wang <tndrwang@gmail.com> Reviewed-by: Tom Lane <tgl@sss.pgh.pa.us>
Discussion: https://postgr.es/m/025a3ffa-5eff-4a88-97fb-8f583b015965@gmail.com
Backpatch-through: 13
The changes made in commit d2b4b4c2259 contained incorrect comments:
They said that certain forward declarations were necessary to "avoid
including pathnodes.h here", but the file is itself pathnodes.h! So
change the comment to just say it's a forward declaration in one case,
and in the other case we don't need the declaration at all because it
already appeared earlier in the file.
David Rowley [Wed, 2 Apr 2025 01:04:10 +0000 (14:04 +1300)]
Doc: add information about partition locking
The documentation around locking of partitions for the executor startup
phase of run-time partition pruning wasn't clear about which partitions
were being locked. Fix that.
Reviewed-by: Tender Wang <tndrwang@gmail.com>
Discussion: https://postgr.es/m/CAApHDvp738G75HfkKcfXaf3a8s%3D6mmtOLh46tMD0D2hAo1UCzA%40mail.gmail.com
Backpatch-through: 13
David Rowley [Tue, 1 Apr 2025 22:57:54 +0000 (11:57 +1300)]
Fix planner's failure to identify multiple hashable ScalarArrayOpExprs
50e17ad28 (v14) and 29f45e299 (v15) made it so the planner could identify
IN and NOT IN clauses which have Const lists as right-hand arguments and
when an appropriate hash function is available for the data types, mark
the ScalarArrayOpExpr as hashable so the executor could execute it more
optimally by building and probing a hash table during expression
evaluation.
These commits both worked correctly when there was only a single
ScalarArrayOpExpr in the given expression being processed by the
planner, but when there were multiple, only the first was checked and any
subsequent ones were not identified, which resulted in less optimal
expression evaluation during query execution for all but the first found
ScalarArrayOpExpr.
Tom Lane [Tue, 1 Apr 2025 20:49:51 +0000 (16:49 -0400)]
Fix detection and handling of strchrnul() for macOS 15.4.
As of 15.4, macOS has strchrnul(), but access to it is blocked behind
a check for MACOSX_DEPLOYMENT_TARGET >= 15.4. But our does-it-link
configure check finds it, so we try to use it, and fail with the
present default deployment target (namely 15.0). This accounts for
today's buildfarm failures on indri and sifaka.
This is the identical problem that we faced some years ago when Apple
introduced preadv and pwritev in the same way. We solved that in
commit f014b1b9b by using AC_CHECK_DECLS instead of AC_CHECK_FUNCS
to check the functions' availability. So do the same now for
strchrnul(). Interestingly, we already had a workaround for
"the link check doesn't agree with <string.h>" cases with glibc,
which we no longer need since only the header declaration is being
checked.
Testing this revealed that the meson version of this check has never
worked, because it failed to use "-Werror=unguarded-availability-new".
(Apparently nobody's tried to build with meson on macOS versions that
lack preadv/pwritev as standard.) Adjust that while at it. Also,
we had never put support for "-Werror=unguarded-availability-new"
into v13, but we need that now.
Co-authored-by: Tom Lane <tgl@sss.pgh.pa.us> Co-authored-by: Peter Eisentraut <peter@eisentraut.org>
Discussion: https://postgr.es/m/385134.1743523038@sss.pgh.pa.us
Backpatch-through: 13
Dean Rasheed [Sat, 29 Mar 2025 09:51:23 +0000 (09:51 +0000)]
Fix MERGE with DO NOTHING actions into a partitioned table.
ExecInitPartitionInfo() duplicates much of the logic in
ExecInitMerge(), except that it failed to handle DO NOTHING
actions. This would cause an "unknown action in MERGE WHEN clause"
error if a MERGE with any DO NOTHING actions attempted to insert into
a partition not already initialised by ExecInitModifyTable().
Bug: #18871 Reported-by: Alexander Lakhin <exclusion@gmail.com>
Author: Tender Wang <tndrwang@gmail.com> Reviewed-by: Gurjeet Singh <gurjeet@singh.im>
Discussion: https://postgr.es/m/18871-b44e3c96de3bd2e8%40postgresql.org
Backpatch-through: 15
Fix guc_malloc calls for consistency and OOM checks
check_createrole_self_grant and check_synchronized_standby_slots
were allocating memory on a LOG elevel without checking if the
allocation succeeded or not, which would have led to a segfault
on allocation failure.
On top of that, a number of callsites were using the ERROR level,
relying on erroring out rather than returning false to allow the
GUC machinery handle it gracefully. Other callsites used WARNING
instead of LOG. While neither being not wrong, this changes all
check_ functions do it consistently with LOG.
init_custom_variable gets a promoted elevel to FATAL to keep
the guc_malloc error handling in line with the rest of the
error handling in that function which already call FATAL. If
we encounter an OOM in this callsite there is no graceful
handling to be had, better to error out hard.
Backpatch the fix to check_createrole_self_grant down to v16
and the fix to check_synchronized_standby_slots down to v17
where they were introduced.
Author: Daniel Gustafsson <daniel@yesql.se> Reported-by: Nikita <pm91.arapov@gmail.com> Reviewed-by: Tom Lane <tgl@sss.pgh.pa.us>
Bug: #18845
Discussion: https://postgr.es/m/18845-582c6e10247377ec@postgresql.org
Backpatch-through: 16
Tom Lane [Thu, 27 Mar 2025 17:20:23 +0000 (13:20 -0400)]
Prevent assertion failure in contrib/pg_freespacemap.
Applying pg_freespacemap() to a relation lacking storage (such as a
view) caused an assertion failure, although there was no ill effect
in non-assert builds. Add an error check for that case.
Michael Paquier [Thu, 27 Mar 2025 01:20:47 +0000 (10:20 +0900)]
doc: Correct description of values used in FSM for indexes
The implementation of FSM for indexes is simpler than heap, where 0 is
used to track if a page is in-use and (BLCKSZ - 1) if a page is free.
One comment in indexfsm.c and one description in the documentation of
pg_freespacemap were incorrect about that.
Author: Alex Friedman <alexf01@gmail.com>
Discussion: https://postgr.es/m/71eef655-c192-453f-ac45-2772fec2cb04@gmail.com
Backpatch-through: 13
Tomas Vondra [Wed, 26 Mar 2025 15:50:13 +0000 (16:50 +0100)]
Keep the decompressed filter in brin_bloom_union
The brin_bloom_union() function combines two BRIN summaries, by merging
one filter into the other. With bloom, we have to decompress the filters
first, but the function failed to update the summary to store the merged
filter. As a consequence, the index may be missing some of the data, and
return false negatives.
This issue exists since BRIN bloom indexes were introduced in Postgres
14, but at that point the union function was called only when two
sessions happened to summarize a range concurrently, which is rare. It
got much easier to hit in 17, as parallel builds use the union function
to merge summaries built by workers.
Fixed by storing a pointer to the decompressed filter, and freeing the
original one. Free the second filter too, if it was decompressed. The
freeing is not strictly necessary, because the union is called in
short-lived contexts, but it's tidy.
Backpatch to 14, where BRIN bloom indexes were introduced.
Reported by Arseniy Mukhin, investigation and fix by me.
Richard Guo [Wed, 26 Mar 2025 08:46:51 +0000 (17:46 +0900)]
Fix integer-overflow problem in scram_SaltedPassword()
Setting the iteration count for SCRAM secret generation to INT_MAX
will cause an infinite loop in scram_SaltedPassword() due to integer
overflow, as the loop uses the "i <= iterations" comparison. To fix,
use "i < iterations" instead.
Back-patch to v16 where the user-settable GUC scram_iterations has
been added.
Author: Kevin K Biju <kevinkbiju@gmail.com> Reviewed-by: Richard Guo <guofenglinux@gmail.com> Reviewed-by: Michael Paquier <michael@paquier.xyz>
Discussion: https://postgr.es/m/CAM45KeEMm8hnxdTOxA98qhfZ9CzGDdgy3mxgJmy0c+2WwjA6Zg@mail.gmail.com
Fix rare assertion failure in standby, if primary is restarted
During hot standby, ExpireAllKnownAssignedTransactionIds() and
ExpireOldKnownAssignedTransactionIds() functions mark old transactions
as no-longer running, but they failed to update xactCompletionCount
and latestCompletedXid. AFAICS it would not lead to incorrect query
results, because those functions effectively turn in-progress
transactions into aborted transactions and an MVCC snapshot considers
both as "not visible". But it could surprise GetSnapshotDataReuse()
and trigger the "TransactionIdPrecedesOrEquals(TransactionXmin,
RecentXmin))" assertion in it, if the apparent xmin in a backend would
move backwards. We saw this happen when GetCatalogSnapshot() would
reuse an older catalog snapshot, when GetTransactionSnapshot() had
already advanced TransactionXmin.
The bug goes back all the way to commit 623a9ba79b in v14 that
introduced the snapshot reuse mechanism, but it started to happen more
frequently with commit 952365cded6 which removed a
GetTransactionSnapshot() call from backend startup. That made it more
likely for ExpireOldKnownAssignedTransactionIds() to be called between
GetCatalogSnapshot() and the first GetTransactionSnapshot() in a
backend.
Andres Freund first spotted this assertion failure on buildfarm member
'skink'. Reproduction and analysis by Tomas Vondra.
Tom Lane [Fri, 21 Mar 2025 15:30:42 +0000 (11:30 -0400)]
Fix plpgsql's handling of simple expressions in scrollable cursors.
exec_save_simple_expr did not account for the possibility that
standard_planner would stick a Materialize node atop the plan
of even a simple Result, if CURSOR_OPT_SCROLL is set. This led
to an "unexpected plan node type" error.
This is a very old bug, but it'd only be reached by declaring a
cursor for a "SELECT simple-expression" query and explicitly
marking it scrollable, which is an odd thing to do. So the lack
of prior reports isn't too surprising.
Bug: #18859 Reported-by: Olleg Samoylov <splarv@ya.ru>
Author: Andrei Lepikhov <lepihov@gmail.com> Reviewed-by: Tom Lane <tgl@sss.pgh.pa.us>
Discussion: https://postgr.es/m/18859-0d5f28ac99a37059@postgresql.org
Backpatch-through: 13
Fujii Masao [Fri, 21 Mar 2025 03:56:39 +0000 (12:56 +0900)]
doc: Remove incorrect description about dropping replication slots.
pg_drop_replication_slot() can drop replication slots created on
a different database than the one where it is executed. This behavior
has been in place since PostgreSQL 9.4, when pg_drop_replication_slot()
was introduced.
However, commit ff539d mistakenly added the following incorrect
description in the documentation:
For logical slots, this must be called when connected to
the same database the slot was created on.
This commit removes that incorrect statement. A similar mistake was
also present in the documentation for the DROP_REPLICATION_SLOT
command, which has now been corrected as well.
Tom Lane [Thu, 13 Mar 2025 20:07:55 +0000 (16:07 -0400)]
Fix ARRAY_SUBLINK and ARRAY[] for int2vector and oidvector input.
If the given input_type yields valid results from both
get_element_type and get_array_type, initArrayResultAny believed the
former and treated the input as an array type. However this is
inconsistent with what get_promoted_array_type does, leading to
situations where the output of an ARRAY() subquery is labeled with
the wrong type: it's labeled as oidvector[] but is really a 2-D
array of OID. That at least results in strange output, and can
result in crashes if further processing such as unnest() is applied.
AFAIK this is only possible with the int2vector and oidvector
types, which are special-cased to be treated mostly as true arrays
even though they aren't quite.
Fix by switching the logic to match get_promoted_array_type by
testing get_array_type not get_element_type, and remove an Assert
thereby made pointless. (We need not introduce a symmetrical
check for get_element_type in the other if-branch, because
initArrayResultArr will check it.) This restores the behavior
that existed before bac27394a introduced initArrayResultAny:
the output really is int2vector[] or oidvector[].
Comparable confusion exists when an input of an ARRAY[] construct
is int2vector or oidvector: transformArrayExpr decides it's dealing
with a multidimensional array constructor, and we end up with
something that's a multidimensional OID array but is alleged to be
of type oidvector. I have not found a crashing case here, but it's
easy to demonstrate totally-wrong results. Adjust that code so
that what you get is an oidvector[] instead, for consistency with
ARRAY() subqueries. (This change also makes these types work like
domains-over-arrays in this context, which seems correct.)
Bug: #18840 Reported-by: yang lei <ylshiyu@126.com>
Author: Tom Lane <tgl@sss.pgh.pa.us>
Discussion: https://postgr.es/m/18840-fbc9505f066e50d6@postgresql.org
Backpatch-through: 13
Handle interrupts while waiting on Append's async subplans
We did not wake up on interrupts while waiting on async events on an
async-capable append node. For example, if you tried to cancel the
query, nothing would happen until one of the async subplans becomes
readable. To fix, add WL_LATCH_SET to the WaitEventSet.
Backpatch down to v14 where async Append execution was introduced.
Tom Lane [Wed, 12 Mar 2025 15:47:19 +0000 (11:47 -0400)]
Build whole-row Vars the same way during parsing and planning.
makeWholeRowVar() has different rules for constructing a
whole-row Var depending on the kind of RTE it's representing.
This turns out to be problematic because the rewriter and planner
can convert view RTEs and set-returning-function RTEs into
subquery RTEs; so a whole-row Var made during planning might
look different from one made by the parser. In isolation this
doesn't cause any problem, but if a query contains Vars made
both ways for the same varno, there are cross-checks in the
executor that will complain. This manifests for UPDATE, DELETE,
and MERGE queries that use whole-row table references.
To fix, we need makeWholeRowVar() to produce the same result
from an inlined RTE as it would have for the original. For
an inlined view, we can use RangeTblEntry.relid to detect
that this had been a view RTE. For inlined SRFs, make a
data structure definition change akin to commit 47bb9db75,
and say that we won't clear RangeTblEntry.functions until
the end of planning. That allows makeWholeRowVar() to
repeat what it would have done with the unmodified RTE.
Reported-by: Duncan Sands <duncan.sands@deepbluecap.com> Reported-by: Dean Rasheed <dean.a.rasheed@gmail.com> Diagnosed-by: Tender Wang <tndrwang@gmail.com>
Author: Tom Lane <tgl@sss.pgh.pa.us> Reviewed-by: Dean Rasheed <dean.a.rasheed@gmail.com>
Discussion: https://postgr.es/m/3518c50a-ab18-482f-b916-a37263622501@deepbluecap.com
Backpatch-through: 13
Ălvaro Herrera [Tue, 11 Mar 2025 11:50:35 +0000 (12:50 +0100)]
BRIN: be more strict about required support procs
With improperly defined operator classes, it's possible to get a
Postgres crash because we'd try to invoke a procedure that doesn't
exist. This is because the code is being a bit too trusting that the
opclass is correctly defined. Add some ereport(ERROR)s for cases where
mandatory support procedures are not defined, transforming the crashes
into errors.
The particular case that was reported is an incomplete opclass in
PostGIS.
Backpatch all the way down to 13.
Reported-by: Tobias Wendorff <tobias.wendorff@tu-dortmund.de> Diagnosed-by: David Rowley <dgrowleyml@gmail.com> Reviewed-by: Tomas Vondra <tomas@vondra.me>
Discussion: https://postgr.es/m/fb6d9a35-6c8e-4869-af80-0a4944a793a4@tu-dortmund.de
Fix snapshot used in logical replication index lookup
The function calls GetLatestSnapshot() to acquire a fresh snapshot,
makes it active, and was meant to pass it to table_tuple_lock(), but
instead called GetLatestSnapshot() again to acquire yet another
snapshot. It was harmless because the heap AM and all other known
table AMs ignore the 'snapshot' argument anyway, but let's be tidy.
In the long run, this perhaps should be redesigned so that snapshot
was not needed in the first place. The table AM API uses TID +
snapshot as the unique identifier for the row version, which is
questionable when the row came from an index scan with a Dirty
snapshot. You might lock a different row version when you use a
different snapshot in the table_tuple_lock() call (a fresh MVCC
snapshot) than in the index scan (DirtySnapshot). However, in the heap
AM and other AMs where the TID alone identifies the row version, it
doesn't matter. So for now, just fix the obvious albeit harmless bug.
This has been wrong ever since the table AM API was introduced in
commit 5db6df0c01, so backpatch to all supported versions.
Tom Lane [Mon, 10 Mar 2025 14:22:08 +0000 (10:22 -0400)]
Doc: improve description of window function processing.
The previous wording talked about a "single pass over the data",
which can be read as promising more than intended (to wit, that only
one WindowAgg plan node will be used). What we promise is only what
the SQL spec requires, namely that the data not get re-sorted between
window functions with compatible PARTITION BY/ORDER BY clauses.
Adjust the wording in hopes of making this clearer.
Reported-by: Christopher Inokuchi <cinokuchi@gmail.com>
Author: Tom Lane <tgl@sss.pgh.pa.us> Reviewed-by: David G. Johnston <david.g.johnston@gmail.com>
Discussion: https://postgr.es/m/CABde6B5va2wMsnM79u_x=n9KUgfKQje_pbLROEBmA9Ru5XWidw@mail.gmail.com
Backpatch-through: 13
Tom Lane [Sun, 9 Mar 2025 17:11:20 +0000 (13:11 -0400)]
Don't try to parallelize array_agg() on an anonymous record type.
This doesn't work because record_recv requires the typmod that
identifies the specific record type (in our session) and
array_agg_deserialize has no convenient way to get that information.
The result is an "input of anonymous composite types is not
implemented" error.
We could probably make this work if we had to, but it does not seem
worth the trouble, given that it took this long to get a field report.
Just shut off parallelization, as though record_recv didn't exist.
Oversight in commit 16fd03e95. Back-patch to v16 where that
came in.
Reported-by: Kirill Zdornyy <kirill@dineserve.com> Diagnosed-by: Richard Guo <guofenglinux@gmail.com>
Author: Tom Lane <tgl@sss.pgh.pa.us> Reviewed-by: David Rowley <dgrowleyml@gmail.com>
Discussion: https://postgr.es/m/atLI5Kce2ie1zcYjU0w_kjtVaxiYbYGTihrkLDmGZQnRDD4pnXukIATaABbnIj9pUnelC4ESvCXMm4HAyHg-v61XABaKpERj0A2IXzJZM7g=@dineserve.com
Backpatch-through: 16
Tom Lane [Sat, 8 Mar 2025 16:24:22 +0000 (11:24 -0500)]
Clear errno before calling strtol() in spell.c.
Per POSIX, a caller of strtol() that wishes to check for errors must
set errno to 0 beforehand. Several places in spell.c neglected that,
so that they risked delivering a false overflow error in case errno
had been ERANGE already. Given the lack of field reports, this case
may be unreachable at present --- but it's surely trouble waiting to
happen, so fix it.
Author: Jacob Brazeal <jacob.brazeal@gmail.com>
Discussion: https://postgr.es/m/CA+COZaBhsq6EromFm+knMJfzK6nTpG23zJ+K2=nfUQQXcj_xcQ@mail.gmail.com
Backpatch-through: 13
John Naylor [Fri, 7 Mar 2025 03:22:56 +0000 (10:22 +0700)]
Doc: correct aggressive vacuum threshold for multixact members storage
The threshold is two billion members, which was interpreted as 2GB
in the documentation. Fix to reflect that each member takes up five
bytes, which translates to about 10GB. This is not exact, because of
page boundaries. While at it, mention the maximum size 20GB.
This has been wrong since commit c552e171d16e, so backpatch to
version 14.
Author: Alex Friedman <alexf01@gmail.com> Reviewed-by: Sami Imseih <samimseih@gmail.com> Reviewed-by: Bertrand Drouvot <bertranddrouvot.pg@gmail.com>
Discussion: https://postgr.es/m/CACbFw60UOk6fCC02KsyT3OfU9Dnuq5roYxdw2aFisiN_p1L0bg@mail.gmail.com
Backpatch-through: 14
Tom Lane [Thu, 6 Mar 2025 16:54:27 +0000 (11:54 -0500)]
Fix some performance issues in GIN query startup.
If a GIN index search had a lot of search keys (for example,
"jsonbcol ?| array[]" with tens of thousands of array elements),
both ginFillScanKey() and startScanKey() took O(N^2) time.
Worse, those loops were uncancelable for lack of CHECK_FOR_INTERRUPTS.
The problem in ginFillScanKey() is the brute-force search key
de-duplication done in ginFillScanEntry(). The most expedient
solution seems to be to just stop trying to de-duplicate once
there are "too many" search keys. We could imagine working harder,
say by using a sort-and-unique algorithm instead of brute force
compare-all-the-keys. But it seems unlikely to be worth the trouble.
There is no correctness issue here, since the code already allowed
duplicate keys if any extra_data is present.
The problem in startScanKey() is the loop that attempts to identify
the first non-required search key. In the submitted test case, that
vainly tests all the key positions, and each iteration takes O(N)
time. One part of that is that it's reinitializing the entryRes[]
array from scratch each time, which is entirely unnecessary given
that the triConsistentFn isn't supposed to scribble on its input.
We can easily adjust the array contents incrementally instead.
The other part of it is that the triConsistentFn may itself take
O(N) time (and does in this test case). This is all extremely
brute force: in simple cases with AND or OR semantics, we could
know without any looping whatever that all or none of the keys
are required. But GIN opclasses don't have any API for exposing
that knowledge, so at least in the short run there is little to
be done about that. Put in a CHECK_FOR_INTERRUPTS so that at
least the loop is cancelable.
These two changes together resolve the primary complaint that
the test query doesn't respond promptly to cancel interrupts.
Also, while they don't completely eliminate the O(N^2) behavior,
they do provide quite a nice speedup for mid-sized examples.
Bug: #18831 Reported-by: Niek <niek.brasa@hitachienergy.com>
Author: Tom Lane <tgl@sss.pgh.pa.us>
Discussion: https://postgr.es/m/18831-e845ac44ebc5dd36@postgresql.org
Backpatch-through: 13
Andres Freund [Wed, 5 Mar 2025 15:29:03 +0000 (10:29 -0500)]
ci: Upgrade FreeBSD image
Upgrade to the current stable version. To avoid needing commits like this in
the future, the CI image name now doesn't contain the OS version number
anymore.
Backpatch to all versions with CI support, we don't want to generate CI images
for multiple FreeBSD versions.
Author: Nazir Bilal Yavuz <byavuz81@gmail.com>
Discussion: https://postgr.es/m/CAN55FZ3_P4JJ6tWZafjf-_XbHgG6DQGXhH-y6Yp78_bwBJjcww@mail.gmail.com
Ălvaro Herrera [Tue, 4 Mar 2025 19:07:30 +0000 (20:07 +0100)]
Fix ALTER TABLE error message
This bogus error message was introduced in 2013 by commit f177cbfe676d,
because of misunderstanding the processCASbits() API; at the time, no
test cases were added that would be affected by this change. Only in ca87c415e2fc was one added (along with a couple of typos), with an XXX
note that the error message was bogus. Fix the whole, add some test
cases.
Tom Lane [Mon, 3 Mar 2025 17:43:29 +0000 (12:43 -0500)]
Fix broken handling of domains in atthasmissing logic.
If a domain type has a default, adding a column of that type (without
any explicit DEFAULT clause) failed to install the domain's default
value in existing rows, instead leaving the new column null. This
is unexpected, and it used to work correctly before v11. The cause
is confusion in the atthasmissing mechanism about which default value
to install: we'd only consider installing an explicitly-specified
default, and then we'd decide that no table rewrite is needed.
To fix, take the responsibility for filling attmissingval out of
StoreAttrDefault, and instead put it into ATExecAddColumn's existing
logic that derives the correct value to fill the new column with.
Also, centralize the logic that determines the need for
default-related table rewriting there, instead of spreading it over
four or five places.
In the back branches, we'll leave the attmissingval-filling code
in StoreAttrDefault even though it's now dead, for fear that some
extension may be depending on that functionality to exist there.
A separate HEAD-only patch will clean up the now-useless code.
Reported-by: jian he <jian.universality@gmail.com>
Author: jian he <jian.universality@gmail.com>
Author: Tom Lane <tgl@sss.pgh.pa.us>
Discussion: https://postgr.es/m/CACJufxHFssPvkP1we7WMhPD_1kwgbG52o=kQgL+TnVoX5LOyCQ@mail.gmail.com
Backpatch-through: 13
Tom Lane [Sat, 1 Mar 2025 19:22:56 +0000 (14:22 -0500)]
Fix pg_strtof() to not crash on NULL endptr.
We had managed not to notice this simple oversight because none
of our calls exercised the case --- until commit 8f427187d.
That led to pg_dump crashing on any platform that uses this code
(currently Cygwin and Mingw).
Even though there's no immediate bug in the back branches, backpatch,
because a non-POSIX-compliant strtof() substitute is trouble waiting
to happen for extensions or future back-patches.
Diagnosed-by: Alexander Lakhin <exclusion@gmail.com>
Author: Tom Lane <tgl@sss.pgh.pa.us>
Discussion: https://postgr.es/m/339b3902-4e98-4e31-a744-94e43b7b9292@gmail.com
Backpatch-through: 13
Michael Paquier [Fri, 28 Feb 2025 01:15:34 +0000 (10:15 +0900)]
pg_upgrade: Fix inconsistency in memory freeing
The function in charge of freeing the memory from a result created by
PQescapeIdentifier() has to be PQfreemem(), to ensure that both
allocation and free come from libpq.
One spot in pg_upgrade was not respecting that for pg_database's
datlocale (daticulocale in v16) when the collation provider is libc (aka
datlocale/daticulocale is NULL) with an allocation done using
pg_strdup() and a free with PQfreemem(). The code is changed to always
use PQescapeLiteral() when processing the input.
Michael Paquier [Thu, 27 Feb 2025 05:05:57 +0000 (14:05 +0900)]
pg_amcheck: Fix inconsistency in memory freeing
The function in charge of freeing the memory from a result created by
PQescapeIdentifier() has to be PQfreemem(), to ensure that both
allocation and free come from libpq, but one spot in pg_amcheck was
missing that.
Masahiko Sawada [Mon, 24 Feb 2025 22:03:10 +0000 (14:03 -0800)]
Fix assertion when decoding XLOG_PARAMETER_CHANGE on promoted primary.
When a standby replays an XLOG_PARAMETER_CHANGE record that lowers
wal_level below logical, we invalidate all logical slots in hot
standby mode. However, if this record was replayed while not in hot
standby mode, logical slots could remain valid even after promotion,
potentially causing an assertion failure during WAL record decoding.
To fix this issue, this commit adds a check for hot_standby status
when restoring a logical replication slot on standbys. This check
ensures that logical slots are invalidated when they become
incompatible due to insufficient wal_level during recovery.
Backpatch to v16 where logical decoding on standby was introduced.
Tom Lane [Fri, 21 Feb 2025 18:37:12 +0000 (13:37 -0500)]
Fix pg_dumpall to cope with dangling OIDs in pg_auth_members.
There is a race condition between "GRANT role" and "DROP ROLE",
which allows GRANT to install pg_auth_members entries that refer to
dropped roles. (Commit 6566133c5 prevented that for the grantor
field, but not for the granted or grantee roles.) We'll soon fix
that, at least in HEAD, but pg_dumpall needs to cope with the
situation in case of pre-existing inconsistency. As pg_dumpall
stands, it will emit invalid commands like 'GRANT foo TO ""',
which causes pg_upgrade to fail. Fix it to emit warnings and skip
those GRANTs, instead.
There was some discussion of removing the problem by changing
dumpRoleMembership's query to use JOIN not LEFT JOIN, but that
would result in silently ignoring such entries. It seems better
to produce a warning.
Pre-v16 branches already coped with dangling grantor OIDs by simply
omitting the GRANTED BY clause. I left that behavior as-is, although
it's somewhat inconsistent with the behavior of later branches.
Andres Freund [Fri, 21 Feb 2025 16:16:57 +0000 (11:16 -0500)]
Make test portlock logic work with meson
Previously the portlock logic, added in 9b4eafcaf41, didn't actually work
properly when the tests were run via meson. 9b4eafcaf41 used the
MESON_BUILD_ROOT environment variable to determine the directory for the port
lock directory, but that's never set for running the tests. That meant that
each test used its own portlock dir, unless the PG_TEST_PORT_DIR environment
variable was set.
Fix the problem by setting top_builddir for the environment. That's also used
for the autoconf/make build.
Backpatch back to 16, where meson support was added.
Reported-by: Zharkov Roman <r.zharkov@postgrespro.ru> Reviewed-by: Andrew Dunstan <andrew@dunslane.net>
Backpatch-through: 16
Michael Paquier [Fri, 21 Feb 2025 11:37:38 +0000 (20:37 +0900)]
Fix cross-version upgrades with XMLSERIALIZE(NO INDENT)
Dumps from versions older than v16 do not know about NO INDENT in a
XMLSERIALIZE() clause. This commit adjusts AdjustUpgrade.pm so as NO
INDENT is discarded in the contents of the new dump adjusted for
comparison when the old version is v15 or older. This should be enough
to make the cross-version upgrade tests pass.
Per report from buildfarm member crake. Oversight in 984410b92326.
Reviewed-by: Andrew Dunstan <andrew@dunslane.net>
Discussion: https://postgr.es/m/88b183f1-ebf9-4f51-9144-3704380ccae7@dunslane.net
Backpatch-through: 16
Amit Kapila [Fri, 21 Feb 2025 08:38:27 +0000 (14:08 +0530)]
Fix a WARNING for data origin discrepancies.
Previously, a WARNING was issued at the time of defining a subscription
with origin=NONE only when the publisher subscribed to the same table from
other publishers, indicating potential data origination from different
origins. However, the publisher can subscribe to the partition ancestors
or partition children of the table from other publishers, which could also
result in mixed-origin data inclusion. So, give a WARNING in those cases
as well.
Reported-by: Sergey Tatarintsev <s.tatarintsev@postgrespro.ru>
Author: Hou Zhijie <houzj.fnst@fujitsu.com>
Author: Shlok Kyal <shlok.kyal.oss@gmail.com> Reviewed-by: Vignesh C <vignesh21@gmail.com> Reviewed-by: Amit Kapila <amit.kapila16@gmail.com>
Backpatch-through: 16, where it was introduced
Discussion: https://postgr.es/m/5eda6a9c-63cf-404d-8a49-8dcb116a29f3@postgrespro.ru
Author: Jim Jones <jim.jones@uni-muenster.de>
Discussion: https://postgr.es/m/bebd457e-5b43-46b3-8fc6-f6a6509483ba@uni-muenster.de
Backpatch-through: 16
Michael Paquier [Thu, 20 Feb 2025 01:43:38 +0000 (10:43 +0900)]
Fix FATAL message for invalid recovery timeline at beginning of recovery
If the requested recovery timeline is not reachable, the logged
checkpoint and timeline should to be the values read from the
backup_label when it is defined. The message generated used the values
from the control file in this case, which is fine when recovering from
the control file without a backup_label, but not if there is a
backup_label.
Issue introduced in ee994272ca50. v15 has introduced xlogrecovery.c and
more simplifications in this area (4a92a1c3d1c3, a27048cbcb58), making
this change a bit simpler to think about, so backpatch only down to this
version.
Andres Freund [Wed, 19 Feb 2025 15:45:48 +0000 (10:45 -0500)]
tests: BackgroundPsql: Fix potential for lost errors on windows
This addresses various corner cases in BackgroundPsql:
- On windows stdout and stderr may arrive out of order, leading to errors not
being reported, or attributed to the wrong statement.
To fix, emit the "query-separation banner" on both stdout and stderr and
wait for both.
- Very occasionally the "query-separation banner" would not get removed, because
we waited until the banner arrived, but then replaced the banner plus
newline.
To fix, wait for banner and newline.
- For interactive psql replacing $banner\n is not sufficient, interactive psql
outputs \r\n.
- For interactive psql, where commands are echoed to stdout, the \echo
command, rather than its output, would be matched.
This would sometimes lead to output from the prior query, or wait_connect(),
being returned in the next command.
This also affected wait_connect(), leading to sometimes sending queries to
psql before the connection actually was established.
While debugging these issues I also found that it's hard to know whether a
query separation banner was attributed to the right query. Make that easier by
counting the queries each BackgroundPsql instance has emitted and include the
number in the banner.
Also emit psql stdout/stderr in query() and wait_connect() as Test::More
notes, without that it's rather hard to debug some issues in CI and buildfarm.
As this can cause issues not just to-be-added tests, but also existing ones,
backpatch the fix to all supported versions.
Andres Freund [Wed, 19 Feb 2025 14:41:08 +0000 (09:41 -0500)]
backport: Extend background_psql() to be able to start asynchronously
This is a backport of ba08edb0654. Originally it was only applied to master,
but I (Andres) am planning to fix a few bugs in BackgroundPsql, which would be
somewhat harder with the behavioural differences across branches. It's also
generally good for test infrastructure to behave similarly across branches, to
avoid pain during backpatching.
This commit extends the constructor routine of BackgroundPsql.pm with a
new "wait" parameter. If set to 0, the routine returns without waiting
for psql to start, ready to consume input.
background_psql() in Cluster.pm gains the same "wait" parameter. The
default behavior is still to wait for psql to start. It becomes now
possible to not wait, giving to TAP scripts the possibility to perform
actions between a BackgroundPsql startup and its wait_connect() call.
Author: Jacob Champion
Discussion: https://postgr.es/m/CAOYmi+=60deN20WDyCoHCiecgivJxr=98s7s7-C8SkXwrCfHXg@mail.gmail.com
Andres Freund [Wed, 19 Feb 2025 14:39:49 +0000 (09:39 -0500)]
backport: Improve handling of empty query results in BackgroundPsql
This is a backport of 70291a3c66e. Originally it was only applied to master,
but I (Andres) am planning to fix a few bugs in BackgroundPsql that are harder
to fix in the backbranches with the old behavior. It's also generally good for
test infrastructure to behave similarly across branches, to avoid pain during
backpatching. 70291a3c66e changes the behavior in some cases, but after
discussing it, we are ok with that, it seems unlikely that there are
out-of-core tests relying on the prior behavior.
A newline is not added at the end of an empty query result, causing the
banner of the hardcoded \echo to not be discarded. This would reflect
on scripts that expect an empty result by showing the "QUERY_SEPARATOR"
in the output returned back to the caller, which was confusing.
This commit changes BackgroundPsql::query() so as empty results are able
to work correctly, making the first newline before the banner optional,
bringing more flexibility.
Note that this change affects 037_invalid_database.pl, where three
queries generated an empty result, with the script relying on the data
from the hardcoded banner to exist in the expected output. These
queries are changed to use query_safe(), leading to a simpler script.
The author has also proposed a test in a different patch where empty
results would exist when using BackgroundPsql.
Author: Jacob Champion Reviewed-by: Andrew Dunstan, Michael Paquier
Discussion: https://postgr.es/m/CAOYmi+=60deN20WDyCoHCiecgivJxr=98s7s7-C8SkXwrCfHXg@mail.gmail.com
Tom Lane [Wed, 19 Feb 2025 02:23:59 +0000 (21:23 -0500)]
Avoid null pointer dereference crash after OOM in Snowball stemmers.
Absorb upstream bug fix (their commit e322673a841d9abd69994ae8cd20e191090b6ef4), which prevents a null
pointer dereference crash if SN_create_env() gets a malloc failure
at just the wrong point.
Thanks to Maksim Korotkov for discovering the null-pointer
bug and submitting the fix to upstream snowball.
Reported-by: Maksim Korotkov <m.korotkov@postgrespro.ru>
Author: Maksim Korotkov <m.korotkov@postgrespro.ru>
Discussion: https://postgr.es/m/1d1a46-67ab1000-21-80c451@83151435
Backpatch-through: 13
Richard Guo [Wed, 19 Feb 2025 02:05:35 +0000 (11:05 +0900)]
Fix unsafe access to BufferDescriptors
When considering a local buffer, the GetBufferDescriptor() call in
BufferGetLSNAtomic() would be retrieving a shared buffer with a bad
buffer ID. Since the code checks whether the buffer is shared before
using the retrieved BufferDesc, this issue did not lead to any
malfunction. Nonetheless this seems like trouble waiting to happen,
so fix it by ensuring that GetBufferDescriptor() is only called when
we know the buffer is shared.
Author: Tender Wang <tndrwang@gmail.com> Reviewed-by: Xuneng Zhou <xunengzhou@gmail.com> Reviewed-by: Richard Guo <guofenglinux@gmail.com>
Discussion: https://postgr.es/m/CAHewXNku-o46-9cmUgyv6LkSZ25doDrWq32p=oz9kfD8ovVJMg@mail.gmail.com
Backpatch-through: 13
Michael Paquier [Wed, 19 Feb 2025 00:45:56 +0000 (09:45 +0900)]
test_escape: Fix handling of short options in getopt_long()
This addresses two errors in the module, based on the set of options
supported:
- '-c', for --conninfo, was not listed.
- '-f', for --force-unsupported, was not listed.
While on it, these are now listed in an alphabetical order.
Author: Japin Li
Discussion: https://postgr.es/m/ME0P300MB04451FB20CE0346A59C25CADB6FA2@ME0P300MB0445.AUSP300.PROD.OUTLOOK.COM
Backpatch-through: 13
Tom Lane [Sun, 16 Feb 2025 17:46:35 +0000 (12:46 -0500)]
In fmtIdEnc(), handle failure of enlargePQExpBuffer().
Coverity complained that we weren't doing that, and it's right.
This fix just makes fmtIdEnc() honor the general convention that OOM
causes a PQExpBuffer to become marked "broken", without any immediate
error. In the pretty-unlikely case that we actually did hit OOM here,
the end result would be to return an empty string to the caller,
probably resulting in invalid SQL syntax in an issued command (if
nothing else went wrong, which is even more unlikely). It's tempting
to throw an "out of memory" error if the buffer becomes broken, but
there's not a lot of point in doing that only here and not in hundreds
of other PQExpBuffer-using places in pg_dump and similar callers.
The whole issue could do with some non-time-crunched redesign, perhaps.
This is a followup to the fixes for CVE-2025-1094, and should be
included if cherry-picking those fixes.
Tom Lane [Sat, 15 Feb 2025 21:20:21 +0000 (16:20 -0500)]
Make escaping functions retain trailing bytes of an invalid character.
Instead of dropping the trailing byte(s) of an invalid or incomplete
multibyte character, replace only the first byte with a known-invalid
sequence, and process the rest normally. This seems less likely to
confuse incautious callers than the behavior adopted in 5dc1e42b4.
While we're at it, adjust PQescapeStringInternal to produce at most
one bleat about invalid multibyte characters per string. This
matches the behavior of PQescapeInternal, and avoids the risk of
producing tons of repetitive junk if a long string is simply given
in the wrong encoding.
This is a followup to the fixes for CVE-2025-1094, and should be
included if cherry-picking those fixes.
Author: Andres Freund <andres@anarazel.de> Co-authored-by: Tom Lane <tgl@sss.pgh.pa.us> Reported-by: Jeff Davis <pgsql@j-davis.com>
Discussion: https://postgr.es/m/20250215012712.45@rfd.leadboat.com
Backpatch-through: 13
In 5dc1e42b4fa I fixed bugs in various escape functions, unfortunately as part
of that I introduced a new bug in PQescapeLiteral()/PQescapeIdentifier(). The
bug is that I made PQescapeInternal() just use strlen(), rather than taking
the specified input length into account.
That's bad, because it can lead to including input that wasn't intended to be
included (in case len is shorter than null termination of the string) and
because it can lead to reading invalid memory if the input string is not null
terminated.
Expand test_escape to this kind of bug:
a) for escape functions with length support, append data that should not be
escaped and check that it is not
b) add valgrind requests to detect access of bytes that should not be touched
Author: Tom Lane <tgl@sss.pgh.pa.us>
Author: Andres Freund <andres@anarazel.de Reviewed-by: Tom Lane <tgl@sss.pgh.pa.us> Reviewed-by: Noah Misch <noah@leadboat.com>
Discussion: https://postgr.es/m/Z64jD3u46gObCo1p@pryzbyj2023
Backpatch: 13
Commit 27cc7cd2bc8a accidentally placed the assertion ensuring
that the pointer isn't NULL after it had already been accessed.
Fix by moving the pointer dereferencing to after the assertion.
Backpatch to all supported branches.
Author: Dmitry Koval <d.koval@postgrespro.ru> Reviewed-by: Daniel Gustafsson <daniel@yesql.se> Reviewed-by: Michael Paquier <michael@paquier.xyz>
Discussion: https://postgr.es/m/1618848d-cdc7-414b-9c03-08cf4bef4408@postgrespro.ru
Backpatch-through: 13
Michael Paquier [Thu, 13 Feb 2025 07:31:08 +0000 (16:31 +0900)]
Fix MakeTransitionCaptureState() to return a consistent result
When an UPDATE trigger referencing a new table and a DELETE trigger
referencing an old table are both present, MakeTransitionCaptureState()
returns an inconsistent result for UPDATE commands in its set of flags
and tuplestores holding the TransitionCaptureState for transition
tables.
As proved by the test added here, this issue causes a crash in v14 and
earlier versions (down to 11, actually, older versions do not support
triggers on partitioned tables) during cross-partition updates on a
partitioned table. v15 and newer versions are safe thanks to 7103ebb7aae8.
This commit fixes the function so that it returns a consistent state
by using portions of the changes made in commit 7103ebb7aae8 for v13 and
v14. v15 and newer versions are slightly tweaked to match with the
older versions, mainly for consistency across branches.
Andres Freund [Wed, 12 Feb 2025 13:15:54 +0000 (08:15 -0500)]
meson: Fix failure to detect bsd_auth.h presence
bsd_auth.h file needs to be included after 'sys/types.h', as documented in
https://man.openbsd.org/authenticate.3
The reason a similar looking stanza works for autoconf is that autoconf
automatically adds AC_INCLUDES_DEFAULT, which in turn includes sys/types.h.
Backpatch to all versions with meson support.
Author: Nazir Bilal Yavuz <byavuz81@gmail.com>
Discussion: https://postgr.es/m/637haqqyhg2wlz7q6wq25m2qupe67g7f2uupngzui64zypy4x2@ysr2xnmynmu4
Backpatch-through: 16
Tom Lane [Mon, 10 Feb 2025 21:30:03 +0000 (16:30 -0500)]
Adapt appendPsqlMetaConnect() to the new fmtId() encoding expectations.
We need to tell fmtId() what encoding to assume, but this function
doesn't know that. Fortunately we can fix that without changing the
function's API, because we can just use SQL_ASCII. That's because
database names in connection requests are effectively binary not text:
no encoding-aware processing will happen on them.
This fixes XversionUpgrade failures seen in the buildfarm. The
alternative of having pg_upgrade use setFmtEncoding() is unappetizing,
given that it's connecting to multiple databases that may have
different encodings.
Andres Freund [Mon, 10 Feb 2025 15:03:39 +0000 (10:03 -0500)]
Fix handling of invalidly encoded data in escaping functions
Previously invalidly encoded input to various escaping functions could lead to
the escaped string getting incorrectly parsed by psql. To be safe, escaping
functions need to ensure that neither invalid nor incomplete multi-byte
characters can be used to "escape" from being quoted.
Functions which can report errors now return an error in more cases than
before. Functions that cannot report errors now replace invalid input bytes
with a byte sequence that cannot be used to escape the quotes and that is
guaranteed to error out when a query is sent to the server.
The following functions are fixed by this commit:
- PQescapeLiteral()
- PQescapeIdentifier()
- PQescapeString()
- PQescapeStringConn()
- fmtId()
- appendStringLiteral()
Reported-by: Stephen Fewer <stephen_fewer@rapid7.com> Reviewed-by: Noah Misch <noah@leadboat.com> Reviewed-by: Tom Lane <tgl@sss.pgh.pa.us>
Backpatch-through: 13
Security: CVE-2025-1094
Andres Freund [Mon, 10 Feb 2025 15:03:39 +0000 (10:03 -0500)]
Specify the encoding of input to fmtId()
This commit adds fmtIdEnc() and fmtQualifiedIdEnc(), which allow to specify
the encoding as an explicit argument. Additionally setFmtEncoding() is
provided, which defines the encoding when no explicit encoding is provided, to
avoid breaking all code using fmtId().
All users of fmtId()/fmtQualifiedId() are either converted to the explicit
version or a call to setFmtEncoding() has been added.
This commit does not yet utilize the now well-defined encoding, that will
happen in a subsequent commit.
Reviewed-by: Noah Misch <noah@leadboat.com> Reviewed-by: Tom Lane <tgl@sss.pgh.pa.us>
Backpatch-through: 13
Security: CVE-2025-1094
Andres Freund [Mon, 10 Feb 2025 15:03:39 +0000 (10:03 -0500)]
Add pg_encoding_set_invalid()
There are cases where we cannot / do not want to error out for invalidly
encoded input. In such cases it can be useful to replace e.g. an incomplete
multi-byte characters with bytes that will trigger an error when getting
validated as part of a larger string.
Unfortunately, until now, for some encoding no such sequence existed. For
those encodings this commit removes one previously accepted input combination
- we consider that to be ok, as the chosen bytes are outside of the valid
ranges for the encodings, we just previously failed to detect that.
As we cannot add a new field to pg_wchar_table without breaking ABI, this is
implemented "in-line" in the newly added function.
Tom Lane [Fri, 7 Feb 2025 18:41:42 +0000 (13:41 -0500)]
Fix pgbench performance issue induced by commit af35fe501.
Commit af35fe501 caused "pgbench -i" to emit a '\r' character
for each data row loaded (when stderr is a terminal).
That's effectively invisible on-screen, but it causes the
connected terminal program to consume a lot of cycles.
It's even worse if you're connected over ssh, as the data
then has to pass through the ssh tunnel.
Simplest fix is to move the added logic inside the if-tests
that check whether to print a progress line. We could do
it another way that avoids duplicating these few lines,
but on the whole this seems the most transparent way to
write it.
Like the previous commit, back-patch to all supported versions.
Reported-by: Andres Freund <andres@anarazel.de>
Author: Tom Lane <tgl@sss.pgh.pa.us> Reviewed-by: Nathan Bossart <nathandbossart@gmail.com>
Discussion: https://postgr.es/m/4k4drkh7bcmdezq6zbkhp25mnrzpswqi2o75d5uv2eeg3aq6q7@b7kqdmzzwzgb
Backpatch-through: 13
Tom Lane [Fri, 7 Feb 2025 17:40:41 +0000 (12:40 -0500)]
Doc: clarify behavior of timestamptz input some more.
Try to make it absolutely plain that we don't retain the
originally specified time zone, only the UTC timestamp.
While at it, make glossary entries for "UTC" and "GMT".
Author: Robert Treat <rob@xzilla.net> Co-authored-by: Tom Lane <tgl@sss.pgh.pa.us>
Discussion: https://postgr.es/m/173796426022.1064.9135167366862649513@wrigleys.postgresql.org
Backpatch-through: 13
Before OpenSSL 1.1.0 the legacy names ssleay32 and libeay32 were
still used on Windows, and while we have support for this auto-
conf the meson buildsystem only used the new names on all plat-
forms. This adds support for the old name scheme when building
on Windows.
This patch only applies to 17 and 16 as master no longer support
OpenSSL 1.0.2.
Author: Darek Ćlusarczyk <dslusarczyk@splunk.com> Reviewed-by: Nazir Bilal Yavuz <byavuz81@gmail.com> Reviewed-by: Daniel Gustafsson <daniel@yesql.se>
Discussion: https://postgr.es/m/CAN55FZ1Nk8wqY=mTrN78H026TuGV50h2H6uq1PwxhTauPYi3ug@mail.gmail.com
Backpatch-through: 16
Two links in the isn module documentation were pointing to tools
which had been moved, resulting in 404 error responses. Update
to the new URLs for the tools. The link to the Sequoia 2000 page
in the history section was no longer working, and since the page
is no longer available online update our link to point at the
paper instead which is on a stable URL.
These links exist in all versions of the documentation so backpatch
to all supported branches.
Author: Daniel Gustafsson <daniel@yesql.se> Reported-by: charukiewicz@protonmail.com
Discussion: https://postgr.es/m/173679670185.705.8565555804465055355@wrigleys.postgresql.org
Backpatch-through: 13
Andres Freund [Tue, 4 Feb 2025 22:45:57 +0000 (17:45 -0500)]
meson: ci: ensure tests are built before running them
Meson 1.7 stopped building all the dependencies of tests as part of the
default build target. But it does breaks CI because we only built the default
target before running the test, and ran the tests with --no-rebuild.
The simplest fix would be to remove --no-rebuild from MTEST_ARGS, but it seems
better to explicitly build the test dependencies, so compiler warnings /
errors are visible as part of the build step.
Discussion: https://postgr.es/m/CAGECzQSvM3iSDmjF+=Kof5an6jN8UbkP_4cKKT9w6GZavmb5yQ@mail.gmail.com
Backpatch: 16-, where meson was added
Andres Freund [Tue, 4 Feb 2025 22:45:57 +0000 (17:45 -0500)]
meson: Add missing dependencies for libpq tests
The missing dependency was, e.g., visible when doing
ninja clean && ninja meson-test-prereq && meson test --no-rebuild --suite setup --suite libpq
This is a bit more complicated than other related fixes, because until now
libpq's tests depended on 'frontend_code', which includes a dependency on
fe_utils, which in turns on libpq. That in turn required
src/interfaces/libpq/test to be entered from the top-level, not from
libpq/meson.build. Because of that the test definitions in libpq/meson.build
could not declare a dependency on the binaries defined in
libpq/test/meson.build.
To fix this, this commit creates frontend_no_fe_utils_code, which allows us to
recurse into libpq/test from withing libpq/meson.build.
Apply this to all branches with meson support, as part of an effort to fix
incorrect test dependencies that can lead to test failures.
Discussion: https://postgr.es/m/CAGECzQSvM3iSDmjF+=Kof5an6jN8UbkP_4cKKT9w6GZavmb5yQ@mail.gmail.com
Discussion: https://postgr.es/m/bdba588f-69a9-4f3e-9b95-62d07210a32e@eisentraut.org
Backpatch: 16-, where meson support was added
Andres Freund [Tue, 4 Feb 2025 22:45:57 +0000 (17:45 -0500)]
meson: Add missing dependencies to libpq_pipeline test
The missing dependency was, e.g., visible when doing
ninja clean && ninja meson-test-prereq && meson test --no-rebuild --suite setup --suite libpq_pipeline
Apply this to all branches with meson support, as part of an effort to fix
incorrect test dependencies that can lead to test failures.
Discussion: https://postgr.es/m/CAGECzQSvM3iSDmjF+=Kof5an6jN8UbkP_4cKKT9w6GZavmb5yQ@mail.gmail.com
Discussion: https://postgr.es/m/bdba588f-69a9-4f3e-9b95-62d07210a32e@eisentraut.org
Backpatch: 16-, where meson support was added
Andres Freund [Tue, 4 Feb 2025 22:45:57 +0000 (17:45 -0500)]
meson: Add pg_regress_ecpg to ecpg test dependencies
This is required to ensure correct test dependencies, previously
pg_regress_ecpg would not necessarily be built.
The missing dependency was, e.g., visible when doing
ninja clean && ninja meson-test-prereq && meson test --no-rebuild --suite setup --suite ecpg
Apply this to all branches with meson support, as part of an effort to fix
incorrect test dependencies that can lead to test failures.
Discussion: https://postgr.es/m/CAGECzQSvM3iSDmjF+=Kof5an6jN8UbkP_4cKKT9w6GZavmb5yQ@mail.gmail.com
Discussion: https://postgr.es/m/bdba588f-69a9-4f3e-9b95-62d07210a32e@eisentraut.org
Backpatch: 16-, where meson support was added
Andres Freund [Tue, 4 Feb 2025 22:45:57 +0000 (17:45 -0500)]
meson: Improve dependencies for tmp_install test target
The missing dependency was, e.g., visible when doing
ninja clean && ninja meson-test-prereq && meson test --no-rebuild --suite setup --suite cube
because meson (and thus its internal meson-test-prereq target) did not know
about a lot of the required targets.
Previously tmp_install did not actually depend on the relevant files being
built. That was mostly not visible, because "meson test" currently uses the
'default' targets as a test's dependency if no dependency is specified.
However, there are plans to narrow that on the meson side, to make it quicker
to run tests.
Apply this to all branches with meson support, as part of an effort to fix
incorrect test dependencies that can lead to test failures.
Discussion: https://postgr.es/m/CAGECzQSvM3iSDmjF+=Kof5an6jN8UbkP_4cKKT9w6GZavmb5yQ@mail.gmail.com
Discussion: https://postgr.es/m/bdba588f-69a9-4f3e-9b95-62d07210a32e@eisentraut.org
Backpatch: 16-, where meson support was added
Andres Freund [Tue, 4 Feb 2025 22:45:57 +0000 (17:45 -0500)]
meson: Narrow dependencies for 'install-quiet' target
Previously test dependencies, which are not actually installed, were
unnecessarily built.
Apply this to all branches with meson support, as part of an effort to fix
incorrect test dependencies that can lead to test failures.
Discussion: https://postgr.es/m/CAGECzQSvM3iSDmjF+=Kof5an6jN8UbkP_4cKKT9w6GZavmb5yQ@mail.gmail.com
Discussion: https://postgr.es/m/bdba588f-69a9-4f3e-9b95-62d07210a32e@eisentraut.org
Backpatch: 16-, where meson support was added
Nathan Bossart [Tue, 4 Feb 2025 19:26:57 +0000 (13:26 -0600)]
vacuumdb: Add missing PQfinish() calls to vacuum_one_database().
A few of the version checks in vacuum_one_database() do not call
PQfinish() before exiting. This precedent was unintentionally
established in commit 00d1e88d36, and while it's probably not too
problematic, it seems better to properly close the connection.
Reviewed-by: Daniel Gustafsson <daniel@yesql.se>
Discussion: https://postgr.es/m/Z6JAwqN1I8ljTuXp%40nathan
Backpatch-through: 13
Michael Paquier [Sun, 2 Feb 2025 02:31:29 +0000 (11:31 +0900)]
Mention jsonlog in description of logging_collector in GUC table
logging_collector was only mentioning stderr and csvlog, and forgot
about jsonlog. Oversight in dc686681e079, that has added support for
jsonlog in log_destination.
While on it, the description in the GUC table is tweaked to be more
consistent with the documentation and postgresql.conf.sample.
Author: Umar Hayat Reviewed-by: Ashutosh Bapat, Tom Lane
Discussion: https://postgr.es/m/CAD68Dp1K_vBYqBEukHw=1jF7e76t8aszGZTFL2ugi=H7r=a7MA@mail.gmail.com
Backpatch-through: 13
Tom Lane [Thu, 30 Jan 2025 20:36:07 +0000 (15:36 -0500)]
Avoid integer overflow while testing wal_skip_threshold condition.
smgrDoPendingSyncs had two distinct risks of integer overflow while
deciding which way to ensure durability of a newly-created relation.
First, it accumulated the total size of all forks in a variable of
type BlockNumber (uint32). While we restrict an individual fork's
size to fit in that, I don't believe there's such a restriction on
all of them added together. Second, it proceeded to multiply the
sum by BLCKSZ, which most certainly could overflow a uint32.
(The exact expression is total_blocks * BLCKSZ / 1024. The
compiler might choose to optimize that to total_blocks * 8,
which is not at quite as much risk of overflow as a literal
reading would be, but it's still wrong.)
If an overflow did occur it could lead to a poor choice to
shove a very large relation into WAL instead of fsync'ing it.
This wouldn't be fatal, but it could be inefficient.
Change total_blocks to uint64 which should be plenty, and
rearrange the comparison calculation to be overflow-safe.
I noticed this while looking for ramifications of the proposed
change in MAX_KILOBYTES. It's not entirely clear to me why
wal_skip_threshold is limited to MAX_KILOBYTES in the
first place, but in any case this code is unsafe regardless
of the range of wal_skip_threshold.
Oversight in c6b92041d which introduced wal_skip_threshold,
so back-patch to v13.
Tom Lane [Wed, 29 Jan 2025 20:31:55 +0000 (15:31 -0500)]
Handle default NULL insertion a little better.
If a column is omitted in an INSERT, and there's no column default,
the code in preptlist.c generates a NULL Const to be inserted.
Furthermore, if the column is of a domain type, we wrap the Const
in CoerceToDomain, so as to throw a run-time error if the domain
has a NOT NULL constraint. That's fine as far as it goes, but
there are two problems:
1. We're being sloppy about the type/typmod that the Const is
labeled with. It really should have the domain's base type/typmod,
since it's the input to CoerceToDomain not the output. This can
result in coerce_to_domain inserting a useless length-coercion
function (useless because it's being applied to a null). The
coercion would typically get const-folded away later, but it'd
be better not to create it in the first place.
2. We're not applying expression preprocessing (specifically,
eval_const_expressions) to the resulting expression tree.
The planner's primary expression-preprocessing pass already happened,
so that means the length coercion step and CoerceToDomain node miss
preprocessing altogether.
This is at the least inefficient, since it means the length coercion
and CoerceToDomain will actually be executed for each inserted row,
though they could be const-folded away in most cases. Worse, it
seems possible that missing preprocessing for the length coercion
could result in an invalid plan (for example, due to failing to
perform default-function-argument insertion). I'm not aware of
any live bug of that sort with core datatypes, and it might be
unreachable for extension types as well because of restrictions of
CREATE CAST, but I'm not entirely convinced that it's unreachable.
Hence, it seems worth back-patching the fix (although I only went
back to v14, as the patch doesn't apply cleanly at all in v13).
There are several places in the rewriter that are building null
domain constants the same way as preptlist.c. While those are
before the planner and hence don't have any reachable bug, they're
still applying a length coercion that will be const-folded away
later, uselessly wasting cycles. Hence, make a utility routine
that all of these places can call to do it right.
Making this code more careful about the typmod assigned to the
generated NULL constant has visible but cosmetic effects on some
of the plans shown in contrib/postgres_fdw's regression tests.
Tom Lane [Wed, 29 Jan 2025 19:24:36 +0000 (14:24 -0500)]
Avoid breaking SJIS encoding while de-backslashing Windows paths.
When running on Windows, canonicalize_path() converts '\' to '/'
to prevent confusing the Windows command processor. It was
doing that in a non-encoding-aware fashion; but in SJIS there
are valid two-byte characters whose second byte matches '\'.
So encoding corruption ensues if such a character is used in
the path.
We can fairly easily fix this if we know which encoding is
in use, but a lot of our utilities don't have much of a clue
about that. After some discussion we decided we'd settle for
fixing this only in psql, and assuming that its value of
client_encoding matches what the user is typing.
It seems hopeless to get the server to deal with the problematic
characters in database path names, so we'll just declare that
case to be unsupported. That means nothing need be done in
the server, nor in utility programs whose only contact with
file path names is for database paths. But psql frequently
deals with client-side file paths, so it'd be good if it
didn't mess those up.
Bug: #18735 Reported-by: Koichi Suzuki <koichi.suzuki@enterprisedb.com>
Author: Tom Lane <tgl@sss.pgh.pa.us> Reviewed-by: Koichi Suzuki <koichi.suzuki@enterprisedb.com>
Discussion: https://postgr.es/m/18735-4acdb3998bb9f2b1@postgresql.org
Backpatch-through: 13
Noah Misch [Sat, 25 Jan 2025 19:28:14 +0000 (11:28 -0800)]
At update of non-LP_NORMAL TID, fail instead of corrupting page header.
The right mix of DDL and VACUUM could corrupt a catalog page header such
that PageIsVerified() durably fails, requiring a restore from backup.
This affects only catalogs that both have a syscache and have DDL code
that uses syscache tuples to construct updates. One of the test
permutations shows a variant not yet fixed.
This makes !TransactionIdIsValid(TM_FailureData.xmax) possible with
TM_Deleted. I think core and PGXN are indifferent to that.
Per bug #17821 from Alexander Lakhin. Back-patch to v13 (all supported
versions). The test case is v17+, since it uses INJECTION_POINT.
Noah Misch [Sat, 25 Jan 2025 19:28:14 +0000 (11:28 -0800)]
Test ECPG decadd(), decdiv(), decmul(), and decsub() for risnull() input.
Since commit 757fb0e5a9a61ac8d3a67e334faeea6dc0084b3f, these
Informix-compat functions return 0 without changing the output
parameter. Initialize the output parameter before the test call, making
that obvious. Before this, the expected test output has been depending
on freed stack memory. "gcc -ftrivial-auto-var-init=pattern" revealed
that. Back-patch to v13 (all supported versions).
Tomas Vondra [Fri, 24 Jan 2025 23:36:48 +0000 (00:36 +0100)]
Use the correct sizeof() in BufFileLoadBuffer
The sizeof() call should reference buffer.data, because that's the
buffer we're reading data into, not the whole PGAlignedBuffer union.
This was introduced by 44cac93464, which replaced the simple buffer
with a PGAlignedBuffer field.
It's benign, because the buffer is the largest field of the union, so
the sizes are the same. But it's easy to trip over this in a patch, so
fix and backpatch. Commit 44cac93464 went into 12, but that's EOL.
projects / thirdparty / postgresql.git / log
