postgresql.git
3 months agoLower default value of autovacuum_worker_slots in initdb as needed.
Nathan Bossart [Tue, 7 Jan 2025 20:38:55 +0000 (14:38 -0600)]
Lower default value of autovacuum_worker_slots in initdb as needed.

Commit c758119e5b increased the default number of semaphores
required for autovacuum workers from 3 to 16.  Unfortunately, some
systems have very low default settings for SEMMNS, and this change
moved the minimum required for Postgres well beyond that limit (see
commit 38da053463 for more details).

With this commit, initdb will lower the default value for
autovacuum_worker_slots as needed, just like it already does for
parameters such as max_connections and shared_buffers.  We test
for (max_connections / 6) slots, which conveniently has the
following properties:

* For the initial max_connections default of 100, the default of
  autovacuum_worker_slots will be 16, which is its initial default
  value specified in the documentation and in guc_tables.c.

* For the lowest possible max_connections default of 25, the
  default of autovacuum_worker_slots will be 4, which means we only
  need one additional semaphore for autovacuum workers (as compared
  to before commit c758119e5b).  This leaves some wiggle room for
  new auxiliary workers, etc. on systems with low SEMMNS, and it
  ensures that the default number of slots will be greater than or
  equal to the default value of autovacuum_max_workers (3).

Reported-by: Tom Lane
Suggested-by: Andres Freund
Reviewed-by: Tom Lane
Discussion: https://postgr.es/m/1346002.1736198977%40sss.pgh.pa.us

3 months agoFix error message wording
Álvaro Herrera [Tue, 7 Jan 2025 19:07:32 +0000 (20:07 +0100)]
Fix error message wording

The originals are ambiguous and a bit out of style.

Reviewed-by: Amit Langote <amitlangote09@gmail.com>
Discussion: https://postgr.es/m/202412141243.efesjyyvzxsz@alvherre.pgsql

3 months agoFix meson detection of a couple of 64 bit builtins.
Thomas Munro [Tue, 7 Jan 2025 18:15:28 +0000 (07:15 +1300)]
Fix meson detection of a couple of 64 bit builtins.

A couple of checks were missed by commit 962da900, so we would fail to
detect the features.

Reported-by: Юрий Соколов <y.sokolov@postgrespro.ru>
Discussion: https://postgr.es/m/42C25E2A-6519-4549-9F47-6B0686E83836%40postgrespro.ru

3 months agoRemove unnecessary code to handle CONSTR_NOTNULL
Álvaro Herrera [Tue, 7 Jan 2025 15:49:41 +0000 (16:49 +0100)]
Remove unnecessary code to handle CONSTR_NOTNULL

Commit 14e87ffa5c54 needlessly added support for CONSTR_NOTNULL entries
to StoreConstraints.  It's dead code, so remove it.

To make the situation regarding constraint creation clearer, change
comments in heap_create_with_catalog, StoreConstraints, MergeAttributes
to explain which types of constraint are used on each.

Author: 何建 (Jian He) <jian.universality@gmail.com>
Discussion: https://postgr.es/m/CACJufxFxzqrCiUNfjJ0tQU+=nKQkQCGtGzUBude=SMOwj5VNjQ@mail.gmail.com

3 months agoImprove nbtree unsatisfiable RowCompare detection.
Peter Geoghegan [Tue, 7 Jan 2025 15:38:30 +0000 (10:38 -0500)]
Improve nbtree unsatisfiable RowCompare detection.

Move nbtree's detection of RowCompare quals that are unsatisfiable due
to having a NULL in their first row element: rather than detecting these
cases at the point where _bt_first builds its insertion scan key, do so
earlier, during preprocessing proper.  This brings the RowCompare case
in line every other case involving an unsatisfiable-due-to-NULL qual.

nbtree now consistently detects such unsatisfiable quals -- even when
they happen to involve a key that isn't examined by _bt_first at all.
Affected cases thereby avoid useless full index scans that cannot
possibly return any matching rows.

Author: Peter Geoghegan <pg@bowt.ie>
Reviewed-By: Matthias van de Meent <boekewurm+postgres@gmail.com>
Discussion: https://postgr.es/m/CAH2-WzmySVXst2hFrOATC-zw1Byg1XC-jYUS314=mzuqsNwk+Q@mail.gmail.com

3 months agonbtree: Simplify _bt_first parallel scan handling.
Peter Geoghegan [Tue, 7 Jan 2025 15:29:46 +0000 (10:29 -0500)]
nbtree: Simplify _bt_first parallel scan handling.

This new structure relieves _bt_first from having separate calls to
_bt_start_array_keys for the serial case and parallel case.  This saves
code, and seems clearer.

Follow-up to work from commits 4e6e375b and b5ee4e52.

Author: Peter Geoghegan <pg@bowt.ie>
Reviewed-By: Matthias van de Meent <boekewurm+postgres@gmail.com>
Discussion: https://postgr.es/m/CAH2-Wz=XjUZjBjHJdhTvuH5MwoJObWGoM2RG2LyFg5WUdWyk=A@mail.gmail.com

3 months agoRemove unused parameter in lookup_var_attr_stats
Richard Guo [Tue, 7 Jan 2025 02:24:14 +0000 (11:24 +0900)]
Remove unused parameter in lookup_var_attr_stats

The parameter 'rel' in lookup_var_attr_stats was once used to draw an
ERROR when ANALYZE failed to acquire sufficient data to build extended
statistics.  bf2a691e0 changed the logic to raise a WARNING in the
caller instead.  As a result, this parameter is no longer needed and
can be removed.  Since this is a static function, we can always easily
reintroduce the parameter if it's ever needed in the future.

Author: Ilia Evdokimov
Reviewed-by: Fabrízio de Royes Mello
Discussion: https://postgr.es/m/b3880f22-5808-4206-88d4-1553a81c3440@tantorlabs.com

3 months agoAllow changing autovacuum_max_workers without restarting.
Nathan Bossart [Mon, 6 Jan 2025 21:01:22 +0000 (15:01 -0600)]
Allow changing autovacuum_max_workers without restarting.

This commit introduces a new parameter named
autovacuum_worker_slots that controls how many autovacuum worker
slots to reserve during server startup.  Modifying this new
parameter's value does require a server restart, but it should
typically be set to the upper bound of what you might realistically
need to set autovacuum_max_workers.  With that new parameter in
place, autovacuum_max_workers can now be changed with a SIGHUP
(e.g., pg_ctl reload).

If autovacuum_max_workers is set higher than
autovacuum_worker_slots, a WARNING is emitted, and the server will
only start up to autovacuum_worker_slots workers at a given time.
If autovacuum_max_workers is set to a value less than the number of
currently-running autovacuum workers, the existing workers will
continue running, but no new workers will be started until the
number of running autovacuum workers drops below
autovacuum_max_workers.

Reviewed-by: Sami Imseih, Justin Pryzby, Robert Haas, Andres Freund, Yogesh Sharma
Discussion: https://postgr.es/m/20240410212344.GA1824549%40nathanxps13

3 months agoRemove duplicate definitions in proc.h
Heikki Linnakangas [Mon, 6 Jan 2025 09:56:03 +0000 (11:56 +0200)]
Remove duplicate definitions in proc.h

These are also present in procnumber.h

Reported-by: Peter Eisentraut
Discussion: https://www.postgresql.org/message-id/bd04d675-4672-4f87-800a-eb5d470c15fc@eisentraut.org

3 months agoflex code modernization: Replace YY_EXTRA_TYPE define with flex option
Peter Eisentraut [Mon, 6 Jan 2025 08:47:58 +0000 (09:47 +0100)]
flex code modernization: Replace YY_EXTRA_TYPE define with flex option

Replace #define YY_EXTRA_TYPE with %option extra-type.  The latter is
the way recommended by the flex manual (available since flex 2.5.34).

Reviewed-by: Heikki Linnakangas <hlinnaka@iki.fi>
Discussion: https://www.postgresql.org/message-id/flat/eb6faeac-2a8a-4b69-9189-c33c520e5b7b@eisentraut.org

3 months agodoc: Clarify log level for VERBOSE messages in maintenance commands.
Fujii Masao [Mon, 6 Jan 2025 08:24:10 +0000 (17:24 +0900)]
doc: Clarify log level for VERBOSE messages in maintenance commands.

VERBOSE messages from ANALYZE, CLUSTER, REINDEX, and VACUUM are logged
at the INFO level, but this detail was missing from the documentation.
This commit updates the docs to mention the log level for these messages.

Author: Masahiro Ikeda
Reviewed-by: Yugo Nagata
Discussion: https://postgr.es/m/b4a4b7916982dccd9607c8efb3ce5116@oss.nttdata.com

3 months agoAlways use the caller-provided context for radix tree leaves
John Naylor [Fri, 20 Dec 2024 07:48:24 +0000 (14:48 +0700)]
Always use the caller-provided context for radix tree leaves

Previously, it would not have worked for a caller to pass a slab
context, since it would have been used for other things which likely
had incompatible size. In an attempt to be helpful and avoid possible
space wastage due to aset's power-of-two rounding, RT_CREATE would
create an additional slab context if the value type was fixed-length
and larger than pointer size. The problem was, we have since added
the bump context type, and the generation context was a possibility as
well, so silently overriding the caller's choice may actually be worse.

Commit e8a6f1f908d arranged so that the caller-provided context is
used only for leaves, so it's safe for the caller to use slab here
if they wish. As demonstration, use slab in one of the radix tree
regression tests.

Reviewed by Masahiko Sawada

Discussion: https://postgr.es/m/CANWCAZZDCo4k5oURg_pPxM6+WZ1oiG=sqgjmQiELuyP0Vtrwig@mail.gmail.com

3 months agoGet rid of radix tree's general purpose memory context
John Naylor [Fri, 20 Dec 2024 06:04:18 +0000 (13:04 +0700)]
Get rid of radix tree's general purpose memory context

Previously, this was notionally used only for the entry point of the
tree and as a convenient parent for other contexts.

For shared memory, the creator previously allocated the entry point
in this context, but attaching backends didn't have access to that,
so they just used the caller's context. For the sake of consistency,
allocate every instance of an entry point in the caller's context.

For local memory, allocate the control object in the caller's context
as well. This commit also makes the "leaf context" the notional parent
of the child contexts used for nodes, so it's a bit of a misnomer,
but a future commit will make the node contexts independent rather
than children, so leave it this way for now to avoid code churn.

The memory context parameter for RT_CREATE is now unused in the case
of shared memory, so remove it and adjust callers to match.

In passing, remove unused "context" member from struct TidStore,
which seems to have been an oversight.

Reviewed by Masahiko Sawada

Discussion: https://postgr.es/m/CANWCAZZDCo4k5oURg_pPxM6+WZ1oiG=sqgjmQiELuyP0Vtrwig@mail.gmail.com

3 months agoUse caller's memory context for radix tree iteration state
John Naylor [Sat, 21 Dec 2024 03:55:31 +0000 (10:55 +0700)]
Use caller's memory context for radix tree iteration state

Typically only one iterator is present at any time, so it's overkill
to devote an entire context for this. Get rid of it and use the
caller's context.

This is tidy-up work, so no backpatch in this form. However, a
hypothetical extension to v17 that tried to start iteration from
an attaching backend would result in a crash, so that'll be fixed
separately in a way that doesn't change behavior in core.

Patch by me, reported and reviewed by Masahiko Sawada

Discussion: https://postgr.es/m/CAD21AoBB2U47V=F+wQRB1bERov_of5=BOZGaybjaV8FLQyqG3Q@mail.gmail.com

3 months agoRemove useless configure check
Peter Eisentraut [Sun, 5 Jan 2025 10:30:48 +0000 (11:30 +0100)]
Remove useless configure check

The test for "decltype" as a variant of "typeof" apparently never
worked (see also commit 3582b223d49), so remove it.

Discussion: https://www.postgresql.org/message-id/flat/795b1c54-c64a-47f9-8fa3-880dcab59975%40eisentraut.org

3 months agomeson: Fix missing name arguments of cc.compiles() calls
Peter Eisentraut [Sun, 5 Jan 2025 10:30:48 +0000 (11:30 +0100)]
meson: Fix missing name arguments of cc.compiles() calls

Without it, the check won't show up in the meson setup/configure
output.

Discussion: https://www.postgresql.org/message-id/flat/795b1c54-c64a-47f9-8fa3-880dcab59975%40eisentraut.org

3 months agoDocument strange jsonb sort order for empty top level arrays
Andrew Dunstan [Fri, 3 Jan 2025 14:23:46 +0000 (09:23 -0500)]
Document strange jsonb sort order for empty top level arrays

Slightly faulty logic in the original jsonb code (commit d9134d0a355)
results in an empty top level array sorting less than a json null. We
can't change the sort order now since it would affect btree indexes over
jsonb, so document the anomaly.

Backpatch to all live branches (13 .. 17)

In master, also add a code comment noting the anomaly.

Reported-by: Yan Chengpen
Reviewed-by: Jian He
Discussion: https://postgr.es/m/OSBPR01MB45199DD8DA2D1CECD50518188E272@OSBPR01MB4519.jpnprd01.prod.outlook.com

3 months agoIgnore nullingrels when looking up statistics
Richard Guo [Thu, 2 Jan 2025 08:56:59 +0000 (17:56 +0900)]
Ignore nullingrels when looking up statistics

When looking up statistical data about an expression, we do not need
to concern ourselves with the outer joins that could null the
Vars/PHVs contained in the expression.  Accounting for nullingrels in
the expression could cause estimate_num_groups to count the same Var
multiple times if it's marked with different nullingrels.  This is
incorrect, and could lead to "ERROR:  corrupt MVNDistinct entry" when
searching for multivariate n-distinct.

Furthermore, the nullingrels could prevent us from matching an
expression to expressional index columns or to the expressions in
extended statistics, leading to inaccurate estimates.

To fix, strip out all the nullingrels from the expression before we
look up statistical data about it.  There is one ensuing plan change
in the regression tests, but it looks reasonable and does not
compromise its original purpose.

This patch could result in plan changes, but it fixes an actual bug,
so back-patch to v16 where the outer-join-aware-Var infrastructure was
introduced.

Author: Richard Guo
Discussion: https://postgr.es/m/CAMbWs4-2Z4k+nFTiZe0Qbu5n8juUWenDAtMzi98bAZQtwHx0-w@mail.gmail.com

3 months agoFix outdated CHUNKHDRSZ value in nodeAgg.c
David Rowley [Thu, 2 Jan 2025 09:04:09 +0000 (22:04 +1300)]
Fix outdated CHUNKHDRSZ value in nodeAgg.c

CHUNKHDRSZ was defined as 16 bytes, which was true when that code went in,
but since c6e0fe1f2, 8 is a more accurate value.  Here we adjust it to use
sizeof(MemoryChunk), which is normally 8, or 16 for cassert builds.

c6e0fe1f2 first appeared in v16, so this is technically wrong in v16 up
to master, but let's apply this only to master as adjusting this does
influence the estimated number of batches in the aggregate costing code
and we don't want to cause plan instability in released versions.

Reviewed-by: Tom Lane
Discussion: https://postgr.es/m/CAApHDvpMpRQvsTqZo3FinXkgytwxwF8sCyZm83xDj-1s_hLe+w@mail.gmail.com

3 months agoFix an assortment of spelling mistakes and typos
David Rowley [Wed, 1 Jan 2025 23:42:01 +0000 (12:42 +1300)]
Fix an assortment of spelling mistakes and typos

Author: Alexander Lakhin <exclusion@gmail.com>
Discussion: https://postgr.es/m/5812a0b9-b0cf-4151-9a14-d9f00e4f2858@gmail.com

3 months agoUpdate copyright for 2025
Bruce Momjian [Wed, 1 Jan 2025 16:21:55 +0000 (11:21 -0500)]
Update copyright for 2025

Backpatch-through: 13

4 months agoUpdate obsolete reference to plpgsql's gram.y file.
Tom Lane [Mon, 30 Dec 2024 19:33:45 +0000 (14:33 -0500)]
Update obsolete reference to plpgsql's gram.y file.

This was evidently missed in 05346c131, which renamed that
file to pl_gram.y.

Japin Li

Discussion: https://postgr.es/m/ME0P300MB0445F7CA7456C2AC67D37A01B6092@ME0P300MB0445.AUSP300.PROD.OUTLOOK.COM

4 months agoinjection_points: Tweak variable-numbered stats to work with pending data
Michael Paquier [Mon, 30 Dec 2024 09:48:18 +0000 (18:48 +0900)]
injection_points: Tweak variable-numbered stats to work with pending data

As coded, the module was not using pending entries to store its data
locally before doing a flush to the central dshash with a timed
pgstat_report_stat() call.  Hence, the flush callback was defined, but
finished by being not used.  As a template, this is more efficient than
the original logic of updating directly the shared memory entries as
this reduces the interactions that need to be done with the pgstats
hash table in shared memory.

injection_stats_flush_cb() was also missing a pgstat_unlock_entry(), so
add one, while on it.

Reviewed-by: Bertrand Drouvot
Discussion: https://postgr.es/m/Z3JbLhKFFm6kKfT8@ip-10-97-1-34.eu-west-3.compute.internal

4 months agoFix memory leak in pgoutput with relation attribute map
Michael Paquier [Mon, 30 Dec 2024 04:33:09 +0000 (13:33 +0900)]
Fix memory leak in pgoutput with relation attribute map

pgoutput caches the attribute map of a relation, that is free()'d only
when validating a RelationSyncEntry.  However, this code path is not
taken when calling any of the SQL functions able to do some logical
decoding, like pg_logical_slot_{get,peek}_changes(), leaking some memory
into CacheMemoryContext on repeated calls.

To address this, a relation's attribute map is allocated in
PGOutputData's cachectx, free()'d at the end of the execution of these
SQL functions when logical decoding ends.  This is available down to 15.
v13 and v14 have a similar leak, which will be dealt with later.

Reported-by: Masahiko Sawada
Author: Vignesh C
Reviewed-by: Hou Zhijie
Discussion: https://postgr.es/m/CAD21AoDkAhQVSukOfH3_reuF-j4EU0-HxMqU3dU+bSTxsqT14Q@mail.gmail.com
Discussion: https://postgr.es/m/CALDaNm1hewNAsZ_e6FF52a=9drmkRJxtEPrzCB6-9mkJyeBBqA@mail.gmail.com
Backpatch-through: 15

4 months agoRemove redundant wording in pg_statistic.h
Michael Paquier [Mon, 30 Dec 2024 03:18:45 +0000 (12:18 +0900)]
Remove redundant wording in pg_statistic.h

Author: Junwang Zhao
Discussion: https://postgr.es/m/CAEG8a3JbMCHna=N5ZSx6huLnTDfW34kw7Pf2n8+3M-9UrrwesA@mail.gmail.com

4 months agoFix failures with incorrect epoch handling for 2PC files at recovery
Michael Paquier [Mon, 30 Dec 2024 00:58:02 +0000 (09:58 +0900)]
Fix failures with incorrect epoch handling for 2PC files at recovery

At the beginning of recovery, an orphaned two-phase file in an epoch
different than the one defined in the checkpoint record could not be
removed based on the assumptions that AdjustToFullTransactionId() relies
on, assuming that all files would be either from the current epoch or
from the previous epoch.

If the checkpoint epoch was 0 while the 2PC file was orphaned and in the
future, AdjustToFullTransactionId() would underflow the epoch used to
build the 2PC file path.  In non-assert builds, this would create a
WARNING message referring to a 2PC file with an epoch of "FFFFFFFF" (or
UINT32_MAX), as an effect of the underflow calculation, leaving the
orphaned file around.

Some tests are added with dummy 2PC files in the past and the future,
checking that these are properly removed.

Issue introduced by 5a1dfde8334b, that has switched two-phase state
files to use FullTransactionIds.

Reported-by: Vitaly Davydov
Author: Michael Paquier
Reviewed-by: Vitaly Davydov
Discussion: https://postgr.es/m/13b5b6-676c3080-4d-531db900@47931709
Backpatch-through: 17

4 months agoFix handling of orphaned 2PC files in the future at recovery
Michael Paquier [Sun, 29 Dec 2024 23:06:07 +0000 (08:06 +0900)]
Fix handling of orphaned 2PC files in the future at recovery

Before 728bd991c3c4, that has improved the support for 2PC files during
recovery, the initial logic scanning files in pg_twophase was done so as
files in the future of the transaction ID horizon were checked first,
followed by a check if a transaction ID is aborted or committed which
could involve a pg_xact lookup.  After this commit, these checks have
been done in reverse order.

Files detected as in the future do not have a state that can be checked
in pg_xact, hence this caused recovery to fail abruptly should an
orphaned 2PC file in the future of the transaction ID horizon exist in
pg_twophase at the beginning of recovery.

A test is added to check for this scenario, using an empty 2PC with a
transaction ID large enough to be in the future when running the test.
This test is added in 16 and older versions for now.  17 and newer
versions are impacted by a second bug caused by the addition of the
epoch in the 2PC file names.  An equivalent test will be added in these
branches in a follow-up commit, once the second set of issues reported
are fixed.

Author: Vitaly Davydov, Michael Paquier
Discussion: https://postgr.es/m/11e597-676ab680-8d-374f23c0@145466129
Backpatch-through: 13

4 months agocontrib/pageinspect: Use SQL-standard function bodies.
Tom Lane [Sun, 29 Dec 2024 19:58:05 +0000 (14:58 -0500)]
contrib/pageinspect: Use SQL-standard function bodies.

In the same spirit as 969bbd0fa13e3796c93f323eba8.

Tom Lane and Ronan Dunklau

Discussion: https://postgr.es/m/3316564.aeNJFYEL58@aivenlaptop

4 months agocontrib/xml2: Use SQL-standard function bodies.
Tom Lane [Sun, 29 Dec 2024 18:53:00 +0000 (13:53 -0500)]
contrib/xml2: Use SQL-standard function bodies.

In the same spirit as 969bbd0fa13e3796c93f323eba8.

Tom Lane and Ronan Dunklau

Discussion: https://postgr.es/m/3316564.aeNJFYEL58@aivenlaptop

4 months agocontrib/citext: Use SQL-standard function bodies.
Tom Lane [Sun, 29 Dec 2024 18:37:35 +0000 (13:37 -0500)]
contrib/citext: Use SQL-standard function bodies.

In the same spirit as 969bbd0fa13e3796c93f323eba8.

Tom Lane and Ronan Dunklau

Discussion: https://postgr.es/m/3316564.aeNJFYEL58@aivenlaptop

4 months agoFix overly large values/nulls arrays
David Rowley [Sun, 29 Dec 2024 10:57:43 +0000 (23:57 +1300)]
Fix overly large values/nulls arrays

These arrays were sized with Natts_pg_trigger (19) when they should have
been sized with Natts_pg_event_trigger (7).  We'd better fix this as
it's clearly a mistake and it could become problematic if
pg_event_trigger were to gain a dozen or so more columns in the future.

No backpatch as there's no actual bug and the column count on those
tables isn't going to change in released versions.

Author: Xin Zhang <zhanghien@qq.com>
Discussion: https://postgr.es/m/tencent_05AD0FB321A414EC3661204D2102AA6EF605@qq.com

4 months agoReplace PGPROC.isBackgroundWorker with isRegularBackend.
Tom Lane [Sat, 28 Dec 2024 21:21:54 +0000 (16:21 -0500)]
Replace PGPROC.isBackgroundWorker with isRegularBackend.

Commit 34486b609 effectively redefined isBackgroundWorker as meaning
"not a regular backend", whereas before it had the narrower
meaning of AmBackgroundWorkerProcess().  For clarity, rename the
field to isRegularBackend and invert its sense.

Discussion: https://postgr.es/m/1808397.1735156190@sss.pgh.pa.us

4 months agoExclude parallel workers from connection privilege/limit checks.
Tom Lane [Sat, 28 Dec 2024 21:08:50 +0000 (16:08 -0500)]
Exclude parallel workers from connection privilege/limit checks.

Cause parallel workers to not check datallowconn, rolcanlogin, and
ACL_CONNECT privileges.  The leader already checked these things
(except for rolcanlogin which might have been checked for a different
role).  Re-checking can accomplish little except to induce unexpected
failures in applications that might not even be aware that their query
has been parallelized.  We already had the principle that parallel
workers rely on their leader to pass a valid set of authorization
information, so this change just extends that a bit further.

Also, modify the ReservedConnections, datconnlimit and rolconnlimit
logic so that these limits are only enforced against regular backends,
and only regular backends are counted while checking if the limits
were already reached.  Previously, background processes that had an
assigned database or role were subject to these limits (with rather
random exclusions for autovac workers and walsenders), and the set of
existing processes that counted against each limit was quite haphazard
as well.  The point of these limits, AFAICS, is to ensure the
availability of PGPROC slots for regular backends.  Since all other
types of processes have their own separate pools of PGPROC slots, it
makes no sense either to enforce these limits against them or to count
them while enforcing the limit.

While edge-case failures of these sorts have been possible for a
long time, the problem got a good deal worse with commit 5a2fed911
(CVE-2024-10978), which caused parallel workers to make some of these
checks using the leader's current role where before we had used its
AuthenticatedUserId, thus allowing parallel queries to fail after
SET ROLE.  The previous behavior was fairly accidental and I have
no desire to return to it.

This patch includes reverting 73c9f91a1, which was an emergency hack
to suppress these same checks in some cases.  It wasn't complete,
as shown by a recent bug report from Laurenz Albe.  We can also revert
fd4d93d26 and 492217301, which hacked around the same problems in one
regression test.

In passing, remove the special case for autovac workers in
CheckMyDatabase; it seems cleaner to have AutoVacWorkerMain pass
the INIT_PG_OVERRIDE_ALLOW_CONNS flag, now that that does what's
needed.

Like 5a2fed911, back-patch to supported branches (which sadly no
longer includes v12).

Discussion: https://postgr.es/m/1808397.1735156190@sss.pgh.pa.us

4 months agoReserve a PGPROC slot and semaphore for the slotsync worker process.
Tom Lane [Sat, 28 Dec 2024 17:30:42 +0000 (12:30 -0500)]
Reserve a PGPROC slot and semaphore for the slotsync worker process.

The need for this was missed in commit 93db6cbda, with the result
being that if we launch a slotsync worker it would consume one of
the PGPROCs in the max_connections pool.  That could lead to inability
to launch the worker, or to subsequent failures of connection requests
that should have succeeded according to the configured settings.

Rather than create some one-off infrastructure to support this,
let's group the slotsync worker with the existing autovac launcher
in a new category of "special worker" processes.  These are kind of
like auxiliary processes, but they cannot use that infrastructure
because they need to be able to run transactions.

For the moment, make these processes share the PGPROC freelist
used for autovac workers (which previously supplied the autovac
launcher too).  This is partly to avoid an ABI change in v17,
and partly because it seems silly to have a freelist with
at most two members.  This might be worth revisiting if we grow
enough workers in this category.

Tom Lane and Hou Zhijie.  Back-patch to v17.

Discussion: https://postgr.es/m/1808397.1735156190@sss.pgh.pa.us

4 months agoIn REASSIGN OWNED of a database, lock the tuple as mandated.
Noah Misch [Sat, 28 Dec 2024 15:16:22 +0000 (07:16 -0800)]
In REASSIGN OWNED of a database, lock the tuple as mandated.

Commit aac2c9b4fde889d13f859c233c2523345e72d32b mandated such locking
and attempted to fulfill that mandate, but it missed REASSIGN OWNED.
Hence, it remained possible to lose VACUUM's inplace update of
datfrozenxid if a REASSIGN OWNED processed that database at the same
time.  This didn't affect the other inplace-updated catalog, pg_class.
For pg_class, REASSIGN OWNED calls ATExecChangeOwner() instead of the
generic AlterObjectOwner_internal(), and ATExecChangeOwner() fulfills
the locking mandate.

Like in GRANT, implement this by following the locking protocol for any
catalog subject to the generic AlterObjectOwner_internal().  It would
suffice to do this for IsInplaceUpdateOid() catalogs only.  Back-patch
to v13 (all supported versions).

Kirill Reshke.  Reported by Alexander Kukushkin.

Discussion: https://postgr.es/m/CAFh8B=mpKjAy4Cuun-HP-f_vRzh2HSvYFG3rhVfYbfEBUhBAGg@mail.gmail.com

4 months agoSpeedup tuple deformation with additional function inlining
David Rowley [Fri, 27 Dec 2024 23:20:42 +0000 (12:20 +1300)]
Speedup tuple deformation with additional function inlining

This adjusts slot_deform_heap_tuple() to add special-case loops to
eliminate much of the branching that was done within the body of the
main deform loop.

Previously, while looping over each attribute to deform,
slot_deform_heap_tuple() would always recheck if the given attribute was
NULL by looking at HeapTupleHasNulls() and if so, went on to check the
tuple's NULL bitmap.  Since many tuples won't contain any NULLs, we can
just check HeapTupleHasNulls() once and when there are no NULLs, use a
more compact version of the deforming loop which contains no NULL checking
code at all.

The same is possible for the "slow" mode checking part of the loop.  That
variable was checked several times for each attribute, once to determine
if the offset to the attribute value could be taken from the attcacheoff,
and again to check if the offset could be cached for next time.

These "slow" checks can mostly be eliminated by instead having multiple
loops.  Initially, we can start in the non-slow loop and break out of
that loop if and only if we must stop caching the offset.  This
eliminates branching for both slow and non-slow deforming methods.  The
amount of code required for the no nulls / non-slow version is very
small.  It's possible to have separate loops like this due to the fact
that once we move into slow mode, we never need to switch back into
non-slow mode for a given tuple.

We have the compiler take care of writing out the multiple required
loops by having a pg_attribute_always_inline function which gets called
various times passing in constant values for the "slow" and "hasnulls"
parameters.  This allows the compiler to eliminate const-false branches
and remove comparisons for const-true ones.

This commit has shown overall query performance increases of around 5-20%
in deform-heavy OLAP-type workloads.

Author: David Rowley
Reviewed-by: Victor Yegorov
Discussion: https://postgr.es/m/CAGnEbog92Og2CpC2S8=g_HozGsWtt_3kRS1sXjLz0jKSoCNfLw@mail.gmail.com
Discussion: https://postgr.es/m/CAApHDvo9e0XG71WrefYaRv5n4xNPLK4k8LjD0mSR3c9KR2vi2Q@mail.gmail.com

4 months agoImprove handling of date_trunc() units for infinite input values
Michael Paquier [Fri, 27 Dec 2024 04:32:40 +0000 (13:32 +0900)]
Improve handling of date_trunc() units for infinite input values

Previously, if an infinite value was passed to date_trunc(), then the
same infinite value would always be returned regardless of the field
unit given by the caller.  This commit updates the function so that an
error is returned when an invalid unit is passed to date_trunc() with an
infinite value.

This matches the behavior of date_trunc() with a finite value and
date_part() with an infinite value, making the handling of interval,
timestamp and timestamptz more consistent across the board for these two
functions.

Some tests are added to cover all these new failure cases, with an
unsupported unit and infinite values for the three data types.  There
were no test cases in core that checked all these patterns up to now.

Author: Joseph Koshakow
Discussion: https://postgr.es/m/CAAvxfHc4084dGzEJR0_pBZkDuqbPGc5wn7gK_M0XR_kRiCdUJQ@mail.gmail.com

4 months agoRemove unused totalrows parameter in compute_expr_stats
David Rowley [Thu, 26 Dec 2024 21:51:22 +0000 (10:51 +1300)]
Remove unused totalrows parameter in compute_expr_stats

The totalrows parameter in compute_expr_stats is unused, so remove it.
This is a static function, so the parameter can easily be added again if
it's ever needed.

Author: Ilia Evdokimov <ilya.evdokimov@tantorlabs.ru>
Discussion: https://postgr.es/m/667b92d2-f953-4fcb-9377-3765f5b94187@tantorlabs.com

4 months agoplpgsql: Rename a variable for clarity
Peter Eisentraut [Thu, 26 Dec 2024 10:11:14 +0000 (11:11 +0100)]
plpgsql: Rename a variable for clarity

Rename "core_yy_extra_type core_yy" to "core_yy_extra".  The previous
name was a bit unclear and confusing.  The new name matches the name
used elsewhere for the same purpose, for example in
src/backend/parser/gramparse.h.

4 months agoFix typo in comment of compute_return_type() in functioncmds.c
Michael Paquier [Thu, 26 Dec 2024 03:53:55 +0000 (12:53 +0900)]
Fix typo in comment of compute_return_type() in functioncmds.c

Author: Japin Li
Discussion: https://postgr.es/m/ME0P300MB0445D51BCFA8680F0B35FD6EB60C2@ME0P300MB0445.AUSP300.PROD.OUTLOOK.COM

4 months agomeson: Export all libcommon functions in Windows builds
Heikki Linnakangas [Wed, 25 Dec 2024 17:22:25 +0000 (19:22 +0200)]
meson: Export all libcommon functions in Windows builds

This fixes "unresolved external symbol" errors with extensions that
use functions from libpgport that need special CFLAGS to
compile. Currently, that includes the CRC-32 functions.

Commit 2571c1d5cc did this for libcommon, but I missed that libpqport
has the same issue.

Reported-by: Tom Lane
Backpatch-through: 16, where Meson was introduced
Discussion: https://www.postgresql.org/message-id/CAOdR5yF0krWrxycA04rgUKCgKugRvGWzzGLAhDZ9bzNv8g0Lag@mail.gmail.com

4 months agoAdd commit 301de6a6f60 to .git-blame-ignore-revs.
Peter Eisentraut [Wed, 25 Dec 2024 17:17:29 +0000 (18:17 +0100)]
Add commit 301de6a6f60 to .git-blame-ignore-revs.

4 months agoPartial pgindent of .l and .y files
Peter Eisentraut [Wed, 25 Dec 2024 16:52:42 +0000 (17:52 +0100)]
Partial pgindent of .l and .y files

Trying to clean up the code a bit while we're working on these files
for the reentrant scanner/pure parser patches.  This cleanup only
touches the code sections after the second '%%' in each file, via a
manually-supervised and locally hacked up pgindent.

4 months agomeson: Export all libcommon functions in Windows builds
Heikki Linnakangas [Wed, 25 Dec 2024 16:14:18 +0000 (18:14 +0200)]
meson: Export all libcommon functions in Windows builds

This fixes "unresolved external symbol" errors with extensions that
use functions from libcommon. This was reported with pgvector.

Reported-by: Andrew Kane
Author: Vladlen Popolitov
Backpatch-through: 16, where Meson was introduced
Discussion: https://www.postgresql.org/message-id/CAOdR5yF0krWrxycA04rgUKCgKugRvGWzzGLAhDZ9bzNv8g0Lag@mail.gmail.com

4 months agoguc: reentrant scanner
Peter Eisentraut [Wed, 25 Dec 2024 13:18:07 +0000 (14:18 +0100)]
guc: reentrant scanner

Use the flex %option reentrant to make the generated scanner
reentrant, and perhaps eventually thread-safe, but that will require
additional work.

Reviewed-by: Heikki Linnakangas <hlinnaka@iki.fi>
Reviewed-by: Andreas Karlsson <andreas@proxel.se>
Discussion: https://www.postgresql.org/message-id/flat/eb6faeac-2a8a-4b69-9189-c33c520e5b7b@eisentraut.org

4 months agojsonpath scanner: reentrant scanner
Peter Eisentraut [Tue, 24 Dec 2024 22:42:41 +0000 (23:42 +0100)]
jsonpath scanner: reentrant scanner

Use the flex %option reentrant to make the generated scanner
reentrant and thread-safe.  Note: The parser was already pure.

Simplify flex scan buffer management: Instead of constructing the
buffer from pieces and then using yy_scan_buffer(), we can just use
yy_scan_string(), which does the same thing internally.  (Actually, we
use yy_scan_bytes() here because we already have the length.)

Use flex yyextra to handle context information, instead of global
variables.  This complements the other changes to make the scanner
reentrant.

Reviewed-by: Heikki Linnakangas <hlinnaka@iki.fi>
Reviewed-by: Andreas Karlsson <andreas@proxel.se>
Discussion: https://www.postgresql.org/message-id/flat/eb6faeac-2a8a-4b69-9189-c33c520e5b7b@eisentraut.org

4 months agoFix nbtree symbol name comment reference.
Peter Geoghegan [Tue, 24 Dec 2024 19:06:16 +0000 (14:06 -0500)]
Fix nbtree symbol name comment reference.

Oversight in commit 5bf748b86b.

4 months agosyncrep parser: pure parser and reentrant scanner
Peter Eisentraut [Mon, 2 Dec 2024 09:35:37 +0000 (10:35 +0100)]
syncrep parser: pure parser and reentrant scanner

Use the flex %option reentrant and the bison option %pure-parser to
make the generated scanner and parser pure, reentrant, and
thread-safe.

Make the generated scanner use palloc() etc. instead of malloc() etc.
Previously, we only used palloc() for the buffer, but flex would still
use malloc() for its internal structures.  Now, all the memory is
under palloc() control.

Simplify flex scan buffer management: Instead of constructing the
buffer from pieces and then using yy_scan_buffer(), we can just use
yy_scan_string(), which does the same thing internally.

The previous code was necessary because we allocated the buffer with
palloc() and the rest of the state was handled by malloc().  But this
is no longer the case; everything is under palloc() now.

Use flex yyextra to handle context information, instead of global
variables.  This complements the other changes to make the scanner
reentrant.

Reviewed-by: Heikki Linnakangas <hlinnaka@iki.fi>
Reviewed-by: Andreas Karlsson <andreas@proxel.se>
Discussion: https://www.postgresql.org/message-id/flat/eb6faeac-2a8a-4b69-9189-c33c520e5b7b@eisentraut.org

4 months agoreplication parser: pure parser and reentrant scanner
Peter Eisentraut [Mon, 2 Dec 2024 09:35:37 +0000 (10:35 +0100)]
replication parser: pure parser and reentrant scanner

Use the flex %option reentrant and the bison option %pure-parser to
make the generated scanner and parser pure, reentrant, and
thread-safe.

Make the generated scanner use palloc() etc. instead of malloc() etc.
Previously, we only used palloc() for the buffer, but flex would still
use malloc() for its internal structures.  As a result, there could be
some small memory leaks in case of uncaught errors.  Now, all the
memory is under palloc() control, so there are no more such issues.

Simplify flex scan buffer management: Instead of constructing the
buffer from pieces and then using yy_scan_buffer(), we can just use
yy_scan_string(), which does the same thing internally.

The previous code was necessary because we allocated the buffer with
palloc() and the rest of the state was handled by malloc().  But this
is no longer the case; everything is under palloc() now.

Use flex yyextra to handle context information, instead of global
variables.  This complements the other changes to make the scanner
reentrant.

Reviewed-by: Heikki Linnakangas <hlinnaka@iki.fi>
Co-authored-by: Andreas Karlsson <andreas@proxel.se>
Reviewed-by: Andreas Karlsson <andreas@proxel.se>
Discussion: https://www.postgresql.org/message-id/flat/eb6faeac-2a8a-4b69-9189-c33c520e5b7b@eisentraut.org

4 months agoRemove pgrminclude and associated scripts
Peter Eisentraut [Tue, 24 Dec 2024 13:02:42 +0000 (14:02 +0100)]
Remove pgrminclude and associated scripts

Per git log, the last time someone tried to do something with
pgrminclude was around 2011.  And it's always had a tendency of
causing trouble when it was active.  Also, pgcominclude is redundant
with headerscheck.

Discussion: https://www.postgresql.org/message-id/flat/2d4dc7b2-cb2e-49b1-b8ca-ba5f7024f05b%40eisentraut.org

4 months agoRemove pgrminclude annotations
Peter Eisentraut [Tue, 24 Dec 2024 10:48:08 +0000 (11:48 +0100)]
Remove pgrminclude annotations

Per git log, the last time someone tried to do something with
pgrminclude was around 2011.  Many (not all) of the "pgrminclude
ignore" annotations are of a newer date but seem to have just been
copied around during refactorings and file moves and don't seem to
reflect an actual need anymore.

There have been some parallel experiments with include-what-you-use
(IWYU) annotations, but these don't seem to correspond very strongly
to pgrminclude annotations, so there is no value in keeping the
existing ones even for that kind of thing.

So, wipe them all away.  We can always add new ones in the future
based on actual needs.

Discussion: https://www.postgresql.org/message-id/flat/2d4dc7b2-cb2e-49b1-b8ca-ba5f7024f05b%40eisentraut.org

4 months agoFix race condition in TupleDescCompactAttr assert code
David Rowley [Tue, 24 Dec 2024 01:54:24 +0000 (14:54 +1300)]
Fix race condition in TupleDescCompactAttr assert code

5983a4cff added CompactAttribute as an abbreviated alternative to
FormData_pg_attribute to allow more cache-friendly processing in tasks
related to TupleDescs.  That commit contained some assert-only code to
check that the CompactAttribute had been populated correctly, however,
the method used to do that checking caused the TupleDesc's
CompactAttribute to be zeroed before it was repopulated and compared to
the snapshot taken before the memset call.  This caused issues as the type
cache caches TupleDescs in shared memory which can be used by multiple
backend processes at the same time.  There was a window of time between
the zero and repopulation of the CompactAttribute where another process
would mistakenly think that the CompactAttribute is invalid due to the
memset.

To fix this, instead of taking a snapshot of the CompactAttribute and
calling populate_compact_attribute() and comparing the snapshot to the
freshly populated TupleDesc's CompactAttribute, refactor things so we
can just populate a temporary CompactAttribute on the stack.  This way
we don't touch the TupleDesc's memory.

Reported-by: Alexander Lakhin, SQLsmith
Discussion: https://postgr.es/m/ca3a256a-5d12-42db-aabe-a75a030d9fb9@gmail.com

4 months agoTry to avoid semaphore-related test failures on NetBSD/OpenBSD.
Tom Lane [Mon, 23 Dec 2024 21:46:07 +0000 (16:46 -0500)]
Try to avoid semaphore-related test failures on NetBSD/OpenBSD.

These two platforms have a remarkably tight default limit on the
number of SysV semaphores in the system: SEMMNS is only 60
out-of-the-box.  Unless manual action is taken to raise that,
we'll only be able to allocate 3 sets of 16 usable semaphores
each, leading to initdb setting max_connections to just 20.
That's problematic because the core regression tests expect
to be able to launch 20 concurrent sessions, leaving us with
no headroom.  This seems to be the cause of intermittent
buildfarm failures on some machines.

While there's no getting around the fact that you'd better raise
SEMMNS for production use on these platforms, it does seem desirable
for "make check" to pass reliably without that.  We can make that
happen, at least for awhile longer, with two small changes:

* Change sysv_sema.c's SEMAS_PER_SET to 19, so that we can eat up
all of the available semas not just most of them.

* Change initdb to make the smallest max_connections value it will
consider be 25 not 20.

As of HEAD this will leave us with four free semaphores (using the
default values for other relevant parameters such as max_wal_senders).
So we won't need to consider this again until we've invented five
more background processes.  Maybe by then we can switch both these
platforms to some other semaphore API.

For the moment, do this only in master; there've not been field
complaints that might justify a back-patch.

Discussion: https://postgr.es/m/db2773a2-aca0-43d0-99c1-060efcd9954e@gmail.com

4 months agoReset btpo_cycleid in nbtree VACUUM's REDO routine.
Peter Geoghegan [Mon, 23 Dec 2024 20:46:00 +0000 (15:46 -0500)]
Reset btpo_cycleid in nbtree VACUUM's REDO routine.

Reset btpo_cycleid to 0 in btree_xlog_vacuum for consistency with
_bt_delitems_vacuum (the corresponding original execution code).  This
makes things neater.

There might be some performance benefit to being consistent like this.
When btvacuumpage doesn't call _bt_delitems_vacuum, it can still
proactively reset btpo_cycleid to 0 via a separate hint-like update
mechanism (it does so whenever it sees that it isn't already set to 0).
And so it's possible that being consistent about resetting btpo_cycleid
like this will save work later on, after standby promotion: subsequent
VACUUMs won't need to clear btpo_cycleid using the hint-like update
mechanism as often as they otherwise would.

Author: Peter Geoghegan <pg@bowt.ie>
Reviewed-By: Andrey Borodin <x4mmm@yandex-team.ru>
Discussion: https://postgr.es/m/CAH2-Wz=+LDFxn9NZyEsCo8ifcyKt6+n-VLyygySEHgMz+oynqw@mail.gmail.com

4 months agopostgres_fdw: re-issue cancel requests a few times if necessary.
Tom Lane [Mon, 23 Dec 2024 20:14:30 +0000 (15:14 -0500)]
postgres_fdw: re-issue cancel requests a few times if necessary.

Despite the best efforts of commit 0e5c82380, we're still seeing
occasional failures of postgres_fdw's query_cancel test in the
buildfarm.  Investigation suggests that its 100ms timeout is
still not enough to reliably ensure that the remote side starts
the query before receiving the cancel request --- and if it
hasn't, it will just discard the request because it's idle.

We discussed allowing a cancel request to kill the next-received
query, but that would have wide and perhaps unpleasant side-effects.
What seems safer is to make postgres_fdw do what a human user would
likely do, which is issue another cancel request if the first one
didn't seem to do anything.  We'll keep the same overall 30 second
grace period before concluding things are broken, but issue additional
cancel requests after 1 second, then 2 more seconds, then 4, then 8.
(The next one in series is 16 seconds, but we'll hit the 30 second
timeout before that.)

Having done that, revert the timeout in query_cancel.sql to 10 ms.
That will still be enough on most machines, most of the time, for
the remote query to start; but now we're intentionally risking the
race condition occurring sometimes in the buildfarm, so that the
repeat-cancel code path will get some testing.

As before, back-patch to v17.  We might eventually contemplate
back-patching this further, and/or adding similar logic to dblink.
But given the lack of field complaints to date, this feels like
mostly an exercise in test case stabilization, so v17 is enough.

Discussion: https://postgr.es/m/colnv3lzzmc53iu5qoawynr6qq7etn47lmggqr65ddtpjliq5d@glkveb4m6nop

4 months agoDon't allow GetTransactionSnapshot() in logical decoding
Heikki Linnakangas [Mon, 23 Dec 2024 10:42:55 +0000 (12:42 +0200)]
Don't allow GetTransactionSnapshot() in logical decoding

A historic snapshot should only be used for catalog access, not
general queries. We never call GetTransactionSnapshot() during logical
decoding, which is good because it wouldn't be very sensible, so the
code to deal with that was unreachable and untested. Turn it into an
error, to avoid doing that in the future either.

Discussion: https://www.postgresql.org/message-id/a868fe78-ddb4-4b0a-9b96-873d91d93cfd@iki.fi

4 months agoRemove unnecessary GetTransactionSnapshot() calls
Heikki Linnakangas [Mon, 23 Dec 2024 10:42:39 +0000 (12:42 +0200)]
Remove unnecessary GetTransactionSnapshot() calls

In get_database_list() and get_subscription_list(), the
GetTransactionSnapshot() call is not required because the catalog
table scans use the catalog snapshot, which is held until the end of
the scan. See table_beginscan_catalog(), which calls
RegisterSnapshot(GetCatalogSnapshot(relid)).

In InitPostgres, it's a little less obvious that it's not required,
but still true I believe. All the catalog lookups in InitPostgres()
also use the catalog snapshot, and the looked up values are copied
while still holding the snapshot.

Furthermore, as the removed FIXME comments said, calling
GetTransactionSnapshot() didn't really prevent MyProc->xmin from being
reset anyway.

Discussion: https://www.postgresql.org/message-id/7c56f180-b9e1-481e-8c1d-efa63de3ecbb@iki.fi

4 months agoFix incorrect source filename references
David Rowley [Mon, 23 Dec 2024 06:41:49 +0000 (19:41 +1300)]
Fix incorrect source filename references

Jian He reported the src/include/utility/tcop.h one and the remainder
were found by using a script to look for src/* and check that we have a
filename or directory of that name.

In passing, fix some out-date comments.

Reported-by: Jian He <jian.universality@gmail.com>
Reviewed-by: Tom Lane
Discussion: https://postgr.es/m/CACJufxGoE3H-7VgO02=PrR4SNuVWDVbfTyUnwO0HvS-Lxurnog@mail.gmail.com

4 months agoFix some comments related to library unloading
Michael Paquier [Mon, 23 Dec 2024 05:46:49 +0000 (14:46 +0900)]
Fix some comments related to library unloading

Library unloading has never been supported with its code removed in
ab02d702ef08, and there were some comments still mentioning that it was
a possible operation.

ChangAo has noticed the incorrect references in dfmgr.c, while I have
noticed the other ones while scanning the rest of the tree for similar
mistakes.

Author: ChangAo Chen, Michael Paquier
Reviewed-by: Tom Lane
Discussion: https://postgr.es/m/tencent_1D09840A1632D406A610C8C4E2491D74DB0A@qq.com

4 months agoUpdate TransactionXmin when MyProc->xmin is updated
Heikki Linnakangas [Sat, 21 Dec 2024 21:42:39 +0000 (23:42 +0200)]
Update TransactionXmin when MyProc->xmin is updated

GetSnapshotData() set TransactionXmin = MyProc->xmin, but when
SnapshotResetXmin() advanced MyProc->xmin, it did not advance
TransactionXmin correspondingly. That meant that TransactionXmin could
be older than MyProc->xmin, and XIDs between than TransactionXmin and
the real MyProc->xmin could be vacuumed away. One known consequence is
in pg_subtrans lookups: we might try to look up the status of an XID
that was already truncated away.

Back-patch to all supported versions.

Reviewed-by: Andres Freund
Discussion: https://www.postgresql.org/message-id/d27a046d-a1e4-47d1-a95c-fbabe41debb4@iki.fi

4 months agoOptimize alignment calculations in tuple form/deform
David Rowley [Fri, 20 Dec 2024 20:43:26 +0000 (09:43 +1300)]
Optimize alignment calculations in tuple form/deform

Here we convert CompactAttribute.attalign from a char, which is directly
derived from pg_attribute.attalign into a uint8, which stores the number
of bytes to align the column's value by in the tuple.

This allows tuple deformation and tuple size calculations to move away
from using the inefficient att_align_nominal() macro, which manually
checks each TYPALIGN_* char to translate that into the alignment bytes
for the given type.  Effectively, this commit changes those to TYPEALIGN
calls, which are branchless and only perform some simple arithmetic with
some bit-twiddling.

The removed branches were often mispredicted by CPUs, especially so in
real-world tables which often contain a mishmash of different types
with different alignment requirements.

Author: David Rowley
Reviewed-by: Andres Freund, Victor Yegorov
Discussion: https://postgr.es/m/CAApHDvrBztXP3yx=NKNmo3xwFAFhEdyPnvrDg3=M0RhDs+4vYw@mail.gmail.com

4 months agoMark CatalogSnapshotData static
Heikki Linnakangas [Thu, 19 Dec 2024 16:02:11 +0000 (18:02 +0200)]
Mark CatalogSnapshotData static

Like CurrentSnapshotData, it should not be accessed directly outside
snapmgr.c.

4 months agoFix variable reference in comment
Heikki Linnakangas [Fri, 20 Dec 2024 17:36:33 +0000 (19:36 +0200)]
Fix variable reference in comment

This used to say "nsubxcnt isn't decreased when subtransactions
abort", but there's no variable called nsubxcnt. Commit 8548ddc61b
changed it to "subxcnt", among other typo fixes, but that was wrong
too: the comment actually talks about txn->nsubtxns. That's the field
that's incremented but never decremented and is used for the
allocation earlier in the function.

4 months agoFix overflow danger in SampleHeapTupleVisible(), take 2
Melanie Plageman [Fri, 20 Dec 2024 14:41:41 +0000 (09:41 -0500)]
Fix overflow danger in SampleHeapTupleVisible(), take 2

28328ec87b45725 addressed one overflow danger in
SampleHeapTupleVisible() but introduced another, albeit a less likely
one. Modify the binary search code to remove this danger.

Reported-by: Richard Guo
Reviewed-by: Richard Guo, Ranier Vilela
Discussion: https://postgr.es/m/CAMbWs4_bE%2BNscChbKWzw6HZOipCUyXfA5133qvoXQ654D3B2gQ%40mail.gmail.com

4 months agoFix corruption when relation truncation fails.
Thomas Munro [Fri, 20 Dec 2024 08:53:25 +0000 (21:53 +1300)]
Fix corruption when relation truncation fails.

RelationTruncate() does three things, while holding an
AccessExclusiveLock and preventing checkpoints:

1. Logs the truncation.
2. Drops buffers, even if they're dirty.
3. Truncates some number of files.

Step 2 could previously be canceled if it had to wait for I/O, and step
3 could and still can fail in file APIs.  All orderings of these
operations have data corruption hazards if interrupted, so we can't give
up until the whole operation is done.  When dirty pages were discarded
but the corresponding blocks were left on disk due to ERROR, old page
versions could come back from disk, reviving deleted data (see
pgsql-bugs #18146 and several like it).  When primary and standby were
allowed to disagree on relation size, standbys could panic (see
pgsql-bugs #18426) or revive data unknown to visibility management on
the primary (theorized).

Changes:

 * WAL is now unconditionally flushed first
 * smgrtruncate() is now called in a critical section, preventing
   interrupts and causing PANIC on file API failure
 * smgrtruncate() has a new parameter for existing fork sizes,
   because it can't call smgrnblocks() itself inside a critical section

The changes apply to RelationTruncate(), smgr_redo() and
pg_truncate_visibility_map().  That last is also brought up to date with
other evolutions of the truncation protocol.

The VACUUM FileTruncate() failure mode had been discussed in older
reports than the ones referenced below, with independent analysis from
many people, but earlier theories on how to fix it were too complicated
to back-patch.  The more recently invented cancellation bug was
diagnosed by Alexander Lakhin.  Other corruption scenarios were spotted
by me while iterating on this patch and earlier commit 75818b3a.

Back-patch to all supported releases.

Reviewed-by: Michael Paquier <michael@paquier.xyz>
Reviewed-by: Robert Haas <robertmhaas@gmail.com>
Reported-by: rootcause000@gmail.com
Reported-by: Alexander Lakhin <exclusion@gmail.com>
Discussion: https://postgr.es/m/18146-04e908c662113ad5%40postgresql.org
Discussion: https://postgr.es/m/18426-2d18da6586f152d6%40postgresql.org

4 months agoRemove pg_attribute.attcacheoff column
David Rowley [Fri, 20 Dec 2024 10:22:37 +0000 (23:22 +1300)]
Remove pg_attribute.attcacheoff column

The column is no longer needed as the offset is now cached in the
CompactAttribute struct per commit 5983a4cff.

Author: David Rowley
Reviewed-by: Andres Freund, Victor Yegorov
Discussion: https://postgr.es/m/CAApHDvrBztXP3yx=NKNmo3xwFAFhEdyPnvrDg3=M0RhDs+4vYw@mail.gmail.com

4 months agoRelax regression test for fsync check of backend-level stats
Michael Paquier [Fri, 20 Dec 2024 10:00:18 +0000 (19:00 +0900)]
Relax regression test for fsync check of backend-level stats

One test added in 9aea73fc61d4 did not take into account that the
backend may have some fsync even after a checkpoint.  Let's relax it to
be more flexible.

Per report from buildfarm member grassquit, via Alexander Lakhin.

Author: Bertrand Drouvot
Discussion: https://postgr.es/m/6143ab0a-9e88-4790-8d9d-50ba45657761@gmail.com

4 months agoIntroduce CompactAttribute array in TupleDesc, take 2
David Rowley [Fri, 20 Dec 2024 09:31:26 +0000 (22:31 +1300)]
Introduce CompactAttribute array in TupleDesc, take 2

The new compact_attrs array stores a few select fields from
FormData_pg_attribute in a more compact way, using only 16 bytes per
column instead of the 104 bytes that FormData_pg_attribute uses.  Using
CompactAttribute allows performance-critical operations such as tuple
deformation to be performed without looking at the FormData_pg_attribute
element in TupleDesc which means fewer cacheline accesses.

For some workloads, tuple deformation can be the most CPU intensive part
of processing the query.  Some testing with 16 columns on a table
where the first column is variable length showed around a 10% increase in
transactions per second for an OLAP type query performing aggregation on
the 16th column.  However, in certain cases, the increases were much
higher, up to ~25% on one AMD Zen4 machine.

This also makes pg_attribute.attcacheoff redundant.  A follow-on commit
will remove it, thus shrinking the FormData_pg_attribute struct by 4
bytes.

Author: David Rowley
Reviewed-by: Andres Freund, Victor Yegorov
Discussion: https://postgr.es/m/CAApHDvrBztXP3yx=NKNmo3xwFAFhEdyPnvrDg3=M0RhDs+4vYw@mail.gmail.com

4 months agoRemove final mention of FREEZE_PAGE from comments
Melanie Plageman [Thu, 19 Dec 2024 23:52:19 +0000 (18:52 -0500)]
Remove final mention of FREEZE_PAGE from comments

b7493e1ab35 removed leftover mentions of XLOG_HEAP2_FREEZE_PAGE records
from comments but neglected to remove one mention of FREEZE_PAGE.

Reported off-list by Alexander Lakhin

4 months agoGet rid of old version of BuildTupleHashTable().
Tom Lane [Thu, 19 Dec 2024 23:07:00 +0000 (18:07 -0500)]
Get rid of old version of BuildTupleHashTable().

It was reasonable to preserve the old API of BuildTupleHashTable()
in the back branches, but in HEAD we should actively discourage use
of that version.  There are no remaining callers in core, so just
get rid of it.  Then rename BuildTupleHashTableExt() back to
BuildTupleHashTable().

While at it, fix up the miserably-poorly-maintained header comment
for BuildTupleHashTable[Ext].  It looks like more than one patch in
this area has had the opinion that updating comments is beneath them.

Discussion: https://postgr.es/m/538343.1734646986@sss.pgh.pa.us

4 months agoUse ExecGetCommonSlotOps infrastructure in more places.
Tom Lane [Thu, 19 Dec 2024 22:07:14 +0000 (17:07 -0500)]
Use ExecGetCommonSlotOps infrastructure in more places.

Append, MergeAppend, and RecursiveUnion can all use the support
functions added in commit 276279295.  The first two can report a
fixed result slot type if all their children return the same fixed
slot type.  That does nothing for the append step itself, but might
allow optimizations in the parent plan node.  RecursiveUnion can
optimize tuple hash table operations in the same way as SetOp now
does.

Patch by me; thanks to Richard Guo and David Rowley for review.

Discussion: https://postgr.es/m/1850138.1731549611@sss.pgh.pa.us

4 months agoImprove planner's handling of SetOp plans.
Tom Lane [Thu, 19 Dec 2024 22:02:25 +0000 (17:02 -0500)]
Improve planner's handling of SetOp plans.

Remove the code for inserting flag columns in the inputs of a SetOp.
That was the only reason why there would be resjunk columns in a
set-operations plan tree, so we can get rid of some code that
supported that, too.

Get rid of choose_hashed_setop() in favor of building Paths for
the hashed and sorted alternatives, and letting them fight it out
within add_path().

Remove set_operation_ordered_results_useful(), which was giving wrong
answers due to examining the wrong ancestor node: we need to examine
the immediate SetOperationStmt parent not the topmost node.  Instead
make each caller of recurse_set_operations() pass down the relevant
parent node.  (This thinko seems to have led only to wasted planning
cycles and possibly-inferior plans, not wrong query answers.  Perhaps
we should back-patch it, but I'm not doing so right now.)

Teach generate_nonunion_paths() to consider pre-sorted inputs for
sorted SetOps, rather than always generating a Sort node.

Patch by me; thanks to Richard Guo and David Rowley for review.

Discussion: https://postgr.es/m/1850138.1731549611@sss.pgh.pa.us

4 months agoConvert SetOp to read its inputs as outerPlan and innerPlan.
Tom Lane [Thu, 19 Dec 2024 21:23:45 +0000 (16:23 -0500)]
Convert SetOp to read its inputs as outerPlan and innerPlan.

The original design for set operations involved appending the two
input relations into one and adding a flag column that allows
distinguishing which side each row came from.  Then the SetOp node
pries them apart again based on the flag.  This is bizarre.  The
only apparent reason to do it is that when sorting, we'd only need
one Sort node not two.  But since sorting is at least O(N log N),
sorting all the data is actually worse than sorting each side
separately --- plus, we have no chance of taking advantage of
presorted input.  On top of that, adding the flag column frequently
requires an additional projection step that adds cycles, and then
the Append node isn't free either.  Let's get rid of all of that
and make the SetOp node have two separate children, using the
existing outerPlan/innerPlan infrastructure.

This initial patch re-implements nodeSetop.c and does a bare minimum
of work on the planner side to generate correctly-shaped plans.
In particular, I've tried not to change the cost estimates here,
so that the visible changes in the regression test results will only
involve removal of useless projection steps and not any changes in
whether to use sorted vs hashed mode.

For SORTED mode, we combine successive identical tuples from each
input into groups, and then merge-join the groups.  The tuple
comparisons now use SortSupport instead of simple equality, but
the group-formation part should involve roughly the same number of
tuple comparisons as before.  The cross-comparisons between left and
right groups probably add to that, but I'm not sure to quantify how
many more comparisons we might need.

For HASHED mode, nodeSetop's logic is almost the same as before,
just refactored into two separate loops instead of one loop that
has an assumption that it will see all the left-hand inputs first.

In both modes, I added early-exit logic to not bother reading the
right-hand relation if the left-hand input is empty, since neither
INTERSECT nor EXCEPT modes can produce any output if the left input
is empty.  This could have been done before in the hashed mode, but
not in sorted mode.  Sorted mode can also stop as soon as it exhausts
the left input; any remaining right-hand tuples cannot have matches.

Also, this patch adds some infrastructure for detecting whether
child plan nodes all output the same type of tuple table slot.
If they do, the hash table logic can use slightly more efficient
code based on assuming that that's the input slot type it will see.
We'll make use of that infrastructure in other plan node types later.

Patch by me; thanks to Richard Guo and David Rowley for review.

Discussion: https://postgr.es/m/1850138.1731549611@sss.pgh.pa.us

4 months agoRemove extra prefetch iterator setup for Bitmap Table Scan
Melanie Plageman [Thu, 19 Dec 2024 16:55:18 +0000 (11:55 -0500)]
Remove extra prefetch iterator setup for Bitmap Table Scan

1a0da347a7ac98db replaced Bitmap Table Scan's separate private and
shared bitmap iterators with a unified iterator. It accidentally set up
the prefetch iterator twice for non-parallel bitmap table scans. Remove
the extra set up call to tbm_begin_iterate().

4 months agoFix bitmap table scan crash on iterator release
Melanie Plageman [Thu, 19 Dec 2024 16:55:03 +0000 (11:55 -0500)]
Fix bitmap table scan crash on iterator release

1a0da347a7ac98db replaced Bitmap Table Scan's individual private and
shared iterators with a unified iterator. It neglected, however, to
check if the iterator had already been cleaned up before doing so on
rescan. Add this check both on rescan and end scan to be safe.

Reported-by: Richard Guo
Author: Richard Guo
Discussion: https://postgr.es/m/CAMbWs48nrhcLY1kcd-u9oD%2B6yiS631F_8Fx8ZGsO-BYDwH%2Bbyw%40mail.gmail.com

4 months agoAvoid nbtree index scan SAOP scanBehind confusion.
Peter Geoghegan [Thu, 19 Dec 2024 16:08:55 +0000 (11:08 -0500)]
Avoid nbtree index scan SAOP scanBehind confusion.

Consistently reset so->scanBehind at the beginning of nbtree array
advancement, even during sktrig_required=false calls (calls where array
advancement is triggered by an unsatisfied non-required array scan key).
Otherwise, it's possible for queries to fail to return all relevant
tuples to the scan given a low-order required scan key that was
previously deemed "satisfied" by a truncated high key attribute value.
This only happened at the point where a later non-required array scan
key needed to be "advanced" once on the next leaf page (that is, once
the right sibling of the truncated high key page was reached).

The underlying issue was that later code within _bt_advance_array_keys
assumed that the so->scanBehind flag must have been set using the
current page's high key (not the previous page's high key).  Any later
successful recheck call to _bt_check_compare would therefore spuriously
be prevented from making _bt_advance_array_keys return true, based on
the faulty belief that the truncated attribute must be from the scan's
current tuple (i.e. the non-pivot tuple at the start of the next page).
_bt_advance_array_keys would return false for the tuple, ultimately
resulting in _bt_checkkeys failing to return a matching tuple.

Oversight in commit 5bf748b8, which enhanced nbtree ScalarArrayOp
execution.

Author: Peter Geoghegan <pg@bowt.ie>
Discussion: https://postgr.es/m/CAH2-WzkJKncfqyAUTeuB5GgRhT1vhsWO2q11dbZNqKmvjopP_g@mail.gmail.com
Backpatch: 17-, where commit 5bf748b8 first appears.

4 months agobootstrap: pure parser and reentrant scanner
Peter Eisentraut [Thu, 19 Dec 2024 14:37:44 +0000 (15:37 +0100)]
bootstrap: pure parser and reentrant scanner

Use the flex %option reentrant and the bison option %pure-parser to
make the generated scanner and parser pure, reentrant, and
thread-safe.

Make the generated scanner use palloc() etc. instead of malloc() etc.

For the bootstrap scanner and parser, reentrancy and memory management
aren't that important, but we make this change here anyway so that all
the scanners and parsers in the backend use a similar set of options
and APIs.

Reviewed-by: Heikki Linnakangas <hlinnaka@iki.fi>
Reviewed-by: Andreas Karlsson <andreas@proxel.se>
Discussion: https://www.postgresql.org/message-id/flat/eb6faeac-2a8a-4b69-9189-c33c520e5b7b@eisentraut.org

4 months agoSmall whitespace improvement
Peter Eisentraut [Thu, 19 Dec 2024 12:00:31 +0000 (13:00 +0100)]
Small whitespace improvement

Author: Andreas Karlsson <andreas@proxel.se>
Discussion: https://www.postgresql.org/message-id/flat/eb6faeac-2a8a-4b69-9189-c33c520e5b7b@eisentraut.org

4 months agoPrevent redeclaration of typedef yyscan_t
Peter Eisentraut [Thu, 19 Dec 2024 10:21:06 +0000 (11:21 +0100)]
Prevent redeclaration of typedef yyscan_t

Fix for 1f0de66ea2a: We need to prevent redeclaration of typedef
yyscan_t.  (This will work with C11 but not currently with C99.)  The
generated scanner files provide their own typedef, but we also need to
provide one for the interfaces that we expose.  So we need to add some
preprocessor guards to avoid a redefinition.  (This is how the
generated scanner files do it internally as well.)  This way
everything now works independent of the order in which things are
included.

Discussion: https://www.postgresql.org/message-id/flat/eb6faeac-2a8a-4b69-9189-c33c520e5b7b@eisentraut.org

4 months agoAdd backend-level statistics to pgstats
Michael Paquier [Thu, 19 Dec 2024 04:19:22 +0000 (13:19 +0900)]
Add backend-level statistics to pgstats

This adds a new variable-numbered statistics kind in pgstats, where the
object ID key of the stats entries is based on the proc number of the
backends.  This acts as an upper-bound for the number of stats entries
that can exist at once.  The entries are created when a backend starts
after authentication succeeds, and are removed when the backend exits,
making the stats entry exist for as long as their backend is up and
running.  These are not written to the pgstats file at shutdown (note
that write_to_file is disabled, as a safety measure).

Currently, these stats include only information about the I/O generated
by a backend, using the same layer as pg_stat_io, except that it is now
possible to know how much activity is happening in each backend rather
than an overall aggregate of all the activity.  A function called
pg_stat_get_backend_io() is added to access this data depending on the
PID of a backend.  The existing structure could be expanded in the
future to add more information about other statistics related to
backends, depending on requirements or ideas.

Auxiliary processes are not included in this set of statistics.  These
are less interesting to have than normal backends as they have dedicated
entries in pg_stat_io, and stats kinds of their own.

This commit includes also pg_stat_reset_backend_stats(), function able
to reset all the stats associated to a single backend.

Bump catalog version and PGSTAT_FILE_FORMAT_ID.

Author: Bertrand Drouvot
Reviewed-by: Álvaro Herrera, Kyotaro Horiguchi, Michael Paquier, Nazir
Bilal Yavuz
Discussion: https://postgr.es/m/ZtXR+CtkEVVE/LHF@ip-10-97-1-34.eu-west-3.compute.internal

4 months agoExtract logic filling pg_stat_get_io()'s tuplestore into its own routine
Michael Paquier [Thu, 19 Dec 2024 01:16:02 +0000 (10:16 +0900)]
Extract logic filling pg_stat_get_io()'s tuplestore into its own routine

This commit adds pg_stat_io_build_tuples(), a helper routine for
pg_stat_get_io(), that fills its result tuplestore based on the contents
of PgStat_BktypeIO.  This will be used in a follow-up commit that uses
the same structures as pg_stat_io for reporting, including the same
object types and contexts, but for a different statistics kind.

Author: Bertrand Drouvot, Michael Paquier
Discussion: https://postgr.es/m/ZtXR+CtkEVVE/LHF@ip-10-97-1-34.eu-west-3.compute.internal

4 months agoOptimize grouping equality checks with virtual slots
David Rowley [Thu, 19 Dec 2024 00:57:21 +0000 (13:57 +1300)]
Optimize grouping equality checks with virtual slots

8f4ee9626 fixed an old Assert failure that could happen when the slot
type used to look up the hash table for BuildTupleHashTableExt() users
wasn't a TTSOpsMinimalTuple slot.  The fix for that in the back branches
had to be to pass the TupleTableSlotOps as NULL, however in master,
since we have the inputOps parameter as was added by d96d1d515, we can
pass that down instead.

At least one caller uses a fixed slot that's always TTSOpsVirtual, so
passing down inputOps for these cases allows ExecBuildGroupingEqual() to
skip adding the EEOP_INNER_FETCHSOME ExprEvalStep.

This should increase the performance of hashed subplans very slightly.

Author: Tom Lane, David Rowley
Discussion: https://postgr.es/m/2543667.1734483723@sss.pgh.pa.us

4 months agoFix Assert failure in WITH RECURSIVE UNION queries
David Rowley [Thu, 19 Dec 2024 00:11:39 +0000 (13:11 +1300)]
Fix Assert failure in WITH RECURSIVE UNION queries

If the non-recursive part of a recursive CTE ended up using
TTSOpsBufferHeapTuple as the table slot type, then a duplicate value
could cause an Assert failure in CheckOpSlotCompatibility() when
checking the hash table for the duplicate value.  The expected slot type
for the deform step was TTSOpsMinimalTuple so the Assert failed when the
TTSOpsBufferHeapTuple slot was used.

This is a long-standing bug which we likely didn't notice because it
seems much more likely that the non-recursive term would have required
projection and used a TTSOpsVirtual slot, which CheckOpSlotCompatibility
is ok with.

There doesn't seem to be any harm done here other than the Assert
failure.  Both TTSOpsMinimalTuple and TTSOpsBufferHeapTuple slot types
require tuple deformation, so the EEOP_*_FETCHSOME ExprState step would
have properly existed in the ExprState.

The solution is to pass NULL for the ExecBuildGroupingEqual's 'lops'
parameter.  This means the ExprState's EEOP_*_FETCHSOME step won't
expect a fixed slot type.  This makes CheckOpSlotCompatibility() happy as
no checking is performed when the ExprEvalStep is not expecting a fixed
slot type.

Reported-by: Richard Guo
Reviewed-by: Tom Lane
Discussion: https://postgr.es/m/CAMbWs4-8U9q2LAtf8+ghV11zeUReA3AmrYkxzBEv0vKnDxwkKA@mail.gmail.com
Backpatch-through: 13, all supported versions

4 months agoRemove leftover mentions of XLOG_HEAP2_FREEZE_PAGE records
Melanie Plageman [Wed, 18 Dec 2024 23:47:21 +0000 (18:47 -0500)]
Remove leftover mentions of XLOG_HEAP2_FREEZE_PAGE records

f83d709760d merged the separate XLOG_HEAP2_FREEZE_PAGE records into a
new combined prune, freeze, and vacuum record with opcode
XLOG_HEAP2_PRUNE_VACUUM_SCAN. Remove the last few references to
XLOG_HEAP2_FREEZE_PAGE records which were accidentally left behind.

Reported-by: Tomas Vondra
Reviewed-by: Robert Haas
Discussion: https://postgr.es/m/CA%2BTgmoY1tYff-1CEn8kYt5FsOrynTbtr%3DUZw%3D7mTC1Hv1HpeBQ%40mail.gmail.com

4 months agoBitmap Table Scans use unified TBMIterator
Melanie Plageman [Wed, 18 Dec 2024 23:43:39 +0000 (18:43 -0500)]
Bitmap Table Scans use unified TBMIterator

With the repurposing of TBMIterator as an interface for both parallel
and serial iteration through TIDBitmaps in commit 7f9d4187e7bab10329cc,
bitmap table scans may now use it.

Modify bitmap table scan code to use the TBMIterator. This requires
moving around a bit of code, so a few variables are initialized
elsewhere.

Author: Melanie Plageman
Reviewed-by: Tomas Vondra
Discussion: https://postgr.es/m/c736f6aa-8b35-4e20-9621-62c7c82e2168%40vondra.me

4 months agoAdd common interface for TBMIterators
Melanie Plageman [Wed, 18 Dec 2024 23:19:28 +0000 (18:19 -0500)]
Add common interface for TBMIterators

Add and use TBMPrivateIterator, which replaces the current TBMIterator
for serial use cases, and repurpose TBMIterator to be a unified
interface for both the serial ("private") and parallel ("shared") TID
Bitmap iterator interfaces. This encapsulation simplifies call sites for
callers supporting both parallel and serial TID Bitmap access.
TBMIterator is not yet used in this commit.

Author: Melanie Plageman
Reviewed-by: Tomas Vondra, Heikki Linnakangas
Discussion: https://postgr.es/m/063e4eb4-32d9-439e-a0b1-75565a9835a8%40iki.fi

4 months agoFix overflow danger in SampleHeapTupleVisible()
Melanie Plageman [Wed, 18 Dec 2024 23:16:43 +0000 (18:16 -0500)]
Fix overflow danger in SampleHeapTupleVisible()

68d9662be1c4b70 made HeapScanDesc->rs_ntuples unsigned but neglected to
change how it was being used in SampleHeapTupleVisible().

Return early if rs_ntuples is 0 to avoid overflowing and incorrectly
executing the loop code in SampleHeapTupleVisible().

Reported-by: Ranier Vilela
Discussion: https://postgr.es/m/CAEudQAot_xQoZyPZjpj1aBUPrPykY5mOPHGyvfe%3Djz%2BWowdA3A%40mail.gmail.com

4 months agoMake rs_cindex and rs_ntuples unsigned
Melanie Plageman [Wed, 18 Dec 2024 16:47:38 +0000 (11:47 -0500)]
Make rs_cindex and rs_ntuples unsigned

HeapScanDescData.rs_cindex and rs_ntuples can't be less than 0. All scan
types using the heap scan descriptor expect these values to be >= 0.
Make that expectation clear by making rs_cindex and rs_ntuples unsigned.

Also remove the test in heapam_scan_bitmap_next_tuple() that checks if
rs_cindex < 0. This was never true, but now that rs_cindex is unsigned,
it makes even less sense.

While we are at it, initialize both rs_cindex and rs_ntuples to 0 in
initscan().

Author: Melanie Plageman
Reviewed-by: Dilip Kumar
Discussion: https://postgr.es/m/CAAKRu_ZxF8cDCM_BFi_L-t%3DRjdCZYP1usd1Gd45mjHfZxm0nZw%40mail.gmail.com

4 months agoseg: pure parser and reentrant scanner
Peter Eisentraut [Wed, 18 Dec 2024 07:47:53 +0000 (08:47 +0100)]
seg: pure parser and reentrant scanner

Use the flex %option reentrant and the bison option %pure-parser to
make the generated scanner and parser pure, reentrant, and
thread-safe.

Make the generated scanner use palloc() etc. instead of malloc() etc.
Previously, we only used palloc() for the buffer, but flex would still
use malloc() for its internal structures.  As a result, there could be
some small memory leaks in case of uncaught errors.  (We do catch
normal syntax errors as soft errors.)  Now, all the memory is under
palloc() control, so there are no more such issues.

Simplify flex scan buffer management: Instead of constructing the
buffer from pieces and then using yy_scan_buffer(), we can just use
yy_scan_string(), which does the same thing internally.

The previous code was necessary because we allocated the buffer with
palloc() and the rest of the state was handled by malloc().  But this
is no longer the case; everything is under palloc() now.

(We could even get rid of the yylex_destroy() call and just let the
memory context cleanup handle everything.  But for now, we preserve
the existing behavior.)

Reviewed-by: Heikki Linnakangas <hlinnaka@iki.fi>
Reviewed-by: Andreas Karlsson <andreas@proxel.se>
Discussion: https://www.postgresql.org/message-id/flat/eb6faeac-2a8a-4b69-9189-c33c520e5b7b@eisentraut.org

4 months agocube: pure parser and reentrant scanner
Peter Eisentraut [Wed, 18 Dec 2024 07:47:34 +0000 (08:47 +0100)]
cube: pure parser and reentrant scanner

Use the flex %option reentrant and the bison option %pure-parser to
make the generated scanner and parser pure, reentrant, and
thread-safe.

Make the generated scanner use palloc() etc. instead of malloc() etc.
Previously, we only used palloc() for the buffer, but flex would still
use malloc() for its internal structures.  As a result, there could be
some small memory leaks in case of uncaught errors.  (We do catch
normal syntax errors as soft errors.)  Now, all the memory is under
palloc() control, so there are no more such issues.

Simplify flex scan buffer management: Instead of constructing the
buffer from pieces and then using yy_scan_buffer(), we can just use
yy_scan_string(), which does the same thing internally.  (Actually, we
use yy_scan_bytes() here because we already have the length.)

The previous code was necessary because we allocated the buffer with
palloc() and the rest of the state was handled by malloc().  But this
is no longer the case; everything is under palloc() now.

(We could even get rid of the yylex_destroy() call and just let the
memory context cleanup handle everything.  But for now, we preserve
the existing behavior.)

Reviewed-by: Heikki Linnakangas <hlinnaka@iki.fi>
Reviewed-by: Andreas Karlsson <andreas@proxel.se>
Discussion: https://www.postgresql.org/message-id/flat/eb6faeac-2a8a-4b69-9189-c33c520e5b7b@eisentraut.org

4 months agopsql: Add more information about service name
Michael Paquier [Wed, 18 Dec 2024 06:16:12 +0000 (15:16 +0900)]
psql: Add more information about service name

This commit adds support for the following items in psql, able to show a
service name, when available:
- Variable SERVICE.
- Substitution %s in PROMPT{1,2,3}.

This relies on 4b99fed7541e, that has made the service name available in
PGconn for libpq.

Author: Michael Banck
Reviewed-by: Greg Sabino Mullane
Discussion: https://postgr.es/m/6723c612.050a0220.1567f4.b94a@mx.google.com

4 months agolibpq: Add service name to PGconn and PQservice()
Michael Paquier [Wed, 18 Dec 2024 05:53:42 +0000 (14:53 +0900)]
libpq: Add service name to PGconn and PQservice()

This commit adds one field to PGconn for the database service name (if
any), with PQservice() as routine to retrieve it.  Like the other
routines of this area, NULL is returned as result if the connection is
NULL.

A follow-up patch will make use of this feature to be able to display
the service name in the psql prompt.

Author: Michael Banck
Reviewed-by: Greg Sabino Mullane
Discusion: https://postgr.es/m/6723c612.050a0220.1567f4.b94a@mx.google.com

4 months agoFix memory leak in pg_restore with zstd-compressed data.
Tom Lane [Wed, 18 Dec 2024 03:31:26 +0000 (22:31 -0500)]
Fix memory leak in pg_restore with zstd-compressed data.

EndCompressorZstd() neglected to free everything.  This was
most visible with a lot of large objects in the dump.

Per report from Tomasz Szypowski.  Back-patch to v16
where this code came in.

Discussion: https://postgr.es/m/DU0PR04MB94193D038A128EF989F922D199042@DU0PR04MB9419.eurprd04.prod.outlook.com

4 months agoFix incorrect slot type in BuildTupleHashTableExt
David Rowley [Tue, 17 Dec 2024 23:05:55 +0000 (12:05 +1300)]
Fix incorrect slot type in BuildTupleHashTableExt

0f5738202 adjusted the execGrouping.c code so it made use of ExprStates to
generate hash values.  That commit made a wrong assumption that the slot
type to pass to ExecBuildHash32FromAttrs() is always &TTSOpsMinimalTuple.
That's not the case as the slot type depends on the slot type passed to
LookupTupleHashEntry(), which for nodeRecursiveunion.c, could be any of
the current slot types.

Here we fix this by adding a new parameter to BuildTupleHashTableExt()
to allow the slot type to be passed in.  In the case of nodeSubplan.c
and nodeAgg.c the slot type is always &TTSOpsVirtual, so for both of
those cases, it's beneficial to pass the known slot type as that allows
ExecBuildHash32FromAttrs() to skip adding the tuple deform step to the
resulting ExprState.  Another possible fix would have been to have
ExecBuildHash32FromAttrs() set "fetch.kind" to NULL so that
ExecComputeSlotInfo() always determines the EEOP_INNER_FETCHSOME is
required, however, that option isn't favorable as slows down aggregation
and hashed subplan evaluation due to the extra (needless) deform step.

Thanks to Nathan Bossart for bisecting to find the offending commit
based on Paul's report.

Reported-by: Paul Ramsey <pramsey@cleverelephant.ca>
Discussion: https://postgr.es/m/99F064C1-B3EB-4BE7-97D2-D2A0AA487A71@cleverelephant.ca

4 months agoAccommodate very large dshash tables.
Nathan Bossart [Tue, 17 Dec 2024 21:24:45 +0000 (15:24 -0600)]
Accommodate very large dshash tables.

If a dshash table grows very large (e.g., the dshash table for
cumulative statistics when there are millions of tables), resizing
it may fail with an error like:

ERROR: invalid DSA memory alloc request size 1073741824

To fix, permit dshash resizing to allocate more than 1 GB by
providing the DSA_ALLOC_HUGE flag.

Reported-by: Andreas Scherbaum
Author: Matthias van de Meent
Reviewed-by: Cédric Villemain, Michael Paquier, Andres Freund
Discussion: https://postgr.es/m/80a12d59-0d5e-4c54-866c-e69cd6536471%40pgug.de
Backpatch-through: 13

4 months agoSkip useless calculation of join RTE column names during EXPLAIN.
Tom Lane [Tue, 17 Dec 2024 20:52:05 +0000 (15:52 -0500)]
Skip useless calculation of join RTE column names during EXPLAIN.

There's no need for set_simple_column_names() to compute unique
column names for join RTEs, because a finished plan tree will
not contain any join alias Vars that we could need names for.
Its other, internal callers will not pass it any join RTEs
anyway, so the upshot is we can just skip join RTEs here.

Aside from getting rid of a klugy against-its-documentation use of
set_relation_column_names, this can speed up EXPLAIN substantially
when considering many-join queries, because the upper join RTEs
tend to have a lot of columns.

Sami Imseih, with cosmetic changes by me

Discussion: https://postgr.es/m/CAA5RZ0th3q-0p1pri58z9grG8r8azmEBa8o1rtkwhLmJg_cH+g@mail.gmail.com

4 months agoCount pages set all-visible and all-frozen in VM during vacuum
Melanie Plageman [Tue, 17 Dec 2024 19:13:27 +0000 (14:13 -0500)]
Count pages set all-visible and all-frozen in VM during vacuum

Heap vacuum already counts and logs pages with newly frozen tuples. Now
count and log the number of pages newly set all-visible and all-frozen
in the visibility map.

Pages that are all-visible but not all-frozen are debt for future
aggressive vacuums. The counts of newly all-visible and all-frozen pages
give us insight into the rate at which this debt is being accrued and
paid down.

Author: Melanie Plageman
Reviewed-by: Masahiko Sawada, Alastair Turner, Nitin Jadhav, Andres Freund, Bilal Yavuz, Tomas Vondra
Discussion: https://postgr.es/m/flat/CAAKRu_ZQe26xdvAqo4weHLR%3DivQ8J4xrSfDDD8uXnh-O-6P6Lg%40mail.gmail.com#6d8d2b4219394f774889509bf3bdc13d,
https://postgr.es/m/ctdjzroezaxmiyah3gwbwm67defsrwj2b5fpfs4ku6msfpxeia%40mwjyqlhwr2wu

4 months agoMake visibilitymap_set() return previous state of vmbits
Melanie Plageman [Tue, 17 Dec 2024 19:13:18 +0000 (14:13 -0500)]
Make visibilitymap_set() return previous state of vmbits

It can be useful to know the state of a relation page's VM bits before
visibilitymap_set(). visibilitymap_set() has the old value on hand, so
returning it is simple. This commit does not use visibilitymap_set()'s
new return value.

Author: Melanie Plageman
Reviewed-by: Masahiko Sawada, Andres Freund, Nitin Jadhav, Bilal Yavuz
Discussion: https://postgr.es/m/flat/CAAKRu_ZQe26xdvAqo4weHLR%3DivQ8J4xrSfDDD8uXnh-O-6P6Lg%40mail.gmail.com#6d8d2b4219394f774889509bf3bdc13d,
https://postgr.es/m/ctdjzroezaxmiyah3gwbwm67defsrwj2b5fpfs4ku6msfpxeia%40mwjyqlhwr2wu

4 months agoRename LVRelState->frozen_pages
Melanie Plageman [Tue, 17 Dec 2024 19:13:00 +0000 (14:13 -0500)]
Rename LVRelState->frozen_pages

Rename frozen_pages to new_frozen_tuple_pages in LVRelState, the struct
used for tracking state during vacuuming of a heap relation.
frozen_pages sounds like it tracks pages set all-frozen. That is a
misnomer. It only includes pages with at least one newly frozen tuple.
It also includes pages that are not all-frozen.

Author: Melanie Plageman
Reviewed-by: Andres Freund, Masahiko Sawada, Nitin Jadhav, Bilal Yavuz
Discussion: https://postgr.es/m/ctdjzroezaxmiyah3gwbwm67defsrwj2b5fpfs4ku6msfpxeia%40mwjyqlhwr2wu

4 months agoSet max_safe_fds whenever we create shared memory and semaphores.
Tom Lane [Tue, 17 Dec 2024 17:23:26 +0000 (12:23 -0500)]
Set max_safe_fds whenever we create shared memory and semaphores.

Formerly we skipped this in bootstrap/check mode and in single-user
mode.  That's bad in check mode because it may allow accepting a
value of max_connections that doesn't actually work: on platforms
where semaphores consume file descriptors, there may not be enough
free FDs left over to satisfy fd.c, causing postmaster start to
fail.  It's also not great in single-user mode, because fd.c will
operate with just the minimum allowable value of max_safe_fds,
resulting in excess file open/close overhead if anything moderately
complicated is done in single-user mode.  (There may be some penalty
for bootstrap mode too, though probably not much.)

Discussion: https://postgr.es/m/2081982.1734393311@sss.pgh.pa.us