There were two spots in pgstat_read_statsfile() where is was possible to
finish with a null-pointer-dereference crash for custom pgstats kinds:
- When reading stats for a fixed-numbered stats entry.
- When reading a variable stats entry with name serialization.
For both cases, these issues were reachable by starting a server after
changing shared_preload_libraries so as the stats written previously
could not be loaded.
The code is changed so as the stats are ignored in this case, like the
other code paths doing similar sanity checks. Two WARNINGs are added to
be able to debug these issues. A test is added for the case of
fixed-numbered stats with the module injection_points.
Oversights in 7949d95945, spotted while looking at a different report.
Discussion: https://postgr.es/m/Ztj0Jftsn4xXuXtl@paquier.xyz
This fixes defects with installcheck for TAP tests that expect the
module injection_points to exist in an installation, but the contents of
src/test/modules are not installed by default with installcheck. This
would cause, for example, failures under installcheck-world for a build
with injection points enabled, when the contents of src/test/modules/
are not installed.
The availability of the module can be done with a scan of
pg_available_extension. This has been introduced in 2cdcae9da6, and
it is refactored here as a new routine in Cluster.pm.
Tests are changed in different ways depending on what they need:
- The libpq TAP test sets up a node even without injection points, so it
is enough to check that CREATE EXTENSION can be used. There is no need
for the variable enable_injection_points.
- In test_misc, 006_signal_autovacuum requires a runtime check.
- 041_checkpoint_at_promote in recovery tests and 005_timeouts in
test_misc are updated to use the routine introduced in Cluster.pm.
- test_slru's 001_multixact, injection_points's 001_stats and
modules/gin/ do not require a check as these modules disable
installcheck entirely.
Discussion: https://postgr.es/m/ZtesYQ-WupeAK7xK@paquier.xyz
These tests depend on the test module injection_points to be installed,
but it may not be available as the contents of src/test/modules/ are not
installed by default.
This commit adds a workaround based on a scan of pg_available_extensions
to check if the extension is available, skipping the test if it is not.
This allows installcheck to work transparently.
There are more tests impacted by this problem on HEAD, but for now this
addresses only the tests that exist on HEAD and v17 as the release is
close by.
Reported-by: Maxim Orlov
Discussion: https://postgr.es/m/CACG=ezZkoT-pFz6a9XnyToiuR-Wg8fGELqHLoyBodr+2h-77qA@mail.gmail.com
Backpatch-through: 17
Rather than the SQL injection_points_load(), this commit changes the
injection point test introduced in 768a9fd553 to rely on the two
macros INJECTION_POINT_LOAD() and INJECTION_POINT_CACHED(), that have
been originally introduced for the sake of this test.
This runs the test as a two-step process: load the injection point, then
run its callback directly from the local cache loaded. What the test
did originally was also fine, but the point here is to have an example
in core of how to use these new macros.
While on it, fix the header ordering in multixact.c, as pointed out by
Alexander Korotkov. This was an oversight in 768a9fd553.
Per discussion with Álvaro Herrera.
Author: Michael Paquier
Discussion: https://postgr.es/m/ZsUnJUlSOBNAzwW1@paquier.xyz
Discussion: https://postgr.es/m/CAPpHfduzaBz7KMhwuVOZMTpG=JniPG4aUosXPZCxZydmzq_oEQ@mail.gmail.com
This GUC controls if cumulative statistics are enabled or not in the
module. Custom statistics require the module to be loaded with
shared_preload_libraries, hence this GUC is made PGC_POSTMASTER. By
default, the stats are disabled. 001_stats.pl is updated to enable the
statistics, as it is the only area where these are required now.
This will be used by an upcoming change for the injection point test
added by 768a9fd553 where stats should not be used, as the test runs a
point callback in a critical section. And the module injection_points
will need to be loaded with shared_preload_libraries there.
Per discussion with Álvaro Herrera.
Author: Michael Paquier
Discussion: https://postgr.es/m/ZsUnJUlSOBNAzwW1@paquier.xyz
This commits adds callbacks to initialize the shared memory state of the
module when loaded with shared_preload_libraries. This is necessary to
be able to update the test introduced in 768a9fd553 to use the macros
INJECTION_POINT_{LOAD,CACHED}() rather than a SQL function in the module
injection_points forcing a load, as this test runs a callback in a
critical section where no memory allocation should happen.
Initializing the shared memory state of the module while loading
provides a strict control on the timing of its allocation. If the
module is not loaded at startup, it will use a GetNamedDSMSegment()
instead to initialize its shmem state on-the-fly.
Per discussion with Álvaro Herrera.
Author: Michael Paquier
Discussion: https://postgr.es/m/ZsUnJUlSOBNAzwW1@paquier.xyz
ab02d702ef has removed from the backend the code able to support the
unloading of modules, because this has never worked. This removes the
last references to _PG_fini(), that could be used as a callback for
modules to manipulate the stack when unloading a library.
The test module ldap_password_func had the idea to declare it, doing
nothing. The function declaration in fmgr.h is gone.
It was left around in 2022 to avoid breaking extension code, but at this
stage there are also benefits in letting extension developers know that
keeping the unloading code is pointless and this move leads to less
maintenance.
Reviewed-by: Tom Lane, Heikki Linnakangas
Discussion: https://postgr.es/m/ZsQfi0AUJoMF6NSd@paquier.xyz
Before commit a0e0fb1ba5, multixact.c contained a case in the
multixact-read path where it would loop sleeping 1ms each time until
another multixact-create path completed, which was uncovered by any
tests. That commit changed the code to rely on a condition variable
instead. Add a test now, which relies on injection points and "loading"
thereof (because of it being in a critical section), per commit
4b211003ec.
Author: Andrey Borodin <x4mmm@yandex-team.ru>
Reviewed-by: Michaël Paquier <michael@paquier.xyz>
Discussion: https://postgr.es/m/0925F9A9-4D53-4B27-A87E-3D83A757B0E0@yandex-team.ru
This adds two counters to the fixed-numbered stats of injection points
to track the number of times injection points have been cached and
loaded from the cache, as of the additions coming from a0a5869a85 and
4b211003ec.
These should have been part of f68cd847fa, but I have lacked time and
energy back then, and it did not prevent the code to be a useful
template.
While on it, this commit simplifies the description of a few tests while
adding coverage for the new stats data.
Author: Yogesh Sharma
Discussion: https://postgr.es/m/3a6977f7-54ab-43ce-8806-11d5e15526a2@catprosystems.com
Like 75534436a4, this acts mainly as a template to show what can be
achieved with fixed-numbered stats (like WAL, bgwriter, etc.) with the
pluggable cumulative statistics APIs introduced in 7949d95945.
Fixed-numbered stats are defined in their own file, named
injection_stats_fixed.c, separated entirely from the variable-numbered
case in injection_stats.c. This is mainly for clarity as having both
examples in the same file would be confusing.
Note that this commit uses the helper routines added in 2eff9e678d.
The stats stored track globally the number of times injection points
have been attached, detached or run. Two more fields should be added
later for the number of times a point has been cached or loaded, but
what's here is enough as a template.
More TAP tests are added, providing coverage for fixed-numbered custom
stats.
Author: Michael Paquier
Reviewed-by: Dmitry Dolgov, Bertrand Drouvot
Discussion: https://postgr.es/m/Zmqm9j5EO0I4W8dx@paquier.xyz
This acts as a template of what can be achieved with the pluggable
cumulative stats APIs introduced in 7949d95945 for the
variable-numbered case where stats entries are stored in the pgstats
dshash, while being potentially useful on its own for injection points,
say to add starting and/or stopping conditions based on the statistics
(want to trigger a callback after N calls, for example?).
Currently, the only data gathered is the number of times an injection
point is run. More fields can always be added as required. All the
routines related to the stats are located in their own file, called
injection_stats.c in the test module injection_points, for clarity.
The stats can be used only if the test module is loaded through
shared_preload_libraries. The key of the dshash uses InvalidOid for the
database, and an int4 hash of the injection point name as object ID.
A TAP test is added to provide coverage for the new custom cumulative
stats APIs, showing the persistency of the data across restarts, for
example.
Author: Michael Paquier
Reviewed-by: Dmitry Dolgov, Bertrand Drouvot
Discussion: https://postgr.es/m/Zmqm9j5EO0I4W8dx@paquier.xyz
The tests had a race condition if autovacuum was set to off. Instead we
create all the tables we are interested in with autovacuum disabled, so
they are only ever touched when in danger of wraparound.
Discussion: https://postgr.es/m/3e2cbd24-f45e-4b2b-ba83-8149214f0a4d@dunslane.net
Masahiko Sawada (slightly tweaked by me)
Backpatch to release 17 where these tests were introduced.
Previously, TidStoreIterateNext() would expand the set of offsets for
each block into an internal buffer that it overwrote each time. In
order to be able to collect the offsets for multiple blocks before
working with them, change the contract. Now, the offsets are obtained
by a separate call to TidStoreGetBlockOffsets(), which can be called at
a later time. TidStoreIteratorResult objects are safe to copy and store
in a queue.
Reviewed-by: Noah Misch <noah@leadboat.com>
Discussion: https://postgr.es/m/CAAKRu_bbkmwAzSBgnezancgJeXrQZXy4G4kBTd+5=cr86H5yew@mail.gmail.com
This new macro is able to perform a direct lookup from the local cache
of injection points (refreshed each time a point is loaded or run),
without touching the shared memory state of injection points at all.
This works in combination with INJECTION_POINT_LOAD(), and it is better
than INJECTION_POINT() in a critical section due to the fact that it
would avoid all memory allocations should a concurrent detach happen
since a LOAD(), as it retrieves a callback from the backend-private
memory.
The documentation is updated to describe in more details how to use this
new macro with a load. Some tests are added to the module
injection_points based on a new SQL function that acts as a wrapper of
INJECTION_POINT_CACHED().
Based on a suggestion from Heikki Linnakangas.
Author: Heikki Linnakangas, Michael Paquier
Discussion: https://postgr.es/m/58d588d0-e63f-432f-9181-bed29313dece@iki.fi
This test was added by commit d2b74882ca, but fails if
log_error_verbosity is set to verbose. Adjust the regex that checks the
error message to allow for it containing an SQL status code.
This commit provides testig coverage for ccd38024bc, checking that a
role granted pg_signal_autovacuum_worker is able to stop a vacuum
worker.
An injection point with a wait is placed at the beginning of autovacuum
worker startup to make sure that a worker is still alive when sending
and processing the signal sent.
Author: Anthony Leung, Michael Paquier, Kirill Reshke
Reviewed-by: Andrey Borodin, Nathan Bossart
Discussion: https://postgr.es/m/CALdSSPiQPuuQpOkF7x0g2QkA5eE-3xXt7hiJFvShV1bHKDvf8w@mail.gmail.com
Test result files might be checked out using Unix or Windows style line
endings, depening on git flags, so on Windows we use the
--strip-trailing-cr flag to tell diff to ignore line endings
differences.
The flag is added to the diff invocation for the test_json_parser module
tests and the pg_bsd_indent tests. in pg_regress.c we replace the
current use of the "-w" flag, which ignore all white space differences,
with this one which only ignores line end differences.
Discussion: https://postgr.es/m/20240707052030.r77hbdkid3mwksop@awork3.anarazel.de
This can be used to load an injection point and prewarm the
backend-level cache before running it, to avoid issues if the point
cannot be loaded due to restrictions in the code path where it would be
run, like a critical section where no memory allocation can happen
(load_external_function() can do allocations when expanding a library
name).
Tests can use a macro called INJECTION_POINT_LOAD() to load an injection
point. The test module injection_points gains some tests, and a SQL
function able to load an injection point.
Based on a request from Andrey Borodin, who has implemented a test for
multixacts requiring this facility.
Reviewed-by: Andrey Borodin
Discussion: https://postgr.es/m/ZkrBE1e2q2wGvsoN@paquier.xyz
This commit removes unused variables and routines from some perl code
that have accumulated across the years. This touches the following
areas:
- Wait event generation script.
- AdjustUpgrade.pm.
- TAP perl code
Author: Alexander Lakhin
Reviewed-by: Dagfinn Ilmari Mannsåker
Discussion: https://postgr.es/m/70b340bc-244a-589d-ef8b-d8aebb707a84@gmail.com
Make the isolation harness recognize injection_points wait events as a
type of blocked state. Test an extant inplace-update bug.
Reviewed by Robert Haas and Michael Paquier.
Discussion: https://postgr.es/m/20240512232923.aa.nmisch@google.com
Commit 534287403 invented SHARED_DEPENDENCY_INITACL entries in
pg_shdepend, but installed them only for non-owner roles mentioned
in a pg_init_privs entry. This turns out to be the wrong thing,
because there is nothing to cue REASSIGN OWNED to go and update
pg_init_privs entries when the object's ownership is reassigned.
That leads to leaving dangling entries in pg_init_privs, as
reported by Hannu Krosing. Instead, install INITACL entries for
all roles mentioned in pg_init_privs entries (except pinned roles),
and change ALTER OWNER to not touch them, just as it doesn't
touch pg_init_privs entries.
REASSIGN OWNED will now substitute the new owner OID for the old
in pg_init_privs entries. This feels like perhaps not quite the
right thing, since pg_init_privs ought to be a historical record
of the state of affairs just after CREATE EXTENSION. However,
it's hard to see what else to do, if we don't want to disallow
dropping the object's original owner. In any case this is
better than the previous do-nothing behavior, and we're unlikely
to come up with a superior solution in time for v17.
While here, tighten up some coding rules about how ACLs in
pg_init_privs should never be null or empty. There's not any
obvious reason to allow that, and perhaps asserting that it's
not so will catch some bugs. (We were previously inconsistent
on the point, with some code paths taking care not to store
empty ACLs and others not.)
This leaves recordExtensionInitPrivWorker not doing anything
with its ownerId argument, but we'll deal with that separately.
catversion bump forced because of change of expected contents
of pg_shdepend when pg_init_privs entries exist.
Discussion: https://postgr.es/m/CAMT0RQSVgv48G5GArUvOVhottWqZLrvC5wBzBa4HrUdXe9VRXw@mail.gmail.com
DeleteInitPrivs did not get the memo about how, when dropping a
whole object (with subid == 0), you should drop entries relating
to its sub-objects too. This is visible in the test_pg_dump test
case if one drops the extension at the end: the entry for
GRANT SELECT(col1) ON regress_pg_dump_table TO public;
was still present in pg_init_privs afterwards, although it was
pointing to a dangling table OID.
Noted while fooling with a fix for REASSIGN OWNED for pg_init_privs
entries. This bug is aboriginal in the pg_init_privs feature
though, and there seems no reason not to back-patch the fix.
The do_set_block_offsets() and other functions accessing the tidstore
did not check if the tidstore was NULL. This led to a segmentation
fault when these functions are called without calling the
test_create().
This commit adds NULL checks in relevant functions of test_tidstore to
raise an error instead if the tidstore is not initialized.
Bug: #18483
Reported-by: Alexander Kozhemyakin
Reviewed-by: Michael Paquier
Discussion: https://postgr.es/m/18483-30bfff42de238000%40postgresql.org
test_predtest() neglected to consider the possibility that
SPI_plan_get_cached_plan would return NULL. This led to a core
dump if the input (incorrectly) contains more than one SQL
command.
While here, let's expend more than zero effort on the error
message for this case and nearby ones.
Per (half of) bug #18483 from Alexander Kozhemyakin.
Back-patch to all supported branches, not because this is
very significant (it's merely test scaffolding) but to make
our world a bit safer for fuzz testing.
Discussion: https://postgr.es/m/18483-30bfff42de238000@postgresql.org
After further review, we want to move in the direction of always
quoting GUC names in error messages, rather than the previous (PG16)
wildly mixed practice or the intermittent (mid-PG17) idea of doing
this depending on how possibly confusing the GUC name is.
This commit applies appropriate quotes to (almost?) all mentions of
GUC names in error messages. It partially supersedes a243569bf6 and
8d9978a717, which had moved things a bit in the opposite direction
but which then were abandoned in a partial state.
Author: Peter Smith <smithpb2250@gmail.com>
Discussion: https://www.postgresql.org/message-id/flat/CAHut%2BPv-kSN8SkxSdoHano_wPubqcg5789ejhCDZAcLFceBR-w%40mail.gmail.com
Run pgindent, pgperltidy, and reformat-dat-files.
The pgindent part of this is pretty small, consisting mainly of
fixing up self-inflicted formatting damage from patches that
hadn't bothered to add their new typedefs to typedefs.list.
In order to keep it from making anything worse, I manually added
a dozen or so typedefs that appeared in the existing typedefs.list
but not in the buildfarm's list. Perhaps we should formalize that,
or better find a way to get those typedefs into the automatic list.
pgperltidy is as opinionated as always, and reformat-dat-files too.
There are some problems with the new way to handle these constraints
that were detected at the last minute, and require fixes that appear too
invasive to be doing this late in the cycle. Revert this (again) for
now, we'll try again with these problems fixed.
The following commits are reverted:
b0e96f3119 Catalog not-null constraints
9b581c5341 Disallow changing NO INHERIT status of a not-null constraint
d0ec2ddbe0 Fix not-null constraint test
ac22a9545c Move privilege check to the right place
b0f7dd915b Check stack depth in new recursive functions
3af7217942 Update information_schema definition for not-null constraints
c3709100be Fix propagating attnotnull in multiple inheritance
d9f686a72e Fix restore of not-null constraints with inheritance
d72d32f52d Don't try to assign smart names to constraints
0cd711271d Better handle indirect constraint drops
13daa33fa5 Disallow NO INHERIT not-null constraints on partitioned tables
d45597f72f Disallow direct change of NO INHERIT of not-null constraints
21ac38f498 Fix inconsistencies in error messages
Discussion: https://postgr.es/m/202405110940.joxlqcx4dogd@alvherre.pgsql
This commit fixes a race condition between injection point run and
detach, where a point detached by a backend and concurrently running in
a second backend could cause the second backend to do an incorrect
condition check. This issue happens because the second backend
retrieves the callback information in a first step in the shmem hash
table for injection points, and the condition in a second step within
the callback. If the point is detached between these two steps, the
condition would be removed, causing the point to run while it should
not. Storing the condition in the new private_data area introduced in
33181b48fd ensures that the condition retrieved is consistent with its
callback.
This commit leads to a lot of simplifications in the module
injection_points, as there is no need to handle the runtime conditions
inside it anymore. Runtime conditions have no more a maximum number.
Per discussion with Noah Misch.
Reviewed-by: Noah Misch
Discussion: https://postgr.es/m/20240509031553.47@rfd.leadboat.com
This commit extends the backend-side infrastructure of injection points
so as it becomes possible to register some input data when attaching a
point. This private data can be registered with the function name and
the library name of the callback when attaching a point, then it is
given as input argument to the callback. This gives the possibility for
modules to pass down custom data at runtime when attaching a point
without managing that internally, in a manner consistent with the
callback entry retrieved from the hash shmem table storing the injection
point data.
InjectionPointAttach() gains two arguments, to be able to define the
private data contents and its size.
A follow-up commit will rely on this infrastructure to close a race
condition with the injection point detach in the module
injection_points.
While on it, this changes InjectionPointDetach() to return a boolean,
returning false if a point cannot be detached. This has been mentioned
by Noah as useful when it comes to implement more complex tests with
concurrent point detach, solid with the automatic detach done for local
points in the test module.
Documentation is adjusted in consequence.
Per discussion with Noah Misch.
Reviewed-by: Noah Misch
Discussion: https://postgr.es/m/20240509031553.47@rfd.leadboat.com
It turns out that we broke this in commit e5bc9454e, because
the code was assuming that no dependent types would appear
among the extension's direct dependencies, and now they do.
This isn't terribly hard to fix: just skip dependent types,
expecting that we will recurse to them when we process the parent
object (which should also be among the direct dependencies).
But a little bit of refactoring is needed so that we can avoid
duplicating logic about what is a dependent type.
Although there is some testing of ALTER EXTENSION SET SCHEMA,
it failed to cover interesting cases, so add more tests.
Discussion: https://postgr.es/m/930191.1715205151@sss.pgh.pa.us
json_lex_string() relies on pg_encoding_mblen_bounded() to point to the
end of a JSON string when generating an error message, and the input it
uses is not guaranteed to be null-terminated.
It was possible to walk off the end of the input buffer by a few bytes
when the last bytes consist of an incomplete multi-byte sequence, as
token_terminator would point to a location defined by
pg_encoding_mblen_bounded() rather than the end of the input. This
commit switches token_terminator so as the error uses data up to the
end of the JSON input.
More work should be done so as this code could rely on an equivalent of
report_invalid_encoding() so as incorrect byte sequences can show in
error messages in a readable form. This requires work for at least two
cases in the JSON parsing API: an incomplete token and an invalid escape
sequence. A more complete solution may be too invasive for a backpatch,
so this is left as a future improvement, taking care of the overread
first.
A test is added on HEAD as test_json_parser makes this issue
straight-forward to check.
Note that pg_encoding_mblen_bounded() no longer has any callers. This
will be removed on HEAD with a separate commit, as this is proving to
encourage unsafe coding.
Author: Jacob Champion
Discussion: https://postgr.es/m/CAOYmi+ncM7pwLS3AnKCSmoqqtpjvA8wmCdoBtKA3ZrB2hZG6zA@mail.gmail.com
Backpatch-through: 13
Injection points created under injection_points_set_local() are cleaned
up by a shmem_exit() callback. The spinlock used by the module would
be hold while calling InjectionPointDetach(), which is incorrect as
spinlocks should avoid external calls while hold.
This commit changes the shmem_exit() callback to detach the points in
three steps with the spinlock acquired twice, knowing that the
injection points should be around with the conditions related to them:
- Scans for the points to detach in a first loop, while holding the
spinlock.
- Detach them.
- Remove the registered conditions.
It is still possible for other processes to detach local points
concurrently of the callback. I have wanted to restrict the detach, but
Noah has mentioned that he has in mind some cases that may require this
capability. No tests in the tree based on injection points need that
currently.
Thinko in f587338dec.
Reported-by: Noah Misch
Reviewed-by: Noah Misch
Discussion: https://postgr.es/m/20240501231214.40@rfd.leadboat.com
If the bootstrap superuser's name requires quoting, regroleout
will supply double quotes ... but the result of CURRENT_USER
is just the literal name. Apply quote_ident() to ensure a match.
Per Andrew Dunstan's off-list investigation of buildfarm member
prion's failures.
