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-patch to all supported
releases, like that commit.
Author: Robert Haas <robertmhaas@gmail.com>
Reported-by: Thomas Munro <thomas.munro@gmail.com>
Discussion: https://postgr.es/m/CA%2BhUKG%2B-2rjGZC2kwqr2NMLBcEBp4uf59QT1advbWYF_uc%2B0Aw%40mail.gmail.com
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-patch to all supported branches.
Tom Lane [Sat, 30 Nov 2024 17:42:20 +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 patch 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 patch.
Patch 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-patch to v16.
Discussion: https://postgr.es/m/E1tGn4j-0003zi-MP@gemulon.postgresql.org
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 nulled 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-patch to v16, as the previous
patch was.
Discussion: https://postgr.es/m/CACZ67_UA_EVrqiFXJu9XK50baEpH=ofEPJswa2kFxg6xuSw-ww@mail.gmail.com
Thomas Munro [Thu, 28 Nov 2024 02:32:57 +0000 (15:32 +1300)]
Skip SectionMemoryManager.h in cpluspluscheck.
Commit
9044fc1d45a0 skipped SectionMemoryManager.h in headerscheck, and
by extension also cpluspluscheck, because it's C++ and would fail both
tests. That worked in master and REL_17_STABLE due to
7b8e2ae2fd3b, but
older branches have a separate cpluspluscheck script. We need to copy
the filtering rule into there too.
This problem was being reported by CI's CompilerWarnings task in the 15
and 16 branches, but was a victim of alert fatigue syndrome (unrelated
problems in the back-branches).
Fix 16, and back-patch to 13, as those are the live branches that have a
separate cpluspluscheck script.
Michael Paquier [Thu, 28 Nov 2024 00:43:22 +0000 (09:43 +0900)]
Revert "Handle better implicit transaction state of pipeline mode"
This reverts commit
d77f91214fb7 on all stable branches, due to concerns
regarding the compatility side effects this could create in a minor
release. The change still exists on HEAD.
Discussion: https://postgr.es/m/CA+TgmoZqRgeFTg4+Yf_CMRRXiHuNz1u6ZC4FvVk+rxw0RmOPnw@mail.gmail.com
Backpatch-through: 13
Andres Freund [Wed, 27 Nov 2024 16:30:08 +0000 (11:30 -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 github account/repo such as postgres/postgres, stop fighting with
each other, adding and removing packages.
Back-patch to 15 where CI began.
Author: Thomas Munro <thomas.munro@gmail.com>
Author: Nazir Bilal Yavuz <byavuz81@gmail.com>
Suggested-by: Andres Freund <andres@anarazel.de>
Discussion: https://postgr.es/m/au2uqfuy2nf43nwy2txmc5t2emhwij7kzupygto3d2ffgtrdgr%40ckvrlwyflnh2
Fujii Masao [Wed, 27 Nov 2024 14:03:44 +0000 (23:03 +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-patch 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
Peter Eisentraut [Wed, 27 Nov 2024 10:12:09 +0000 (11:12 +0100)]
Fix typo
from commit
9044fc1d45a
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.
Michael Paquier [Wed, 27 Nov 2024 00:31:38 +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 backpatched 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
Backpatch-through: 13
Andrew Dunstan [Sun, 26 May 2024 21:34:45 +0000 (17:34 -0400)]
Fix meson uuid header check so it works with MSVC
The OSSP uuid.h file includes unistd.h, so to use it with MSVC we need to
include the postgres include directories so it picks up our version of
that in src/include/port/win32_msvc. Adjust the meson test accordingly.
Backported from commit
7c655a04a2, so we can build release 16 with UUID
per request from Marina Polyakova
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) <r.takahashi_2@fujitsu.com>
Discussion: https://www.postgresql.org/message-id/flat/TY3PR01MB118912125614599641CA881B782522%40TY3PR01MB11891.jpnprd01.prod.outlook.com
Peter Eisentraut [Tue, 22 Oct 2024 06:12:43 +0000 (08:12 +0200)]
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
Discussion: https://www.postgresql.org/message-id/flat/87o72eo9iu.fsf%40gentoo.org
Peter Eisentraut [Tue, 13 Aug 2024 04:15:28 +0000 (06:15 +0200)]
Rename C23 keyword
constexpr is a keyword in C23. Rename a conflicting identifier for
future-proofing.
Reviewed-by: Robert Haas <robertmhaas@gmail.com>
Discussion: https://www.postgresql.org/message-id/flat/
08abc832-1384-4aca-a535-
1a79765b565e%40eisentraut.org
Discussion: https://www.postgresql.org/message-id/flat/87o72eo9iu.fsf%40gentoo.org
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-patch to all
supported branches.
Discussion: https://postgr.es/m/18722-
fd9e645448cc78b4@postgresql.org
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-patch 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-patch of a SearchSysCacheLocked1() call.
Reported by Aya Iwata.
Discussion: https://postgr.es/m/OS7PR01MB11964507B5548245A7EE54E70EA212@OS7PR01MB11964.jpnprd01.prod.outlook.com
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-patch to 16, where meson begins.
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-patch to all supported branches.
Discussion: https://postgr.es/m/
4496616.iHFcN1HehY@portable-bastien
Peter Eisentraut [Mon, 25 Nov 2024 07:03:16 +0000 (08:03 +0100)]
Add support for Tcl 9
Tcl 9 changed several API functions to take Tcl_Size, which is
ptrdiff_t, instead of int, for 64-bit enablement. We have to change a
few local variables to be compatible with that. We also provide a
fallback typedef of Tcl_Size for older Tcl versions.
The affected variables are used for quantities that will not approach
values beyond the range of int, so this doesn't change any
functionality.
Reviewed-by: Tristan Partin <tristan@partin.io>
Discussion: https://www.postgresql.org/message-id/flat/
bce0fe54-75b4-438e-b42b-
8e84bc7c0e9c%40eisentraut.org
Thomas Munro [Mon, 25 Nov 2024 00:11:28 +0000 (13:11 +1300)]
Assume that <stdbool.h> conforms to the C standard.
Previously we checked "for <stdbool.h> that conforms to C99" using
autoconf's AC_HEADER_STDBOOL macro. We've required C99 since PostgreSQL
12, so the test was redundant, and under C23 it was broken: autoconf
2.69's implementation doesn't understand C23's new empty header (the
macros it's looking for went away, replaced by language keywords).
Later autoconf versions fixed that, but let's just remove the
anachronistic test.
HAVE_STDBOOL_H and HAVE__BOOL will no longer be defined, but they
weren't directly tested in core or likely extensions (except in 11, see
below). PG_USE_STDBOOL (or USE_STDBOOL in 11 and 12) is still defined
when sizeof(bool) is 1, which should be true on all modern systems.
Otherwise we define our own bool type and values of size 1, which would
fail to compile under C23 as revealed by the broken test. (We'll
probably clean that dead code up in master, but here we want a minimal
back-patchable change.)
This came to our attention when GCC 15 recently started using using C23
by default and failed to compile the replacement code, as reported by
Sam James and build farm animal alligator.
Back-patch to all supported releases, and then two older versions that
also know about <stdbool.h>, per the recently-out-of-support policy[1].
12 requires C99 so it's much like the supported releases, but 11 only
assumes C89 so it now uses AC_CHECK_HEADERS instead of the overly picky
AC_HEADER_STDBOOL. (I could find no discussion of which historical
systems had <stdbool.h> but failed the conformance test; if they ever
existed, they surely aren't relevant to that policy's goals.)
[1] https://wiki.postgresql.org/wiki/Committing_checklist#Policies
Reported-by: Sam James <sam@gentoo.org>
Reviewed-by: Peter Eisentraut <peter@eisentraut.org> (master version)
Reviewed-by: Tom Lane <tgl@sss.pgh.pa.us> (approach)
Discussion: https://www.postgresql.org/message-id/flat/87o72eo9iu.fsf%40gentoo.org
Michael Paquier [Mon, 25 Nov 2024 00:15:43 +0000 (09:15 +0900)]
doc: Fix example with __next__() in PL/Python function
Per PEP 3114, iterator.next() has been renamed to iterator.__next__(),
and one example in the documentation still used next(). This caused the
example provided to fail the function creation since Python 2 is not
supported anymore since
19252e8ec93.
Author: Erik Wienhold
Discussion: https://postgr.es/m/
173209043143.
2092749.
13692266486972491694@wrigleys.postgresql.org
Backpatch-through: 15
Michael Paquier [Fri, 22 Nov 2024 03:17:55 +0000 (12:17 +0900)]
psql: Include \pset xheader_width in --help=commands|variables
psql's --help was missed the description of the \pset variable
xheader_width, that should be listed when using \? or --help=commands,
and described for --help=variables.
Oversight in
a45388d6e098.
Author: Pavel Luzanov
Discussion: https://postgr.es/m/
1e3e06d6-0807-4e62-a9f6-
c11481e6eb10@postgrespro.ru
Backpatch-through: 16
Thomas Munro [Fri, 22 Nov 2024 01:53:21 +0000 (14:53 +1300)]
jit: Use -mno-outline-atomics for bitcode on ARM.
If the executable's .o files were produced by a compiler (probably gcc)
not using -moutline-atomics, and the corresponding .bc files were
produced by clang using -moutline-atomics (probably by default), then
the generated bitcode functions would have the target attribute
"+outline-atomics", and could fail at runtime when inlined. If the
target ISA at bitcode generation time was armv8-a (the most conservative
aarch64 target, no LSE), then LLVM IR atomic instructions would generate
calls to functions in libgcc.a or libclang_rt.*.a that switch between
LL/SC and faster LSE instructions depending on a runtime AT_HWCAP check.
Since the corresponding .o files didn't need those functions, they
wouldn't have been included in the executable, and resolution would
fail.
At least Debian and Ubuntu are known to ship gcc and clang compilers
that target armv8-a but differ on the use of outline atomics by default.
Fix, by suppressing the outline atomics attribute in bitcode explicitly.
Inline LL/SC instructions will be generated for atomic operations in
bitcode built for armv8-a. Only configure scripts are adjusted for now,
because the meson build system doesn't generate bitcode yet.
This doesn't seem to be a new phenomenon, so real cases of functions
using atomics that are inlined by JIT must be rare in the wild given how
long it took for a bug report to arrive. The reported case could be
reduced to:
postgres=# set jit_inline_above_cost = 0;
SET
postgres=# set jit_above_cost = 0;
SET
postgres=# select pg_last_wal_receive_lsn();
WARNING: failed to resolve name __aarch64_swp4_acq_rel
FATAL: fatal llvm error: Program used external function
'__aarch64_swp4_acq_rel' which could not be resolved!
The change doesn't affect non-ARM systems or later target ISAs.
Back-patch to all supported releases.
Reported-by: Alexander Kozhemyakin <a.kozhemyakin@postgrespro.ru>
Discussion: https://postgr.es/m/18610-
37bf303f904fede3%40postgresql.org
Álvaro Herrera [Thu, 21 Nov 2024 16:04:26 +0000 (17:04 +0100)]
Fix newly introduced 010_keep_recycled_wals.pl
It failed to set the archive_command as it desired because of a syntax
problem. Oversight in commit
90bcc7c2db1d.
This bug doesn't cause the test to fail, because the test only checks
pg_rewind's output messages, not the actual outcome (and the outcome in
both cases is that the file is kept, not deleted). But in either case
the message about the file being kept is there, so it's hard to get
excited about doing much more.
Reported-by: Antonin Houska <ah@cybertec.at>
Author: Alexander Kukushkin <cyberdemn@gmail.com>
Discussion: https://postgr.es/m/7822.
1732167825@antos
Álvaro Herrera [Thu, 21 Nov 2024 15:54:36 +0000 (16:54 +0100)]
Fix outdated bit in README.tuplock
Apparently this information has been outdated since first committed,
because we adopted a different implementation during development per
reviews and this detail was not updated in the README.
This has been wrong since commit
0ac5ad5134f2 introduced the file in
2013. Backpatch to all live branches.
Reported-by: Will Mortensen <will@extrahop.com>
Discussion: https://postgr.es/m/CAMpnoC6yEQ=c0Rdq-J7uRedrP7Zo9UMp6VZyP23QMT68n06cvA@mail.gmail.com
Michael Paquier [Thu, 21 Nov 2024 06:14:13 +0000 (15:14 +0900)]
Fix memory leak in pgoutput for the WAL sender
RelationSyncCache, the hash table in charge of tracking the relation
schemas sent through pgoutput, was forgetting to free the TupleDesc
associated to the two slots used to store the new and old tuples,
causing some memory to be leaked each time a relation is invalidated
when the slots of an existing relation entry are cleaned up.
This is rather hard to notice as the bloat is pretty minimal, but a
long-running WAL sender would be in trouble over time depending on the
workload. sysbench has proved to be pretty good at showing the problem,
coupled with some memory monitoring of the WAL sender.
Issue introduced in
52e4f0cd472d, that has added row filters for tables
logically replicated.
Author: Boyu Yang
Reviewed-by: Michael Paquier, Hou Zhijie
Discussion: https://postgr.es/m/DM3PR84MB3442E14B340E553313B5C816E3252@DM3PR84MB3442.NAMPRD84.PROD.OUTLOOK.COM
Backpatch-through: 15
Tom Lane [Wed, 20 Nov 2024 17:03:47 +0000 (12:03 -0500)]
Avoid assertion failure if a setop leaf query contains setops.
Ordinarily transformSetOperationTree will collect all UNION/
INTERSECT/EXCEPT steps into the setOperations tree of the topmost
Query, so that leaf queries do not contain any setOperations.
However, it cannot thus flatten a subquery that also contains
WITH, ORDER BY, FOR UPDATE, or LIMIT. I (tgl) forgot that in
commit
07b4c48b6 and wrote an assertion in rule deparsing that
a leaf's setOperations would always be empty.
If it were nonempty then we would want to parenthesize the subquery
to ensure that the output represents the setop nesting correctly
(e.g. UNION below INTERSECT had better get parenthesized). So
rather than just removing the faulty Assert, let's change it into
an additional case to check to decide whether to add parens. We
don't expect that the additional case will ever fire, but it's
cheap insurance.
Man Zeng and Tom Lane
Discussion: https://postgr.es/m/tencent_7ABF9B1F23B0C77606FC5FE3@qq.com
Michael Paquier [Wed, 20 Nov 2024 05:21:17 +0000 (14:21 +0900)]
doc: Fix section of functions age(xid) and mxid_age(xid)
In 17~, age(xid) and mxid_age(xid) were listed as deprecated. Based on
the discussion that led to
48b5aa3143, this is not intentional as this
could break many existing monitoring queries. Note that vacuumdb also
uses both of them.
In 16, both functions were listed under "Control Data Functions", which
is incorrect, so let's move them to the list of functions related to
transaction IDs and snapshots.
Author: Bertrand Drouvot
Discussion: https://postgr.es/m/Zzr2zZFyeFKXWe8a@ip-10-97-1-34.eu-west-3.compute.internal
Discussion: https://postgr.es/m/
20231114013224.4z6oxa6p6va33rxr@awork3.anarazel.de
Backpatch-through: 16
Tom Lane [Tue, 19 Nov 2024 23:26:19 +0000 (18:26 -0500)]
Compare collations before merging UNION operations.
In the dim past we figured it was okay to ignore collations
when combining UNION set-operation nodes into a single N-way
UNION operation. I believe that was fine at the time, but
it stopped being fine when we added nondeterministic collations:
the semantics of distinct-ness are affected by those. v17 made
it even less fine by allowing per-child sorting operations to
be merged via MergeAppend, although I think we accidentally
avoided any live bug from that.
Add a check that collations match before deciding that two
UNION nodes are equivalent. I also failed to resist the
temptation to comment plan_union_children() a little better.
Back-patch to all supported branches (v13 now), since they
all have nondeterministic collations.
Discussion: https://postgr.es/m/
3605568.
1731970579@sss.pgh.pa.us
Tom Lane [Mon, 18 Nov 2024 20:33:54 +0000 (15:33 -0500)]
Stamp 16.6.
Tom Lane [Sat, 16 Nov 2024 22:09:53 +0000 (17:09 -0500)]
Release notes for 17.2, 16.6, 15.10, 14.15, 13.18, 12.22.
Tom Lane [Sat, 16 Nov 2024 17:58:26 +0000 (12:58 -0500)]
Undo unintentional ABI break in struct ResultRelInfo.
Commits
aac2c9b4f et al. added a bool field to struct ResultRelInfo.
That's no problem in the master branch, but in released branches
care must be taken when modifying publicly-visible structs to avoid
an ABI break for extensions. Frequently we solve that by adding the
new field at the end of the struct, and that's what was done here.
But ResultRelInfo has stricter constraints than just about any other
node type in Postgres. Some executor APIs require extensions to index
into arrays of ResultRelInfo, which means that any change whatever in
sizeof(ResultRelInfo) causes a fatal ABI break.
Fortunately, this is easy to fix, because the new field can be
squeezed into available padding space instead --- indeed, that's where
it was put in master, so this fix also removes a cross-branch coding
variation.
Per report from Pavan Deolasee. Patch v14-v17 only; earlier versions
did not gain the extra field, nor is there any problem in master.
Discussion: https://postgr.es/m/CABOikdNmVBC1LL6pY26dyxAS2f+gLZvTsNt=2XbcyG7WxXVBBQ@mail.gmail.com
Noah Misch [Sat, 16 Nov 2024 04:39:56 +0000 (20:39 -0800)]
Fix per-session activation of ALTER {ROLE|DATABASE} SET role.
After commit
5a2fed911a85ed6d8a015a6bafe3a0d9a69334ae, the catalog state
resulting from these commands ceased to affect sessions. Restore the
longstanding behavior, which is like beginning the session with a SET
ROLE command. If cherry-picking the CVE-2024-10978 fixes, default to
including this, too. (This fixes an unintended side effect of fixing
CVE-2024-10978.) Back-patch to v12, like that commit. The release team
decided to include v12, despite the original intent to halt v12 commits
earlier this week.
Tom Lane and Noah Misch. Reported by Etienne LAFARGE.
Discussion: https://postgr.es/m/CADOZwSb0UsEr4_UTFXC5k7=fyyK8uKXekucd+-uuGjJsGBfxgw@mail.gmail.com
Masahiko Sawada [Sat, 16 Nov 2024 01:06:05 +0000 (17:06 -0800)]
Fix a possibility of logical replication slot's restart_lsn going backwards.
Previously LogicalIncreaseRestartDecodingForSlot() accidentally
accepted any LSN as the candidate_lsn and candidate_valid after the
restart_lsn of the replication slot was updated, so it potentially
caused the restart_lsn to move backwards.
A scenario where this could happen in logical replication is: after a
logical replication restart, based on previous candidate_lsn and
candidate_valid values in memory, the restart_lsn advances upon
receiving a subscriber acknowledgment. Then, logical decoding restarts
from an older point, setting candidate_lsn and candidate_valid based
on an old RUNNING_XACTS record. Subsequent subscriber acknowledgments
then update the restart_lsn to an LSN older than the current value.
In the reported case, after WAL files were removed by a checkpoint,
the retreated restart_lsn prevented logical replication from
restarting due to missing WAL segments.
This change essentially modifies the 'if' condition to 'else if'
condition within the function. The previous code had an asymmetry in
this regard compared to LogicalIncreaseXminForSlot(), which does
almost the same thing for different fields.
The WAL removal issue was reported by Hubert Depesz Lubaczewski.
Backpatch to all supported versions, since the bug exists since 9.4
where logical decoding was introduced.
Reviewed-by: Tomas Vondra, Ashutosh Bapat, Amit Kapila
Discussion: https://postgr.es/m/Yz2hivgyjS1RfMKs%40depesz.com
Discussion: https://postgr.es/m/
85fff40e-148b-4e86-b921-
b4b846289132%40vondra.me
Backpatch-through: 13
Tom Lane [Fri, 15 Nov 2024 23:23:38 +0000 (18:23 -0500)]
Avoid assertion due to disconnected NFA sub-graphs in regex parsing.
In commit
08c0d6ad6 which introduced "rainbow" arcs in regex NFAs,
I didn't think terribly hard about what to do when creating the color
complement of a rainbow arc. Clearly, the complement cannot match any
characters, and I took the easy way out by just not building any arcs
at all in the complement arc set. That mostly works, but Nikolay
Shaplov found a case where it doesn't: if we decide to delete that
sub-NFA later because it's inside a "{0}" quantifier, delsub()
suffered an assertion failure. That's because delsub() relies on
the target sub-NFA being fully connected. That was always true
before, and the best fix seems to be to restore that property.
Hence, invent a new arc type CANTMATCH that can be generated in
place of an empty color complement, and drop it again later when we
start NFA optimization. (At that point we don't need to do delsub()
any more, and besides there are other cases where NFA optimization can
lead to disconnected subgraphs.)
It appears that this bug has no consequences in a non-assert-enabled
build: there will be some transiently leaked NFA states/arcs, but
they'll get cleaned up eventually. Still, we don't like assertion
failures, so back-patch to v14 where rainbow arcs were introduced.
Per bug #18708 from Nikolay Shaplov.
Discussion: https://postgr.es/m/18708-
f94f2599c9d2c005@postgresql.org
Álvaro Herrera [Fri, 15 Nov 2024 11:53:12 +0000 (12:53 +0100)]
Avoid deleting critical WAL segments during pg_rewind
Previously, in unlucky cases, it was possible for pg_rewind to remove
certain WAL segments from the rewound demoted primary. In particular
this happens if those files have been marked for archival (i.e., their
.ready files were created) but not yet archived; the newly promoted node
no longer has such files because of them having been recycled, but they
are likely critical for recovery in the demoted node. If pg_rewind
removes them, recovery is not possible anymore.
Fix this by maintaining a hash table of files in this situation in the
scan that looks for a checkpoint, which the decide_file_actions phase
can consult so that it knows to preserve them.
Backpatch to 14. The problem also exists in 13, but that branch was not
blessed with commit
eb00f1d4bf96, so this patch is difficult to apply
there. Users of older releases will just have to continue to be extra
careful when rewinding.
Co-authored-by: Полина Бунгина (Polina Bungina) <bungina@gmail.com>
Co-authored-by: Alexander Kukushkin <cyberdemn@gmail.com>
Reviewed-by: Kyotaro Horiguchi <horikyota.ntt@gmail.com>
Reviewed-by: Atsushi Torikoshi <torikoshia@oss.nttdata.com>
Discussion: https://postgr.es/m/CAAtGL4AhzmBRsEsaDdz7065T+k+BscNadfTqP1NcPmsqwA5HBw@mail.gmail.com
Michael Paquier [Fri, 15 Nov 2024 02:32:16 +0000 (11:32 +0900)]
Fix race conditions with drop of reused pgstats entries
This fixes a set of race conditions with cumulative statistics where a
shared stats entry could be dropped while it should still be valid in
the event when it is reused: an entry may refer to a different object
but requires the same hash key. This can happen with various stats
kinds, like:
- Replication slots that compute internally an index number, for
different slot names.
- Stats kinds that use an OID in the object key, where a wraparound
causes the same key to be used if an OID is used for the same object.
- As of PostgreSQL 18, custom pgstats kinds could also be an issue,
depending on their implementation.
This issue is fixed by introducing a counter called "generation" in the
shared entries via PgStatShared_HashEntry, initialized at 0 when an
entry is created and incremented when the same entry is reused, to avoid
concurrent issues on drop because of other backends still holding a
reference to it. This "generation" is copied to the local copy that a
backend holds when looking at an object, then cross-checked with the
shared entry to make sure that the entry is not dropped even if its
"refcount" justifies that if it has been reused.
This problem could show up when a backend shuts down and needs to
discard any entries it still holds, causing statistics to be removed
when they should not, or even an assertion failure. Another report
involved a failure in a standby after an OID wraparound, where the
startup process would FATAL on a "can only drop stats once", stopping
recovery abruptly. The buildfarm has been sporadically complaining
about the problem, as well, but the window is hard to reach with the
in-core tests.
Note that the issue can be reproduced easily by adding a sleep before
dshash_find() in pgstat_release_entry_ref() to enlarge the problematic
window while repeating test_decoding's isolation test oldest_xmin a
couple of times, for example, as pointed out by Alexander Lakhin.
Reported-by: Alexander Lakhin, Peter Smith
Author: Kyotaro Horiguchi, Michael Paquier
Reviewed-by: Bertrand Drouvot
Discussion: https://postgr.es/m/CAA4eK1KxuMVyAryz_Vk5yq3ejgKYcL6F45Hj9ZnMNBS-g+PuZg@mail.gmail.com
Discussion: https://postgr.es/m/17947-
b9554521ad963c9c@postgresql.org
Backpatch-through: 15
Peter Geoghegan [Wed, 13 Nov 2024 01:57:41 +0000 (20:57 -0500)]
Count contrib/bloom index scans in pgstat view.
Maintain the pg_stat_user_indexes.idx_scan pgstat counter during
contrib/Bloom index scans.
Oversight in commit
9ee014fc, which added the Bloom index contrib
module.
Author: Masahiro Ikeda <ikedamsh@oss.nttdata.com>
Reviewed-By: Peter Geoghegan <pg@bowt.ie>
Discussion: https://postgr.es/m/
c48839d881388ee401a01807c686004d@oss.nttdata.com
Backpatch: 13- (all supported branches).
Alexander Korotkov [Mon, 11 Nov 2024 23:44:20 +0000 (01:44 +0200)]
Fix arrays comparison in CompareOpclassOptions()
The current code calls array_eq() and does not provide FmgrInfo. This commit
provides initialization of FmgrInfo and uses C collation as the safe option
for text comparison because we don't know anything about the semantics of
opclass options.
Backpatch to 13, where opclass options were introduced.
Reported-by: Nicolas Maus
Discussion: https://postgr.es/m/18692-
72ea398df3ec6712%40postgresql.org
Backpatch-through: 13
Tom Lane [Mon, 11 Nov 2024 22:44:10 +0000 (17:44 -0500)]
Stamp 16.5.
Tom Lane [Mon, 11 Nov 2024 22:40:13 +0000 (17:40 -0500)]
Last-minute updates for release notes.
Security: CVE-2024-10976, CVE-2024-10977, CVE-2024-10978, CVE-2024-10979
Tom Lane [Mon, 11 Nov 2024 22:05:53 +0000 (17:05 -0500)]
Parallel workers use AuthenticatedUserId for connection privilege checks.
Commit
5a2fed911 had an unexpected side-effect: the parallel worker
launched for the new test case would fail if it couldn't use a
superuser-reserved connection slot. The reason that test failed
while all our pre-existing ones worked is that the connection
privilege tests in InitPostgres had been based on the superuserness
of the leader's AuthenticatedUserId, but after the rearrangements
of
5a2fed911 we were testing the superuserness of CurrentUserId,
which the new test case deliberately made to be a non-superuser.
This all seems very accidental and probably not the behavior we really
want, but a security patch is no time to be redesigning things.
Pending some discussion about desirable semantics, hack it so that
InitPostgres continues to pay attention to the superuserness of
AuthenticatedUserId when starting a parallel worker.
Nathan Bossart and Tom Lane, per buildfarm member sawshark.
Security: CVE-2024-10978
Tom Lane [Mon, 11 Nov 2024 18:57:21 +0000 (13:57 -0500)]
Fix cross-version upgrade tests.
TestUpgradeXversion knows how to make the main regression database's
references to pg_regress.so be version-independent. But it doesn't
do that for plperl's database, so that the C function added by
commit
b7e3a52a8 is causing cross-version upgrade test failures.
Path of least resistance is to just drop the function at the end
of the new test.
In <= v14, also take the opportunity to clean up the generated
test files.
Security: CVE-2024-10979
Noah Misch [Mon, 11 Nov 2024 18:55:18 +0000 (10:55 -0800)]
src/tools/msvc: Respect REGRESS_OPTS in plcheck.
v16 commit
8fe3e697a1a83a722b107c7cb9c31084e1f4d077 used REGRESS_OPTS in
a way needing this. That broke "vcregress plcheck". Back-patch
v16..v12; newer versions don't have this build system.
Tom Lane [Mon, 11 Nov 2024 17:27:06 +0000 (12:27 -0500)]
Avoid bizarre meson behavior with backslashes in command arguments.
Ooops, missed that v16 has another text2macro call in the MSVC scripts.
Security: CVE-2024-10979
Tom Lane [Mon, 11 Nov 2024 17:20:08 +0000 (12:20 -0500)]
Avoid bizarre meson behavior with backslashes in command arguments.
meson makes the backslashes in text2macro.pl's --strip argument
into forward slashes, effectively disabling comment stripping.
That hasn't caused us issues before, but it breaks the test case
for
b7e3a52a8. We don't really need the pattern to be adjustable,
so just hard-wire it into the script instead.
Context: https://github.com/mesonbuild/meson/issues/1564
Security: CVE-2024-10979
Tom Lane [Mon, 11 Nov 2024 15:29:54 +0000 (10:29 -0500)]
Fix improper interactions between session_authorization and role.
The SQL spec mandates that SET SESSION AUTHORIZATION implies
SET ROLE NONE. We tried to implement that within the lowest-level
functions that manipulate these settings, but that was a bad idea.
In particular, guc.c assumes that it doesn't matter in what order
it applies GUC variable updates, but that was not the case for these
two variables. This problem, compounded by some hackish attempts to
work around it, led to some security-grade issues:
* Rolling back a transaction that had done SET SESSION AUTHORIZATION
would revert to SET ROLE NONE, even if that had not been the previous
state, so that the effective user ID might now be different from what
it had been.
* The same for SET SESSION AUTHORIZATION in a function SET clause.
* If a parallel worker inspected current_setting('role'), it saw
"none" even when it should see something else.
Also, although the parallel worker startup code intended to cope
with the current role's pg_authid row having disappeared, its
implementation of that was incomplete so it would still fail.
Fix by fully separating the miscinit.c functions that assign
session_authorization from those that assign role. To implement the
spec's requirement, teach set_config_option itself to perform "SET
ROLE NONE" when it sets session_authorization. (This is undoubtedly
ugly, but the alternatives seem worse. In particular, there's no way
to do it within assign_session_authorization without incompatible
changes in the API for GUC assign hooks.) Also, improve
ParallelWorkerMain to directly set all the relevant user-ID variables
instead of relying on some of them to get set indirectly. That
allows us to survive not finding the pg_authid row during worker
startup.
In v16 and earlier, this includes back-patching
9987a7bf3 which
fixed a violation of GUC coding rules: SetSessionAuthorization
is not an appropriate place to be throwing errors from.
Security: CVE-2024-10978
Nathan Bossart [Mon, 11 Nov 2024 15:00:00 +0000 (09:00 -0600)]
Ensure cached plans are correctly marked as dependent on role.
If a CTE, subquery, sublink, security invoker view, or coercion
projection references a table with row-level security policies, we
neglected to mark the plan as potentially dependent on which role
is executing it. This could lead to later executions in the same
session returning or hiding rows that should have been hidden or
returned instead.
Reported-by: Wolfgang Walther
Reviewed-by: Noah Misch
Security: CVE-2024-10976
Backpatch-through: 12
Noah Misch [Mon, 11 Nov 2024 14:23:43 +0000 (06:23 -0800)]
Block environment variable mutations from trusted PL/Perl.
Many process environment variables (e.g. PATH), bypass the containment
expected of a trusted PL. Hence, trusted PLs must not offer features
that achieve setenv(). Otherwise, an attacker having USAGE privilege on
the language often can achieve arbitrary code execution, even if the
attacker lacks a database server operating system user.
To fix PL/Perl, replace trusted PL/Perl %ENV with a tied hash that just
replaces each modification attempt with a warning. Sites that reach
these warnings should evaluate the application-specific implications of
proceeding without the environment modification:
Can the application reasonably proceed without the modification?
If no, switch to plperlu or another approach.
If yes, the application should change the code to stop attempting
environment modifications. If that's too difficult, add "untie
%main::ENV" in any code executed before the warning. For example,
one might add it to the start of the affected function or even to
the plperl.on_plperl_init setting.
In passing, link to Perl's guidance about the Perl features behind the
security posture of PL/Perl.
Back-patch to v12 (all supported versions).
Andrew Dunstan and Noah Misch
Security: CVE-2024-10979
Peter Eisentraut [Mon, 11 Nov 2024 12:53:52 +0000 (13:53 +0100)]
Translation updates
Source-Git-URL: https://git.postgresql.org/git/pgtranslation/messages.git
Source-Git-Hash:
2bf252d27e0167b62b663baaab5e9b4c773ba9de
Michael Paquier [Mon, 11 Nov 2024 01:19:58 +0000 (10:19 +0900)]
libpq: Bail out during SSL/GSS negotiation errors
This commit changes libpq so that errors reported by the backend during
the protocol negotiation for SSL and GSS are discarded by the client, as
these may include bytes that could be consumed by the client and write
arbitrary bytes to a client's terminal.
A failure with the SSL negotiation now leads to an error immediately
reported, without a retry on any other methods allowed, like a fallback
to a plaintext connection.
A failure with GSS discards the error message received, and we allow a
fallback as it may be possible that the error is caused by a connection
attempt with a pre-11 server, GSS encryption having been introduced in
v12. This was a problem only with v17 and newer versions; older
versions discard the error message already in this case, assuming a
failure caused by a lack of support for GSS encryption.
Author: Jacob Champion
Reviewed-by: Peter Eisentraut, Heikki Linnakangas, Michael Paquier
Security: CVE-2024-10977
Backpatch-through: 12
Tom Lane [Sun, 10 Nov 2024 18:40:41 +0000 (13:40 -0500)]
Release notes for 17.1, 16.5, 15.9, 14.14, 13.17, 12.21.
Tom Lane [Fri, 8 Nov 2024 18:42:01 +0000 (13:42 -0500)]
Improve fix for not entering parallel mode when holding interrupts.
Commit
ac04aa84a put the shutoff for this into the planner, which is
not ideal because it doesn't prevent us from re-using a previously
made parallel plan. Revert the planner change and instead put the
shutoff into InitializeParallelDSM, modeling it on the existing code
there for recovering from failure to allocate a DSM segment.
However, that code path is mostly untested, and testing a bit harder
showed there's at least one bug: ExecHashJoinReInitializeDSM is not
prepared for us to have skipped doing parallel DSM setup. I also
thought the Assert in ReinitializeParallelWorkers is pretty
ill-advised, and replaced it with a silent Min() operation.
The existing test case added by
ac04aa84a serves fine to test this
version of the fix, so no change needed there.
Patch by me, but thanks to Noah Misch for the core idea that we
could shut off worker creation when !INTERRUPTS_CAN_BE_PROCESSED.
Back-patch to v12, as
ac04aa84a was.
Discussion: https://postgr.es/m/CAC-SaSzHUKT=vZJ8MPxYdC_URPfax+yoA1hKTcF4ROz_Q6z0_Q@mail.gmail.com
Amit Langote [Fri, 8 Nov 2024 07:30:07 +0000 (16:30 +0900)]
Disallow partitionwise join when collations don't match
If the collation of any join key column doesn’t match the collation of
the corresponding partition key, partitionwise joins can yield incorrect
results. For example, rows that would match under the join key collation
might be located in different partitions due to the partitioning
collation. In such cases, a partitionwise join would yield different
results from a non-partitionwise join, so disallow it in such cases.
Reported-by: Tender Wang <tndrwang@gmail.com>
Author: Jian He <jian.universality@gmail.com>
Reviewed-by: Tender Wang <tndrwang@gmail.com>
Reviewed-by: Junwang Zhao <zhjwpku@gmail.com>
Discussion: https://postgr.es/m/CAHewXNno_HKiQ6PqyLYfuqDtwp7KKHZiH1J7Pqyz0nr+PS2Dwg@mail.gmail.com
Backpatch-through: 12
Amit Langote [Fri, 8 Nov 2024 07:07:05 +0000 (16:07 +0900)]
Disallow partitionwise grouping when collations don't match
If the collation of any grouping column doesn’t match the collation of
the corresponding partition key, partitionwise grouping can yield
incorrect results. For example, rows that would be grouped under the
grouping collation may end up in different partitions under the
partitioning collation. In such cases, full partitionwise grouping
would produce results that differ from those without partitionwise
grouping, so disallowed that.
Partial partitionwise aggregation is still allowed, as the Finalize
step reconciles partition-level aggregates with grouping requirements
across all partitions, ensuring that the final output remains
consistent.
This commit also fixes group_by_has_partkey() by ensuring the
RelabelType node is stripped from grouping expressions when matching
them to partition key expressions to avoid false mismatches.
Bug: #18568
Reported-by: Webbo Han <1105066510@qq.com>
Author: Webbo Han <
1105066510@qq.com>
Reviewed-by: Tender Wang <tndrwang@gmail.com>
Reviewed-by: Aleksander Alekseev <aleksander@timescale.com>
Reviewed-by: Jian He <jian.universality@gmail.com>
Discussion: https://postgr.es/m/18568-
2a9afb6b9f7e6ed3@postgresql.org
Discussion: https://postgr.es/m/tencent_9D9103CDA420C07768349CC1DFF88465F90A@qq.com
Discussion: https://postgr.es/m/CAHewXNno_HKiQ6PqyLYfuqDtwp7KKHZiH1J7Pqyz0nr+PS2Dwg@mail.gmail.com
Backpatch-through: 12
Peter Eisentraut [Fri, 8 Nov 2024 06:17:55 +0000 (07:17 +0100)]
Message style improvement
Backpatch the part of
edee0c621de that applies to
a90bdd7a44d, which
was also backpatched. That way, the message is consistent in all
branches.
Álvaro Herrera [Thu, 7 Nov 2024 13:06:24 +0000 (14:06 +0100)]
doc: Reword ALTER TABLE ATTACH restriction on NO INHERIT constraints
The previous wording is easy to read incorrectly; this change makes it
simpler, less ambiguous, and less prominent.
Backpatch to all live branches.
Reviewed-by: Amit Langote <amitlangote09@gmail.com>
Discussion: https://postgr.es/m/
202411051201.zody6mld7vkw@alvherre.pgsql
Thomas Munro [Wed, 6 Nov 2024 09:04:44 +0000 (22:04 +1300)]
Monkey-patch LLVM code to fix ARM relocation bug.
Supply a new memory manager for RuntimeDyld, to avoid crashes in
generated code caused by memory placement that can overflow a 32 bit
data type. This is a drop-in replacement for the
llvm::SectionMemoryManager class in the LLVM library, with Michael
Smith's proposed fix from
https://www.github.com/llvm/llvm-project/pull/71968.
We hereby slurp it into our own source tree, after moving into a new
namespace llvm::backport and making some minor adjustments so that it
can be compiled with older LLVM versions as far back as 12. It's harder
to make it work on even older LLVM versions, but it doesn't seem likely
that people are really using them so that is not investigated for now.
The problem could also be addressed by switching to JITLink instead of
RuntimeDyld, and that is the LLVM project's recommended solution as
the latter is about to be deprecated. We'll have to do that soon enough
anyway, and then when the LLVM version support window advances far
enough in a few years we'll be able to delete this code. Unfortunately
that wouldn't be enough for PostgreSQL today: in most relevant versions
of LLVM, JITLink is missing or incomplete.
Several other projects have already back-ported this fix into their fork
of LLVM, which is a vote of confidence despite the lack of commit into
LLVM as of today. We don't have our own copy of LLVM so we can't do
exactly what they've done; instead we have a copy of the whole patched
class so we can pass an instance of it to RuntimeDyld.
The LLVM project hasn't chosen to commit the fix yet, and even if it
did, it wouldn't be back-ported into the releases of LLVM that most of
our users care about, so there is not much point in waiting any longer
for that. If they make further changes and commit it to LLVM 19 or 20,
we'll still need this for older versions, but we may want to
resynchronize our copy and update some comments.
The changes that we've had to make to our copy can be seen by diffing
our SectionMemoryManager.{h,cpp} files against the ones in the tree of
the pull request. Per the LLVM project's license requirements, a copy
is in SectionMemoryManager.LICENSE.
This should fix the spate of crash reports we've been receiving lately
from users on large memory ARM systems.
Back-patch to all supported releases.
Co-authored-by: Thomas Munro <thomas.munro@gmail.com>
Co-authored-by: Anthonin Bonnefoy <anthonin.bonnefoy@datadoghq.com>
Reviewed-by: Anthonin Bonnefoy <anthonin.bonnefoy@datadoghq.com>
Reviewed-by: Daniel Gustafsson <daniel@yesql.se> (license aspects)
Reported-by: Anthonin Bonnefoy <anthonin.bonnefoy@datadoghq.com>
Discussion: https://postgr.es/m/CAO6_Xqr63qj%3DSx7HY6ZiiQ6R_JbX%2B-p6sTPwDYwTWZjUmjsYBg%40mail.gmail.com
Michael Paquier [Tue, 5 Nov 2024 00:40:58 +0000 (09:40 +0900)]
Clear padding of PgStat_HashKey when handling pgstats entries
PgStat_HashKey is currently initialized in a way that could result in
random data if the structure has any padding bytes. The structure
has no padding bytes currently, fortunately, but it could become a
problem should the structure change at some point in the future.
The code is changed to use some memset(0) so as any padding would be
handled properly, as it would be surprising to see random failures in
the pgstats entry lookups. PgStat_HashKey is a structure internal to
pgstats, and an ABI change could be possible in the scope of a bug fix,
so backpatch down to 15 where this has been introduced.
Author: Bertrand Drouvot
Reviewed-by: Jelte Fennema-Nio, Michael Paquier
Discussion: https://postgr.es/m/Zyb7RW1y9dVfO0UH@ip-10-97-1-34.eu-west-3.compute.internal
Backpatch-through: 15
Tom Lane [Mon, 4 Nov 2024 23:08:48 +0000 (18:08 -0500)]
Use portable diff options in pg_bsd_indent's regression test.
We had been using "diff -upd", which evidently works for most people,
but Solaris's diff doesn't like it. (We'd not noticed because the
Solaris buildfarm animals weren't running this test until they were
upgraded to the latest buildfarm client script.) Change to "diff -U3"
which is what pg_regress has used for ages.
Per buildfarm (and off-list discussion with Noah Misch).
Back-patch to v16 where this test was added. In v16,
also back-patch the relevant part of
628c1d1f2 so that
the test script looks about the same in all branches.
Noah Misch [Sun, 3 Nov 2024 02:42:52 +0000 (19:42 -0700)]
Suppress new "may be used uninitialized" warning.
Buildfarm member mamba fails to deduce that the function never uses this
variable without initializing it. Back-patch to v12, like commit
b412f402d1e020c5dac94f3bf4a005db69519b99.
Noah Misch [Sat, 2 Nov 2024 16:04:55 +0000 (09:04 -0700)]
Move I/O before the index_update_stats() buffer lock region.
Commit
a07e03fd8fa7daf4d1356f7cb501ffe784ea6257 enlarged the work done
here under the pg_class heap buffer lock. Two preexisting actions are
best done before holding that lock. Both RelationGetNumberOfBlocks()
and visibilitymap_count() do I/O, and the latter might exclusive-lock a
visibility map buffer. Moving these reduces contention and risk of
undetected LWLock deadlock. Back-patch to v12, like that commit.
Discussion: https://postgr.es/m/
20241031200139.b4@rfd.leadboat.com
Noah Misch [Sat, 2 Nov 2024 16:05:00 +0000 (09:05 -0700)]
Revert "For inplace update, send nontransactional invalidations."
This reverts commit
95c5acb3fc261067ab65ddc0b2dca8e162f09442 (v17) and
counterparts in each other non-master branch. If released, that commit
would have caused a worst-in-years minor release regression, via
undetected LWLock self-deadlock. This commit and its self-deadlock fix
warrant more bake time in the master branch.
Reported by Alexander Lakhin.
Discussion: https://postgr.es/m/
10ec0bc3-5933-1189-6bb8-
5dec4114558e@gmail.com
Noah Misch [Sat, 2 Nov 2024 16:04:59 +0000 (09:04 -0700)]
Revert "WAL-log inplace update before revealing it to other sessions."
This reverts commit
bfd5c6e279c8e1702eea882439dc7ebdf4d4b3a5 (v17) and
counterparts in each other non-master branch. This unblocks reverting a
commit on which it depends.
Discussion: https://postgr.es/m/
10ec0bc3-5933-1189-6bb8-
5dec4114558e@gmail.com
Bruce Momjian [Fri, 1 Nov 2024 17:54:28 +0000 (13:54 -0400)]
doc: fix ALTER DOMAIN domain_constraint to spell out options
It used to refer to CREATE DOMAIN, but CREATE DOMAIN allows NULL, while
ALTER DOMAIN does not.
Reported-by: elionescu@yahoo.com
Discussion: https://postgr.es/m/
172225092461.915373.
6103973717483380183@wrigleys.postgresql.org
Backpatch-through: 12
Bruce Momjian [Fri, 1 Nov 2024 15:30:53 +0000 (11:30 -0400)]
doc: remove mention of ActiveState for Perl and Tcl on Windows
Replace with Strawberry Perl and Magicsplat Tcl.
Reported-by: Yasir Hussain
Discussion: https://postgr.es/m/CAA9OW9fAAM_WDYYpAquqF6j1hmfRMzHPsFkRfP5E6oSfkF=dMA@mail.gmail.com
Backpatch-through: 12
Álvaro Herrera [Wed, 30 Oct 2024 09:54:03 +0000 (10:54 +0100)]
Fix some more bugs in foreign keys connecting partitioned tables
* In DetachPartitionFinalize() we were applying a tuple conversion map
to tuples that didn't need one, which can lead to erratic behavior if
a partitioned table has a partition with a different column order, as
reported by Alexander Lakhin. This was introduced by
53af9491a043.
Don't do that. Also, modify a recently added test case to exercise
this.
* The same function as well as CloneFkReferenced() were acquiring
AccessShareLock on a partition, only to have CreateTrigger() later
acquire ShareRowExclusiveLock on it. This can lead to deadlock by
lock escalation, unnecessarily. Avoid that by acquiring the stronger
lock to begin with. This probably dates back to branch 12, but I have
never seen a report of this being a problem in the field.
* Innocuous but wasteful: also introduced by
53af9491a043, we were
reading a pg_constraint tuple from syscache that we don't need, as
reported by Tender Wang. Don't.
Backpatch to 15.
Discussion: https://postgr.es/m/
461e9c26-2076-8224-e119-
84998b6a784e@gmail.com
Noah Misch [Tue, 29 Oct 2024 16:39:55 +0000 (09:39 -0700)]
Unpin buffer before inplace update waits for an XID to end.
Commit
a07e03fd8fa7daf4d1356f7cb501ffe784ea6257 changed inplace updates
to wait for heap_update() commands like GRANT TABLE and GRANT DATABASE.
By keeping the pin during that wait, a sequence of autovacuum workers
and an uncommitted GRANT starved one foreground LockBufferForCleanup()
for six minutes, on buildfarm member sarus. Prevent, at the cost of a
bit of complexity. Back-patch to v12, like the earlier commit. That
commit and heap_inplace_lock() have not yet appeared in any release.
Discussion: https://postgr.es/m/
20241026184936.ae.nmisch@google.com
Tom Lane [Tue, 29 Oct 2024 15:49:38 +0000 (11:49 -0400)]
Update time zone data files to tzdata release 2024b.
Historical corrections for Mexico, Mongolia, and Portugal.
Notably, Asia/Choibalsan is now an alias for Asia/Ulaanbaatar
rather than being a separate zone, mainly because the differences
between those zones were found to be based on untrustworthy data.
Michael Paquier [Tue, 29 Oct 2024 06:35:18 +0000 (15:35 +0900)]
doc: Add better description for rewrite functions in event triggers
There are two functions that can be used in event triggers to get more
details about a rewrite happening on a relation. Both had a limited
documentation:
- pg_event_trigger_table_rewrite_reason() and
pg_event_trigger_table_rewrite_oid() were not mentioned in the main
event trigger section in the paragraph dedicated to the event
table_rewrite.
- pg_event_trigger_table_rewrite_reason() returns an integer which is a
bitmap of the reasons why a rewrite happens. There was no explanation
about the meaning of these values, forcing the reader to look at the
code to find out that these are defined in event_trigger.h.
While on it, let's add a comment in event_trigger.h where the
AT_REWRITE_* are defined, telling to update the documentation when
these values are changed.
Backpatch down to 13 as a consequence of
1ad23335f36b, where this area
of the documentation has been heavily reworked.
Author: Greg Sabino Mullane
Discussion: https://postgr.es/m/CAKAnmmL+Z6j-C8dAx1tVrnBmZJu+BSoc68WSg3sR+CVNjBCqbw@mail.gmail.com
Backpatch-through: 13
David Rowley [Tue, 29 Oct 2024 03:25:08 +0000 (16:25 +1300)]
Doc: clarify enable_indexscan=off also disabled Index Only Scans
Disabling enable_indexscan has always also disabled Index Only Scans.
Here we make that more clear in the documentation in an attempt to
prevent future complaints complaining about this expected behavior.
Reported-by: Melanie Plageman
Author: David G. Johnston, David Rowley
Backpatch-through: 12, oldest supported version
Discussion: https://postgr.es/m/CAAKRu_atV=kovgpaLREyG68PB5+ncKvJ2UNoeRetEgyC3Yb5Sw@mail.gmail.com
Tom Lane [Mon, 28 Oct 2024 18:33:55 +0000 (14:33 -0400)]
Guard against enormously long input in pg_saslprep().
Coverity complained that pg_saslprep() could suffer integer overflow,
leading to under-allocation of the output buffer, if the input string
exceeds SIZE_MAX/4. This hazard seems largely hypothetical, but it's
easy enough to defend against, so let's do so.
This patch creates a third place in src/common/ where we are locally
defining MaxAllocSize so that we can test against that in the same way
in backend and frontend compiles. That seems like about two places
too many, so the next patch will move that into common/fe_memutils.h.
I'm hesitant to do that in back branches however.
Back-patch to v14. The code looks similar in older branches, but
before commit
67a472d71 there was a separate test on the input string
length that prevented this hazard.
Per Coverity report.
Heikki Linnakangas [Mon, 28 Oct 2024 12:07:38 +0000 (14:07 +0200)]
Fix overflow in bsearch_arg() with more than INT_MAX elements
This was introduced in commit
bfa2cee784, which replaced the old
bsearch_cmp() function we had in extended_stats.c with the current
implementation. The original discussion or commit message of
bfa2cee784 didn't mention where the new implementation came from, but
based on some googling, I'm guessing *BSD or libiberty, all of which
share this same code, with or without this fix.
Author: Ranier Vilela
Reviewed-by: Nathan Bossart
Backpatch-through: 14
Discussion: https://www.postgresql.org/message-id/CAEudQAp34o_8u6sGSVraLwuMv9F7T9hyHpePXHmRaxR2Aboi%2Bw%40mail.gmail.com
Noah Misch [Fri, 25 Oct 2024 13:51:03 +0000 (06:51 -0700)]
WAL-log inplace update before revealing it to other sessions.
A buffer lock won't stop a reader having already checked tuple
visibility. If a vac_update_datfrozenid() and then a crash happened
during inplace update of a relfrozenxid value, datfrozenxid could
overtake relfrozenxid. That could lead to "could not access status of
transaction" errors. Back-patch to v12 (all supported versions). In
v14 and earlier, this also back-patches the assertion removal from
commit
7fcf2faf9c7dd473208fd6d5565f88d7f733782b.
Discussion: https://postgr.es/m/
20240620012908.92.nmisch@google.com
Noah Misch [Fri, 25 Oct 2024 13:51:02 +0000 (06:51 -0700)]
For inplace update, send nontransactional invalidations.
The inplace update survives ROLLBACK. The inval didn't, so another
backend's DDL could then update the row without incorporating the
inplace update. In the test this fixes, a mix of CREATE INDEX and ALTER
TABLE resulted in a table with an index, yet relhasindex=f. That is a
source of index corruption. Back-patch to v12 (all supported versions).
The back branch versions don't change WAL, because those branches just
added end-of-recovery SIResetAll(). All branches change the ABI of
extern function PrepareToInvalidateCacheTuple(). No PGXN extension
calls that, and there's no apparent use case in extensions.
Reviewed by Nitin Motiani and (in earlier versions) Andres Freund.
Discussion: https://postgr.es/m/
20240523000548.58.nmisch@google.com
Noah Misch [Fri, 25 Oct 2024 13:51:06 +0000 (06:51 -0700)]
At end of recovery, reset all sinval-managed caches.
An inplace update's invalidation messages are part of its transaction's
commit record. However, the update survives even if its transaction
aborts or we stop recovery before replaying its transaction commit.
After recovery, a backend that started in recovery could update the row
without incorporating the inplace update. That could result in a table
with an index, yet relhasindex=f. That is a source of index corruption.
This bulk invalidation avoids the functional consequences. A future
change can fix the !RecoveryInProgress() scenario without changing the
WAL format. Back-patch to v17 - v12 (all supported versions). v18 will
instead add invalidations to WAL.
Discussion: https://postgr.es/m/
20240618152349.7f.nmisch@google.com
Noah Misch [Thu, 24 Oct 2024 16:16:14 +0000 (09:16 -0700)]
Stop reading uninitialized memory in heap_inplace_lock().
Stop computing a never-used value. This removes the read; the read had
no functional implications. Back-patch to v12, like commit
a07e03fd8fa7daf4d1356f7cb501ffe784ea6257.
Reported by Alexander Lakhin.
Discussion: https://postgr.es/m/
6c92f59b-f5bc-e58c-9bdd-
d1f21c17c786@gmail.com
Amit Langote [Wed, 23 Oct 2024 08:54:48 +0000 (17:54 +0900)]
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
Michael Paquier [Tue, 22 Oct 2024 23:35:02 +0000 (08:35 +0900)]
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
Álvaro Herrera [Tue, 22 Oct 2024 14:01:18 +0000 (16:01 +0200)]
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
Tom Lane [Mon, 21 Oct 2024 19:08:22 +0000 (15:08 -0400)]
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
Heikki Linnakangas [Mon, 21 Oct 2024 06:49:21 +0000 (09:49 +0300)]
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
Álvaro Herrera [Sun, 20 Oct 2024 13:36:20 +0000 (15:36 +0200)]
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
Amit Langote [Sun, 20 Oct 2024 03:21:03 +0000 (12:21 +0900)]
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
Nathan Bossart [Fri, 18 Oct 2024 15:20:15 +0000 (10:20 -0500)]
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
Thomas Munro [Thu, 17 Oct 2024 02:52:24 +0000 (15:52 +1300)]
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
Michael Paquier [Wed, 16 Oct 2024 22:21:43 +0000 (07:21 +0900)]
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
Tom Lane [Wed, 16 Oct 2024 21:36:29 +0000 (17:36 -0400)]
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
Masahiko Sawada [Wed, 16 Oct 2024 19:08:00 +0000 (12:08 -0700)]
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
Amit Langote [Wed, 16 Oct 2024 09:11:53 +0000 (18:11 +0900)]
Fix typo in comment of transformJsonAggConstructor()
An oversight of
3a8a1f3254b.
Reported-by: Tender Wang <tndrwang@gmail.com>
Author: Tender Wang <tndrwang@gmail.com>
Backpatch-through: 16
Michael Paquier [Mon, 14 Oct 2024 03:28:01 +0000 (12:28 +0900)]
psql: Fix \watch when using interval values less than 1ms
Attempting to use an interval of time less than 1ms would cause \watch
to hang. This was confusing, so let's change the logic so as an
interval lower than 1ms behaves the same as 0.
Comments are added to mention that the internals of do_watch() had
better rely on "sleep_ms", the interval value in milliseconds. While on
it, this commit adds a test to check the behavior of interval values
less than 1ms.
\watch hanging for interval values less than 1ms existed before
6f9ee74d45aa, that has changed the code to support an interval value of
0.
Reported-by: Heikki Linnakangas
Author: Andrey M. Borodin, Michael Paquier
Discussion: https://postgr.es/m/
88445e0e-3156-4b9d-afae-
9a1a7b1631f6@iki.fi
Backpatch-through: 16
Tom Lane [Sat, 12 Oct 2024 18:56:08 +0000 (14:56 -0400)]
Correctly identify which EC members are computable at a plan node.
find_computable_ec_member() had the wrong mental model of what
its primary caller prepare_sort_from_pathkeys() would do with
the selected EquivalenceClass member expression. We will not
compute the EC expression in a plan node atop the one returning
the passed-in targetlist; rather, the EC expression will be
computed as an additional column of that targetlist. So any
Var or quasi-Var used in the given tlist is also available to the
EC expression. In simple cases this makes no difference because
the given tlist is just a list of Vars or quasi-Vars --- but if
we are considering an appendrel member produced by flattening
a UNION ALL, the tlist may contain expressions, resulting in
failure to match and a "could not find pathkey item to sort"
error.
To fix, we can flatten both the tlist and the EC members with
pull_var_clause(), and then just check for subset-ness, so
that the code is actually shorter than before.
While this bug is quite old, the present patch only works back to
v13. We could possibly make it work in v12 by back-patching parts
of
375398244. On the whole though I don't like the risk/reward
ratio of that idea. v12's final release is next month, meaning
there would be no chance to correct matters if the patch causes a
regression. Since this failure has escaped notice for 14 years,
it's likely nobody will hit it in the field with v12.
Per bug #18652 from Alexander Lakhin.
Andrei Lepikhov and Tom Lane
Discussion: https://postgr.es/m/18652-
deaa782ebcca85d1@postgresql.org
Daniel Gustafsson [Wed, 9 Oct 2024 07:34:34 +0000 (09:34 +0200)]
Remove incorrect function import from pgindent
Commit
149ac7d4559 which re-implemented pgindent in Perl explicitly
imported the devnull function from File::Spec, but the module does
not export anything. In recent versions of Perl calling a missing
import function cause a warning, which combined with warnings being
fatal cause pgindent to error out.
Backpatch to all supported versions.
Author: Erik Wienhold <ewie@ewie.name>
Reviewed-by: Andrew Dunstan <andrew@dunslane.net>
Reviewed-by: Daniel Gustafsson <daniel@yesql.se>
Discusson: https://postgr.es/m/
2372cd74-11b0-46f9-b28e-
8f9627215d19@ewie.name
Backpatch-through: v12
Amit Kapila [Tue, 8 Oct 2024 06:31:35 +0000 (12:01 +0530)]
Stabilize the test added by commit
022564f60c.
The test was unstable in branches 14 and 15 as we were relying on the
number of changes in the table having a toast column to start streaming.
On branches >= 16, we have a GUC debug_logical_replication_streaming which
can stream each change, so the test was stable in those branches.
Change the test to use PREPARE TRANSACTION as that should make the result
consistent and test the code changed in
022564f60c.
Reported-by: Daniel Gustafsson as per buildfarm
Author: Hou Zhijie, Amit Kapila
Backpatch-through: 14
Discussion: https://postgr.es/m/
8C2F86AA-981E-4803-B14D-
E264C0255330@yesql.se
Nathan Bossart [Mon, 7 Oct 2024 21:49:20 +0000 (16:49 -0500)]
vacuumdb: Schema-qualify operator in catalog query's WHERE clause.
Commit
1ab67c9dfa, which modified this catalog query so that it
doesn't return temporary relations, forgot to schema-qualify the
operator. A comment earlier in the function implores us to fully
qualify everything in the query:
* Since we execute the constructed query with the default search_path
* (which could be unsafe), everything in this query MUST be fully
* qualified.
This commit fixes that. While at it, add a newline for consistency
with surrounding code.
Reviewed-by: Noah Misch
Discussion: https://postgr.es/m/ZwQJYcuPPUsF0reU%40nathan
Backpatch-through: 12
Nathan Bossart [Mon, 7 Oct 2024 18:51:03 +0000 (13:51 -0500)]
Fix Y2038 issues with MyStartTime.
Several places treat MyStartTime as a "long", which is only 32 bits
wide on some platforms. In reality, MyStartTime is a pg_time_t,
i.e., a signed 64-bit integer. This will lead to interesting bugs
on the aforementioned systems in 2038 when signed 32-bit integers
are no longer sufficient to store Unix time (e.g., "pg_ctl start"
hanging). To fix, ensure that MyStartTime is handled as a 64-bit
value everywhere. (Of course, users will need to ensure that
time_t is 64 bits wide on their system, too.)
Co-authored-by: Max Johnson
Discussion: https://postgr.es/m/CO1PR07MB905262E8AC270FAAACED66008D682%40CO1PR07MB9052.namprd07.prod.outlook.com
Backpatch-through: 12
Amit Kapila [Mon, 7 Oct 2024 09:34:05 +0000 (15:04 +0530)]
Fix fetching default toast value during decoding of in-progress transactions.
During logical decoding of in-progress transactions, we perform the toast
table scan while fetching the default toast value for an attribute. We
forgot to initialize the flag during this scan to indicate that the system
table scan is in progress. We need this flag to ensure that during logical
decoding we never directly access the tableam or heap APIs because we check
for concurrent aborts only in systable_* APIs.
Reported-by: Alexander Lakhin
Author: Takeshi Ideriha, Hou Zhijie
Reviewed-by: Amit Kapila, Hou Zhijie
Backpatch-through: 14
Discussion: https://postgr.es/m/18641-
6687273b7f15269d@postgresql.org
Tom Lane [Sun, 6 Oct 2024 20:03:48 +0000 (16:03 -0400)]
Ignore not-yet-defined Portals in pg_cursors view.
pg_cursor() supposed that any Portal it finds in the hash table must
have sourceText set up, but there's an edge case where that is not so.
A newly-created Portal has sourceText = NULL, and that doesn't change
until PortalDefineQuery is called. In SPI_cursor_open_internal,
we perform GetCachedPlan between CreatePortal and PortalDefineQuery,
and it's possible for user-defined code to execute during that
planning and cause a fetch from the pg_cursors view, resulting in a
null-pointer-dereference crash. (It looks like the same could happen
in exec_bind_message, but I've not tried to provoke a failure there.)
I considered trying to fix this by setting sourceText sooner, but
there may be instances of this same calling pattern in extensions,
and we couldn't be sure they'd get the memo promptly. It seems
better to redefine pg_cursor as not showing Portals that have
not yet had PortalDefineQuery called on them, which we can do by
just skipping them if sourceText is still NULL.
(Before
a1c692358, pg_cursor would instead return a row with NULL
in the statement column. We could revert to that behavior but it
doesn't really seem like a better definition, especially since our
documentation doesn't suggest that the column could be NULL.)
Per report from PetSerAl. Back-patch to all supported branches.
Discussion: https://postgr.es/m/CAKygsHTBXLXjwV43kpZa+Cs+XTiaeeJiZdL4cPBm9f4MTdw7wg@mail.gmail.com
Thomas Munro [Sat, 5 Oct 2024 00:48:33 +0000 (13:48 +1300)]
Reject non-ASCII locale names.
Commit
bf03cfd1 started scanning all available BCP 47 locale names on
Windows. This caused an abort/crash in the Windows runtime library if
the default locale name contained non-ASCII characters, because of our
use of the setlocale() save/restore pattern with "char" strings. After
switching to another locale with a different encoding, the saved name
could no longer be understood, and setlocale() would abort.
"Turkish_Türkiye.1254" is the example from recent reports, but there are
other examples of countries and languages with non-ASCII characters in
their names, and they appear in Windows' (old style) locale names.
To defend against this:
1. In initdb, reject non-ASCII locale names given explicity on the
command line, or returned by the operating system environment with
setlocale(..., ""), or "canonicalized" by the operating system when we
set it.
2. In initdb only, perform the save-and-restore with Windows'
non-standard wchar_t variant of setlocale(), so that it is not subject
to round trip failures stemming from char string encoding confusion.
3. In the backend, we don't have to worry about the save-and-restore
problem because we have already vetted the defaults, so we just have to
make sure that CREATE DATABASE also rejects non-ASCII names in any new
databases. SET lc_XXX doesn't suffer from the problem, but the ban
applies to it too because it uses check_locale(). CREATE COLLATION
doesn't suffer from the problem either, but it doesn't use
check_locale() so it is not included in the new ban for now, to minimize
the change.
Anyone who encounters the new error message should either create a new
duplicated locale with an ASCII-only name using Windows Locale Builder,
or consider using BCP 47 names like "tr-TR". Users already couldn't
initialize a cluster with "Turkish_Türkiye.1254" on PostgreSQL 16+, but
the new failure mode is an error message that explains why, instead of a
crash.
Back-patch to 16, where
bf03cfd1 landed. Older versions are affected
in theory too, but only 16 and later are causing crash reports.
Reviewed-by: Andrew Dunstan <andrew@dunslane.net> (the idea, not the patch)
Reported-by: Haifang Wang (Centific Technologies Inc) <v-haiwang@microsoft.com>
Discussion: https://postgr.es/m/PH8PR21MB3902F334A3174C54058F792CE5182%40PH8PR21MB3902.namprd21.prod.outlook.com