Give the new GUC introduced by d4e71df6 a name that is clearly not
intended for mainstream use quite yet.
Future proposals would drop the prefix only after adding infrastructure
to make it efficient. Having the switch in the tree sooner is good
because it might lead to new discoveries about the hazards awaiting us
on a wide range of systems, but that name was too enticing and could
lead to cross-version confusion in future, per complaints from Noah and
Justin.
Suggested-by: Noah Misch <noah@leadboat.com>
Reviewed-by: Noah Misch <noah@leadboat.com>
Reviewed-by: Justin Pryzby <pryzby@telsasoft.com> (the idea, not the patch)
Reviewed-by: Tom Lane <tgl@sss.pgh.pa.us> (ditto)
Discussion: https://postgr.es/m/20230430041106.GA2268796%40rfd.leadboat.com
I've had a bee in my bonnet for some time about getting rid of
RestrictInfo.is_pushed_down, because it's squishily defined and
requires not-inexpensive extra tests to use (cf RINFO_IS_PUSHED_DOWN).
In commit 2489d76c4, I tried to make remove_rel_from_query() not
depend on that macro; but the replacement test is buggy,
as exposed by a report from Rushabh Lathia and Robert Haas.
That change was pretty incidental to the main goal of 2489d76c4,
so let's just revert it for now. (Getting rid of is_pushed_down
is still far away, anyway.)
Discussion: https://postgr.es/m/CA+TgmoYco=hmg+iX1CW9Y1_CzNoSL81J03wUG-d2_3=rue+L2A@mail.gmail.com
There was some odd wording in corner-case gram.y error messages "some
error ... at or near", which appears to have been modeled after "syntax
error" messages. However, they don't work that way, and they're just
wrong. They're also uncovered by tests. Remove the trailing words,
and also add tests.
They were introduced with 5a2832465fd8; backpatch to 15.
Author: Álvaro Herrera <alvherre@alvh.no-ip.org>
An update of the GUC stats_fetch_consistency in a transaction would be
able to trigger an assertion when doing cache->snapshot. In this case,
when retrieving a pgstat entry after the switch, a new snapshot would be
rebuilt, confusing pgstat_build_snapshot() because a snapshot is already
cached with an unexpected mode ("cache").
In order to fix this problem, this commit adds a flag to force a
snapshot clear each time this GUC is changed. Some tests are added to
check, while on it.
Some optimizations in avoiding the snapshot clear should be possible
depending on what is cached and the current GUC value, I guess, but this
solution is simple, and ensures that the state of the cache is updated
each time a new pgstat entry is fetched, hence being consistent with the
level wanted by the client that has set the GUC.
Note that cache->none and snapshot->none would not cause issues, as
fetching a pgstat entry would be retrieved from shared memory on the
second attempt, however a snapshot would still be cached. Similarly,
none->snapshot and none->cache would build a new snapshot on the second
fetch attempt. Finally, snapshot->cache would cache a new snapshot on
the second attempt.
Reported-by: Alexander Lakhin
Author: Kyotaro Horiguchi
Discussion: https://postgr.es/m/17804-2a118cd046f2d0e5@postgresql.org
backpatch-through: 15
The commit a6e04b1d20 added a test to ensure that the invalidated logical
slots don't retain WAL. The test was ensuring that the checkpoint removes
the WAL files corresponding to invalidated logical slots on the standby
node but missed the point that the standby node also had a physical slot
which led to the prevention of WAL file removal. Move the creation of
physical slot on the standby and initialization of cascading standby closer
to the test case that actually required it so that other tests don't get
affected by the presence of the physical slot on standby.
Author: Bertrand Drouvot
Reviewed-by: Amit Kapila
Discussion: https://postgr.es/m/2fefa454-5a70-2174-ddbf-4a0e41537139@gmail.com
If an SRF in the FROM clause references a table having row-level
security policies, and we inline that SRF into the calling query,
we neglected to mark the plan as potentially dependent on which
role is executing it. This could lead to later executions in the
same session returning or hiding rows that should have been hidden
or returned instead.
Our thanks to Wolfgang Walther for reporting this problem.
Stephen Frost and Tom Lane
Security: CVE-2023-2455
The two methods don't cooperate, so set_config_option("search_path",
...) has been ineffective under non-empty overrideStack. This defect
enabled an attacker having database-level CREATE privilege to execute
arbitrary code as the bootstrap superuser. While that particular attack
requires v13+ for the trusted extension attribute, other attacks are
feasible in all supported versions.
Standardize on the combination of NewGUCNestLevel() and
set_config_option("search_path", ...). It is newer than
PushOverrideSearchPath(), more-prevalent, and has no known
disadvantages. The "override" mechanism remains for now, for
compatibility with out-of-tree code. Users should update such code,
which likely suffers from the same sort of vulnerability closed here.
Back-patch to v11 (all supported versions).
Alexander Lakhin. Reported by Alexander Lakhin.
Security: CVE-2023-2454
CREATE SCHEMA AUTHORIZATION with appended schema elements can lead to
crashes when comparing the schema name of the query with the schemas
used in the qualification of some clauses in the elements' queries.
The origin of the problem is that the transformation routine for the
elements listed in a CREATE SCHEMA query uses as new, expected, schema
name the one listed in CreateSchemaStmt itself. However, depending on
the query, CreateSchemaStmt.schemaname may be NULL, being computed
instead from the role specification of the query given by the
AUTHORIZATION clause, that could be either:
- A user name string, with the new schema name being set to the same
value as the role given.
- Guessed from CURRENT_ROLE, SESSION_ROLE or CURRENT_ROLE, with a new
schema name computed from the security context where CREATE SCHEMA is
running.
Regression tests are added for CREATE SCHEMA with some appended elements
(some of them with schema qualifications), covering also some role
specification patterns.
While on it, this simplifies the context structure used during the
transformation of the elements listed in a CREATE SCHEMA query by
removing the fields for the role specification and the role type. They
were not used, and for the role specification this could be confusing as
the schema name may by extracted from that at the beginning of
CreateSchemaCommand().
This issue exists for a long time, so backpatch down to all the versions
supported.
Reported-by: Song Hongyu
Author: Michael Paquier
Reviewed-by: Richard Guo
Discussion: https://postgr.es/m/17909-f65c12dfc5f0451d@postgresql.org
Backpatch-through: 11
The test to ensure that decoding changes via logical slot from another
database will fail was incorrectly done on the primary node instead of on
the standby node.
In the passing, make the test to wait for replay catchup by using
wait_for_replay_catchup(). This will make it consistent with the way we
wait at other places in the test.
Author: Shi yu
Reviewed-by: Bertrand Drouvot, Amit Kapila
Discussion: https://postgr.es/m/OSZPR01MB6310B0A507A0F2A2D379F38CFD6A9@OSZPR01MB6310.jpnprd01.prod.outlook.com
Commit 664d75753 added the BackgroundPsql module with helper functions
for tests running interactive or background psql tasks. The new module
was however not added to the install rules of the build systems.
Reported-by: Peter Eisentraut <peter.eisentraut@enterprisedb.com>
Discussion: https://postgr.es/m/c0ba3008-dbc8-e53f-29f2-2e9abe72b2a2@enterprisedb.com
The errormessage for an incorrect require_auth method wasn't using the
common "invalid %s value" errormessage which lessens the burden on our
translators. Fix by changing to that format to make use of existing
translations and to make error messages consistent in wording.
Reported and fixed by Gurjeet Singh with some tweaking by myself.
Author: Gurjeet Singh <gurjeet@singh.im>
Discussion: https://postgr.es/m/CABwTF4Xu3g9zohJ9obu8m7MKbf8g63NgpRDjwqPHQgAtB+Gb8Q@mail.gmail.com
a9c70b46 added the statistics view pg_stat_io which contained columns
"io_context" and "io_object". Given that the columns are in the
pg_stat_io view, the "io" prefix is somewhat redundant, so remove it.
The code variables referring to these fields are kept unchanged so as
they can keep their context about I/O.
Bump catalog version.
Author: Melanie Plageman
Reviewed-by: Kyotaro Horiguchi, Fabrízio de Royes Mello
Discussion: https://postgr.es/m/CAAKRu_aAQoJWrvT2BYYQvJChFKra_O-5ra3jhzKJZqWsTR1CPQ@mail.gmail.com
The error message for a missing or invalid system CA when using
sslrootcert=system differs based on the OpenSSL version used.
In OpenSSL 1.0.1-3.0 it is reported as SSL Error, with varying
degrees of helpfulness in the error message. With OpenSSL 3.1 it
is reported as an SSL SYSCALL error with "Undefined error" as
the error message. This fix pulls out the particular error in
OpenSSL 3.1 as a certificate verify error in order to help the
user better figure out what happened, and to keep the ssl test
working. While there is no evidence that extracing the errors
will clobber errno, this adds a guard against that regardless
to also make the consistent with how we handle OpenSSL errors
elsewhere. It also memorizes the output from OpenSSL 3.0 in
the test in cases where the system CA isn't responding.
Reported-by: Peter Eisentraut <peter.eisentraut@enterprisedb.com>
Discussion: https://postgr.es/m/c39be3c5-c1a5-1e33-1024-16f527e251a4@enterprisedb.com
Utils.pm has a BEGIN block that editorializes on the locale-related
environment variables, primarily in order to stabilize the behavior
of child programs. It turns out that if the calling test script
has already done "use locale", this fails to affect the behavior
of Perl itself, causing locale behavior to be different between
Perl and child programs. That breaks commit cd82e5c79's attempt
to deal with locale-specific behavior in psql.
To fix, we just need to call setlocale() to redo the calculation
of locale.
Per report from Aleksander Alekseev. No back-patch for now, since
there are no locale-dependent TAP tests in prior branches, and
I'm not yet convinced that this won't have side-effects of its own.
Discussion: https://postgr.es/m/CAJ7c6TO9KpYYxoVVseWEQB5KtjWDkt8NfyAeKPcHoe2Jq+ykpw@mail.gmail.com
This fixes many spelling mistakes in comments, but a few references to
invalid parameter names, function names and option names too in comments
and also some in string constants
Also, fix an #undef that was undefining the incorrect definition
Author: Alexander Lakhin
Reviewed-by: Justin Pryzby
Discussion: https://postgr.es/m/d5f68d19-c0fc-91a9-118d-7c6a5a3f5fad@gmail.com
The finalfunc might return a read-write expanded object. If we
de-duplicate multiple call sites for the aggregate, any function(s)
receiving the aggregate result earlier could alter or destroy the
value that reaches the ones called later. This is a brown-paper-bag
bug in commit 42b746d4c, because we actually considered the need
for read-only-ness but failed to realize that it applied to the case
with a finalfunc as well as the case without.
Per report from Justin Pryzby. New error in HEAD,
no need for back-patch.
Discussion: https://postgr.es/m/ZDm5TuKsh3tzoEjz@telsasoft.com
Commit 3e310d837 taught isAssignmentIndirectionExpr() to look through
CoerceToDomain nodes. That's not sufficient, because since commit
04fe805a1 it's been possible for the planner to simplify
CoerceToDomain to RelabelType when the domain has no constraints
to enforce. So we need to look through RelabelType too.
Per bug #17897 from Alexander Lakhin. Although 3e310d837 was
back-patched to v11, it seems sufficient to apply this change
to v12 and later, since 04fe805a1 came in in v12.
Dmitry Dolgov
Discussion: https://postgr.es/m/17897-4216c546c3874044@postgresql.org
For some reason I had not implemented RBM_ZERO_AND_CLEANUP_LOCK support in
ExtendBufferedRelTo(), likely thinking it not being reachable. But it is
reachable, e.g. when replaying a WAL record for a page in a relation that
subsequently is truncated (likely only reachable when doing crash recovery or
PITR, not during ongoing streaming replication).
As now all of the RBM_* modes are supported, remove assertions checking mode.
As we had no test coverage for this scenario, add a new TAP test. There's
plenty more that ought to be tested in this area...
Reported-by: Tom Lane <tgl@sss.pgh.pa.us>
Reported-by: Alexander Lakhin <exclusion@gmail.com>
Reviewed-by: Alexander Lakhin <exclusion@gmail.com>
Discussion: https://postgr.es/m/392271.1681238924%40sss.pgh.pa.us
Discussion: https://postgr.es/m/0b5eb82b-cb99-e0a4-b932-3dc60e2e3926@gmail.com
The partition pruning logic assumed that "b IS NOT true" was exactly the
same as "b IS FALSE". This is not the case when considering NULL values.
Fix this so we correctly include any partition which could hold NULL
values for the NOT case.
Additionally, this fixes a bug in the partition pruning code which handles
partitioned tables partitioned like ((NOT boolcol)). This is a seemingly
unlikely schema design, and it was untested and also broken.
Here we add tests for the ((NOT boolcol)) case and insert some actual data
into those tables and verify we do get the correct rows back when running
queries. I've also adjusted the existing boolpart tests to include some
data and verify we get the correct results too.
Both the bugs being fixed here could lead to incorrect query results with
fewer rows being returned than expected. No additional rows could have
been returned accidentally.
In passing, remove needless ternary expression. It's more simple just to
pass !is_not_clause to makeBoolConst(). It makes sense to do this so the
code is consistent with the bug fix in the "else if" condition just below.
David Kimura did submit a patch to fix the first of the issues here, but
that's not what's being committed here.
Reported-by: David Kimura
Reviewed-by: Richard Guo, David Kimura
Discussion: https://postgr.es/m/CAHnPFjQ5qxs6J_p+g8=ww7GQvfn71_JE+Tygj0S7RdRci1uwPw@mail.gmail.com
Backpatch-through: 11, all supported versions
Hash join tuples reuse the HOT status bit to indicate match status
during hash join execution. Correct reuse requires clearing the bit in
all tuples. Serial hash join and parallel multi-batch hash join do so
upon inserting the tuple into the hashtable. Single batch parallel hash
join and batch 0 of unexpected multi-batch hash joins forgot to do this.
It hadn't come up before because hashtable tuple match bits are only
used for right and full outer joins and parallel ROJ and FOJ were
unsupported. 11c2d6fdf5 introduced support for parallel ROJ/FOJ but
neglected to ensure the match bits were reset.
Author: Melanie Plageman <melanieplageman@gmail.com>
Reported-by: Richard Guo <guofenglinux@gmail.com>
Discussion: https://postgr.es/m/flat/CAMbWs48Nde1Mv%3DBJv6_vXmRKHMuHZm2Q_g4F6Z3_pn%2B3EV6BGQ%40mail.gmail.com
Make sure that function declarations use names that exactly match the
corresponding names from function definitions in a few places. These
inconsistencies were all introduced relatively recently, after the code
base had parameter name mismatches fixed in bulk (see commits starting
with commits 4274dc22 and 035ce1fe).
pg_bsd_indent still has a couple of similar inconsistencies, which I
(pgeoghegan) have left untouched for now.
Like all earlier commits that cleaned up function parameter names, this
commit was written with help from clang-tidy.
This reverts commit 3d03b24c3 (Revert Add support for Kerberos
credential delegation) which was committed on the grounds of concern
about portability, but on further review and discussion, it's clear that
we are better off explicitly requiring MIT Kerberos as that appears to
be the only GSSAPI library currently that's under proper maintenance
and ongoing development. The API used for storing credentials was added
to MIT Kerberos over a decade ago while for the other libraries which
appear to be mainly based on Heimdal, which exists explicitly to be a
re-implementation of MIT Kerberos, the API never made it to a released
version (even though it was added to the Heimdal git repo over 5 years
ago..).
This post-feature-freeze change was approved by the RMT.
Discussion: https://postgr.es/m/ZDDO6jaESKaBgej0%40tamriel.snowman.net
The test previously had a list of OSes that direct I/O was expected to
work on. That worked well enough for the systems in our build farm, but
didn't survive contact with the Debian build bots running on tmpfs via
overlayfs. tmpfs does not support O_DIRECT, but we don't want to
exclude Linux generally.
The new approach is to try to create an empty file with O_DIRECT from
Perl first. If that fails, we'll skip the test and report what the
error was.
Reported-by: Christoph Berg <myon@debian.org>
Reviewed-by: Dagfinn Ilmari Mannsåker <ilmari@ilmari.org>
Reviewed-by: Andrew Dunstan <andrew@dunslane.net>
Discussion: https://postgr.es/m/ZDYd4A78cT2ULxZZ%40msg.df7cb.de
This reverts commit e056c557aef4 and minor later fixes thereof.
There's a few problems in this new feature -- most notably regarding
pg_upgrade behavior, but others as well. This new feature is not in any
way critical on its own, so instead of scrambling to fix it we revert it
and try again in early 17 with these issues in mind.
Discussion: https://postgr.es/m/3801207.1681057430@sss.pgh.pa.us
At least one slow buildfarm system (hoverfly) showed that the database
creation was not replicated before we try to create logical replication slots
on the standby, in that database.
Reported-by: Noah Misch <noah@leadboat.com>
Author: "Drouvot, Bertrand" <bertranddrouvot.pg@gmail.com>
Discussion: https://postgr.es/m/20230411053657.GA1177147@rfd.leadboat.com
There's no need to check if opt->arg is NULL since defGetString() already
does that and raises an ERROR if it is. Let's just remove that check.
Also, combine the two remaining ERRORs into a single check. It seems
better to give an indication about what sort of values we're looking for
rather than just to state that the value given isn't valid. Make
BUFFER_USAGE_LIMIT uppercase in this ERROR message too. It's already
upper case in one other error message, so make that consistent.
Reported-by: Kyotaro Horiguchi
Discussion: https://postgr.es/m/20230411.102335.1643720544536884844.horikyota.ntt@gmail.com
Checking for the required versions of IO::Pty as well as IPC::Run
can be achieved with a single eval call, and by using the VERSION
function the comparison is guaranteed to follow the same rules as
calling 'use' on the module with a version.
Reported-by: Andrew Dunstan <andrew@dunslane.net>
Discussion: https://postgr.es/m/6d880ea2-f8ca-f458-4dcd-a7a3e6d6cd7c@dunslane.net
The new direct I/O test deliberately uses a very small shared_buffers to
force some disk transfers without making the data set large and slow,
but ran into a problem with wal_level = minimal: log_newpage_range()
pins many buffers, leading to a few intermittent "no unpinned buffers
available" errors.
We could presumably fix that by adjusting shared_buffers, but crake
seems to be trying to tell us something interesting with these settings,
so let's just avoid wal_level = minimal in this test for now.
Reported-by: Andres Freund <andres@anarazel.de>
Discussion: https://postgr.es/m/20230408060408.n7xdwk3mxj5oykt6%40awork3.anarazel.de
Our convention for some time has been that successful tests shouldn't
print anything on stderr. A stray "diag" call violated that, and
for that matter messed up the normal TAP progress display.
IPC::Run versions prior to 0.98 cause the interactive session to time out,
so SKIP the test in case these versions are detected (they are within the
base requirement for our TAP tests in general). Error reported by the BF
and investigation by Tom Lane.
Discussion: https://postgr.es/m/414A86BD-986B-48A7-A1E4-EEBCE5AF08CB@yesql.se
This reverts commit 3d4fa227bce4294ce1cc214b4a9d3b7caa3f0454.
Per discussion and buildfarm, this depends on APIs that seem to not
be available on at least one platform (NetBSD). Should be certainly
possible to rework to be optional on that platform if necessary but bit
late for that at this point.
Discussion: https://postgr.es/m/3286097.1680922218@sss.pgh.pa.us
Previously, a PostgreSQL-specific callback checked by the regex engine
had a way to trigger a special error code REG_CANCEL if it detected that
the next call to CHECK_FOR_INTERRUPTS() would certainly throw via
ereport().
A later proposed bugfix aims to move some complex logic out of signal
handlers, so that it won't run until the next CHECK_FOR_INTERRUPTS(),
which makes the above design impossible unless we split
CHECK_FOR_INTERRUPTS() into two phases, one to run logic and another to
ereport(). We may develop such a system in the future, but for the
regex code it is no longer necessary.
An earlier commit moved regex memory management over to our
MemoryContext system. Given that the purpose of the two-phase interrupt
checking was to free memory before throwing, something we don't need to
worry about anymore, it seems simpler to inject CHECK_FOR_INTERRUPTS()
directly into cancelation points, and just let it throw.
Since the plan is to keep PostgreSQL-specific concerns separate from the
main regex engine code (with a view to bein able to stay in sync with
other projects), do this with a new macro INTERRUPT(), customizable in
regcustom.h and defaulting to nothing.
Reviewed-by: Tom Lane <tgl@sss.pgh.pa.us>
Discussion: https://postgr.es/m/CA%2BhUKGK3PGKwcKqzoosamn36YW-fsuTdOPPF1i_rtEO%3DnEYKSg%40mail.gmail.com
During WAL replay on the standby, when a conflict with a logical slot is
identified, invalidate such slots. There are two sources of conflicts:
1) Using the information added in 6af1793954e, logical slots are invalidated if
required rows are removed
2) wal_level on the primary server is reduced to below logical
Uses the infrastructure introduced in the prior commit. FIXME: add commit
reference.
Change InvalidatePossiblyObsoleteSlot() to use a recovery conflict to
interrupt use of a slot, if called in the startup process. The new recovery
conflict is added to pg_stat_database_conflicts, as confl_active_logicalslot.
See 6af1793954e for an overall design of logical decoding on a standby.
Bumps catversion for the addition of the pg_stat_database_conflicts column.
Bumps PGSTAT_FILE_FORMAT_ID for the same reason.
Author: "Drouvot, Bertrand" <bertranddrouvot.pg@gmail.com>
Author: Andres Freund <andres@anarazel.de>
Author: Amit Khandekar <amitdkhan.pg@gmail.com> (in an older version)
Reviewed-by: "Drouvot, Bertrand" <bertranddrouvot.pg@gmail.com>
Reviewed-by: Andres Freund <andres@anarazel.de>
Reviewed-by: Robert Haas <robertmhaas@gmail.com>
Reviewed-by: Fabrízio de Royes Mello <fabriziomello@gmail.com>
Reviewed-by: Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com>
Reviewed-by: Amit Kapila <amit.kapila16@gmail.com>
Reviewed-by: Alvaro Herrera <alvherre@alvh.no-ip.org>
Discussion: https://postgr.es/m/20230407075009.igg7be27ha2htkbt@awork3.anarazel.de
Needed for logical decoding on a standby. Slots need to be invalidated because
of the horizon if rows required for logical decoding are removed. If the
primary's wal_level is lowered from 'logical', logical slots on the standby
need to be invalidated.
The new invalidation methods will be used in a subsequent commit.
Logical slots that have been invalidated can be identified via the new
pg_replication_slots.conflicting column.
See 6af1793954e for an overall design of logical decoding on a standby.
Bumps catversion for the addition of the new pg_replication_slots column.
Author: "Drouvot, Bertrand" <bertranddrouvot.pg@gmail.com>
Author: Andres Freund <andres@anarazel.de>
Author: Amit Khandekar <amitdkhan.pg@gmail.com> (in an older version)
Reviewed-by: "Drouvot, Bertrand" <bertranddrouvot.pg@gmail.com>
Reviewed-by: Andres Freund <andres@anarazel.de>
Reviewed-by: Robert Haas <robertmhaas@gmail.com>
Reviewed-by: Fabrízio de Royes Mello <fabriziomello@gmail.com>
Reviewed-by: Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com>
Reviewed-by: Amit Kapila <amit.kapila16@gmail.com>
Reviewed-by: Melanie Plageman <melanieplageman@gmail.com>
Reviewed-by: Alvaro Herrera <alvherre@alvh.no-ip.org>
Discussion: https://postgr.es/m/20230407075009.igg7be27ha2htkbt@awork3.anarazel.de
