When a stats entry is reinitialized and dropped again while a backend
still holds a reference from an older generation, the backend releasing
the last reference found a generation mismatch and left behind an entry
marked as dropped with a refcount of 0. Nothing would free it, and the
checkpointer would trigger an assertion at shutdown when flushing the
stats.
Looking more at the issue, using some hardcoded sleeps before the
dshash_find() that takes an exclusive lock when releasing an entry with
some concurrent activity, there was more going on:
- A missing entry could be reported as an error at backend exit, error
that should not be hit even if the entry was reinitialized and dropped
concurrently.
- The generation check was not correct for the case of 0 due to the
first issue. A generation of 0 refers to an entry that has been freshly
created.
This commit reworks the entry release to be safer concurrent-wise when
an entry targetted for release is found as already existing:
- If marked as dropped without refount at 0, free it.
- In other cases, ignore it. The entry has been reinitialized or
created again with the same key, and is still in use.
Sami has posted a basic test case (equivalent added in this commit) that
gives a way to reproduce the assertion failure on backend exit, which is
a consequence of the incorrect handling of entry releases.
Reported-by: Sami Imseih <samimseih.pg@gmail.com>
Reviewed-by: Sami Imseih <samimseih.pg@gmail.com>
Discussion: https://postgr.es/m/CAN12+YJXE+rEOVo74uM5Ofbj_t_m0DHvd_Mo4ppeHV=aDvvXoQ@mail.gmail.com
Backpatch-through: 15
Previously we called strtol() with base 0, so that a leading zero
digit caused the number to be read as octal. This is not documented
because it was not the original intent: before commit 7ccb6dc2d, this
code used atoi() which would have read the number as decimal always.
Use base 10 instead to restore the original behavior.
Also reject a bare minus sign consistently. Per POSIX, strtol()
"may" set errno for that case but doesn't have to, and if it doesn't
then we'd not have noticed the bad syntax but read the value as 0.
Although this change restores old behavior, 7ccb6dc2d was a long
time ago, so it's at least possible that someone out there is
expecting the octal behavior. So it doesn't seem like a great
idea to put this into stable branches. But I (tgl) judge it
not too late for v19.
Author: Marko Grujic <marko.grujic@enterprisedb.com>
Reviewed-by: Manu <manuelreyesbravo@gmail.com>
Reviewed-by: Tom Lane <tgl@sss.pgh.pa.us>
Discussion: https://postgr.es/m/CAOvwyF3=brZoxwcwHRQqvLjuqSqxiTN7RXpKMNHXJ1WAS5GQ2Q@mail.gmail.com
Backpatch-through: 19
To perform cross-type index scans with a timestamp index and a
timestamptz comparison value, btree_gin must convert the timestamptz
to timestamp to establish the starting point for the indexscan. This
is hazardous because timestamp_cmp_timestamptz is actually defined to
upconvert the timestamp to timestamptz and then compare timestamptz's.
When dealing with ambiguous timestamps near a fall-back DST transition,
the value may not round-trip, leading to selecting a scan start point
that misses some index entries that should match.
To fix that, move the converted value back an hour at a time until
it's clearly <= the given comparison value. (We could try to be
smarter, but this way seems more certain to work across all possible
DST behaviors.)
Another hazard is that for timestamptz's that are finite but close to
the upper range limit, the conversion may produce timestamp +Infinity,
again comparing greater than the given value. It's less clear whether
this can lead to missing matching values in practice, but it would at
least risk making the aforesaid correction loop very slow. So replace
+Infinity by the maximum finite timestamp.
The reverse case of timestamptz index and timestamp comparison value
is not at risk because upconverting the timestamp stays consistent
with what timestamptz_cmp_timestamp will do. Also, while
date-vs-timestamptz could theoretically be at risk, the conversion
error would have to be a full day which seems not to occur in reality.
Author: shihao zhong <zhong950419@gmail.com>
Reviewed-by: Tom Lane <tgl@sss.pgh.pa.us>
Discussion: https://postgr.es/m/CAGRkXqQio=7PWgTfKvjyQY9-7A_0A36E3put1gxJeDypgqFSzw@mail.gmail.com
Backpatch-through: 19
When constructing a historic snapshot for logical decoding,
reorderbuffer needs to keep track of movement of tuples in catalog
tables. For the most part this works correctly, except that when a
subtransaction that wrote a catalog tuple aborts, we failed to "forget"
about that change. There's no known reports of anything actually
breaking except assertion failures, but it is likely possible to
construct cases where historical snapshots produce bogus visibility
answers, potentially causing incorrect decoding output.
To fix, have the decode of a subtransaction abort go over the TUPLECID
list that has been accumulated on the ReorderBufferTXN for its topmost
parent transaction, and remove all the entries that were marked as
originating from the aborted subtransaction or its children. There are
some little details in which various devils lie, which the added code
comments explain (we hope).
Add a couple of isolation test cases that trip the assertions with the
original coding. They're not ideal test design, because they pass in
non-assert builds without the fix. But we'd rather have the fix be
verified in that limited scenario than nowhere at all, and they are
cheap enough.
This bug is sibling to the one fixed in commit 8c67d29fd5 and, like
that one, it goes all the way back to the introduction of
reorderbuffers, so backpatch to all live branches.
While reviewing this, I (Álvaro) was bothered by the snapmgr.c
tuplecid_data file-scoped variable, as it's easily confused with a
function parameter used widely in reorderbuffer.c. In branches 19 and
master, rename that to avoid confusion.
Reported-by: Arseny Sher <a.sher@postgrespro.ru>
Reported-by: Mark Dilger <mark.dilger@enterprisedb.com>
Reported-by: Alexander Kozhemyakin <a.kozhemyakin@postgrespro.ru>
Author: Bingshuai Li <lucian1412@outlook.com>
Author: Hayato Kuroda <kuroda.hayato@fujitsu.com>
Author: Zhijie Hou <houzhijie22@gmail.com>
Backpatch-through: 14
Bug: #19555
Discussion: https://postgr.es/m/87r2c8xocq.fsf@ars-thinkpad
Discussion: https://postgr.es/m/CAHgHdKu5e3XY5e90Tuaxq_R4WrKxSV734Q%2BLwo5y39Omp2A-Gg@mail.gmail.com
Discussion: https://postgr.es/m/19555-3c700f40a13045cd@postgresql.org
When a table is partitioned, the previous code did not emit NO_GATHER
advice for the parent table, on the theory that doing this for the
child tables was good enough. However, it isn't. I believe what
happened here is that, during development, I was planning to enforce
NO_GATHER by clearing the consider_parallel flag for the children,
which would have been good enough to ensure that parallelism could
not be used at any higher level of the plan tree. Eventually, I
switched course, but failed to realize that this code needed to be
updated as a result. Do that, and add a regression test.
Backpatch-through: 19
Discussion: http://postgr.es/m/CA+TgmoYYHC4i-Gpzc9-JYHqih6pOK+6LTYFiNR-PioXG1nKkOQ@mail.gmail.com
Commit 0d4391b265 taught the planner
to store information about the RTIs of SubqueryScan, Append, and
MergeAppend node elided from the plan using a new ElidedNode data
structure; an hour later, 7358abcc60
taught to save the RTIs of an Append node into another Append
node using a new child_append_relid_sets field. Regrettably,
despite the fact that both commits were written by me and the
fact that both were part of the same patch set, the latter commit
failed to deal with the case where an Append node first acquires
child_append_relid_sets != NIL and then gets elided. I believe that
I thought that was impossible, but the included test case shows that
it isn't.
The proper fix involves updating the definition of ElidedNode to
carry child_append_relid_sets, so this commit does that, breaking
ABI compatibility. That's fine for the master branch, but it's a
bit unfortunate to be doing do that so close to the planned ship
date for v19. However, shipping v19 without fixing this appears
(at least to me) to be even more unfortunate.
Backpatch-through: 19
Discussion: http://postgr.es/m/CA+TgmoYYHC4i-Gpzc9-JYHqih6pOK+6LTYFiNR-PioXG1nKkOQ@mail.gmail.com
Given an Append or MergeAppend node, pgpa_build_scan needs to call
pgpa_make_scan for the node's own apprelids, and also for each element
of child_append_relid_sets. It did that, but it overlooked the fact
that the scan type applicable to the child scans might differ from
the scan type applicable to the parent scan. The problem can only
occur in one direction: an RTE_SUBQUERY (Merge)Append can appear on
top of a RTE_RELATION (Merge)Append, but not the reverse, so the
only failure mode here is to classify a partitionwise scan of a child
appendrel as PGPA_SCAN_ORDINARY rather than PGPA_SCAN_PARTITIONWISE,
leading to failure to include PARTITIONWISE(something) in the
generated advice string.
Backpatch-through: 19
Discussion: http://postgr.es/m/CA+TgmoYYHC4i-Gpzc9-JYHqih6pOK+6LTYFiNR-PioXG1nKkOQ@mail.gmail.com
Commit 7358abcc60 taught the planner to
store information about Append node consolidation in the final plan,
but it forgot that child_append_relid_sets needs to be adjusted
at setrefs.c time just like every other place where we store RTIs
in a plan tree. Fix that.
Although this can cause incorrect plan advice to be emitted in cases
involving multi-level partitioning, it seems more appropriate to put
the test case for it in pg_overexplain, so this commit does that.
Backpatch-through: 19
Discussion: http://postgr.es/m/CA+TgmoYYHC4i-Gpzc9-JYHqih6pOK+6LTYFiNR-PioXG1nKkOQ@mail.gmail.com
bpchar equality ignores trailing spaces, so equal values in a column
with no length specifier can have different images. Similarly,
oidvector equality intentionally pays no attention to the array lower
bound field. We nevertheless allowed nbtree deduplication to be used
with indexes built with bpchar_ops, bpchar_pattern_ops, or
oidvector_ops. A posting list split could therefore place a TID in an
index tuple whose key was equal to, but not bytewise identical to, the
corresponding table value. This allowed index-only scans to return
subtly wrong results.
To fix, drop support function 4 from the affected opclasses (we can't
change catalog contents on branches 14 through 18, so we just disable
deduplication by adding hard-coding to the relevant support functions).
Deliberately leave the oidvector opclass alone on branches 14 through
18, though. Disallowing oidvector deduplication on these branches would
make pg_amcheck report that pg_proc_proname_args_nsp_index has a corrupt
metapage, needlessly alarming users.
System catalog oidvector values always have a lower bound of 0, so the
issue can't cause harm there; a user can only hit it by indexing an
oidvector column that stores the output of a function like trim_array().
It seems very unlikely that any user would ever do that.
Users should REINDEX any B-Tree index on a bpchar column.
Author: Shihao Zhong <zhong950419@gmail.com>
Co-authored-by: Peter Geoghegan <pg@bowt.ie>
Reported-by: Ke <kehan5800@gmail.com>
Reported-by: Tom Lane <tgl@sss.pgh.pa.us>
Reviewed-by: Tom Lane <tgl@sss.pgh.pa.us>
Bug: #19749
Discussion: https://postgr.es/m/19749-d46adf7c7d910b07@postgresql.org
Discussion: https://postgr.es/m/CAGRkXqQQ311Lg=62x5UdZtPuRFgpT6XpPMw4X9E3tgbTu162Mw@mail.gmail.com
Backpatch-through: 14
build_grouped_rel() makes a flat copy of the input relation, including
its FDW fields. As a result, when building a grouped join relation
for eager aggregation, add_paths_to_joinrel() called the FDW's
GetForeignJoinPaths for it. postgres_fdw then failed with "Aggref
found where not expected" on the partial Aggrefs in the target list,
and in any case it would have treated a partially aggregated input as
a plain foreign scan.
FDWs are not prepared to handle grouped relations, so clear serverid,
fdwroutine and fdw_private in build_grouped_rel().
Reported-by: Robert Haas <robertmhaas@gmail.com>
Author: Richard Guo <guofenglinux@gmail.com>
Discussion: https://postgr.es/m/CA+Tgmob7iSM9YkRM44VjUDuaCchW-fY54MV5njpTZTL9uNyV4w@mail.gmail.com
Backpatch-through: 19
When we converted these types' input functions to report errors
through the soft-error mechanism, we missed that stack-overflow
errors can be triggered through bad user input. Convert those
to be reported softly, too.
Bug: #19745
Reported-by: Ke <kehan5800@gmail.com>
Author: Tom Lane <tgl@sss.pgh.pa.us>
Discussion: https://postgr.es/m/19745-39cd7b93a505f9a9@postgresql.org
Backpatch-through: 16
This commit addresses the following issues in commit de28140de:
* When the local transaction was DEFERRABLE, begin_remote_xact() started
a remote transaction in the same mode without checking the remote
server version. Since DEFERRABLE is supported only in v9.1 and later,
this would cause a syntax error on older remote servers. Fix this by
propagating the mode only to remote servers that support it.
* pgfdw_xact_callback() committed remote transactions without
synchronizing the read-only mode. If the local transaction had become
READ ONLY since the last remote operation, this would cause deferred
triggers on remote servers to still run in READ WRITE mode. Fix this
by synchronizing the mode at commit time.
* The documentation did not match the actual implementation. Update the
documentation to reflect the implementation.
Reported-by: Fujii Masao <masao.fujii@gmail.com>
Author: Etsuro Fujita <etsuro.fujita@gmail.com>
Reviewed-by: Matheus Alcantara <matheusssilv97@gmail.com>
Discussion: https://postgr.es/m/CAHGQGwE4UMaWGOZq1Z7YUTUSg-CgL4yAzO5YaGwTZe1-6Pjg2g%40mail.gmail.com
Backpatch-through: 19
On Windows, a backend exiting resets its connection, and the client
discards any data not yet read, including the FATAL message of a
terminated backend. isolationtester polls one connection at a time, so
when it waits on a different session the message can get lost, failing
the permutation and the tests that run after it.
wait_cleanup in injection_points (down to v17) and slot_creation_error
in test_decoding (down to v15) have this pattern, and have been failing
randomly in the buildfarm. Let's skip them on Windows.
The cause of the instabilities is proving to be tricky to deal with,
pointing for example at 29992a6a50. This will be tackled later,
perhaps with a backpatch (or perhaps with a HEAD-only solution), but it
is difficult to be sure at this stage of the work. In theory,
a8458f508a should make a revert of 29992a6a50 (revert of a revert!)
safe on WIN32.
Note that 278cdea6b9, that has introduced the isolation test
slot_creation_error in test_decoding, has mentioned the potential output
instability.
Author: Zsolt Parragi <zsolt.parragi@percona.com>
Author: Michael Paquier <michael@paquier.xyz>
Discussion: https://postgr.es/m/CAN4CZFOoiKa9E4Gnh_OzLk3S5Z6RFyg_oY1e_WddCVYhkpTVow@mail.gmail.com
Backpatch-through: 15
When ANALYZE imports statistics from a remote server, it fetched/stored
the statistics while still running as the user executing ANALYZE, not as
the foreign table's owner. This is inconsistent with the sampling path,
and it causes a security issue: a role that owns a foreign table but has
no access to the underlying remote data (eg, no access privileges on the
remote server) could obtain that data by having a privileged user run
ANALYZE on the table; the imported statistics like most_common_vals then
expose sampled values from the remote data, as shown in the reproducer
on the discussion thread.
To fix, switch to the foreign table owner's userid in analyze_rel()
before calling the ImportForeignStatistics() routine, mirroring the
identity used by the sampling path. This not only makes the privilege
handling consistent between the sampling/import paths, but also prevents
statistics import from disclosing data the table owner couldn't
otherwise obtain.
Oversight in commit 28972b6fc.
Reported-by: Fujii Masao <masao.fujii@gmail.com>
Reported-by: Osama Abdul Qader <osamaabdulqader.cs@gmail.com>
Reported-by: Noah Misch <noah@leadboat.com>
Author: Noah Misch <noah@leadboat.com>
Reviewed-by: Matheus Alcantara <matheusssilv97@gmail.com>
Reviewed-by: Etsuro Fujita <etsuro.fujita@gmail.com>
Discussion: https://postgr.es/m/CAPmGK16jVk+i2KMkkgR9ajoPdaUAinvcspkk5Bc6urbo2xYTMQ@mail.gmail.com
Backpatch-through: 19
The header comment for this file says it assumes that "the CPU is not
picky about alignment", which we wouldn't really accept for Postgres.
The two functions that have alignment requirements have been hacked in
ways that are inconsistent and yet both ugly. des_setkey just casts
its "const char *" argument to "const uint32 *", relying on the caller
to ensure that that pointer is actually word-aligned, which the caller
does by declaring the buffer as uint32[] and then casting to "char *".
Meanwhile des_cipher carefully memcpy's to and from an internal
uint32[] buffer, which is pretty silly given that what it's passed
must be word-aligned for the benefit of des_setkey. And on top of
that we have a bunch of ugly casting and oddly-written pointer
arithmetic in the caller px_crypt_des. The odd pointer arithmetic
causes warnings about possible buffer overrun when using recent gcc
at -O3 or higher.
Let's clean this up by converting the buffer variable into a union
of a uint8 array and a uint32 array, so that we can remove all these
casts, and also replace the strange loop logic with a simple subscript
variable to satisfy gcc.
Reported-by: Andres Freund <andres@anarazel.de>
Author: Tom Lane <tgl@sss.pgh.pa.us>
Co-authored-by: Ayush Tiwari <ayushtiwari.slg01@gmail.com>
Discussion: https://postgr.es/m/2005885.1790359097@sss.pgh.pa.us
Discussion: https://postgr.es/m/3nuudxv365kjnmwjhnygdakhxuktpdjvf26rt26eb44esgqdrj@y2x3vomkrfoo
Backpatch-through: 14
The heuristic to avoid setting the VM during on-access pruning when
doing so would emit an extra FPI missed a few cases. First it missed
temp and unlogged tables. Those will never emit an FPI, so they can
always set the VM if the page is all-visible. It also missed that if
hint bits are not WAL-logged, setting only the VM passes REGBUF_NO_IMAGE
for an already WAL-logged heap page, forbidding a heap FPI in the WAL
record. A page that has never been WAL-logged still requires a heap FPI.
The third is more subtle: If the page is all-visible, the new prune xid
will be InvalidTransactionId. On-access pruning only executes when the
current pd_prune_xid is valid and visible. So, when on-access pruning
finds the page all-visible, it will always clear pd_prune_xid, modifying
the page.
Knowing this means we can set the VM without emitting an extra heap FPI
in more cases. When hint bits are WAL-logged, if the heap buffer is
clean, modifying pd_prune_xid will emit an FPI if one is required. There
is no reason to try to avoid an FPI by not setting the VM. So, set the
VM in this case. However, when the heap buffer is already dirty,
modifying pd_prune_xid can avoid an FPI; so if the page hasn't been
logged since the last checkpoint, setting it all-visible will emit an
extra heap page FPI. We will still avoid setting the VM in this case.
This is being backpatched to 19 because since 378a216187 pd_prune_xid
is set on insert, and if we execute a prune cycle and skip setting the
VM because of an incorrect heuristic, we have added new wasted work in
PG 19.
This commit adds some tests covering these cases. It also updates one of
the existing temp table tests to avoid exceeding the pin limit. Setting
the VM and FSM on-access when querying temp tables takes more local pins
and can run into the limit with fewer heap buffers pinned.
Reported-by: Melanie Plageman <melanieplageman@gmail.com>
Author: Melanie Plageman <melanieplageman@gmail.com>
Reviewed-by: Andrey Borodin <x4mmm@yandex-team.ru>
Discussion: https://postgr.es/m/CAAKRu_amj7qLF4c=9ijd=708Fu2G8gg-2EqwBu=aCdAHU2sPHg@mail.gmail.com
Backpatch-through: 19
The query to get attribute statistics from a remote server applied the
"C" collation to a column of type name without checking the server's
version. Since the name type was made collatable in Postgres 12, this
could break attribute statistics import if the server was older. Fix by
casting the column value to text before collating it if the server is
older.
Also, fix a typo in the if-test in the same function.
Oversights in commit 28972b6fc; the former was reported by Fujii Masao.
Reported-by: Fujii Masao <masao.fujii@gmail.com>
Author: Etsuro Fujita <etsuro.fujita@gmail.com>
Reviewed-by: Corey Huinker <corey.huinker@gmail.com>
Discussion: https://postgr.es/m/CAHGQGwGvA_3q7Wg7GXQkuugGZj3%3DkxDRjDsKgVvK1uwAxHTopQ%40mail.gmail.com
Backpatch-through: 19
This commit fixes several related problems. All of these problems
escaped testing for the same reason: they do not occur when only
generated advice is supplied, and therefore test_plan_advice was
incapable of detecting these shortcomings.
First, prior to this commit, the advice feedback code never regarded
an unordered sublist of a JOIN_ORDER specification as matching an
unrolled join. As a result, when JOIN_ORDER() advice contained
unordered sublists, the advice feedback tended to be "matched, failed"
even when the advice worked exactly as intended. Only the case where
an unordered sublist was implemented by a plan shape not subject to
unrolling, such as a partitionwise join, worked properly.
Second, prior to this commit, a join order sublist in the initial
position wasn't properly handled. Note that this overlaps with the
problem described in the previous paragraph; JOIN_ORDER({a b} c) was
broken both because of the unordered sublist and because of the
sublist being in the initial position. However, there's more to this
case: JOIN_ORDER((a b) c) means the same as JOIN_ORDER(a b c), but the
advice feedback code didn't know that, and would generate "matched,
failed" for the former case even when everything was working.
Finally, prior to this commit, advice feedback didn't correctly handle
single-element sublists. Those are pretty nonsensical, since by
definition a join involves at least 2 tables, so we could just ban
that case. Instead, at least for now, I've chosen to make advice
feedback handle such cases in the same way that advice enforcement
already does: the extra grouping levels are simply disregarded, so
that JOIN_ORDER({a} ((b))) is enforced in the same way as, and also
gets the same advice feedback as, JOIN_ORDER(a b).
Reported-by: Noah Misch <noah@leadboat.com>
Backpatch-through: 19
Discussion: https://postgr.es/m/CA+TgmoYmXy-jiP5qDhqNEiYFEBzQsArO6O2d9E8szNZqi1bePQ@mail.gmail.com
Reviewed-by: Ayush Tiwari <ayushtiwari.slg01@gmail.com>
Up until now, pg_plan_advice has had a feature that allows an advice
target to mention a partition name but omit the partition schema;
that is, something like SEQ_SCAN(foo/bar) forces a sequential scan on
every child of table foo whose partition name is bar, regardless of
the schema in which bar appears. However, that feature turns out to
have a nasty design flaw: while advice enforcement handles this case
just fine, the advice feedback code doesn't know about it and will
mark such advice as "matched, failed" even when everything worked
perfectly. Unfortunately, there seems to be no simple code fix for
this problem.
Since the release of PostgreSQL 19 is imminent, take the conservative
course and revert this feature of pg_plan_advice. In other words,
require the partition schema whenever the partition name is present.
You must now write e.g. SEQ_SCAN(foo/public.bar) rather than just
SEQ_SCAN(foo/bar). This doesn't affect any cases where automatically
generated advice is supplied, since generated advice has always
included the partition schema anyway. Manually written advice will
have to conform to the new, stricter rule. For a later release, we
can consider whether to again relax this restriction in some way,
but now is not the time to design new things.
Reported-by: Noah Misch <noah@leadboat.com>
Backpatch-through: 19
Discussion: https://postgr.es/m/CA+TgmoYmXy-jiP5qDhqNEiYFEBzQsArO6O2d9E8szNZqi1bePQ@mail.gmail.com
List of commits reverted:
8e72d914c5 Add UPDATE/DELETE FOR PORTION OF
b6ccd30d8f Add isolation tests for UPDATE/DELETE FOR PORTION OF
33db6c4baf Fix DELETE/UPDATE FOR PORTION OF with rules
7b22f15a01 Add psql tab completion for FOR PORTION OF clause
7ca8c94296 Fix FOR PORTION OF with non-updatable view columns
993a7aa0e4 Fix cross-leftover pollution in FOR PORTION OF insert triggers
7ac030d5b1 Require UPDATE permission on FOR PORTION OF column
f0aedc7cb0 Small terminology fixes in comments
7d13b03a2e Fix FOR PORTION OF for inheritance children
a272a58b94 Move FOR PORTION OF volatile check into planner
a40fdf6588 Reject child partition FDWs in FOR PORTION OF
bc3ae886a7 Forbid FOR PORTION OF with WHERE CURRENT OF
c8d49ffb00 Forbid generated columns in FOR PORTION OF
5b5e99047a Forbid FOR PORTION OF on views with INSTEAD OF triggers
c58c83ce50 Fix RLS checks for FOR PORTION OF leftover rows
5454fe4a6e Test what BEFORE UPDATE triggers do to FOR PORTION OF
2a9541ddfd Deparse FOR PORTION OF using the range column's current name.
fd92f74b91 Avoid RETURNING side effects for FOR PORTION OF leftovers.
f3a116a26f Enforce WITH CHECK OPTION on DELETE FOR PORTION OF leftovers
c7e34c31e8 Fix error code for null FOR PORTION OF target
938f4f8f34 Fix assertion failures in DELETE FOR PORTION OF tuple routing
f194032168 Resolve untyped parameters in FOR PORTION OF FROM/TO bounds
98f6984e5c Don't evaluate the FOR PORTION OF target under EXPLAIN
plus release notes entries.
Discussion: https://www.postgresql.org/message-id/62bf70cc-dc33-4188-8070-a8c177bb61de%40eisentraut.org
To import attribute statistics, postgres_fdw sent a query wity COLLATE
"C" to remote servers without checking their version. Since COLLATE is
only supported since v9.1, this could break the import against older
servers; fix by modifying postgres_fdw to check their version and if
older, fall back to sampling. (This commit still allows importing only
relation statistics from such an older server.)
Also, expand the documentation to mention that attribute statistics are
stored into the local server without applying the foreign table columns'
n_distinct option.
Oversight in commit 28972b6fc.
Reported-by: Fujii Masao <masao.fujii@gmail.com>
Reported-by: Osama Abdul Qader <osamaabdulqader.cs@gmail.com>
Author: Etsuro Fujita <etsuro.fujita@gmail.com>
Reviewed-by: Corey Huinker <corey.huinker@gmail.com>
Discussion: https://postgr.es/m/CAHGQGwH8%2BuDTwfohpck9v_9L3nwSWACbmC36JYRyqHd5xQN_OA%40mail.gmail.com
Backpatch-through: 19
The replica identity determination was "flawed": it fell back to the
primary key if the marked index was dropped, which is a defensible
choice, but logical decoding has a differing opinion and fails to
provide usable tuple identity data in this case. "Fix" REPACK by
refusing to use the primary key in that case.
This is arguably broken behavior in RelationGetIndexList, but I'll
refrain from changing that at this stage.
Reported-by: Nathan Bossart <nathandbossart@gmail.com>
Author: Matthias van de Meent <boekewurm+postgres@gmail.com>
Author: Ewan Young <kdbase.hack@gmail.com>
Backpatch-through: 19
Discussion: https://postgr.es/m/apCBRKCH8jwKiaSY@nathan
The general nature of this interaction has been understood (at least
by me) since before the original commit, but I failed to explicitly
document it. I also left behind an XXX in the README which I intended
to remove before commit. Fix those things.
Reported-by: Noah Misch <noah@leadboat.com>
Backpatch-through: 19
Discussion: https://postgr.es/m/CA+TgmoYmXy-jiP5qDhqNEiYFEBzQsArO6O2d9E8szNZqi1bePQ@mail.gmail.com
Without CHECK_FOR_INTERRUPTS(), backends might not respond to query
cancellations in a timely fashion, especially for JOIN_ORDER()
specifications, where matching against each join can take O(n^2) time
in the length of the join order list in degenerate cases.
Recursive functions should check_stack_depth().
Reported-by: Noah Misch <noah@leadboat.com>
Discussion: https://postgr.es/m/CA+TgmoYmXy-jiP5qDhqNEiYFEBzQsArO6O2d9E8szNZqi1bePQ@mail.gmail.com
Backpatch-through: 19
Commit 86f7c82cf1 reported the user catalog table restriction as a
detail, but the other checks in check_concurrent_repack_requirements()
were still a mix: some were hints, and the details repeated
"REPACK (CONCURRENTLY)", which the primary message already names.
Report them as details with the same "This operation ..." wording. The
one actionable hint, ALTER TABLE ... REPLICA IDENTITY USING INDEX,
stays.
Backpatch to v19, where REPACK (CONCURRENTLY) was introduced.
Author: Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com>
Suggested-by: Alvaro Herrera <alvherre@kurilemu.de>
Discussion: https://postgr.es/m/apBIFWzHYOaG0auN%40nathan
Backpatch-through: 19
Previously, running REPACK (CONCURRENTLY) on a materialized view failed
with a confusing "has no identity index" error.
REPACK (CONCURRENTLY) replays changes decoded from WAL, but a
materialized view produces none, since it cannot be modified by DML and
REFRESH rewrites it through a transient heap that is not logically
decoded.
Fix this by reporting a clear error for materialized views upfront,
alongside the existing checks for system catalogs and TOAST tables, and
document the limitation.
Author: Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com>
Reviewed-by: Kiran Kaki <itskkpg@gmail.com>
Backpatch-through: 19
Discussion: https://postgr.es/m/CALj2ACXPvy4_LkQ0nHKgbZjAGK9fkDqRYUvXA3hgwxPm_X7Eng@mail.gmail.com
REPACK (CONCURRENTLY) is not an MVCC-safe operation; it doesn't
preserve the visibility information, which logical decoding needs
because it reads user catalog tables under a historic snapshot.
Disallow REPACK (CONCURRENTLY) on user catalog tables. Removing this
check requires making it MVCC-safe and logical rewrite mappings.
Backpatch to v19, where REPACK (CONCURRENTLY) was introduced.
Reported-by: Nathan Bossart <nathandbossart@gmail.com>
Author: Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com>
Reviewed-by: Antonin Houska <ah@cybertec.at>
Reviewed-by: Masahiko Sawada <sawada.mshk@gmail.com>
Reviewed-by: Alvaro Herrera <alvherre@kurilemu.de>
Discussion: https://postgr.es/m/apBIFWzHYOaG0auN%40nathan
Backpatch-through: 19
There's not really any good reason to store an empty advice string
in an advice stash, but it's entirely possible that some people might
do it by accident. If they do that without this fix, restarting the
system will result in failure while reloading pg_stash_advice.tsv.
The root cause is that I (rhaas) did not think clearly enough when
defining pgsa_next_tsv_field(). After returning each field on the
line, it set *cursor to the start of the next field, and after
returning all of them, it set *cursor to the trailing NUL byte. But
when the last field on the line is empty, this is ambiguous: if we're
pointing at the trailing NUL byte, it could be either because it's the
start of the zero-length field or because we're out of fields. To
remove the ambiguity, this commit decides that *cursor will instead
be set to NULL when there are no more fields.
Reported-by: Noah Misch <noah@leadboat.com>
Discussion: https://postgr.es/m/20260827171830.68.noahmisch@microsoft.com
Backpatch-through: 19
List of commits reverted:
2f094e7ac6 SQL Property Graph Queries (SQL/PGQ)
cd8844e7db Make some tests more stable by adding more explicit ordering
182cdf5aea pg_dump: Add appropriate version check
c9babbc881 Dump labels in reproducible order
040a56be4b Cleanup users and roles in graph_table_rls test
5282bf535e Fix some typos and make small stylistic improvements
720f0f89d6 Reject consecutive element patterns of same kind
288ae96872 Add a graph pattern variable only once
c5b3253b8a Property references are preferred over regular column references
a0dd0702e4 Fix cross variable references in graph pattern causing segfault
2ff289d039 Check for stack overflow when rewriting graph queries
9082680c34 Fix typos and grammar in graph table rewrite code
ac3bcc041c Fix collation of expressions in GRAPH_TABLE COLUMNS clause
9d2979dd68 pg_get_viewdef() and lateral references in COLUMNS of GRAPH_TABLE
891a57c739 Do not define type for a property graph
dc9e7c9ed9 Handle nodes that may appear in GraphPattern expression trees
f6edd8ed70 Add ORDER BY to test query to stabilize test
1190f858ea doc: Small synopsis wording change for consistency
d0eac3cafb Make spelling consistent
6827de95ee Simplify code in objectaddress.c for some property graph objects
5778fb3eaf Fix typo in error message
72498a8698 Handle element label and label property objects in object address functions
4cb2a9863d Fix LATERAL references in GRAPH_TABLE with multi-label pattern
9d8cdcbe0c Record dependencies on graph labels and properties
2a7e95b659 Readable identity strings for property graph objects
ac0ad6a7c9 Prevent dropping the last label from a property graph element
36aae3d029 Fix handling of dropping a property not associated with the given label
cc9aa7f3a9 Resolve unknown-type literals in GRAPH_TABLE COLUMNS
d868736830 Make property graph object descriptions better translatable
45d4c917ff Fix properties orphaned by dropping a label
33bfad0f3c Remove apparent support for SECURITY LABEL ON PROPERTY GRAPH
ccca2cd81b Fix pg_dump ACL minimization for PROPERTY GRAPH.
7883ea13ff Fix replace_property_refs() ignoring the root of expression tree
b820c623dd Resolve unknown-type literals in property expressions
01c544e1af Prohibit locking clauses on GRAPH_TABLE
8d2beee027 Restore the ability to use | and -> as prefix operators.
d2ac26eb9b Fix missing space before WHERE in GRAPH_TABLE deparse
4054ec5b4f Prohibit GRANT ... ON TABLE on a property graph
77964322d6 doc: Add PROPERTY GRAPH to the access privilege tables
c5e11de2f3 More tab-completion for DROP PROPERTY GRAPH
8ce749f8f6 Disallow aggregates, window functions, and SRFs in GRAPH_TABLE COLUMNS
22d0eebfef Report duplicate property and label names with a proper error
41a300ae3d Make generate_queries_for_path_pattern_recurse() interruptible
58af213874 Fix inferred property graph keys with INCLUDE columns
63f8e9773a Fix pg_event_trigger_ddl_commands for GRANT ON PROPERTY GRAPH
07df2b2532 doc: reformat GRAPH_TABLE examples
1c8c790ec1 Coerce GRAPH_TABLE pattern WHERE clauses to boolean
plus release notes entries and typedefs.list changes.
Discussion: https://www.postgresql.org/message-id/CAAKRu_bEtjWYWhYxSo0o_t3DaZYRQd5PkC0abA0g9Mp4%2BovH0w%40mail.gmail.com
When analyzing a foreign table pointing to a partitioned or inherited
table on the remote server, this method just imported relpages and
reltuples for the parent table from the remote pg_class, causing a
discrepancy or an error in results compared to the sampling method.
* In the partitioning case, while the sampling method sets relpages=0,
this method set relpages=-1. (For reltuples, the results of both
methods match.)
* In the inheritance case, while the sampling method sets
reltuples=(# of rows in the parent and child tables), this method set
reltuples=(# of rows in the parent table). (For relpages, the results
of both methods match.)
As relpages and reltuples are used for costing foreign paths, these
would lead to incorrect plans or plan changes depending on the method.
To fix, do the following:
* For the partitioning case, modify postgres_fdw to set relpages=0 to
match the sampling method.
* For the inheritance case, we could also modify it to set the correct
reltuples value by fetching reltuples for the child tables as well,
but 1) the change isn't small, and 2) table inheritance hasn't been
used that much these days, so fix by disabling this method.
While at it, adjust comments and docs slightly.
Oversight in commit 28972b6fc.
Author: Etsuro Fujita <etsuro.fujita@gmail.com>
Discussion: https://postgr.es/m/CAPmGK173b2Xvyoeq%3DY7L09H%2BajgMgxC2fYe%3D7CCunZz8tGTfFA%40mail.gmail.com
Backpatch-through: 19
FOREIGN_JOIN target sublists must contain at least two relation
identifiers. However, the parser checked only for sublists with
exactly one identifier, so FOREIGN_JOIN(()) was accepted.
Reject sublists with fewer than two relation identifiers, and add
regression coverage.
Author: Chao Li <lic@highgo.com>
Discussion: http://postgr.es/m/BEDC04E0-6732-4310-95BA-6EC34BC1442C@gmail.com
Explicitly make space for the NUL when casefolding.
No known bug in the previous code, because the previous buffer (size
12) was more than large enough for folding any codepoint with enough
room left for a NUL. The builtin provider's limit is 7; ICU's limit
seems to be 7 also; and libc always does 1:1 mappings so the real
limit is MAX_MULTIBYTE_CHAR_LEN + 1 (size 5).
Reviewed-by: Heikki Linnakangas <hlinnaka@iki.fi>
Reviewed-by: Rithvika Devisetti <devisettirithvika@gmail.com>
Discussion: https://postgr.es/m/2cfbc37ae8deccd3825e36b7f31c391cbf8ff9b8.camel@j-davis.com
Backpatch-through: 18
Tied hashes, and probably tied arrays, are capable of returning Perl
value pointers that are actually NULL, not the usual pointer to an
undef SV. We were not defending against that everywhere, leading
to possible SIGSEGV. Fix the code to consistently treat a null
pointer returned from hv_iternext or av_fetch like a !SvOK one.
(Note that the large diff in SV_to_JsonbValue is actually quite
trivial, but it required reindenting a chunk of existing code.)
Claude Code found the instance in hstore_plperl, and I found the
others by code auditing. Perhaps the other instances aren't
actually reachable, but I see little reason to assume that.
The known test cases for these errors require perl's Tie modules,
which may not be present, so it doesn't seem worth the trouble
to create regression test cases that would cover them.
Reported-by: Claude Code (via Noah Misch)
Author: Tom Lane <tgl@sss.pgh.pa.us>
Discussion: https://postgr.es/m/569769.1786901901@sss.pgh.pa.us
Backpatch-through: 14
Commit 072ee847ad taught logical decoding to discard the changes of a
transaction that is already known to be aborted when it is picked for
eviction. That path reuses ReorderBufferTruncateTXN(), which marks
every subtransaction that still has in-memory changes as
streamed. Since nothing is streamed in that path, and the top-level
transaction is never marked, a subtransaction ends up flagged as
streamed even though the output plugin has never seen it. Decoding the
subsequent abort record then makes ReorderBufferAbort() invoke the
stream_abort callback for that subtransaction.
For pgoutput this sends a Stream Abort ('A') message to a subscriber
that requested streaming = off, and it does so regardless of the
negotiated protocol version, so even a client speaking a version that
predates transaction streaming receives a message it cannot
parse. test_decoding dereferences a NULL pointer and crashes, since it
allocates its per-transaction state in the begin or stream start
callback, neither of which runs for a transaction discarded as
aborted.
This commit fixes this by marking a subtransaction as streamed only
when it has changes and its top-level transaction is already marked as
streamed. All streaming call sites mark the top-level transaction
before truncating it, so their behavior is unchanged, while the
abort-discard path never marks the top-level transaction and therefore
now leaves its subtransactions unmarked.
Backpatch to v18, where commit 072ee847ad was introduced.
Bug: #19616
Reported-by: Tyler Smart <tyler@smarts.io>
Author: Andrey Rachitskiy <pl0h0yp1@gmail.com>
Reviewed-by: Hayato Kuroda <kuroda.hayato@fujitsu.com>
Reviewed-by: Fujii Masao <masao.fujii@gmail.com>
Reviewed-by: Masahiko Sawada <sawada.mshk@gmail.com>
Discussion: https://postgr.es/m/19616-f6153af509910853@postgresql.org
Backpatch-through: 18
plperl_func_handler()'s stanza for handling an arrayref result in
a SETOF function could loop forever (or at least till OOM) when
given a tied array, since av_fetch won't necessarily ever return
a null pointer in that case. Be consistent with the other places
where we traverse a perl array: call av_len() once and use len+1
as the loop limit, silently ignoring any null pointers we get back
from that range of subscripts.
But actually, Perl's preferred locution for this seems to be to
use av_count() not av_len()+1. av_count() seems better since
there's less risk of forgetting to add 1. Also, both of those
functions return Size_t (or SSize_t) not int, creating at least
a theoretical overflow hazard. While we're modernizing this,
let's use the correct variable type where we can, and include an
overflow check where we can't.
Reported-by: Claude Code (via Noah Misch)
Author: Tom Lane <tgl@sss.pgh.pa.us>
Reviewed-by: Andrey Rachitskiy <pl0h0yp1@gmail.com>
Discussion: https://postgr.es/m/569769.1786901901@sss.pgh.pa.us
Backpatch-through: 14
Simple EXISTS subplans can be converted to hashed ANY subplans as
an alternative implementation. Previously, both the original EXISTS
subplan and the converted ANY subplan used names with the
exists_ prefix, making EXPLAIN output harder to read and
pg_plan_advice targets less clear.
Use the exists_to_any_ prefix for converted ANY subplans so that
their names distinguish them from the original EXISTS alternative.
This only changes the names shown by EXPLAIN and used by plan advice;
it does not affect planner behavior.
Author: Yugo Nagata <nagata@sraoss.co.jp>
Reviewed-by: Tom Lane <tgl@sss.pgh.pa.us>
Reviewed-by: solai v <solai.cdac@gmail.com>
Reviewed-by: Fujii Masao <masao.fujii@gmail.com>
Discussion: https://postgr.es/m/20260605165641.3950f99ace0aad8f807abe96@sraoss.co.jp
Backpatch-through: 19
Commit 13b935cd changed hash_get_num_entries to return int64 instead
of long. This fixes a few more users of hash_get_num_entries which
were missed in the original commit.
PGSS_FILE_HEADER is left unchanged since this only affects as of yet
unreleased versions of PostgreSQL. Backpatch to v19 where the int64
change was performed.
Author: Karina Litskevich <litskevichkarina@gmail.com>
Reviewed-by: Daniel Gustafsson <daniel@yesql.se>
Discussion: https://postgr.es/m/CACiT8iYTkc33YWaA2D3t51Y5s=GqBO7T1zX7bkpSmet2njcLLw@mail.gmail.com
Backpatch-through: 19
The option added by commit 28972b6fc was named "restore_stats", because
it was calling pg_restore_relation_stats()/pg_restore_attribute_stats()
in place of fetching a remote rowsample. However, those code paths have
been replaced with calls to
import_relation_statistics()/import_attribute_statistics() (cf. commit
54cd6fc83), so the option is now slightly misnamed.
Switch the option to "import_stats", which more closely reflects both
the functions being called internally, and the user's perception of what
the operation is doing.
Back-patch to v19 where commit 28972b6fc went in.
Suggested-by: Etsuro Fujita <etsuro.fujita@gmail.com>
Author: Corey Huinker <corey.huinker@gmail.com>
Discussion: https://postgr.es/m/CADkLM%3DezuspTk3VD-2EM6XtFP-Dfz7cJKtnbn0cB6gocsAgu1g%40mail.gmail.com
Backpatch-through: 19
The previous commit raises an ERROR during PGP operations if OpenSSL
does not support the cipher in use. However, any existing messages
created with faulty encryption will no longer be accessible via
pgp_[sym|pub]_decrypt().
To help users out of this situation, add a new ignore-cipher-failure
option which reverts to the broken behavior during decryption only. A
faulty encryption wrapper, created by an OpenSSL configuration that does
not support the cipher, can then be stripped back off by that same
OpenSSL in order to safely reencrypt it. (Note that when OpenSSL does
support the cipher, corrupted messages will not be decrypted regardless
of the ignore-cipher-failure setting; this is unchanged.)
The new tests add a corrupted Blowfish message for both public- and
symmetric-key decryption, resulting in the following test matrix:
- Blowfish supported, default behavior: fails to decrypt
- Blowfish supported, ignore-cipher-failure: fails to decrypt
- Blowfish unsupported, default behavior: fails to load cipher
- Blowfish unsupported, ignore-cipher-failure: strips faulty encryption
The previous commit's change to the pubkey tests is expanded similarly:
correctly encrypted messages cannot be decrypted by an OpenSSL that does
not support the cipher, regardless of the option's setting, though the
failure mode will change.
Suggested-by: Noah Misch <noah@leadboat.com>
Reviewed-by: Daniel Gustafsson <daniel@yesql.se>
Reviewed-by: Noah Misch <noah@leadboat.com>
Security: CVE-2026-14663
Backpatch-through: 14
PGP encryption was using px_cipher_encrypt without checking if any
error was returned. When OpenSSL is running in FIPS mode, or when
the legacy provider hasn't been loaded, not all ciphers which are
supported by the PGP code are available and fail the init step in
px_cipher_encrypt. Since the PGP encryption failed to notice this
it XORed the non-encrypted block with the plaintext, effectively
disabling the encryption.
This was found due to a report of PGP encryption not respecting
the pgcrypto.builtin_crypto_enabled flag and allowing Blowfish
and DES. This however turned out to be a false positive, since
the PGP code only use ciphers from OpenSSL and not the built in
ciphers.
Bug: #19457
Reported-by: Shishir Sharma <ansh01072001@gmail.com>
Reviewed-by: Jacob Champion <jacob.champion@enterprisedb.com>
Discussion: https://postgr.es/m/19457-4bab15c17aea36c7@postgresql.org
Security: CVE-2026-14663
Backpatch-through: 14
