I'd not checked that this iteration of the test actually worked
with a bootstrap superuser not named 'postgres'. It didn't,
because the coercion rules for CASE caused us to try to cast
the 'postgres' literal to regrole. Mea culpa.
Per buildfarm (via Alexander Korotkov)
Discussion: https://postgr.es/m/CAPpHfdsV=iTvH6B858hnH1bLgewYH6cdTnO_eOOw9EOa8kehkA@mail.gmail.com
This had been disabled because the test "doesn't delete its user".
It doesn't seem like a great idea for the meson tests to act
differently from the makefile tests, though, and the makefiles
had no such exception (which is how come only copperhead noticed
the problem just fixed in 534287403). In any case, the premise
is false since 936e3fa37, so let's remove the restriction.
Discussion: https://postgr.es/m/2857513.1713733688@sss.pgh.pa.us
If an ACL recorded in pg_init_privs mentions a non-pinned role,
that reference must also be noted in pg_shdepend so that we know
that the role can't go away without removing the ACL reference.
Otherwise, DROP ROLE could succeed and leave dangling entries
behind, which is what's causing the recent upgrade-check failures
on buildfarm member copperhead.
This has been wrong since pg_init_privs was introduced, but it's
escaped notice because typical pg_init_privs entries would only
mention the bootstrap superuser (pinned) or at worst the owner
of the extension (who can't go away before the extension does).
We lack even a representation of such a role reference for
pg_shdepend. My first thought for a solution was entries listing
pg_init_privs in classid, but that doesn't work because then there's
noplace to put the granted-on object's classid. Rather than adding
a new column to pg_shdepend, let's add a new deptype code
SHARED_DEPENDENCY_INITACL. Much of the associated boilerplate
code can be cribbed from code for SHARED_DEPENDENCY_ACL.
A lot of the bulk of this patch just stems from the new need to pass
the object's owner ID to recordExtensionInitPriv, so that we can
consult it while updating pg_shdepend. While many callers have that
at hand already, a few places now need to fetch the owner ID of an
arbitrary privilege-bearing object. For that, we assume that there
is a catcache on the relevant catalog's OID column, which is an
assumption already made in ExecGrant_common so it seems okay here.
We do need an entirely new routine RemoveRoleFromInitPriv to perform
cleanup of pg_init_privs ACLs during DROP OWNED BY. It's analogous
to RemoveRoleFromObjectACL, but we can't share logic because that
function operates by building a command parsetree and invoking
existing GRANT/REVOKE infrastructure. There is of course no SQL
command that would update pg_init_privs entries when we're not in
process of creating their extension, so we need a routine that can
do the updates directly.
catversion bump because this changes the expected contents of
pg_shdepend. For the same reason, there's no hope of back-patching
this, even though it fixes a longstanding bug. Fortunately, the
case where it's a problem seems to be near nonexistent in the field.
If it weren't for the buildfarm breakage, I'd have been content to
leave this for v18.
Patch by me; thanks to Daniel Gustafsson for review and discussion.
Discussion: https://postgr.es/m/1745535.1712358659@sss.pgh.pa.us
Also, fix a memory leak when updating from non-embeddable to
embeddable. Both were unreachable without adding C code.
Reported-by: Noah Misch
Author: Noah Misch
Reviewed-by: Masahiko Sawada, John Naylor
Discussion: https://postgr.es/m/20240424210319.4c.nmisch%40google.com
. Add missing copytight notices
. improve code coverage
. put work files in a temp directory in the standard location
. improve error checking in C code
. indent perl files with perltidy
. add some comments
per comments from Michael Paquier
Discussion: https://postgr.es/m/ZiC3-cdFys4-6xSk@paquier.xyz
This fixes various typos, duplicated words, and tiny bits of whitespace
mainly in code comments but also in docs.
Author: Daniel Gustafsson <daniel@yesql.se>
Author: Heikki Linnakangas <hlinnaka@iki.fi>
Author: Alexander Lakhin <exclusion@gmail.com>
Author: David Rowley <dgrowleyml@gmail.com>
Author: Nazir Bilal Yavuz <byavuz81@gmail.com>
Discussion: https://postgr.es/m/3F577953-A29E-4722-98AD-2DA9EFF2CBB8@yesql.se
Per gripes from Michael Paquier
Discussion: https://postgr.es/m/ZhTQ6_w1vwOhqTQI@paquier.xyz
Along the way, also clean up a handful of typos in 3311ea86ed and
ea7b4e9a2a, found by Alexander Lakhin, and a couple of stylistic
snafus noted by Daniel Westermann and Daniel Gustafsson.
f587338dec has introduced in the test module injection_points a SQL
function called injection_points_set_local(), that can be used to make
all the injection points linked to the process where they are attached,
discarded automatically if any remain once the process exits.
e2e3b8ae9e has added a NO_INSTALLCHECK to the test module to prevent
the use of installcheck. Now that there is a way to make the test
concurrent-safe, let's use it and remove the installcheck restriction.
Concurrency issues could be easily reproduced by running in a tight
loop a command like this one, in src/test/modules/gin/ (hardcoding
pg_sleep() after attaching injection points enlarges the race window)
and a second test suite like contrib/btree_gin/:
make installcheck USE_MODULE_DB=1
Reviewed-by: Andrey Borodin
Discussion: https://postgr.es/m/ZhNG4Io9uYOgwv3F@paquier.xyz
The module relies on a shmem exit callback to clean up any injection
points linked to a specific process. One of the tests checks for the
case of an injection point name reused in a second connection where the
first connection should clean it up, but it did not count for the fact
that the shmem exit callback of the first connection may not have run
when the second connection begins its work.
The regress library includes a wait_pid() that can be used for this
purpose, instead of a custom wait logic, so let's rely on it to wait for
the first connection to exit before working with the second connection.
The module gains a REGRESS_OPTS to be able to look at the regress
library's dlpath.
This issue could be reproduced with a hardcoded sleep() in the shmem
exit callback, and the CI has been able to trigger it sporadically.
Oversight in f587338dec.
Reported-by: Bharath Rupireddy
Reviewed-by: Andrey Borodin
Discussion: https://postgr.es/m/ZhOd3NXAutteokGL@paquier.xyz
Let table AM define custom reloptions for its tables. This allows specifying
AM-specific parameters by the WITH clause when creating a table.
The reloptions, which could be used outside of table AM, are now extracted
into the CommonRdOptions data structure. These options could be by decision
of table AM directly specified by a user or calculated in some way.
The new test module test_tam_options evaluates the ability to set up custom
reloptions and calculate fields of CommonRdOptions on their base.
The code may use some parts from prior work by Hao Wu.
Discussion: https://postgr.es/m/CAPpHfdurb9ycV8udYqM%3Do0sPS66PJ4RCBM1g-bBpvzUfogY0EA%40mail.gmail.com
Discussion: https://postgr.es/m/AMUA1wBBBxfc3tKRLLdU64rb.1.1683276279979.Hmail.wuhao%40hashdata.cn
Reviewed-by: Reviewed-by: Pavel Borisov, Matthias van de Meent, Jess Davis
This adds a new SQL function injection_points_set_local() that can be
used to force injection points to be run only in the process where they
are attached. This is handy for SQL tests to:
- Detach automatically injection points when the process exits.
- Allow tests with injection points to run concurrently with other test
suites, so as such modules do not have to be marked with
NO_INSTALLCHECK.
Currently, the only condition that can be registered is for a PID.
This could be extended to more kinds later, if required, like database
names/OIDs, roles, or more concepts I did not consider.
Using a single function for SQL scripts is an idea from Heikki
Linnakangas.
Reviewed-by: Andrey Borodin
Discussion: https://postgr.es/m/ZfP7IDs9TvrKe49x@paquier.xyz
As written, the test would fail when run repeatedly because one of the
injection points attached was not detached. This would not matter if
the test is rewritten to be concurrently safe, but let's be clean and
it is a good practice.
Oversight in 6a1ea02c49.
Discussion: https://postgr.es/m/ZfP7IDs9TvrKe49x@paquier.xyz
This patch generalizes libpq's existing single-row mode to allow
individual partial-result PGresults to contain up to N rows, rather
than always one row. This reduces malloc overhead compared to plain
single-row mode, and it is very useful for psql's FETCH_COUNT feature,
since otherwise we'd have to add code (and cycles) to either merge
single-row PGresults into a bigger one or teach psql's
results-printing logic to accept arrays of PGresults.
To avoid API breakage, PQsetSingleRowMode() remains the same, and we
add a new function PQsetChunkedRowsMode() to invoke the more general
case. Also, PGresults obtained the old way continue to carry the
PGRES_SINGLE_TUPLE status code, while if PQsetChunkedRowsMode() is
used then their status code is PGRES_TUPLES_CHUNK. The underlying
logic is the same either way, though.
Daniel Vérité, reviewed by Laurenz Albe and myself (and whacked
around a bit by me, so any remaining bugs are my fault)
Discussion: https://postgr.es/m/CAKZiRmxsVTkO928CM+-ADvsMyePmU3L9DQCa9NwqjvLPcEe5QA@mail.gmail.com
This new DDL command splits a single partition into several parititions.
Just like ALTER TABLE ... MERGE PARTITIONS ... command, new patitions are
created using createPartitionTable() function with parent partition as the
template.
This commit comprises quite naive implementation which works in single process
and holds the ACCESS EXCLUSIVE LOCK on the parent table during all the
operations including the tuple routing. This is why this new DDL command
can't be recommended for large partitioned tables under a high load. However,
this implementation come in handy in certain cases even as is.
Also, it could be used as a foundation for future implementations with lesser
locking and possibly parallel.
Discussion: https://postgr.es/m/c73a1746-0cd0-6bdd-6b23-3ae0b7c0c582%40postgrespro.ru
Author: Dmitry Koval
Reviewed-by: Matthias van de Meent, Laurenz Albe, Zhihong Yu, Justin Pryzby
Reviewed-by: Alvaro Herrera, Robert Haas, Stephane Tachoires
This new DDL command merges several partitions into the one partition of the
target table. The target partition is created using new
createPartitionTable() function with parent partition as the template.
This commit comprises quite naive implementation which works in single process
and holds the ACCESS EXCLUSIVE LOCK on the parent table during all the
operations including the tuple routing. This is why this new DDL command
can't be recommended for large partitioned tables under a high load. However,
this implementation come in handy in certain cases even as is.
Also, it could be used as a foundation for future implementations with lesser
locking and possibly parallel.
Discussion: https://postgr.es/m/c73a1746-0cd0-6bdd-6b23-3ae0b7c0c582%40postgrespro.ru
Author: Dmitry Koval
Reviewed-by: Matthias van de Meent, Laurenz Albe, Zhihong Yu, Justin Pryzby
Reviewed-by: Alvaro Herrera, Robert Haas, Stephane Tachoires
The link commands in test_json_parser/Makefile were a long way
shy of a load, as evidenced by buildfarm failures. Model them
on pgxs.mk's PROGRAM rule. (Probably we should have put these
two test programs in different subdirectories so we could
actually use the PROGRAM rule. But I won't question that
decision today.)
Add overlooked .gitignore entries.
Fix test_json_parser/Makefile to use the pgxs.mk clean rule
instead of fighting it. Suppresses a warning from make,
at least for me.
This parser uses an explicit prediction stack, unlike the present
recursive descent parser where the parser state is represented on the
call stack. This difference makes the new parser suitable for use in
incremental parsing of huge JSON documents that cannot be conveniently
handled piece-wise by the recursive descent parser. One potential use
for this will be in parsing large backup manifests associated with
incremental backups.
Because this parser is somewhat slower than the recursive descent
parser, it is not replacing that parser, but is an additional parser
available to callers.
For testing purposes, if the build is done with -DFORCE_JSON_PSTACK, all
JSON parsing is done with the non-recursive parser, in which case only
trivial regression differences in error messages should be observed.
Author: Andrew Dunstan
Reviewed-By: Jacob Champion
Discussion: https://postgr.es/m/7b0a51d6-0d9d-7366-3a1a-f74397a02f55@dunslane.net
Project policy is to not leave global objects behind after a regress
test run. This was found as a result of the development of a patch
to make pg_regress detect such leftovers automatically, which in the
end was withdrawn due to issues with parallel runs.
Discussion: https://postgr.es/m/E1phvk7-000VAH-7k@gemulon.postgresql.org
Previously, the behavior of TidStoreCreate() was inconsistent between
local and shared TidStore instances in terms of memory limitation. For
local TidStore, a memory context was created with initial and maximum
memory block sizes, as well as a minimum memory context size, based on
the specified max_bytes values. However, for shared TidStore, the
provided DSA area was used for TID storage. Although commit bb952c8c8b
allowed specifying the initial and maximum DSA segment sizes, callers
would have needed to clamp their own limits, which was not consistent
and user-friendly.
With this commit, when creating a shared TidStore, a dedicated DSA
area is created for TID storage instead of using a provided DSA
area. The initial and maximum DSA segment sizes are chosen based on
the specified max_bytes. Other processes can attach to the shared
TidStore using the handle of the created DSA returned by the new
TidStoreGetDSA() function and the DSA pointer returned by
TidStoreGetHandle(). The created DSA has the same lifetime as the
shared TidStore and is deleted when all processes detach from it.
To improve clarity, the TidStoreCreate() function has been divided
into two separate functions: TidStoreCreateLocal() and
TidStoreCreateShared().
Reviewed-by: John Naylor
Discussion: https://postgr.es/m/CAD21AoAyc1j%3DBCdUqZfk6qbdjZ68UgRx1Gkpk0oah4K7S0Ri9g%40mail.gmail.com
Put the node-type-dependent operations into switches on nodeTag.
This should ease addition of new proof rules for other expression
node types. There is no functional change, although some tests
are made in a different order than before.
Also, add a couple of new cross-checks in test_predtest.c.
James Coleman (part of a larger patch series)
Discussion: https://postgr.es/m/CAAaqYe8Bo4bf_i6qKj8KBsmHMYXhe3Xt6vOe3OBQnOaf3_XBWg@mail.gmail.com
TIDStore is a data structure designed to efficiently store large sets
of TIDs. For TID storage, it employs a radix tree, where the key is
a block number, and the value is a bitmap representing offset
numbers. The TIDStore can be created on a DSA area and used by
multiple backend processes simultaneously.
There are potential future users such as tidbitmap.c, though it's very
likely the interface will need to evolve as we come to understand the
needs of different kinds of users. For example, we can support
updating the offset bitmap of existing values.
Currently, the TIDStore is not used for anything yet, aside from the
test code. But an upcoming patch will use it.
This includes a unit test module, in src/test/modules/test_tidstore.
Co-authored-by: John Naylor
Discussion: https://postgr.es/m/CAD21AoAfOZvmfR0j8VmZorZjL7RhTiQdVttNuC4W-Shdc2a-AA%40mail.gmail.com
This checks that certain I/O-related Perl functions properly check
their return value. Some parts of the PostgreSQL code had been a bit
sloppy about that. The new perlcritic warnings are fixed here. I
didn't design any beautiful error messages, mostly just used "or die
$!", which mostly matches existing code, and also this is
developer-level code, so having the system error plus source code
reference should be ok.
Initially, we only activate this check for a subset of what the
perlcritic check would warn about. The effective list is
chmod flock open read rename seek symlink system
The initial set of functions is picked because most existing code
already checked the return value of those, so any omissions are
probably unintended, or because it seems important for test
correctness.
The actual perlcritic configuration is written as an exclude list.
That seems better so that we are clear on what we are currently not
checking. Maybe future patches want to investigate checking some of
the other functions. (In principle, we might eventually want to check
all of them, but since this is test and build support code, not
production code, there are probably some reasonable compromises to be
made.)
Reviewed-by: Daniel Gustafsson <daniel@yesql.se>
Discussion: https://www.postgresql.org/message-id/flat/88b7d4f2-46d9-4cc7-b1f7-613c90f9a76a%40eisentraut.org
I disabled it in cc6e64afda because it was unstable; hopefully the
changes here make it more stable. (Jelte proposed to submit the queries
using simple query protocol, but I chose to instead make the monitor
connection wait until the PgSleep wait event is seen. This is probably
not a terribly meaningful increase in coverage ...)
Discussion: https://postgr.es/m/CAGECzQRQh_5tSy+5cndgv08eNJ2O0Zpwn2YddJtSsmC=Wpy1BQ@mail.gmail.com
The buildfarm script attempts to run all tests marked as
NO_INSTALLCHECK under src/test/modules without paying attention to
whether they are enabled or disabled in the parent Makefile. That
hasn't been a problem so far, because all the tests marked with
NO_INSTALLCHECK ran unconditionally in "make check". But commit
e2e3b8ae9e changed that: the injection fault tests are marked as
NO_INSTALLCHECK, and also depend on --enable-injection-points.
Try to work around that by ensuring that "make check" does nothing in
the those subdirectories. We can hopefully get rid of this hack soon,
after fixing the buildfarm client, or by switching to meson.
Discussion: https://www.postgresql.org/message-id/8e4cf596-dd70-432e-9068-16466ed596ed%40iki.fi
This commit adds new tests to verify that transaction_timeout,
idle_session_timeout, and idle_in_transaction_session_timeout work as expected.
We introduce new injection points in before throwing a timeout FATAL error
and check these injection points are reached.
Discussion: https://postgr.es/m/CAAhFRxiQsRs2Eq5kCo9nXE3HTugsAAJdSQSmxncivebAxdmBjQ%40mail.gmail.com
Author: Andrey Borodin
Reviewed-by: Alexander Korotkov
The newly introduced cancel test in libpq_pipeline was flaky. It's not
completely clear why, but one option is that the check for "active" was
actually seeing the active state for the previous query. This change
should address any such race condition by first waiting until the
connection is reported as idle.
Author: Jelte Fennema-Nio <me@jeltef.nl>
Discussion: https://postgr.es/m/CAGECzQRvmUK5-d68A+cm+fgmfht9Dv2uZ28-qq3QiaF6EAZqPQ@mail.gmail.com
The existing PQcancel API uses blocking IO, which makes PQcancel
impossible to use in an event loop based codebase without blocking the
event loop until the call returns. It also doesn't encrypt the
connection over which the cancel request is sent, even when the original
connection required encryption.
This commit adds a PQcancelConn struct and assorted functions, which
provide a better mechanism of sending cancel requests; in particular all
the encryption used in the original connection are also used in the
cancel connection. The main entry points are:
- PQcancelCreate creates the PQcancelConn based on the original
connection (but does not establish an actual connection).
- PQcancelStart can be used to initiate non-blocking cancel requests,
using encryption if the original connection did so, which must be
pumped using
- PQcancelPoll.
- PQcancelReset puts a PQcancelConn back in state so that it can be
reused to send a new cancel request to the same connection.
- PQcancelBlocking is a simpler-to-use blocking API that still uses
encryption.
Additional functions are
- PQcancelStatus, mimicks PQstatus;
- PQcancelSocket, mimicks PQcancelSocket;
- PQcancelErrorMessage, mimicks PQerrorMessage;
- PQcancelFinish, mimicks PQfinish.
Author: Jelte Fennema-Nio <postgres@jeltef.nl>
Reviewed-by: Denis Laxalde <denis.laxalde@dalibo.com>
Discussion: https://postgr.es/m/AM5PR83MB0178D3B31CA1B6EC4A8ECC42F7529@AM5PR83MB0178.EURPRD83.prod.outlook.com
Most callers of strerror() are removed from the backend code. The
remaining callers require special handling with a saved errno from a
previous system call. The frontend code still needs strerror() where
error states need to be handled outside of fprintf.
Note that pg_regress is not changed to use %m as the TAP output may
clobber errno, since those functions call fprintf() and friends before
evaluating the format string.
Support for %m in src/port/snprintf.c has been added in d6c55de1f9,
hence all the stable branches currently supported include it.
Author: Dagfinn Ilmari Mannsåker
Discussion: https://postgr.es/m/87sf13jhuw.fsf@wibble.ilmari.org
Tom Lane