Commit Graph

37622 Commits

Author SHA1 Message Date
Michael Paquier 09a69f6e23 Add test for CREATE INDEX CONCURRENTLY with not-so-immutable predicate
83158f7 has improved index_set_state_flags() so as it is possible to use
transactional updates when updating pg_index state flags, but there was
not really a test case which stressed directly the possibility it fixed.
This commit adds such a test, using a predicate that looks valid in
appearance but calls a stable function.

Author: Andrey Lepikhov
Discussion: https://postgr.es/m/9b905019-5297-7372-0ad2-e1a4bb66a719@postgrespro.ru
Backpatch-through: 9.6
2021-06-28 11:17:05 +09:00
Tom Lane 642c0697c9 Remove memory leaks in isolationtester.
specscanner.l leaked a kilobyte of memory per token of the spec file.
Apparently somebody thought that the introductory code block would be
executed once; but it's once per yylex() call.

A couple of functions in isolationtester.c leaked small amounts of
memory due to not bothering to free one-time allocations.  Might
as well improve these so that valgrind gives this program a clean
bill of health.  Also get rid of an ugly static variable.

Coverity complained about one of the one-time leaks, which led me
to try valgrind'ing isolationtester, which led to discovery of the
larger leak.
2021-06-27 12:45:04 -04:00
Peter Eisentraut c302a61390 Error message refactoring
Take some untranslatable things out of the message and replace by
format placeholders, to reduce translatable strings and reduce
translation mistakes.
2021-06-27 09:41:16 +02:00
Tom Lane 8ec00dc5cd Remove undesirable libpq dependency on stringinfo.c.
Commit c0cb87fbb unwisely introduced a dependency on the StringInfo
machinery in fe-connect.c.  We must not use that in libpq, because
it will do a summary exit(1) if it hits OOM, and that is not
appropriate behavior for a general-purpose library.  The goal of
allowing arbitrary line lengths in service files doesn't seem like
it's worth a lot of effort, so revert back to the previous method
of using a stack-allocated buffer and failing on buffer overflow.

This isn't an exact revert though.  I kept that patch's refactoring
to have a single exit path, as that seems cleaner than having each
error path know what to do to clean up.  Also, I made the fixed-size
buffer 1024 bytes not 256, just to push off the need for an expandable
buffer some more.

There is more to do here; in particular the lack of any mechanical
check for this type of mistake now seems pretty hazardous.  But this
fix gets us back to the level of robustness we had in v13, anyway.

Discussion: https://postgr.es/m/daeb22ec6ca8ef61e94d766a9b35fb03cabed38e.camel@vmware.com
2021-06-26 14:20:17 -04:00
Michael Paquier d5a2c413fc Remove non-existing variable reference in MSVC's Solution.pm
The version string is grabbed from PACKAGE_VERSION in pg_config.h in the
MSVC build since 8f4fb4c6, but an error message referenced a variable
that existed before that.  This had no consequences except if one messes
up enough with the version number of the build.

Author: Anton Voloshin
Discussion: https://postgr.es/m/af79ee1b-9962-b299-98e1-f90a289e19e6@postgrespro.ru
Backpatch-through: 13
2021-06-26 13:52:48 +09:00
Michael Paquier 704e1dbd9a Remove some useless logs from the TAP tests of pgbench
002_pgbench_no_server was printing some array pointers instead of the
actual contents of those arrays for the expected outputs of stdout and
stderr for a tested command.  This does not add any new information that
can help with debugging as the test names allow to track failure
locations, if any.

This commit simply removes those logs as the rest of the printed
information is redundant with command_checks_all().

Per discussion with Andrew Dunstan and Álvaro Herrera.

Discussion: https://postgr.es/m/YNXNFaG7IgkzZanD@paquier.xyz
Backpatch-through: 11
2021-06-26 12:39:54 +09:00
Tom Lane 5a0f1c8c01 Remove unnecessary failure cases in RemoveRoleFromObjectPolicy().
It's not really necessary for this function to open or lock the
relation associated with the pg_policy entry it's modifying.  The
error checks it's making on the rel are if anything counterproductive
(e.g., if we don't want to allow installation of policies on system
catalogs, here is not the place to prevent that).  In particular, it
seems just wrong to insist on an ownership check.  That has the net
effect of forcing people to use superuser for DROP OWNED BY, which
surely is not an effect we want.  Also there is no point in rebuilding
the dependencies of the policy expressions, which aren't being
changed.  Lastly, locking the table also seems counterproductive; it's
not helping to prevent race conditions, since we failed to re-read the
pg_policy row after acquiring the lock.  That means that concurrent
DDL would likely result in "tuple concurrently updated/deleted"
errors; which is the same behavior this code will produce, with less
overhead.

Per discussion of bug #17062.  Back-patch to all supported versions,
as the failure cases this eliminates seem just as undesirable in 9.6
as in HEAD.

Discussion: https://postgr.es/m/1573181.1624220108@sss.pgh.pa.us
2021-06-25 13:59:38 -04:00
Michael Paquier 38ff135d94 Cleanup some code related to pgbench log checks in TAP tests
This fixes a couple of problems within the so-said code of this commit
subject:
- Replace the use of open() with slurp_file(), fixing an issue reported
by buildfarm member fairywren whose perl installation keep around CRLF
characters, causing the matching patterns for the logs to fail.
- Remove the eval block, which is not really necessary.

This set of issues has come into light after fixing a different issue
with c13585fe, and this is wrong since this code has been introduced.

Reported-by: Andrew Dunstan, and buildfarm member fairywren
Author: Michael Paquier
Reviewed-by: Andrew Dunstan
Discussion: https://postgr.es/m/0f49303e-7784-b3ee-200b-cbf67be2eb9e@dunslane.net
Backpatch-through: 11
2021-06-25 20:15:24 +09:00
Peter Eisentraut 3af10943ce Put option listing back into alphabetical order 2021-06-25 11:40:06 +02:00
Peter Eisentraut e59d428f34 Fixes in ALTER SUBSCRIPTION DROP PUBLICATION code
ALTER SUBSCRIPTION DROP PUBLICATION does not actually support
copy_data option, so remove it from tab completion.

Also, reword the error message that is thrown when all the
publications from a subscription are specified to be dropped.

Also, made few doc and cosmetic adjustments.

Author: Vignesh C <vignesh21@gmail.com>
Reviewed-by: Bharath Rupireddy <bharath.rupireddy@enterprisedb.com>
Reviewed-by: Japin Li <japinli@hotmail.com>
Discussion: https://www.postgresql.org/message-id/flat/CALDaNm21RwsDzs4xj14ApteAF7auyyomHNnp+NEL-sH8m-jMvQ@mail.gmail.com
2021-06-25 09:57:02 +02:00
Peter Eisentraut a60c4c5c1a Remove redundant variable pageSize in gistinitpage
In gistinitpage, pageSize variable looks redundant, instead just
pass BLCKSZ. This will be consistent with its peers BloomInitPage,
brin_page_init and SpGistInitPage.

Author: Bharath Rupireddy <bharath.rupireddy@enterprisedb.com>
Discussion: https://www.postgresql.org/message-id/flat/CALj2ACWj=V1k5591eeZK2sOg2FYuBUp6azFO8tMkBtGfXf8PMQ@mail.gmail.com
2021-06-25 07:55:34 +02:00
Michael Paquier 87b2124dfa Add more debugging information with log checks in TAP tests of pgbench
fairywren is not happy with the pattern checks introduced by c13585f.
I am not sure if this outlines a bug in pgbench or if the regex patterns
used in the tests are too restrictive for this buildfarm member's
environment.  This adds more debugging information to show the log
entries that do not match with the expected pattern, to help in finding
out what's happening.  That seems like a good addition in the long-term
anyway as that may not be the only issue in this area.

Discussion: https://postgr.es/m/YNUad2HvgW+6eXyo@paquier.xyz
2021-06-25 09:56:44 +09:00
Michael Paquier 797b0fc0b0 doc: Move remove_temp_files_after_crash to section for developer options
The main goal of this option is to allow inspecting temporary files for
debugging purposes, so moving the parameter there is natural.

Oversight in cd91de0.

Reported-by: Justin Pryzby
Author: Euler Taveira
Discussion: https://postgr.es/m/20210612004347.GP16435@telsasoft.com
2021-06-25 08:40:16 +09:00
Thomas Munro 9b4e4cfe66 Prepare for forthcoming LLVM 13 API change.
LLVM 13 (due out in September) has changed the semantics of
LLVMOrcAbsoluteSymbols(), so we need to bump some reference counts to
avoid a double-free that causes crashes and bad query results.

A proactive change seems necessary to avoid having a window of time
where our respective latest releases would interact badly.  It's
possible that the situation could change before then, though.

Thanks to Fabien Coelho for monitoring bleeding edge LLVM and Andres
Freund for tracking down the change.

Back-patch to 11, where the JIT code arrived.

Discussion: https://postgr.es/m/CA%2BhUKGLEy8mgtN7BNp0ooFAjUedDTJj5dME7NxLU-m91b85siA%40mail.gmail.com
2021-06-25 11:28:20 +12:00
Michael Paquier c13585fe9e Fix pattern matching logic for logs in TAP tests of pgbench
The logic checking for the format of per-thread logs used grep() with
directly "$re", which would cause the test to consider all the logs as
a match without caring about their format at all.  Using "/$re/" makes
grep() perform a regex test, which is what we want here.

While on it, improve some of the tests to be more picky with the
patterns expected and add more comments to describe the tests.

Issue discovered while digging into a separate patch.

Author: Fabien Coelho, Michael Paquier
Discussion: https://postgr.es/m/YNPsPAUoVDCpPOGk@paquier.xyz
Backpatch-through: 11
2021-06-25 06:52:36 +09:00
Heikki Linnakangas 9b8ed0f52b Another fix to relmapper race condition.
In previous commit, I missed that relmap_redo() was also not acquiring the
RelationMappingLock. Thanks to Thomas Munro for pointing that out.

Backpatch-through: 9.6, like previous commit.
Discussion: https://www.postgresql.org/message-id/CA%2BhUKGLev%3DPpOSaL3WRZgOvgk217et%2BbxeJcRr4eR-NttP1F6Q%40mail.gmail.com
2021-06-24 11:19:03 +03:00
Heikki Linnakangas b6d8d2073f Prevent race condition while reading relmapper file.
Contrary to the comment here, POSIX does not guarantee atomicity of a
read(), if another process calls write() concurrently. Or at least Linux
does not. Add locking to load_relmap_file() to avoid the race condition.

Fixes bug #17064. Thanks to Alexander Lakhin for the report and test case.

Backpatch-through: 9.6, all supported versions.
Discussion: https://www.postgresql.org/message-id/17064-bb0d7904ef72add3@postgresql.org
2021-06-24 10:45:23 +03:00
Tom Lane a443c1b2d6 Allow non-quoted identifiers as isolation test session/step names.
For no obvious reason, isolationtester has always insisted that
session and step names be written with double quotes.  This is
fairly tedious and does little for test readability, especially
since the names that people actually choose almost always look
like normal identifiers.  Hence, let's tweak the lexer to allow
SQL-like identifiers not only double-quoted strings.

(They're SQL-like, not exactly SQL, because I didn't add any
case-folding logic.  Also there's no provision for U&"..." names,
not that anyone's likely to care.)

There is one incompatibility introduced by this change: if you write
"foo""bar" with no space, that used to be taken as two identifiers,
but now it's just one identifier with an embedded quote mark.

I converted all the src/test/isolation/ specfiles to remove
unnecessary double quotes, but stopped there because my
eyes were glazing over already.

Like 741d7f104, back-patch to all supported branches, so that this
isn't a stumbling block for back-patching isolation test changes.

Discussion: https://postgr.es/m/759113.1623861959@sss.pgh.pa.us
2021-06-23 18:41:39 -04:00
Tom Lane 126cdaf47a Don't assume GSSAPI result strings are null-terminated.
Our uses of gss_display_status() and gss_display_name() assumed
that the gss_buffer_desc strings returned by those functions are
null-terminated.  It appears that they generally are, given the
lack of field complaints up to now.  However, the available
documentation does not promise this, and some man pages
for gss_display_status() show examples that rely on the
gss_buffer_desc.length field instead of expecting null
termination.  Also, we now have a report that on some
implementations, clang's address sanitizer is of the opinion
that the byte after the specified length is undefined.

Hence, change the code to rely on the length field instead.

This might well be cosmetic rather than fixing any real bug, but
it's hard to be sure, so back-patch to all supported branches.
While here, also back-patch the v12 changes that made pg_GSS_error
deal honestly with multiple messages available from
gss_display_status.

Per report from Sudheer H R.

Discussion: https://postgr.es/m/5372B6D4-8276-42C0-B8FB-BD0918826FC3@tekenlight.com
2021-06-23 14:01:32 -04:00
Tom Lane 4a054069a3 Improve display of query results in isolation tests.
Previously, isolationtester displayed SQL query results using some
ad-hoc code that clearly hadn't had much effort expended on it.
Field values longer than 14 characters weren't separated from
the next field, and usually caused misalignment of the columns
too.  Also there was no visual separation of a query's result
from subsequent isolationtester output.  This made test result
files confusing and hard to read.

To improve matters, let's use libpq's PQprint() function.  Although
that's long since unused by psql, it's still plenty good enough
for the purpose here.

Like 741d7f104, back-patch to all supported branches, so that this
isn't a stumbling block for back-patching isolation test changes.

Discussion: https://postgr.es/m/582362.1623798221@sss.pgh.pa.us
2021-06-23 11:13:00 -04:00
Alvaro Herrera 24043c27b4
Add test case for obsoleting slot with active walsender, take 2
The code to signal a running walsender when its reserved WAL size grows
too large is completely uncovered before this commit; this adds coverage
for that case.

This test involves sending SIGSTOP to walsender and walreceiver, then
advancing enough WAL for a checkpoint to trigger, then sending SIGCONT.

There's no precedent for STOP signalling in Perl tests, and my reading
of relevant manpages says it's likely to fail on Windows.  Because of
this, this test is always skipped on that platform.

This version fixes a couple of rarely hit race conditions in the
previous attempt 09126984a263; most notably, both LOG string searches
are loops, not just the second one; we acquire the start-of-log position
before STOP-signalling; and reference the correct process name in the
test description.  All per Tom Lane.

Author: Álvaro Herrera <alvherre@alvh.no-ip.org>
Discussion: https://postgr.es/m/202106102202.mjw4huiix7lo@alvherre.pgsql
2021-06-23 09:53:18 -04:00
Tom Lane 741d7f1047 Use annotations to reduce instability of isolation-test results.
We've long contended with isolation test results that aren't entirely
stable.  Some test scripts insert long delays to try to force stable
results, which is not terribly desirable; but other erratic failure
modes remain, causing unrepeatable buildfarm failures.  I've spent a
fair amount of time trying to solve this by improving the server-side
support code, without much success: that way is fundamentally unable
to cope with diffs that stem from chance ordering of arrival of
messages from different server processes.

We can improve matters on the client side, however, by annotating
the test scripts themselves to show the desired reporting order
of events that might occur in different orders.  This patch adds
three types of annotations to deal with (a) test steps that might or
might not complete their waits before the isolationtester can see them
waiting; (b) test steps in different sessions that can legitimately
complete in either order; and (c) NOTIFY messages that might arrive
before or after the completion of a step in another session.  We might
need more annotation types later, but this seems to be enough to deal
with the instabilities we've seen in the buildfarm.  It also lets us
get rid of all the long delays that were previously used, cutting more
than a minute off the runtime of the isolation tests.

Back-patch to all supported branches, because the buildfarm
instabilities affect all the branches, and because it seems desirable
to keep isolationtester's capabilities the same across all branches
to simplify possible future back-patching of tests.

Discussion: https://postgr.es/m/327948.1623725828@sss.pgh.pa.us
2021-06-22 21:43:12 -04:00
Tom Lane d102aafb62 Restore the portal-level snapshot for simple expressions, too.
Commits 84f5c2908 et al missed the need to cover plpgsql's "simple
expression" code path.  If the first thing we execute after a
COMMIT/ROLLBACK is one of those, rather than a full-fledged SPI command,
we must explicitly do EnsurePortalSnapshotExists() to make sure we have
an outer snapshot.  Note that it wouldn't be good enough to just push a
snapshot for the duration of the expression execution: what comes back
might be toasted, so we'd better have a snapshot protecting it.

The test case demonstrating this fact cheats a bit by marking a SQL
function immutable even though it fetches from a table.  That's
nothing that users haven't been seen to do, though.

Per report from Jim Nasby.  Back-patch to v11, like the previous fix.

Discussion: https://postgr.es/m/378885e4-f85f-fc28-6c91-c4d1c080bf26@amazon.com
2021-06-22 17:48:39 -04:00
Peter Geoghegan 8e638845ff Add list of ignorable pgindent commits for git-blame.
Add a .git-blame-ignore-revs file with a list of pgindent, pgperlyidy,
and reformat-dat-files commit hashes.  Postgres hackers that configure
git to use the ignore file will get git-blame output that avoids
attributing line changes to the ignored indent commits.  This makes
git-blame output much easier to work with in practice.

Author: Peter Geoghegan <pg@bowt.ie>
Reviewed-By: Tom Lane <tgl@sss.pgh.pa.us>
Discussion: https://postgr.es/m/CAH2-Wz=cVh3GHTP6SdLU-Gnmt2zRdF8vZkcrFdSzXQ=WhbWm9Q@mail.gmail.com
2021-06-22 09:06:32 -07:00
Andres Freund 5a1e1d8302 Use correct horizon when vacuuming catalog relations.
In dc7420c2c9 I (Andres) accidentally used
RelationIsAccessibleInLogicalDecoding() as the sole condition to use the
non-shared catalog horizon in GetOldestNonRemovableTransactionId(). That is
incorrect, as RelationIsAccessibleInLogicalDecoding() checks whether wal_level
is logical.

The correct check, as done e.g. in GlobalVisTestFor(), is to check
IsCatalogRelation() and RelationIsAccessibleInLogicalDecoding().

The observed misbehavior of this bug was that there could be an endless loop
in lazy_scan_prune(), because the horizons used in heap_page_prune() and the
individual tuple liveliness checks did not match. Likely there are other
potential consequences as well.

A later commit will unify the determination which horizon has to be used, and
add additional assertions to make it easier to catch a bug like this.

Reported-By: Justin Pryzby <pryzby@telsasoft.com>
Diagnosed-By: Matthias van de Meent <boekewurm+postgres@gmail.com>
Author: Matthias van de Meent <boekewurm+postgres@gmail.com>
Discussion: https://postgr.es/m/CAEze2Wg32Y9+WJfw=aofkRx1ZRFt_Ev6bNPc4PSaz7PjSFtZgQ@mail.gmail.com
2021-06-21 05:13:46 -07:00
David Rowley 8d29d45d9b Fix assert failure in expand_grouping_sets
linitial_node() fails in assert enabled builds if the given pointer is
not of the specified type.  Here the type is IntList.  The code thought
it should be expecting List, but it was wrong.

In the existing tests which run this code the initial list element is
always NIL.  Since linitial_node() allows NULL, we didn't trigger any
assert failures in the existing regression tests.

There is still some discussion as to whether we need a few more tests in
this area, but for now, since beta2 is looming, fix the bug first.

Bug: #17067
Discussion: https://postgr.es/m/17067-665d50fa321f79e0@postgresql.org
Reported-by: Yaoguang Chen
2021-06-21 23:11:23 +12:00
Peter Eisentraut a7bb0ce58f Translation updates
Source-Git-URL: git://git.postgresql.org/git/pgtranslation/messages.git
Source-Git-Hash: 70796ae860c444c764bb591c885f22cac1c168ec
2021-06-21 12:33:50 +02:00
Peter Geoghegan e8f201ab82 Remove overzealous VACUUM failsafe assertions.
The failsafe can trigger when index processing is already disabled.
This can happen when VACUUM's INDEX_CLEANUP parameter is "off" and the
failsafe happens to trigger.  Remove assertions that assume that index
processing is directly tied to the failsafe.

Oversight in commit c242baa4, which made it possible for the failsafe to
trigger in a two-pass strategy VACUUM that has yet to make its first
call to lazy_vacuum_all_indexes().
2021-06-20 18:14:00 -07:00
Alvaro Herrera 9679517681
Revert "Add test case for obsoleting slot with active walsender"
This reverts commit 09126984a263; the test case added there failed once
in circumstances that remain mysterious.  It seems better to remove the
test for now so that 14beta2 doesn't have random failures built in.
2021-06-20 12:28:08 -04:00
Tom Lane 6991e774e0 Provide feature-test macros for libpq features added in v14.
We had a request to provide a way to test at compile time for the
availability of the new pipeline features.  More generally, it
seems like a good idea to provide a way to test via #ifdef for
all new libpq API features.  People have been using the version
from pg_config.h for that; but that's more likely to represent the
server version than the libpq version, in the increasingly-common
scenario where they're different.  It's safer if libpq-fe.h itself
is the source of truth about what features it offers.

Hence, establish a policy that starting in v14 we'll add a suitable
feature-is-present macro to libpq-fe.h when we add new API there.
(There doesn't seem to be much point in applying this policy
retroactively, but it's not too late for v14.)

Tom Lane and Alvaro Herrera, per suggestion from Boris Kolpackov.

Discussion: https://postgr.es/m/boris.20210617102439@codesynthesis.com
2021-06-19 11:44:39 -04:00
Amit Kapila 2731ce1bd5 Handle no replica identity index case in RelationGetIdentityKeyBitmap.
Commit e7eea52b2d has introduced a new function
RelationGetIdentityKeyBitmap which omits to handle the case where there is
no replica identity index on a relation.

Author: Mark Dilger
Reviewed-by: Takamichi Osumi, Amit Kapila
Discussion: https://www.postgresql.org/message-id/4C99A862-69C8-431F-960A-81B1151F1B89@enterprisedb.com
2021-06-19 11:36:33 +05:30
Peter Geoghegan 3499df0dee Support disabling index bypassing by VACUUM.
Generalize the INDEX_CLEANUP VACUUM parameter (and the corresponding
reloption): make it into a ternary style boolean parameter.  It now
exposes a third option, "auto".  The "auto" option (which is now the
default) enables the "bypass index vacuuming" optimization added by
commit 1e55e7d1.

"VACUUM (INDEX_CLEANUP TRUE)" is redefined to once again make VACUUM
simply do any required index vacuuming, regardless of how few dead
tuples are encountered during the first scan of the target heap relation
(unless there are exactly zero).  This gives users a way of opting out
of the "bypass index vacuuming" optimization, if for whatever reason
that proves necessary.  It is also expected to be used by PostgreSQL
developers as a testing option from time to time.

"VACUUM (INDEX_CLEANUP FALSE)" does the same thing as it always has: it
forcibly disables both index vacuuming and index cleanup.  It's not
expected to be used much in PostgreSQL 14.  The failsafe mechanism added
by commit 1e55e7d1 addresses the same problem in a simpler way.
INDEX_CLEANUP can now be thought of as a testing and compatibility
option.

Author: Peter Geoghegan <pg@bowt.ie>
Reviewed-By: Masahiko Sawada <sawada.mshk@gmail.com>
Reviewed-By: Justin Pryzby <pryzby@telsasoft.com>
Discussion: https://postgr.es/m/CAH2-WznrBoCST4_Gxh_G9hA8NzGUbeBGnOUC8FcXcrhqsv6OHQ@mail.gmail.com
2021-06-18 20:04:07 -07:00
Alvaro Herrera 09126984a2
Add test case for obsoleting slot with active walsender
The code to signal a running walsender when its reserved WAL size grows
too large is completely uncovered before this commit; this adds coverage
for that case.

This test involves sending SIGSTOP to walsender and walreceiver and
running a checkpoint while advancing WAL, then sending SIGCONT.  There's
no precedent for this coding in Perl tests, and my reading of relevant
manpages says it's likely to fail on Windows.  Because of this, this
test is always skipped on that platform.

Author: Álvaro Herrera <alvherre@alvh.no-ip.org>
Discussion: https://postgr.es/m/202106102202.mjw4huiix7lo@alvherre.pgsql
2021-06-18 18:42:00 -04:00
Tom Lane d21fca0843 Fix misbehavior of DROP OWNED BY with duplicate polroles entries.
Ordinarily, a pg_policy.polroles array wouldn't list the same role
more than once; but CREATE POLICY does not prevent that.  If we
perform DROP OWNED BY on a role that is listed more than once,
RemoveRoleFromObjectPolicy either suffered an assertion failure
or encountered a tuple-updated-by-self error.  Rewrite it to cope
correctly with duplicate entries, and add a CommandCounterIncrement
call to prevent the other problem.

Per discussion, there's other cleanup that ought to happen here,
but this seems like the minimum essential fix.

Per bug #17062 from Alexander Lakhin.  It's been broken all along,
so back-patch to all supported branches.

Discussion: https://postgr.es/m/17062-11f471ae3199ca23@postgresql.org
2021-06-18 18:00:09 -04:00
Tom Lane 84bee96109 Improve version reporting in pgbench.
Commit 547f04e73 caused pgbench to start printing its version number,
which seems like a fine idea, but it needs a bit more work:
* Print the server version number too, when different.
* Print the PG_VERSION string, not some reconstructed approximation.

This patch copies psql's well-tested code for the same purpose.

Discussion: https://postgr.es/m/1226654.1624036821@sss.pgh.pa.us
2021-06-18 17:05:23 -04:00
Tom Lane 7c337b6b52 Centralize the logic for protective copying of utility statements.
In the "simple Query" code path, it's fine for parse analysis or
execution of a utility statement to scribble on the statement's node
tree, since that'll just be thrown away afterwards.  However it's
not fine if the node tree is in the plan cache, as then it'd be
corrupted for subsequent executions.  Up to now we've dealt with
that by having individual utility-statement functions apply
copyObject() if they were going to modify the tree.  But that's
prone to errors of omission.  Bug #17053 from Charles Samborski
shows that CREATE/ALTER DOMAIN didn't get this memo, and can
crash if executed repeatedly from plan cache.

In the back branches, we'll just apply a narrow band-aid for that,
but in HEAD it seems prudent to have a more principled fix that
will close off the possibility of other similar bugs in future.
Hence, let's hoist the responsibility for doing copyObject up into
ProcessUtility from its children, thus ensuring that it happens for
all utility statement types.

Also, modify ProcessUtility's API so that its callers can tell it
whether a copy step is necessary.  It turns out that in all cases,
the immediate caller knows whether the node tree is transient, so
this doesn't involve a huge amount of code thrashing.  In this way,
while we lose a little bit in the execute-from-cache code path due
to sometimes copying node trees that wouldn't be mutated anyway,
we gain something in the simple-Query code path by not copying
throwaway node trees.  Statements that are complex enough to be
expensive to copy are almost certainly ones that would have to be
copied anyway, so the loss in the cache code path shouldn't be much.

(Note that this whole problem applies only to utility statements.
Optimizable statements don't have the issue because we long ago made
the executor treat Plan trees as read-only.  Perhaps someday we will
make utility statement execution act likewise, but I'm not holding
my breath.)

Discussion: https://postgr.es/m/931771.1623893989@sss.pgh.pa.us
Discussion: https://postgr.es/m/17053-3ca3f501bbc212b4@postgresql.org
2021-06-18 11:22:58 -04:00
Andrew Dunstan 0a4efdc7eb
Don't set a fast default for anything but a plain table
The fast default code added in Release 11 omitted to check that the
table a fast default was being added to was a plain table. Thus one
could be added to a foreign table, which predicably blows up. Here we
perform that check.

In addition, on the back branches, since some of these might have
escaped into the wild, if we encounter a missing value for
an attribute of something other than a plain table we ignore it.

Fixes bug #17056

Backpatch to release 11,

Reviewed by: Andres Freund, Álvaro Herrera and Tom Lane
2021-06-18 06:51:12 -04:00
Fujii Masao 981524d2e3 Make archiver process handle barrier events.
Commit d75288fb27 made WAL archiver process an auxiliary process.
An auxiliary process needs to handle barrier events but the commit
forgot to make archiver process do that.

Reported-by: Thomas Munro
Author: Fujii Masao
Reviewed-by: Thomas Munro
Discussion: https://postgr.es/m/CA+hUKGLah2w1pWKHonZP_+EQw69=q56AHYwCgEN8GDzsRG_Hgw@mail.gmail.com
2021-06-18 17:57:09 +09:00
Heikki Linnakangas d24c5658a8 Tidy up GetMultiXactIdMembers()'s behavior on error
One of the error paths left *members uninitialized. That's not a live
bug, because most callers don't look at *members when the function
returns -1, but let's be tidy. One caller, in heap_lock_tuple(), does
"if (members != NULL) pfree(members)", but AFAICS it never passes an
invalid 'multi' value so it should not reach that error case.

The callers are also a bit inconsistent in their expectations.
heap_lock_tuple() pfrees the 'members' array if it's not-NULL, others
pfree() it if "nmembers >= 0", and others if "nmembers > 0". That's
not a live bug either, because the function should never return 0, but
add an Assert for that to make it more clear. I left the callers alone
for now.

I also moved the line where we set *nmembers. It wasn't wrong before,
but I like to do that right next to the 'return' statement, to make it
clear that it's always set on return.

Also remove one unreachable return statement after ereport(ERROR), for
brevity and for consistency with the similar if-block right after it.

Author: Greg Nancarrow with the additional changes by me
Backpatch-through: 9.6, all supported versions
2021-06-17 14:50:42 +03:00
Tom Lane 131ea3e908 Fix plancache refcount leak after error in ExecuteQuery.
When stuffing a plan from the plancache into a Portal, one is
not supposed to risk throwing an error between GetCachedPlan and
PortalDefineQuery; if that happens, the plan refcount incremented
by GetCachedPlan will be leaked.  I managed to break this rule
while refactoring code in 9dbf2b7d7.  There is no visible
consequence other than some memory leakage, and since nobody is
very likely to trigger the relevant error conditions many times
in a row, it's not surprising we haven't noticed.  Nonetheless,
it's a bug, so rearrange the order of operations to remove the
hazard.

Noted on the way to looking for a better fix for bug #17053.
This mistake is pretty old, so back-patch to all supported
branches.
2021-06-16 19:30:17 -04:00
Tomas Vondra 99cea49d65 Fix copying data into slots with FDW batching
Commit b676ac443b optimized handling of tuple slots with bulk inserts
into foreign tables, so that the slots are initialized only once and
reused for all batches. The data was however copied into the slots only
after the initialization, inserting duplicate values when the slot gets
reused. Fixed by moving the ExecCopySlot outside the init branch.

The existing postgres_fdw tests failed to catch this due to inserting
data into foreign tables without unique indexes, and then checking only
the number of inserted rows. This adds a new test with both a unique
index and a check of inserted values.

Reported-by: Alexander Pyhalov
Discussion: https://postgr.es/m/7a8cf8d56b3d18e5c0bccd6cd42d04ac%40postgrespro.ru
2021-06-16 23:49:25 +02:00
Tom Lane 6b787d9e32 Improve SQLSTATE reporting in some replication-related code.
I started out with the goal of reporting ERRCODE_CONNECTION_FAILURE
when walrcv_connect() fails, but as I looked around I realized that
whoever wrote this code was of the opinion that errcodes are purely
optional.  That's not my understanding of our project policy.  Hence,
make sure that an errcode is provided in each ereport that (a) is
ERROR or higher level and (b) isn't arguably an internal logic error.
Also fix some very dubious existing errcode assignments.

While this is not per policy, it's also largely cosmetic, since few
of these cases could get reported to applications.  So I don't
feel a need to back-patch.

Discussion: https://postgr.es/m/2189704.1623512522@sss.pgh.pa.us
2021-06-16 11:52:05 -04:00
Heikki Linnakangas d0303bc8d2 Fix outdated comment that talked about seek position of WAL file.
Since commit c24dcd0cfd, we have been using pg_pread() to read the WAL
file, which doesn't change the seek position (unless we fall back to
the implementation in src/port/pread.c). Update comment accordingly.

Backpatch-through: 12, where we started to use pg_pread()
2021-06-16 12:36:15 +03:00
Tom Lane d3c878499c Update another variant expected-result file.
This should have been updated in 533e9c6b0, but it was overlooked.
Given the lack of complaints, I won't bother back-patching.
2021-06-15 16:11:45 -04:00
Tom Lane f6352a0d4e Remove another orphan expected-result file.
aborted-keyrevoke_2.out was apparently needed when it was added (in
commit 0ac5ad513) to handle the case of serializable transaction mode.
However, the output in serializable mode actually matches the regular
aborted-keyrevoke.out file, and AFAICT has done so for a long time.
There's no need to keep dragging this variant along.
2021-06-15 16:09:14 -04:00
Andrew Dunstan 54a5ed2201
Further refinement of stuck_on_old_timeline recovery test
TestLib::perl2host can take a file argument as well as a directory
argument, so that code becomes substantially simpler. Also add comments
on why we're using forward slashes, and why we're setting
PERL_BADLANG=0.

Discussion: https://postgr.es/m/e9947bcd-20ee-027c-f0fe-01f736b7e345@dunslane.net
2021-06-15 15:35:47 -04:00
Alexander Korotkov 817bb0a7d1 Revert 29854ee8d1 due to buildfarm failures
Reported-by: Tom Lane
Discussion: https://postgr.es/m/CAPpHfdvcnw3x7jdV3r52p4%3D5S4WUxBCzcQKB3JukQHoicv1LSQ%40mail.gmail.com
2021-06-15 21:44:40 +03:00
Peter Geoghegan 958cfbcf2d Remove unneeded field from VACUUM state.
Bugfix commit 5fc89376 effectively made the lock_waiter_detected field
from vacuumlazy.c's global state struct into private state owned by
lazy_truncate_heap().  Finish this off by replacing the struct field
with a local variable.
2021-06-15 08:59:36 -07:00
Alexander Korotkov 29854ee8d1 Support for unnest(multirange) and cast multirange as an array of ranges
It has been spotted that multiranges lack of ability to decompose them into
individual ranges.  Subscription and proper expanded object representation
require substantial work, and it's too late for v14.  This commit
provides the implementation of unnest(multirange) and cast multirange as
an array of ranges, which is quite trivial.

unnest(multirange) is defined as a polymorphic procedure.  The catalog
description of the cast underlying procedure is duplicated for each multirange
type because we don't have anyrangearray polymorphic type to use here.

Catversion is bumped.

Reported-by: Jonathan S. Katz
Discussion: https://postgr.es/m/flat/60258efe-bd7e-4886-82e1-196e0cac5433%40postgresql.org
Author: Alexander Korotkov
Reviewed-by: Justin Pryzby, Jonathan S. Katz, Zhihong Yu
2021-06-15 15:59:20 +03:00
Amit Kapila 4daa140a2f Fix decoding of speculative aborts.
During decoding for speculative inserts, we were relying for cleaning
toast hash on confirmation records or next change records. But that
could lead to multiple problems (a) memory leak if there is neither a
confirmation record nor any other record after toast insertion for a
speculative insert in the transaction, (b) error and assertion failures
if the next operation is not an insert/update on the same table.

The fix is to start queuing spec abort change and clean up toast hash
and change record during its processing. Currently, we are queuing the
spec aborts for both toast and main table even though we perform cleanup
while processing the main table's spec abort record. Later, if we have a
way to distinguish between the spec abort record of toast and the main
table, we can avoid queuing the change for spec aborts of toast tables.

Reported-by: Ashutosh Bapat
Author: Dilip Kumar
Reviewed-by: Amit Kapila
Backpatch-through: 9.6, where it was introduced
Discussion: https://postgr.es/m/CAExHW5sPKF-Oovx_qZe4p5oM6Dvof7_P+XgsNAViug15Fm99jA@mail.gmail.com
2021-06-15 08:28:36 +05:30
Tom Lane 0a1e80c5c4 Update variant expected-result file.
This should have been updated in d2d8a229b, but it was overlooked.
According to 31a877f18 which added it, this file is meant to show the
results you get under default_transaction_isolation = serializable.
We've largely lost track of that goal in other isolation tests, but
as long as we've got this one, it should be right.

Noted while fooling about with the isolationtester.
2021-06-14 21:58:26 -04:00
Tom Lane ffbe9dec13 Remove orphaned expected-result file.
This should have been removed in 43e084197, which removed the
corresponding spec file.  Noted while fooling about with the
isolationtester.
2021-06-14 21:28:21 -04:00
Noah Misch 5f1df62a45 Remove pg_wait_for_backend_termination().
It was unable to wait on a backend that had already left the procarray.
Users tolerant of that limitation can poll pg_stat_activity.  Other
users can employ the "timeout" argument of pg_terminate_backend().

Reviewed by Bharath Rupireddy.

Discussion: https://postgr.es/m/20210605013236.GA208701@rfd.leadboat.com
2021-06-14 17:29:37 -07:00
Noah Misch 0aac73e6a2 Copy-edit text for the pg_terminate_backend() "timeout" parameter.
Revert the pg_description entry to its v13 form, since those messages
usually remain shorter and don't discuss individual parameters.  No
catversion bump, since pg_description content does not impair backend
compatibility or application compatibility.

Justin Pryzby

Discussion: https://postgr.es/m/20210612182743.GY16435@telsasoft.com
2021-06-14 17:29:37 -07:00
Alvaro Herrera 33c5099567
Fix logic bug in 1632ea4368
I overlooked that one condition was logically inverted.  The fix is a
little bit more involved than simply negating the condition, to make
the code easier to read.

Fix some outdated comments left by the same commit, while at it.

Author: Masahiko Sawada <sawada.mshk@gmail.com>
Author: Álvaro Herrera <alvherre@alvh.no-ip.org>
Reviewed-by: Amit Kapila <amit.kapila16@gmail.com>
Discussion: https://postgr.es/m/YMRlmB3/lZw8YBH+@paquier.xyz
2021-06-14 16:31:12 -04:00
Michael Paquier 2d689babe3 Improve handling of dropped objects in pg_event_trigger_ddl_commands()
An object found as dropped when digging into the list of objects
returned by pg_event_trigger_ddl_commands() could cause a cache lookup
error, as the calls grabbing for the object address and the type name
would fail if the object was missing.

Those lookup errors could be seen with combinations of ALTER TABLE
sub-commands involving identity columns.  The lookup logic is changed in
this code path to get a behavior similar to any other SQL-callable
function by ignoring objects that are not found, taking advantage of
2a10fdc.  The back-branches are not changed, as they require this commit
that is too invasive for stable branches.

While on it, add test cases to exercise event triggers with identity
columns, and stress more cases with the event ddl_command_end for
relations.

Author: Sven Klemm, Aleksander Alekseev, Michael Paquier
Discussion: https://postgr.es/m/CAMCrgp2R1cEXU53iYKtW6yVEp2_yKUz+z=3-CTrYpPP+xryRtg@mail.gmail.com
2021-06-14 14:57:22 +09:00
Michael Paquier dbab0c07e5 Remove forced toast recompression in VACUUM FULL/CLUSTER
The extra checks added by the recompression of toast data introduced in
bbe0a81 is proving to have a performance impact on VACUUM or CLUSTER
even if no recompression is done.  This is more noticeable with more
toastable columns that contain non-NULL values.

Improvements could be done to make those extra checks less expensive,
but that's not material for 14 at this stage, and we are not sure either
if the code path of VACUUM FULL/CLUSTER is adapted for this job.

Per discussion with several people, including Andres Freund, Robert
Haas, Álvaro Herrera, Tom Lane and myself.

Discussion: https://postgr.es/m/20210527003144.xxqppojoiwurc2iz@alap3.anarazel.de
2021-06-14 09:25:50 +09:00
Tom Lane f807e3410f Work around portability issue with newer versions of mktime().
Recent glibc versions have made mktime() fail if tm_isdst is
inconsistent with the prevailing timezone; in particular it fails for
tm_isdst = 1 when the zone is UTC.  (This seems wildly inconsistent
with the POSIX-mandated treatment of "incorrect" values for the other
fields of struct tm, so if you ask me it's a bug, but I bet they'll
say it's intentional.)  This has been observed to cause cosmetic
problems when pg_restore'ing an archive created in a different
timezone.

To fix, do mktime() using the field values from the archive, and if
that fails try again with tm_isdst = -1.  This will give a result
that's off by the UTC-offset difference from the original zone, but
that was true before, too.  It's not terribly critical since we don't
do anything with the result except possibly print it.  (Someday we
should flush this entire bit of logic and record a standard-format
timestamp in the archive instead.  That's not okay for a back-patched
bug fix, though.)

Also, guard our only other use of mktime() by having initdb's
build_time_t() set tm_isdst = -1 not 0.  This case could only have
an issue in zones that are DST year-round; but I think some do exist,
or could in future.

Per report from Wells Oliver.  Back-patch to all supported
versions, since any of them might need to run with a newer glibc.

Discussion: https://postgr.es/m/CAOC+FBWDhDHO7G-i1_n_hjRzCnUeFO+H-Czi1y10mFhRWpBrew@mail.gmail.com
2021-06-13 14:32:42 -04:00
Andrew Dunstan 9d97c34083
Further tweaks to stuck_on_old_timeline recovery test
Translate path slashes on target directory path. This was confusing old
branches, but is applied to all branches for the sake of uniformity.
Perl is perfectly able to understand paths with forward slashes.

Along the way, restore the previous archive_wait query, for the sake of
uniformity with other tests, per gripe from Tom Lane.
2021-06-13 07:19:34 -04:00
Michael Paquier a9e0b3b08f Ignore more environment variables in pg_regress.c
This is similar to the work done in 8279f68 for TestLib.pm, where
environment variables set may cause unwanted failures if using a
temporary installation with pg_regress.  The list of variables reset is
adjusted in each stable branch depending on what is supported.

Comments are added to remember that the lists in TestLib.pm and
pg_regress.c had better be kept in sync.

Reviewed-by: Álvaro Herrera
Discussion: https://postgr.es/m/YMNR9GYDn+fHlMta@paquier.xyz
Backpatch-through: 9.6
2021-06-13 20:07:39 +09:00
Tom Lane f452aaf7d4 Restore robustness of TAP tests that wait for postmaster restart.
Several TAP tests use poll_query_until() to wait for the postmaster
to restart.  They were checking to see if a trivial query
(e.g. "SELECT 1") succeeds.  However, that's problematic in the wake
of commit 11e9caff8, because now that we feed said query to psql
via stdin, we risk IPC::Run whining about a SIGPIPE failure if psql
quits before reading the query.  Hence, we can't use a nonempty
query in cases where we need to wait for connection failures to
stop happening.

Per the precedent of commits c757a3da0 and 6d41dd045, we can pass
"undef" as the query in such cases to ensure that IPC::Run has
nothing to write.  However, then we have to say that the expected
output is empty, and this exposes a deficiency in poll_query_until:
if psql fails altogether and returns empty stdout, poll_query_until
will treat that as a success!  That's because, contrary to its
documentation, it makes no actual check for psql failure, looking
neither at the exit status nor at stderr.

To fix that, adjust poll_query_until to insist on empty stderr as
well as a stdout match.  (I experimented with checking exit status
instead, but it seems that psql often does exit(1) in cases that we
need to consider successes.  That might be something to fix someday,
but it would be a non-back-patchable behavior change.)

Back-patch to v10.  The test cases needing this exist only as far
back as v11, but it seems wise to keep poll_query_until's behavior
the same in v10, in case we back-patch another such test case in
future.  (9.6 does not currently need this change, because in that
branch poll_query_until can't be told to accept empty stdout as
a success case.)

Per assorted buildfarm failures, mostly on hoverfly.

Discussion: https://postgr.es/m/CAA4eK1+zM6L4QSA1XMvXY_qqWwdUmqkOS1+hWvL8QcYEBGA1Uw@mail.gmail.com
2021-06-12 15:12:10 -04:00
Tom Lane 1250aad425 Ensure pg_filenode_relation(0, 0) returns NULL.
Previously, a zero value for the relfilenode resulted in
a confusing error message about "unexpected duplicate".
This function returns NULL for other invalid relfilenode
values, so zero should be treated likewise.

It's been like this all along, so back-patch to all supported
branches.

Justin Pryzby

Discussion: https://postgr.es/m/20210612023324.GT16435@telsasoft.com
2021-06-12 13:29:24 -04:00
Tom Lane fe6a20ce54 Don't use Asserts to check for violations of replication protocol.
Using an Assert to check the validity of incoming messages is an
extremely poor decision.  In a debug build, it should not be that easy
for a broken or malicious remote client to crash the logrep worker.
The consequences could be even worse in non-debug builds, which will
fail to make such checks at all, leading to who-knows-what misbehavior.
Hence, promote every Assert that could possibly be triggered by wrong
or out-of-order replication messages to a full test-and-ereport.

To avoid bloating the set of messages the translation team has to cope
with, establish a policy that replication protocol violation error
reports don't need to be translated.  Hence, all the new messages here
use errmsg_internal().  A couple of old messages are changed likewise
for consistency.

Along the way, fix some non-idiomatic or outright wrong uses of
hash_search().

Most of these mistakes are new with the "streaming replication"
patch (commit 464824323), but a couple go back a long way.
Back-patch as appropriate.

Discussion: https://postgr.es/m/1719083.1623351052@sss.pgh.pa.us
2021-06-12 12:59:15 -04:00
Andrew Dunstan c3652f976b
Fix new recovery test for use under msys
Commit caba8f0d43 wasn't quite right for msys, as demonstrated by
several buildfarm animals, including jacana and fairywren. We need to
use the msys perl in the archive command, but call it in such a way that
Windows will understand the path. Furthermore, inside the copy script we
need to convert a Windows path to an msys path.
2021-06-12 08:43:54 -04:00
Michael Paquier b56b83aa0d Simplify some code in getObjectTypeDescription()
This routine is designed to never return an empty description or NULL,
providing description fallbacks even if missing objects are accepted,
but it included a code path where this was considered possible.  All the
callers of this routine already consider NULL as not possible, so
change a bit the code to map with the assumptions of the callers, and
add more comments close to the callers of this routine to outline the
behavior expected.

This code is new as of 2a10fdc, so no backpatch is needed.

Discussion: https://postgr.es/m/YMNY6RGPBRCeLmFb@paquier.xyz
2021-06-12 16:29:11 +09:00
Michael Paquier bfd96b7a3d Improve log pattern detection in recently-added TAP tests
ab55d74 has introduced some tests with rows found as missing in logical
replication subscriptions for partitioned tables, relying on a logic
with a lookup of the logs generated, scanning the whole file.  This
commit makes the logic more precise, by scanning the logs only from the
position before the key queries are run to the position where we check
for the logs.  This will reduce the risk of issues with log patterns
overlapping with each other if those tests get more complicated in the
future.

Per discussion with Tom Lane.

Discussion: https://postgr.es/m/YMP+Gx2S8meYYHW4@paquier.xyz
Backpatch-through: 13
2021-06-12 15:29:48 +09:00
Andres Freund d8e950d3ae Improve and cleanup ProcArrayAdd(), ProcArrayRemove().
941697c3c1 changed ProcArrayAdd()/Remove() substantially. As reported by
zhanyi, I missed that due to the introduction of the PGPROC->pgxactoff
ProcArrayRemove() does not need to search for the position in
procArray->pgprocnos anymore - that's pgxactoff. Remove the search loop.

The removal of the search loop reduces assertion coverage a bit - but we can
easily do better than before by adding assertions to other loops over
pgprocnos elements.

Also do a bit of cleanup, mostly by reducing line lengths by introducing local
helper variables and adding newlines.

Author: zhanyi <w@hidva.com>
Author: Andres Freund <andres@anarazel.de>
Discussion: https://postgr.es/m/tencent_5624AA3B116B3D1C31CA9744@qq.com
2021-06-11 21:33:07 -07:00
Alvaro Herrera 5cc1cd5028
Report sort phase progress in parallel btree build
We were already reporting it, but only after the parallel workers were
finished, which is visibly much later than what happens in a serial
build.

With this change we report it when the leader starts its own sort phase
when participating in the build (the normal case).  Now this might
happen a little later than when the workers start their sorting phases,
but a) communicating the actual phase start from workers is likely to be
a hassle, and b) the sort phase start is pretty fuzzy anyway, since
sorting per se is actually initiated by tuplesort.c internally earlier
than tuplesort_performsort() is called.

Backpatch to pg12, where the progress reporting code for CREATE INDEX
went in.

Reported-by: Tomas Vondra <tomas.vondra@enterprisedb.com>
Author: Matthias van de Meent <boekewurm+postgres@gmail.com>
Reviewed-by: Greg Nancarrow <gregn4422@gmail.com>
Reviewed-by: Álvaro Herrera <alvherre@alvh.no-ip.org>
Discussion: https://postgr.es/m/1128176d-1eee-55d4-37ca-e63644422adb
2021-06-11 19:07:32 -04:00
Tom Lane ab55d742eb Fix multiple crasher bugs in partitioned-table replication logic.
apply_handle_tuple_routing(), having detected and reported that
the tuple it needed to update didn't exist, tried to update that
tuple anyway, leading to a null-pointer dereference.

logicalrep_partition_open() failed to ensure that the
LogicalRepPartMapEntry it built for a partition was fully
independent of that for the partition root, leading to
trouble if the root entry was later freed or rebuilt.

Meanwhile, on the publisher's side, pgoutput_change() sometimes
attempted to apply execute_attr_map_tuple() to a NULL tuple.

The first of these was reported by Sergey Bernikov in bug #17055;
I found the other two while developing some test cases for this
sadly under-tested code.

Diagnosis and patch for the first issue by Amit Langote; patches
for the others by me; new test cases by me.  Back-patch to v13
where this logic came in.

Discussion: https://postgr.es/m/17055-9ba800ec8522668b@postgresql.org
2021-06-11 16:12:41 -04:00
Alvaro Herrera 4efcf47053
Add 'Portal Close' message to pipelined PQsendQuery()
Commit acb7e4eb6b added a new implementation for PQsendQuery so that
it works in pipeline mode (by using extended query protocol), but it
behaves differently from the 'Q' message (in simple query protocol) used
by regular implementation: the new one doesn't close the unnamed portal.
Change the new code to have identical behavior to the old.

Reported-by: Yura Sokolov <y.sokolov@postgrespro.ru>
Author: Álvaro Herrera <alvherre@alvh.no-ip.org>
Discussion: https://postgr.es/m/202106072107.d4i55hdscxqj@alvherre.pgsql
2021-06-11 16:05:50 -04:00
Alvaro Herrera 1632ea4368
Return ReplicationSlotAcquire API to its original form
Per 96540f80f833; the awkward API introduced by c655077639 is no
longer needed.

Author: Andres Freund <andres@anarazel.de>
Reviewed-by: Álvaro Herrera <alvherre@alvh.no-ip.org>
Discussion: https://postgr.es/m/20210408020913.zzprrlvqyvlt5cyy@alap3.anarazel.de
2021-06-11 15:48:26 -04:00
Tomas Vondra b676ac443b Optimize creation of slots for FDW bulk inserts
Commit b663a41363 introduced bulk inserts for FDW, but the handling of
tuple slots turned out to be problematic for two reasons. Firstly, the
slots were re-created for each individual batch. Secondly, all slots
referenced the same tuple descriptor - with reasonably small batches
this is not an issue, but with large batches this triggers O(N^2)
behavior in the resource owner code.

These two issues work against each other - to reduce the number of times
a slot has to be created/dropped, larger batches are needed. However,
the larger the batch, the more expensive the resource owner gets. For
practical batch sizes (100 - 1000) this would not be a big problem, as
the benefits (latency savings) greatly exceed the resource owner costs.
But for extremely large batches it might be much worse, possibly even
losing with non-batching mode.

Fixed by initializing tuple slots only once (and reusing them across
batches) and by using a new tuple descriptor copy for each slot.

Discussion: https://postgr.es/m/ebbbcc7d-4286-8c28-0272-61b4753af761%40enterprisedb.com
2021-06-11 20:23:33 +02:00
Alvaro Herrera 96540f80f8
Fix race condition in invalidating obsolete replication slots
The code added to mark replication slots invalid in commit c655077639
had the race condition that a slot can be dropped or advanced
concurrently with checkpointer trying to invalidate it.  Rewrite the
code to close those races.

The changes to ReplicationSlotAcquire's API added with c655077639 are
not necessary anymore.  To avoid an ABI break in released branches, this
commit leaves that unchanged; it'll be changed in a master-only commit
separately.

Backpatch to 13, where this code first appeared.

Reported-by: Andres Freund <andres@anarazel.de>
Author: Andres Freund <andres@anarazel.de>
Author: Álvaro Herrera <alvherre@alvh.no-ip.org>
Discussion: https://postgr.es/m/20210408001037.wfmk6jud36auhfqm@alap3.anarazel.de
2021-06-11 12:16:14 -04:00
Michael Paquier d08237b5b4 Improve psql tab completion for options of subcriptions and publications
The list of options provided by the tab completion was outdated for the
following commands:
- ALTER SUBSCRIPTION
- CREATE SUBSCRIPTION
- ALTER PUBLICATION
- CREATE PUBLICATION

Author: Vignesh C
Reviewed-by: Bharath Rupireddy
Discussion: https://postgr.es/m/CALDaNm18oHDFu6SFCHE=ZbiO153Fx7E-L1MG0YyScbaDV--U+A@mail.gmail.com
2021-06-11 15:46:18 +09:00
Noah Misch 13a1ca160d Change position of field "transformed" in struct CreateStatsStmt.
Resolve the disagreement with nodes/*funcs.c field order in favor of the
latter, which is better-aligned with the IndexStmt field order.  This
field is new in v14.

Discussion: https://postgr.es/m/20210611045546.GA573364@rfd.leadboat.com
2021-06-10 21:56:14 -07:00
Noah Misch d0e750c0ac Rename PQtraceSetFlags() to PQsetTraceFlags().
We have a dozen PQset*() functions.  PQresultSetInstanceData() and this
were the libpq setter functions having a different word order.  Adopt
the majority word order.

Reviewed by Alvaro Herrera and Robert Haas, though this choice of name
was not unanimous.

Discussion: https://postgr.es/m/20210605060555.GA216695@rfd.leadboat.com
2021-06-10 21:56:13 -07:00
David Rowley 04539e73fa Use the correct article for abbreviations
We've accumulated quite a mix of instances of "an SQL" and "a SQL" in the
documents.  It would be good to be a bit more consistent with these.

The most recent version of the SQL standard I looked at seems to prefer
"an SQL".  That seems like a good lead to follow, so here we change all
instances of "a SQL" to become "an SQL".  Most instances correctly use
"an SQL" already, so it also makes sense to use the dominant variation in
order to minimise churn.

Additionally, there were some other abbreviations that needed to be
adjusted. FSM, SSPI, SRF and a few others.  Also fix some pronounceable,
abbreviations to use "a" instead of "an".  For example, "a SASL" instead
of "an SASL".

Here I've only adjusted the documents and error messages.  Many others
still exist in source code comments.  Translator hint comments seem to be
the biggest culprit.  It currently does not seem worth the churn to change
these.

Discussion: https://postgr.es/m/CAApHDvpML27UqFXnrYO1MJddsKVMQoiZisPvsAGhKE_tsKXquw%40mail.gmail.com
2021-06-11 13:38:04 +12:00
Tom Lane e56bce5d43 Reconsider the handling of procedure OUT parameters.
Commit 2453ea142 redefined pg_proc.proargtypes to include the types of
OUT parameters, for procedures only.  While that had some advantages
for implementing the SQL-spec behavior of DROP PROCEDURE, it was pretty
disastrous from a number of other perspectives.  Notably, since the
primary key of pg_proc is name + proargtypes, this made it possible to
have multiple procedures with identical names + input arguments and
differing output argument types.  That would make it impossible to call
any one of the procedures by writing just NULL (or "?", or any other
data-type-free notation) for the output argument(s).  The change also
seems likely to cause grave confusion for client applications that
examine pg_proc and expect the traditional definition of proargtypes.

Hence, revert the definition of proargtypes to what it was, and
undo a number of complications that had been added to support that.

To support the SQL-spec behavior of DROP PROCEDURE, when there are
no argmode markers in the command's parameter list, we perform the
lookup both ways (that is, matching against both proargtypes and
proallargtypes), succeeding if we get just one unique match.
In principle this could result in ambiguous-function failures
that would not happen when using only one of the two rules.
However, overloading of procedure names is thought to be a pretty
rare usage, so this shouldn't cause many problems in practice.
Postgres-specific code such as pg_dump can defend against any
possibility of such failures by being careful to specify argmodes
for all procedure arguments.

This also fixes a few other bugs in the area of CALL statements
with named parameters, and improves the documentation a little.

catversion bump forced because the representation of procedures
with OUT arguments changes.

Discussion: https://postgr.es/m/3742981.1621533210@sss.pgh.pa.us
2021-06-10 17:11:36 -04:00
Tom Lane 3a09d75b4f Rearrange logrep worker's snapshot handling some more.
It turns out that worker.c's code path for TRUNCATE was also
careless about establishing a snapshot while executing user-defined
code, allowing the checks added by commit 84f5c2908 to fail when
a trigger is fired in that context.

We could just wrap Push/PopActiveSnapshot around the truncate call,
but it seems better to establish a policy of holding a snapshot
throughout execution of a replication step.  To help with that and
possible future requirements, replace the previous ensure_transaction
calls with pairs of begin/end_replication_step calls.

Per report from Mark Dilger.  Back-patch to v11, like the previous
changes.

Discussion: https://postgr.es/m/B4A3AF82-79ED-4F4C-A4E5-CD2622098972@enterprisedb.com
2021-06-10 12:27:27 -04:00
Tom Lane bb4aed46a5 Shut down EvalPlanQual machinery when LockRows node reaches the end.
Previously, we left the EPQ sub-executor alone until ExecEndLockRows.
This caused any buffer pins or other resources that it might hold to
remain held until ExecutorEnd, which in some code paths means that
they are held till the Portal is closed.  That can cause user-visible
problems, such as blocking VACUUM; and it's unlike the behavior of
ordinary table-scanning nodes, which will have released all buffer
pins by the time they return an EOF indication.

We can make LockRows work more like other plan nodes by calling
EvalPlanQualEnd just before returning NULL.  We still need to call it
in ExecEndLockRows in case the node was not run to completion, but in
the normal case the second call does nothing and costs little.

Per report from Yura Sokolov.  In principle this is a longstanding
bug, but in view of the lack of other complaints and the low severity
of the consequences, I chose not to back-patch.

Discussion: https://postgr.es/m/4aa370cb91ecf2f9885d98b80ad1109c@postgrespro.ru
2021-06-10 11:15:13 -04:00
Tom Lane 9bb5eecce6 Avoid ECPG test failures in some GSS-capable environments.
Buildfarm member hamerkop has been reporting that two cases in
connect/test5.pgc show different error messages than the test expects,
because since commit ffa2e4670 libpq's connection failure messages
are exposing the fact that a GSS-encrypted connection was attempted
and failed.  That's pretty interesting information in itself, and
I certainly don't wish to shoot the messenger, but we need to do
something to stabilize the ECPG results.

For the second of these two failure cases, we can add the
gssencmode=disable option to prevent the discrepancy.  However,
that solution is problematic for the first failure, because the only
unique thing about that case is that it's testing a completely-omitted
connection target; there's noplace to add the option without defeating
the point of the test case.  After some thrashing around with
alternative fixes that turned out to have undesirable side-effects,
the most workable answer is just to give up and remove that test case.
Perhaps we can revert this later, if we figure out why the GSS code
is misbehaving in hamerkop's environment.

Thanks to Michael Paquier for exploration of alternatives.

Discussion: https://postgr.es/m/YLRZH6CWs9N6Pusy@paquier.xyz
2021-06-10 10:45:42 -04:00
Peter Eisentraut b29fa951ec Add some const decorations
One of these functions is new in PostgreSQL 14; might as well start it
out right.
2021-06-10 16:21:48 +02:00
Robert Haas 4dcb1d087a Adjust new test case to set wal_keep_size.
Per buildfarm member conchuela and Kyotaro Horiguchi, it's possible
for the WAL segment that the cascading standby needs to be removed
too quickly. Hopefully this will prevent that.

Kyotaro Horiguchi

Discussion: http://postgr.es/m/20210610.101240.1270925505780628275.horikyota.ntt@gmail.com
2021-06-10 09:46:08 -04:00
David Rowley 55ba5973d9 Fix an asssortment of typos in brin_minmax_multi.c and mcv.c
Discussion: https://postgr.es/m/CAApHDvrbyJNOPBws4RUhXghZ7+TBjtdO-rznTsqZECuowNorXg@mail.gmail.com
2021-06-10 20:13:44 +12:00
Robert Haas caba8f0d43 Fix corner case failure of new standby to follow new primary.
This only happens if (1) the new standby has no WAL available locally,
(2) the new standby is starting from the old timeline, (3) the promotion
happened in the WAL segment from which the new standby is starting,
(4) the timeline history file for the new timeline is available from
the archive but the WAL files for are not (i.e. this is a race),
(5) the WAL files for the new timeline are available via streaming,
and (6) recovery_target_timeline='latest'.

Commit ee994272ca introduced this
logic and was an improvement over the previous code, but it mishandled
this case. If recovery_target_timeline='latest' and restore_command is
set, validateRecoveryParameters() can change recoveryTargetTLI to be
different from receiveTLI. If streaming is then tried afterward,
expectedTLEs gets initialized with the history of the wrong timeline.
It's supposed to be a list of entries explaining how to get to the
target timeline, but in this case it ends up with a list of entries
explaining how to get to the new standby's original timeline, which
isn't right.

Dilip Kumar and Robert Haas, reviewed by Kyotaro Horiguchi.

Discussion: http://postgr.es/m/CAFiTN-sE-jr=LB8jQuxeqikd-Ux+jHiXyh4YDiZMPedgQKup0g@mail.gmail.com
2021-06-09 16:17:00 -04:00
Michael Paquier 845cad4d51 Fix inconsistencies in psql --help=commands
The set of subcommands supported by \dAp, \do and \dy was described
incorrectly in psql's --help.  The documentation was already consistent
with the code.

Reported-by: inoas, from IRC
Author: Matthijs van der Vleuten
Reviewed-by: Neil Chen
Discussion: https://postgr.es/m/6a984e24-2171-4039-9050-92d55e7b23fe@www.fastmail.com
Backpatch-through: 9.6
2021-06-09 16:24:52 +09:00
Tom Lane be90098907 Force NO SCROLL for plpgsql's implicit cursors.
Further thought about bug #17050 suggests that it's a good idea
to use CURSOR_OPT_NO_SCROLL for the implicit cursor opened by
a plpgsql FOR-over-query loop.  This ensures that, if somebody
commits inside the loop, PersistHoldablePortal won't try to
rewind and re-read the cursor.  While we'd have selected NO_SCROLL
anyway if FOR UPDATE/SHARE appears in the query, there are other
hazards with volatile functions; and in any case, it's silly to
expend effort storing rows that we know for certain won't be needed.

(While here, improve the comment in exec_run_select, which was a bit
confused about the rationale for when we can use parallel mode.
Cursor operations aren't a hazard for nameless portals.)

This wasn't an issue until v11, which introduced the possibility
of persisting such cursors.  Hence, back-patch to v11.

Per bug #17050 from Алексей Булгаков.

Discussion: https://postgr.es/m/17050-f77aa827dc85247c@postgresql.org
2021-06-08 18:40:06 -04:00
Tom Lane ba2c6d6cec Avoid misbehavior when persisting a non-stable cursor.
PersistHoldablePortal has long assumed that it should store the
entire output of the query-to-be-persisted, which requires rewinding
and re-reading the output.  This is problematic if the query is not
stable: we might get different row contents, or even a different
number of rows, which'd confuse the cursor state mightily.

In the case where the cursor is NO SCROLL, this is very easy to
solve: just store the remaining query output, without any rewinding,
and tweak the portal's cursor state to match.  Aside from removing
the semantic problem, this could be significantly more efficient
than storing the whole output.

If the cursor is scrollable, there's not much we can do, but it
was already the case that scrolling a volatile query's result was
pretty unsafe.  We can just document more clearly that getting
correct results from that is not guaranteed.

There are already prohibitions in place on using SCROLL with
FOR UPDATE/SHARE, which is one way for a SELECT query to have
non-stable results.  We could imagine prohibiting SCROLL when
the query contains volatile functions, but that would be
expensive to enforce.  Moreover, it could break applications
that work just fine, if they have functions that are in fact
stable but the user neglected to mark them so.  So settle for
documenting the hazard.

While this problem has existed in some guise for a long time,
it got a lot worse in v11, which introduced the possibility
of persisting plpgsql cursors (perhaps implicit ones) even
when they violate the rules for what can be marked WITH HOLD.
Hence, I've chosen to back-patch to v11 but not further.

Per bug #17050 from Алексей Булгаков.

Discussion: https://postgr.es/m/17050-f77aa827dc85247c@postgresql.org
2021-06-08 17:50:29 -04:00
Tomas Vondra cb92703384 Adjust batch size in postgres_fdw to not use too many parameters
The FE/BE protocol identifies parameters with an Int16 index, which
limits the maximum number of parameters per query to 65535. With
batching added to postges_fdw this limit is much easier to hit, as
the whole batch is essentially a single query, making this error much
easier to hit.

The failures are a bit unpredictable, because it also depends on the
number of columns in the query. So instead of just failing, this patch
tweaks the batch_size to not exceed the maximum number of parameters.

Reported-by: Hou Zhijie <houzj.fnst@cn.fujitsu.com>
Reviewed-by: Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com>
Discussion: https://postgr.es/m/OS0PR01MB571603973C0AC2874AD6BF2594299%40OS0PR01MB5716.jpnprd01.prod.outlook.com
2021-06-08 20:28:31 +02:00
Tomas Vondra d1f0aa7696 Fix pg_visibility regression failure with CLOBBER_CACHE_ALWAYS
Commit 8e03eb92e9 reverted a bit too much code, reintroducing one of the
issues fixed by 39b66a91bd - a page might have been left partially empty
after relcache invalidation.

Reported-By: Tom Lane
Author: Masahiko Sawada
Discussion: https://postgr.es/m/822752.1623032114@sss.pgh.pa.us
Discussion: https://postgr.es/m/CAD21AoA%3D%3Df2VSw3c-Cp_y%3DWLKHMKc1D6s7g3YWsCOvgaYPpJcg%40mail.gmail.com
2021-06-08 19:33:11 +02:00
Tom Lane bfeede9fa4 Don't crash on empty statements in SQL-standard function bodies.
gram.y should discard NULL pointers (empty statements) when
assembling a routine_body_stmt_list, as it does for other
sorts of statement lists.

Julien Rouhaud and Tom Lane, per report from Noah Misch.

Discussion: https://postgr.es/m/20210606044418.GA297923@rfd.leadboat.com
2021-06-08 11:59:34 -04:00
Peter Eisentraut 37e1cce4dd libpq: Fix SNI host handling
Fix handling of NULL host name (possibly by using hostaddr).  It
previously crashed.  Also, we should look at connhost, not pghost, to
handle multi-host specifications.

Also remove an unnecessary SSL_CTX_free().

Reported-by: Jacob Champion <pchampion@vmware.com>
Reviewed-by: Michael Paquier <michael@paquier.xyz>
Discussion: https://www.postgresql.org/message-id/504c276ab6eee000bb23d571ea9b0ced4250774e.camel@vmware.com
2021-06-08 16:01:05 +02:00
Michael Paquier 4e47b02834 Reorder superuser check in pg_log_backend_memory_contexts()
The use of this function is limited to superusers and the code includes
a hardcoded check for that.  However, the code would look for the PGPROC
entry to signal for the memory dump before checking if the user is a
superuser or not, which does not make sense if we know that an error
will be returned.  Note that the code would let one know if a process
was a PostgreSQL process or not even for non-authorized users, which is
not the case now, but this avoids taking ProcArrayLock that will most
likely finish by being unnecessary.

Thanks to Julien Rouhaud and Tom Lane for the discussion.

Discussion: https://postgr.es/m/YLxw1uVGIAP5uMPl@paquier.xyz
2021-06-08 08:53:12 +09:00
Peter Eisentraut 3bb309be75 Add _outTidRangePath()
We have outNode() coverage for all path nodes, but this one was
missed when it was added.
2021-06-07 21:32:53 +02:00
Tom Lane 42f94f56bf Fix incautious handling of possibly-miscoded strings in client code.
An incorrectly-encoded multibyte character near the end of a string
could cause various processing loops to run past the string's
terminating NUL, with results ranging from no detectable issue to
a program crash, depending on what happens to be in the following
memory.

This isn't an issue in the server, because we take care to verify
the encoding of strings before doing any interesting processing
on them.  However, that lack of care leaked into client-side code
which shouldn't assume that anyone has validated the encoding of
its input.

Although this is certainly a bug worth fixing, the PG security team
elected not to regard it as a security issue, primarily because
any untrusted text should be sanitized by PQescapeLiteral or
the like before being incorporated into a SQL or psql command.
(If an app fails to do so, the same technique can be used to
cause SQL injection, with probably much more dire consequences
than a mere client-program crash.)  Those functions were already
made proof against this class of problem, cf CVE-2006-2313.

To fix, invent PQmblenBounded() which is like PQmblen() except it
won't return more than the number of bytes remaining in the string.
In HEAD we can make this a new libpq function, as PQmblen() is.
It seems imprudent to change libpq's API in stable branches though,
so in the back branches define PQmblenBounded as a macro in the files
that need it.  (Note that just changing PQmblen's behavior would not
be a good idea; notably, it would completely break the escaping
functions' defense against this exact problem.  So we just want a
version for those callers that don't have any better way of handling
this issue.)

Per private report from houjingyi.  Back-patch to all supported branches.
2021-06-07 14:15:25 -04:00
Michael Paquier 68a6d8a870 Fix portability issue in test indirect_toast
When run on a server using default_toast_compression set to LZ4, this
test would fail because of a consistency issue with the order of the
tuples treated.  LZ4 causes one tuple to be stored inline instead of
getting externalized.  As the goal of this test is to check after data
stored externally, stick to pglz as the compression algorithm used, so
as all data of this test is stored the way it should.

Analyzed-by: Dilip Kumar
Discussion: https://postgr.es/m/YLrDWxJgM8WWMoCg@paquier.xyz
2021-06-07 18:12:29 +09:00
Amit Kapila be57f21650 Remove two_phase variable from CreateReplicationSlotCmd struct.
Commit 19890a064e added the option to enable two_phase commits via
pg_create_logical_replication_slot but didn't extend the support of same
in replication protocol. However, by mistake, it added the two_phase
variable in CreateReplicationSlotCmd which is required only when we extend
the replication protocol.

Reported-by: Jeff Davis
Author: Ajin Cherian
Reviewed-by: Amit Kapila
Discussion: https://postgr.es/m/64b9f783c6e125f18f88fbc0c0234e34e71d8639.camel@j-davis.com
2021-06-07 09:32:06 +05:30
Etsuro Fujita f3baaf28a6 Fix rescanning of async-aware Append nodes.
In cases where run-time pruning isn't required, the synchronous and
asynchronous subplans for an async-aware Append node determined using
classify_matching_subplans() should be re-used when rescanning the node,
but the previous code re-determined them using that function repeatedly
each time when rescanning the node, leading to incorrect results in a
normal build and an Assert failure in an Assert-enabled build as that
function doesn't assume that it's called repeatedly in such cases.  Fix
the code as mentioned above.

My oversight in commit 27e1f1456.

While at it, initialize async-related pointers/variables to NULL/zero
explicitly in ExecInitAppend() and ExecReScanAppend(), just to be sure.
(The variables would have been set to zero before we get to the latter
function, but let's do so.)

Reviewed-by: Kyotaro Horiguchi
Discussion: https://postgr.es/m/CAPmGK16Q4B2_KY%2BJH7rb7wQbw54AUprp7TMekGTd2T1B62yysQ%40mail.gmail.com
2021-06-07 12:45:00 +09:00
Tom Lane a65e9f3f14 Fix inconsistent equalfuncs.c behavior for FuncCall.funcformat.
Other equalfuncs.c checks on CoercionForm fields use
COMPARE_COERCIONFORM_FIELD (which makes them no-ops),
but commit 40c24bfef neglected to make _equalFuncCall
do likewise.  Fix that.

This is only strictly correct if FuncCall.funcformat has
no semantic effect, instead just determining ruleutils.c
display formatting.  40c24bfef added a couple of checks
in parse analysis that could break that rule; but on closer
inspection, they're redundant, so just take them out again.

Per report from Noah Misch.

Discussion: https://postgr.es/m/20210606063331.GC297923@rfd.leadboat.com
2021-06-06 15:46:58 -04:00
Tomas Vondra d57ecebd12 Add transformed flag to nodes/*funcs.c for CREATE STATISTICS
Commit a4d75c86bf added a new flag, tracking if the statement was
processed by transformStatsStmt(), but failed to add this flag to
nodes/*funcs.c.

Catversion bump, due to adding a flag to copy/equal/out functions.

Reported-by: Noah Misch
Discussion: https://postgr.es/m/ad7891d2-e90c-b446-9fe2-7419143847d7%40enterprisedb.com
2021-06-06 20:52:58 +02:00
Noah Misch a2dee328bb Standardize nodes/*funcs.c cosmetics for ForeignScan.resultRelation.
catversion bump due to readfuncs.c field order change.
2021-06-06 00:08:21 -07:00
Peter Eisentraut 4a682d85a1 Fix subtransaction test for Python 3.10
Starting with Python 3.10, the stacktrace looks differently:
  -  PL/Python function "subtransaction_exit_subtransaction_in_with", line 3, in <module>
  -    s.__exit__(None, None, None)
  +  PL/Python function "subtransaction_exit_subtransaction_in_with", line 2, in <module>
  +    with plpy.subtransaction() as s:
Using try/except specifically makes the error look always the same.

(See https://github.com/python/cpython/pull/25719 for the discussion
of this change in Python.)

Author: Honza Horak <hhorak@redhat.com>
Discussion: https://www.postgresql.org/message-id/flat/853083.1620749597%40sss.pgh.pa.us
RHBZ: https://bugzilla.redhat.com/show_bug.cgi?id=1959080
2021-06-05 07:16:34 +02:00
David Rowley 8bdb36aab2 Clean up some questionable usages of DatumGet* macros
This tidies up some questionable coding which made use of
DatumGetPointer() for Datums being passed into functions where the
parameter is expected to be a cstring.  We saw no compiler warnings with
the old code as the Pointer type used in DatumGetPointer() happens to
be a char * rather than a void *.  However, that's no excuse and we should
be using the correct macro for the job.

Here we also make use of OutputFunctionCall() rather than using
FunctionCall1() directly to call the type's output function.
OutputFunctionCall() is the standard way to do this.  It casts the
returned value to a cstring for us.

In passing get rid of a duplicate call to strlen().  Most compilers will
likely optimize away the 2nd call, but there may be some that won't.  In
any case, this just aligns the code to some other nearby code that already
does this.

Discussion: https://postgr.es/m/CAApHDvq1D=ehZ8hey8Hz67N+_Zth0GHO5wiVCfv1YcGPMXJq0A@mail.gmail.com
2021-06-04 22:42:17 +12:00
David Rowley 7fc26d11e3 Adjust locations which have an incorrect copyright year
A few patches committed after ca3b37487 mistakenly forgot to make the
copyright year 2021.  Fix these.

Discussion: https://postgr.es/m/CAApHDvqyLmd9P2oBQYJ=DbrV8QwyPRdmXtCTFYPE08h+ip0UJw@mail.gmail.com
2021-06-04 12:19:50 +12:00
Andrew Dunstan 11e9caff82
In PostgresNode.pm, don't pass SQL to psql on the command line
The Msys shell mangles certain patterns in its command line, so avoid
handing arbitrary SQL to psql on the command line and instead use
IPC::Run's redirection facility for stdin. This pattern is already
mostly whats used, but query_poll_until() was not doing the right thing.

Problem discovered on the buildfarm when a new TAP test failed on msys.
2021-06-03 16:14:06 -04:00
Tom Lane 3590680b85 Fix incorrect permissions on pg_subscription.
The documented intent is for all columns except subconninfo to be
publicly readable.  However, this has been overlooked twice.
subsynccommit has never been readable since it was introduced,
nor has the oid column (which is important for joining).

Given the lack of previous complaints, it's not clear that it's
worth doing anything about this in the back branches.  But there's
still time to fix it inexpensively for v14.

Per report from Israel Barth (via Euler Taveira).

Patch by Euler Taveira, possibly-vain comment updates by me.

Discussion: https://postgr.es/m/b8f7c17c-0041-46b6-acfe-2d1f5a985ab4@www.fastmail.com
2021-06-03 14:54:06 -04:00
Michael Paquier 187682c321 Reduce risks of conflicts in internal queries of REFRESH MATVIEW CONCURRENTLY
The internal SQL queries used by REFRESH MATERIALIZED VIEW CONCURRENTLY
include some aliases for its diff and temporary relations with
rather-generic names: diff, newdata, newdata2 and mv.  Depending on the
queries used for the materialized view, using CONCURRENTLY could lead to
some internal failures if the matview query and those internal aliases
conflict.

Those names have been chosen in 841c29c8.  This commit switches instead
to a naming pattern which is less likely going to cause conflicts, based
on an idea from Thomas Munro, by appending _$ to those aliases.  This is
not perfect as those new names could still conflict, but at least it has
the advantage to keep the code readable and simple while reducing the
likelihood of conflicts to be close to zero.

Reported-by: Mathis Rudolf
Author: Bharath Rupireddy
Reviewed-by: Bernd Helmle, Thomas Munro, Michael Paquier
Discussion: https://postgr.es/m/109c267a-10d2-3c53-b60e-720fcf44d9e8@credativ.de
Backpatch-through: 9.6
2021-06-03 15:28:24 +09:00
Peter Eisentraut cb3cffe694 doc: Group options in pg_amcheck reference page
The previous arrangement was just one big list, and the internal order
was not all consistent either.  Now arrange the options by group and
sort them, the way it's already done in the --help output and one
other reference pages.  Also fix some ordering in the --help output.
2021-06-03 06:55:04 +02:00
David Rowley f736e188ce Standardize usages of appendStringInfo and appendPQExpBuffer
Fix a few places that were using appendStringInfo() when they should have
been using appendStringInfoString().  Also some cases of
appendPQExpBuffer() that would have been better suited to use
appendPQExpBufferChar(), and finally, some places that used
appendPQExpBuffer() when appendPQExpBufferStr() would have suited better.

There are no bugs are being fixed here.  The aim is just to make the code
use the most optimal function for the job.

All the code being changed here is new to PG14.  It makes sense to fix
these before we branch for PG15.  There are a few other places that we
could fix, but those cases are older code so fixing those seems less
worthwhile as it may cause unnecessary back-patching pain in the future.

Author: Hou Zhijie
Discussion: https://postgr.es/m/OS0PR01MB5716732158B1C4142C6FE375943D9@OS0PR01MB5716.jpnprd01.prod.outlook.com
2021-06-03 16:38:03 +12:00
Michael Paquier 8279f68a1b Ignore more environment variables in TAP tests
Various environment variables were not getting reset in the TAP tests,
which would cause failures depending on the tests or the environment
variables involved.  For example, PGSSL{MAX,MIN}PROTOCOLVERSION could
cause failures in the SSL tests.  Even worse, a junk value of
PGCLIENTENCODING makes a server startup fail.  The list of variables
reset is adjusted in each stable branch depending on what is supported.

While on it, simplify a bit the code per a suggestion from Andrew
Dunstan, using a list of variables instead of doing single deletions.

Reviewed-by: Andrew Dunstan, Daniel Gustafsson
Discussion: https://postgr.es/m/YLbjjRpucIeZ78VQ@paquier.xyz
Backpatch-through: 9.6
2021-06-03 11:50:56 +09:00
Tom Lane 2955c2be79 Re-allow custom GUC names that have more than two components.
Commit 3db826bd5 disallowed this case, but it turns out that some
people are depending on it.  Since the core grammar has allowed
it since 3dc37cd8d, it seems like this code should fall in line.

Per bug #17045 from Robert Sosinski.

Discussion: https://postgr.es/m/17045-6a4a9f0d1513f72b@postgresql.org
2021-06-02 18:50:23 -04:00
Tomas Vondra 8e03eb92e9 Revert most of 39b66a91bd
Reverts most of commit 39b66a91bd, which was found to cause significant
regression for REFRESH MATERIALIZED VIEW. This means only rows inserted
by heap_multi_insert will benefit from the optimization, implemented in
commit 7db0cd2145.

Reported-by: Masahiko Sawada
Discussion: https://postgr.es/m/CAD21AoA%3D%3Df2VSw3c-Cp_y%3DWLKHMKc1D6s7g3YWsCOvgaYPpJcg%40mail.gmail.com
2021-06-03 00:13:59 +02:00
Tom Lane 889592344c Fix planner's row-mark code for inheritance from a foreign table.
Commit 428b260f8 broke planning of cases where row marks are needed
(SELECT FOR UPDATE, etc) and one of the query's tables is a foreign
table that has regular table(s) as inheritance children.  We got the
reverse case right, but apparently were thinking that foreign tables
couldn't be inheritance parents.  Not so; so we need to be able to
add a CTID junk column while adding a new child, not only a wholerow
junk column.

Back-patch to v12 where the faulty code came in.

Amit Langote

Discussion: https://postgr.es/m/CA+HiwqEmo3FV1LAQ4TVyS2h1WM=kMkZUmbNuZSCnfHvMcUcPeA@mail.gmail.com
2021-06-02 14:38:14 -04:00
Tom Lane 79c50ca578 Update plannodes.h's comments about PlanRowMark.
The reference here to different physical column numbers in inherited
UPDATE/DELETE plans is obsolete as of 86dc90056; remove it.  Also
rework the text about inheritance cases to make it clearer.
2021-06-02 11:52:35 -04:00
Tom Lane 9e3b3ff266 Teach tab-complete.c about recently-added CREATE TYPE options.
Commit c7aba7c14 missed adding SUBSCRIPT here,
and commit 6df7a9698 missed adding MULTIRANGE_TYPE_NAME.

Haiying Tang and Tom Lane

Discussion: https://postgr.es/m/OS0PR01MB6113F9EDA46FA53BAA5445BDFB3D9@OS0PR01MB6113.jpnprd01.prod.outlook.com
2021-06-02 10:44:16 -04:00
Fujii Masao df466d30c6 Remove unnecessary use of Time::HiRes from 013_crash_restart.pl.
The regression test 013_crash_restart.pl included "use Time::HiRes qw(usleep)",
but the usleep was not used there.

Author: Fujii Masao
Reviewed-by: Kyotaro Horiguchi
Discussion: https://postgr.es/m/63ad1368-18e2-8900-8443-524bdfb1bef5@oss.nttdata.com
2021-06-02 12:20:15 +09:00
Fujii Masao 6bbc5c5e96 Add regression test for recovery pause.
Previously there was no regression test for recovery pause feature.
This commit adds the test that checks

- recovery can be paused or resumed expectedly
- pg_get_wal_replay_pause_state() reports the correct pause state
- the paused state ends and promotion continues if a promotion
   is triggered while recovery is paused

Suggested-by: Michael Paquier
Author: Fujii Masao
Reviewed-by: Kyotaro Horiguchi, Dilip Kumar
Discussion: https://postgr.es/m/YKNirzqM1HYyk5h4@paquier.xyz
2021-06-02 12:19:39 +09:00
Noah Misch 42344ad0ed Add Windows file version information to libpq_pipeline.exe. 2021-06-01 18:04:15 -07:00
Noah Misch 49527a32ca Fix missing gettimeofday() declaration.
This avoids a warning under MinGW versions having gettimeofday(), per
buildfarm member walleye.
2021-06-01 18:04:14 -07:00
Tom Lane 1103033aed Reject SELECT ... GROUP BY GROUPING SETS (()) FOR UPDATE.
This case should be disallowed, just as FOR UPDATE with a plain
GROUP BY is disallowed; FOR UPDATE only makes sense when each row
of the query result can be identified with a single table row.
However, we missed teaching CheckSelectLocking() to check
groupingSets as well as groupClause, so that it would allow
degenerate grouping sets.  That resulted in a bad plan and
a null-pointer dereference in the executor.

Looking around for other instances of the same bug, the only one
I found was in examine_simple_variable().  That'd just lead to
silly estimates, but it should be fixed too.

Per private report from Yaoguang Chen.
Back-patch to all supported branches.
2021-06-01 11:12:56 -04:00
Amit Kapila eb89cb43a0 pgoutput: Fix memory leak due to RelationSyncEntry.map.
Release memory allocated when creating the tuple-conversion map and its
component TupleDescs when its owning sync entry is invalidated.
TupleDescs must also be freed when no map is deemed necessary, to begin
with.

Reported-by: Andres Freund
Author: Amit Langote
Reviewed-by: Takamichi Osumi, Amit Kapila
Backpatch-through: 13, where it was introduced
Discussion: https://postgr.es/m/MEYP282MB166933B1AB02B4FE56E82453B64D9@MEYP282MB1669.AUSP282.PROD.OUTLOOK.COM
2021-06-01 14:27:14 +05:30
Thomas Munro a40646e30d Fix error handling in replacement pthread_barrier_init().
Commit 44bf3d50 incorrectly used an errno-style interface when supplying
missing pthread functionality (i.e. on macOS), but it should check for
and return error numbers directly.
2021-06-01 11:31:06 +12:00
Peter Eisentraut 7c544ecdad Fix RADIUS error reporting in hba file parsing
The RADIUS-related checks in parse_hba_line() did not respect elevel
and did not fill in *err_msg.  Also, verify_option_list_length()
pasted together error messages in an untranslatable way.  To fix the
latter, remove the function and do the error checking inline.  It's a
bit more verbose but only minimally longer, and it makes fixing the
first two issues straightforward.

Reviewed-by: Magnus Hagander <magnus@hagander.net>
Discussion: https://www.postgresql.org/message-id/flat/8381e425-8c23-99b3-15ec-3115001db1b2%40enterprisedb.com
2021-05-31 18:43:48 +02:00
Tom Lane 6ee41a301e Fix mis-planning of repeated application of a projection.
create_projection_plan contains a hidden assumption (here made
explicit by an Assert) that a projection-capable Path will yield a
projection-capable Plan.  Unfortunately, that assumption is violated
only a few lines away, by create_projection_plan itself.  This means
that two stacked ProjectionPaths can yield an outcome where we try to
jam the upper path's tlist into a non-projection-capable child node,
resulting in an invalid plan.

There isn't any good reason to have stacked ProjectionPaths; indeed the
whole concept is faulty, since the set of Vars/Aggs/etc needed by the
upper one wouldn't necessarily be available in the output of the lower
one, nor could the lower one create such values if they weren't
available from its input.  Hence, we can fix this by adjusting
create_projection_path to strip any top-level ProjectionPath from the
subpath it's given.  (This amounts to saying "oh, we changed our
minds about what we need to project here".)

The test case added here only fails in v13 and HEAD; before that, we
don't attempt to shove the Sort into the parallel part of the plan,
for reasons that aren't entirely clear to me.  However, all the
directly-related code looks generally the same as far back as v11,
where the hazard was introduced (by d7c19e62a).  So I've got no faith
that the same type of bug doesn't exist in v11 and v12, given the
right test case.  Hence, back-patch the code changes, but not the
irrelevant test case, into those branches.

Per report from Bas Poot.

Discussion: https://postgr.es/m/534fca83789c4a378c7de379e9067d4f@politie.nl
2021-05-31 12:03:00 -04:00
Noah Misch d03eeab886 Raise a timeout to 180s, in test 010_logical_decoding_timelines.pl.
Per buildfarm member hornet.  Also, update Pod documentation showing the
lower value.  Back-patch to v10, where the test first appeared.
2021-05-31 00:29:58 -07:00
Michael Paquier 12cc956664 Improve some error wording with multirange type parsing
Braces were referred in some error messages as only brackets (not curly
brackets or curly braces), which can be confusing as other types of
brackets could be used.

While on it, add one test to check after the case of junk characters
detected after a right brace.

Author: Kyotaro Horiguchi
Discussion: https://postgr.es/m/20210514.153153.1814935914483287479.horikyota.ntt@gmail.com
2021-05-31 11:35:00 +09:00
Thomas Munro b1d6538903 Fix race condition when sharing tuple descriptors.
Parallel query processes that called BlessTupleDesc() for identical
tuple descriptors at the same moment could crash.  There was code to
handle that rare case, but it dereferenced a bogus DSA pointer.  Repair.

Back-patch to 11, where commit cc5f8136 added support for sharing tuple
descriptors in parallel queries.

Reported-by: Eric Thinnes <e.thinnes@gmx.de>
Discussion: https://postgr.es/m/99aaa2eb-e194-bf07-c29a-1a76b4f2bcf9%40gmx.de
2021-05-29 15:12:34 +12:00
Andrew Dunstan d69fcb9cae
fix syntax error 2021-05-28 09:35:11 -04:00
Andrew Dunstan fb424ae85f
Report configured port in MSVC built pg_config
This is a long standing omission, discovered when trying to write code
that relied on it.

Backpatch to all live branches.
2021-05-28 09:30:16 -04:00
Peter Geoghegan 9afdea9824 Fix VACUUM VERBOSE's LP_DEAD item pages output.
Oversight in commit 5100010e.
2021-05-27 17:09:16 -07:00
Tom Lane a4390abecf Reduce the range of OIDs reserved for genbki.pl.
Commit ab596105b increased FirstBootstrapObjectId from 12000 to 13000,
but we've had some push-back about that.  It's worrisome to reduce the
daylight between there and FirstNormalObjectId, because the number of
OIDs consumed during initdb for collation objects is hard to predict.

We can improve the situation by abandoning the assumption that these
OIDs must be globally unique.  It should be sufficient for them to be
unique per-catalog.  (Any code that's unhappy about that is broken
anyway, since no more than per-catalog uniqueness can be guaranteed
once the OID counter wraps around.)  With that change, the largest OID
assigned during genbki.pl (starting from a base of 10000) is a bit
under 11000.  This allows reverting FirstBootstrapObjectId to 12000
with reasonable confidence that that will be sufficient for many years
to come.

We are not, at this time, abandoning the expectation that
hand-assigned OIDs (below 10000) are globally unique.  Someday that'll
likely be necessary, but the need seems years away still.

This is late for v14, but it seems worth doing it now so that
downstream software doesn't have to deal with the consequences of
a change in FirstBootstrapObjectId.  In any case, we already
bought into forcing an initdb for beta2, so another catversion
bump won't hurt.

Discussion: https://postgr.es/m/1665197.1622065382@sss.pgh.pa.us
2021-05-27 15:55:08 -04:00
Tom Lane e6241d8e03 Rethink definition of pg_attribute.attcompression.
Redefine '\0' (InvalidCompressionMethod) as meaning "if we need to
compress, use the current setting of default_toast_compression".
This allows '\0' to be a suitable default choice regardless of
datatype, greatly simplifying code paths that initialize tupledescs
and the like.  It seems like a more user-friendly approach as well,
because now the default compression choice doesn't migrate into table
definitions, meaning that changing default_toast_compression is
usually sufficient to flip an installation's behavior; one needn't
tediously issue per-column ALTER SET COMPRESSION commands.

Along the way, fix a few minor bugs and documentation issues
with the per-column-compression feature.  Adopt more robust
APIs for SetIndexStorageProperties and GetAttributeCompression.

Bump catversion because typical contents of attcompression will now
be different.  We could get away without doing that, but it seems
better to ensure v14 installations all agree on this.  (We already
forced initdb for beta2, anyway.)

Discussion: https://postgr.es/m/626613.1621787110@sss.pgh.pa.us
2021-05-27 13:24:27 -04:00
Peter Eisentraut a717e5c771 Fix vpath build in libpq_pipeline test
The path needs to be set to refer to the build directory, not the
current directory, because that's actually the source directory at
that point.

fix for 6abc8c2596
2021-05-27 16:40:52 +02:00
Peter Eisentraut 6abc8c2596 Add NO_INSTALL option to pgxs
Apply in libpq_pipeline test makefile, so that the test file is not
installed into tmp_install.

Reviewed-by: Alvaro Herrera <alvherre@alvh.no-ip.org>
Reviewed-by: Tom Lane <tgl@sss.pgh.pa.us>
Discussion: https://www.postgresql.org/message-id/flat/cb9d16a6-760f-cd44-28d6-b091d5fb6ca7%40enterprisedb.com
2021-05-27 13:58:29 +02:00
Michael Paquier 0251106634 Fix MSVC scripts when building with GSSAPI/Kerberos
The deliverables of upstream Kerberos on Windows are installed with
paths that do not match our MSVC scripts.  First, the include folder was
named "inc/" in our scripts, but the upstream MSIs use "include/".
Second, the build would fail with 64-bit environments as the libraries
are named differently.

This commit adjusts the MSVC scripts to be compatible with the latest
installations of upstream, and I have checked that the compilation was
able to work with the 32-bit and 64-bit installations.

Special thanks to Kondo Yuta for the help in investigating the situation
in hamerkop, which had an incorrect configuration for the GSS
compilation.

Reported-by: Brian Ye
Discussion: https://postgr.es/m/162128202219.27274.12616756784952017465@wrigleys.postgresql.org
Backpatch-through: 9.6
2021-05-27 20:11:00 +09:00
Peter Eisentraut 388e75ad33 Replace run-time error check with assertion
The error message was checking that the structures returned from the
parser matched expectations.  That's something we usually use
assertions for, not a full user-facing error message.  So replace that
with an assertion (hidden inside lfirst_node()).

Reviewed-by: Tom Lane <tgl@sss.pgh.pa.us>
Discussion: https://www.postgresql.org/message-id/flat/452e9df8-ec89-e01b-b64a-8cc6ce830458%40enterprisedb.com
2021-05-27 09:54:14 +02:00
Michael Paquier 2941138e60 doc: Fix description of some GUCs in docs and postgresql.conf.sample
The following parameters have been imprecise, or incorrect, about their
description (PGC_POSTMASTER or PGC_SIGHUP):
- autovacuum_work_mem (docs, as of 9.6~)
- huge_page_size (docs, as of 14~)
- max_logical_replication_workers (docs, as of 10~)
- max_sync_workers_per_subscription (docs, as of 10~)
- min_dynamic_shared_memory (docs, as of 14~)
- recovery_init_sync_method (postgresql.conf.sample, as of 14~)
- remove_temp_files_after_crash (docs, as of 14~)
- restart_after_crash (docs, as of 9.6~)
- ssl_min_protocol_version (docs, as of 12~)
- ssl_max_protocol_version (docs, as of 12~)

This commit adjusts the description of all these parameters to be more
consistent with the practice used for the others.

Revewed-by: Justin Pryzby
Discussion: https://postgr.es/m/YK2ltuLpe+FbRXzA@paquier.xyz
Backpatch-through: 9.6
2021-05-27 14:57:28 +09:00
Amit Kapila 6f4bdf8152 Fix assertion during streaming of multi-insert toast changes.
While decoding the multi-insert WAL we can't clean the toast untill we get
the last insert of that WAL record. Now if we stream the changes before we
get the last change, the memory for toast chunks won't be released and we
expect the txn to have streamed all changes after streaming.  This
restriction is mainly to ensure the correctness of streamed transactions
and it doesn't seem worth uplifting such a restriction just to allow this
case because anyway we will stream the transaction once such an insert is
complete.

Previously we were using two different flags (one for toast tuples and
another for speculative inserts) to indicate partial changes. Now instead
we replaced both of them with a single flag to indicate partial changes.

Reported-by: Pavan Deolasee
Author: Dilip Kumar
Reviewed-by: Pavan Deolasee, Amit Kapila
Discussion: https://postgr.es/m/CABOikdN-_858zojYN-2tNcHiVTw-nhxPwoQS4quExeweQfG1Ug@mail.gmail.com
2021-05-27 07:59:43 +05:30
Michael Paquier 190fa5a00a Fix typo in heapam.c
Author: Hou Zhijie
Discussion: https://postgr.es/m/OS0PR01MB571612191738540B27A8DE5894249@OS0PR01MB5716.jpnprd01.prod.outlook.com
2021-05-26 19:53:03 +09:00
Alvaro Herrera eb43bdbf51
Make detach-partition-concurrently-4 less timing sensitive
Same as 5e0b1aeb2d, for the companion test file.

This one seems lower probability (only two failures in a month of runs);
I was hardly able to reproduce a failure without a patch, so the fact
that I was also unable to reproduce one with it doesn't say anything.
We'll have to wait for further buildfarm results to see if we need any
further adjustments.

Discussion: https://postgr.es/m/20210524090712.GA3771394@rfd.leadboat.com
2021-05-25 19:44:55 -04:00
Tom Lane e30e3fdea8 Fix use of uninitialized variable in inline_function().
Commit e717a9a18 introduced a code path that bypassed the call of
get_expr_result_type, which is not good because we need its rettupdesc
result to pass to check_sql_fn_retval.  We'd failed to notice right
away because the code path in which check_sql_fn_retval uses that
argument is fairly hard to reach in this context.  It's not impossible
though, and in any case inline_function would have no business
assuming that check_sql_fn_retval doesn't need that value.

To fix, move get_expr_result_type out of the if-block, which in
turn requires moving the construction of the dummy FuncExpr
out of it.

Per report from Ranier Vilela.  (I'm bemused by the lack of any
compiler complaints...)

Discussion: https://postgr.es/m/CAEudQAqBqQpQ3HruWAGU_7WaMJ7tntpk0T8k_dVtNB46DqdBgw@mail.gmail.com
2021-05-25 12:55:55 -04:00
Alvaro Herrera 5e0b1aeb2d
Make detach-partition-concurrently-3 less timing-sensitive
This recently added test has shown to be too sensitive to timing when
sending a cancel to a session waiting for a lock.

We fix this by running a no-op query in the blocked session immediately
after the cancel; this avoids the session that sent the cancel sending
another query immediately before the cancel has been reported.
Idea by Noah Misch.

With that fix, we sometimes see that the cancel error report is shown
only relative to the step that is cancelled, instead of together with
the step that sends the cancel.  To increase the probability that both
steps are shown togeter, add a 0.1s sleep to the cancel.  In normal
conditions this appears sufficient to silence most failures, but we'll
see that the slower buildfarm members say about it.

Reported-by: Takamichi Osumi <osumi.takamichi@fujitsu.com>
Discussion: https://postgr.es/m/OSBPR01MB4888C4ABA361C7E81094AC66ED269@OSBPR01MB4888.jpnprd01.prod.outlook.com
2021-05-25 12:53:57 -04:00
Peter Eisentraut 8673a37c85 postgresql.conf.sample: Make vertical spacing consistent 2021-05-25 11:49:54 +02:00
Michael Paquier fb0f5f0172 Fix memory leak when de-toasting compressed values in VACUUM FULL/CLUSTER
VACUUM FULL and CLUSTER can be used to enforce the use of the existing
compression method of a toastable column if a value currently stored is
compressed with a method that does not match the column's defined
method.  The code in charge of decompressing and recompressing toast
values at rewrite left around the detoasted values, causing an
accumulation of memory allocated in TopTransactionContext.

When processing large relations, this could cause the system to run out
of memory.  The detoasted values are not needed once their tuple is
rewritten, and this commit ensures that the necessary cleanup happens.

Issue introduced by bbe0a81d.  The comments of the area are reordered a
bit while on it.

Reported-by: Andres Freund
Analyzed-by: Andres Freund
Author: Michael Paquier
Reviewed-by: Dilip Kumar
Discussion: https://postgr.es/m/20210521211929.pcehg6f23icwstdb@alap3.anarazel.de
2021-05-25 14:27:18 +09:00
Amit Kapila 0734b0e983 Improve docs and error messages for parallel vacuum.
The error messages, docs, and one of the options were using
'parallel degree' to indicate parallelism used by vacuum command. We
normally use 'parallel workers' at other places so change it for parallel
vacuum accordingly.

Author: Bharath Rupireddy
Reviewed-by: Dilip Kumar, Amit Kapila
Backpatch-through: 13
Discussion: https://postgr.es/m/CALj2ACWz=PYrrFXVsEKb9J1aiX4raA+UBe02hdRp_zqDkrWUiw@mail.gmail.com
2021-05-25 09:26:53 +05:30
Michael Paquier 01e6f1a842 Disallow SSL renegotiation
SSL renegotiation is already disabled as of 48d23c72, however this does
not prevent the server to comply with a client willing to use
renegotiation.  In the last couple of years, renegotiation had its set
of security issues and flaws (like the recent CVE-2021-3449), and it
could be possible to crash the backend with a client attempting
renegotiation.

This commit takes one extra step by disabling renegotiation in the
backend in the same way as SSL compression (f9264d15) or tickets
(97d3a0b0).  OpenSSL 1.1.0h has added an option named
SSL_OP_NO_RENEGOTIATION able to achieve that.  In older versions
there is an option called SSL3_FLAGS_NO_RENEGOTIATE_CIPHERS that
was undocumented, and could be set within the SSL object created when
the TLS connection opens, but I have decided not to use it, as it feels
trickier to rely on, and it is not official.  Note that this option is
not usable in OpenSSL < 1.1.0h as the internal contents of the *SSL
object are hidden to applications.

SSL renegotiation concerns protocols up to TLSv1.2.

Per original report from Robert Haas, with a patch based on a suggestion
by Andres Freund.

Author: Michael Paquier
Reviewed-by: Daniel Gustafsson
Discussion: https://postgr.es/m/YKZBXx7RhU74FlTE@paquier.xyz
Backpatch-through: 9.6
2021-05-25 10:10:09 +09:00
David Rowley cba5c70b95 Fix setrefs.c code for Result Cache nodes
Result Cache, added in 9eacee2e6 neglected to properly adjust the plan
references in setrefs.c.  This could lead to the following error during
EXPLAIN:

ERROR:  cannot decompile join alias var in plan tree

Fix that.

Bug: 17030
Reported-by: Hans Buschmann
Discussion: https://postgr.es/m/17030-5844aecae42fe223@postgresql.org
2021-05-25 12:50:22 +12:00
Peter Geoghegan c242baa4a8 Consider triggering VACUUM failsafe during scan.
The wraparound failsafe mechanism added by commit 1e55e7d1 handled the
one-pass strategy case (i.e. the "table has no indexes" case) by adding
a dedicated failsafe check.  This made up for the fact that the usual
one-pass checks inside lazy_vacuum_all_indexes() cannot ever be reached
during a one-pass strategy VACUUM.

This approach failed to account for two-pass VACUUMs that opt out of
index vacuuming up-front.  The INDEX_CLEANUP off case in the only case
that works like that.

Fix this by performing a failsafe check every 4GB during the first scan
of the heap, regardless of the details of the VACUUM.  This eliminates
the special case, and will make the failsafe trigger more reliably.

Author: Peter Geoghegan <pg@bowt.ie>
Reported-By: Andres Freund <andres@anarazel.de>
Reviewed-By: Masahiko Sawada <sawada.mshk@gmail.com>
Discussion: https://postgr.es/m/20210424002921.pb3t7h6frupdqnkp@alap3.anarazel.de
2021-05-24 17:14:02 -07:00
David Rowley 99c5852e20 Add missing NULL check when building Result Cache paths
Code added in 9e215378d to disable building of Result Cache paths when
not all join conditions are part of the parameterization of a unique
join failed to first check if the inner path's param_info was set before
checking the param_info's ppi_clauses.

Add a check for NULL values here and just bail on trying to build the
path if param_info is NULL. lateral_vars are not considered when
deciding if the join is unique, so we're not missing out on doing the
optimization when there are lateral_vars and no param_info.

Reported-by: Coverity, via Tom Lane
Discussion: https://postgr.es/m/457998.1621779290@sss.pgh.pa.us
2021-05-24 12:37:11 +12:00
Tom Lane f5024d8d7b Re-order pg_attribute columns to eliminate some padding space.
Now that attcompression is just a char, there's a lot of wasted
padding space after it.  Move it into the group of char-wide
columns to save a net of 4 bytes per pg_attribute entry.  While
we're at it, swap the order of attstorage and attalign to make for
a more logical grouping of these columns.

Also re-order actions in related code to match the new field ordering.

This patch also fixes one outright bug: equalTupleDescs() failed to
compare attcompression.  That could, for example, cause relcache
reload to fail to adopt a new value following a change.

Michael Paquier and Tom Lane, per a gripe from Andres Freund.

Discussion: https://postgr.es/m/20210517204803.iyk5wwvwgtjcmc5w@alap3.anarazel.de
2021-05-23 12:12:09 -04:00