Commit 1ff3180ca0 made a parallel autovacuum leader propagate changes
to the cost-based delay parameters to its workers only from
vacuum_delay_point(), which it no longer reaches once it is waiting in
WaitForParallelWorkersToFinish(). While waiting, the leader neither
processed a config reload nor noticed a change in the number of
autovacuum workers sharing the cost limit, so its workers kept running
with stale parameters until they finished.
Fix by refreshing and propagating the parameters on every wakeup of
that wait loop, and by setting the latches of the balanced autovacuum
workers when their count changes so that a waiting leader wakes up.
A regression test will be added in a separate commit.
Backpatch to v19, where parallel autovacuum was introduced.
Reported-by: Nikolay Samokhvalov <nik@postgres.ai>
Author: Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com>
Reviewed-by: Manu <manuelreyesbravo@gmail.com>
Reviewed-by: Zsolt Parragi <zsolt.parragi@percona.com>
Reviewed-by: Daniel Gustafsson <daniel@yesql.se>
Reviewed-by: Masahiko Sawada <sawada.mshk@gmail.com>
Discussion: https://postgr.es/m/CAM527d-GL=Jp2EJXBSnVBGPK-4XEZwWof5Cv8P0hghS_og6oAg@mail.gmail.com
Backpatch-through: 19
Commit 7164dcb665 added the repack_decode isolation test, whose
s1_decode step counts every change decoded from the slot. A concurrent
autovacuum transaction that modifies only system catalogs is decoded
as an empty BEGIN/COMMIT pair, so the count could come out larger
than expected.
Fix by passing skip-empty-xacts to test_decoding. We pass it, along
with include-xids=0, to s1_decode_updates as well so that both steps
use the same options for consistency, though that step shows only
UPDATE lines and wasn't affected.
Per buildfarm animal skink.
Reported-by: Alexander Lakhin <exclusion@gmail.com>
Discussion: https://postgr.es/m/d704cde4-b1f2-48dc-8c5e-659d1128d286@gmail.com
Backpatch-through: 19
There have been several recent complaints that << and >> don't behave
the same way on all supported platforms. There is a risk that <<
overflows the signed integer type, which is undefined behavior, and there
are platform-dependent behaviors from shifting by numbers of bits >= the
bitwidth of the type, and also from shifting by negative values.
Document this to warn unsuspecting users of these facts.
Author: David Rowley <dgrowleyml@gmail.com>
Reviewed-by: Tom Lane <tgl@sss.pgh.pa.us>
Discussion: https://postgr.es/m/CAApHDvr0O6ZF8NO=Rq9Q+Sz=FHT=NZ1kkNTXZDcNph-OWuk7TQ@mail.gmail.com
Backpatch-through: 14
Canada's Northwest Territories will move to permanent UTC-06 on
2026-11-01. For now, assume their TZ abbreviation will be CST from
that time forward. This affects only America/Inuvik as the rest of
the territory is covered by America/Edmonton, for which the
equivalent change was released in 2026c.
Manitoba (America/Winnipeg) will move to permanent UTC-05 on
2026-10-31. For now, assume their TZ abbreviation will be EST
from that time forward.
(Upstream notes that more changes may be needed soon if smaller
jurisdictions choose to break away from what these have done,
but for now the plan is wait-and-see.)
Historical corrections for Colombia, Iran, and Ireland.
The backward-compatibility zone names EST5EDT, CST6CDT, MST7MDT, and
PST8PDT now conform better to POSIX. For example, EST5EDT now always
uses the abbreviation "EST" for standard time (now always 5 hours
behind UTC) and "EDT" for daylight saving time, whereas it formerly
had different UT offsets before standard time was introduced and
sometimes used abbreviations like "LMT", "EWT" and "EPT", all contrary
to POSIX. Also, though not specified by POSIX these names now use US
federal rules rather than rules of specific places like New York,
reverting to 2024a behavior. This change affects only timestamps
before 1966-10-30 at 01:00 standard time.
Backpatch-through: 14
Commit 0785d1b8b wrote
jsonapi_shlib.o: override CPPFLAGS += -DJSONAPI_USE_PQEXPBUFFER
forgetting that GNU make treats this change to CPPFLAGS as affecting
not only jsonapi_shlib.o, but anything that gets built in pursuit of
building jsonapi_shlib.o. Because of an even more ancient hack
(cf 7143b3e82), that will include jsonapi.o if it's not been built
already. If we do apply this -D switch to jsonapi.o, it gets built
with the wrong expansible-string infrastructure, which manifests
immediately as a link failure in pg_combinebackup and would misbehave
at runtime even if the build somehow completed.
Perhaps remarkably, we'd not seen field reports of build failures
till now, but it's pretty easy to reproduce a mis-build by trying
to build src/common/ with --shuffle and a high -j factor.
To fix, don't modify CPPFLAGS this way; instead make a dedicated
build rule for jsonapi_shlib.o that injects the additional switches.
There might be other places with similar problems, but some desultory
attempts with -j32 --shuffle on a 32-thread CPU didn't find any more
build failures. Also, it looks like the meson build infrastructure
lacks this problem (but I'm no meson expert).
Bug: #19727
Reported-by: Ross Burton <ross@burtonini.com>
Author: Tom Lane <tgl@sss.pgh.pa.us>
Reviewed-by: Ross Burton <ross@burtonini.com>
Discussion: https://postgr.es/m/19727-32d67d9dea410f38@postgresql.org
Backpatch-through: 18
The explanation of aggressive scans claimed that if the multixact
members grows to 2 billion entries, we launch autovacuum even if it's
nominally disabled. That's no longer true; anti-wraparound vacuum is
only triggered if we're in danger of running out of multixids, we no
longer launch it just to shrink the 'members' SLRU. There's no
wraparound danger with 'members' anymore, so it's no worse than if a
user table becomes very bloated.
We do still adjust the freeze threshold though, so that if autovacuum
runs, whether on schedule or as an emergency autovacuum to avoid
wraparound, it will freeze more aggressively if the members SLRU is
large.
I removed the mention of the 4 billion member threshold, even though
that still exists, because the explanation felt misleading. The
aggressiveness is scaled proportionally between 2 and 4 billion
members. At 4 billion it's as aggressive as it gets, but there's no
sudden cliff.
In the passing, fix some comments. Remove obsolete mention of safe
threshold for member storage triggering autovacuum. Secondly, the
freezing cutoff is adjusted to avoid the *members* SLRU from growing
too large.
Reported-by: Noah Misch <noah@leadboat.com>
Discussion: https://www.postgresql.org/message-id/20260827231757.78.noahmisch@microsoft.com
Backpatch-through: 19
ShmemRequestInternal() rejected SHMEM_ATTACH_UNKNOWN_SIZE whenever
IsUnderPostmaster was false. That also rejected an after-startup
attachment in a standalone backend, even if the area exists.
To fix, check whether the shared memory has been initialized
(ShmemIndex == NULL) rather than IsUnderPostmaster.
Refactor the tests, and add more test cases for after-startup
attachment, with SHMEM_ATTACH_UNKNOWN_SIZE and a mismatching size, and
for this single-user case.
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
ReplicationSlotRelease() copied MyProc->statusFlags unconditionally
when resetting PROC_IN_LOGICAL_DECODING, using its pgxactoff to decide
which area to update.
Slot invalidations can be done by the checkpointer or the startup
process, and ReplicationSlotRelease() ignored the fact that pgxactoff is
only meaningful for processes in the proc array, that auxiliary
processes never join. pgxactoff would remain at 0 for these, meaning
that an incorrect set of status flags would be updated.
The consequences of this issue could be various, depending on the
statsFlags lost in the proc slot incorrectly overwritten. Losing
PROC_AFFECTS_ALL_HORIZONS has been mentioned as the worse case, causing
VACUUM to remove rows a standby may needs even if standby feedback is
enabled. Assertions failures are another possibility.
Let's skip the update of statusFlags unless PROC_IN_LOGICAL_DECODING is
set. Nothing but StartupDecodingContext() sets it, and no auxiliary
processes call it.
Author: Vlad Lesin <vladlesin@gmail.com>
Reviewed-by: Hayato Kuroda <kuroda.hayato@fujitsu.com>
Reviewed-by: Michael Paquier <michael@paquier.xyz>
Discussion: https://postgr.es/m/889a06dd-45fc-423b-9dd2-87d5b5dc60c6@gmail.com
Backpatch-through: 14
SJE didn't consider the possibility that two RTEs referencing the
same table have different securityQuals, and might choose to merge
them anyway, thereby possibly losing quals that need to be enforced.
This is a regression introduced by 2ebf25e7d, since before that we
didn't re-generate baserestrictinfo lists from the RTEs' securityQuals
after performing SJE.
To defend against this, only consider SJE between RTEs with the same
checkAsUser values. That solves the problem because the set of
applicable RLS policies depends only on the role that is considered
to be accessing the table, so that the securityQuals must be equal
if the checkAsUser values are. It might also keep us from creating
similar bugs if we ever invent other features that depend on the
accessing role. And it's a lot cheaper than comparing securityQuals
trees themselves would be, not least because we can't sort them so
the preliminary sort step wouldn't help.
(The first proposed solution was to not perform SJE at all on RTEs
with nonempty securityQuals, but that seems rather sad, especially
since such cases worked before 2ebf25e7d. Making the restriction
depend on checkAsUser seems much less likely to interfere with SJE
unnecessarily, since in most cases that'll be the same for all
potentially-mergeable RTEs.)
The added test cases show that SJE is rejected when necessary, and
also demonstrate two quirks of this implementation. One is that if
a removed security qual includes an InitPlan, we'll still attach the
now-unused InitPlan to the plan. That's because SS_process_sublinks
runs before self-join elimination, so the extra InitPlan has already
been made. The other quirk is that we won't merge RTEs having zero
and nonzero checkAsUser fields, even if the calling user matches the
nonzero checkAsUser value so that there is no difference for this
user. That's intentional so that SJE doesn't require having to mark
the plan as caller-dependent.
Reported-by: Yonghwa Lee <underdog@theori.io>
Author: Tom Lane <tgl@sss.pgh.pa.us>
Reviewed-by: Ayush Tiwari <ayushtiwari.slg01@gmail.com>
Discussion: https://postgr.es/m/20260926174457.13.noahmisch@microsoft.com
Backpatch-through: 18
The help text for -c option documents that zero for either transaction
ID means to leave the corresponding value unchanged. Commit
978cf02bb8 replaced the separate checks for the two values with a
single flag indicating that the option was specified, causing zero to
be written to the control file instead.
Also parse the second transaction ID with strtouint32_strict(), like
the first one. Using strtoul() there was an oversight in commit
636c1914b4, which intended to reject negative and out-of-range
arguments.
Author: Chao Li <lic@highgo.com>
Discussion: https://www.postgresql.org/message-id/F61E486B-746B-42F3-A532-B8D4F1519583@gmail.com
Backpatch-through: 19
calcSumX2 is used to inform the aggregate transition function whether it
should include the square of the transition value in the aggregate
state. The deserialize functions for various numeric aggregates that do
use calcSumX2 weren't properly setting the property to true. This does
not matter in core as we never perform any aggregate transitions after a
deserialize operation.
This fix is only required for external code which plumbs aggregate paths
together in such a way that could result in aggregate transitions being
performed after states have been deserialized.
Author: Maxime Schoemans <maxime.schoemans@enterprisedb.com>
Reviewed-by: Andrey Borodin <x4mmm@yandex-team.ru>
Discussion: https://postgr.es/m/FEBF5C3D-CC53-455F-AA9D-4D377A2E957C@enterprisedb.com
Backpatch-through: 14
If no DSM segment can be created, a ParallelContext can still be
created with zero workers, but its seg pointer is NULL. Parallel
autovacuum registered a segment detach callback on that NULL pointer,
crashing the autovacuum worker.
Fix by setting up the shared cost parameters and the detach callback
only when the parallel context has workers, as there are no workers to
propagate the parameters to otherwise.
Oversight in commit 1ff3180ca0.
Found by Bharath using AI assisted review with Claude.
Reported-by: Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com>
Author: Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com>
Discussion: https://postgr.es/m/CALj2ACVjBN03Fq4XWgKH1zOfkapK6-P3ufNLHUHnZcW1zPm+Mg@mail.gmail.com
Backpatch-through: 19
Presently, s_lock.h insists that every platform provide TAS(), and
its header comment doesn't say which of the macros a new platform
actually needs to supply. The real contract is that a platform
must provide either S_LOCK() or a TAS() for the default S_LOCK() to
be built on.
To better express that, this commit moves the "no spinlock support"
error to where the default S_LOCK() is defined, compiles s_lock()
only when the default S_LOCK() is in use, and provides a default
TAS_SPIN() only when there is a TAS() to base it on. The direct
callers of s_lock() in the spinlock tests are adjusted to match.
The header comment is reworked accordingly, dropping the note about
pre-9.5 volatile requirements and the caution about TAS()
spuriously failing along the way.
Co-authored-by: Tom Lane <tgl@sss.pgh.pa.us>
Discussion: https://postgr.es/m/afkUeI7UhacZ5ZFm%40nathan
create_partial_grouping_paths() called the FDW callback
GetForeignUpperPaths for UPPERREL_PARTIAL_GROUP_AGG, but never the
general-purpose create_upper_paths_hook that non-FDW extensions rely
on. create_partial_distinct_paths() already fires both for
UPPERREL_PARTIAL_DISTINCT.
Add the missing call right next to GetForeignUpperPaths, so an
extension can add partial-aggregate paths here too, before the caller
gathers them and builds a Finalize Aggregate on top.
Reviewed-by: Bryan Green <dbryan.green@gmail.com>
Reviewed-by: Himanshu Upadhyaya <upadhyaya.himanshu@gmail.com>
Reviewed-by: Shihao Zhong <zhong950419@gmail.com>
Discussion: https://postgr.es/m/6fc72a7d-9f52-4a72-9adf-1912da255605@dunslane.net
Switch to wait_for_replay_catchup in checksum tests to explicitly
wait for the upstream flush LSN and not the write LSN. The write
LSN can include unrelated unflushed WAL which isn't available for
streaming on an idle primary.
In the test for using a rewound node as standby of a new primary in
the 013_rewind suite; also switch WAL on the source before starting
the target. This flushes through the minimum recovery point chosen
to avoid a startup delay while an unflushed full_page_writes change
waits for the background writer to log its next snapshot.
This reduces test times significantly without changing the semantics
of the tests.
Author: Nazir Bilal Yavuz <byavuz81@gmail.com>
Reviewed-by: Zsolt Parragi <zsolt.parragi@percona.com>
Reviewed-by: Daniel Gustafsson <daniel@yesql.se>
Discussion: https://postgr.es/m/CAN55FZ1Yak_xBqMaDQsD7atpBkGLEkF-DKXcs3nLHM1Uq4YRew%40mail.gmail.com
The decoding worker records the relfilenumber of the TOAST table when it
starts, and skips changes stored under any other one, but the TOAST
table was not locked until the data copy began. If VACUUM FULL rewrote
it while the worker was starting, concurrent updates of TOASTed columns
were silently lost.
Fix by locking the TOAST table in cluster_rel() in the concurrent case,
before the worker starts.
Author: Shihao Zhong <zhong950419@gmail.com>
Reported-by: Thom Brown <thom@linux.com>
Backpatch-through: 19
Discussion: https://postgr.es/m/CAA-aLv5MF6BLL+BWvix2Yw+CBardtH43AofPReQunhDZPNBtuA@mail.gmail.com
When searching the local relation by index, the apply worker can use the
first match only if the index is the relation's replica identity or
primary key. Otherwise, every match must be compared with the search slot.
Only the index OID was saved in the relation map, so this was determined
again during the search. Concurrent DROP INDEX or REINDEX can change the
index status between these two checks, causing the tuple search to fail
and the change to be silently skipped as an update_missing conflict.
Fix this by recording the answer as idxisreplident in the relation map
entry and avoid getting it second time in FindReplTupleInLocalRel() and
RelationFindReplTupleByIndex().
Oversight in 89e46da5e5.
Author: Mikhail Nikalayeu <mihailnikalayeu@gmail.com>
Author: vignesh C <vignesh21@gmail.com>
Reviewed-by: Amit Kapila <amit.kapila16@gmail.com>
Reviewed-by: Zhijie Hou <houzj.fnst@fujitsu.com>
Discussion: https://postgr.es/m/CADzfLwUJovFcnknCC9wjZKECX9xecgnGzC2r2TMV8h4QDD_jwQ@mail.gmail.com
Backpatch-through: 16, where it was introduced
Since 112faf1378, NOTICE, WARNING, and similar messages received through
dblink, postgres_fdw, and replication connections have been logged with
ereport(LOG). With the default log_min_error_statement setting, each
such log entry can also include the current local SQL statement. An active
error-context callback can add local CONTEXT information as well. Repeating
these fields for every remote message increases log volume and processing
overhead.
Use errhidestmt() and errhidecontext() when logging these messages to
prevent the local STATEMENT and CONTEXT fields, when present, from being
repeated for each remote message in the server log.
Reported-by: Merlin Moncure <mmoncure@gmail.com>
Author: Vignesh C <vignesh21@gmail.com
Reviewed-by: Merlin Moncure <mmoncure@gmail.com>
Reviewed-by: Fujii Masao <masao.fujii@gmail.com>
Discussion: https://postgr.es/m/CAHyXU0yDwOYkWpRi=gtpDwjBxVqeaezgkgZLRWm2p1_FgaiPFQ@mail.gmail.com
Backpatch-though: 19
The counted_by attribute allows specifying that a flexible array
member of a struct is "counted by" another member of the same struct.
The attribute makes arrays a little more self-documenting, but it is
also a hint to the compiler such that it can improve detection of
object size information and provide better results in compile-time
diagnostics and run-time features, such as the array bounds sanitizer.
Both GCC and clang warn when the attribute is used in C++, where it is
ignored, so define it away there.
In its current form, it is best used for flexible array members only.
Pointer array support may come later on.
This commit only contains a single use of the attribute for testing.
More will be added afterwards.
Author: Tristan Partin <tristan@partin.io>
Reviewed-by: Peter Eisentraut <peter@eisentraut.org>
Reviewed-by: Japin Li <japinli@hotmail.com>
Reviewed-by: Greg Burd <greg@burd.me>
Discussion: https://www.postgresql.org/message-id/flat/DKBEBF32QLUA.3L2DJRPUYNKHZ%40partin.io
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
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
toast_save_datum() ended with two copies of the same three steps, one
each external on-disk TOAST pointer: allocate a datum, set its tag, then
copy the contents from a varatt_external_oid or a varatt_external_oid8
into it.
These steps are moved into a new helper function, toast_pointer_build(),
which takes the tag and a pointer to the struct and derives everything
else from the tag. For now, only two varatt_external_* exist for
external on-disk TOAST pointers. Some more are under discussion, where
this refactoring shows some benefits in the shape of less duplication.
There should be no behavior change.
Author: Nikhil Kumar Veldanda <veldanda.nikhilkumar17@gmail.com>
Discussion: https://postgr.es/m/CAFAfj_FeJQZrKyVn7ur0aJx8iz8SR14NheVJNyc8JNoqtMX+og@mail.gmail.com
_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
Variable-length catalog columns have been declared like
text attoptions[1];
but that "1" has always been a fiction. Before the use of #ifdef
CATALOG_VARLEN, these declarations were visible to the C compiler, and
this was also before flexible array members were universally
available, so this was just a convenient workaround to make this
compile. But these reasons are long gone, and the "1" is now just a
confusing relic. Change this to
text attoptions[];
which more intuitively reflects the actual nature of these
fields (while still being syntactically valid but semantically invalid
C code).
Catalog.pm could already parse both spellings, but no existing code
used bare []. To enforce future consistency, it is changed to no
longer permit digits between the brackets. But to help developers
transition, it still recognizes the old style and gives an explicit
error message. (Otherwise it would produce some garbage output and
give confusing errors later.)
Reviewed-by: Heikki Linnakangas <hlinnaka@iki.fi>
Reviewed-by: Daniel Gustafsson <daniel@yesql.se>
Reviewed-by: Chao Li <li.evan.chao@gmail.com>
Discussion: https://www.postgresql.org/message-id/flat/963b2109-833c-4e93-bdb1-546741d893f4%40eisentraut.org
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
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
Currently, the message numbers in sinvaladt.c are ints, but they
are never negative, and the code already takes pains to keep them
from overflowing. This commit changes them to uint32. The only
wrinkle is that SICleanupQueue() computes two thresholds by
subtracting from maxMsgNum, and those could previously go negative.
They are now clamped at zero, which disables the corresponding
checks just as a negative threshold did.
This is preparatory work for a follow-up commit that will convert
maxMsgNum to an unsigned atomic variable.
Author: Yura Sokolov <y.sokolov@postgrespro.ru>
Reviewed-by: Heikki Linnakangas <hlinnaka@iki.fi>
Reviewed-by: Peter Eisentraut <peter@eisentraut.org>
Reviewed-by: Andres Freund <andres@anarazel.de>
Discussion: https://postgr.es/m/30aa0030-f694-44ef-a19d-6ef7ddb69374%40postgrespro.ru
Discussion: https://postgr.es/m/alAJeRRzehDjLaF1%40nathan
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
Our Solaris-camp buildfarm animals (billbug, hake, pollock, so far)
are complaining about dumpcolors()'s local variables in the wake of
3ee0edb91. This is unsurprising on one level, since that code
clearly violates the new expectation; but AFAICS these animals
aren't explicitly enabling -DREG_DEBUG, so how are they reaching
the now-faulty code? Do Solaris' headers #define that?
In any case, the fix is simple enough, just narrow the
declaration scopes of this function's loop variables.
(I could have done that only to "c", but thought it best
to maintain a consistent style in this function.)
I don't see any need to back-patch this.
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
