59743 Commits

Author SHA1 Message Date
Jeff Davis
56b1e88c80 Fix compiler warning.
Some buildfarm members complained about an always-true test in the
SOFT_ERROR_OCCURRED macro. Fix by reading the field directly rather
than using the macro.

Reported-by: Tom Lane
Discussion: https://postgr.es/m/2144895.1729653514@sss.pgh.pa.us
2024-10-23 10:24:17 -07:00
Jeff Davis
07d00692c8 Documentation fixup.
Wrong return type for pg_clear_attribute_stats().

Author: Noriyoshi Shinoda
Discussion: https://postgr.es/m/DM4PR84MB17347944F27A552F0CCDF84CEE4C2@DM4PR84MB1734.NAMPRD84.PROD.OUTLOOK.COM
2024-10-23 09:44:36 -07:00
Daniel Gustafsson
940f7a5627 Fix incorrect struct reference in comment
SASL frontend mechanisms are implemented with pg_fe_sasl_mech and
not the _be_ variant which is the backend implementation. Spotted
while reading adjacent code.
2024-10-23 16:13:28 +02:00
Daniel Gustafsson
6d16f9deba Make SASL max message length configurable
The proposed OAUTHBEARER SASL mechanism will need to allow larger
messages in the exchange, since tokens are sent directly by the
client.  Move this limit into the pg_be_sasl_mech struct so that
it can be changed per-mechanism.

Author: Jacob Champion <jacob.champion@enterprisedb.com>
Reviewed-by: Daniel Gustafsson <daniel@yesql.se>
Discussion: https://postgr.es/m/CAOYmi+nqX_5=Se0W0Ynrr55Fha3CMzwv_R9P3rkpHb=1kG7ZTQ@mail.gmail.com
2024-10-23 16:10:27 +02:00
Daniel Gustafsson
17b4aa77c3 doc: Fix INSERT statement syntax for identity columns
The INSERT statements in the examples were erroneously using
VALUE instead of VALUES. Backpatch to v17 where the examples
were added through a37bb7c1399.

Reported-by: shixiong327926@gmail.com
Discussion: https://postgr.es/m/172958472112.696.6075270400394560263@wrigleys.postgresql.org
Backpatch-through: 17
2024-10-23 14:58:17 +02:00
Amit Langote
55e6d712af Remove unnecessary word in a comment
Relations opened by the executor are only closed once in
ExecCloseRangeTableRelations(), so the word "again" in the comment
for ExecGetRangeTableRelation() is misleading and unnecessary.

Discussion: https://postgr.es/m/CA+HiwqHnw-zR+u060i3jp4ky5UR0CjByRFQz50oZ05de7wUg=Q@mail.gmail.com
Backpatch-through: 12
2024-10-23 17:54:48 +09:00
Michael Paquier
a0bff38d13 ecpg: Fix out-of-bound read in DecodeDateTime()
It was possible for the code to read out-of-bound data from the
"day_tab" table with some crafted input data.  Let's treat these as
invalid input as the month number is incorrect.

A test is added to test this case with a check on the errno returned by
the decoding routine.  A test close to the new one added in this commit
was testing for a failure, but did not look at the errno generated, so
let's use this commit to also change it, adding a check on the errno
returned by DecodeDateTime().

Like the other test scripts, dt_test should likely be expanded to
include more checks based on the errnos generated in these code paths.
This is left as future work.

This issue exists since 2e6f97560a83, so backpatch all the way down.

Reported-by: Pavel Nekrasov
Author: Bruce Momjian, Pavel Nekrasov
Discussion: https://postgr.es/m/18614-6bbe00117352309e@postgresql.org
Backpatch-through: 12
2024-10-23 08:33:54 +09:00
Jeff Davis
ce207d2a79 Add functions pg_set_attribute_stats() and pg_clear_attribute_stats().
Enable manipulation of attribute statistics. Only superficial
validation is performed, so it's possible to add nonsense, and it's up
to the planner (or other users of statistics) to behave reasonably in
that case.

Bump catalog version.

Author: Corey Huinker
Discussion: https://postgr.es/m/CADkLM=eErgzn7ECDpwFcptJKOk9SxZEk5Pot4d94eVTZsvj3gw@mail.gmail.com
2024-10-22 15:06:55 -07:00
Jeff Davis
dbe6bd4343 Change pg_*_relation_stats() functions to return type to void.
These functions will either raise an ERROR or run to normal
completion, so no return value is necessary.

Bump catalog version.

Author: Corey Huinker
Discussion: https://postgr.es/m/CADkLM=cBF8rnphuTyHFi3KYzB9ByDgx57HwK9Rz2yp7S+Om87w@mail.gmail.com
2024-10-22 12:48:01 -07:00
Tom Lane
774171c4f6 Improve reporting of errors in extension script files.
Previously, CREATE/ALTER EXTENSION gave basically no useful
context about errors reported while executing script files.
I think the idea was that you could run the same commands
manually to see the error, but that's often quite inconvenient.
Let's improve that.

If we get an error during raw parsing, we won't have a current
statement identified by a RawStmt node, but we should always get
a syntax error position.  Show the portion of the script from
the last semicolon-newline before the error position to the first
one after it.  There are cases where this might show only a
fragment of a statement, but that should be uncommon, and it
seems better than showing the whole script file.

Without an error cursor, if we have gotten past raw parsing (which
we probably have), we can report just the current SQL statement as
an item of error context.

In any case also report the script file name as error context,
since it might not be entirely obvious which of a series of
update scripts failed.  We can also show an approximate script
line number in case whatever we printed of the query isn't
sufficiently identifiable.

The error-context code path is already exercised by some
test_extensions test cases, but add tests for the syntax-error
path.

Discussion: https://postgr.es/m/ZvV1ClhnbJLCz7Sm@msg.df7cb.de
2024-10-22 11:31:45 -04:00
Tom Lane
14e5680eee Improve parser's reporting of statement start locations.
Up to now, the parser's reporting of a statement's stmt_location
included any preceding whitespace or comments.  This isn't really
desirable but was done to avoid accounting honestly for nonterminals
that reduce to empty.  It causes problems for pg_stat_statements,
which partially compensates by manually stripping whitespace, but
is not bright enough to strip /*-style comments.  There will be
more problems with an upcoming patch to improve reporting of errors
in extension scripts, so it's time to do something about this.

The thing we have to do to make it work right is to adjust
YYLLOC_DEFAULT to scan the inputs of each production to find the
first one that has a valid location (i.e., did not reduce to
empty).  In theory this adds a little bit of per-reduction overhead,
but in practice it's negligible.  I checked by measuring the time
to run raw_parser() on the contents of information_schema.sql, and
there was basically no change.

Having done that, we can rely on any nonterminal that didn't reduce
to completely empty to have a correct starting location, and we don't
need the kluges the stmtmulti production formerly used.

This should have a side benefit of allowing parse error reports to
include an error position in some cases where they formerly failed to
do so, due to trying to report the position of an empty nonterminal.
I did not go looking for an example though.  The one previously known
case where that could happen (OptSchemaEltList) no longer needs the
kluge it had; but I rather doubt that that was the only case.

Discussion: https://postgr.es/m/ZvV1ClhnbJLCz7Sm@msg.df7cb.de
2024-10-22 11:26:05 -04:00
Fujii Masao
7c4d3fe272 ecpg: Refactor ecpg_log() to skip unnecessary calls to ECPGget_sqlca().
Previously, ecpg_log() always called ECPGget_sqlca() to retrieve sqlca,
even though it was only needed for debug logging. This commit updates
ecpg_log() to call ECPGget_sqlca() only when debug logging is enabled.

Author: Yuto Sasaki
Reviewed-by: Alvaro Herrera, Tom Lane, Fujii Masao
Discussion: https://postgr.es/m/TY2PR01MB3628A85689649BABC9A1C6C3C1782@TY2PR01MB3628.jpnprd01.prod.outlook.com
2024-10-22 23:57:35 +09:00
Álvaro Herrera
53af9491a0
Restructure foreign key handling code for ATTACH/DETACH
... to fix bugs when the referenced table is partitioned.

The catalog representation we chose for foreign keys connecting
partitioned tables (in commit f56f8f8da6af) is inconvenient, in the
sense that a standalone table has a different way to represent the
constraint when referencing a partitioned table, than when the same
table becomes a partition (and vice versa).  Because of this, we need to
create additional catalog rows on detach (pg_constraint and pg_trigger),
and remove them on attach.  We were doing some of those things, but not
all of them, leading to missing catalog rows in certain cases.

The worst problem seems to be that we are missing action triggers after
detaching a partition, which means that you could update/delete rows
from the referenced partitioned table that still had referencing rows on
that table, the server failing to throw the required errors.

!!!
Note that this means existing databases with FKs that reference
partitioned tables might have rows that break relational integrity, on
tables that were once partitions on the referencing side of the FK.

Another possible problem is that trying to reattach a table
that had been detached would fail indicating that internal triggers
cannot be found, which from the user's point of view is nonsensical.

In branches 15 and above, we fix this by creating a new helper function
addFkConstraint() which is in charge of creating a standalone
pg_constraint row, and repurposing addFkRecurseReferencing() and
addFkRecurseReferenced() so that they're only the recursive routine for
each side of the FK, and they call addFkConstraint() to create
pg_constraint at each partitioning level and add the necessary triggers.
These new routines can be used during partition creation, partition
attach and detach, and foreign key creation.  This reduces redundant
code and simplifies the flow.

In branches 14 and 13, we have a much simpler fix that consists on
simply removing the constraint on detach.  The reason is that those
branches are missing commit f4566345cf40, which reworked the way this
works in a way that we didn't consider back-patchable at the time.

We opted to leave branch 12 alone, because it's different from branch 13
enough that the fix doesn't apply; and because it is going in EOL mode
very soon, patching it now might be worse since there's no way to undo
the damage if it goes wrong.

Existing databases might need to be repaired.

In the future we might want to rethink the catalog representation to
avoid this problem, but for now the code seems to do what's required to
make the constraints operate correctly.

Co-authored-by: Jehan-Guillaume de Rorthais <jgdr@dalibo.com>
Co-authored-by: Tender Wang <tndrwang@gmail.com>
Co-authored-by: Alvaro Herrera <alvherre@alvh.no-ip.org>
Reported-by: Guillaume Lelarge <guillaume@lelarge.info>
Reported-by: Jehan-Guillaume de Rorthais <jgdr@dalibo.com>
Reported-by: Thomas Baehler (SBB CFF FFS) <thomas.baehler2@sbb.ch>
Discussion: https://postgr.es/m/20230420144344.40744130@karst
Discussion: https://postgr.es/m/20230705233028.2f554f73@karst
Discussion: https://postgr.es/m/GVAP278MB02787E7134FD691861635A8BC9032@GVAP278MB0278.CHEP278.PROD.OUTLOOK.COM
Discussion: https://postgr.es/m/18541-628a61bc267cd2d3@postgresql.org
2024-10-22 16:01:18 +02:00
Alexander Korotkov
e1555645d7 Make all Perl warnings fatal in 043_wal_replay_wait.pl
This file was committed after c5385929593, but accidentally missed changing
all warnings into fatal errors.

Reported-by: Anton Voloshin
Discussion: https://postgr.es/m/aa8a55d5-554a-4027-a491-1b0ca7c85f7a%40postgrespro.ru
2024-10-22 13:25:11 +03:00
Peter Eisentraut
d2b4b4c225 Fix C23 compiler warning
The approach of declaring a function pointer with an empty argument
list and hoping that the compiler will not complain about casting it
to another type no longer works with C23, because foo() is now
equivalent to foo(void).

We don't need to do this here.  With a few struct forward declarations
we can supply a correct argument list without having to pull in
another header file.

(This is the only new warning with C23.  Together with the previous
fix a67a49648d9, this makes the whole code compile cleanly under C23.)

Reviewed-by: Tom Lane <tgl@sss.pgh.pa.us>
Discussion: https://www.postgresql.org/message-id/flat/95c6a9bf-d306-43d8-b880-664ef08f2944%40eisentraut.org
2024-10-22 08:17:42 +02:00
Michael Paquier
45e0ba30fc pg_stat_statements: Add tests for nested queries with level tracking
There have never been any regression tests in PGSS for various query
patterns for nested queries combined with level tracking, like:
- Multi-statements.
- CREATE TABLE AS
- CREATE/REFRESH MATERIALIZED VIEW
- DECLARE CURSOR
- EXPLAIN, with a subset of the above supported.
- COPY.

All the tests added here track historical, sometimes confusing, existing
behaviors.  For example, EXPLAIN stores two PGSS entries with the same
top-level query string but two different query IDs as one is calculated
for the top-level EXPLAIN (this part is right) and a second one for the
inner query in the EXPLAIN (this part is not right).

A couple of patches are under discussion to improve the situation, and
all the tests added here will prove useful to evaluate the changes
discussed.

Author: Anthonin Bonnefoy
Reviewed-by: Michael Paquier, Jian He
Discussion: https://postgr.es/m/CAO6_XqqM6S9bQ2qd=75W+yKATwoazxSNhv5sjW06fjGAtHbTUA@mail.gmail.com
2024-10-22 13:05:51 +09:00
Tom Lane
68ad9816c1 Fix wrong assertion and poor error messages in "COPY (query) TO".
If the query is rewritten into a NOTIFY command by a DO INSTEAD
rule, we'd get an assertion failure, or in non-assert builds
issue a rather confusing error message.  Improve that.

Also fix a longstanding grammar mistake in a nearby error message.

Per bug #18664 from Alexander Lakhin.  Back-patch to all supported
branches.

Tender Wang and Tom Lane

Discussion: https://postgr.es/m/18664-ffd0ebc2386598df@postgresql.org
2024-10-21 15:08:22 -04:00
Heikki Linnakangas
3c7d78427e Update outdated comment on WAL-logged locks with invalid XID
We haven't generated those for a long time.

Discussion: https://www.postgresql.org/message-id/b439edfc-c5e5-43a9-802d-4cb51ec20646@iki.fi
2024-10-21 14:28:43 +03:00
Heikki Linnakangas
1a43de5e0a Fix race condition in committing a serializable transaction
The finished transaction list can contain XIDs that are older than the
serializable global xmin. It's a short-lived state;
ClearOldPredicateLocks() removes any such transactions from the list,
and it's called whenever the global xmin advances. But if another
backend calls SummarizeOldestCommittedSxact() in that window, it will
call SerialAdd() on an XID that's older than the global xmin, or if
there are no more transactions running, when global xmin is
invalid. That trips the assertion in SerialAdd().

Fixes bug #18658 reported by Andrew Bille. Thanks to Alexander Lakhin
for analysis. Backpatch to all versions.

Discussion: https://www.postgresql.org/message-id/18658-7dab125ec688c70b%40postgresql.org
2024-10-21 09:49:21 +03:00
Michael Paquier
57a36e890d Fix grammar of a comment in bufmgr.c
Author: Junwang Zhao
Discussion: https://postgr.es/m/CAEG8a3L5YjxXCjx0LhkwHdDGsNgpFGEqH7SqtXRPNP+dwFMVZQ@mail.gmail.com
2024-10-21 11:25:29 +09:00
Michael Paquier
a7800cf498 injection_points: Add basic isolation test
This test can act as a template when implementing an isolation test with
injection points, and tracks in a much simpler way some of the behaviors
implied in the existing isolation test "inplace" that has been added in
c35f419d6efb.  Particularly, a detach does not affect a backend wait; a
wait needs to be interrupted by a wakeup.

Reviewed-by: Bertrand Drouvot
Discussion: https://postgr.es/m/ZxGTONm_ctQz--io@paquier.xyz
2024-10-21 11:10:51 +09:00
Álvaro Herrera
f1c141fe14
Note that index_name in ALTER INDEX ATTACH PARTITION can be schema-qualified
Missed in 8b08f7d4820f; backpatch to all supported branches.

Reported-by: alvaro@datadoghq.com
Reviewed-by: Tom Lane <tgl@sss.pgh.pa.us>
Discussion: https://postgr.es/m/172924785099.698.15236991344616673753@wrigleys.postgresql.org
2024-10-20 15:36:20 +02:00
Amit Langote
11c87216d1 SQL/JSON: Fix some oversights in commit b6e1157e7
The decision in b6e1157e7 to ignore raw_expr when evaluating a
JsonValueExpr was incorrect.  While its value is not ultimately
used (since formatted_expr's value is), failing to initialize it
can lead to problems, for instance,  when the expression tree in
raw_expr contains Aggref nodes, which must be initialized to
ensure the parent Agg node works correctly.

Also, optimize eval_const_expressions_mutator()'s handling of
JsonValueExpr a bit.  Currently, when formatted_expr cannot be folded
into a constant, we end up processing it twice -- once directly in
eval_const_expressions_mutator() and again recursively via
ece_generic_processing().  This recursive processing is required to
handle raw_expr. To avoid the redundant processing of formatted_expr,
we now  process raw_expr directly in eval_const_expressions_mutator().

Finally, update the comment of JsonValueExpr to describe the roles of
raw_expr and formatted_expr more clearly.

Bug: #18657
Reported-by: Alexander Lakhin <exclusion@gmail.com>
Diagnosed-by: Fabio R. Sluzala <fabio3rs@gmail.com>
Diagnosed-by: Tender Wang <tndrwang@gmail.com>
Reviewed-by: Tom Lane <tgl@sss.pgh.pa.us>
Discussion: https://postgr.es/m/18657-1b90ccce2b16bdb8@postgresql.org
Backpatch-through: 16
2024-10-20 12:20:55 +09:00
Tom Lane
52475b4d30 Fix comment about pg_authid.
pg_shadow is not "publicly readable".  (pg_group is, but there seems
no need to make that distinction here.)  Seems to be a thinko dating
clear back to 7762619e9.

Antonin Houska

Discussion: https://postgr.es/m/31926.1729252247@antos
2024-10-19 11:44:14 -04:00
Jeff Davis
779972e534 Disable autovacuum for tables in stats import tests.
While we haven't observed any test instability, it seems like a good
idea to disable autovacuum during the stats import tests.

Author: Corey Huinker
Discussion: https://postgr.es/m/CADkLM=fajh1Lpcyr_XsMmq-9Z=SGk-u+_Zeac7Pt0RAN3uiVCg@mail.gmail.com
2024-10-18 10:57:46 -07:00
Jeff Davis
b391d882ff Allow pg_set_relation_stats() to set relpages to -1.
While the default value for relpages is 0, if a partitioned table with
at least one child has been analyzed, then the partititoned table will
have a relpages value of -1.

Author: Corey Huinker
Discussion: https://postgr.es/m/CADkLM=fajh1Lpcyr_XsMmq-9Z=SGk-u+_Zeac7Pt0RAN3uiVCg@mail.gmail.com
2024-10-18 10:44:15 -07:00
Peter Geoghegan
1bd4bc85ca Optimize nbtree backwards scans.
Make nbtree backwards scans optimistically access the next page to be
read to the left by following a prevPage block number that's now stashed
in currPos when the leaf page is first read.  This approach matches the
one taken during forward scans, which follow a symmetric nextPage block
number from currPos.  We stash both a prevPage and a nextPage, since the
scan direction might change (when fetching from a scrollable cursor).

Backwards scans will no longer need to lock the same page twice, except
in rare cases where the scan detects a concurrent page split (or page
deletion).  Testing has shown this optimization to be particularly
effective during parallel index-only backwards scans: ~12% reductions in
query execution time are quite possible.

We're much better off being optimistic; concurrent left sibling page
splits are rare in general.  It's possible that we'll need to lock more
pages than the pessimistic approach would have, but only when there are
_multiple_ concurrent splits of the left sibling page we now start at.
If there's just a single concurrent left sibling page split, the new
approach to scanning backwards will at least break even relative to the
old one (we'll acquire the same number of leaf page locks as before).

The optimization from this commit has long been contemplated by comments
added by commit 2ed5b87f96, which changed the rules for locking/pinning
during nbtree index scans.  The approach that that commit introduced to
leaf level link traversal when scanning forwards is now more or less
applied all the time, regardless of the direction we're scanning in.

Following uniform conventions around sibling link traversal is simpler.
The only real remaining difference between our forward and backwards
handling is that our backwards handling must still detect and recover
from any concurrent left sibling splits (and concurrent page deletions),
as documented in the nbtree README.  That is structured as a single,
isolated extra step that takes place in _bt_readnextpage.

Also use this opportunity to further simplify the functions that deal
with reading pages and traversing sibling links on the leaf level, and
to document their preconditions and postconditions (with respect to
things like buffer locks, buffer pins, and seizing the parallel scan).

This enhancement completely supersedes the one recently added by commit
3f44959f.

Author: Matthias van de Meent <boekewurm+postgres@gmail.com>
Author: Peter Geoghegan <pg@bowt.ie>
Discussion: https://postgr.es/m/CAEze2WgpBGRgTTxTWVPXc9+PB6fc1a7t+VyGXHzfnrFXcQVxnA@mail.gmail.com
Discussion: https://postgr.es/m/CAH2-WzkBTuFv7W2+84jJT8mWZLXVL0GHq2hMUTn6c9Vw=eYrCw@mail.gmail.com
2024-10-18 11:25:32 -04:00
Nathan Bossart
9e2d813d59 Adjust documentation for configuring Linux huge pages.
The present wording about viewing shared_memory_size_in_huge_pages
seems to suggest that the parameter cannot be viewed after startup
at all, whereas the intent is to make it clear that you can't use
"postgres -C" to view this parameter while the server is running.
This commit rephrases this section to remove the ambiguity.

Author: Seino Yuki
Reviewed-by: Michael Paquier, David G. Johnston, Fujii Masao
Discussion: https://postgr.es/m/420584fd274f9ec4f337da55ffb3b790%40oss.nttdata.com
Backpatch-through: 15
2024-10-18 10:20:15 -05:00
Peter Eisentraut
4b652692e9 Fix memory leaks from incorrect strsep() uses
Commit 5d2e1cc117b introduced some strsep() uses, but it did the
memory management wrong in some cases.  We need to keep a separate
pointer to the allocate memory so that we can free it later, because
strsep() advances the pointer we pass to it, and it at the end it
will be NULL, so any free() calls won't do anything.

(This fixes two of the four places changed in commit 5d2e1cc117b.  The
other two don't have this problem.)

Reported-by: Alexander Lakhin <exclusion@gmail.com>
Discussion: https://www.postgresql.org/message-id/flat/79692bf9-17d3-41e6-b9c9-fc8c3944222a@eisentraut.org
2024-10-18 11:29:20 +02:00
Peter Eisentraut
24a36f91e3 Fix strsep() use for SCRAM secrets parsing
The previous code (from commit 5d2e1cc117b) did not detect end of
string correctly, so it would fail to error out if fewer than the
expected number of fields were present, which could then later lead to
a crash when NULL string pointers are accessed.

Reported-by: Alexander Lakhin <exclusion@gmail.com>
Reported-by: Ranier Vilela <ranier.vf@gmail.com>
Discussion: https://www.postgresql.org/message-id/flat/79692bf9-17d3-41e6-b9c9-fc8c3944222a@eisentraut.org
2024-10-18 11:15:54 +02:00
Fujii Masao
9272bdeac8 Remove unused code for unlogged materialized views.
Commit 3bf3ab8c56 initially introduced support for unlogged
materialized views, but this was later disallowed by commit 3223b25ff7.
Additionally, commit d25f519107 added more code for handling
unlogged materialized views. This commit cleans up all unused
code related to them.

If unlogged materialized views had been supported in any official
release, psql would need to retain code to handle them for compatibility
with older servers. However, since they were never included in
an official release, this code is no longer necessary.

Author: Pixian Shi
Reviewed-by: Yugo Nagata, Fujii Masao
Discussion: https://postgr.es/m/CAAccyYKRZ=OvAvgowiSH+OELbStLP=p2Ht=R3CgT=OaNSH5DAA@mail.gmail.com
2024-10-18 17:18:57 +09:00
Michael Paquier
19567b3eb4 Fix description of PostgreSQL::Test::Cluster::wait_for_event()
The arguments of the function were listed in an incorrect order in the
description of the routine.  This information can be seen with perldoc.

Issue spotted while working on this area of the code.

Backpatch-through: 17
2024-10-18 13:49:58 +09:00
Jeff Davis
eecd9138a0 Improve ThrowErrorData() comments for use with soft errors.
Reviewed-by: Corey Huinker
Discussion: https://postgr.es/m/901ab7cf01957f92ea8b30b6feeb0eacfb7505fc.camel@j-davis.com
2024-10-17 14:56:44 -07:00
Tom Lane
1fed234f9f ecpg: fix more minor mishandling of bad input in preprocessor.
Don't get confused by an unmatched right brace in the input.
(Previously, this led to discarding information about file-level
variables and then possibly crashing.)

Detect, rather than crash on, an attempt to index into a non-array
variable.

As before, in the absence of field complaints I'm not too
excited about back-patching these.

Per valgrind testing by Alexander Lakhin.

Discussion: https://postgr.es/m/a239aec2-6c79-5fc9-9272-cea41158a360@gmail.com
2024-10-17 15:28:32 -04:00
Thomas Munro
98c7c7152d Fix extreme skew detection in Parallel Hash Join.
After repartitioning the inner side of a hash join that would have
exceeded the allowed size, we check if all the tuples from a parent
partition moved to one child partition.  That is evidence that it
contains duplicate keys and later attempts to repartition will also
fail, so we should give up trying to limit memory (for lack of a better
fallback strategy).

A thinko prevented the check from working correctly in partition 0 (the
one that is partially loaded into memory already).  After
repartitioning, we should check for extreme skew if the *parent*
partition's space_exhausted flag was set, not the child partition's.
The consequence was repeated futile repartitioning until per-partition
data exceeded various limits including "ERROR: invalid DSA memory alloc
request size 1811939328", OS allocation failure, or temporary disk space
errors.  (We could also do something about some of those symptoms, but
that's material for separate patches.)

This problem only became likely when PostgreSQL 16 introduced support
for Parallel Hash Right/Full Join, allowing NULL keys into the hash
table.  Repartitioning always leaves NULL in partition 0, no matter how
many times you do it, because the hash value is all zero bits.  That's
unlikely for other hashed values, but they might still have caused
wasted extra effort before giving up.

Back-patch to all supported releases.

Reported-by: Craig Milhiser <craig@milhiser.com>
Reviewed-by: Andrei Lepikhov <lepihov@gmail.com>
Discussion: https://postgr.es/m/CA%2BwnhO1OfgXbmXgC4fv_uu%3DOxcDQuHvfoQ4k0DFeB0Qqd-X-rQ%40mail.gmail.com
2024-10-17 22:11:59 +13:00
Peter Eisentraut
d893a299ce Remove superfluous forward declaration
The need for this was removed by commit dc9c3b0ff21.
2024-10-17 08:57:56 +02:00
Peter Eisentraut
6234a9ce0e Fix whitespace 2024-10-17 08:43:08 +02:00
Peter Eisentraut
665785d85f Fix unnecessary casts of copyObject() result
The result is already of the correct type, so these casts don't do
anything.

Reviewed-by: Nathan Bossart <nathandbossart@gmail.com>
Reviewed-by: Tender Wang <tndrwang@gmail.com>
Discussion: https://www.postgresql.org/message-id/flat/637eeea8-5663-460b-a114-39572c0f6c6e%40eisentraut.org
2024-10-17 08:36:48 +02:00
Peter Eisentraut
eafda78fc4 Improve node type forward reference
Instead of using Node *, we can use an incomplete struct.  That way,
everything has the correct type and fewer casts are required.  This
technique is already used elsewhere in node type definitions.

Reviewed-by: Nathan Bossart <nathandbossart@gmail.com>
Reviewed-by: Tender Wang <tndrwang@gmail.com>
Discussion: https://www.postgresql.org/message-id/flat/637eeea8-5663-460b-a114-39572c0f6c6e%40eisentraut.org
2024-10-17 08:36:48 +02:00
Peter Eisentraut
41b023946d jsonapi: fully initialize dummy lexer
Valgrind reports that checks on lex->inc_state are undefined for the
"dummy lexer" used for incremental parsing, since it's only partially
initialized on the stack. This was introduced in 0785d1b8b2.
Zero-initialize the whole struct.

Author: Jacob Champion <jacob.champion@enterprisedb.com>
Reported-by: Alexander Lakhin <exclusion@gmail.com>
Discussion: https://www.postgresql.org/message-id/CAOYmi+n9QWr4gsAADZc6qFQjFViXQYVk=gBy_EvxuqsgPJcb_g@mail.gmail.com
2024-10-17 08:23:46 +02:00
Peter Eisentraut
342fb8a332 Fix unusual include style
Project-internal header files should be included using " ", not < >.
2024-10-17 08:14:45 +02:00
David Rowley
9ca67658d1 Don't store intermediate hash values in ExprState->resvalue
adf97c156 made it so ExprStates could support hashing and changed Hash
Join to use that instead of manually extracting Datums from tuples and
hashing them one column at a time.

When hashing multiple columns or expressions, the code added in that
commit stored the intermediate hash value in the ExprState's resvalue
field.  That was a mistake as steps may be injected into the ExprState
between each hashing step that look at or overwrite the stored
intermediate hash value.  EEOP_PARAM_SET is an example of such a step.

Here we fix this by adding a new dedicated field for storing
intermediate hash values and adjust the code so that all apart from the
final hashing step store their result in the intermediate field.

In passing, rename a variable so that it's more aligned to the
surrounding code and also so a few lines stay within the 80 char margin.

Reported-by: Andres Freund
Reviewed-by: Alena Rybakina <a.rybakina@postgrespro.ru>
Discussion: https://postgr.es/m/CAApHDvqo9eenEFXND5zZ9JxO_k4eTA4jKMGxSyjdTrsmYvnmZw@mail.gmail.com
2024-10-17 14:25:08 +13:00
Michael Paquier
089aac631b Fix validation of COPY FORCE_NOT_NULL/FORCE_NULL for the all-column cases
This commit adds missing checks for COPY FORCE_NOT_NULL and FORCE_NULL
when applied to all columns via "*".  These options now correctly
require CSV mode and are disallowed in COPY TO, making their behavior
consistent with FORCE_QUOTE.

Some regression tests are added to verify the correct behavior for the
all-columns case, including FORCE_QUOTE, which was not tested.

Backpatch down to 17, where support for the all-column grammar with
FORCE_NOT_NULL and FORCE_NULL has been added.

Author: Joel Jacobson
Reviewed-by: Zhang Mingli
Discussion: https://postgr.es/m/65030d1d-5f90-4fa4-92eb-f5f50389858e@app.fastmail.com
Backpatch-through: 17
2024-10-17 08:44:50 +09:00
Michael Paquier
03bf0d9a4b Rewrite some regression queries for option checks with COPY
Some queries in copy2 are there to check various option combinations,
and used "stdin" or "stdout" incompatible with the COPY TO or FROM
clauses combined with them, which was confusing.  This commit rewrites
these queries to use a compatible grammar.

The coverage of the tests is unchanged.  Like the original commit
451d1164b9d0, backpatch down to 16 where these have been introduced.  A
follow-up commit will rely on this area of the tests for a bug fix.

Author: Joel Jacobson
Reviewed-by: Zhang Mingli
Discussion: https://postgr.es/m/65030d1d-5f90-4fa4-92eb-f5f50389858e@app.fastmail.com
Backpatch-through: 16
2024-10-17 07:21:35 +09:00
Peter Geoghegan
c0490b0ef7 nbtree: fix read page recheck typo.
Oversight in commit 79fa7b3b.
2024-10-16 17:38:38 -04:00
Tom Lane
c96de42c4b Further refine _SPI_execute_plan's rule for atomic execution.
Commit 2dc1deaea turns out to have been still a brick shy of a load,
because CALL statements executing within a plpgsql exception block
could still pass the wrong snapshot to stable functions within the
CALL's argument list.  That happened because standard_ProcessUtility
forces isAtomicContext to true if IsTransactionBlock is true, which
it always will be inside a subtransaction.  Then ExecuteCallStmt
would think it does not need to push a new snapshot --- but
_SPI_execute_plan didn't do so either, since it thought it was in
nonatomic mode.

The best fix for this seems to be for _SPI_execute_plan to operate
in atomic execution mode if IsSubTransaction() is true, even when the
SPI context as a whole is non-atomic.  This makes _SPI_execute_plan
have the same rules about when non-atomic execution is allowed as
_SPI_commit/_SPI_rollback have about when COMMIT/ROLLBACK are allowed,
which seems appropriately symmetric.  (If anyone ever tries to allow
COMMIT/ROLLBACK inside a subtransaction, this would all need to be
rethought ... but I'm unconvinced that such a thing could be logically
consistent at all.)

For further consistency, also check IsSubTransaction() in
SPI_inside_nonatomic_context.  That does not matter for its
one present-day caller StartTransaction, which can't be reached
inside a subtransaction.  But if any other callers ever arise,
they'd presumably want this definition.

Per bug #18656 from Alexander Alehin.  Back-patch to all
supported branches, like previous fixes in this area.

Discussion: https://postgr.es/m/18656-cade1780866ef66c@postgresql.org
2024-10-16 17:36:40 -04:00
Jeff Davis
a7f2f6adc2 Whitespace fixup from generated unicode tables.
When running the 'update-unicode' build target, generate files that
conform to pgindent whitespace rules.
2024-10-16 12:21:13 -07:00
Jeff Davis
b360d1762b Fix #include order from e839c8ecc9.
Reported-by: Alexander Korotkov
Discussion: https://postgr.es/m/CAPpHfduAiGSsvUc614Z-JOnyQffcMeJncWMF2HnUL8wFy4fuWA@mail.gmail.com
2024-10-16 12:13:40 -07:00
Masahiko Sawada
1b9b6cc345 Reduce memory block size for decoded tuple storage to 8kB.
Commit a4ccc1cef introduced the Generation Context and modified the
logical decoding process to use a Generation Context with a fixed
block size of 8MB for storing tuple data decoded during logical
decoding (i.e., rb->tup_context). Several reports have indicated that
the logical decoding process can be terminated due to
out-of-memory (OOM) situations caused by excessive memory usage in
rb->tup_context.

This issue can occur when decoding a workload involving several
concurrent transactions, including a long-running transaction that
modifies tuples. By design, the Generation Context does not free a
memory block until all chunks within that block are
released. Consequently, if tuples modified by the long-running
transaction are stored across multiple memory blocks, these blocks
remain allocated until the long-running transaction completes, leading
to substantial memory fragmentation. The memory usage during logical
decoding, tracked by rb->size, does not account for memory
fragmentation, resulting in potentially much higher memory consumption
than the value of the logical_decoding_work_mem parameter.

Various improvement strategies were discussed in the relevant
thread. This change reduces the block size of the Generation Context
used in rb->tup_context from 8MB to 8kB. This modification
significantly decreases the likelihood of substantial memory
fragmentation occurring and is relatively straightforward to
backport. Performance testing across multiple platforms has confirmed
that this change will not introduce any performance degradation that
would impact actual operation.

Backport to all supported branches.

Reported-by: Alex Richman, Michael Guissine, Avi Weinberg
Reviewed-by: Amit Kapila, Fujii Masao, David Rowley
Tested-by: Hayato Kuroda, Shlok Kyal
Discussion: https://postgr.es/m/CAD21AoBTY1LATZUmvSXEssvq07qDZufV4AF-OHh9VD2pC0VY2A%40mail.gmail.com
Backpatch-through: 12
2024-10-16 12:08:05 -07:00
Tom Lane
9b4bf51690 ecpg: fix some minor mishandling of bad input in preprocessor.
Avoid null-pointer crash when considering a cursor declaration
that's outside any C function (a case which is useless anyway).

Ensure a cursor for a prepared statement is marked as initially
not open.  At worst, if we chanced to get not-already-zeroed memory
from malloc(), this oversight would result in failing to issue a
"cursor "foo" has been declared but not opened" warning that would
have been appropriate.

Avoid running off the end of the buffer when there are mismatched
square brackets following a variable name.  This could lead to
SIGSEGV after reaching the end of memory.

Given the lack of field complaints, none of these seem to be worth
back-patching, but let's clean them up in HEAD.

Per valgrind testing by Alexander Lakhin.

Discussion: https://postgr.es/m/5f5bcecd-d7ec-b8c0-6c92-d1a7c6e0f639@gmail.com
2024-10-16 12:25:00 -04:00