postgresql.git
5 months agoSimplify executor's determination of whether to use parallelism.
Tom Lane [Mon, 9 Dec 2024 19:38:19 +0000 (14:38 -0500)]
Simplify executor's determination of whether to use parallelism.

Our parallel-mode code only works when we are executing a query
in full, so ExecutePlan must disable parallel mode when it is
asked to do partial execution.  The previous logic for this
involved passing down a flag (variously named execute_once or
run_once) from callers of ExecutorRun or PortalRun.  This is
overcomplicated, and unsurprisingly some of the callers didn't
get it right, since it requires keeping state that not all of
them have handy; not to mention that the requirements for it were
undocumented.  That led to assertion failures in some corner
cases.  The only state we really need for this is the existing
QueryDesc.already_executed flag, so let's just put all the
responsibility in ExecutePlan.  (It could have been done in
ExecutorRun too, leading to a slightly shorter  -- but if
there's ever more than one caller of ExecutePlan, it seems better
to have this logic in the subroutine than the callers.)

This makes those ExecutorRun/PortalRun parameters unnecessary.
In master it seems okay to just remove them, returning the
API for those functions to what it was before parallelism.
Such an API break is clearly not okay in stable branches,
but for them we can just leave the parameters in place after
documenting that they do nothing.

Per report from Yugo Nagata, who also reviewed and tested
this .  Back- to all supported branches.

Discussion: https://postgr.es/m/20241206062549.710dc01cf91224809dd6c0e1@sraoss.co.jp

5 months agoRemove remants of "snapshot too old"
Heikki Linnakangas [Mon, 9 Dec 2024 16:13:03 +0000 (18:13 +0200)]
Remove remants of "snapshot too old"

Remove the 'whenTaken' and 'lsn' fields from SnapshotData. After the
removal of the "snapshot too old" feature, they were never set to a
non-zero value.

This largely reverts commit 3e2f3c2e423, which added the
OldestActiveSnapshot tracking, and the init_toast_snapshot()
function. That was only required for setting the 'whenTaken' and 'lsn'
fields. SnapshotToast is now a constant again, like SnapshotSelf and
SnapshotAny. I kept a thin get_toast_snapshot() wrapper around
SnapshotToast though, to check that you have a registered or active
snapshot. That's still a useful sanity check.

Reviewed-by: Nathan Bossart, Andres Freund, Tom Lane
Discussion: https://www.postgresql.org/message-id/cd4b4f8c-e63a-41c0-95f6-6e6cd9b83f6d@iki.fi

5 months agoAvoid unnecessary wrapping for Vars and PHVs
Richard Guo [Mon, 9 Dec 2024 11:38:22 +0000 (20:38 +0900)]
Avoid unnecessary wrapping for Vars and PHVs

When pulling up a lateral subquery that is under an outer join, the
current code always wraps a Var or PHV in the subquery's targetlist
into a new PlaceHolderVar if it is a lateral reference to something
outside the subquery.  This is necessary when the Var/PHV references
the non-nullable side of the outer join from the nullable side: we
need to ensure that it is evaluated at the right place and hence is
forced to null when the outer join should do so.  However, if the
referenced rel is under the same lowest nulling outer join, we can
actually omit the wrapping.  That's safe because if the subquery
variable is forced to NULL by the outer join, the lateral reference
variable will come out as NULL too.  It could be beneficial to get rid
of such PHVs because they imply lateral dependencies, which force us
to resort to nestloop joins.

This  leverages the newly introduced nullingrel_info to check if
the nullingrels of the subquery RTE are a subset of those of the
laterally referenced rel, in order to determine if the referenced rel
is under the same lowest nulling outer join.

No back as this could result in plan changes.

Author: Richard Guo
Reviewed-by: James Coleman, Dmitry Dolgov, Andrei Lepikhov
Discussion: https://postgr.es/m/CAMbWs48uk6C7Z9m_FNT8_21CMCk68hrgAsz=z6zpP1PNZMkeoQ@mail.gmail.com

5 months agoFix right-semi-joins in HashJoin rescans
Richard Guo [Mon, 9 Dec 2024 11:36:23 +0000 (20:36 +0900)]
Fix right-semi-joins in HashJoin rescans

When resetting a HashJoin node for rescans, if it is a single-batch
join and there are no parameter changes for the inner subnode, we can
just reuse the existing hash table without rebuilding it.  However,
for join types that depend on the inner-tuple match flags in the hash
table, we need to reset these match flags to avoid incorrect results.
This applies to right, right-anti, right-semi, and full joins.

When I introduced "Right Semi Join" plan shapes in aa86129e1, I failed
to reset the match flags in the hash table for right-semi joins in
rescans.  This oversight has been shown to produce incorrect results.
This  fixes it.

Author: Richard Guo
Discussion: https://postgr.es/m/CAMbWs4-nQF9io2WL2SkD0eXvfPdyBc9Q=hRwfQHCGV2usa0jyA@mail.gmail.com

5 months agoFix memory in pgoutput with publication list cache
Michael Paquier [Mon, 9 Dec 2024 07:41:46 +0000 (16:41 +0900)]
Fix memory  in pgoutput with publication list cache

The pgoutput module caches publication names in a list and frees it upon
invalidation.  However, the code forgot to free the actual publication
names within the list elements, as publication names are pstrdup()'d in
GetPublication().  This would cause memory to  in
CacheMemoryContext, bloating it over time as this context is not
cleaned.

This is a problem for WAL senders running for a long time, as an
accumulation of invalidation requests would bloat its cache memory
usage.  A second case, where this  is easier to see, involves a
backend calling SQL functions like pg_logical_slot_{get,peek}_changes()
which create a new decoding context with each execution.  More
publications create more bloat.

To address this, this commit adds a new memory context within the
logical decoding context and resets it each time the publication names
cache is invalidated, based on a suggestion from Amit Kapila.  This
ensures that the lifespan of the publication names aligns with that of
the logical decoding context.

This solution changes PGOutputData, which is fine for HEAD but it could
cause an ABI breakage in stable branches as the structure size would
change, so these are left out for now.

Analyzed-by: Michael Paquier, Jeff Davis
Author: Zhijie Hou
Reviewed-by: Michael Paquier, Masahiko Sawada, Euler Taveira
Discussion: https://postgr.es/m/[email protected]

5 months agoImprove comment about dropped entries in pgstat.c
Michael Paquier [Mon, 9 Dec 2024 05:35:39 +0000 (14:35 +0900)]
Improve comment about dropped entries in pgstat.c

pgstat_write_statsfile() discards any entries marked as dropped from
being written to the stats file at shutdown, and also included an
assertion based on the same condition.

The intention of the assertion is to track that no pgstats entries
should be left around as terminating backends should drop any entries
they still hold references on before the stats file is written by the
checkpointer, and it not worth taking down the server in this case if
there is a bug making that possible.

Let's improve the comment of this area to document clearly what's
intended.

Based on a discussion with Bertrand Drouvot and Anton A. Melnikov.

Author: Bertrand Drouvot
Discussion: https://postgr.es/m/a13e8cdf-b97a-4ecb-8f42-aaa367974e29@postgrespro.ru
Back-through: 15

5 months agoImprove the error message introduced in commit 87ce27de696.
Amit Kapila [Mon, 9 Dec 2024 03:41:45 +0000 (09:11 +0530)]
Improve the error message introduced in commit 87ce27de696.

The error detail message "Replica identity consists of an unpublished
generated column." implies that the entire replica identity is made up of
an unpublished generated column which may not be the case.

Reported-by: Peter Smith
Author: Shlok Kyal
Reviewed-by: Peter Smith, Amit Kapila
Discussion: https://postgr.es/m/CAHut+PuwMhKx0PhOA4APhJTLoBGNykbeCQpr_CuwGT-SkswG5w@mail.gmail.com

5 months agoFix invalidation of local pgstats references for entry reinitialization
Michael Paquier [Mon, 9 Dec 2024 01:45:28 +0000 (10:45 +0900)]
Fix invalidation of local pgstats references for entry reinitialization

818119afccd3 has introduced the "generation" concept in pgstats entries,
incremented a counter when a pgstats entry is reinitialized, but it did
not count on the fact that backends still holding local references to
such entries need to be refreshed if the cache age is outdated.  The
previous logic only updated local references when an entry was dropped,
but it needs also to consider entries that are reinitialized.

This matters for replication slot stats (as well as custom pgstats kinds
in 18~), where concurrent drops and creates of a slot could cause
incorrect stats to be locally referenced.  This would lead to an
assertion failure at shutdown when writing out the stats file, as the
backend holding an outdated local reference would not be able to drop
during its shutdown sequence the stats entry that should be dropped, as
the last process holding a reference to the stats entry.  The
checkpointer was then complaining about such an entry late in the
shutdown sequence, after the shutdown checkpoint is finished with the
control file updated, causing the stats file to not be generated.  In
non-assert builds, the entry would just be skipped with the stats file
written.

Note that only logical replication slots use statistics.

A test case based on TAP is added to test_decoding, where a persistent
connection peeking at a slot's data is kept with concurrent drops and
creates of the same slot.  This is based on the isolation test case that
Anton has sent.  As it requires a node shutdown with a check to make
sure that the stats file is written with this specific sequence of
events, TAP is used instead.

Reported-by: Anton A. Melnikov
Reviewed-by: Bertrand Drouvot
Discussion: https://postgr.es/m/56bf8ff9-dd8c-47b2-872a-748ede82af99@postgrespro.ru
Back-through: 15

5 months agoFix possible crash during WindowAgg evaluation
David Rowley [Mon, 9 Dec 2024 01:23:21 +0000 (14:23 +1300)]
Fix possible crash during WindowAgg evaluation

When short-circuiting WindowAgg node evaluation on the top-level
WindowAgg node using quals on monotonic window functions, because the
WindowAgg run condition can mean there's no need to evaluate subsequent
window function results in the same partition once the run condition
becomes false, it was possible that the executor would use stale results
from the previous invocation of the window function in some cases.

A fix for this was partially done by a5832722, but that commit only
fixed the issue for non-top-level WindowAgg nodes.  I mistakenly thought
that the top-level WindowAgg didn't have this issue, but Jayesh's example
case clearly shows that's incorrect.  At the time, I also thought that
this only affected 32-bit systems as all window functions which then
supported run conditions returned BIGINT, however, that's wrong as
ExecProject is still called and that could cause evaluation of any other
window function belonging to the same WindowAgg node, one of which may
return a byref type.

The only queries affected by this are WindowAggs with a "Run Condition"
which contains at least one window function with a byref result type,
such as lead() or lag() on a byref column.  The window clause must also
contain a PARTITION BY clause (without a PARTITION BY, execution of the
WindowAgg stops immediately when the run condition becomes false and
there's no risk of using the stale results).

Reported-by: Jayesh Dehankar
Discussion: https://postgr.es/m/193261e2c4d.3dd3cd7c1842.871636075166132237@zohocorp.com
Back-through: 15, where WindowAgg run conditions were added

5 months agoEnsure that pg_amop/amproc entries depend on their lefttype/righttype.
Tom Lane [Sat, 7 Dec 2024 20:56:28 +0000 (15:56 -0500)]
Ensure that pg_amop/amproc entries depend on their lefttype/righttype.

Usually an entry in pg_amop or pg_amproc does not need a dependency on
its amoplefttype/amoprighttype/amproclefttype/amprocrighttype types,
because there is an indirect dependency via the argument types of its
referenced operator or procedure, or via the opclass it belongs to.
However, for some support procedures in some index AMs, the argument
types of the support procedure might not mention the column data type
at all.  Also, the amop/amproc entry might be treated as "loose" in
the opfamily, in which case it lacks a dependency on any particular
opclass; or it might be a cross-type entry having a reference to a
datatype that is not its opclass' opcintype.

The upshot of all this is that there are cases where a datatype can
be dropped while leaving behind amop/amproc entries that mention it,
because there is no path in pg_depend showing that those entries
depend on that type.  Such entries are harmless in normal activity,
because they won't get used, but they cause problems for maintenance
actions such as dropping the operator family.  They also cause pg_dump
to produce bogus output.  The previous commit put a band-aid on the
DROP OPERATOR FAMILY failure, but a real fix is needed.

To fix, add pg_depend entries showing that a pg_amop/pg_amproc entry
depends on its lefttype/righttype.  To avoid bloating pg_depend too
much, skip this if the referenced operator or function has that type
as an input type.  (I did not bother with considering the possible
indirect dependency via the opclass' opcintype; at least in the
reported case, that wouldn't help anyway.)

Probably, the reason this has escaped notice for so long is that
add-on datatypes and relevant opclasses/opfamilies are usually
packaged as extensions nowadays, so that there's no way to drop
a type without dropping the referencing opclasses/opfamilies too.
Still, in the absence of pg_depend entries there's nothing that
constrains DROP EXTENSION to drop the opfamily entries before the
datatype, so it seems possible for a DROP failure to occur anyway.

The specific case that was reported doesn't fail in v13, because
v13 prefers to attach the support procedure to the opclass not the
opfamily.  But it's surely possible to construct other edge cases
that do fail in v13, so  that too.

Per report from Yoran Heling.  Back- to all supported branches.

Discussion: https://postgr.es/m/Z1MVCOh1hprjK5Sf@gmai021

5 months agoMake getObjectDescription robust against dangling amproc type links.
Tom Lane [Sat, 7 Dec 2024 19:28:16 +0000 (14:28 -0500)]
Make getObjectDescription robust against dangling amproc type links.

Yoran Heling reported a case where a data type could be dropped
while references to its OID remain behind in pg_amproc.  This
causes getObjectDescription to fail, which blocks dropping the
operator family (since our DROP code likes to construct descriptions
of everything it's dropping).  The proper fix for this requires
adding more pg_depend entries.  But to allow DROP to go through with
already-corrupt catalogs, tweak getObjectDescription to print "???"
for the type instead of failing when it processes such an entry.

I changed the logic for pg_amop similarly, for consistency,
although it is not known that the problem can manifest in pg_amop.

Per report from Yoran Heling.  Back- to all supported
branches (although the problem may be unreachable in v13).

Discussion: https://postgr.es/m/Z1MVCOh1hprjK5Sf@gmai021

5 months agoFix is_digit labeling of to_timestamp's FFn format codes.
Tom Lane [Sat, 7 Dec 2024 18:12:32 +0000 (13:12 -0500)]
Fix is_digit labeling of to_timestamp's FFn format codes.

These format codes produce or consume strings of digits, so they
should be labeled with is_digit = true, but they were not.
This has effect in only one place, where is_next_separator()
is checked to see if the preceding format code should slurp up
all the available digits.  Thus, with a format such as '...SSFF3'
with remaining input '12345', the 'SS' code would consume all
five digits (and then complain about seconds being out of range)
when it should eat only two digits.

Per report from Nick Davies.  This bug goes back to d589f9446
where the FFn codes were introduced, so back- to v13.

Discussion: https://postgr.es/m/AM8PR08MB6356AC979252CFEA78B56678B6312@AM8PR08MB6356.eurprd08.prod.outlook.com

5 months agodoc: remove LC_COLLATE and LC_CTYPE from SHOW command
Peter Eisentraut [Sat, 7 Dec 2024 11:55:55 +0000 (12:55 +0100)]
doc: remove LC_COLLATE and LC_CTYPE from SHOW command

The corresponding read-only server settings have been removed since
in PG16. See commit b0f6c437160db6.

Author: Pierre Giraud <[email protected]>
Discussion: https://www.postgresql.org/message-id/flat/a75a2fb0-f4b3-4c0c-be3d-7a62d266d760%40dalibo.com

5 months agoComment fix: "buffer context lock" to "buffer content lock".
Jeff Davis [Fri, 6 Dec 2024 17:59:12 +0000 (09:59 -0800)]
Comment fix: "buffer context lock" to "buffer content lock".

The term "buffer context lock" is outdated as of commit 5d5087363d.

5 months agoRemove useless casts to (const void *)
Peter Eisentraut [Fri, 6 Dec 2024 16:22:19 +0000 (17:22 +0100)]
Remove useless casts to (const void *)

Similar to commit 7f798aca1d5, but I didn't think to look for "const"
as well.

5 months agoFix printf format string warning on MinGW.
Thomas Munro [Thu, 5 Dec 2024 23:34:33 +0000 (12:34 +1300)]
Fix printf format string warning on MinGW.

Commit 517bf2d91 changed a printf format string to placate MinGW, which
at the time warned about "%lld".  Current MinGW is now warning about the
replacement "%I64d".  Reverting the change clears the warning on the
MinGW CI task, and hopefully it will clear it on build farm animal
fairywren too.

Reviewed-by: Tom Lane <[email protected]>
Reported-by: "Hayato Kuroda (Fujitsu)" <[email protected]>
Discussion: https://postgr.es/m/TYAPR01MB5866A71B744BE01B3BF71791F5AEA%40TYAPR01MB5866.jpnprd01.prod.outlook.com

5 months agoRemove pg_regex_collation
Peter Eisentraut [Thu, 5 Dec 2024 06:19:37 +0000 (07:19 +0100)]
Remove pg_regex_collation

We can also use the existing pg_regex_locale as the cache key, which
is the only use of this variable.

Reviewed-by: Jeff Davis <[email protected]>
Discussion: https://www.postgresql.org/message-id/flat/b1b92ae1-2e06-4619-a87a-4b4858e547ec%40eisentraut.org

5 months agoFix header inclusion order in c.h.
Thomas Munro [Thu, 5 Dec 2024 01:27:35 +0000 (14:27 +1300)]
Fix header inclusion order in c.h.

Commit 962da900a added #include <stdint.h> to postgres_ext.h, which
broke c.h's header ordering rule.

The system headers on some systems would then lock down off_t's size in
private macros, before they'd had a chance to see our definition of
_FILE_OFFSET_BITS (and presumably other things).  This was picked up by
perl's ABI compatibility checks on some 32 bit systems in the build
farm.

Move #include "postgres_ext.h" down below the system header section, and
make the comments clearer (thanks to Tom for the new wording).

Diagnosed-by: Tom Lane <[email protected]>
Discussion: https://postgr.es/m/2397643.1733347237%40sss.pgh.pa.us

5 months agoProvide a better error message for misplaced dis options.
Nathan Bossart [Wed, 4 Dec 2024 21:04:15 +0000 (15:04 -0600)]
Provide a better error message for misplaced dis options.

Before this , misplacing a special must-be-first option for
dising to a subprogram (e.g., postgres -D . --single) would
fail with an error like

FATAL:  --single requires a value

This  adjusts this error to more accurately complain that the
special option wasn't listed first.  The aforementioned error
message now looks like

FATAL:  --single must be first argument

The dis option parsing code has been refactored for use
wherever ParseLongOption() is called.  Beyond the obvious advantage
of avoiding code duplication, this should prevent similar problems
when new dis options are added.  Note that we assume that none
of the dis option names match another valid command-line
argument, such as the name of a configuration parameter.

Ideally, we'd remove this must-be-first requirement for these
options, but after some investigation, we decided that wasn't worth
the added complexity and behavior changes.

Author: Nathan Bossart, Greg Sabino Mullane
Reviewed-by: Greg Sabino Mullane, Peter Eisentraut, Álvaro Herrera, Tom Lane
Discussion: https://postgr.es/m/CAKAnmmJkZtZAiSryho%3DgYpbvC7H-HNjEDAh16F3SoC9LPu8rqQ%40mail.gmail.com

5 months agoReturn actual error code from FOP failure in PDF build
Bruce Momjian [Wed, 4 Dec 2024 19:37:24 +0000 (14:37 -0500)]
Return actual error code from FOP failure in PDF build

Previously we returned "1" on error.  Improvement on 77c189cdafe.

Back-through: master

5 months agoFix dead code
Peter Eisentraut [Wed, 4 Dec 2024 15:43:07 +0000 (16:43 +0100)]
Fix dead code

from commit 85b7efa1cdd

per Coverity report

5 months agoFix use-after-free in parallel_vacuum_reset_dead_items
John Naylor [Wed, 4 Dec 2024 09:51:55 +0000 (16:51 +0700)]
Fix use-after-free in parallel_vacuum_reset_dead_items

parallel_vacuum_reset_dead_items used a local variable to hold a
pointer from the passed vacrel, purely as a shorthand. This pointer
was later freed and a new allocation was made and stored to the
struct. Then the local pointer was mistakenly referenced again.

This apparently happened not to break anything since the freed chunk
would have been put on the context's freelist, so it was accidentally
the same pointer anyway, in which case the DSA handle was correctly
updated. The minimal fix is to change two places so they access
dead_items through the vacrel. This coding style is a maintenance
hazard, so while at it get rid of most other similar usages, which
were inconsistently used anyway.

Analysis and  by Vallimaharajan G, with further defensive coding
by me

Backpath to v17, when TidStore came in

Discussion: https://postgr.es/m/1936493cc38.68cb2ef27266.7456585136086197135@zohocorp.com

5 months agoSimplify IsIndexUsableForReplicaIdentityFull()
Peter Eisentraut [Wed, 4 Dec 2024 07:33:28 +0000 (08:33 +0100)]
Simplify IsIndexUsableForReplicaIdentityFull()

Take Relation as argument instead of IndexInfo.  Building the
IndexInfo is an unnecessary intermediate step here.

A future  wants to get some information that is in the relcache
but not in IndexInfo, so this will also help there.

Discussion: https://www.postgresql.org/message-id/333d3886-b737-45c3-93f4-594c96bb405d@eisentraut.org

5 months agoEnsure stored generated columns must be published when required.
Amit Kapila [Wed, 4 Dec 2024 04:15:18 +0000 (09:45 +0530)]
Ensure stored generated columns must be published when required.

Ensure stored generated columns that are part of REPLICA IDENTITY must be
published explicitly for UPDATE and DELETE operations to be published. We
can publish generated columns by listing them in the column list or by
enabling the publish_generated_columns option.

This commit changes the behavior of the test added in commit adedf54e65 by
giving an ERROR for the UPDATE operation in such cases. There is no way to
trigger the bug reported in commit adedf54e65 but we didn't remove the
corresponding code change because it is still relevant when replicating
changes from a publisher with version less than 18.

We decided not to back this behavior change to avoid the risk of
breaking existing output plugins that may be sending generated columns by
default although we are not aware of any such plugin. Also, we didn't see
any reports related to this on STABLE branches which is another reason not
to back this change.

Author: Shlok Kyal, Hou Zhijie
Reviewed-by: Vignesh C, Amit Kapila
Discussion: https://postgr.es/m/CANhcyEVw4V2Awe2AB6i0E5AJLNdASShGfdBLbUd1XtWDboymCA@mail.gmail.com

5 months agoProperly use $(AWK) in Makefile, not 'awk'
Bruce Momjian [Wed, 4 Dec 2024 03:31:12 +0000 (22:31 -0500)]
Properly use $(AWK) in Makefile, not 'awk'

Fix for commit 498f1307569.

Back-through: master

5 months agoUse <stdint.h> and <inttypes.h> for c.h integers.
Thomas Munro [Wed, 4 Dec 2024 01:46:59 +0000 (14:46 +1300)]
Use <stdint.h> and <inttypes.h> for c.h integers.

Redefine our exact width types with standard C99 types and macros,
including int64_t, INT64_MAX, INT64_C(), PRId64 etc.  We were already
using <stdint.h> types in a few places.

One complication is that Windows' <inttypes.h> uses format strings like
"%I64d", "%I32", "%I" for PRI*64, PRI*32, PTR*PTR, instead of mapping to
other standardized format strings like "%lld" etc as seen on other known
systems.  Teach our snprintf.c to understand them.

This removes a lot of configure clutter, and should also allow 64-bit
numbers and other standard types to be used in localized messages
without casting.

Reviewed-by: Peter Eisentraut <[email protected]>
Discussion: https://postgr.es/m/ME3P282MB3166F9D1F71F787929C0C7E7B6312%40ME3P282MB3166.AUSP282.PROD.OUTLOOK.COM

5 months agoDefine __EXTENSIONS__ on Solaris, too.
Tom Lane [Wed, 4 Dec 2024 01:21:23 +0000 (20:21 -0500)]
Define __EXTENSIONS__ on Solaris, too.

Apparently, if you define _POSIX_C_SOURCE on Solaris,
that's interpreted as "you get ONLY what's defined by POSIX".
Results from BF member hake show that that breaks perl.h,
and doubtless it'd cause more problems if we got past that.
Adopt the suggestion from standards(7) that we also need to
define __EXTENSIONS__, in hopes of un-breaking things.

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

5 months agoFix Makefile so invalid characters warning preserves error code
Bruce Momjian [Tue, 3 Dec 2024 23:27:41 +0000 (18:27 -0500)]
Fix Makefile so invalid characters warning preserves error code

Fix for commit e4c8865196f.

Reported-by: Peter Eisentraut
Discussion: https://postgr.es/m/88cb6ecf-22bb-431e-974b-1cd236a80364@eisentraut.org

Back-through: master

5 months agoNow that we have non-Latin1 SGML detection, restore Latin1 chars
Bruce Momjian [Tue, 3 Dec 2024 22:09:49 +0000 (17:09 -0500)]
Now that we have non-Latin1 SGML detection, restore Latin1 chars

This reverts the change in commit 641a5b7a144 that converted them to
HTML entities.

Reported-by: Peter Eisentraut
Discussion: https://postgr.es/m/[email protected]

Back-through: master

5 months agoMove check for ucol_strcollUTF8 to pg_locale_icu.c
Jeff Davis [Tue, 3 Dec 2024 19:32:14 +0000 (11:32 -0800)]
Move check for ucol_strcollUTF8 to pg_locale_icu.c

The result of the check is only used by pg_locale_icu.c.

Author: Andreas Karlsson
Discussion: https://postgr.es/m/4548a168-62cd-457b-8d06-9ba7b985c477@proxel.se

5 months agoDefine _POSIX_C_SOURCE as 200112L on Solaris.
Tom Lane [Tue, 3 Dec 2024 17:44:43 +0000 (12:44 -0500)]
Define _POSIX_C_SOURCE as 200112L on Solaris.

This is an attempt to suppress some compiler warnings that appeared in
the wake of commit 7f798aca1: it seems that by default Solaris/illumos
declares shmdt() to take "char *" not "void *".  We'd like the system
headers to provide modern POSIX APIs, and POSIX 2001 seems to be as
modern as is available there.

illumos' standards(7) man page suggests that we might also need to
define __EXTENSIONS__, but let's see what happens with just this.

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

5 months agoFix synchronized_standby_slots GUC check hook
Álvaro Herrera [Tue, 3 Dec 2024 16:50:57 +0000 (17:50 +0100)]
Fix synchronized_standby_slots GUC check hook

The validate_sync_standby_slots subroutine requires an LWLock, so it
cannot run in processes without PGPROC; skip it there to avoid a crash.

This replaces the current test for ReplicationSlotCtl being not null,
which appears to be a solution for the same problem but less general.
I also rewrote a related comment that mentioned ReplicationSlotCtl in
StandbySlotsHaveCaughtup.

This code came in with commit bf279ddd1c28; back to 17.

Reported-by: Gabriele Bartolini <[email protected]>
Reviewed-by: Amit Kapila <[email protected]>
Reviewed-by: Zhijie Hou <[email protected]>
Discussion: https://postgr.es/m/202411281216[email protected]

5 months agoDrop "Lock" suffix from LWLock wait event names
Álvaro Herrera [Tue, 3 Dec 2024 14:50:03 +0000 (15:50 +0100)]
Drop "Lock" suffix from LWLock wait event names

Commit da952b415f44 unintentially reverted the SQL-visible part of
commit 14a910109126, which breaks queries joining pg_wait_events with
pg_stat_acivity.  Remove the suffix again.

Back to 17.

Reported-by: Christophe Courtois <[email protected]>
Author: Bertrand Drouvot <[email protected]>
Discussion: https://postgr.es/m/18728-450924477056a339%40postgresql.org
Discussion: https://postgr.es/m/[email protected]

5 months agoUpdate obsolete comment
Álvaro Herrera [Tue, 3 Dec 2024 13:46:31 +0000 (14:46 +0100)]
Update obsolete comment

Commit 3aa0395d4ed3 made worrying about BKI_ROWTYPE_OID matching no
longer necessary, but this comment didn't get the memo.

5 months agoFix handling of CREATE DOMAIN with GENERATED constraint syntax
Peter Eisentraut [Tue, 3 Dec 2024 13:32:45 +0000 (14:32 +0100)]
Fix handling of CREATE DOMAIN with GENERATED constraint syntax

Stuff like

    CREATE DOMAIN foo AS int CONSTRAINT cc GENERATED ALWAYS AS (2) STORED

is not supported for domains, but the parser allows it, because it's
the same syntax as for table constraints.  But CreateDomain() did not
explicitly handle all ConstrType values, so the above would get an
internal error like

    ERROR:  unrecognized constraint subtype: 4

Fix that by providing a user-facing error message for all ConstrType
values.  Also, remove the switch default case, so future additions to
ConstrType are caught.

Reported-by: Jian He <[email protected]>
Discussion: https://www.postgresql.org/message-id/CACJufxF8fmM=Dbm4pDFuV_nKGz2-No0k4YifhrF3-rjXTWJM3w@mail.gmail.com

5 months agoFix temporary memory in system table index scans
Peter Eisentraut [Tue, 3 Dec 2024 08:04:20 +0000 (09:04 +0100)]
Fix temporary memory  in system table index scans

Commit 811af9786b introduced palloc() calls into systable_beginscan()
and systable_beginscan_ordered().  But there was no pfree(), as is the
usual style.

It turns out that an ANALYZE of a partitioned table can invoke many
thousand system table index scans, and this memory is not cleaned up
until the end of the command, so this can temporarily  quite a bit
of memory.  Maybe there are improvements to be made at a higher level
about this, but for now, insert a couple of corresponding pfree()
calls to fix this particular issue.

Reported-by: Justin Pryzby <[email protected]>
Discussion: https://www.postgresql.org/message-id/Z0XTfIq5xUtbkiIh@pryzbyj2023

5 months agoPerform provider-specific initialization in new functions.
Jeff Davis [Tue, 3 Dec 2024 07:20:32 +0000 (23:20 -0800)]
Perform provider-specific initialization in new functions.

Reviewed-by: Andreas Karlsson
Discussion: https://postgr.es/m/4548a168-62cd-457b-8d06-9ba7b985c477@proxel.se

5 months agodoc: Clarify some terms for pg_createsubscriber
Michael Paquier [Tue, 3 Dec 2024 07:21:07 +0000 (16:21 +0900)]
doc: Clarify some terms for pg_createsubscriber

The last section of pg_createsubscriber used the terms
"publication-name", "replication-slot-name", and "subscription-name".
These terms are not defined on the page, which was confusing, and the
intention is clearly to refer to the values one would give to the
options --publication, --subscription and --replication-slot.  Let's
simplify the documentation by mentioning the option switches, instead of
these terms.

Reported-by: Christophe Courtois
Author: Shubham Khanna
Reviewed-by: Vignesh C, Peter Smith
Discussion: https://postgr.es/m/173288198026.714.15127074046508836738@wrigleys.postgresql.org
Back-through: 17

5 months agoFix unintentional behavior change in commit e9931bfb75.
Jeff Davis [Tue, 3 Dec 2024 05:59:02 +0000 (21:59 -0800)]
Fix unintentional behavior change in commit e9931bfb75.

Prior to that commit, there was special case to use ASCII case mapping
behavior for the libc provider with a single-byte encoding when that's
the default collation. Commit e9931bfb75 mistakenly eliminated that
special case; this commit restores it.

Discussion: https://postgr.es/m/01a104f0d2179d756261e90d96fd65c36ad6fcf0[email protected]

5 months agoRevert "Introduce CompactAttribute array in TupleDesc"
David Rowley [Tue, 3 Dec 2024 04:12:38 +0000 (17:12 +1300)]
Revert "Introduce CompactAttribute array in TupleDesc"

This reverts commit d28dff3f6cd6a7562fb2c211ac0fb74a33ffd032.

Quite a large number of buildfarm members didn't like this commit and
it's not yet clear why.  Reverting this before too many animals turn
red.

Discussion: https://postgr.es/m/CAApHDvr9i6T5=iAwQCxFDgMsthr_obVxgwBaEJkC8KUH6yM3Hw@mail.gmail.com

5 months agoIntroduce CompactAttribute array in TupleDesc
David Rowley [Tue, 3 Dec 2024 03:50:59 +0000 (16:50 +1300)]
Introduce CompactAttribute array in TupleDesc

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.  With this
change, NAMEDATALEN could be increased with a much smaller negative impact
on performance.

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
Discussion: https://postgr.es/m/CAApHDvrBztXP3yx=NKNmo3xwFAFhEdyPnvrDg3=M0RhDs+4vYw@mail.gmail.com
Reviewed-by: Andres Freund, Victor Yegorov
5 months agodoc Makefile: issue warning about chars that cannot be output
Bruce Momjian [Tue, 3 Dec 2024 02:25:12 +0000 (21:25 -0500)]
doc Makefile: issue warning about chars that cannot be output

A follow-up improvement to commit 641a5b7a144.

Reported-by: Tatsuo Ishii, Tom Lane
Discussion: https://postgr.es/m/20241126.182513.1752581942460106099[email protected]

Back-through: master

5 months agoRework some code handling pg_subscription data in psql and pg_dump
Michael Paquier [Tue, 3 Dec 2024 00:48:12 +0000 (09:48 +0900)]
Rework some code handling pg_subscription data in psql and pg_dump

This commit fixes some inconsistencies found in the frontend code when
dealing with subscription catalog data.

The following changes are done:
- pg_subscription.h gains a EXPOSE_TO_CLIENT_CODE, so as more content
defined in pg_subscription.h becomes available in pg_subscription_d.h
for the frontend.
- In psql's describe.c, substream can be switched to use CppAsString2()
with its three LOGICALREP_STREAM_* values, with pg_subscription_d.h
included.
- pg_dump.c included pg_subscription.h, which is a header that should
only be used in the backend code.  The code is updated to use
pg_subscription_d.h instead.
- pg_dump stored all the data from pg_subscription in SubscriptionInfo
with only strings, and a good chunk of them are boolean and char values.
Using strings is not necessary, complicates the code (see for example
two_phase_disabled[] removed here), and is inconsistent with the way
other catalogs' data is handled.  The fields of SubscriptionInfo are
reordered to match with the order in its catalog, while on it.

Reviewed-by: Hayato Kuroda
Discussion: https://postgr.es/m/[email protected]

5 months agoRelationTruncate() must set DELAY_CHKPT_START.
Thomas Munro [Mon, 2 Dec 2024 20:27:05 +0000 (09:27 +1300)]
RelationTruncate() must set DELAY_CHKPT_START.

Previously, it set only DELAY_CHKPT_COMPLETE. That was important,
because it meant that if the XLOG_SMGR_TRUNCATE record preceded a
XLOG_CHECKPOINT_ONLINE record in the WAL, then the truncation would also
happen on disk before the XLOG_CHECKPOINT_ONLINE record was
written.

However, it didn't guarantee that the sync request for the truncation
was processed before the XLOG_CHECKPOINT_ONLINE record was written. By
setting DELAY_CHKPT_START, we guarantee that if an XLOG_SMGR_TRUNCATE
record is written to WAL before the redo pointer of a concurrent
checkpoint, the sync request queued by that operation must be processed
by that checkpoint, rather than being left for the following one.

This is a refinement of commit 412ad7a5563.  Back- to all supported
releases, like that commit.

Author: Robert Haas <[email protected]>
Reported-by: Thomas Munro <[email protected]>
Discussion: https://postgr.es/m/CA%2BhUKG%2B-2rjGZC2kwqr2NMLBcEBp4uf59QT1advbWYF_uc%2B0Aw%40mail.gmail.com

5 months agoDeprecate MD5 passwords.
Nathan Bossart [Mon, 2 Dec 2024 19:30:07 +0000 (13:30 -0600)]
Deprecate MD5 passwords.

MD5 has been considered to be unsuitable for use as a cryptographic
hash algorithm for some time.  Furthermore, MD5 password hashes in
PostgreSQL are vulnerable to pass-the-hash attacks, i.e., knowing
the username and hashed password is sufficient to authenticate.
The SCRAM-SHA-256 method added in v10 is not subject to these
problems and is considered to be superior to MD5.

This commit marks MD5 password support in PostgreSQL as deprecated
and to be removed in a future release.  The documentation now
contains several deprecation notices, and CREATE ROLE and ALTER
ROLE now emit deprecation warnings when setting MD5 passwords.  The
warnings can be disabled by setting the md5_password_warnings
parameter to "off".

Reviewed-by: Greg Sabino Mullane, Jim Nasby
Discussion: https://postgr.es/m/ZwbfpJJol7lDWajL%40nathan

5 months agoAdd a planner support function for numeric generate_series().
Dean Rasheed [Mon, 2 Dec 2024 11:37:57 +0000 (11:37 +0000)]
Add a planner support function for numeric generate_series().

This allows the planner to estimate the number of rows returned by
generate_series(numeric, numeric[, numeric]), when the input values
can be estimated at plan time.

Song Jinzhou, reviewed by Dean Rasheed and David Rowley.

Discussion: https://postgr.es/m/tencent_F43E7F4DD50EF5986D1051DE8DE547910206%40qq.com
Discussion: https://postgr.es/m/tencent_1F6D5B9A1545E02FD7D0EE508DFD056DE50A%40qq.com

5 months agoFix #include order in timestamp.c.
Dean Rasheed [Mon, 2 Dec 2024 11:34:26 +0000 (11:34 +0000)]
Fix #include order in timestamp.c.

Oversight in 036bdcec9f.

5 months agoFix error code for referential action RESTRICT
Peter Eisentraut [Mon, 2 Dec 2024 07:18:36 +0000 (08:18 +0100)]
Fix error code for referential action RESTRICT

According to the SQL standard, if the referential action RESTRICT is
triggered, it has its own error code.  We previously didn't use that,
we just used the error code for foreign key violation.  But RESTRICT
is not necessarily an actual foreign key violation.  The foreign key
might still be satisfied in theory afterwards, but the RESTRICT
setting prevents the action even then.  So it's a separate kind of
error condition.

Discussion: https://www.postgresql.org/message-id/ea5b2777-266a-46fa-852f-6fca6ec480ad@eisentraut.org

5 months agoFix broken list-munging in ecpg's remove_variables().
Tom Lane [Sun, 1 Dec 2024 19:15:37 +0000 (14:15 -0500)]
Fix broken list-munging in ecpg's remove_variables().

The loops over cursor argument variables neglected to ever advance
"prevvar".  The code would accidentally do the right thing anyway
when removing the first or second list entry, but if it had to
remove the third or later entry then it would also remove all
entries between there and the first entry.  AFAICS this would
only matter for cursors that reference out-of-scope variables,
which is a weird Informix compatibility hack; between that and
the lack of impact for short lists, it's not so surprising that
nobody has complained.  Nonetheless it's a pretty obvious bug.

It would have been more obvious if these loops used a more standard
coding style for chasing the linked lists --- this business with the
"prev" pointer sometimes pointing at the current list entry is
confusing and overcomplicated.  So rather than just add a minimal
band-aid, I chose to rewrite the loops in the same style we use
elsewhere, where the "prev" pointer is NULL until we are dealing with
a non-first entry and we save the "next" pointer at the top of the
loop.  (Two of the four loops touched here are not actually buggy,
but it seems better to make them all look alike.)

Coverity discovered this problem, but not until 2b41de4a5 added code
to free no-longer-needed arguments structs.  With that, the incorrect
link updates are possibly touching freed memory, and it complained
about that.  Nonetheless the list corruption hazard is ancient, so
back- to all supported branches.

5 months agoAvoid mislabeling of lateral references, redux.
Tom Lane [Sat, 30 Nov 2024 17:42:19 +0000 (12:42 -0500)]
Avoid mislabeling of lateral references, redux.

As I'd feared, commit 5c9d8636d was still a few bricks shy of a load.
We can't just leave pulled-up lateral-reference Vars with no new
nullingrels: we have to carefully compute what subset of the
to-be-replaced Var's nullingrels apply to them, else we still get
"wrong varnullingrels" errors.  This is a bit tedious, but it looks
like we can use the nullingrel data this  computes for other
purposes, enabling better optimization.  We don't want to inject
unnecessary plan changes into stable branches though, so leave that
idea for a later HEAD-only .

 by me, but thanks to Richard Guo for devising a test case that
broke 5c9d8636d, and for preliminary investigation about how to fix
it.  As before, back- to v16.

Discussion: https://postgr.es/m/[email protected]

5 months agodoc: Fix typo
Peter Eisentraut [Sat, 30 Nov 2024 07:43:46 +0000 (08:43 +0100)]
doc: Fix typo

for commit 1e08905842f

Reported-by: Marcos Pegoraro <[email protected]>
5 months agoSmall indenting fixes in jsonpath_scan.l
Peter Eisentraut [Fri, 29 Nov 2024 10:33:21 +0000 (11:33 +0100)]
Small indenting fixes in jsonpath_scan.l

Some lines were indented by an inconsistent number of spaces.  While
we're here, also fix some code that used the newline after left
parenthesis style, which is obsolete.

5 months agodoc: Improve description of referential actions
Peter Eisentraut [Fri, 29 Nov 2024 07:52:28 +0000 (08:52 +0100)]
doc: Improve description of referential actions

Some of the differences between NO ACTION and RESTRICT were not
explained fully.

Discussion: https://www.postgresql.org/message-id/ea5b2777-266a-46fa-852f-6fca6ec480ad@eisentraut.org

5 months agoAdd tests for foreign keys with case-insensitive collations
Peter Eisentraut [Fri, 29 Nov 2024 07:52:27 +0000 (08:52 +0100)]
Add tests for foreign keys with case-insensitive collations

Some of the behaviors of the different referential actions, such as
the difference between NO ACTION and RESTRICT are best illustrated
using a case-insensitive collation.  So add some tests for that.

(What is actually being tested here is the behavior with values that
are "distinct" (binary different) but compare as equal.  Another way
to do that would be with positive and negative zeroes with float
types.  But this way seems nicer and more flexible.)

Discussion: https://www.postgresql.org/message-id/ea5b2777-266a-46fa-852f-6fca6ec480ad@eisentraut.org

5 months agoSkip not SOAP-supported indexes while transforming an OR clause into SAOP
Alexander Korotkov [Fri, 29 Nov 2024 07:48:29 +0000 (09:48 +0200)]
Skip not SOAP-supported indexes while transforming an OR clause into SAOP

There is no point in transforming OR-clauses into SAOP's if the target index
doesn't support SAOP scans anyway.  This commit adds corresponding checks
to match_orclause_to_indexcol() and group_similar_or_args().  The first check
fixes the actual bug, while the second just saves some cycles.

Reported-by: Alexander Lakhin
Discussion: https://postgr.es/m/8174de69-9e1a-0827-0e81-ef97f56a5939%40gmail.com
Author: Alena Rybakina
Reviewed-by: Ranier Vilela, Alexander Korotkov, Andrei Lepikhov
5 months agoFix typo in header comment for set_operation_ordered_results_useful
David Rowley [Fri, 29 Nov 2024 02:56:24 +0000 (15:56 +1300)]
Fix typo in header comment for set_operation_ordered_results_useful

Reported-by: Richard Guo
Discussion: https://postgr.es/m/CAMbWs492vMy3XNjDZRtqtHfFTK6HVeDwhrEQH7eXGgF_h5Jnzw@mail.gmail.com

5 months agopsql: Sprinkle more CppAsString2() in describe.c
Michael Paquier [Thu, 28 Nov 2024 23:53:09 +0000 (08:53 +0900)]
psql: Sprinkle more CppAsString2() in describe.c

Like 91f5a4a000ea for pg_amcheck, this makes the code more
self-documented as there is less need to look in the headers what a
hardcoded value means.  This touches queries related to procedures, AMs,
functions, databases, relations, constraints, collations, types and
extended stats, pulling into psql their *_d.h headers.  The queries are
written the same way as originally.

There are still a couple of hardcoded values.  These cannot be included
yet as they are not exposed in headers that are safe to use in frontend
code.

Note that describe.c was including pg_am.h that should be used only in
backend code.  This is updated to use pg_am_d.h.

Reviewed-by: Daniel Gustafsson, Corey Huinker
Discussion: https://postgr.es/m/[email protected]

5 months agoAvoid mislabeling of lateral references when pulling up a subquery.
Tom Lane [Thu, 28 Nov 2024 22:33:16 +0000 (17:33 -0500)]
Avoid mislabeling of lateral references when pulling up a subquery.

If we are pulling up a subquery that's under an outer join, and
the subquery's target list contains a strict expression that uses
both a subquery variable and a lateral-reference variable, it's okay
to pull up the expression without wrapping it in a PlaceHolderVar.
That's safe because if the subquery variable is forced to NULL
by the outer join, the expression result will come out as NULL too,
so we don't have to force that outcome by evaluating the expression
below the outer join.  It'd be correct to wrap in a PHV, but that can
lead to very significantly worse plans, since we'd then have to use
a nestloop plan to pass down the lateral reference to where the
expression will be evaluated.

However, when we do that, we should not mark the lateral reference
variable as being  by the outer join, because it isn't after
we pull up the expression in this way.  So the marking logic added
by cb8e50a4a was incorrect in this detail, leading to "wrong
varnullingrels" errors from the consistency-checking logic in
setrefs.c.  It seems to be sufficient to just not mark lateral
references at all in this case.  (I have a nagging feeling that more
complexity may be needed in cases where there are several levels of
outer join, but some attempts to break it with that didn't succeed.)

Per report from Bertrand Mamasam.  Back- to v16, as the previous
 was.

Discussion: https://postgr.es/m/CACZ67_UA_EVrqiFXJu9XK50baEpH=ofEPJswa2kFxg6xuSw-ww@mail.gmail.com

5 months agoFix wording in comment
Daniel Gustafsson [Thu, 28 Nov 2024 14:17:49 +0000 (15:17 +0100)]
Fix wording in comment

Author: Peter Smith <[email protected]>
Reviewed-by: vignesh C <[email protected]>
Discussion: https://postgr.es/m/CAHut+PvE+2T2etdTaHi3n+xbCG_UYrshQuCbaAdJCFPpQGLwgQ@mail.gmail.com

5 months agopsql: Add tab completion for COPY (MERGE ...
Peter Eisentraut [Thu, 28 Nov 2024 08:14:41 +0000 (09:14 +0100)]
psql: Add tab completion for COPY (MERGE ...

The underlying feature for this was added in PostgreSQL 17.

Author: Jian He <[email protected]>
Discussion: https://www.postgresql.org/message-id/CACJufxEmNjxvf1deR1zBrJbjAMeCooooLRzZ+yaaBuqDKh_6-Q@mail.gmail.com

5 months agoRemove useless casts to (void *)
Peter Eisentraut [Thu, 28 Nov 2024 07:19:22 +0000 (08:19 +0100)]
Remove useless casts to (void *)

Many of them just seem to have been copied around for no real reason.
Their presence causes (small) risks of hiding actual type mismatches
or silently discarding qualifiers

Discussion: https://www.postgresql.org/message-id/flat/461ea37c-8b58-43b4-9736-52884e862820@eisentraut.org

5 months agoRequire sizeof(bool) == 1.
Thomas Munro [Wed, 27 Nov 2024 22:48:07 +0000 (11:48 +1300)]
Require sizeof(bool) == 1.

The C standard says that sizeof(bool) is implementation-defined, but we
know of no current systems where it is not 1.  The last known systems
seem to have been Apple macOS/PowerPC 10.5 and Microsoft Visual C++ 4,
both long defunct.

PostgreSQL has always required sizeof(bool) == 1 for the definition of
bool that it used, but previously it would define its own type if the
system-provided bool had a different size.  That was liable to cause
memory layout problems when interacting with system and third-party
libraries on (by now hypothetical) computers with wider _Bool, and now
C23 has introduced a new problem by making bool a built-in datatype
(like C++), so the fallback code doesn't even compile.  We could
probably work around that, but then we'd be writing new untested code
for a computer that doesn't exist.

Instead, delete the unreachable and C23-uncompilable fallback code, and
let existing static assertions fail if the system-provided bool is too
wide.  If we ever get a problem report from a real system, then it will
be time to figure out what to do about it in a way that also works on
modern compilers.

Note on C++: Previously we avoided including <stdbool.h> or trying to
define a new bool type in headers that might be included by C++ code.
These days we might as well just include <stdbool.h> unconditionally:
it should be visible to C++11 but do nothing, just as in C23.  We
already include <stdint.h> without C++ guards in c.h, and that falls
under the same C99-compatibility section of the C++11 standard as
<stdbool.h>, so let's remove the guards here too.

Reviewed-by: Tom Lane <[email protected]>
Discussion: https://postgr.es/m/3198438.1731895163%40sss.pgh.pa.us

5 months agoUse __attribute__((target(...))) for SSE4.2 CRC-32C support.
Nathan Bossart [Wed, 27 Nov 2024 22:19:05 +0000 (16:19 -0600)]
Use __attribute__((target(...))) for SSE4.2 CRC-32C support.

Presently, we check for compiler support for the required
intrinsics both with and without the -msse4.2 compiler flag, and
then depending on the results of those checks, we pick which files
to compile with which flags.  This is tedious and complicated, and
it results in unsustainable coding patterns such as separate files
for each portion of code that may need to be built with different
compiler flags.

This commit makes use of the newly-added support for
__attribute__((target(...))) in the SSE4.2 CRC-32C code.  This
simplifies both the configure-time checks and the build scripts,
and it allows us to place the functions that use the intrinsics in
files that we otherwise do not want to build with special CPU
instructions (although this commit refrains from doing so).  This
is also preparatory work for a proposed follow-up commit that will
further optimize the CRC-32C code with AVX-512 instructions.

While at it, this commit modifies meson's checks for SSE4.2 CRC
support to be the same as autoconf's.  meson was choosing whether
to use a runtime check based purely on whether -msse4.2 is
required, while autoconf has long checked for the __SSE4_2__
preprocessor symbol to decide.  meson's previous approach seems to
work just fine, but this change avoids needing to build multiple
test programs and to keep track of whether to actually use
pg_attribute_target().

Ideally we'd use __attribute__((target(...))) for ARMv8 CRC
support, too, but there's little point in doing so because until
clang 16, using the ARM intrinsics still requires special compiler
flags.  Perhaps we can re-evaluate this decision after some time
has passed.

Author: Raghuveer Devulapalli
Discussion: https://postgr.es/m/PH8PR11MB8286BE735A463468415D46B5FB5C2%40PH8PR11MB8286.namprd11.prod.outlook.com

5 months agoMake GUC_check_errdetail messages full sentences
Álvaro Herrera [Wed, 27 Nov 2024 18:46:06 +0000 (19:46 +0100)]
Make GUC_check_errdetail messages full sentences

They were all missing punctuation, one was missing initial capital.
Per our message style guidelines.

No back, to avoid breaking existing translations.

5 months agoRemove redundant relam initialization
Álvaro Herrera [Wed, 27 Nov 2024 18:13:37 +0000 (19:13 +0100)]
Remove redundant relam initialization

This struct member is initialized again a few lines below in the same
function.  This is cosmetic, so no back.

Reported-by: Jingtang Zhang <[email protected]>
Discussion: https://postgr.es/m/AFF74506-B925-46BB-B875-CF5A946170EB@gmail.com

5 months agoecpg: clean up some other assorted memory s.
Tom Lane [Wed, 27 Nov 2024 17:50:15 +0000 (12:50 -0500)]
ecpg: clean up some other assorted memory s.

Avoid ing the prior value when updating the "connection"
state variable.

Ditto for ECPGstruct_sizeof.  (It seems like this one ought to
be statement-local, but testing says it isn't, and I didn't
feel like diving deeper.)

The actual_type[] entries are statement-local, though, so
no need to mm_strdup() strings stored in them.

Likewise, sqlda variables are statement-local, so we can
loc_alloc them.

Also clean up sloppiness around management of the argsinsert and
argsresult lists.

progname changes are strictly to prevent valgrind from complaining
about  allocations.

With this, valgrind reports zero age in the ecpg preprocessor
for all of our ecpg regression test cases.

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

5 months agoecpg: put all string-valued tokens returned by pgc.l in local storage.
Tom Lane [Wed, 27 Nov 2024 17:44:03 +0000 (12:44 -0500)]
ecpg: put all string-valued tokens returned by pgc.l in local storage.

This didn't work earlier in the  series (I think some of
the strings were ending up in data-type-related structures),
but apparently we're now clean enough for it.  This considerably
reduces process-lifespan memory age.

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

5 months agoecpg: fix some memory age of data-type-related structures.
Tom Lane [Wed, 27 Nov 2024 17:41:20 +0000 (12:41 -0500)]
ecpg: fix some memory age of data-type-related structures.

ECPGfree_type() and related functions were quite incomplete
about removing subsidiary data structures.  Possibly this is
because ecpg wasn't careful to make sure said data structures
always had their own storage.  Previous es in this series
cleaned up a lot of that, and I had to add a couple more
mm_strdup's here.

Also, ecpg.trailer tended to overwrite struct_member_list[struct_level]
without bothering to free up its previous contents, thus potentially
ing a lot of struct-member-related storage.  Add
ECPGfree_struct_member() calls at appropriate points.

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

5 months agojsonapi: add lexer option to keep token ownership
Andrew Dunstan [Wed, 27 Nov 2024 17:05:44 +0000 (12:05 -0500)]
jsonapi: add lexer option to keep token ownership

Commit 0785d1b8b adds support for libpq as a JSON client, but
allocations for string tokens can still be  during parsing
failures. This is tricky to fix for the object_field semantic callbacks:
the field name must remain valid until the end of the object, but if a
parsing error is encountered partway through, object_field_end() won't
be invoked and the client won't get a chance to free the field name.

This  adds a flag to switch the ownership of parsed tokens to the
lexer. When this is enabled, the client must make a copy of any tokens
it wants to persist past the callback lifetime, but the lexer will
handle necessary cleanup on failure.

Backend uses of the JSON parser don't need to use this flag, since the
parser's allocations will occur in a short lived memory context.

A -o option has been added to test_json_parser_incremental to exercise
the new setJsonLexContextOwnsTokens() API, and the test_json_parser TAP
tests make use of it. (The test program now cleans up allocated memory,
so that tests can be usefully run under  sanitizers.)

Author: Jacob Champion

Discussion: https://postgr.es/m/CAOYmi+kb38EciwyBQOf9peApKGwraHqA7pgzBkvoUnw5BRfS1g@mail.gmail.com

5 months agoci: Fix cached MacPorts installation management
Andres Freund [Wed, 27 Nov 2024 16:28:59 +0000 (11:28 -0500)]
ci: Fix cached MacPorts installation management

1.  The error reporting of "port setrequested list-of-packages..."
changed, hiding errors we were relying on to know if all packages in our
list were already installed.  Use a loop instead.

2.  The cached MacPorts installation was shared between PostgreSQL
major branches, though each branch wanted different packages.  Add the
list of packages to cache key, so that different branches, when tested
in one  account/repo such as postgres/postgres, stop fighting with
each other, adding and removing packages.

Back- to 15 where CI began.

Author: Thomas Munro <[email protected]>
Author: Nazir Bilal Yavuz <[email protected]>
Suggested-by: Andres Freund <[email protected]>
Discussion: https://postgr.es/m/au2uqfuy2nf43nwy2txmc5t2emhwij7kzupygto3d2ffgtrdgr%40ckvrlwyflnh2

5 months agoLook up backend type in pg_signal_backend() more cheaply.
Nathan Bossart [Wed, 27 Nov 2024 16:32:25 +0000 (10:32 -0600)]
Look up backend type in pg_signal_backend() more cheaply.

Commit ccd38024bc, which introduced the pg_signal_autovacuum_worker
role, added a call to pgstat_get_beentry_by_proc_number() for the
purpose of determining whether the process is an autovacuum worker.
This function calls pgstat_read_current_status(), which can be
fairly expensive and may return cached, out-of-date information.
Since we just need to look up the target backend's BackendType, and
we already know its ProcNumber, we can instead inspect the
BackendStatusArray directly, which is much less expensive and
possibly more up-to-date.  There are some caveats with this
approach (which are documented in the code), but it's still
substantially better than before.

Reported-by: Andres Freund
Reviewed-by: Andres Freund
Discussion: https://postgr.es/m/ujenaa2uabzfkwxwmfifawzdozh3ljr7geozlhftsuosgm7n7q%40g3utqqyyosb6

5 months agopostmaster: Reduce verbosity of environment dump debug message
Andres Freund [Wed, 27 Nov 2024 16:17:23 +0000 (11:17 -0500)]
postmaster: Reduce verbosity of environment dump debug message

Emitting each variable separately is unnecessarily verbose / hard to skim
over. Emit the whole thing in one ereport() to address that.

Also remove program name and function reference from the message. The former
doesn't seem particularly helpful and the latter is provided by the elog.c
infrastructure these days.

Reviewed-by: Heikki Linnakangas <[email protected]>
Reviewed-by: Peter Eisentraut <[email protected]>
Discussion: https://postgr.es/m/leouteo5ozcrux3fepuhtbp6c56tbfd4naxeokidbx7m75cabz@hhw6g4urlowt

5 months agofile_fdw: Add regression tests for ON_ERROR and other options.
Fujii Masao [Wed, 27 Nov 2024 14:40:11 +0000 (23:40 +0900)]
file_fdw: Add regression tests for ON_ERROR and other options.

This commit introduces regression tests to validate incorrect settings
for the ON_ERROR, LOG_VERBOSITY, and REJECT_LIMIT options in file_fdw.

Author: Atsushi Torikoshi
Reviewed-by: Fujii Masao
Suggested-by: Yugo Nagata
Discussion: https://postgr.es/m/20241113231706.09e5b5ea9640289312835be8@sraoss.co.jp

5 months agopgbench: Ensure previous progress message is fully cleared when updating.
Fujii Masao [Wed, 27 Nov 2024 14:01:53 +0000 (23:01 +0900)]
pgbench: Ensure previous progress message is fully cleared when updating.

During pgbench's table initialization, progress updates could display
leftover characters from the previous message if the new message
was shorter. This commit resolves the issue by appending spaces to
the current message to fully overwrite any remaining characters from
the previous line.

Back- to all the supported versions.

Author: Yushi Ogiwara, Tatsuo Ishii, Fujii Masao
Reviewed-by: Tatsuo Ishii, Fujii Masao
Discussion: https://postgr.es/m/9a9b8b95b6a709877ae48ad5b0c59bb9@oss.nttdata.com

5 months agoFix pg_get_constraintdef for NOT NULL constraints on domains
Álvaro Herrera [Wed, 27 Nov 2024 12:50:27 +0000 (13:50 +0100)]
Fix pg_get_constraintdef for NOT NULL constraints on domains

We added pg_constraint rows for all not-null constraints, first for
tables and later for domains; but while the ones for tables were
reverted, the ones for domains were not.  However, we did accidentally
revert ruleutils.c support for the ones on domains in 6f8bb7c1e961,
which breaks running pg_get_constraintdef() on them.  Put that back.

This is only needed in branch 17, because we've reinstated this code in
branch master with commit 14e87ffa5c54.  Add some new tests in both
branches.

I couldn't find anything else that needs de-reverting.

Reported-by: Erki Eessaar <[email protected]>
Reviewed-by: Magnus Hagander <[email protected]>
Discussion: https://postgr.es/m/AS8PR01MB75110350415AAB8BBABBA1ECFE222@AS8PR01MB7511.eurprd01.prod.exchangelabs.com

5 months agogitattributes: Add .cpp files to whitespace checks
Peter Eisentraut [Wed, 27 Nov 2024 10:15:53 +0000 (11:15 +0100)]
gitattributes: Add .cpp files to whitespace checks

Use the same rules as .c files.

5 months agoFix typo
Peter Eisentraut [Wed, 27 Nov 2024 10:12:09 +0000 (11:12 +0100)]
Fix typo

from commit 9044fc1d45a

5 months agoExclude LLVM files from whitespace checks
Peter Eisentraut [Wed, 27 Nov 2024 10:08:12 +0000 (11:08 +0100)]
Exclude LLVM files from whitespace checks

Commit 9044fc1d45a added some files from upstream LLVM.  These files
have different whitespace rules, which make the git whitespace checks
powered by gitattributes fail.  To fix, add those files to the exclude
list.

5 months agoRevert "Blind attempt to fix _configthreadlocale() failures on MinGW."
Thomas Munro [Wed, 27 Nov 2024 09:56:41 +0000 (22:56 +1300)]
Revert "Blind attempt to fix _configthreadlocale() failures on MinGW."

This reverts commit 2cf91ccb73ce888c44e3751548fb7c77e87335f2.

When using the old msvcrt.dll, MinGW would supply its own dummy version
of _configthreadlocale() that just returns -1 if you try to use it.  For
a time we tolerated that to shut the build farm up.  We would fall back
to code that was enough for the tests to pass, but it would surely have
risked crashing a real multithreaded program.

We don't need that kludge anymore, because we can count on ucrt.  We
expect the real _configthreadlocale() to be present, and the ECPG tests
will now fail if it isn't.  The workaround was dead code and it's time
to revert it.

(A later  still under review proposes to remove this use of
_configthreadlocale() completely but we're unwinding this code in
steps.)

Reviewed-by: Peter Eisentraut <[email protected]>
Discussion: https://postgr.es/m/d9e7731c-ca1b-477c-9298-fa51e135574a%40eisentraut.org

5 months agoRequire ucrt if using MinGW.
Thomas Munro [Wed, 27 Nov 2024 09:34:11 +0000 (22:34 +1300)]
Require ucrt if using MinGW.

Historically we tolerated the absence of various C runtime library
features for the benefit of the MinGW tool chain, because it used
ancient msvcrt.dll for a long period of time.  It now uses ucrt by
default (like Windows 10+, Visual Studio 2015+), and that's the only
configuration we're testing.

In practice, we effectively required ucrt already in PostgreSQL 17, when
commit 8d9a9f03 required _create_locale etc, first available in
msvcr120.dll (Visual Studio 2013, the last of the pre-ucrt series of
runtimes), and for MinGW users that practically meant ucrt because it
was difficult or impossible to use msvcr120.dll.  That may even not have
been the first such case, but old MinGW configurations had already
dropped off our testing radar so we weren't paying much attention.

This commit formalizes the requirement.  It also removes a couple of
obsolete comments that discussed msvcrt.dll limitations, and some tests
of !defined(_MSC_VER) to imply msvcrt.dll.  There are many more
anachronisms, but it'll take some time to figure out how to remove them
all.  APIs affected relate to locales, UTF-8, threads, large files and
more.

Thanks to Peter Eisentraut for the documentation change.  It's not
really necessary to talk about ucrt explicitly in such a short section,
since it's the default for MinGW-w64 and MSYS2.  It's enough to prune
references and broken links to much older tools.

Reviewed-by: Peter Eisentraut <[email protected]>
Discussion: https://postgr.es/m/d9e7731c-ca1b-477c-9298-fa51e135574a%40eisentraut.org

5 months agoRemove configure check for _configthreadlocale().
Thomas Munro [Wed, 27 Nov 2024 09:34:03 +0000 (22:34 +1300)]
Remove configure check for _configthreadlocale().

All modern Windows systems have _configthreadlocale().  It was first
introduced in msvcr80.dll from Visual Studio 2005.  Historically, MinGW
was stuck on even older msvcrt.dll, but added its own dummy
implementation of the function when using msvcrt.dll years ago anyway,
effectively rendering the configure test useless.  In practice we don't
encounter the dummy anymore because modern MinGW uses ucrt.

Reviewed-by: Peter Eisentraut <[email protected]>
Discussion: https://postgr.es/m/CWZBBRR6YA8D.8EHMDRGLCKCD%40neon.tech

5 months agoImprove slightly misleading internal error message
Peter Eisentraut [Wed, 27 Nov 2024 09:55:35 +0000 (10:55 +0100)]
Improve slightly misleading internal error message

The error message was talking about RowCompareType but was actually
checking strategy numbers.  While those are closely related, it is
better to be accurate.

Discussion: https://www.postgresql.org/message-id/flat/E72EAA49-354D-4C2E-8EB9-255197F55330@enterprisedb.com

5 months agoFix buildfarm failure from commit 8fcd80258b.
Amit Kapila [Wed, 27 Nov 2024 09:24:26 +0000 (14:54 +0530)]
Fix buildfarm failure from commit 8fcd80258b.

The test case was incorrectly matching the error code.

Author: Vignesh C
Discussion: https://postgr.es/m/CALDaNm0C5LPiTxkdqsxiyeaL=nuUP8t6ne81sp9jE0=MFz=-ew@mail.gmail.com

5 months agoSupport LIKE with nondeterministic collations
Peter Eisentraut [Wed, 27 Nov 2024 07:18:35 +0000 (08:18 +0100)]
Support LIKE with nondeterministic collations

This allows for example using LIKE with case-insensitive collations.
There was previously no internal implementation of this, so it was met
with a not-supported error.  This adds the internal implementation and
removes the error.  The implementation follows the specification of
the SQL standard for this.

Unlike with deterministic collations, the LIKE matching cannot go
character by character but has to go substring by substring.  For
example, if we are matching against LIKE 'foo%bar', we can't start by
looking for an 'f', then an 'o', but instead with have to find
something that matches 'foo'.  This is because the collation could
consider substrings of different lengths to be equal.  This is all
internal to MatchText() in like_match.c.

The changes in GenericMatchText() in like.c just pass through the
locale information to MatchText(), which was previously not needed.
This matches exactly Generic_Text_IC_like() below.

ILIKE is not affected.  (It's unclear whether ILIKE makes sense under
nondeterministic collations.)

This also updates match_pattern_prefix() in like_support.c to support
optimizing the case of an exact pattern with nondeterministic
collations.  This was already alluded to in the previous code.

(includes documentation examples from Daniel Vérité and test cases
from Paul A Jungwirth)

Reviewed-by: Jian He <[email protected]>
Discussion: https://www.postgresql.org/message-id/flat/700d2e86-bf75-4607-9cf2-f5b7802f6e88@eisentraut.org

5 months agoImprove error message for replication of generated columns.
Amit Kapila [Wed, 27 Nov 2024 03:39:20 +0000 (09:09 +0530)]
Improve error message for replication of generated columns.

Currently, logical replication produces a generic error message when
targeting a subscriber-side table column that is either missing or
generated. The error message can be misleading for generated columns.

This  introduces a specific error message to clarify the issue when
generated columns are involved.

Author: Shubham Khanna
Reviewed-by: Peter Smith, Vignesh C, Amit Kapila
Discussion: https://postgr.es/m/CAHv8RjJBvYtqU7OAofBizOmQOK2Q8h+w9v2_cQWxT_gO7er3Aw@mail.gmail.com

5 months agoHandle better implicit transaction state of pipeline mode
Michael Paquier [Wed, 27 Nov 2024 00:31:22 +0000 (09:31 +0900)]
Handle better implicit transaction state of pipeline mode

When using a pipeline, a transaction starts from the first command and
is committed with a Sync message or when the pipeline ends.

Functions like IsInTransactionBlock() or PreventInTransactionBlock()
were already able to understand a pipeline as being in a transaction
block, but it was not the case of CheckTransactionBlock().  This
function is called for example to generate a WARNING for SET LOCAL,
complaining that it is used outside of a transaction block.

The current state of the code caused multiple problems, like:
- SET LOCAL executed at any stage of a pipeline issued a WARNING, even
if the command was at least second in line where the pipeline is in a
transaction state.
- LOCK TABLE failed when invoked at any step of a pipeline, even if it
should be able to work within a transaction block.

The pipeline protocol assumes that the first command of a pipeline is
not part of a transaction block, and that any follow-up commands is
considered as within a transaction block.

This commit changes the backend so as an implicit transaction block is
started each time the first Execute message of a pipeline has finished
processing, with this implicit transaction block ended once a sync is
processed.  The checks based on XACT_FLAGS_PIPELINING in the routines
checking if we are in a transaction block are not necessary: it is
enough to rely on the existing ones.

Some tests are added to pgbench, that can be backed down to v17
when \syncpipeline is involved and down to v14 where \startpipeline and
\endpipeline are available.  This is unfortunately limited regarding the
error patterns that can be checked, but it provides coverage for various
pipeline combinations to check if these succeed or fail.  These tests
are able to capture the case of SET LOCAL's WARNING.  The author has
proposed a different feature to improve the coverage by adding similar
meta-commands to psql where error messages could be checked, something
more useful for the cases where commands cannot be used in transaction
blocks, like REINDEX CONCURRENTLY or VACUUM.  This is considered as
future work for v18~.

Author: Anthonin Bonnefoy
Reviewed-by: Jelte Fennema-Nio, Michael Paquier
Discussion: https://postgr.es/m/CAO6_XqrWO8uNBQrSu5r6jh+vTGi5Oiyk4y8yXDORdE2jbzw8xw@mail.gmail.com
Back-through: 13

5 months agoFix commit 641a5b7a144 for "nbsp" output in SVG files
Bruce Momjian [Tue, 26 Nov 2024 18:07:53 +0000 (13:07 -0500)]
Fix commit 641a5b7a144 for "nbsp" output in SVG files

In commit 641a5b7a144, I removed "nbsp" characters from SVG files, not
realizing the SVG files were generated from GV files and that the "nbsp"
characters were caused by trailing ASCII spaces in GV files.  This
commit restores the "nbsp" SVG characters and adds a GV comment about
how the trailing spaces cause the "nbsp" output.

Reported-by: Peter Eisentraut
Discussion: https://postgr.es/m/2c5dd601-b245-4092-9c27-6d1ad51609df%40eisentraut.org

Back-through: master

5 months agoDistinguish between AcquireExternalFD and epoll_create1 / kqueue failing
Andres Freund [Tue, 26 Nov 2024 17:20:59 +0000 (12:20 -0500)]
Distinguish between AcquireExternalFD and epoll_create1 / kqueue failing

The error messages in CreateWaitEventSet() made it hard to know whether the
syscall or AcquireExternalFD() failed. This is particularly relevant because
AcquireExternalFD() imposes a lower limit than what would cause syscalls fail
with EMFILE.

I did not change the message in libpqsrv_connect_prepare(), which is the one
other use of AcquireExternalFD() in our codebase, as the error message already
is less ambiguous.

Reviewed-by: Tom Lane <[email protected]>
Discussion: https://postgr.es/m/xjjx7r4xa7beixuu4qtkdhnwdbchrrpo3gaeb3jsbinvvdiat5@cwjw55mna5of

5 months agomeson: Build pgevent as shared_module rather than shared_library
Peter Eisentraut [Tue, 26 Nov 2024 17:06:08 +0000 (18:06 +0100)]
meson: Build pgevent as shared_module rather than shared_library

This matches the behavior of the makefiles and the old MSVC build
system.  The main effect is that the build result gets installed into
pkglibdir rather than bindir.  The documentation says to locate the
library in pkglibdir, so this makes the code match the documentation
again.

Reviewed-by: Ryohei Takahashi (Fujitsu) <[email protected]>
Discussion: https://www.postgresql.org/message-id/flat/TY3PR01MB118912125614599641CA881B782522%40TY3PR01MB11891.jpnprd01.prod.outlook.com

5 months agoClean up newlines following left parentheses
Álvaro Herrera [Tue, 26 Nov 2024 16:10:07 +0000 (17:10 +0100)]
Clean up newlines following left parentheses

Most came in during the 17 cycle, so back there.  Some
(particularly reorderbuffer.h) are very old, but backing doesn't
seem useful.

Like commits c9d297751959c4f113e8fef9.

5 months agoImprove InitShmemAccess()
Peter Eisentraut [Tue, 26 Nov 2024 07:25:23 +0000 (08:25 +0100)]
Improve InitShmemAccess() 

The code comment said, 'the argument should be declared "PGShmemHeader
*seghdr", but we use void to avoid having to include ipc.h in
shmem.h.'  We can achieve the original goal with a struct forward
declaration.  (ipc.h was also not the correct header file.)

Discussion: https://www.postgresql.org/message-id/flat/cnthxg2eekacrejyeonuhiaezc7vd7o2uowlsbenxqfkjwgvwj@qgzu6eoqrglb

5 months agoFix test case from a8ccf4e93
Richard Guo [Tue, 26 Nov 2024 02:12:57 +0000 (11:12 +0900)]
Fix test case from a8ccf4e93

Commit a8ccf4e93 uses the same table name "distinct_tbl" in both
select_distinct.sql and select_distinct_on.sql, which could cause
conflicts when these two test scripts are run in parallel.

Fix by renaming the table in select_distinct_on.sql to
"distinct_on_tbl".

Per buildfarm (via Tom Lane)

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

5 months agopg_amcheck: Use CppAsString2() for relkind and relpersistence in queries
Michael Paquier [Tue, 26 Nov 2024 00:45:34 +0000 (09:45 +0900)]
pg_amcheck: Use CppAsString2() for relkind and relpersistence in queries

This utility has been using hardcoded values for relkind and
relpersistence in its queries generated.  These queries are switched to
use CppAsString2() instead, with the values fetched directly from the
header of pg_class.  This has the advantage of making the code more
self-documented, as it becomes unnecessary to look at a header for the
meaning of a value.

There should be no functional changes; the queries are generated the
same way as before this commit.

Reviewed-by: Nathan Bossart, Daniel Gustafsson, Álvaro Herrera, Karina
Litskevich
Discussion: https://postgr.es/m/[email protected]

5 months agoRemove dead code in get_param_path_clause_serials()
Richard Guo [Tue, 26 Nov 2024 00:27:53 +0000 (09:27 +0900)]
Remove dead code in get_param_path_clause_serials()

The function get_param_path_clause_serials() is used to get the set of
pushed-down clauses enforced within a parameterized Path.  Since we
don't currently support parameterized MergeAppend paths, and it
doesn't look like that is going to change anytime soon (as explained
in the comments for generate_orderedappend_paths), we don't need to
consider MergeAppendPath in this function.

This change won't make any measurable difference in performance; it's
just for clarity's sake.

Author: Richard Guo
Reviewed-by: Andrei Lepikhov
Discussion: https://postgr.es/m/CAMbWs4_Puie4DQ2ODvjQB_3CxYkUODnrJm8jn_ObMAcrjYNW7Q@mail.gmail.com

5 months agoReordering DISTINCT keys to match input path's pathkeys
Richard Guo [Tue, 26 Nov 2024 00:25:18 +0000 (09:25 +0900)]
Reordering DISTINCT keys to match input path's pathkeys

The ordering of DISTINCT items is semantically insignificant, so we
can reorder them as needed.  In fact, in the parser, we absorb the
sorting semantics of the sortClause as much as possible into the
distinctClause, ensuring that one clause is a prefix of the other.
This can help avoid a possible need to re-sort.

In this commit, we attempt to adjust the DISTINCT keys to match the
input path's pathkeys.  This can likewise help avoid re-sorting, or
allow us to use incremental-sort to save efforts.

For DISTINCT ON expressions, the parser already ensures that they
match the initial ORDER BY expressions.  When reordering the DISTINCT
keys, we must ensure that the resulting pathkey list matches the
initial distinctClause pathkeys.

This introduces a new GUC, enable_distinct_reordering, which allows
the optimization to be disabled if needed.

Author: Richard Guo
Reviewed-by: Andrei Lepikhov
Discussion: https://postgr.es/m/CAMbWs48dR26cCcX0f=8bja2JKQPcU64136kHk=xekHT9xschiQ@mail.gmail.com

5 months agoFix NULLIF()'s handling of read-write expanded objects.
Tom Lane [Mon, 25 Nov 2024 23:08:58 +0000 (18:08 -0500)]
Fix NULLIF()'s handling of read-write expanded objects.

If passed a read-write expanded object pointer, the EEOP_NULLIF
code would hand that same pointer to the equality function
and then (unless equality was reported) also return the same
pointer as its value.  This is no good, because a function that
receives a read-write expanded object pointer is fully entitled
to scribble on or even delete the object, thus corrupting the
NULLIF output.  (This problem is likely unobservable with the
equality functions provided in core Postgres, but it's easy to
demonstrate with one coded in plpgsql.)

To fix, make sure the pointer passed to the equality function
is read-only.  We can still return the original read-write
pointer as the NULLIF result, allowing optimization of later
operations.

Per bug #18722 from Alexander Lakhin.  This has been wrong
since we invented expanded objects, so back- to all
supported branches.

Discussion: https://postgr.es/m/18722-fd9e645448cc78b4@postgresql.org

5 months agoAvoid "you don't own a lock of type ExclusiveLock" in GRANT TABLESPACE.
Noah Misch [Mon, 25 Nov 2024 22:42:35 +0000 (14:42 -0800)]
Avoid "you don't own a lock of type ExclusiveLock" in GRANT TABLESPACE.

This WARNING appeared because SearchSysCacheLocked1() read
cc_relisshared before catcache initialization, when the field is false
unconditionally.  On the basis of reading false there, it constructed a
locktag as though pg_tablespace weren't relisshared.  Only shared
catalogs could be affected, and only GRANT TABLESPACE was affected in
practice.  SearchSysCacheLocked1() callers use one other shared-relation
syscache, DATABASEOID.  DATABASEOID is initialized by the end of
CheckMyDatabase(), making the problem unreachable for pg_database.

Back- to v13 (all supported versions).  This has no known impact
before v16, where ExecGrant_common() first appeared.  Earlier branches
avoid trouble by having a separate ExecGrant_Tablespace() that doesn't
use LOCKTAG_TUPLE.  However, leaving this unfixed in v15 could ensnare a
future back- of a SearchSysCacheLocked1() call.

Reported by Aya Iwata.

Discussion: https://postgr.es/m/OS7PR01MB11964507B5548245A7EE54E70EA212@OS7PR01MB11964.jpnprd01.prod.outlook.com

5 months agopg_dump: Add dumpSchema and dumpData derivative flags.
Nathan Bossart [Mon, 25 Nov 2024 22:36:37 +0000 (16:36 -0600)]
pg_dump: Add dumpSchema and dumpData derivative flags.

Various parts of pg_dump consult the --schema-only and --data-only
options to determine whether to run a section of code.  While this
is simple enough for two mutually-exclusive options, it will become
progressively more complicated as more options are added.  In
anticipation of that, this commit introduces new internal flags
called dumpSchema and dumpData, which are derivatives of
--schema-only and --data-only.  This commit also removes the
schemaOnly and dataOnly members from the dump/restore options
structs to prevent their use elsewhere.

Note that this change neither adds new user-facing command-line
options nor changes the existing --schema-only and --data-only
options.

Author: Corey Huinker
Reviewed-by: Jeff Davis
Discussion: https://postgr.es/m/CADkLM%3DcQgghMJOS8EcAVBwRO4s1dUVtxGZv5gLPfZkQ1nL1gzA%40mail.gmail.com

5 months agoClean up <stdbool.h> reference in meson.build.
Thomas Munro [Mon, 25 Nov 2024 22:29:31 +0000 (11:29 +1300)]
Clean up <stdbool.h> reference in meson.build.

Commit bc5a4dfc accidentally left a check for <stdbool.h> in
meson.build's header_checks.  Synchronize with configure, which no
longer defines HAVE_STDBOOL_H.

There is still a reference to <stdbool.h> in an earlier test to see if
we need -std=c99 to get C99 features, like autoconf 2.69's
AC_PROG_CC_C99.  (Therefore the test remove by this commit was
tautological since day one: you'd have copped "C compiler does not
support C99" before making it this far.)

Back- to 16, where meson begins.

5 months agoUpdate configure probes for CFLAGS needed for ARM CRC instructions.
Tom Lane [Mon, 25 Nov 2024 17:50:17 +0000 (12:50 -0500)]
Update configure probes for CFLAGS needed for ARM CRC instructions.

On ARM platforms where the baseline CPU target lacks CRC instructions,
we need to supply a -march flag to persuade the compiler to compile
such instructions.  It turns out that our existing choice of
"-march=armv8-a+crc" has not worked for some time, because recent gcc
will interpret that as selecting software floating point, and then
will spit up if the platform requires hard-float ABI, as most do
nowadays.  The end result was to silently fall back to software CRC,
which isn't very desirable since in practice almost all currently
produced ARM chips do have hardware CRC.

We can fix this by using "-march=armv8-a+crc+simd" to enable the
correct ABI choice.  (This has no impact on the code actually
generated, since neither of the files we compile with this flag
does any floating-point stuff, let alone SIMD.)  Keep the test for
"-march=armv8-a+crc" since that's required for soft-float ABI,
but try that second since most platforms we're likely to build on
use hard-float.

Since this isn't working as-intended on the last several years'
worth of gcc releases, back- to all supported branches.

Discussion: https://postgr.es/m/4496616.iHFcN1HehY@portable-bastien