Thomas Munro [Sat, 7 May 2022 03:19:52 +0000 (15:19 +1200)]
Fix old-fd issues using global barriers everywhere.
Commits
4eb21763 and
b74e94dc introduced a way to force every backend to
close all relation files, to fix an ancient Windows-only bug.
This commit extends that behavior to all operating systems and adds
a couple of extra barrier points, to fix a totally different class of
bug: the reuse of relfilenodes in scenarios that have no other kind of
cache invalidation to prevent file descriptor mix-ups.
In all releases, data corruption could occur when you moved a database
to another tablespace and then back again. Despite that, no back-patch
for now as the infrastructure required is too new and invasive. In
master only, since commit
aa010514, it could also happen when using
CREATE DATABASE with a user-supplied OID or via pg_upgrade.
Author: Andres Freund <andres@anarazel.de>
Reviewed-by: Robert Haas <robertmhaas@gmail.com>
Reviewed-by: Thomas Munro <thomas.munro@gmail.com>
Discussion: https://postgr.es/m/
20220209220004.kb3dgtn2x2k2gtdm%40alap3.anarazel.de
Thomas Munro [Sat, 7 May 2022 04:19:42 +0000 (16:19 +1200)]
Rethink PROCSIGNAL_BARRIER_SMGRRELEASE.
With sufficiently bad luck, it was possible for IssuePendingWritebacks()
to reopen a file after we'd processed PROCSIGNAL_BARRIER_SMGRRELEASE and
before the file was unlinked by some other backend. That left a small
hole in commit
4eb21763's plan to fix all spurious errors from DROP
TABLESPACE and similar on Windows.
Fix by closing md.c's segments, instead of just closing fd.c's
descriptors, and then teaching smgrwriteback() not to open files that
aren't already open.
Reported-by: Andres Freund <andres@anarazel.de>
Reviewed-by: Robert Haas <robertmhaas@gmail.com>
Discussion: https://postgr.es/m/
20220209220004.kb3dgtn2x2k2gtdm%40alap3.anarazel.de
Robert Haas [Fri, 6 May 2022 13:24:06 +0000 (09:24 -0400)]
Fix misleading comments about background worker registration.
Since
6bc8ef0b7f1f1df3998745a66e1790e27424aa0c, the maximum number
of backends can't change as background workers are registered, but
these comments still reflect the way things worked prior to that.
Also, per recent discussion, some modules call SetConfigOption()
from _PG_init(). It's not entirely clear to me whether we want to
regard that as a fully supported operation, but since we know it's
a thing that happens, it at least deserves a mention in the comments,
so add that.
Nathan Bossart, reviewed by Anton A. Melnikov
Discussion: http://postgr.es/m/
20220419154658.GA2487941@nathanxps13
Daniel Gustafsson [Fri, 6 May 2022 12:41:36 +0000 (14:41 +0200)]
pgcrypto: remove questionmark from error message
The PXE_CIPHER_INIT error is used to report initialization errors, so
appending a questionmark to the error isn't entirely accurate (using a
space before the questionmark doubly so).
Discussion: https://postgr.es/m/
C89D932C-501E-4473-9750-
638CFCD9095E@yesql.se
Daniel Gustafsson [Fri, 6 May 2022 12:41:33 +0000 (14:41 +0200)]
pgcrypto: report init errors as PXE_CIPHER_INIT
Report OpenSSL errors during initialization as PXE_CIPHER_INIT since
that's just what they were, and not generic unknown errors. This also
removes the last users of the generic error, and thus it can be removed.
Discussion: http://postgr.es/m/
C89D932C-501E-4473-9750-
638CFCD9095E@yesql.se
Daniel Gustafsson [Fri, 6 May 2022 12:41:31 +0000 (14:41 +0200)]
Clear the OpenSSL error queue before cryptohash operations
Setting up an EVP context for ciphers banned under FIPS generate
two OpenSSL errors in the queue, and as we only consume one from
the queue the other is at the head for the next invocation:
postgres=# select md5('foo');
ERROR: could not compute MD5 hash: unsupported
postgres=# select md5('foo');
ERROR: could not compute MD5 hash: initialization error
Clearing the error queue when creating the context ensures that
we don't pull in an error from an earlier operation.
Discussion: https://postgr.es/m/
C89D932C-501E-4473-9750-
638CFCD9095E@yesql.se
Michael Paquier [Fri, 6 May 2022 11:01:15 +0000 (20:01 +0900)]
Fix typo in origin.c
Introduced in
5aa2350.
Author: Peter Smith
Discussion: https://postgr.es/m/CAHut+PsuWz6_7aCmivNU5FahgQxDUTQtc3+__XnWkBzQcfn43w@mail.gmail.com
Peter Eisentraut [Fri, 6 May 2022 07:17:38 +0000 (09:17 +0200)]
Update SQL features
Update a few items that have become supported or mostly supported but
weren't updated at the time.
Peter Eisentraut [Fri, 6 May 2022 07:14:15 +0000 (09:14 +0200)]
Fix some whitespace in documentation markup
Peter Eisentraut [Fri, 6 May 2022 07:07:14 +0000 (09:07 +0200)]
Tom Lane [Thu, 5 May 2022 18:54:53 +0000 (14:54 -0400)]
Update time zone data files to tzdata release 2022a.
DST law changes in Palestine. Historical corrections for
Chile and Ukraine.
Andres Freund [Wed, 4 May 2022 19:50:38 +0000 (12:50 -0700)]
Fix timing issue in deadlock recovery conflict test.
Per buildfarm members longfin and skink.
Discussion: https://postgr.es/m/
20220413002626.udl7lll7f3o7nre7@alap3.anarazel.de
Backpatch: 10-
Tom Lane [Wed, 4 May 2022 18:44:40 +0000 (14:44 -0400)]
Fix rowcount estimate for SubqueryScan that's under a Gather.
SubqueryScan was always getting labeled with a rowcount estimate
appropriate for non-parallel cases. However, nodes that are
underneath a Gather should be treated as processing only one
worker's share of the rows, whether the particular node is explicitly
parallel-aware or not. Most non-scan-level node types get this
right automatically because they base their rowcount estimate on
that of their input sub-Path(s). But SubqueryScan didn't do that,
instead using the whole-relation rowcount estimate as if it were
a non-parallel-aware scan node. If there is a parallel-aware node
below the SubqueryScan, this is wrong, and it results in inflating
the cost estimates for nodes above the SubqueryScan, which can cause
us to not choose a parallel plan, or choose a silly one --- as indeed
is visible in the one regression test whose results change with this
patch. (Although that plan tree appears to contain no SubqueryScans,
there were some in it before setrefs.c deleted them.)
To fix, use path->subpath->rows not baserel->tuples as the number
of input tuples we'll process. This requires estimating the quals'
selectivity afresh, which is slightly annoying; but it shouldn't
really add much cost thanks to the caching done in RestrictInfo.
This is pretty clearly a bug fix, but I'll refrain from back-patching
as people might not appreciate plan choices changing in stable branches.
The fact that it took us this long to identify the bug suggests that
it's not a major problem.
Per report from bucoo, though this is not his proposed patch.
Discussion: https://postgr.es/m/
202204121457159307248@sohu.com
Peter Eisentraut [Wed, 4 May 2022 15:36:31 +0000 (17:36 +0200)]
Remove JsonPathSpec typedef
It doesn't seem very useful, and it's a bit in the way of the planned
node support automation.
Discussion: https://www.postgresql.org/message-id/
202204191140.3wsbevfhqmu3@alvherre.pgsql
Peter Eisentraut [Wed, 4 May 2022 15:34:22 +0000 (17:34 +0200)]
Add missing enum tag in enum used in nodes
Similar to
983bdc4fac492a99bb8ab5a471ca7437139e5cf6.
Author: Alvaro Herrera <alvherre@alvh.no-ip.org>
Discussion: https://www.postgresql.org/message-id/
202204191140.3wsbevfhqmu3@alvherre.pgsql
Peter Eisentraut [Wed, 4 May 2022 11:33:59 +0000 (13:33 +0200)]
Simplify configure test
The test for lz4.h used AC_CHECK_HEADERS, but nothing was using the
resulting symbol HAVE_LZ4_H. Change this to use AC_CHECK_HEADER
instead. This was probably an oversight, seeing that the nearby
similar tests do this correctly.
Daniel Gustafsson [Wed, 4 May 2022 12:15:25 +0000 (14:15 +0200)]
Rename libpq test programs with libpq_ prefix
The testclient and uri-regress programs in the libpq test suite had
quite generic names which didn't convey much meaning. Since they are
installed as part of the MSVC test runs, ensure that their purpose
is a little bit clearer by renaming with a libpq_ prefix. While at
it rename uri-regress to uri_regress to avoid mixing dash and under-
score in the same filename.
Reported-by: Noah Misch <noah@leadboat.com>
Discussion: https://postgr.es/m/
20220501080706.GA1542365@rfd.leadboat.com
Peter Eisentraut [Wed, 4 May 2022 05:57:39 +0000 (07:57 +0200)]
Fix incorrect format placeholders
Andres Freund [Tue, 3 May 2022 01:25:00 +0000 (18:25 -0700)]
Fix possibility of self-deadlock in ResolveRecoveryConflictWithBufferPin().
The tests added in
9f8a050f68d failed nearly reliably on FreeBSD in CI, and
occasionally on the buildfarm. That turns out to be caused not by a bug in the
test, but by a longstanding bug in recovery conflict handling.
The standby timeout handler, used by ResolveRecoveryConflictWithBufferPin(),
executed SendRecoveryConflictWithBufferPin() inside a signal handler. A bad
idea, because the deadlock timeout handler (or a spurious latch set) could
have interrupted ProcWaitForSignal(). If unlucky that could cause a
self-deadlock on ProcArrayLock, if the deadlock check is in
SendRecoveryConflictWithBufferPin()->CancelDBBackends().
To fix, set a flag in StandbyTimeoutHandler(), and check the flag in
ResolveRecoveryConflictWithBufferPin().
Subsequently the recovery conflict tests will be backpatched.
Discussion: https://postgr.es/m/
20220413002626.udl7lll7f3o7nre7@alap3.anarazel.de
Backpatch: 10-
Andres Freund [Tue, 3 May 2022 00:19:11 +0000 (17:19 -0700)]
Add tests for recovery deadlock conflicts.
The recovery conflict tests added in
9f8a050f68d surfaced a bug in the
interaction between buffer pin and deadlock recovery conflicts. To make sure
that the bugfix won't break deadlock conflict detection, add a test for that
scenario.
031_recovery_conflict.pl will later be backpatched, with this included.
Discussion: https://postgr.es/m/
20220413002626.udl7lll7f3o7nre7@alap3.anarazel.de
Michael Paquier [Mon, 2 May 2022 11:16:19 +0000 (20:16 +0900)]
basebackup_to_shell: Add missing MarkGUCPrefixReserved()
Oversight in
c6306db24, as per a requirement from
88103567. All the
other modules in the tree, be they in contrib/ or src/test/modules/,
already do that.
Author: Bharath Rupireddy
Discussion: https://postgr.es/m/CALj2ACUy7q_KwSMda+2SHPSWep32tNUM8cXGRS3=-Vfodo9OUg@mail.gmail.com
Etsuro Fujita [Mon, 2 May 2022 07:45:00 +0000 (16:45 +0900)]
Fix typo in comment.
Jeff Davis [Sat, 30 Apr 2022 15:28:33 +0000 (08:28 -0700)]
pg_walinspect: fix case where flush LSN is in the middle of a record.
Instability in the test for pg_walinspect revealed that
pg_get_wal_records_info_till_end_of_wal(x) would try to decode all the
records with a start LSN earlier than the flush LSN, even though that
might include a partial record at the end of the range. In that case,
read_local_xlog_page_no_wait() would return NULL when it tried to read
past the flush LSN, which would be interpreted as an error by the
caller. That caused a test failure only on a BF animal that had been
restarted recently, but could be expected to happen in the wild quite
easily depending on the alignment of various parameters.
Fix by using private data in read_local_xlog_page_no_wait() to signal
end-of-wal to the caller, so that it can be properly distinguished
from a real error.
Discussion: https://postgr.es/m/Ymd/e5eeZMNAkrXo%40paquier.xyz
Discussion: https://postgr.es/m/111657.
1650910309@sss.pgh.pa.us
Authors: Thomas Munro, Bharath Rupireddy.
Tom Lane [Sat, 30 Apr 2022 15:54:28 +0000 (11:54 -0400)]
Tighten enforcement of variable CONSTANT markings in plpgsql.
I noticed that plpgsql would allow assignment of a new value to a
variable even when that variable is marked CONSTANT, if the variable
is used as an output parameter in CALL or is a refcursor variable
that OPEN assigns a new value to. Fix these oversights.
In the CALL case, the check has to be done at runtime because we
cannot know at parse time which parameters are OUT parameters.
For OPEN, it seems best to likewise enforce at runtime because
then we needn't throw error if the variable has a nonnull value
(since OPEN will only try to overwrite a null value).
Although this is surely a bug fix, no back-patch: it seems unlikely
that anyone would thank us for breaking formerly-working code in
minor releases.
Discussion: https://postgr.es/m/214453.
1651182729@sss.pgh.pa.us
Andrew Dunstan [Fri, 29 Apr 2022 13:01:05 +0000 (09:01 -0400)]
Claim SQL standard compliance for SQL/JSON features
Discussion: https://postgr.es/m/
d03d809c-d0fb-fd6a-1476-
d6dc18ec940e@dunslane.net
Andrew Dunstan [Thu, 28 Apr 2022 19:28:20 +0000 (15:28 -0400)]
Fix JSON_OBJECTAGG uniquefying bug
Commit
f4fb45d15c contained a bug in removing items with null values when
unique keys are required, where the leading items that are sorted
contained such values. Fix that and add a test for it.
Discussion: https://postgr.es/m/CAJA4AWQ_XbSmsNbW226UqNyRLJ+wb=iQkQMj77cQyoNkqtf=2Q@mail.gmail.com
Etsuro Fujita [Thu, 28 Apr 2022 06:15:00 +0000 (15:15 +0900)]
Disable asynchronous execution if using gating Result nodes.
mark_async_capable_plan(), which is called from create_append_plan() to
determine whether subplans are async-capable, failed to take into
account that the given subplan created from a given subpath might
include a gating Result node if the subpath is a SubqueryScanPath or
ForeignPath, causing a segmentation fault there when the subplan created
from a SubqueryScanPath includes the Result node, or causing
ExecAsyncRequest() to throw an error about an unrecognized node type
when the subplan created from a ForeignPath includes the Result node,
because in the latter case the Result node was unintentionally
considered as async-capable, but we don't currently support executing
Result nodes asynchronously. Fix by modifying mark_async_capable_plan()
to disable asynchronous execution in such cases. Also, adjust code in
the ProjectionPath case in mark_async_capable_plan(), for consistency
with other cases, and adjust/improve comments there.
is_async_capable_path() added in commit
27e1f1456, which was rewritten
to mark_async_capable_plan() in a later commit, has the same issue,
causing the error at execution mentioned above, so back-patch to v14
where the aforesaid commit went in.
Per report from Justin Pryzby.
Etsuro Fujita, reviewed by Zhihong Yu and Justin Pryzby.
Discussion: https://postgr.es/m/
20220408124338.GK24419%40telsasoft.com
Michael Paquier [Thu, 28 Apr 2022 04:08:16 +0000 (13:08 +0900)]
Revert recent changes with durable_rename_excl()
This reverts commits
2c902bb and
ccfbd92. Per buildfarm members
kestrel, rorqual and calliphoridae, the assertions checking that a TLI
history file should not exist when created by a WAL receiver have been
failing, and switching to durable_rename() over durable_rename_excl()
would cause the newest TLI history file to overwrite the existing one.
We need to think harder about such cases, so revert the new logic for
now.
Note that all the failures have been reported in the test
025_stuck_on_old_timeline.
Discussion: https://postgr.es/m/511362.
1651116498@sss.pgh.pa.us
John Naylor [Thu, 28 Apr 2022 02:27:32 +0000 (09:27 +0700)]
Fix SQL syntax in comment in logical/worker.c
Euler Taveira
Disussion: https://www.postgresql.org/message-id/
25f95189-eef8-43c4-9d7b-
419b651963c8%40www.fastmail.com
Michael Paquier [Thu, 28 Apr 2022 02:10:40 +0000 (11:10 +0900)]
Remove durable_rename_excl()
ccfbd92 has replaced all existing in-core callers of this function in
favor of durable_rename(). durable_rename_excl() is by nature unsafe on
crashes happening at the wrong time, so just remove it.
Author: Nathan Bossart
Reviewed-by: Robert Haas, Kyotaro Horiguchi, Michael Paquier
Discussion: https://postgr.es/m/
20220407182954.GA1231544@nathanxps13
Michael Paquier [Thu, 28 Apr 2022 01:11:45 +0000 (10:11 +0900)]
Replace existing durable_rename_excl() calls with durable_rename()
durable_rename_excl() attempts to avoid overwriting any existing files
by using link() and unlink(), falling back to rename() on some platforms
(e.g., Windows where link() followed by unlink() is not concurrent-safe,
see
909b449). Most callers of durable_rename_excl() use it just in case
there is an existing file, but it happens that for all of them we never
expect a target file to exist (WAL segment recycling, creation of
timeline history file and basic_archive).
basic_archive used durable_rename_excl() to avoid overwriting an archive
concurrently created by another server. Now, there is a stat() call to
avoid overwriting an existing archive a couple of lines above, so note
that this change opens a small TOCTOU window in this module between the
stat() call and durable_rename().
Furthermore, as mentioned in the top comment of durable_rename_excl(),
this routine can result in multiple hard links to the same file and data
corruption, with two or more links to the same file in pg_wal/ if a
crash happens before the unlink() call during WAL recycling.
Specifically, this would produce links to the same file for the current
WAL file and the next one because the half-recycled WAL file was
re-recycled during crash recovery of a follow-up cluster restart.
This change replaces all calls to durable_rename_excl() with
durable_rename(). This removes the protection against accidentally
overwriting an existing file, but some platforms are already living
without it, and all those code paths never expect an existing file (a
couple of assertions are added to check after that, in case).
This is a bug fix, but knowing the unlikeliness of the problem involving
one of more crashes at an exceptionally bad moment, no backpatch is
done. This could be revisited in the future.
Author: Nathan Bossart
Reviewed-by: Robert Haas, Kyotaro Horiguchi, Michael Paquier
Discussion: https://postgr.es/m/
20220407182954.GA1231544@nathanxps13
Peter Eisentraut [Wed, 27 Apr 2022 07:49:10 +0000 (09:49 +0200)]
Fix incorrect format placeholders
Peter Eisentraut [Wed, 27 Apr 2022 07:15:09 +0000 (09:15 +0200)]
Handle NULL fields in WRITE_INDEX_ARRAY
Unlike existing WRITE_*_ARRAY macros, WRITE_INDEX_ARRAY needs to
handle the case that the field is NULL. We already have the
convention to print NULL fields as "<>", so we do that here as well.
There is currently no corresponding read function for this, so reading
this back in is not implemented, but it could be if needed.
Reported-by: Richard Guo <guofenglinux@gmail.com>
Reviewed-by: Tom Lane <tgl@sss.pgh.pa.us>
Discussion: https://www.postgresql.org/message-id/flat/CAMbWs4-LN%3DbF8f9eU2R94dJtF54DfDvBq%2BovqHnOQqbinYDrUw%40mail.gmail.com
Michael Paquier [Tue, 26 Apr 2022 05:24:13 +0000 (14:24 +0900)]
Fix typo in pg_walinspect.c
Spotted while looking at the surroundings, introduced by
2258e76.
Michael Paquier [Tue, 26 Apr 2022 04:41:17 +0000 (13:41 +0900)]
Add some isolation tests for CLUSTER
This commit adds two isolation tests for CLUSTER, using:
- A normal table, making sure that CLUSTER blocks and completes if the
table is locked by a concurrent session.
- A partitioned table with a partition owned by a different user. If
the partitioned table is locked by a concurrent session, CLUSTER on the
partitioned table should block. If the partition owned by a different
user is locked, CLUSTER on its partitioned table should complete and
skip the partition.
3f19e17 has added an early check to ignore such a
partition with a SQL regression test, but this was not checking that
CLUSTER should not block.
Discussion: https://postgr.es/m/YlqveniXn9AI6RFZ@paquier.xyz
Andrew Dunstan [Mon, 25 Apr 2022 19:02:13 +0000 (15:02 -0400)]
Inhibit mingw CRT's auto-globbing of command line arguments
For some reason by default the mingw C Runtime takes it upon itself to
expand program arguments that look like shell globbing characters. That
has caused much scratching of heads and mis-attribution of the causes of
some TAP test failures, so stop doing that.
This removes an inconsistency with Windows binaries built with MSVC,
which have no such behaviour.
Per suggestion from Noah Misch.
Backpatch to all live branches.
Discussion: https://postgr.es/m/
20220423025927.GA1274057@rfd.leadboat.com
Alvaro Herrera [Mon, 25 Apr 2022 13:48:13 +0000 (15:48 +0200)]
Drop unlogged table after test is done
Another test is constructed on top of regression tests, which does not
work correctly with unlogged tables. For now, cope with that by making
sure no unlogged table is left behind.
Per buildfarm pink after
4fb5c794e586.
Alvaro Herrera [Mon, 25 Apr 2022 13:00:49 +0000 (15:00 +0200)]
Cover brin/gin/gist/spgist ambuildempty routines in regression tests
Changing some TEMP or permanent tables to UNLOGGED is sufficient to
invoke these ambuildempty routines, which were all not uncovered by any
tests. These changes do not otherwise affect the test suite.
Author: Amul Sul <sulamul@gmail.com>
Discussion: https://postgr.es/m/CAAJ_b95nneRCLM-=qELEdgCYSk6W_++-C+Q_t+wH3SW-hF50iw@mail.gmail.com
Alvaro Herrera [Mon, 25 Apr 2022 08:32:13 +0000 (10:32 +0200)]
Always pfree strings returned by GetDatabasePath
Several places didn't do it, and in many cases it didn't matter because
it would be a small allocation in a short-lived context; but other
places may accumulate a few (for example, in CreateDatabaseUsingFileCopy,
one per tablespace). In most databases this is highly unlikely to be
very serious either, but it seems better to make the code consistent in
case there's future copy-and-paste.
The only case of actual concern seems to be the aforementioned routine,
which is new with commit
9c08aea6a309, so there's no need to backpatch.
As pointed out by Coverity.
Tom Lane [Sat, 23 Apr 2022 16:16:12 +0000 (12:16 -0400)]
Fix incautious CTE matching in rewriteSearchAndCycle().
This function looks for a reference to the recursive WITH CTE,
but it checked only the CTE name not ctelevelsup, so that it could
seize on a lower CTE that happened to have the same name. This
would result in planner failures later, either weird errors such as
"could not find attribute 2 in subquery targetlist", or crashes
or assertion failures. The code also merely Assert'ed that it found
a matching entry, which is not guaranteed at all by the parser.
Per bugs #17320 and #17318 from Zhiyong Wu.
Thanks to Kyotaro Horiguchi for investigation.
Discussion: https://postgr.es/m/17320-
70e37868182512ab@postgresql.org
Discussion: https://postgr.es/m/17318-
2eb65a3a611d2368@postgresql.org
Noah Misch [Sat, 23 Apr 2022 03:20:11 +0000 (20:20 -0700)]
Test ALIGNOF_DOUBLE==4 compatibility under ALIGNOF_DOUBLE==8.
Today's test case detected alignment problems only when executing on
AIX. This change lets popular platforms detect the same problems.
Reviewed by Masahiko Sawada.
Discussion: https://postgr.es/m/
20220415072601.GG862547@rfd.leadboat.com
Robert Haas [Fri, 22 Apr 2022 20:16:52 +0000 (16:16 -0400)]
Remove some recently-added pg_dump test cases.
Commit
d2d35479796c3510e249d6fc72adbd5df918efbf included a pretty
extensive set of test cases, and some of them don't work on all
of our Windows machines. This happens because IPC::Run expands
its arguments as shell globs on a few machines, but doesn't on most
of the buildfarm. It might be good to fix that problem systematically
somehow, but in the meantime, there are enough test cases for this
commit that it seems OK to just remove the ones that are failing.
Discussion: http://postgr.es/m/
3a190754-b2b0-d02b-dcfd-
4ec1610ffbcb@dunslane.net
Discussion: http://postgr.es/m/CA+TgmoYRGUcFBy6VgN0+Pn4f6Wv=2H0HZLuPHqSy6VC8Ba7vdg@mail.gmail.com
Peter Eisentraut [Fri, 22 Apr 2022 09:19:17 +0000 (11:19 +0200)]
doc: Add links to tables
Formal tables should generally have an xref in the text that points to
them. Add them here.
David Rowley [Fri, 22 Apr 2022 04:02:15 +0000 (16:02 +1200)]
Fix performance regression in tuplesort specializations
697492434 added 3 new qsort specialization functions aimed to improve the
performance of sorting many of the common pass-by-value data types when
they're the leading or only sort key.
Unfortunately, that has caused a performance regression when sorting
datasets where many of the values being compared were equal. What was
happening here was that we were falling back to the standard sort
comparison function to handle tiebreaks. When the two given Datums
compared equally we would incur both the overhead of an indirect function
call to the standard comparer to perform the tiebreak and also the
standard comparer function would go and compare the leading key needlessly
all over again.
Here improve the situation in the 3 new comparison functions. We now
return 0 directly when the two Datums compare equally and we're performing
a 1-key sort.
Here we don't do anything to help the multi-key sort case where the
leading key uses one of the sort specializations functions. On testing
this case, even when the leading key's values are all equal, there
appeared to be no performance regression. Let's leave it up to future
work to optimize that case so that the tiebreak function no longer
re-compares the leading key over again.
Another possible fix for this would have been to add 3 additional sort
specialization functions to handle single-key sorts for these
pass-by-value types. The reason we didn't do that here is that we may
deem some other sort specialization to be more useful than single-key
sorts. It may be impractical to have sort specialization functions for
every single combination of what may be useful and it was already decided
that further analysis into which ones are the most useful would be delayed
until the v16 cycle. Let's not let this regression force our hand into
trying to make that decision for v15.
Author: David Rowley
Reviewed-by: John Naylor
Discussion: https://postgr.es/m/CA+hUKGJRbzaAOUtBUcjF5hLtaSHnJUqXmtiaLEoi53zeWSizeA@mail.gmail.com
Tom Lane [Thu, 21 Apr 2022 21:58:52 +0000 (17:58 -0400)]
Remove inadequate assertion check in CTE inlining.
inline_cte() expected to find exactly as many references to the
target CTE as its cterefcount indicates. While that should be
accurate for the tree as emitted by the parser, there are some
optimizations that occur upstream of here that could falsify it,
notably removal of unused subquery output expressions.
Trying to make the accounting 100% accurate seems expensive and
doomed to future breakage. It's not really worth it, because
all this code is protecting is downstream assumptions that every
referenced CTE has a plan. Let's convert those assertions to
regular test-and-elog just in case there's some actual problem,
and then drop the failing assertion.
Per report from Tomas Vondra (thanks also to Richard Guo for
analysis). Back-patch to v12 where the faulty code came in.
Discussion: https://postgr.es/m/
29196a1e-ed47-c7ca-9be2-
b1c636816183@enterprisedb.com
Tom Lane [Thu, 21 Apr 2022 21:12:49 +0000 (17:12 -0400)]
Fix missed cases in libpq's error handling.
Commit
618c16707 invented an "error_result" flag in PGconn, which
intends to represent the state that we have an error condition and
need to build a PGRES_FATAL_ERROR PGresult from the message text in
conn->errorMessage, but have not yet done so. (Postponing construction
of the error object simplifies dealing with out-of-memory conditions
and with concatenation of messages for multiple errors.) For nearly all
purposes, this "virtual" PGresult object should act the same as if it
were already materialized. But a couple of places in fe-protocol3.c
didn't get that memo, and were only testing conn->result as they used
to, without also checking conn->error_result.
In hopes of reducing the probability of similar mistakes in future,
I invented a pgHavePendingResult() macro that includes both tests.
Per report from Peter Eisentraut.
Discussion: https://postgr.es/m/
b52277b9-fa66-b027-4a37-
fb8989c73ff8@enterprisedb.com
Tom Lane [Thu, 21 Apr 2022 20:23:12 +0000 (16:23 -0400)]
Rethink method for assigning OIDs to the template0 and postgres DBs.
Commit
aa0105141 assigned fixed OIDs to template0 and postgres
in a very ad-hoc way. Notably, instead of teaching Catalog.pm
about these OIDs, the unused_oids script was just hacked to
not show them as unused. That's problematic since, for example,
duplicate_oids wouldn't report any future conflict. Hence,
invent a macro DECLARE_OID_DEFINING_MACRO() that can be used to
define an OID that is known to Catalog.pm and will participate
in duplicate-detection as well as renumbering by renumber_oids.pl.
(We don't anticipate renumbering these particular OIDs, but we
might as well build out all the Catalog.pm infrastructure while
we're here.)
Another issue is that
aa0105141 neglected to touch IsPinnedObject,
with the result that it now claimed template0 and postgres are
pinned. The right thing to do there seems to be to teach it that
no database is pinned, since in fact DROP DATABASE doesn't check
for pinned-ness (and at least for these cases, that is an
intentional choice). It's not clear whether this wrong answer
had any visible effect, but perhaps it could have resulted in
erroneous management of dependency entries.
In passing, rename the TemplateDbOid macro to Template1DbOid
to reduce confusion (likely we should have done that way back
when we invented template0, but we didn't), and rename the
OID macros for template0 and postgres to have a similar style.
There are no changes to postgres.bki here, so no need for a
catversion bump.
Discussion: https://postgr.es/m/
2935358.
1650479692@sss.pgh.pa.us
Alvaro Herrera [Thu, 21 Apr 2022 17:12:21 +0000 (19:12 +0200)]
Standardize references to Zstandard as <productname>
Some places used ZSTD, which isn't widely used anywhere. Use ZSTD only
to refer to the environment variable; use zstd (all lowercase) to refer
to the utility.
Per complaint from Justin Pryzby.
Discussion: https://postgr.es/m/
20220414003301.GT26620@telsasoft.com
Alvaro Herrera [Thu, 21 Apr 2022 16:57:40 +0000 (18:57 +0200)]
CREATE PUBLICATION ref: Minor tweaks to row filters
Prompted by a complaint from Justin Pryzby.
Reviewed-by: Amit Kapila <amit.kapila16@gmail.com>
Discussion: https://postgr.es/m/
20220414003301.GT26620@telsasoft.com
Tom Lane [Thu, 21 Apr 2022 16:02:23 +0000 (12:02 -0400)]
Use DECLARE_TOAST_WITH_MACRO() to simplify toast-table declarations.
This is needed so that renumber_oids.pl can handle renumbering
shared catalog declarations, which need to provide C macros for
the OIDs of the shared toast table and index. The previous
method of writing a C macro separately was error-prone anyway.
Also teach renumber_oids.pl about DECLARE_UNIQUE_INDEX_PKEY,
as we missed doing when inventing that macro.
There are no changes to postgres.bki here, so no need for a
catversion bump.
Discussion: https://postgr.es/m/
2995325.
1650487527@sss.pgh.pa.us
Etsuro Fujita [Thu, 21 Apr 2022 06:30:00 +0000 (15:30 +0900)]
postgres_fdw: Disable batch insert when BEFORE ROW INSERT triggers exist.
Previously, we allowed this, but such triggers might query the table to
insert into and act differently if the tuples that have already been
processed and prepared for insertion are not there, so disable it in
such cases.
Back-patch to v14 where batch insert was added.
Discussion: https://postgr.es/m/CAPmGK16_uPqsmgK0-LpLSUk54_BoK13bPrhxhfjSoSTVz414hA%40mail.gmail.com
Peter Geoghegan [Thu, 21 Apr 2022 01:29:02 +0000 (18:29 -0700)]
vacuumlazy.c: MultiXactIds are MXIDs, not XMIDs.
Oversights in commits
0b018fab and
f3c15cbe.
Peter Geoghegan [Thu, 21 Apr 2022 00:17:43 +0000 (17:17 -0700)]
Fix CLUSTER tuplesorts on abbreviated expressions.
CLUSTER sort won't use the datum1 SortTuple field when clustering
against an index whose leading key is an expression. This makes it
unsafe to use the abbreviated keys optimization, which was missed by the
logic that sets up SortSupport state. Affected tuplesorts output tuples
in a completely bogus order as a result (the wrong SortSupport based
comparator was used for the leading attribute).
This issue is similar to the bug fixed on the master branch by recent
commit
cc58eecc5d. But it's a far older issue, that dates back to the
introduction of the abbreviated keys optimization by commit
4ea51cdfe8.
Backpatch to all supported versions.
Author: Peter Geoghegan <pg@bowt.ie>
Author: Thomas Munro <thomas.munro@gmail.com>
Discussion: https://postgr.es/m/CA+hUKG+bA+bmwD36_oDxAoLrCwZjVtST2fqe=b4=qZcmU7u89A@mail.gmail.com
Backpatch: 10-
Tom Lane [Wed, 20 Apr 2022 22:08:15 +0000 (18:08 -0400)]
Disallow infinite endpoints in generate_series() for timestamps.
Such cases will lead to infinite loops, so they're of no practical
value. The numeric variant of generate_series() already threw error
for this, so borrow its message wording.
Per report from Richard Wesley. Back-patch to all supported branches.
Discussion: https://postgr.es/m/
91B44E7B-68D5-448F-95C8-
B4B3B0F5DEAF@duckdblabs.com
Robert Haas [Wed, 20 Apr 2022 15:02:35 +0000 (11:02 -0400)]
Allow db.schema.table patterns, but complain about random garbage.
psql, pg_dump, and pg_amcheck share code to process object name
patterns like 'foo*.bar*' to match all tables with names starting in
'bar' that are in schemas starting with 'foo'. Before v14, any number
of extra name parts were silently ignored, so a command line '\d
foo.bar.baz.bletch.quux' was interpreted as '\d bletch.quux'. In v14,
as a result of commit
2c8726c4b0a496608919d1f78a5abc8c9b6e0868, we
instead treated this as a request for table quux in a schema named
'foo.bar.baz.bletch'. That caused problems for people like Justin
Pryzby who were accustomed to copying strings of the form
db.schema.table from messages generated by PostgreSQL itself and using
them as arguments to \d.
Accordingly, revise things so that if an object name pattern contains
more parts than we're expecting, we throw an error, unless there's
exactly one extra part and it matches the current database name.
That way, thisdb.myschema.mytable is accepted as meaning just
myschema.mytable, but otherdb.myschema.mytable is an error, and so
is some.random.garbage.myschema.mytable.
Mark Dilger, per report from Justin Pryzby and discussion among
various people.
Discussion: https://www.postgresql.org/message-id/
20211013165426.GD27491%40telsasoft.com
Tom Lane [Wed, 20 Apr 2022 15:04:28 +0000 (11:04 -0400)]
Remove trailing whitespace from *.sgml files.
Historically we've been lax about this, but seeing that we're not
lax in C files, there doesn't seem to be a good reason to be so
in the documentation. Remove the existing occurrences (mostly
though not entirely in copied-n-pasted psql output), and modify
.gitattributes so that "git diff --check" will warn about future
cases.
While at it, add *.pm to the set of extensions .gitattributes
knows about, and remove some obsolete entries for files that
we don't have in the tree anymore.
Per followup discussion of commit
5a892c9b1.
Discussion: https://postgr.es/m/E1nfcV1-000kOR-E5@gemulon.postgresql.org
Peter Eisentraut [Wed, 20 Apr 2022 14:11:14 +0000 (16:11 +0200)]
Fix incorrect format placeholders
Alvaro Herrera [Wed, 20 Apr 2022 09:44:08 +0000 (11:44 +0200)]
set_deparse_plan: Reuse variable to appease Coverity
Coverity complains that dpns->outer_plan is deferenced (to obtain
->targetlist) when possibly NULL. We can avoid this by using
dpns->outer_tlist instead, which was already obtained a few lines up.
The fact that we end up with
dpns->inner_tlist = dpns->outer_tlist
is a bit suspicious-looking and maybe worthy of more investigation, but
I'll leave that for another day.
Reviewed-by: Michaël Paquier <michael@paquier.xyz>
Discussion: https://postgr.es/m/
202204191345.qerjy3kxi3eb@alvherre.pgsql
Alvaro Herrera [Wed, 20 Apr 2022 09:18:04 +0000 (11:18 +0200)]
Move ModifyTableContext->lockmode to UpdateContext
Should have been done this way to start with, but I failed to notice
This way we avoid some pointless initialization, and better contains the
variable to exist in the scope where it is really used.
Reviewed-by: Michaël Paquier <michael@paquier.xyz>
Discussion: https://postgr.es/m/
202204191345.qerjy3kxi3eb@alvherre.pgsql
Alvaro Herrera [Wed, 20 Apr 2022 08:34:58 +0000 (10:34 +0200)]
ExecModifyTable: use context.planSlot instead of planSlot
There's no reason to keep a separate local variable when we have a place
for it elsewhere. This allows to simplify some code.
Reviewed-by: Michaël Paquier <michael@paquier.xyz>
Discussion: https://postgr.es/m/
202204191345.qerjy3kxi3eb@alvherre.pgsql
Amit Kapila [Wed, 20 Apr 2022 03:29:55 +0000 (08:59 +0530)]
Stabilize streaming tests in test_decoding.
We have some streaming tests that rely on the size of changes which can
fail if there are additional changes like invalidation messages by
background activity like auto analyze. Avoid such failures by increasing
autovacuum_naptime to a reasonably high value (1d).
Author: Dilip Kumar
Backpatch-through: 14
Discussion: https://postgr.es/m/
1958043.
1650129119@sss.pgh.pa.us
David Rowley [Wed, 20 Apr 2022 03:17:56 +0000 (15:17 +1200)]
Doc: use "an SQL" consistently rather than "a SQL"
Similarly to what was done in
04539e73f, we standardized on SQL being
pronounced "es-que-ell" rather than "sequel" in our documentation.
Two inconsistencies have crept in during the v15 cycle. The others
existed before but were missed in
04539e73f due to none of the searches
accounting for "SQL" being wrapped in tags.
As with
04539e73f, we don't touch code comments here in order to not
create unnecessary back-patching pain.
Discussion: https://postgr.es/m/CAApHDvpML27UqFXnrYO1MJddsKVMQoiZisPvsAGhKE_tsKXquw%40mail.gmail.com
Tom Lane [Wed, 20 Apr 2022 03:03:59 +0000 (23:03 -0400)]
Fix breakage in AlterFunction().
An ALTER FUNCTION command that tried to update both the function's
proparallel property and its proconfig list failed to do the former,
because it stored the new proparallel value into a tuple that was
no longer the interesting one. Carelessness in
7aea8e4f2.
(I did not bother with a regression test, because the only likely
future breakage would be for someone to ignore the comment I added
and add some other field update after the heap_modify_tuple step.
A test using existing function properties could not catch that.)
Per report from Bryn Llewellyn. Back-patch to all supported branches.
Discussion: https://postgr.es/m/
8AC9A37F-99BD-446F-A2F7-
B89AD0022774@yugabyte.com
Michael Paquier [Wed, 20 Apr 2022 02:05:34 +0000 (11:05 +0900)]
Remove duplicated word in comment of basebackup.c
Oversight in
39969e2.
Author: Martín Marqués
Discussion: https://postgr.es/m/CABeG9LviA01oHC5h=ksLUuhMyXxmZR_tftRq6q3341CMT=j=4g@mail.gmail.com
Peter Eisentraut [Tue, 19 Apr 2022 18:38:53 +0000 (20:38 +0200)]
Fix extract epoch from interval calculation
The new numeric code for extract epoch from interval accidentally
truncated the DAYS_PER_YEAR value to an integer, leading to results
that mismatched the floating-point interval_part calculations.
The commit
a2da77cdb4661826482ebf2ddba1f953bc74afe4 that introduced
this actually contains the regression test change that this reverts.
I suppose this was missed at the time.
Reported-by: Joseph Koshakow <koshy44@gmail.com>
Reviewed-by: Tom Lane <tgl@sss.pgh.pa.us>
Discussion: https://www.postgresql.org/message-id/flat/CAAvxfHd5n%3D13NYA2q_tUq%3D3%3DSuWU-CufmTf-Ozj%3DfrEgt7pXwQ%40mail.gmail.com
Tatsuo Ishii [Tue, 19 Apr 2022 08:04:27 +0000 (17:04 +0900)]
Fix aggregate logging of pgbench.
Remove meaningless "failures" column from the aggregate logging. It
was just a sum of "serialization failures" and "deadlock failures".
Pointed out by Tom Lane. Patch reviewed by Fabien COELHO.
Discussion: https://postgr.es/m/
4183048.
1649536705%40sss.pgh.pa.us
Amit Kapila [Tue, 19 Apr 2022 03:19:49 +0000 (08:49 +0530)]
Fix the check to limit sync workers.
We don't allow to invoke more sync workers once we have reached the sync
worker limit per subscription. But the check to enforce this also doesn't
allow to launch an apply worker if it gets restarted.
This code was introduced by commit
de43897122 but we caught the problem
only with the test added by recent commit
c91f71b9dc which started failing
occasionally in the buildfarm.
As per buildfarm.
Diagnosed-by: Amit Kapila, Masahiko Sawada, Tomas Vondra
Author: Amit Kapila
Backpatch-through: 10
Discussion: https://postgr.es/m/CAH2L28vddB_NFdRVpuyRBJEBWjz4BSyTB=_ektNRH8NJ1jf95g@mail.gmail.com
https://postgr.es/m/
f90d2b03-4462-ce95-a524-
d91464e797c8@enterprisedb.com
Tom Lane [Tue, 19 Apr 2022 00:04:55 +0000 (20:04 -0400)]
Add missing error handling in pg_md5_hash().
It failed to provide an error string as expected for the
admittedly-unlikely case of OOM in pg_cryptohash_create().
Also, make it initialize *errstr to NULL for success,
as pg_md5_binary() does.
Also add missing comments. Readers should not have to
reverse-engineer the API spec for a publicly visible routine.
Tom Lane [Mon, 18 Apr 2022 16:16:45 +0000 (12:16 -0400)]
Avoid invalid array reference in transformAlterTableStmt().
Don't try to look at the attidentity field of system attributes,
because they're not there in the TupleDescAttr array. Sometimes
this is harmless because we accidentally pick up a zero, but
otherwise we'll report "no owned sequence found" from an attempt
to alter a system attribute. (It seems possible that a SIGSEGV
could occur, too, though I've not seen it in testing.)
It's not in this function's charter to complain that you can't
alter a system column, so instead just hard-wire an assumption
that system attributes aren't identities. I didn't bother with
a regression test because the appearance of the bug is very
erratic.
Per bug #17465 from Roman Zharkov. Back-patch to all supported
branches. (There's not actually a live bug before v12, because
before that get_attidentity() did the right thing anyway.
But for consistency I changed the test in the older branches too.)
Discussion: https://postgr.es/m/17465-
f2a554a6cb5740d3@postgresql.org
Michael Paquier [Mon, 18 Apr 2022 04:41:40 +0000 (13:41 +0900)]
Fix second race condition in 002_archiving.pl with archive_cleanup_command
Checking the execution of archive_cleanup_command on a standby requires
a valid checkpoint coming from its primary, but the logic did not check
that the standby replayed up to the point of the checkpoint, causing the
test checking for the execution of archive_cleanup_command to fail.
This race was more visible in slow environments.
Issue introduced in
46dea24, so no backpatch is needed.
Author: Tom Lane
Discussion: https://postgr.es/m/
4015413.
1649454951@sss.pgh.pa.us
Amit Kapila [Mon, 18 Apr 2022 03:12:37 +0000 (08:42 +0530)]
Add additional documentation for row filters.
Commit
52e4f0cd47 added a feature to allow specifying row filters for
logical replication of tables. This patch adds detailed documentation on
that feature including examples to make it easier for users to understand.
Author: Peter Smith, Euler Taveira
Reviewed By: Greg Nancarrow, Aleksander Alekseev, Amit Kapila, Ajin Cherian, Alvaro Herrera
Discussion: https://postgr.es/m/CAHut+PtnsBr59=_NvxXp_=S-em0WxyuDOQmSTuHGb4sVhkHffg@mail.gmail.com
Michael Paquier [Mon, 18 Apr 2022 02:39:50 +0000 (11:39 +0900)]
Fix race in TAP test 002_archiving.pl when restoring history file
This test, introduced in
df86e52, uses a second standby to check that
it is able to remove correctly RECOVERYHISTORY and RECOVERYXLOG at the
end of recovery. This standby uses the archives of the primary to
restore its contents, with some of the archive's contents coming from
the first standby previously promoted. In slow environments, it was
possible that the test did not check what it should, as the history file
generated by the promotion of the first standby may not be stored yet on
the archives the second standby feeds on. So, it could be possible that
the second standby selects an incorrect timeline, without restoring a
history file at all.
This commits adds a wait phase to make sure that the history file
required by the second standby is archived before this cluster is
created. This relies on poll_query_until() with pg_stat_file() and an
absolute path, something not supported in REL_10_STABLE.
While on it, this adds a new test to check that the history file has
been restored by looking at the logs of the second standby. This
ensures that a RECOVERYHISTORY, whose removal needs to be checked,
is created in the first place. This should make the test more robust.
This test has been introduced by
df86e52, but it came in light as an
effect of the bug fixed by
acf1dd42, where the extra restore_command
calls made the test much slower.
Reported-by: Andres Freund
Discussion: https://postgr.es/m/YlT23IvsXkGuLzFi@paquier.xyz
Backpatch-through: 11
Michael Paquier [Mon, 18 Apr 2022 01:18:34 +0000 (10:18 +0900)]
Handle compression level in pg_receivewal for LZ4
The new option set of pg_receivewal introduced in
042a923 to control the
compression method makes it now easy to pass down various options,
including the compression level. The change to be able to do is simple,
and requires one LZ4F_preferences_t fed to LZ4F_compressBegin().
Note that LZ4F_INIT_PREFERENCES could be used to initialize the contents
of LZ4F_preferences_t as required by LZ4, but this is only available
since v1.8.3. memset()'ing its structure to 0 is enough.
Discussion: https://postgr.es/m/YlPQGNAAa04raObK@paquier.xyz
Noah Misch [Sun, 17 Apr 2022 00:43:54 +0000 (17:43 -0700)]
Add a temp-install prerequisite to src/interfaces/ecpg "checktcp".
The target failed, tested $PATH binaries, or tested a stale temporary
installation. Commit
c66b438db62748000700c9b90b585e756dd54141 missed
this. Back-patch to v10 (all supported versions).
Thomas Munro [Sat, 16 Apr 2022 22:22:03 +0000 (10:22 +1200)]
Don't retry restore_command while reading ahead.
Suppress further attempts to read ahead in the WAL if we run out of
data, until the records already decoded have been replayed. This
restores the traditional behavior for continuous archive recovery, which
is to retry the failing restore_command only every 5 seconds. With the
coding in
5dc0418f, we would start retrying every time through the
recovery loop when our WAL decoding window hit the end of the current
segment and we tried to look ahead into a not-yet-available next file.
That was very slow.
Also change the no_readahead_until mechanism to use <= rather than <,
which seems more useful. Otherwise we'd either get one extra unwanted
retry of restore_command, or we'd need to add 1 to an LSN.
No change in behavior for regular streaming. That was already limited
by the flushedUpto variable, which won't be updated until we replay what
we have already.
Reported by Andres Freund while analyzing the failure of a TAP test on
build farm animal skink (investigation ongoing but probably due to
otherwise unrelated timing bugs triggered by this slowness magnified by
valgrind).
Discussion: https://postgr.es/m/
20220409005910.alw46xqmmgny2sgr%40alap3.anarazel.de
Andres Freund [Sat, 16 Apr 2022 19:13:31 +0000 (12:13 -0700)]
pgstat: Use correct lock level in pgstat_drop_all_entries().
Previously we didn't, which lead to an assertion failure when resetting
partially loaded statistics. This was encountered on the buildfarm, for
as-of-yet unknown reasons.
Ttighten up a validity check when reading the stats file, verifying 'E'
signals the end of the file (rather than just stopping reading). That's then
used in a test appending to the stats file that crashed before the fix in
pgstat_drop_all_entries().
Reported by buildfarm animals mylodon and kestrel, via Tom Lane.
Discussion: https://postgr.es/m/
1656446.
1650043715@sss.pgh.pa.us
Tom Lane [Sat, 16 Apr 2022 20:04:50 +0000 (16:04 -0400)]
Fix incorrect logic in HaveRegisteredOrActiveSnapshot().
This function gave the wrong answer when there's more than one
RegisteredSnapshots entry, whether or not any of them is the
CatalogSnapshot. This leads to assertion failure in some scenarios
involving fetching toasted data using a cursor. (As per discussion,
I'm dubious that this is the right contract to be enforcing at all;
but it surely doesn't help to be enforcing it incorrectly.)
Fetching toasted data using a cursor is evidently under-tested,
so add a test case too.
Per report from Erik Rijkers. This is new code, so no need for
back-patch.
Discussion: https://postgr.es/m/
dc9dd229-ed30-6c62-4c41-
d733ffff776b@xs4all.nl
Andrew Dunstan [Sat, 16 Apr 2022 13:35:15 +0000 (09:35 -0400)]
Build libpq test programs under MSVC
This allows the newly added TAP tests to run.
Peter Eisentraut [Sat, 16 Apr 2022 07:05:07 +0000 (09:05 +0200)]
Fix some trailing whitespace in documentation files
Noah Misch [Sat, 16 Apr 2022 06:15:38 +0000 (23:15 -0700)]
Use standard timeout, in 010_pg_basebackup.pl.
Per buildfarm member mandrill. The test is new in v15, so no back-patch.
Peter Geoghegan [Fri, 15 Apr 2022 22:48:39 +0000 (15:48 -0700)]
Fix multi-table VACUUM VERBOSE accounting.
Per-backend global variables like VacuumPageHit are initialized once per
VACUUM command. This was missed by commit
49c9d9fc, which unified
VACUUM VERBOSE and autovacuum logging. As a result of that oversight,
incorrect values were shown when multiple relations were processed by a
single VACUUM VERBOSE command.
Relations that happened to be processed later on would show "buffer
usage:" values that incorrectly included buffer accesses made while
processing earlier unrelated relations. The same accesses were counted
multiple times.
To fix, take initial values for the tracker variables at the start of
heap_vacuum_rel(), and report delta values later on.
Tom Lane [Fri, 15 Apr 2022 22:31:01 +0000 (18:31 -0400)]
psql: fix \l display for pre-v15 databases.
With a pre-v15 server, show NULL for the "ICU Locale" column,
matching what you see in v15 when the database locale isn't ICU.
The previous coding incorrectly repeated datcollate here.
(There's an unfinished discussion about whether to consolidate
these columns in \l output, but in any case we'd want this fix
for \l+ output.)
Euler Taveira, per report from Christoph Berg
Discussion: https://postgr.es/m/YlmIFCqu+TZSW4rB@msg.df7cb.de
Tom Lane [Fri, 15 Apr 2022 21:50:01 +0000 (17:50 -0400)]
Tighten ComputeXidHorizons' handling of walsenders.
ComputeXidHorizons (nee GetOldestXmin) thought that it could identify
walsenders by checking for proc->databaseId == 0. Perhaps that was
safe when the code was written, but it's been wrong at least since
autovacuum was invented. Background processes that aren't connected
to any particular database, such as the autovacuum launcher and
logical replication launcher, look like that too.
This imprecision is harmful because when such a process advertises an
xmin, the result is to hold back dead-tuple cleanup in all databases,
though it'd be sufficient to hold it back in shared catalogs (which
are the only relations such a process can access). Aside from being
generally inefficient, this has recently been seen to cause regression
test failures in the buildfarm, as a consequence of the logical
replication launcher's startup transaction preventing VACUUM from
marking pages of a user table as all-visible.
We only want that global hold-back effect for the case where a
walsender is advertising a hot standby feedback xmin. Therefore,
invent a new PGPROC flag that says that a process' xmin should be
considered globally, and check that instead of using the incorrect
databaseId == 0 test. Currently only a walsender sets that flag,
and only if it is not connected to any particular database. (This is
for bug-compatibility with the undocumented behavior of the existing
code, namely that feedback sent by a client who has connected to a
particular database would not be applied globally. I'm not sure this
is a great definition; however, such a client is capable of issuing
plain SQL commands, and I don't think we want xmins advertised for
such commands to be applied globally. Perhaps this could do with
refinement later.)
While at it, I rewrote the comment in ComputeXidHorizons, and
re-ordered the commented-upon if-tests, to make them match up
for intelligibility's sake.
This is arguably a back-patchable bug fix, but given the lack of
complaints I think it prudent to let it age awhile in HEAD first.
Discussion: https://postgr.es/m/
1346227.
1649887693@sss.pgh.pa.us
Peter Geoghegan [Fri, 15 Apr 2022 21:20:56 +0000 (14:20 -0700)]
VACUUM VERBOSE: Show dead items for an empty table.
Be consistent about the lines that VACUUM VERBOSE outputs by including
an "index scan not needed: " line for completely empty tables. This
makes the output more readable, especially with multiple distinct VACUUM
operations processed by the same VACUUM command. It's also more
consistent; even empty tables can use the failsafe, which wasn't
reported in the standard way until now.
Follow-up to commit
6e20f460, which taught VACUUM VERBOSE to be more
consistent about reporting on scanned pages with empty tables.
Peter Geoghegan [Fri, 15 Apr 2022 20:21:43 +0000 (13:21 -0700)]
Adjust VACUUM's removable cutoff log message.
The age of OldestXmin (a.k.a. "removable cutoff") when VACUUM ends often
indicates the approximate number of XIDs consumed while VACUUM ran.
However, there is at least one important exception: the cutoff could be
held back by a snapshot that was acquired before our VACUUM even began.
Successive VACUUM operations may even use exactly the same old cutoff in
extreme cases involving long held snapshots.
The log messages that described how removable cutoff aged (which were
added by commit
872770fd) created the impression that we were reporting
on how VACUUM's usable cutoff advanced while VACUUM ran, which was
misleading in these extreme cases. Fix by using a more general wording.
Per gripe from Tom Lane.
In passing, relocate related instrumentation code for clarity.
Author: Peter Geoghegan <pg@bowt.ie>
Discussion: https://postgr.es/m/
1643035.
1650035653@sss.pgh.pa.us
Tom Lane [Fri, 15 Apr 2022 17:29:39 +0000 (13:29 -0400)]
Revert "Temporarily add some probes of tenk1's relallvisible in create_index.sql."
This reverts commit
5bb2b6abc8d6cf120a814317816e4384bcbb9c1e.
Not needed anymore.
Andrew Dunstan [Fri, 15 Apr 2022 11:47:12 +0000 (07:47 -0400)]
Small cleanups in SQL/JSON code
These are to keep Coverity happy. In one case remove a redundant NULL
check, and in another explicitly ignore a function result that is already
known.
Andres Freund [Fri, 15 Apr 2022 00:40:25 +0000 (17:40 -0700)]
pgstat: set timestamps of fixed-numbered stats after a crash.
When not loading stats at startup (i.e. pgstat_discard_stats() getting
called), reset timestamps of fixed numbered stats would be left at
0. Oversight in
5891c7a8ed8.
Instead use pgstat_reset_after_failure() and add tests verifying that
fixed-numbered reset timestamps are set appropriately.
Reported-By: "David G. Johnston" <david.g.johnston@gmail.com>
Discussion: https://postgr.es/m/CAKFQuwamFuaQHKdhcMt4Gbw5+Hca2UE741B8gOOXoA=TtAd2Yw@mail.gmail.com
Alvaro Herrera [Thu, 14 Apr 2022 20:11:06 +0000 (22:11 +0200)]
Have CLUSTER ignore partitions not owned by caller
If a partitioned table has partitions owned by roles other than the
owner of the partitioned table, don't include them in the to-be-
clustered list. This is similar to what VACUUM FULL does (except we do
it sooner, because there is no reason to postpone it). Add a simple
test to verify that only owned partitions are clustered.
While at it, change memory context switch-and-back to occur once per
partition instead of outside of the loop.
Author: Justin Pryzby <pryzby@telsasoft.com>
Reviewed-by: Zhihong Yu <zyu@yugabyte.com>
Reviewed-by: Michael Paquier <michael@paquier.xyz>
Discussion: https://postgr.es/m/
20220411140609.GF26620@telsasoft.com
Alvaro Herrera [Thu, 14 Apr 2022 19:52:20 +0000 (21:52 +0200)]
Reword text on ROW SHARE lock as acquired by SELECT FOR <lock>
It was missing lock levels FOR KEY SHARE and FOR NO KEY EXCLUSIVE; but
also SELECT FOR UPDATE is not a command separate from SELECT, as the
original text implied. It is clearer to state that FOR <lock strength>
is an option of regular SELECT.
Per suggestion from Joey Bodoia <jbodoia21@cmc.edu>
Reviewed-by: Joey Bodoia <jbodoia21@cmc.edu> (offlist)
Reviewed-by: Erikjan Rijkers <er@xs4all.nl>
Discussion: https://postgr.es/m/
164908765512.682.
17348032020747341013@wrigleys.postgresql.org
Tom Lane [Thu, 14 Apr 2022 16:14:01 +0000 (12:14 -0400)]
Temporarily add some probes of tenk1's relallvisible in create_index.sql.
This is to gather some more evidence about why buildfarm member wrasse
is failing. We should revert it (or at least scale it way back) once
that's resolved.
Discussion: https://postgr.es/m/
1346227.
1649887693@sss.pgh.pa.us
Andrew Dunstan [Thu, 14 Apr 2022 14:26:29 +0000 (10:26 -0400)]
Improve a couple of sql/json error messages
Fix the grammar in two, and add a hint to one.
Andrew Dunstan [Thu, 14 Apr 2022 12:57:09 +0000 (08:57 -0400)]
Fix transformJsonBehavior
Commit
1a36bc9dba8 conained some logic that was a little opaque and
could have involved a NULL dereference, as complained about by Coverity.
Make the logic more transparent and in doing so avoid the NULL
dereference.
Michael Paquier [Thu, 14 Apr 2022 06:08:03 +0000 (15:08 +0900)]
pageinspect: Fix handling of all-zero pages
Getting from get_raw_page() an all-zero page is considered as a valid
case by the buffer manager and it can happen for example when finding a
corrupted page with zero_damaged_pages enabled (using zero_damaged_pages
to look at corrupted pages happens), or after a crash when a relation
file is extended before any WAL for its new data is generated (before a
vacuum or autovacuum job comes in to do some cleanup).
However, all the functions of pageinspect, as of the index AMs (except
hash that has its own idea of new pages), heap, the FSM or the page
header have never worked with all-zero pages, causing various crashes
when going through the page internals.
This commit changes all the pageinspect functions to be compliant with
all-zero pages, where the choice is made to return NULL or no rows for
SRFs when finding a new page. get_raw_page() still works the same way,
returning a batch of zeros in the bytea of the page retrieved. A hard
error could be used but NULL, while more invasive, is useful when
scanning relation files in full to get a batch of results for a single
relation in one query. Tests are added for all the code paths
impacted.
Reported-by: Daria Lepikhova
Author: Michael Paquier
Discussion: https://postgr.es/m/
561e187b-3549-c8d5-03f5-
525c14e65bd0@postgrespro.ru
Backpatch-through: 10
David Rowley [Wed, 13 Apr 2022 21:28:56 +0000 (09:28 +1200)]
Add missing spaces after single-line comments
Only 1 of 3 of these changes appear to be handled by pgindent. That change
is new to v15. The remaining two appear to be left alone by pgindent. The
exact reason for that is not 100% clear to me. It seems related to the
fact that it's a line that contains *only* a single line comment and no
actual code. It does not seem worth investigating this in too much
detail. In any case, these do not conform to our usual practices, so fix
them.
Author: Justin Pryzby
Discussion: https://postgr.es/m/
20220411020336.GB26620@telsasoft.com
David Rowley [Wed, 13 Apr 2022 21:16:05 +0000 (09:16 +1200)]
Docs: fix some spelling mistakes and also do some wordsmithing
All except one of these are new to v15. Only one of the wordsmithing
changes appears in older versions. The wordsmithing improvement does not
seem significant enough to warrant backpatching.
Author: Justin Pryzby
Discussion: https://postgr.es/m/
20220411020336.GB26620@telsasoft.com
Tom Lane [Wed, 13 Apr 2022 20:26:34 +0000 (16:26 -0400)]
Fix case sensitivity in psql's tab completion for GUC names.
Input for these should be case-insensitive, but was not completely
so. Comparing to the similar queries for timezone names, I realized
that we'd missed forcing the comparison pattern to lower-case.
With that, it behaves as I expect.
While here, flatten the sub-selects in these queries; I don't
find that those add any readability.
Discussion: https://postgr.es/m/
3369130.
1649348542@sss.pgh.pa.us
Tom Lane [Wed, 13 Apr 2022 19:03:58 +0000 (15:03 -0400)]
Further tweak the default behavior of psql's \dconfig.
Define "parameters with non-default settings" as being those that
not only have pg_settings.source different from 'default', but
also have a current value different from the hard-wired boot_val.
Adding the latter restriction removes a number of not-very-interesting
cases where the active setting is chosen by initdb but in practice
tends to be the same all the time.
Per discussion with Jonathan Katz.
Discussion: https://postgr.es/m/YlFQLzlPi4QD0wSi@msg.df7cb.de
Tom Lane [Wed, 13 Apr 2022 17:35:02 +0000 (13:35 -0400)]
Prevent access to no-longer-pinned buffer in heapam_tuple_lock().
heap_fetch() used to have a "keep_buf" parameter that told it to return
ownership of the buffer pin to the caller after finding that the
requested tuple TID exists but is invisible to the specified snapshot.
This was thoughtlessly removed in commit
5db6df0c0, which broke
heapam_tuple_lock() (formerly EvalPlanQualFetch) because that function
needs to do more accesses to the tuple even if it's invisible. The net
effect is that we would continue to touch the page for a microsecond or
two after releasing pin on the buffer. Usually no harm would result;
but if a different session decided to defragment the page concurrently,
we could see garbage data and mistakenly conclude that there's no newer
tuple version to chain up to. (It's hard to say whether this has
happened in the field. The bug was actually found thanks to a later
change that allowed valgrind to detect accesses to non-pinned buffers.)
The most reasonable way to fix this is to reintroduce keep_buf,
although I made it behave slightly differently: buffer ownership
is passed back only if there is a valid tuple at the requested TID.
In HEAD, we can just add the parameter back to heap_fetch().
To avoid an API break in the back branches, introduce an additional
function heap_fetch_extended() in those branches.
In HEAD there is an additional, less obvious API change: tuple->t_data
will be set to NULL in all cases where buffer ownership is not returned,
in particular when the tuple exists but fails the time qual (and
!keep_buf). This is to defend against any other callers attempting to
access non-pinned buffers. We concluded that making that change in back
branches would be more likely to introduce problems than cure any.
In passing, remove a comment about heap_fetch that was obsoleted by
9a8ee1dc6.
Per bug #17462 from Daniil Anisimov. Back-patch to v12 where the bug
was introduced.
Discussion: https://postgr.es/m/17462-
9c98a0f00df9bd36@postgresql.org
Alvaro Herrera [Wed, 13 Apr 2022 17:14:20 +0000 (19:14 +0200)]
Remove extraneous blank lines before block-closing braces
These are useless and distracting. We wouldn't have written the code
with them to begin with, so there's no reason to keep them.
Author: Justin Pryzby <pryzby@telsasoft.com>
Discussion: https://postgr.es/m/
20220411020336.GB26620@telsasoft.com
Discussion: https://postgr.es/m/attachment/133167/0016-Extraneous-blank-lines.patch