Previously, pg_locks.waitstart could report the start time of a previous
lock wait for a new wait. For a regular backend, this could happen briefly
before the new start time was stored. On a standby, the startup process could
show the old time throughout the next wait, making it appear to have started
earlier than it did.
This happened because PGPROC->waitStart could remain set after a previous
wait in two cases. First, RemoveFromWaitQueue() did not clear it when a wait
was canceled due to a lock timeout, cancellation, or deadlock. Second, even
after a successful lock grant, ProcWakeup() could clear it before the waiter
stored its new start time.
Fix this by clearing waitStart in RemoveFromWaitQueue() when a failed wait
is removed, at the end of ProcSleep() to handle writes made after
ProcWakeup() cleared it, and in LockErrorCleanup() when an error bypasses
the ProcSleep() reset. This prevents pg_locks.waitstart from showing the
previous wait's time for a new wait.
Backpatch to all supported versions.
Author: Shihao Zhong <zhong950419@gmail.com>
Reported-by: Alex Shapalov <shapalov@gmail.com>
Reviewed-by: Chao Li <li.evan.chao@gmail.com>
Reviewed-by: Michael Paquier <michael@paquier.xyz>
Reviewed-by: Fujii Masao <masao.fujii@gmail.com>
Reviewed-by: Andrew Krylosov <krylosov.andrew@gmail.com>
Discussion: https://postgr.es/m/CAPrb+Q+XN=sNusXiUeWmMo2H7Qgq3Y4uPekSSLkHcnCyf7GhXg@mail.gmail.com
Discussion: https://postgr.es/m/CAGRkXqQLxZBr-ouVrtaX2utMggi4+TiVMbgH04b_0JLgKryxbA@mail.gmail.com
Backpatch-through: 14
This marker could break translated strings if used, so let's remove it
and encourage instead the use of PRIu64, something that xgettext is able
to digest.
This marker has been introduced in b139bd3b6e, down to v19 that has
not been officially released yet, so a removal is still possible without
breaking the ABI requirements.
Discussion: https://postgr.es/m/aqeO1OU7uZjhioAq@alvherre.pgsql
Backpatch-through: 19
_reconnectToDB() failed to consider the possibility that it could
be called without any restrict_key set. This is reachable while
generating the "restore.sql" file in tar dump mode, which nobody
noticed because that file is mostly unused (and nothing really
bad happens anyway unless you specified --inserts mode). But
I wonder if there are other code paths that reach the issue.
Bug: #19718
Reported-by: Pavel Kopylov <pkopylov@cloudlinux.com>
Author: Pavel Kopylov <pkopylov@cloudlinux.com>
Reviewed-by: Tom Lane <tgl@sss.pgh.pa.us>
Discussion: https://postgr.es/m/19718-7e945d0ff9b5a589@postgresql.org
Backpatch-through: 14
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
ri_FastPathCheck() invokes two functions on the FK values: the
equality operator's function, via the ScanKey the index AM evaluates,
and, when the FK column's type differs from the PK column's, the
implicit cast function. The SPI path gets EXECUTE on both checked by
ExecInitFunc() when it initializes the generated query, as the
referenced table's owner, but the fast path performed neither check.
Add ri_CheckFunctionPermissions() and call it once the fast-path
metadata is populated, under the switch to the referenced table's
owner, so the checks happen at the same point as the existing relation
permission check. Add a regression test for each function, reaching
the fast path through per-row validation of a new constraint, and
repeat each against a partitioned referenced table, which takes the
SPI path, to show that the two behave identically.
Reported-by: Nikolay Samokhvalov <nik@postgres.ai> (offlist)
Reviewed-by: Matheus Alcantara <matheusssilv97@gmail.com>
Discussion: https://postgr.es/m/CA+HiwqGH+b7sXmsH8sT18diujbheSObWs0gWXqLZqRccMKvZAA@mail.gmail.com
Backpatch-through: 19
pg_restore_attribute_stats() rejected range_length_histogram,
range_empty_frac and range_bounds_histogram for a column whose type is a
domain over a range or a multirange type. The same issue existed for
most_common_elems and elem_count_histogram for a domain over tsvector.
pg_restore_extended_stats() had the same set of problems for expressions
whose type is a domain over a range, a multirange type, or tsvector.
ANALYZE is able to generate such stats, incorporating the knowledge to
handle domains in the in-core typanalyze callbacks.
This problem is resolved by being more aggressive with the fetch of the
base type of a domain in the early phases of restore for attribute and
extended stats (special tip to Jian He for pointing out the unnecessary
typcache lookups), reflecting on the surrounding helper routines shared
by both code paths. The fix for v18 is local to attribute_stats.c, as
only attribute stats need to be touched.
Tests are included in a fashion consistent with the surroundings. The
consequence of this issue was the rejection of stats that ANALYZE was
able to build, which was not critical but annoying as it would lead to a
gap in the stats restored.
Reported-by: Qifan Liu <imchifan@163.com>
Author: Michael Paquier <michael@paquier.xyz>
Reviewed-by: Jian He <jian.universality@gmail.com>
Reviewed-by: Corey Huinker <corey.huinker@gmail.com>
Discussion: https://postgr.es/m/19715-b8be35083016f289@postgresql.org
Backpatch-through: 18
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
When applying the concurrent data changes, REPACK deformed the decoded
tuples with the descriptor of the transient relation. That descriptor
has no "missing" values, because make_new_heap() gives the transient
relation none of the defaults and constraints of the source relation. A
tuple written before an ALTER TABLE ... ADD COLUMN that did not rewrite
the table has fewer attributes than the descriptor, so the missing
values came out as NULL, even in a column declared NOT NULL. If such a
value belongs to the replica identity, the key used to find the target
tuple was NULL too, so the tuple to modify cannot not be found. The
scan key does not remember that a value is NULL, so a pass-by-reference
key would even crash the comparison function. Such tuples will be rare,
because new row versions are normally formed with the current
descriptor. A BEFORE ROW UPDATE trigger returning OLD is one way to
produce one.
Fix by deforming the decoded tuples with the descriptor of the source
relation, which does have the missing values. Tuples formed with it are
still valid for the transient relation, whose attributes are a copy of
the source relation ones. Commit 20d3fe9009 took the same approach
for INSERT and UPDATE in the executor. Expanding the decoded tuple with
heap_expand_tuple() would be the other option, but that is the
workaround of commit ba9f18abd that 20d3fe9009 got rid of, so do not
bring it back.
Add an isolation test.
Author: Sami Imseih <samimseih.pg@gmail.com>
Reported-by: Shihao Zhong <zhong950419@gmail.com>
Reviewed-by: Kirill Reshke <reshkekirill@gmail.com>
Backpatch-through: 19
Discussion: https://postgr.es/m/CAN12+Y+NJwr5EqKVrHpPD=5+b_NRAOzU9fhrVDO3H46x2WRxoA@mail.gmail.com
gcc 12.5.0 complains about this instance in LookupFuncWithArgs().
This is the same type of problem fixed in commit f04781df5, and
it's just as obscure why this compiler version picks on this case
when others don't. Still, silence it in the same way as f04781df5.
Author: Tom Lane <tgl@sss.pgh.pa.us>
Discussion: https://postgr.es/m/1812498.1790279465@sss.pgh.pa.us
Backpatch-through: 14
The sequencesync worker asks the publisher about a batch of sequences,
tagging each one with its position in the worker's own list, and the
publisher returns that position alongside the sequence's data. The
received position is used to subscript the list using list_nth(),
which bounds-checks only on assert builds, so it was possible that an
index we never sent made the worker read a pointer from past the end
of the list and then store the remote last_value through it.
Check the position against the list before using it. No sane publisher
can trigger this, but we should not let a remote server steer a memory
access. An in-range position from another batch still gets through and
would attach one sequence's data to another, but that's a wrong value
rather than a corrupt process.
Also, receive the index as int4 rather than int8. Previously, an
out-of-range int8 value was silently truncated to its lower 32 bits by
DatumGetInt32(), which could turn it into a valid index and defeat the
check.
Backpatch to v19, where sequence synchronization was introduced.
Reported-by: Anthropic OSS program
Reviewed-by: Amit Kapila <amit.kapila16@gmail.com>
Reviewed-by: Chao Li <li.evan.chao@gmail.com>
Discussion: https://postgr.es/m/CAD21AoBWDMuMmnevZaZ1xSOi75eUx8X7LbVQzmUZudk6F8Bdgg@mail.gmail.com
Backpatch-through: 19
Autovacuum doesn't necessarily fully empty the GIN pending list. It
stops at the tail as of the start of cleanup, so that it doesn't keep
chasing pages appended by concurrent inserters. GIN decided this with
AmAutoVacuumWorkerProcess(), which returns false in parallel vacuum
workers launched by an autovacuum worker, so those workers tried to
fully empty the list as a manual VACUUM does.
Fix this by adding an is_autovacuum field to IndexVacuumInfo, set from
the shared parallel vacuum state in parallel workers, and using it in
GIN instead of checking the process type.
Oversight in commit 1ff3180ca0.
Reported-by: Jingtang Zhang <mrdrivingduck@gmail.com>
Author: Jingtang Zhang <mrdrivingduck@gmail.com>
Reviewed-by: Andrey Borodin <x4mmm@yandex-team.ru>
Reviewed-by: Yuhang Qiu <iamqyh@gmail.com>
Reviewed-by: Masahiko Sawada <sawada.mshk@gmail.com>
Discussion: https://postgr.es/m/CAPsk3_DiF2i1o=JRXRk1YPVMv1VzJaCAbfiEqgwCva4=ZewVAw@mail.gmail.com
Backpatch-through: 19
ShmemInitStruct() only looked for an existing allocation when
IsUnderPostmaster was true. Because of that, you could not reattach
to an existing shmem area in a standalone backend.
Usually, you only call ShmemInitStruct() for the same shmem area once
in a process and hold onto the returned pointer, but there was no rule
that you couldn't call it twice for the same area if you wanted to,
and before commit 283e823f9d, it worked. To fix, check for an
existing allocation regardless of process type, like the old
implementation did.
Author: Ayush Tiwari <ayushtiwari.slg01@gmail.com>
Reviewed-by: Ashutosh Bapat <ashutosh.bapat.oss@gmail.com>
Discusssion: https://www.postgresql.org/message-id/CAJTYsWXREGXCnWdZJsXt8pT2UY_Y2GSx+HOUu2X3s8YDCLj0_A@mail.gmail.com
Backpatch-through: 19
Remove assertion that was too strict, and tripped if you queried the
pg_shmem_allocations_numa or the pg_buffercache_os_pages view in
single-user mode.
The point of the assertion was that the function should only be called
after "the server has started", but it would be OK to call it from the
postmaster, too, as long as it's after startup, specifically after
huge_pages_status has been set. We already have a direct assertion
for that.
Author: Ayush Tiwari <ayushtiwari.slg01@gmail.com>
Discussion: https://www.postgresql.org/message-id/CAJTYsWV-=w5_oFgCnfAixWGc=gMa2pkDinLyGLsb4sKcDaf2FA@mail.gmail.com
Backpatch-through: 18
The fast path's ri_CheckPermissions() called ExecCheckOneRelPerms()
directly, so ExecutorCheckPerms_hook never saw the referenced-table
access that the SPI path's query exposed to it via ExecutorStart().
Modules such as sepgsql that rely on this hook therefore lost control
over FK checks against referenced tables that go through the fast
path.
So, build a one-entry range table and permission-info list and call
ExecCheckPermissions(). The hook now sees the same relation and
requiredPerms as before, once per check that goes through the fast
path. This also drops the shortcut that skipped the column-level
check when table-level privileges sufficed, since the hook must run
regardless. While at it, drop the explicit frees in the old code,
because both callers run ri_CheckPermissions() in a per-tuple context
that is reset after each check, so they were redundant.
Reported-by: Noah Misch <noah@leadboat.com>
Discussion: https://postgr.es/m/20260705210533.ee.noahmisch%40microsoft.com
Backpatch-through: 19
SnapBuildInitialSnapshot() converts the snapshot builder's list of
committed transactions into a regular MVCC snapshot, which is then used
with HeapTupleSatisfiesMVCC(). However, a snapshot produced that way
and used as an MVCC snapshot can potentially cause data corruption, if
it captures a transaction after it writes its commit WAL record but
before it has updated its CLOG entry: it will be incorrectly used to set
hint bits as if that transaction had aborted.
Fix by having SnapBuildInitialSnapshot() wait until such transactions
have removed themselves from procarray, which guarantees correct
visibility.
Other uses of SnapBuildBuildSnapshot only use the snapshot as
historical, so they don't need the waits, but add commentary in that
function to alert possible future callers.
While at this, backpatch commit 504fe10d1d, "Fix misplaced comment in
snapbuild.c." The comment it moved was nearby the place we modify.
This bug was diagnosed by stress-testing of REPACK (CONCURRENTLY) in
branch 19, so add a test case that tickles it using that feature in that
branch and master. However, the bug exists in all branches, and in
branch 18 we can add a test with regular logical decoding using
injection points that tickles it. Unfortunately that test cannot be
backpatched to earlier branches for lack of facilities.
Reported-by: Mihail Nikalayeu <mihailnikalayeu@gmail.com>
Author: Antonin Houska <ah@cybertec.at>
Author: Rui Zhao <zhaorui126@gmail.com>
Backpatch-through: 14
Discussion: https://postgr.es/m/85833.1768840165@localhost
Discussion: https://postgr.es/m/CADzfLwU78as45To9a%3D-Qkr5jEg3tMxc5rUtdKy2MTv4r_SDGng%40mail.gmail.com
Commit 7f25eccf41 added a cross-check between pg_database.datminmxid
and oldestMulti, but because PostgreSQL 9.2 doesn't have datminmxid,
it broke upgrade from version 9.2. Disable the check when upgrading
from 9.2 to fix.
This is for REL_19_STABLE only. Earlier versions don't have the check,
and on 'master' we no longer support upgrading from version 9.2.
Reported-by: Andrew Dunstan <andrew@dunslane.net>
Discussion: https://www.postgresql.org/message-id/baef33d6-0416-48db-8e4d-0529a10a101a@dunslane.net
MultiXactState->oldestOffset is not maintained during recovery, so
calling pg_get_multixact_stats() in this context would report misleading
information. We could try to work this out by adding this field to the
control file, for example, but such a change would be too invasive at
this stage of the v19 development cycle.
The information of pg_get_multixact_stats() is usually useful to control
the need of some VACUUM or some freezing, operations that cannot happen
on a standby, so the argument to make the function work while recovery
is in progress gets thinner with this point in the balance.
The function is updated to return an ERROR when called under
RecoveryInProgress().
Per discussion with Heikki Linnakangas and Naga Appani.
Reported-by: Noah Misch <noah@leadboat.com>
Discussion: https://postgr.es/m/CA+QeY+DcnsZRL4GZD8JyJu5HD-d6p8_9M5kT4jgF3gq3Rh8n4w@mail.gmail.com
Discussion: https://postgr.es/m/20260827231757.78.noahmisch@microsoft.com
Backpatch-through: 19
The "initializing" phase was described as expected to be very brief,
but for REPACK (CONCURRENTLY), it also covers enabling and
initializing logical decoding, either of which can take a long
time. Also, the "catch-up" phase includes waiting for the ACCESS
EXCLUSIVE lock needed to swap the files, which was not mentioned.
Adding separate progress phases for these steps is left for v20; for
v19 we only clarify the description.
Author: Sami Imseih <samimseih.pg@gmail.com>
Reviewed-by: Masahiko Sawada <sawada.mshk@gmail.com>
Reviewed-by: Manuel Reyes Bravo <manuelreyesbravo@gmail.com>
Discussion: https://postgr.es/m/CAN12%2BYJOeOyPYB5JS28VE_%2Brzg%3DRbzQXfj58TGfMYwv-TX9mJA%40mail.gmail.com
Backpatch-through: 19
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
heapgettup_advance_block() (or heapgettup() and heapgettup_pagemode() in
versions before v16) would incorrectly decrement rs_numblocks when
advancing to the next block. Ordinarily this was fine, but if the TID
Range scan plan was used by a scrollable cursor and the cursor was fetched
backwards enough to result in another block being required, then
rs_numblocks may have reached 0 prematurely resulting in the scan ending
prematurely.
Reported-by: Jeff Davis <pgsql@j-davis.com>
Author: Ewan Young <kdbase.hack@gmail.com>
Author: David Rowley <dgrowleyml@gmail.com>
Discussion: https://postgr.es/m/6b50233fcc0f7d6f4845f083777fa9702d9612e4.camel@j-davis.com
Backpatch-through: 14
When executing a WHEN NOT MATCHED BY SOURCE action in MERGE, if the
tuple from the target table was concurrently deleted, the retry code
would incorrectly execute a WHEN NOT MATCHED BY TARGET action for the
row instead, giving incorrect results.
Fix in ExecMergeMatched() -- when handling a TM_Deleted failure
status, it should only set *matched to false (telling the caller to
execute any WHEN NOT MATCHED BY TARGET actions) if the failed action
was a WHEN MATCHED action. For a failed WHEN NOT MATCHED BY SOURCE
action, the source tuple did not exist, and the target tuple has just
been deleted, so it should do nothing.
Back-patch to v17, where support for WHEN NOT MATCHED BY SOURCE
actions was introduced.
Reported-by: Jeff Davis <pgsql@j-davis.com>
Author: Tender Wang <tndrwang@gmail.com>
Reviewed-by: Dean Rasheed <dean.a.rasheed@gmail.com>
Discussion: https://postgr.es/m/ccdab5ba02c65af195b5a6d2d744a01d9de47cd3.camel@j-davis.com
Backpatch-through: 17
Commit 05c4b10fc5 added an alignas specifier, and placed it in the
middle of the declaration, unlike most existing uses, possibly to
avoid the indentation problem recently fixed in commit 884f82ae26.
This placement is valid in C but not in C++, which requires it to be
at the beginning. This results in a warning in g++ and an error in
clang++. Fix by moving it accordingly. The fix for the indentation
problem has been backpatched to support this patch.
pg_bsd_indent doesn't know about alignas(), so a struct member
declared with it is misindented unless it's the first member.
Disguise each alignas(...) call as a plain identifier while
indenting, restoring it afterward. Stash the call's original text
by index rather than embedding it in the placeholder: embedding only
works for a single-token argument, and fails to match at all against
nested parens (e.g. alignas(sizeof(x))) or a line break, silently
skipping the disguise.
This is backpatched to PostgreSQL 19 because it is by coincidence
needed for a bug fix.
Co-authored-by: Peter Eisentraut <peter@eisentraut.org>
Co-authored-by: Andrew Dunstan <andrew@dunslane.net>
Discussion: https://postgr.es/m/94a128da-bf3e-46bd-9e2d-609573c484da@eisentraut.org
A sequencesync worker builds its list of sequences and then processes them
in batches. If ALTER SUBSCRIPTION ... REFRESH PUBLICATION removes one of
those sequences in the meantime, the worker updated the local sequence and
then failed while marking it READY, because its catalog row was gone. That
rolled back the batch, and with disable_on_error the entire subscription
was disabled.
The worker now checks that the sequence is still part of the subscription
before updating it, and skips it if not. The check is done under the lock
on the subscription that the refresh also takes, so the row cannot go away
in between.
Reported-by: Nikolay Samokhvalov <nik@postgres.ai>
Author: vignesh C <vignesh21@gmail.com>
Reviewed-by: shveta malik <shveta.malik@gmail.com>
Reviewed-by: Zhijie Hou <houzj.fnst@fujitsu.com>
Reviewed-by: Andrey Borodin <x4mmm@yandex-team.ru>
Discussion: https://postgr.es/m/CAM527d9mL-bOofX-G7Sp441vA3Y_fa_7JBh03m-ZPduikn_XUg@mail.gmail.com
Backpatch-through: 19, where it was introduced
6dbb490261 combined pruning and freezing in a single WAL record but
unconditionally requested a cleanup lock during replay. Prior to version
17, freeze-only records were replayed under an ordinary exclusive lock.
1252a4ee28 subsequently folded visibility map updates into the same
records, extending the unnecessary cleanup-lock requirement to VM-only
updates starting in version 19.
Request a cleanup lock in redo only when the record actually prunes.
Freezing and visibility updates do not move or remove tuple storage, so
they need not wait for other buffer pins to be released. This avoids
unnecessary replay delays and cancellation of standby queries holding
buffer pins.
The fix is the same for all versions, though it addresses two oversights
in master/19 and one in 18/17.
Reported-by: Satyanarayana Narlapuram <satyanarlapuram@gmail.com>
Reported-by: Andrey Borodin <x4mmm@yandex-team.ru>
Reviewed-by: Andrey Borodin <x4mmm@yandex-team.ru>
Discussion: https://postgr.es/m/85b6968b-49f2-4243-86a6-9b4116e6439b@iki.fi
Discussion: https://postgr.es/m/CAHg+QDf3NcB3vOAqQ5EFcV5DYYUZ9snCSJAU-x1JDbtcjK3eBQ@mail.gmail.com
Discussion: https://postgr.es/m/F9BF304E-1298-481C-829A-B4E10AED025A@yandex-team.ru
Backpatch-through: 17
Before pruning a heap page, we get the current status of the
corresponding VM page. If the passed in vmbuffer isn't the right one,
visibilitymap_get_status() will silently unpin it and pin the correct
page. Pruning assumes the caller manages the vmbuffer lifecycle, so this
would leave the caller with a stale VM reference and would leak the new
VM pin. To avoid mistakes in development, assert that the correct VM
page is pinned before beginning.
Reported-by: 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
Similar to commit "Remove unused global variables", silence
-Wunused-but-set-global warnings from clang 23 (included in -Wall).
These are variables that are used in assert-enabled builds only, so
annotate them with PG_USED_FOR_ASSERTS_ONLY.
The test added by c62b330912 creates a btree opfamily whose members
include built-in integer operators, which get_mergejoin_opfamilies()
finds regardless of schema. So any concurrently running test that
planned an integer join or sort could pick the family up while it
existed. That's the likely cause of the intermittent plan-shape
changes seen in window.sql and equivclass.sql on the buildfarm after
that commit.
Run the test inside a transaction that is rolled back, so the family
is never committed and never visible to other backends.
Per buildfarm members sifaka, prion, turaca, widowbird.
Discussion: https://postgr.es/m/CA+HiwqGitHCmO7nvV4nXXYWDkNYA+zVcJj=JNmL3C1O9B0Gz0A@mail.gmail.com
Backpatch-though: 19
When a join has an alias, the parser marks every Var referenced
through that alias with the join's RT index as its syntactic referent
(varnosyn), even though varno points at the underlying base relation.
Since commit 2ebf25e7d, join removal strips the removed relids from
the query tree using ChangeVarNodes() with INVALID_VAR, which asserts
that the relid doesn't appear in any field identifying a single
relation. A varnosyn that names the removed join violates that, so a
query like
SELECT j.x FROM (t1 s(x) LEFT JOIN t2 ON s.x = t2.a) j;
with t2.a unique fails an assertion. In non-assert builds, varnosyn
is silently set to INVALID_VAR.
Such syntactic references are still valid after the removal: the join
is only taken out of the jointree, and its RTE remains in the
rangetable through to the finished plan. So, when deleting a relid,
leave varnosyn unchanged, as join removal did before 2ebf25e7d.
Back-patch to v16, as with commit 2ebf25e7d.
Author: Richard Guo <guofenglinux@gmail.com>
Reviewed-by: Tom Lane <tgl@sss.pgh.pa.us>
Discussion: https://postgr.es/m/CAMbWs484SQSdzWTgZYKvwtuC6J4m7acw0LfBjp3aWPvav7gC_w@mail.gmail.com
Backpatch-through: 16
Commit e28cf96e9 preprocessed the copies of a query level's
PlaceHolderVars that were pushed down into its subqueries in a single
walk at the start of subquery_planner. That was wrong in two ways.
First, such copies can also be inserted later, when a join alias Var
of the outer level is expanded within a LATERAL subquery or a
SubLink's subselect and the alias expression contains a PHV. Those
copies were never preprocessed, so a SubLink within them survived into
the subquery's lateral references or into a SubPlan's argument list,
tripping an assertion in identify_current_nestloop_params or failing
in cost_qual_eval.
Second, when a pushed-down copy directly contains another PHV of the
same level, the walk preprocessed the inner one first and then the
outer one, so the outer one's preprocessing ran into the SubPlans just
created in the inner one and tripped an assertion in
flatten_join_alias_vars.
To fix, preprocess the copies right after join alias expansion
instead: within preprocess_expression, just before SubLinks are turned
into SubPlans, and for LATERAL subquery RTEs, right after their
aliases are flattened. Preprocessing a copy handles everything within
it, including any further copies nested inside its SubLinks, so the
walker no longer looks inside a copy.
Back-patch to v16, as with commit e28cf96e9.
Reported-by: Fujii Masao <masao.fujii@gmail.com>
Author: Richard Guo <guofenglinux@gmail.com>
Discussion: https://postgr.es/m/CAHGQGwH-acRnAk1=Ki4EiAJEt67+Rz=_jyg7pbP_c6BK4oHZ4Q@mail.gmail.com
Backpatch-through: 16
The PASSING arguments of JSON_EXISTS(), JSON_QUERY(), JSON_VALUE() and
JSON_TABLE() are evaluated by plain expression steps and given to the
jsonpath executor as-is.
When dealing with a varlena, JsonItemFromDatum() was building a
jbvString directly the contents of a varlena. For a varatt_external
(varatt_external[oid/oid8] on HEAD), this ignored that the contents of
the varlena need to be detoasted, leading to incorrect contents
returned.
Let's detoast the value, like the jsonb and json cases. b0feda79fd
has fixed the same issue for subscript values in jsonb.
Note that the results of DatumGetTextPP() are allocated in the memory
context used for the evaluation of the expression, which is reset once
per row.
Oversight in 6185c9737c.
Author: Chaitanya Choudhary <chaitanyyachoudhary@gmail.com>
Author: Shihao Zhong <zhong950419@gmail.com>
Discussion: https://postgr.es/m/19693-2ecd2b52c838b3e5@postgresql.org
Backpatch-through: 17
The RI fast path checks a foreign key by probing the referenced
unique index directly, using the equality operator recorded for
the constraint. Whether the fast path can be used is decided once
and cached in RI_ConstraintInfo, and that cache is invalidated on
pg_constraint changes but not on pg_amop. So after an ALTER
OPERATOR FAMILY drops the recorded operator and adds another in its
place, the cached decision is stale, and the next fast-path check
probes the index with an operator no longer in the opfamily and
errors out with "operator XXX is not a member of opfamily XXX".
The SPI path is unaffected, because the planner just stops matching
the index.
To fix, register an AMOPOPID syscache callback to flush the RI
cache on pg_amop changes, and have ri_check_fastpath_index()
recheck that the recorded operator is still the equality member of
the index's opfamily, falling back to SPI when it is not.
Add regression test coverage.
Reported-by: Nikolay Samokhvalov <nik@postgres.ai>
Author: Nikolay Samokhvalov <nik@postgres.ai>
Discussion: https://www.postgr.es/m/CAM527d9PzFzagr67N0%3DEx2ng1p5HzrcAszy3j5OoZKHXQMARXA%40mail.gmail.com
Backpatch-through: 19
The fast path required table-level SELECT on the referenced table,
rejecting checks that the SPI path allows with column-level grants.
It also omitted the UPDATE privilege required by FOR KEY SHARE.
When table privileges do not suffice, use ExecCheckOneRelPerms() with
the referenced key columns as selectedCols and an empty updatedCols.
This accepts SELECT on all referenced columns and UPDATE on any column,
the same privileges the executor would require for the SELECT ... FOR
KEY SHARE the SPI path runs. Keep the table-privilege check as a
shortcut that avoids constructing a column bitmap in the usual case.
Add missing regression test coverage for the fixed cases.
Reported-by: Nikolay Samokhvalov <nik@postgres.ai>
Author: Nikolay Samokhvalov <nik@postgres.ai>
Co-authored-by: Amit Langote <amitlangote09@gmail.com>
Discussion: https://www.postgr.es/m/CAM527d9BgPjeOOYmbCBTd57R145qHCk-dzw9qNq%2BnOrDq1j__A%40mail.gmail.com
Backpatch-through: 19
Now that pg_upgrade will rewrite multixid members, starting from
oldestMulti, it's important that oldestMulti is valid. Add a sanity
check that oldestMulti is not newer than the oldest datminmxid value
in pg_database.
One case where this could happen is if the cluster was previously
upgraded to version 9.3 with a buggy pg_upgrade version that didn't
have commit a61daa14d5. This new pg_upgrade check is similar to the
defence that was added in commit 78db307bb2 to VACUUM to avoid
truncating away multixids if oldestMulti is too new. This pg_upgrade
check differs in that we don't try to soldier on with the upgrade if
the oldestMultiXID is inconsistent, but rather just abort the upgrade.
Reported-by: Noah Misch <noah@leadboat.com>
Discussion: https://www.postgresql.org/message-id/20260827231757.78.noahmisch@microsoft.com
Backpatch-through: 19
pgoutput records the toplevel XID in the relation's streamed_txns
list, allocated in CacheMemoryContext, when it sends a relation's
schema during a streamed transaction. We clean up that list at stream
commit and stream abort, but missed stream prepare, so the XIDs stay
around until a relcache invalidation for the relation or until the
walsender exits.
Fix by cleaning up the cache after writing the stream prepare message,
marking the schema as sent as we do at stream commit. The subscriber
applies the spooled relation messages at PREPARE time and its relation
map isn't rolled back by ROLLBACK PREPARED, so we don't need to send
the schema again even if the prepared transaction eventually aborts.
Oversight in commit 63cf61cdeb.
Author: Ayush Tiwari <ayushtiwari.slg01@gmail.com>
Reviewed-by: Hayato Kuroda <kuroda.hayato@fujitsu.com>
Reviewed-by: Masahiko Sawada <sawada.mshk@gmail.com>
Discussion: https://postgr.es/m/CAJTYsWWt5aaKHCnV=jNOwABzspd-WbnhWLndDqtESDQRQ-BkKQ@mail.gmail.com
Backpatch-through: 15
We unintentionally copied the original query's limitOption field while
building the modified subquery. If it was LIMIT_OPTION_WITH_TIES,
that behavior would propagate through to execution, potentially
causing a "more than one row returned by a subquery used as an
expression" error. Fix by resetting to the default LIMIT_OPTION_COUNT.
This bug dates back to the introduction of WITH TIES.
Bug: #19694
Reported-by: Chaitanya Choudhary <chaitanyyachoudhary@gmail.com>
Author: Chaitanya Choudhary <chaitanyyachoudhary@gmail.com>
Reviewed-by: Tom Lane <tgl@sss.pgh.pa.us>
Discussion: https://postgr.es/m/19694-4e61bd0475398eef@postgresql.org
Backpatch-through: 14
Commits 2bc7e886fc and 90eae926ab taught ON CONFLICT to include
indexes matching an already selected arbiter, so that an index left
behind by REINDEX CONCURRENTLY continues to arbitrate together with its
replacement. Both checks were too permissive:
a) infer_arbiter_indexes() compared a candidate with a named
constraint's index using only attributes, expressions and predicate, but
ignored collation, NULLS NOT DISTINCT setting or deferrability. As a
result, an index with a difference in these settings could be accepted
even though it did not identify the same conflicts. Also, a deferrable
index that otherwise matches an arbiter index would also cause ON
CONFLICT to fail with "ON CONFLICT does not support deferrable unique
constraints/exclusion constraints as arbiters".
b) ExecInitPartitionInfo() also failed due to the failure to compare
deferrability, so partition-local deferrable indexes would be considered
and break inserts routed to that partition with the error mentioned
above.
Fix by making IsIndexCompatibleAsArbiter() compare those properties, and
changing infer_arbiter_indexes() to use that routine instead of open
coding equivalent logic. Also handle the named-constraint case in
infer_arbiter_indexes() separately instead of passing the constraint
index through the regular clause-inference matching. That function is
not static anymore, so move it to index.c, and also reimplement it to
use the Relation from the indexes only, no longer receiving the
IndexInfo (which wasn't really necessary); also, use equal() to compare
the expression and predicate lists rather than list_difference().
Add tests for ON CONFLICT ON CONSTRAINT with deferrable, NULLS NOT
DISTINCT, and different-collation sibling indexes, preserving the
behavior of released pre-19 versions. Also test routed inserts with a
partition-local deferrable unique constraint.
Author: Zsolt Parragi <zsolt.parragi@percona.com>
Author: Mihail Nikalayeu <mihailnikalayeu@gmail.com>
Reported-by: Zsolt Parragi <zsolt.parragi@percona.com>
Reviewed-by: Michael Paquier <michael@paquier.xyz>
Backpatch-through: 19
Discussion: https://postgr.es/m/CAN4CZFPEYXeYFTxHpoPujfVFb+1Tx1jnXVboDMBg-ZhpgpQ-_g@mail.gmail.com
SHMEM_ATTACH_UNKNOWN_SIZE can be passed as argument to
ShmemRequestStruct() when the caller wants to attach to an existing
shared memory structure whose size it doesn't know. If the shared
memory structure doesn't exist, the request should fail, but instead,
ProcessShmemRequestsAfterStartup() tried to create the structure with
size = -1. That led to integer overflow in ShmemAllocRaw() and memory
corruption.
Fix by rejecting requests with SHMEM_ATTACH_UNKNOWN_SIZE when the
structure doesn't exist. Also add an integer overflow check in
ShmemAllocRaw(), to protect from this kind of confusion or simply too
large requests. Also document SHMEM_ATTACH_UNKNOWN_SIZE.
Author: Ashutosh Bapat <ashutosh.bapat.oss@gmail.com>
Discussion: https://www.postgresql.org/message-id/CAExHW5u_fTsOAS85kG981Vu6eR1GV-344rup6zYew7xMjEDREw@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
The reference page said WAIT "cannot be used in contexts where such a
snapshot must remain active, including transactions running at isolation
levels higher than READ COMMITTED", which reads as a blanket ban at those
levels. It is not one. The rule the code enforces is that the current
transaction must hold no snapshot, and a transaction-snapshot-mode
transaction has no snapshot until its first query: WaitStmt is exempt in
PlannedStmtRequiresSnapshot(), so the command itself does not take one.
WAIT therefore works before that first query, which is the order an
application wants anyway, and it is what 447aae13b0 set out to allow by
keeping the wait snapshot-free.
State the rule in terms of the snapshot rather than the isolation level,
say that WAIT acquires none of its own and name the other ways to hold
one, and give the consequence of holding one: it can delay replay, which
standby_replay waits for and which the other standby modes ultimately
depend on as well.
The error for a held snapshot named the internal snapshot states instead
of saying what the user did, and attached the isolation-level
explanation as DETAIL unconditionally, so a READ COMMITTED session with a
cursor was told its isolation level was too high. Report what is held
rather than how it is held, and emit the DETAIL only when the isolation
level is in fact the reason.
Reported-by: Sami Imseih <samimseih.pg@gmail.com>
Reported-by: Xuneng Zhou <xunengzhou@gmail.com>
Reviewed-by: Sami Imseih <samimseih.pg@gmail.com>
Reviewed-by: Xuneng Zhou <xunengzhou@gmail.com>
Discussion: https://postgr.es/m/CAN12%2BYJddgAZzA36JZFX%2BYrnNR7Uubp9qWFz6ASs3v0kL30AMA%40mail.gmail.com
Backpatch-through: 19
Commit a49b9cfd72 began reporting the EXCEPT clause relation with a
schema-qualified name that quotes identifiers when needed, inside a
message that already puts the name in quotes. So a schema or relation name
that needs quoting came out quoted twice.
Fix by building the qualified name as "%s.%s" without identifier quoting,
so it renders like the qualified names in other backend error messages.
The message text, and so its translations, is unchanged.
RelationGetQualifiedRelationName() has no callers left, so remove it.
Author: Manuel Reyes Bravo <manuelreyesbravo@gmail.com>
Reviewed-by: shveta malik <shveta.malik@gmail.com>
Reviewed-by: Vignesh C <vignesh21@gmail.com>
Reviewed-by: Peter Smith <smithpb2250@gmail.com>
Reviewed-by: Zhijie Hou <houzj.fnst@fujitsu.com>
Reviewed-by: Amit Kapila <amit.kapila16@gmail.com>
Discussion: https://postgr.es/m/CAHGQGwHfESBexa7fq99EvFCf31av=O9h9udnw22ymxmm6LMZzw@mail.gmail.com
Backpatch-through: 19, where it was introduced
