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
bt_child_highkey_check() walks right along the child level following
btpo_next links, reading a page on each iteration, but its loop lacked a
CHECK_FOR_INTERRUPTS(). Every other page-traversal loop in
verify_nbtree.c already has one.
The loop's existing checks do not guarantee a timely exit on a corrupt
index. The in-loop cycle check only fires when a rightlink points back
through the block the walk started from, or to a page whose btpo_prev
points to itself, so a cycle further downstream can go undetected -- for
example a run of pages all flagged P_INCOMPLETE_SPLIT, which skips both
the high-key comparison and bt_downlink_missing_check() and so never
reaches an error. Such an index, or simply a very long rightlink chain,
makes the walk effectively uninterruptible. Since amcheck exists to be
run against possibly-corrupt indexes, make this walk respond to query
cancellation and shutdown requests like the sibling loops do.
Author: Paul Kim <mok03127@gmail.com>
Reviewed-by: shihao zhong <zhong950419@gmail.com>
Discussion: https://www.postgresql.org/message-id/20260717151437.10713-1-mok03127@gmail.com
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
In clang, -fsanitize=undefined includes -fsanitize=function, which
reports every call made through a function pointer whose type does not
exactly match the called function, so it fires all over the place on
expression tree walkers and mutators, as well as a few other places.
So -fsanitize=undefined hasn't been working cleanly under clang for a
while. (Before clang 17, it only applied to C++.)
This is the same issue that caused us to use
-Wno-cast-function-type-strict with clang. That warning applies at
the place where the mismatching function pointer is passed, so there
are potentially hundreds of sites. Therefore, a global disabling is
appropriate. The sanitizer, on the other hand, triggers where the
function is called, which are only about two dozen places, so it seems
possible to silence these checks individually and still main the check
for accidental violations elsewhere.
Add pg_attribute_no_sanitize_function() and place it on the functions
that make such calls.
Reviewed-by: Chao Li <li.evan.chao@gmail.com>
Discussion: https://www.postgresql.org/message-id/flat/2db401d9-ef16-430e-a42c-34477006a49f%40eisentraut.org
The OID8_FORMAT marker could break translated messages, as proven by
ceae3099da, so let's remove it while we can instead of encouraging its
use.
An alternative solution that would allow OID8_FORMAT is to save the
result into an intermediate string, itself used in the translated
strings, but per discussion this is not an appealing long-term approach.
Note that this commit partially reworks 072d4c4240.
Per discussion with Peter Eisentraut, Álvaro Herrera and Tom Lane.
Discussion: https://postgr.es/m/aqeO1OU7uZjhioAq@alvherre.pgsql
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
toast_internals.h described the header of a compressed-in-line varlena
a second time, as toast_compress_header, with its own TOAST_COMPRESS_*
macros to read and write the tcinfo word. varatt.h already describes
the same bytes as varattrib_4b.va_compressed and provides equivalent
macros that act as setter and getter of this information, as of
VARDATA_COMPRESSED_GET_EXTSIZE() and GET_COMPRESS_METHOD().
Let's remove the duplicate definition, and switch the TOAST code to use
the macros from varatt.h. There should be no behavior change.
Note that toast_compression.h is removed from toast_internals.h, added
to the C files where its declarations are required.
Author: Nikhil Kumar Veldanda <veldanda.nikhilkumar17@gmail.com>
Author: Michael Paquier <michael@paquier.xyz>
Discussion: https://postgr.es/m/CAFAfj_FeJQZrKyVn7ur0aJx8iz8SR14NheVJNyc8JNoqtMX+og@mail.gmail.com
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
Back in the days before a91e2fa94, hash_any() was the primary hash
function and it returned a Datum. That commit changed hash_any() to a
wrapper over hash_bytes(). Some places actually wanted the underlying
uint32 from hash_bytes(), but that now presents as useless round-trip
casting, so update the call sites where we don't immediately need a
Datum.
hash_numeric() didn't have round-trip casting, but it was internally
inconsistent: Paths returning a constant used PG_RETURN_UINT32(), but
the path using the result of hash_any() used PG_RETURN_DATUM. Change
it to use uint32 throughout.
Author: Aleksander Alekseev <aleksander@tigerdata.com>
Suggested-by: John Naylor <johncnaylorls@gmail.com>
Reviewed-by: Zsolt Parragi <zsolt.parragi@percona.com>
Discussion: https://postgr.es/m/CAJ7c6TMPhDRQMmkUHPv8oOK97B1mR8NRS61DgjpdaZUPAwaeZQ%40mail.gmail.com
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>
These errors have been reported to the mailing lists in the last few
weeks. Some of the spots fixed have been noticed by myself during
independent code reviews, but not all.
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
Some of the tests introduced by c68cba09dd in these two modules want
to rely on out-of-line values (compressed and not compressed), but the
values inserted that should be compressed were not (STORAGE EXTENDED).
The tests are updated to do what they should, reusing a cheap trick to
ensure out-of-line values coming from a different test in test_decoding.
Some checks are added to ensure that the expected set of values is
inserted.
Reported-by: Rui Zhao <zhaorui126@gmail.com>
Discussion: https://postgr.es/m/CAHWVJhGxVc4CpDd+mb=6YbeL2KHDUtsUC=R68y7NeMg1cO8QMg@mail.gmail.com
Add table_index_getnext_slot, a new table AM interface that serves as
the single entry point for both plain and index-only index scans that
use amgettuple. The table AM's index_scan_begin callback resolves which
implementation the scan will use and stores it in an IndexScanDesc
function pointer (xs_getnext_slot), which table_index_getnext_slot
dispatches through.
The interface intentionally avoids using fixed TableAmRoutine members,
allowing the table AM to dynamically register an implementation based on
conditions known at the start of each scan. That way a table AM can
provide specialized variants that help the compiler produce more
efficient code. heapam provides two implementations: one for plain
index scans, and one for index-only scans. An upcoming commit that adds
the amgetbatch interface will add two more. (Table AMs can opt to
provide a single generic implementation, or to provide many specialized
implementations, as requirements dictate.)
heapam's xs_getnext_slot implementations make aggressive use of forced
inlining to ensure that plain and index-only code paths are fully
specialized at compile time, without duplicating the code they have in
common. This specialization is necessary to keep icache and iTLB
pressure to a minimum.
The new interface moves visibility map checks required by index-only
scans out of the executor (and selfuncs.c) and into heapam, enabling
batching of visibility map lookups (though for now we continue to just
perform retail lookups). Using the new higher level slot-based
interface greatly simplifies nodeIndexonlyscan.c, which no longer has to
deal with the visibility map directly. More importantly, this is a
significant architectural improvement: table AMs can now implement
index-only scans that are not tied to heapam's visibility map.
Two users of the now removed table_index_fetch_tuple interface,
_bt_check_unique and unique_key_recheck, fundamentally need to pass a
TID to the table AM to perform constraint enforcement. Neither actually
performs an index scan (even though their TIDs are taken from an index),
so neither has any use for most of the index scan machinery. Switch
these callers over to fetch_tid, a new special-purpose table AM
interface for constraint enforcement code. All true index scan callers
now use the new slot-based interface. (Note that fetch_tid doesn't
perform on-access pruning, which matches how things worked prior to
Postgres 12 commits c2fe139c2 and 71bdc99d0.)
The VISITED_PAGES_LIMIT mechanism used by get_actual_variable_range to
cap scan overhead during planning is reworked to go through a new scan
descriptor field (xs_visited_pages_limit), rather than having selfuncs.c
count heap page fetches and terminate the scan itself. This is
necessary because callers that use the new slot-based interface no
longer have direct access to which heap blocks were fetched. Similarly,
nodeIndexonlyscan.c can no longer use InstrCountTuples2 to count heap
fetches during an EXPLAIN ANALYZE. EXPLAIN ANALYZE now obtains this
information from a new IndexScanInstrumentation field, which table AMs
are required to maintain during index-only scans.
Though independently useful, this commit is preparatory work for an
upcoming commit that will add an amgetbatch index AM interface, where
the table AM takes full responsibility for managing the progress of
index scans. The table AM determines when to request the next batch
from the index AM, at a time of its choosing. A follow-on commit will
rely on this to prefetch table blocks during index scans. Having a
single entry point for all index scan callers unambiguously puts the
table AM in control of the scan.
Author: Peter Geoghegan <pg@bowt.ie>
Reviewed-by: Andres Freund <andres@anarazel.de>
Reviewed-by: Tomas Vondra <tomas@vondra.me>
Reviewed-by: Rui Zhao <zhaorui126@gmail.com>
Discussion: https://postgr.es/m/CAH2-WzmYqhacBH161peAWb5eF=Ja7CFAQ+0jSEMq=qnfLVTOOg@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
e994f956e4 Forbid generated columns in FOR PORTION OF
dfce19c230 Forbid FOR PORTION OF on views with INSTEAD OF triggers
20fe90bbb9 Fix RLS checks for FOR PORTION OF leftover rows
9170c8b716 Test what BEFORE UPDATE triggers do to FOR PORTION OF
7090c696cc Deparse FOR PORTION OF using the range column's current name.
62d1a5f8be Avoid RETURNING side effects for FOR PORTION OF leftovers.
9924e2d900 Enforce WITH CHECK OPTION on DELETE FOR PORTION OF leftovers
c16a1b4f2c Fix error code for null FOR PORTION OF target
610976420a Fix assertion failures in DELETE FOR PORTION OF tuple routing
438f027238 Resolve untyped parameters in FOR PORTION OF FROM/TO bounds
140fdfcdf1 Don't evaluate the FOR PORTION OF target under EXPLAIN
Discussion: https://www.postgresql.org/message-id/62bf70cc-dc33-4188-8070-a8c177bb61de%40eisentraut.org
Commit 0ee83dd4a9 added a call of assign_record_type_typmod() in
postgres_fdw.c, but relied on utils/typcache.h being included via
nodes/execnodes.h. That was put there as part of the FOR PORTION OF
feature, and if that were to change, there would be a possible
unrelated compilation failure. To fix, add the required include
directly.
A chunk_id type that does not match with one of the expected values (oid
or oid8) was missing a "return" clause. This caused the code to report
two corruptions instead of one.
Oversight in 6e58d6356f.
Discussion: https://postgr.es/m/aqju1K96ex28yz2X@paquier.xyz
A TOAST table whose chunk_id column is oid8 is now fed a 64-bit object
ID thanks to the introduction of a new vartag_external called
VARTAG_ONDISK_OID8, coupled with a new varatt_external_oid8. The only
difference between the existing varatt_external_oid and the new
varatt_external_oid8 is an extra 4 bytes to store 8 bytes worth an
object ID, spread across two uint32 to force alignement as memcmp may be
used for TOAST pointer comparison (line varatt_external_oid).
There is a one-one mapping between the vartag_external and the type of a
chunk_id:
- An oid8 chunk_id implies the use of VARTAG_ONDISK_OID8.
- An oid chunk_id implies the use of VARTAG_ONDISK_OID.
There are a couple of changes worth noting:
- In toast_tuple_init(), for the comparison of old and new external
values with a memcmp over VARSIZE_EXTERNAL(old_value). Now that
multiple types of external on-disk TOAST pointers are possible, we
need to add a check based on the vartag used by an old or new value.
One of the tests added by c68cba09dd cover this case.
- In toast_tuple_find_biggest_attribute(), the initial "biggest_size"
depends on the type of chunk_id. This needs a change in RelationData to
track the chunk_id type; this cached data is filled on demand.
- In detoast.h, toast_external_info_get() and toast_external_data act as
a translation layer to retrieve all the data associated to an Oid or an
Oid8 TOAST pointer, without the need to worry about the vartag dealt
with. The introduction of this function and structure has reduced a lot
of code churn that existed in the initial versions of this patch, as per
a suggestion from Bharath Rupireddy.
- amcheck is updated to check the vartag <-> chunk_id type mapping
requirements, handling both vartags depending on the underlying type of
chunk_id on the TOAST table.
Bump catalog version, due to at least the change of VARTAG_SIZE().
(This counts for the bump forgotten in a625fc570c22..)
Author: Michael Paquier <michael@paquier.xyz>
Reviewed-by: Greg Burd <greg@burd.me>
Reviewed-by: Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com>
Reviewed-by: Yugo Nagata <nagata@sraoss.co.jp>
Discussion: https://postgr.es/m/af19kUjwjhaoUTLn@paquier.xyz
The tests added by this commit cover a large ground in terms of the
introduction of oid8 in TOAST tables, with the following areas covered,
with all the gaps found during the development of the feature:
- Tables with oid8 TOAST tables in general, under strings.sql.
- ALTER TABLE with attribute manipulations in transactions and different
toast_table_value settings.
- Relation rewrites, not affecting the TOAST relation after initial
creation with VACUUM FULL, CLUSTER, REPACK.
- pg_column_compression().
- test_decoding, for external and extended TOAST data.
- amcheck, external and extended storage with heap checked.
- UPDATE with out-of-line datums that belongs to another TOAST table.
This offers coverage for toast_tuple_init(), where external vartags
could be mixed once multiple on-disk external TOAST pointers can
co-exist. This pattern can be triggered with asan/ubsan, with an
out-of-bound memcpy when unpatched.
These tests are added before the new vartag_external, mostly as a matter
of avoiding dead code in the tree.
Author: Michael Paquier <michael@paquier.xyz>
Reviewed-by: Greg Burd <greg@burd.me>
Reviewed-by: Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com>
Reviewed-by: Yugo Nagata <nagata@sraoss.co.jp>
Discussion: https://postgr.es/m/af19kUjwjhaoUTLn@paquier.xy
This commit adds the possibility to define TOAST tables with oid8 as
their chunk_id, based on the reloption toast_value_type. All the
external TOAST pointers still rely on varatt_external_oid and a single
vartag. The values inserted in the oid8 TOAST tables are fed from the
OID8 value generator, casted to OID for now, as we do not have a
vartag_external able to store Oid8 values yet.
An upcoming commit will add support for a new vartag_external and its
associated structures, with the code being able to use a different
on-disk external TOAST pointer depending on the attribute type of
chunk_id defined in TOAST relations.
All the changes done here are mechanical, mostly around the TOAST code
that needs to be able to do chunk_id lookups based on the two types now
supported.
Bump catalog version.
Author: Michael Paquier <michael@paquier.xyz>
Reviewed-by: Greg Burd <greg@burd.me>
Reviewed-by: Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com>
Reviewed-by: Yugo Nagata <nagata@sraoss.co.jp>
Discussion: https://postgr.es/m/af19kUjwjhaoUTLn@paquier.xy
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
This is a mechanical update to change most of the TOAST code paths that
assume TOAST value IDs to be Oid variables to become Oid8 variables,
easing an the introduction of an upcoming patch to allow larger TOAST
values, at 8 bytes.
The areas touched by the change are related to table AM, amcheck and
logical decoding's reorder buffer. A good chunk of the changes involve
switching printf() markers from %u to OID8_FORMAT.
For the sake of the release notes in terms of compatibility (perhaps
worth mentioning, or not), note that this changes some callback
definitions in the table AM section, upgrading some TOAST callbacks to
use Oid8 instead of Oid when referring to a TOAST value.
Author: Michael Paquier <michael@paquier.xyz>
Reviewed-by: Greg Burd <greg@burd.me>
Reviewed-by: Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com>
Reviewed-by: Yugo Nagata <nagata@sraoss.co.jp>
Discussion: https://postgr.es/m/af19kUjwjhaoUTLn@paquier.xy
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
Commit 0ee83dd4a9 allowed postgres_fdw to push down inner joins between
foreign tables and functions in FROM clauses to the remote server.
However, planning such a join with use_remote_estimate enabled could
cause a segmentation fault. For example, the following query crashed
when ft had use_remote_estimate set to true:
EXPLAIN SELECT * FROM generate_series(1, 3) g JOIN ft ON ft.i = g;
The crash occurred because init_func_stub_fpinfo() copied the connection
information and options from the foreign side to the function-side stub,
but omitted the user mapping, leaving it NULL. When the function was the
outer side of the join, the join inherited this NULL mapping and passed it
to GetConnection() when requesting a remote estimate, causing a
segmentation fault.
Fix this by having init_func_stub_fpinfo() also copy the user mapping
from the foreign side.
Author: Fujii Masao <masao.fujii@gmail.com>
Reviewed-by: Alexander Pyhalov <a.pyhalov@postgrespro.ru>
Discussion: https://postgr.es/m/CAHGQGwGE9xPzCke0C0r4omTkh-hU08aBd2j9oSA0FYmJLCf9jA@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
