This reverts commit ef38a4d975 which
implemented the GROUP BY ALL syntax, as well as 2ce745836 which
made some comment improvements therein.
A postcommit review discovered that GROUP BY ALL missed our special
handling of entries that also appear in an ORDER BY in the query.
This caused the query to return wrong results when ORDER BY
specifies non-default equality semantics. While this should be
fixable with some refactoring, doing it cleanly seems like too
much code churn for late beta. We'll revert and try again in v20.
The reverted commit also included some additional comment wordsmithing
and docs cleanup, which are retained as they weren't connected to the
reverted feature.
catversion bump needed due to change in struct Query.
Reported-by: Chao Li <li.evan.chao@gmail.com>
Author: Daniel Gustafsson <daniel@yesql.se>
Reviewed-by: Tom Lane <tgl@sss.pgh.pa.us>
Discussion: https://postgr.es/m/5243308F-8E5C-45AA-828C-FAD96C4F34DA@gmail.com
Backpatch-through: 19
An index-only scan filled its result slot from the HeapTuple an index AM
returns in scan->xs_hitup by deforming it with the virtual slot's own
tuple descriptor (during GiST and SP-GiST index-only scans). But index
AMs form that heap tuple using their own descriptor, scan->xs_hitupdesc.
The AM's descriptor may disagree with the IoS virtual slot's descriptor
about each column's precise alignment, leading to "can't happen" errors
in certain rare edge cases. Hard crashes were possible but much less
likely.
To fix, deform the tuple with the descriptor it was formed with. This
is simpler, and makes xs_hitup handling (used by GiST and SP-GiST)
uniform with the nearby existing xs_itup handling (used by nbtree).
In practice this issue was very unlikely to be hit (it was found during
testing of a patch that will change the table AM API used during index
scans). The only currently affected core opclass is GiST's range_ops.
It was only possible for the datum to be accessed at an incorrectly
aligned offset when reading the second or subsequent column from a
multicolumn GiST index. This couldn't happen in the common case where
the datum used an unaligned short varlena header. Moreover, an earlier
column had to leave the range datum at an offset where the two
alignments actually disagree (e.g., an odd-length varlena datum).
Author: Peter Geoghegan <pg@bowt.ie>
Reviewed-by: Tomas Vondra <tomas@vondra.me>
Discussion: https://postgr.es/m/CAH2-WzkGXa2SKnebdW29RT1hCcQBo_p03v3iqif2u9bjzLB-aQ@mail.gmail.com
Backpatch-through: 14
Commit e1d917182 added support for base64url encoding, a base64 variant
intended to be safe for usage in URLs and filenames. The padding rules
for base64url and base64 differ in that base64url require no extra '='
padding, but the commit unintentionally relaxed this requirement for
base64 as well. Fix by making sure that the truncation logic check
for the encoding and add a test to make sure.
Backpatch down to v19 where support for base64url was introduced.
Author: Daniel Gustafsson <daniel@yesql.se>
Reviewed-by: David E. Wheeler <david@justatheory.com>
Discussion: https://postgr.es/m/3258FC72-F5E1-40B9-B5D7-64478CAF7728@yesql.se
Backpatch-through: 19
The data checksums launcher and workers were exposed in pg_stat_io
with the same broad set of object/context combinations as general
background workers. However, several of those entries can never
accumulate I/O statistics for these processes, such as bulkwrite,
relation init, temporary relation, and launcher vacuum entries.
Teach pgstat_tracks_io_object() and pgstat_tracks_io_op() about the
actual I/O performed by the data checksum processes. Keep the entries
needed for catalog scans, including bulkread catalog scans, worker
relation processing with a vacuum access strategy, and WAL writes and
initialization, while excluding WAL reads and other object/context
combinations that can never be used.
Author: Fujii Masao <masao.fujii@gmail.com>
Reviewed-by: Daniel Gustafsson <daniel@yesql.se>
Discussion: https://postgr.es/m/CAHGQGwHz_-nt+YkHDMRZNBZrnoHro8cMOgSwuXEmSYT6vxgQ=w@mail.gmail.com
Backpatch-through: 19
The previous text claimed that once the slot is disassociated with
ALTER SUBSCRIPTION ... SET (slot_name = NONE), DROP SUBSCRIPTION "will no
longer attempt any actions on a remote host". That is inaccurate:
DROP SUBSCRIPTION may still connect to the publisher to drop
internally-created table synchronization slots when some table
synchronization is left unfinished. Reword to describe this, and note
that if the publisher is unreachable those slots (and the main slot, if
it still exists) must be dropped manually to avoid indefinitely reserving
WAL.
Reported-by: Jeff Davis <pgsql@j-davis.com>
Author: Amit Kapila <amit.kapila16@gmail.com>
Backpatch-through: 14
Discussion: https://postgr.es/m/CAA4eK1+tyYSpPxMBy1974kjivuGeR7YY=yopwRGrK3+vCTysdg@mail.gmail.com
Discussion: https://postgr.es/m/D908370F-2695-4231-851D-17179A6A6F2A@gmail.com
Commit c7aeb775 rewrote the HOT-chain offset sanity checks in three
places, but in heap_get_root_tuples it accidentally tested offnum -- the
outer loop variable, which is already bounded by the loop condition --
instead of nextoffnum, the offset actually passed to PageGetItemId. The
pre-c7aeb775 check tested nextoffnum.
With the check ineffective, a stale t_ctid could make PageGetItemId read
past the end of the line pointer array (which is data corruption that we
expect to be able to catch here).
Author: Peter Geoghegan <pg@bowt.ie>
Reported-by: Konstantin Knizhnik <knizhnik@garret.ru>
Discussion: https://postgr.es/m/87c7d8a4-3a82-4334-bee6-e8c2ad3f3293@garret.ru
Backpatch-through: 15
Crash recovery running with wal_level='minimal' can replay an
XLOG_LOGICAL_DECODING_STATUS_CHANGE record that activates logical
decoding, if the server previously ran with a higher wal_level and
crashed after the last logical slot was dropped but before the
checkpointer deactivated logical decoding. Replaying such a record is
correct since it reflects the status at the time it was
written. However, UpdateLogicalDecodingStatusEndOfRecovery() asserted
that logical decoding is never active with wal_level='minimal',
causing an assertion failure at the end of recovery. In production
builds, logical decoding would remain active while running with
wal_level='minimal'.
Instead of special-casing wal_level='minimal', recompute the status at
the end of recovery as usual: no logical slot can exist with
wal_level='minimal' as RestoreSlotFromDisk() would have rejected it,
so the recomputation always deactivates logical decoding in this case,
also writing the corresponding status change record.
Oversight in 67c20979ce.
Reviewed-by: Guoqing Yang <yanggq1988@126.com>
Reviewed-by: Hayato Kuroda <kuroda.hayato@fujitsu.com>
Discussion: https://postgr.es/m/CAD21AoAnPAugUnDic+ESvrfXjXHk2bss9eHAD7zP0-Chy2UabA@mail.gmail.com
Backpatch-through: 19
uuidv7(interval) shifts the current time by the given interval before
encoding it into the 48-bit Unix-millisecond timestamp field of the
generated UUID. Two cases were mishandled:
An infinite interval ('infinity' or '-infinity') produced an infinite
timestamp, which overflowed during the conversion to Unix-epoch
microseconds and yielded a garbage UUID. Reject infinite intervals up
front, before any timestamp arithmetic.
A shift that moved the timestamp outside the range representable by
the 48-bit field was silently accepted. Timestamps before the Unix
epoch wrapped when cast to unsigned, and timestamps beyond
approximately year 10889 overflowed the field; both produced UUIDs
with bogus timestamps that break sort ordering. Reject any shifted
timestamp outside the supported range.
Also document that infinite intervals and out-of-range shifts are
rejected.
Although raising a new error changes behavior in a stable branch, this
is back-patched to 18 (where uuidv7(interval) was introduced) because
the previous behavior can silently corrupt data. Failing loudly is far
safer than silently accepting the wraparound; otherwise users may not
discover that their UUIDv7 values are no longer sortable until years
later, when recovery is painful. It also matches how PostgreSQL
already handles timestamp + interval overflow, which raises an
error. The change only affects applications passing an interval large
enough to push the result outside the representable range.
Backpatch to 18, where uuidv7(interval) was introduced.
Reported-by: Christophe Pettus <xof@thebuild.com>
Author: Baji Shaik <baji.pgdev@gmail.com>
Reviewed-by: Masahiko Sawada <sawada.mshk@gmail.com>
Reviewed-by: Zsolt Parragi <zsolt.parragi@percona.com>
Reviewed-by: Tristan Partin <tristan@partin.io>
Reviewed-by: Kyotaro Horiguchi <horikyota.ntt@gmail.com>
Discussion: https://www.postgresql.org/message-id/799A70FA-6E5C-4118-99EB-2FBBE1CBAC54@thebuild.com
Backpatch-through: 18
Commit d4a657b0a4 added a call to has_sequence_privilege() while
fetching sequence information from the publisher, so that
publisher-side permission failures could be distinguished from missing
sequences. It also assumed that has_sequence_privilege() could never
return NULL, and asserted accordingly.
However, that assumption was incorrect. If a sequence is dropped after
the synchronization worker collects its metadata but while fetching the
sequence information, has_sequence_privilege() can return NULL.
This can trigger the assertion failure. This was also reported in
a buildfarm failure on member culicidae.
Fix this by treating a NULL result from has_sequence_privilege() as
indicating that the sequence was dropped concurrently, and report it as
a missing sequence instead of asserting that the result is never NULL.
Reported-by: Noah Misch <noah@leadboat.com>
Author: Vignesh C <vignesh21@gmail.com>
Reviewed-by: Hayato Kuroda <kuroda.hayato@fujitsu.com>
Reviewed-by: Zsolt Parragi <zsolt.parragi@percona.com>
Reviewed-by: Fujii Masao <masao.fujii@gmail.com>
Discussion: https://postgr.es/m/20260710045217.f0.noahmisch@microsoft.com
Discussion: https://postgr.es/m/CALDaNm2fHGLeiQKj0r6OG7N9QeayxSmpLrWYJRyt4dL_m3VRWw@mail.gmail.com
Backpatch-through: 19
The regression test for postgres_fdw_get_connections(true) assumed that
a terminated remote connection would still remain visible in the FDW
connection cache long enough to be reported as closed with a nonzero
remote_backend_pid.
That assumption is not always valid. postgres_fdw_get_connections()
reports only entries that are still present in ConnectionHash, while
pgfdw_inval_callback() may immediately discard an idle cached connection
(xact_depth == 0) when a relevant invalidation arrives. In CI, that can
happen between terminating the remote backend and querying
postgres_fdw_get_connections(true), causing the function to return no
rows.
Adjust the idle-connection test to accept either outcome: if the cache
entry is still present, verify that it reports the expected server name,
closed status, and nonzero remote backend PID; otherwise treat zero rows
as a legitimate result.
To preserve coverage of the terminated-backend reporting path, add a
separate check inside an explicit transaction. In that case, concurrent
invalidation may mark the connection invalid but cannot discard it
before transaction end, so postgres_fdw_get_connections(true) should
still report the terminated connection as in-use, closed, and associated
with a nonzero remote backend PID.
Backpatch to v18, where the affected postgres_fdw_get_connections(true)
test was introduced.
Reported-by: Robert Haas <robertmhaas@gmail.com>
Author: Fujii Masao <masao.fujii@gmail.com>
Reviewed-by: Robert Haas <robertmhaas@gmail.com>
Reviewed-by: Daniel Gustafsson <daniel@yesql.se>
Discussion: https://postgr.es/m/CA+Tgmoax3cHXHsm9OidN4F-xiu16y8q2W8T5dTNFic1Zoo2cOw@mail.gmail.com
Backpatch-through: 18
Commit 67846550dc removed the xreflabels for initdb options, which
turned the sentence "The second field contains the page checksum if
data checksums are enabled" into "The second field contains the page
checksum if -k are enabled", as well "Only has effect if data checksums
are enabled" into "Only has effect if -k are enabled".
Fix by setting an explicit link text, and while there also change the
link to point to the data checksum page which has more information
than just the initdb option.
The original report was for one instance, further inspection turned
up quite a few more cases. Also redirect the link in the amcheck
docs which albeit was reading right, but will be more helpful if
linking to the main page on data checksums. Backpatch to v18 where
the xreflabels were removed.
Author: Daniel Gustafsson <daniel@yesql.se>
Reported-by: y.saburov@gmail.com
Reviewed-by: Laurenz Albe <laurenz.albe@cybertec.at>
Discussion: https://postgr.es/m/178350739237.73862.4549076173872335741@wrigleys.postgresql.org
Backpatch-through: 18
Previously, libpqrcv_create_slot() checked only that
CREATE_REPLICATION_SLOT returned PGRES_TUPLES_OK before reading
values from the first row. If the server unexpectedly returned an
invalid result, such as zero rows, PQgetvalue() could return NULL,
leading to a crash while parsing the LSN.
Other replication commands, such as IDENTIFY_SYSTEM, already validate
the response shape before accessing result values, but
CREATE_REPLICATION_SLOT did not.
Fix this by verifying that CREATE_REPLICATION_SLOT response contains
exactly one row with four fields, and report a protocol violation otherwise.
Backpatch to all supported versions.
Bug: #19547
Reported-by: Yuelin Wang <1217816127@qq.com>
Author: Kenny Chen <kennychen851228@gmail.com>
Reviewed-by: Hayato Kuroda <kuroda.hayato@fujitsu.com>
Reviewed-by: Fujii Masao <masao.fujii@gmail.com>
Discussion: https://postgr.es/m/19547-f7986f668f71e788@postgresql.org
Discussion: https://postgr.es/m/CAPXstDtW2iqe+DJAOTQTX+rRziJp2UhZSo1+HRj1COAtbu+nKw@mail.gmail.com
Backpatch-through: 14
The documentation for log_parameter_max_length said it affects messages
generated by log_duration. However, log_duration alone does not log bind
parameter values, so this is misleading.
This commit updates the documentation to reference log_min_duration_statement,
which can log bind parameters, to better reflect actual behavior.
Backpatch to all supported versions.
Author: Fujii Masao <masao.fujii@gmail.com>
Reviewed-by: Surya Poondla <suryapoondla4@gmail.com>
Discussion: https://postgr.es/m/CAHGQGwGnCVMVz8-LU9F8Sh57bkQX3jMZzx7age7M0LFEz5=Fog@mail.gmail.com
Backpatch-through: 14
Heap WAL records that clear bits on the visibility map (like inserts and
deletes) did not register the visibility map blocks they modified.
Because the WAL summarizer only records registered blocks, an
incremental backup taken over such operations would omit the changed VM
pages. On restore, the VM would retain stale all-visible/all-frozen
bits, which can cause wrong results from index-only scans and incorrect
relfrozenxid advancement due to vacuum page skipping.
Not registering the VM buffer also meant we never emitted FPIs of VM
pages when clearing bits. A torn VM page won't raise an error because
the VM is read with ZERO_ON_ERROR; with checksums on, it would be
detected and zeroed, but with checksums off, it is accepted as-is and
can lead to data corruption.
Fix this by registering the VM buffer in the WAL record when clearing VM
bits. The VM buffer must now be locked throughout the critical section
that modifies the VM and heap pages and emits the WAL record. This can
slow down operations that clear the VM, since the VM lock is held longer
and VM FPIs may be emitted, but it is required for correctness.
Note that this fix does not repair existing incremental backups.
Bumps XLOG_PAGE_MAGIC. Though it is late in the cycle (post-beta 2) to
be doing so, that seemed better than maintaining the backwards
compatability code in yet another branch.
Author: Melanie Plageman <melanieplageman@gmail.com>
Author: Andres Freund <andres@anarazel.de>
Reviewed-by: Robert Haas <robertmhaas@gmail.com>
Reviewed-by: Andrey Borodin <x4mmm@yandex-team.ru>
Discussion: https://postgr.es/m/flat/CAAKRu_bn%2Be7F4yPFBgFbnP%2BsyJRKyNK092bjD2LKvZW7O4Svag>
Backpatch-through: 17
When registering a buffer with the WAL machinery, the caller assigns it a
block reference ID, and replay must read each block back by that same ID.
Today these IDs are bare integers assigned by convention (0, 1, 2, ...),
which is easy to follow when a record registers a single block, or when the
blocks are handled during replay in their registration order.
An upcoming bug fix registers up to two visibility map blocks in addition
to the heap block(s) when clearing the VM, and these are not handled during
replay in a straightforward 1:1, in-registration-order fashion. Relying on
bare integers for the block IDs in that case is error-prone.
Introduce macros naming the block reference IDs for the heap record types
that the upcoming commit extends to register visibility map blocks, so the
registration and replay sites refer to the same block by a meaningful name.
Author: Melanie Plageman <melanieplageman@gmail.com>
Reviewed-by: Robert Haas <robertmhaas@gmail.com>
Discussion: https://postgr.es/m/66mqpfyti3qhfttcsv6r2lbvqqd32rrmpn6i47ovrsnvguts46%40gou54xc>
Backpatch through: 17
When bulk-extending a relation, we add the newly-added blocks that we
won't immediately use to the free space map and then call
FreeSpaceMapVacuumRange() to propagate that free space up the FSM tree,
so other backends can find and reuse it.
However, the end block argument to FreeSpaceMapVacuumRange() is
exclusive, and we passed the number of the last added block (since
00d1e02be2). If that block was the first one covered by a new FSM page,
its free space wasn't propagated up the tree and was therefore invisible
to FSM searches until the next FSM vacuum.
Fix by passing the block number one past the last added block, so the
full range is vacuumed.
Author: Jingtang Zhang <mrdrivingduck@gmail.com>
Reviewed-by: Melanie Plageman <melanieplageman@gmail.com>
Discussion: https://postgr.es/m/flat/CAPsk3_Bx_vdybN%3D-DZu8HLStf%2BXnuFUBkLwxouONSMkWuO9oug%40mail.gmail.com
Backpatch-through: 16
Code elsewhere in the system assumes that fork numbers and chunk sizes
are within bounds, so the code that reads those quantities from disk
should validate that they are. Without these additional checks, a
corrupted file can cause us to index off the end of fork number or chunk
entry arrays, potentially resulting in a crash.
Reported-by: oxsignal <awo@kakao.com> (chunk sizes)
Reported-by: Robert Haas <rhaas@postgresql.org> (fork numbers)
Reviewed-by: Daniel Gustafsson <daniel@yesql.se>
Discussion: http://postgr.es/m/CA+TgmoYP8RKoBGosS7C6Fdr-GNCfyz_W1zmK=Tx1Fe0ZvzGh0g@mail.gmail.com
Backpatch-through: 17
pg_clear_attribute_stats() checks its required arguments manually
because the function is not strict. Previously, when schemaname or
relname was passed as NULL, the error incorrectly reported the
argument name as "relation" in both cases:
ERROR: argument "relation" must not be null
This was misleading, especially for schemaname, and inconsistent with
the function's SQL-visible argument names.
The cause is that cleararginfo[] in attribute_stats.c used
"relation" for both the schema-name and relation-name arguments.
This commit fixes the issue by using "schemaname" and "relname" instead,
matching the function's declared argument names so that the error reports
the correct argument name.
Backpatch to v18, where pg_clear_attribute_stats() was introduced.
Author: Ilia Evdokimov <ilya.evdokimov@tantorlabs.com>
Reviewed-by: Fujii Masao <masao.fujii@gmail.com>
Discussion: https://postgr.es/m/4bf66c5e-8dd7-4ef3-8691-db67ecff6f16@tantorlabs.com
Backpatch-through: 18
'ALTER SUBSCRIPTION ... REFRESH SEQUENCES' can race with an already
running sequence synchronization worker. If a second refresh request
resets the synchronization state while the worker has already fetched
sequence values from the publisher but has not yet applied them to the
subscriber, the worker can overwrite the subscriber with stale values
and mark the synchronization as complete.
Avoid this race by rejecting 'ALTER SUBSCRIPTION ... REFRESH SEQUENCES'
when a sequence synchronization worker is already running for the
subscription. The command reports an error asking the user to rerun it
after the current synchronization completes.
Also add a wait for the re-added 'regress_s4' sequence to finish
synchronizing in 036_sequences.pl, so the subsequent test does not race
against its sequencesync worker.
Reported-by: Noah Misch <noah@leadboat.com>
Author: vignesh C <vignesh21@gmail.com>
Reviewed-by: Shveta Malik <shveta.malik@gmail.com>
Reviewed-by: Hayato Kuroda <kuroda.hayato@fujitsu.com>
Reviewed-by: Amit Kapila <amit.kapila16@gmail.com>
Backpatch-through: 19, where it was introduced
Discussion: https://postgr.es/m/20260710045217.f0.noahmisch@microsoft.com
Contrary to the from_serialized_data callback used by the pgstats reads
at startup, the to_serialized_data callback used for the pgstats writes
matched with pgstat_write_statsfile(), by not returning a boolean
status, expecting a ferror() failure to deal with the discard of the
stats file should an error happen while writing the stats. This was
slightly confusing designed this way.
Things are changed in this commit with:
- to_serialized_data now returns a boolean status on a write failure.
pgstat_write_statsfile() detects that and switches to failure mode
instead of continuing to process the entries to write, speeding up the
shutdown.
- pgstat_write_statsfile() now uses STATS_DISCARD if a failure happens,
to let the registered callbacks directly know that something is wrong,
and that things need to be cleaned up. This gives a better error path
detection for custom stats kinds. For example, they do not have to rely
solely on the expectation of an ferror() for an auxiliary file.
This new set of behaviors matches with what is already done in
pgstat_read_statsfile() for the finish() callback (DISCARD on failure,
READ on success) and the from_serialized_data with a status returned.
Author: Sami Imseih <samimseih@gmail.com>
Discussion: https://postgr.es/m/CAA5RZ0sMgOvuhpb2P=KSJOjgjC6AfUu+GYcu9mHar-y_Xtd=Pg@mail.gmail.com
Backpatch-through: 19
equalPolicy() is used in the relation cache to check if two policy
definitions are equivalent, but missed to check for polpermissive.
ALTER POLICY cannot switch a policy to be PERMISSIVE or RESTRICTIVE, so
this would need a dropped and then re-created policy, which would
trigger a relcache invalidation. Anyway, there is no harm in being
consistent in the check, and if one decides to add an ALTER POLICY to
switch PERMISSIVE or RESTRICTIVE, we would be silently in trouble.
Author: Andreas Lind <andreaslindpetersen@gmail.com>
Reviewed-by: Laurenz Albe <laurenz.albe@cybertec.at>
Discussion: https://postgr.es/m/CAMxA3rv1CS6R7JR5ojz-3CmCEnZEFrqu+XXTnGbLRWrjJRH7sA@mail.gmail.com
Backpatch-through: 14
Commit 9a60f295b stripped the stale PlaceHolderVars left behind by
left-join removal from the surviving rels' baserestrictinfo and from
EquivalenceClass member expressions, but it overlooked join clauses.
A PlaceHolderVar embedded in a join clause can likewise retain the
removed rel and join in its phrels, since remove_rel_from_query()
fixes up the RestrictInfo's own relid sets but not the PHVs inside its
expression.
As before, this is normally harmless, because later processing
consults those relid sets rather than the embedded PHVs. However, a
restriction clause derived from such an OR join clause inherits the
stale PlaceHolderVar, and when the derived clause is translated for an
appendrel child, pull_varnos() recomputes its relids and folds the
removed relation back in. The rebuilt clause then references a
no-longer-existent relation, tripping an assertion during path
generation.
Fix by also stripping the removed relation from the PlaceHolderVars in
the surviving rels' join clauses, including the sub-clauses of any OR
clause.
Like 9a60f295b, this is only reachable on v18 and later, where
match_index_to_operand() began ignoring PlaceHolderVars.
Author: Arne Roland <arne.roland@malkut.net>
Reviewed-by: Tender Wang <tndrwang@gmail.com>
Reviewed-by: Richard Guo <guofenglinux@gmail.com>
Discussion: https://postgr.es/m/27a44087-3d65-473e-8d88-7c12228e0d7e@malkut.net
Backpatch-through: 18
This reverts commit ed823da128, that has made pgstat_write_chunk() and
pgstat_read_chunk() available for public use. These routines do not
have a symmetric API definition across reads and writes, with the write
part returning a void status, deferring an error detection once all the
stats entries have been processed with an ferror(), and the read part
returning a boolean status.
These routines are just tiny wrappers around fread() and fwrite(), and
extensions can just define they own routines instead of relying on the
same facilities as the core pgstat.c. This commit removes their
declaration from the public headers, to reduce the confusion.
test_custom_stats is updated to use its own read/write routines.
Perhaps something better could be designed in the future; trying to do
so for v19 is not feasable during beta.
Reported-by: Peter Eisentraut <peter@eisentraut.org>
Author: Sami Imseih <samimseih@gmail.com>
Discussion: https://postgr.es/m/a4a8e9af-3eaf-4bbf-9b21-21620f3fc434@eisentraut.org
Backpatch-through: 19
Commit 62c3b4cd9d taught postgres_fdw to push down ArrayCoerceExpr, but
foreign_expr_walker() only recursed into the input array expression and
never examined elemexpr, the per-element conversion that gives the
coercion its semantics. deparseArrayCoerceExpr() then shipped a bare
"arg::resulttype" cast, or nothing at all for an implicit-format
coercion, leaving the remote server to re-resolve the element conversion
against its own catalogs and session state.
This produced wrong results or remote errors whenever the element
conversion was not a plain relabeling, and it was inconsistent with how
postgres_fdw treats the equivalent scalar coercions. An ArrayCoerceExpr
was shipped even when its elemexpr was a cast function (whose
shippability was never checked), a CoerceViaIO (e.g. float8out or
byteaout, which depend on extra_float_digits / bytea_output that
postgres_fdw sets differently on the remote session), or a
CoerceToDomain (which pushes domain enforcement to the remote catalog).
By contrast, a scalar CoerceViaIO is never shipped, and a scalar cast
function is shipped only when it is shippable.
Restrict pushdown to element coercions that are a plain relabeling, that
is, elemexpr is a RelabelType or a bare CaseTestExpr. Any other element
coercion is now evaluated locally. This keeps the common
binary-coercible case pushed down, including "col = ANY($1)" with a
varchar[]-to-text[] relabeling, which is the case 62c3b4cd9d set out to
optimize.
Pushing down shippable element cast functions, to reach parity with the
scalar case, is left out here for simplicity.
Reported-by: Noah Misch <noah@leadboat.com>
Discussion: https://postgr.es/m/20260711024234.43.noahmisch%40microsoft.com
Backpatch-through: 19
The pg_attribute_always_inline macro name is so long it forces pgindent
to format the code in strange ways. Which may incentivize patch authors
to either structure the code in strange ways (e.g. reorder prototypes),
use shorter names, etc. Neither is very desirable for code readability.
This shortens the name by removing the _attribute_ part. It also makes
it more consistent with pg_noinline, which does not have the _attribute_
part either.
Backpatched to all supported branches, to prevent conflicts when
backpatching other fixes. The backbranches however keep both the old and
new macro name, so that existing code keeps working.
Author: Andres Freund <andres@anarazel.de>
Reviewed-by: Peter Geoghegan <pg@bowt.ie>
Reviewed-by: Tomas Vondra <tomas@vondra.me>
Discussion: https://postgr.es/m/bqqdehahpoa36igpictuqyn2s2mexk3t3ehidh2ffd2slb35e5@rzgksuiszgbg
Backpatch-through: 14
In c75ebc657f I (Andres) introduced UnlockBufHdrExt() which can set and
clear bits in the buffer state using CAS. Unfortunately I added bits before
subtracting them, which means that a bit that was both removed and set would
remain unset. Fix the order of operations.
The only known case where that is a problem is that BM_IO_ERROR would not
actually remain set.
It's unfortunately not trivial to add a decent, race-free, test to verify that
BM_IO_ERROR remains set. That's therefore left for the 20 cycle.
Reported-by: Yura Sokolov <y.sokolov@postgrespro.ru>
Discussion: https://postgr.es/m/ab0dcc9e-aba0-44e3-ac23-8d74c48888e6@postgrespro.ru
Backpatch-through: 19, where c75ebc657f went in
When doing a whole database repack, we build a list of tables to process
taking a lock on each. But because it's a regular transaction-scoped
lock, it's automatically released immediately after building the list
anyway, which makes it not very useful. (Also, we have three ways to
obtain a list of tables to repack, and only one of them acquired this
lock.) Remove that lock acquisition, as it's useless and inconsistent.
We acquire a lock properly afterwards (and recheck that the table can
still be repacked as indicated), so we don't need to do anything other
than drop that initial lock acquisition and harden the code in
repack_is_permitted_for_relation() against possible concurrent drops.
This is similar to how vacuum does it in get_all_vacuum_rels().
In order for this to work reliably, also change
repack_is_permitted_for_relation() to cope with the possibility of the
table going away partway through. Similarly, in ExecRepack(), be
prepared for what we believed to be a table or matview to now be
something else, and skip it without erroring out, by changing
try_table_open() to try_relation_open() and testing the relkind
separately.
While at it, replace one relation_close() call in get_tables_to_repack()
with table_close() to match the table_open() that opened the catalog.
Author: ChangAo Chen <cca5507@qq.com>
Backpatch-through: 19
Discussion: https://postgr.es/m/tencent_9F290B256A3F52B66542F1140E32ECC64309@qq.com
When building the list of temporary relations to wait for, the code
previously included temporary relations without storage, such as
temporary views, even though they are irrelevant to checksum
processing. As a result, enabling data checksums could wait for a
long-lived session that owned only a temporary view.
This commit fixes the issue by filtering temporary relations with storage
only, matching the existing behavior for non-temporary relations.
Also, when enabling data checksums online, the launcher assigns the
first worker to process shared catalogs and prevents later workers from
doing so. Previously, if that worker's database was dropped after it
had been selected for processing but before checksum processing began,
the worker failed without processing the shared catalogs, yet they were
still marked as processed. As a result, later workers skipped them, and
checksum enabling could complete successfully even though the shared
catalogs had never been processed.
This commit fixes the issue by marking shared catalogs as processed
only after a worker completes successfully.
Author: Fujii Masao <masao.fujii@gmail.com>
Reviewed-by: Kyotaro Horiguchi <horikyota.ntt@gmail.com>
Reviewed-by: Daniel Gustafsson <daniel@yesql.se>
Discussion: https://postgr.es/m/CAHGQGwGDHAQw=bmpRzk+EmKzVtxZiD5YDurMUffBMwr6WXugQA@mail.gmail.com
Backpatch-through: 19
pg_stat_progress_data_checksums uses -1 as a sentinel value that is
displayed as NULL for progress counters. However, after
pgstat_progress_start_command() initialized all progress counters to
zero, data checksum progress did not reset those counters to -1.
As a result, some counters could incorrectly appear as zero instead
of NULL. For example, workers could report zero database counters,
and the disabling launcher could report zero relation and block counters.
Also, blocks_done was not reset when a worker started processing
a new relation fork. As a result, it could temporarily exceed blocks_total
or report a stale value for an empty relation fork.
Fix this by initializing the data checksum progress counters to -1
when progress reporting starts for both launcher and worker processes.
Also reset blocks_done together with blocks_total when starting each
relation fork.
Author: Fujii Masao <masao.fujii@gmail.com>
Reviewed-by: Daniel Gustafsson <daniel@yesql.se>
Discussion: https://postgr.es/m/CAHGQGwEOQyEzW2cqrHEzvwbcsAsuH8MEe7MMidFOFxECy0E1_Q@mail.gmail.com
Backpatch-through: 19
Previously, this function imported remote statistics by executing SQL
functions like pg_restore_relation_stats and pg_restore_attribute_stats
via SPI (in read-write mode). As the SQL functions take a schema name
and a relation name as two separate arguments, rather than a single OID
argument, if the containing schema was concurrently renamed, the
callback function would throw an error like this:
ERROR: schema "foo" does not exist
To fix, 1) provide new interface functions to import remote statistics
that are directly callable from FDWs and take a single OID, and 2)
modify the callback function to use the interface functions instead when
importing remote statistics.
For #1, this commit does a bit of refactoring to
relation_statistics_update and attribute_statistics_update, which are
the workhorse functions for pg_restore_relation_stats and
pg_restore_attribute_stats respectively: since they also take a schema
name and a relation name, separate the guts of them into new functions
so that they take a single OID and are callable not only from the
workhorse functions but from the interface functions introduced by #1.
Oversight in commit 28972b6fc.
Reported-by: Robert Haas <robertmhaas@gmail.com>
Suggested-by: Robert Haas <robertmhaas@gmail.com>
Author: Corey Huinker <corey.huinker@gmail.com>
Co-authored-by: Etsuro Fujita <etsuro.fujita@gmail.com>
Discussion: https://postgr.es/m/CA%2BTgmoYqMtWb4zLUkT98oFnEkJ%3DWz0Pw-ggDJrp9wnSXPzUaeQ%40mail.gmail.com
Backpatch-through: 19
Check explicitly for pqsecure_read() returning an error. It shouldn't
fail, and we would've caught it in the check for a short read, but
better to be explicit so that the error message is more informative.
We also shouldn't update 'inEnd' when the read fails, although that
too is just pro forma as we will bail out and close the connection on
error.
Reported-by: Peter Eisentraut <peter@eisentraut.org>
Discussion: https://www.postgresql.org/message-id/34844e8c-267c-4daf-b1e0-f26059a4a7d3@eisentraut.org
Backpatch-through: 14
