Variable-numbered pgstats entries currently all live in one shared dshash
table. This works for stats kinds with each one having a rather "small"
number of entries, but there are limits if a stats kind wants to
allocate a large number of entries, especially if these need to be
scanned on a periodic basis.
This commit adds a new own_hash option for variable-numbered stats
kinds, allowing these to use an independent dshash table backed by its
own DSA area. This gives a way for stats kind to leverage their efforts
across more than one shared hashtable, splitting the load, with the same
guarantees as the main hashtable. Fixed-sized stats kind cannot use
this option. All the hashtables use the same key pattern,
PgStat_HashKey.
The motivation of this change is pg_stat_statements, to add it as a
custom stats kind. Out-of-core custom stats kind, or even built-in
stats kinds can also use this option.
At startup, each stats kind that opts in creates one initial DSA area in
shared memory at startup. At attach time, each registered
variable-numbered stats kind is mapped to either the shared hash and DSA
or its dedicated hash and DSA. The local references are extended to
track which hash table to use on a per-kind basis, with arrays based on
the kind ID.
The test module test_custom_stats is extended and refactored to provide
coverage for this new own_hash.
The idea of being able to use more than one hash table for pgstats has
been originally mentioned off-list during a conference by Robert Haas
(note: if my memory does not fail me here).
Author: Sami Imseih <samimseih@gmail.com>
Reviewed-by: Michael Paquier <michael@paquier.xyz>
Reviewed-by: Bertrand Drouvot <bertranddrouvot.pg@gmail.com>
Discussion: https://postgr.es/m/CAA5RZ0supQBxSkh=CWB39=j+cL3hHcLPki3tcBk0B1r4fesg_g@mail.gmail.com
It's a bad idea to sleep while holding locks, because you might block
another process that wants to acquire the same lock. Furthermore,
it's not safe to call ProcessConfigFile() in a critical section or
while holding locks. So avoid calling vacuum_delay_point() while
holding locks, and also make vacuum_delay_point() return quickly
without sleeping if it's called while interrupts cannot be processed.
To find the vacuum_delay_point() calls that were made while holding
locks, I added "Assert(INTERRUPTS_CAN_BE_PROCESSED())" in
vacuum_delay_point() and ran the regression tests. I didn't include
that Assert in this commit, because there might be more places that do
that that are not covered by the regression tests, including
extensions.
In ginInsertCleanup(), there was already another vacuum_delay_point()
call later in the loop, while not holding any locks, so just remove
the other call that was made while holding the lock.
In hashbucketcleanup(), move the vacuum_delay_point() call up the
stack to its caller. The call in hashbucketcleanup() was not able to
handle interrupts because it held a lock, and because there were no
vacuum_delay_point() or CHECK_FOR_INTERRUPTS() calls in the caller's
loop, the whole hash index vacuuming phase was uninterruptible by
pending shutdown or query cancel. Now it can be interrupted between
buckets. Unfortunately, hashbucketcleanup() has to process all the
bucket's pages in one go without pausing; fixing that would require
changing how hashbucketcleanup()'s lock chaining works.
In acquire_sample_rows(), move the vacuum_delay_point() call in the
loop to between pages, to a time where we're not holding the buffer
lock.
In master, also remove a misleading CHECK_FOR_INTERRUPTS() call from
pgstat_write_statsfile(). It was a no-op because we're always holding
a lock on the current dshash partition at that point. A call that
does nothing is harmless, but it makes you think that the loop is
interruptible when in reality it's not.
Author: Kevin Rocker <me@kevinrocker.com>
Author: Andrey Borodin <amborodin@acm.org>
Author: Mostafa <mostafa.nabil.nafie@gmail.com>
Reported-by: Greg Burd <greg@burd.me>
Reported-by: Sergei Kornilov <sk@zsrv.org>
Reviewed-by: Kirill Reshke <reshkekirill@gmail.com>
Reviewed-by: Neil Chen <carpenter.nail.cz@gmail.com>
Tested-by: Greg Burd <greg@burd.me>
Discussion: https://postgr.es/m/19628-c2b17d358181a1ea@postgresql.org
Discussion: https://postgr.es/m/492c6247-43d3-477b-8981-fb0c56767b38%40app.fastmail.com
Backpatch-through: 14
8191e0c16a made pgstat_init_entry() return NULL on DSA allocation
failure so callers could delete a just-inserted hash entry. That works
when dsa_allocate_extended(DSA_ALLOC_NO_OOM) returns InvalidDsaPointer,
but not when the creation of a new DSM segment raises ERROR.
Under this type of failure, pgstat_init_entry() would mark the hash
entry live before allocating the body and cause an inconsistent state in
shared memory. The next backend trying to acquire the existing,
partially-initialized entry in pgstat_acquire_entry_ref() would crash,
taking down the server. Some of the public reports mentioned ENOSPC on
POSIX shared memory as one initial cause.
This commit refactors the pgstats entry initialization to happen in two
phases:
- Allocate the DSA chunk.
- Insert the hash entry with a valid segment.
If a concurrent insert wins, the allocation of the first phase needs an
extra set of dsa_free() calls (pulling back a DSM into its free list).
As this changes the signature of pgstat_init_entry(), no backpatch is
done.
Author: Yuriy Grigoryev <ju.grigorev@ftdata.ru>
Author: Michael Paquier <michael@paquier.xyz>
Discussion: https://postgr.es/m/ddc3ecfb01ce4e9698b23cc59767f016@localhost.localdomain
pgstat_init_entry() can fail after its hash entry has been inserted, in
which case the caller has to remove the entry, as done by
pgstat_get_entry_ref() since 8191e0c16a. pgstat_read_statsfile() was
not exactly doing that, leaving an entry that could still point to an
InvalidDsaPointer.
There are no consequences here, even after 8191e0c16a. The stats are
loaded from disk by the startup process, where an ERROR gets promoted to
a FATAL, cleaning up shared memory. So this is mostly an argument of
consistency with the normal entry creation path, especially if someone
decides to downgrade this ERROR to a WARNING in pgstat_read_statsfile().
Author: Grigorev Jurij <ju.grigorev@ftdata.ru>
Discussion: https://postgr.es/m/d55ecaf911844d53bd0a931751dce582@localhost.localdomain
This new structure is split depending on the stats kind it deals with:
- PGSTAT_KIND_RELATION, for tables.
- PGSTAT_KIND_INDEX, for indexes.
This change designs a cleaner barrier for the handling of stats data
related to tables and indexes, by being able to track precisely what are
the counters used by one or the other for pending data. Using a common
ground for both eases the tracking of Relations in the relcache, with a
structure based on a union separated by stats kind.
The code originally pointed to some counters that may not be used at
all. For example, indexes have no need for the tracking of
sub-transaction data, still the code implied that these could be touched
for an index. A consequence is that PgStat_TableXactStatus is not
renamed, as it is not used by indexes.
Some asserts are added as an extra layer of protection to prevent the
update of fields that should not be touched. find_tabstat_entry() and
find_tabstat_entry_kind() are removed, replaced by a single
find_relstat_entry_kind() able to work for indexes and the rest.
72a6dad1c9 took care of the shared memory and on-disk part of the
split. This focuses on the pending data part at backend level.
Author: Michael Paquier <michael@paquier.xyz>
Reviewed-by: Bertrand Drouvot <bertranddrouvot.pg@gmail.com>
Discussion: https://postgr.es/m/f572abe7-a1bb-e13b-48c7-2ca150546822@gmail.com
This commit splits the relation and index stats data, where the index
data is now moved to a new kind called PGSTAT_KIND_INDEX. A benefit of
this refactoring is a reduction of the shared memory required for each
index, the new PgStat_StatIdxEntry including only the fields required
for indexes. This leads to less data flushed to disk on shutdown.
Another benefit is that the addition of new fields for relation-level
stats is easier to think about. Note that indexes have no need for most
of the xact-level fields that relations care about, clarifying the role
of each counter for each relkind.
System views are updated to use the index-level functions. Note this
split means a possible break for tools that had the idea to call the
SQL functions for relation stats (not documented), as now these would
return 0 when queried for an index. These tools can use the new index
functions to achieve the same result.
As of this change, the pending statistics are still controlled by
PgStat_TableStatus for both relation and index kinds. This is left as a
follow-up piece, this one being good enough on its own. This commit is
focused on the changes required for the SQL interface, shared memory
management and the on-disk format.
Bump catalog version.
Bump PGSTAT_FILE_FORMAT_ID.
Author: Bertrand Drouvot <bertranddrouvot.pg@gmail.com>
Author: Michael Paquier <michael@paquier.xyz>
Discussion: https://postgr.es/m/f572abe7-a1bb-e13b-48c7-2ca150546822@gmail.com
This commit adds pgstat_prep_pending_from_entry_ref(), a new pgstats
routine that is able to prepare an existing PgStat_EntryRef to receive
pending stats. This split gives a way for callers to obtain first a
reference via pgstat_get_entry_ref(), then set up pending data as two
separate, distinctive, steps.
Previously, the only way to get an entry reference with pending data
ready was pgstat_prep_pending_entry(), which bundles lookup, creation,
and pending setup in a single call. Callers that need finer control
over the entry creation had no way to attach pending data to an
already-obtained entry reference. One case where this has shown to
matter for a stats kind is where one wants to check some capacity (for
example where a GUC bounds the maximum numer of entries allowed) before
deciding if a new entry should be created. So this split can help in
reducing calls to pgstat_get_entry_ref(), meaning less shmem hash table
lookups.
The only logical ordering change is that pgStatPendingContext is
initialized after calling pgstat_get_entry_ref() in
pgstat_prep_pending_entry(). This does not matter in practice.
pgstat_prep_pending_entry() is refactored to use the new function
internally. All the existing callers are unchanged. Existing
out-of-core custom stats kinds should see no impact.
Author: Sami Imseih <samimseih@gmail.com>
Reviewed-by: Kyotaro Horiguchi <horikyota.ntt@gmail.com>
Reviewed-by: Michael Paquier <michael@paquier.xyz>
Discussion: https://postgr.es/m/CAA5RZ0sV+TsLejUMhAM=PJoOm8u-t8ru7B67KvyLCy=19sM87g@mail.gmail.com
Contrary to the from_serialized_data callback used by the pgstats reads
at startup, the to_serialized_data callback used for the pgstats writes
matched with pgstat_write_statsfile(), by not returning a boolean
status, expecting a ferror() failure to deal with the discard of the
stats file should an error happen while writing the stats. This was
slightly confusing designed this way.
Things are changed in this commit with:
- to_serialized_data now returns a boolean status on a write failure.
pgstat_write_statsfile() detects that and switches to failure mode
instead of continuing to process the entries to write, speeding up the
shutdown.
- pgstat_write_statsfile() now uses STATS_DISCARD if a failure happens,
to let the registered callbacks directly know that something is wrong,
and that things need to be cleaned up. This gives a better error path
detection for custom stats kinds. For example, they do not have to rely
solely on the expectation of an ferror() for an auxiliary file.
This new set of behaviors matches with what is already done in
pgstat_read_statsfile() for the finish() callback (DISCARD on failure,
READ on success) and the from_serialized_data with a status returned.
Author: Sami Imseih <samimseih@gmail.com>
Discussion: https://postgr.es/m/CAA5RZ0sMgOvuhpb2P=KSJOjgjC6AfUu+GYcu9mHar-y_Xtd=Pg@mail.gmail.com
Backpatch-through: 19
This reverts commit ed823da128, that has made pgstat_write_chunk() and
pgstat_read_chunk() available for public use. These routines do not
have a symmetric API definition across reads and writes, with the write
part returning a void status, deferring an error detection once all the
stats entries have been processed with an ferror(), and the read part
returning a boolean status.
These routines are just tiny wrappers around fread() and fwrite(), and
extensions can just define they own routines instead of relying on the
same facilities as the core pgstat.c. This commit removes their
declaration from the public headers, to reduce the confusion.
test_custom_stats is updated to use its own read/write routines.
Perhaps something better could be designed in the future; trying to do
so for v19 is not feasable during beta.
Reported-by: Peter Eisentraut <peter@eisentraut.org>
Author: Sami Imseih <samimseih@gmail.com>
Discussion: https://postgr.es/m/a4a8e9af-3eaf-4bbf-9b21-21620f3fc434@eisentraut.org
Backpatch-through: 19
pgstat_register_kind() did not validate that required callbacks are
set, which could lead to NULL pointer dereferences when trying to
register a stats kind. This adds a couple of checks:
- Fox fixed-sized kinds, init_shmem_cb, reset_all_cb, and snapshot_cb
are required.
- For variable-sized kinds, flush_pending_cb is called when there is
pending data, pending_size being required.
These issues should be easy to notice for someone developing an
extension that relies on the custom pgstats APIs. No backpatch is done
as it is mainly a life improvement.
Author: Sami Imseih <samimseih@gmail.com>
Discussion: https://postgr.es/m/CAA5RZ0uNoe=xT7QsU1K0mMRg-QAwPtupPWZ2J3weM2PjVL2tiA@mail.gmail.com
pgstat_drop_entry_internal() generates an ERROR if facing a pgstats
entry already marked as dropped. With a workload doing a lot of
concurrent CALL and DROP/CREATE PROCEDURE, it could be possible for
AtEOXact_PgStat_DroppedStats(), that wants to do transactional drops, to
find entries that are already dropped, after a commit record has been
written. In this case, ERRORs are upgraded to PANIC, taking down the
server.
This issue is fixed by making pgstat_drop_entry() optionally more
tolerant to concurrent drops, adding to the routine a missing_ok option
to make some of its callers more tolerant (spoiler: some of the callers
want a strict behavior, like replication slots and backend stats).
pgstat_drop_entry_internal() cannot be called anymore for an entry
marked as dropped, hence its error is replaced by an assertion.
Functions are handled as a special case in core; this problem could also
apply to custom stats kinds depending on what an extension does.
track_functions is costly when enabled (disabled by default), which is
perhaps the main reason why this has not be found yet.
A similar version of this patch has been proposed by Sami Imseih on a
different thread for a feature in development. This version has tweaked
here by me for the sake of fixing this issue.
Reported-by: zhanglihui <zlh21343@163.com>
Author: Sami Imseih <samimseih@gmail.com>
Author: Michael Paquier <michael@paquier.xyz>
Reviewed-by: Ayush Tiwari <ayushtiwari.slg01@gmail.com>
Discussion: https://postgr.es/m/19520-73873648d44793cf@postgresql.org
Backpatch-through: 15
Presently, relation_needs_vacanalyze() unconditionally frees the
pgstat entry returned by pgstat_fetch_stat_tabentry_ext(). This
behavior was first added by commit 02502c1bca to avoid memory
leakage in autovacuum. While this is fine for autovacuum since it
forces stats_fetch_consistency to "none", it is not okay for other
callers that use "cache" or "snapshot". This manifests as a
double-free when pg_stat_autovacuum_scores is called multiple times
in the same transaction.
To fix, add a "bool *may_free" parameter to
pgstat_fetch_stat_tabentry_ext() that returns whether it is safe
for the caller to explicitly pfree() the result. If a caller would
rather leave it to the memory context machinery to free the result,
it can pass NULL as the "may_free" argument (or just ignore its
value).
Oversight in commit 87f61f0c82.
Reported-by: Tender Wang <tndrwang@gmail.com>
Reported-by: Alexander Lakhin <exclusion@gmail.com>
Suggested-by: Andres Freund <andres@anarazel.de>
Suggested-by: Tom Lane <tgl@sss.pgh.pa.us>
Author: Sami Imseih <samimseih@gmail.com>
Discussion: https://postgr.es/m/CAHewXNkJKdwb3D5OnksrdOqzqUnXUEMpDam1TPW0vfUkW%3D7jUw%40mail.gmail.com
Discussion: https://postgr.es/m/5684f479-858e-4c5d-b8f5-bcf05de1f909%40gmail.com
This commit adds a new stats kind, called PGSTAT_KIND_LOCK, implementing
statistics for lock tags, as reported by pg_locks. The implementation
is fixed-sized, as the data is caped based on the number of lock tags in
LockTagType.
The new statistics kind records the following fields, providing insight
regarding lock behavior, while avoiding impact on performance-critical
code paths (such as fast-path lock acquisition):
- waits and wait_time: respectively track the number of times a lock
required waiting and the total time spent acquiring it. These metrics
are only collected once a lock is successfully acquired and after
deadlock_timeout has been exceeded.
fastpath_exceeded: counts how often a lock could not be acquired via
the fast path due to the max_locks_per_transaction slot limits.
A new view called pg_stat_lock can be used to access this data, coupled
with a SQL function called pg_stat_get_lock().
Bump stat file format PGSTAT_FILE_FORMAT_ID.
Bump catalog version.
Author: Bertrand Drouvot <bertranddrouvot.pg@gmail.com>
Reviewed-by: Andres Freund <andres@anarazel.de>
Reviewed-by: Michael Paquier <michael@paquier.xyz>
Discussion: https://postgr.es/m/aIyNxBWFCybgBZBS%40ip-10-97-1-34.eu-west-3.compute.internal
Cumulative stats kinds gain the capability to write additional per-entry
data when flushing the stats at shutdown, and read this data when
loading back the stats at startup. This can be fit for example in the
case of variable-length data (like normalized query strings), so as it
becomes possible to link the shared memory stats entries to data that is
stored in a different area, like a DSA segment.
Three new optional callbacks are added to PgStat_KindInfo, available to
variable-numbered stats kinds:
* to_serialized_data: writes auxiliary data for an entry.
* from_serialized_data: reads auxiliary data for an entry.
* finish: performs actions after read/write/discard operations. This is
invoked after processing all the entries of a kind, allowing extensions
to close file handles and clean up resources.
Stats kinds have the option to store this data in the existing pgstats
file, but can as well store it in one or more additional files whose
names can be built upon the entry keys. The new serialized callbacks
are called once an entry key is read or written from the main stats
file. A file descriptor to the main pgstats file is available in the
arguments of the callbacks.
Author: Sami Imseih <samimseih@gmail.com>
Co-authored-by: Michael Paquier <michael@paquier.xyz>
Reviewed-by: Chao Li <li.evan.chao@gmail.com>
Discussion: https://postgr.es/m/CAA5RZ0s9SDOu+Z6veoJCHWk+kDeTktAtC-KY9fQ9Z6BJdDUirQ@mail.gmail.com
This commit renames write_chunk and read_chunk to respectively
pgstat_write_chunk() and pgstat_read_chunk(), along with the *_s
convenience macros.
These are made available for plug-ins, so as any code that decides to
write and/or read stats data can rely on a single code path for this
work.
Extracted from a larger patch by the same author.
Author: Sami Imseih <samimseih@gmail.com>
Discussion: https://postgr.es/m/CAA5RZ0s9SDOu+Z6veoJCHWk+kDeTktAtC-KY9fQ9Z6BJdDUirQ@mail.gmail.com
It is possible to call pg_stat_reset_single_function_counters() for a
single function, but the reset time was missing the system view showing
its statistics. Like all the fields of pg_stat_user_functions, the GUC
track_functions needs to be enabled to show the statistics about
function executions.
Bump catalog version.
Bump PGSTAT_FILE_FORMAT_ID, as a result of the new field added to
PgStat_StatFuncEntry.
Author: Bertrand Drouvot <bertranddrouvot.pg@gmail.com>
Discussion: https://postgr.es/m/aONjnsaJSx-nEdfU@paquier.xyz
It is possible to call pg_stat_reset_single_table_counters() on a
relation (index or table) but the reset time was missing from the system
views showing their statistics.
This commit adds the reset time as an attribute of pg_stat_all_tables,
pg_stat_all_indexes, and other relations related to them.
Bump catalog version.
Bump PGSTAT_FILE_FORMAT_ID, as a result of the new field added to
PgStat_StatTabEntry.
Author: Bertrand Drouvot <bertranddrouvot.pg@gmail.com>
Reviewed-by: Sami Imseih <samimseih@gmail.com>
Reviewed-by: Michael Paquier <michael@paquier.xyz>
Discussion: https://postgr.es/m/aN8l182jKxEq1h9f@paquier.xyz
Stats kinds can set a new option called "track_entry_count" (disabled by
default, available for variable-numbered stats) that will make pgstats
track the number of entries that exist in its shared hashtable.
As there is only one code path where a new entry is added, and one code
path where entries are freed, the count tracking is straight-forward in
its implementation. Reads of these counters are optimistic, and may
change across two calls. The counter is incremented when an entry is
created (not when reused), and is decremented when an entry is freed
from the hashtable (marked for drop with its refcount reaching 0), which
is something that pgstats decides internally.
A first use case of this facility would be pg_stat_statements, where we
need to be able to cap the number of entries that would be stored in the
shared hashtable, based on its "max" GUC. The module currently relies
on hash_get_num_entries(), which offers a cheap way to count how many
entries are in its hash table, but we cannot do that in pgstats for
variable-sized stats kinds as a single hashtable is used for all the
stats kinds. Independently of PGSS, this is useful for other custom
stats kinds that want to cap, control, or track the number of entries
they have, without depending on a potentially expensive sequential scan
to know the number of entries while holding an extra exclusive lock.
Author: Michael Paquier <michael@paquier.xyz>
Reviewed-by: Sami Imseih <samimseih@gmail.com>
Reviewed-by: Keisuke Kuroda <keisuke.kuroda.3862@gmail.com>
Reviewed-by: Chao Li <li.evan.chao@gmail.com>
Discussion: https://postgr.es/m/aMPKWR81KT5UXvEr@paquier.xyz
This change is a tighter rework of 7d85d87f4d, which tried to improve
the code so as it would work should PgStat_HashKey gain new fields that
create padding bytes. However, the previous change is proving to not be
enough as some code paths of pgstats do not pass PgStat_HashKey by
reference (valgrind would warn when padding is added to the structure,
through a new field).
Per discussion, let's document and check that PgStat_HashKey has no
padding rather than try to complicate the code of pgstats so as it is
able to work around that.
This removes a couple of memset(0) calls that should not be required.
While on it, this commit adds a static assertion checking that no
padding is introduced in the structure, by checking that the size of
PgStat_HashKey matches with the sum of the size of all its fields.
The object ID part of the hash key is already 8 bytes, which should be
plenty enough already. A comment is added to discourage the addition of
new fields.
Author: Michael Paquier <michael@paquier.xyz>
Reviewed-by: Sami Imseih <samimseih@gmail.com>
Discussion: https://postgr.es/m/CAA5RZ0t9omat+HVSakJXwTMWvhpYFcAZb41RPWKwrKFUgmAFBQ@mail.gmail.com
A new pgstats entry is created as a two-step process:
- The entry is looked at in the shared hashtable of pgstats, and is
inserted if not found.
- When not found and inserted, its fields are then initialized. This
part include a DSA chunk allocation for the stats data of the new entry.
As currently coded, if the DSA chunk allocation fails due to an
out-of-memory failure, an ERROR is generated, leaving in the pgstats
shared hashtable an inconsistent entry due to the first step, as the
entry has already been inserted in the hashtable. These broken entries
can then be found by other backends, crashing them.
There are only two callers of pgstat_init_entry(), when loading the
pgstats file at startup and when creating a new pgstats entry. This
commit changes pgstat_init_entry() so as we use dsa_allocate_extended()
with DSA_ALLOC_NO_OOM, making it return NULL on allocation failure
instead of failing. This way, a backend failing an entry creation can
take appropriate cleanup actions in the shared hashtable before throwing
an error. Currently, this means removing the entry from the shared
hashtable before throwing the error for the allocation failure.
Out-of-memory errors unlikely happen in the wild, and we do not bother
with back-patches when these are fixed, usually. However, the problem
dealt with here is a degree worse as it breaks the shared memory state
of pgstats, impacting other processes that may look at an inconsistent
entry that a different process has failed to create.
Author: Mikhail Kot <mikhail.kot@databricks.com>
Discussion: https://postgr.es/m/CAAi9E7jELo5_-sBENftnc2E8XhW2PKZJWfTC3i2y-GMQd2bcqQ@mail.gmail.com
Backpatch-through: 15
The callback added in fc415edf8c used to check if there is any pending
data to flush for fixed-numbered statistics, done by looping across all
the builtin and custom stats kinds with a call to have_fixed_pending_cb,
is proving to able to show in workloads that do not report any stats
(read-only, no function calls, no WAL, no IO, etc). The code used in
v17 was cheaper than that what HEAD has introduced, relying on three
boolean checks for WAL, SLRU and IO stats.
This commit switches the code to use a more efficient approach than
fc415edf8c, with a single boolean flag that can be switched to "true"
by any fixed-numbered stats kinds to force pgstat_report_stat() to go
through one round of reports. The flag is reset by pgstat_report_stat()
once a full round of reports is done. The flag being false means that
fixed-numbered stats kinds saw no activity, and that there is no pending
data to flush.
ac000fca74 took one step in improving the performance by reducing the
number of stats kinds that the backend can hold. This commit takes a
more drastic step by bringing back the code efficiency to what it was
before v18 with a cheap check at the beginning of pgstat_report_stat()
for its fast-exit path.
The callback have_static_pending_cb is removed as an effect of all that.
Reported-by: Andres Freund <andres@anarazel.de>
Reviewed-by: Bertrand Drouvot <bertranddrouvot.pg@gmail.com>
Discussion: https://postgr.es/m/eb224uegsga2hgq7dfq3ps5cduhpqej7ir2hjxzzozjthrekx5@dysei6buqthe
Backpatch-through: 18
Not having this check would produce a core dump at startup when running
pgstat_read_statsfile(), in the case where the information of a stats
kind for an entry in the dshash could not be found. The same check
already happens for fixed-numbered stats and entries that are stored
with their names. This issue can be seen with custom stats kinds.
Note that this problem can be reproduced what what is in the core code:
- Tweak the test module injection_points to not load the fixed-numbered
stats part, leaving only the variable-numbered stats.
- Create an instance with injection_points defined in
shared_preload_libraries.
- Create a pgstats entry by attaching and running a point.
- Restart the server without shared_preload_libraries. The startup
process detects that something is wrong and reports a WARNING.
Author: Bertrand Drouvot <bertranddrouvot.pg@gmail.com>
Discussion: https://postgr.es/m/aAieZAvM+K1d89R2@ip-10-97-1-34.eu-west-3.compute.internal
This assertion, based on pending_since (timestamp used to prevent stats
reports to be too frequent or should a partial flush happen), is reached
when it is found that no data can be flushed but a previous call of
pgstat_report_stat() determined that some stats data has been found as
in need of a flush. So pending_since is set when some stats data is
pending (in non-force mode) or if report attempts are too frequent, and
reset to 0 once all stats have been flushed.
Since 5cbbe70a9c, WAL senders have begun to report their stats on a
periodic basis for IO stats in v16~ and backend stats on HEAD, creating
some friction with the concurrent pgstat_report_stat() calls that can
happen in the context of a WAL sender (shutdown callback doing a final
report or backend-related code paths). This problem is the cause of
spurious failures in the TAP tests.
In theory, this assertion can be also reached in v15, even if that's
very unlikely. For example, a process, say a background worker, could
do periodic and direct stats flushes with concurrent calls of
pgstat_report_stat() that could cause conflicting values of
pending_since. This can be done with WAL or SLRU stats flushes using
pgstat_flush_wal() or pgstat_slru_flush(). HEAD makes this situation
easier to happen with custom cumulative stats.
This commit removes the assertion altogether, per discussion, as it is
more useful to keep the state of things as they are for the WAL sender.
The assertion could use a special state based on for example
am_walsender, but I doubt that this would be meaningful in the long run
based on the other arguments raised while discussing this issue.
Reported-by: Tom Lane <tgl@sss.pgh.pa.us>
Reported-by: Andres Freund <andres@anarazel.de>
Discussion: https://postgr.es/m/1489124.1744685908@sss.pgh.pa.us
Discussion: https://postgr.es/m/dwrkeszz6czvtkxzr5mqlciy652zau5qqnm3cp5f3p2po74ppk@omg4g3cc6dgq
Backpatch-through: 15
This should silence Coverity's complaints about the result being
sometimes ignored.
I'm inclined to think that these routines are simply misdesigned,
because sometimes it's okay to ignore the result and sometimes it
isn't, and we have no way to enforce the latter. But for now
I just added a comment.
This reverts commit b860848232, that was added as a prerequisite for
the support of pgstats data flush across checkpoints, linking a pgstats
file to a specific checkpoint redo LSN.
As reported, this is proving to be currently problematic when going
through a pg_upgrade, that does direct manipulations of the control file
in the new cluster. The LSN stored in the pgstats file is not able to
cope with any changes done in the control file by pg_upgrade yet,
causing the pgstats file to be discarded when starting the new cluster
after overriding its redo LSN (one is a `pg_resetwal -l` where the new
cluster's start LSN is bumped by a hardcoded value of 8 segments, see
copy_xact_xlog_xid).
The least painful path going forward is likely going to be a refactor of
the pgstats code so as it is possible to read and write some of its data
with some routines in src/common/, so as pg_upgrade or pg_resetwal are
able to update its data. The main point is that we are going to need a
LSN in the stats file should we make it written at checkpoint time and
not only as part of a shutdown sequence. It is too late to dive into
these details for v18, so let's revert the change, and let's try to
figure out all the details in the next release cycle. The pgstats file
is currently only written as part of a shutdown sequence, and its
contents are still lost on crash, same as older releases.
Bump PGSTAT_FILE_FORMAT_ID.
Reported-by: Tom Lane <tgl@sss.pgh.pa.us>
Discussion: https://postgr.es/m/2563883.1741826489@sss.pgh.pa.us
9aea73fc61 has added support for backend statistics, relying on
PgStat_EntryRef->pending for its data pending for flush. This design
lacks in flexibility, because the pending list does some memory
allocation, making it unsuitable if incrementing counters in critical
sections.
Pending data of backend statistics is reworked so the implementation
does not depend on PgStat_EntryRef->pending anymore, relying on a static
area of memory to store the counters that are flushed when stats are
reported to the pgstats dshash. An advantage of this approach is to
allow the pending data to be manipulated in critical sections; some
patches are under discussion and require that.
The pending data is tracked by PendingBackendStats, local to
pgstat_backend.c. Two routines are introduced to allow IO statistics to
update the backend-side counters. have_static_pending_cb and
flush_static_cb are used for the flush, instead of flush_pending_cb.
Author: Bertrand Drouvot, Michael Paquier
Discussion: https://postgr.es/m/66efowskppsns35v5u2m7k4sdnl7yoz5bo64tdjwq7r5lhplrz@y7dme5xwh2r5
The two callbacks have_fixed_pending_cb and flush_fixed_cb have been
introduced in fc415edf8c to provide a way for fixed-numbered
statistics to control the flush of their data. These are renamed to
respectively have_static_pending_cb and flush_static_cb. The
restriction that these only apply to fixed-numbered stats is removed.
A follow-up patch will make use of them for backend statistics. This
stats kind is variable-numbered, and patches are under discussion to
track WAL data for IO and backend stats which cannot use
PgStat_EntryRef->pending as pending data would be touched in critical
sections, where no memory allocation can happen.
Per discussion with Andres Freund.
Author: Bertrand Drouvot
Reviewed-by: Michael Paquier
Discussion: https://postgr.es/m/66efowskppsns35v5u2m7k4sdnl7yoz5bo64tdjwq7r5lhplrz@y7dme5xwh2r5
This commit changes the way pending backend statistics are tracked by
moving them into a new structure called PgStat_BackendPending, removing
PgStat_BackendPendingIO. PgStat_BackendPending currently only includes
PgStat_PendingIO for the pending I/O stats.
pgstat_flush_backend() is extended with a "flags" argument to control
which parts of the stats of a backend should be flushed.
With this refactoring, it becomes easier to plug into backend statistics
more data. A patch to add information related to WAL in this stats kind
is under discussion.
Author: Bertrand Drouvot
Discussion: https://postgr.es/m/Z3zqc4o09dM/Ezyz@ip-10-97-1-34.eu-west-3.compute.internal
This adds a new variable-numbered statistics kind in pgstats, where the
object ID key of the stats entries is based on the proc number of the
backends. This acts as an upper-bound for the number of stats entries
that can exist at once. The entries are created when a backend starts
after authentication succeeds, and are removed when the backend exits,
making the stats entry exist for as long as their backend is up and
running. These are not written to the pgstats file at shutdown (note
that write_to_file is disabled, as a safety measure).
Currently, these stats include only information about the I/O generated
by a backend, using the same layer as pg_stat_io, except that it is now
possible to know how much activity is happening in each backend rather
than an overall aggregate of all the activity. A function called
pg_stat_get_backend_io() is added to access this data depending on the
PID of a backend. The existing structure could be expanded in the
future to add more information about other statistics related to
backends, depending on requirements or ideas.
Auxiliary processes are not included in this set of statistics. These
are less interesting to have than normal backends as they have dedicated
entries in pg_stat_io, and stats kinds of their own.
This commit includes also pg_stat_reset_backend_stats(), function able
to reset all the stats associated to a single backend.
Bump catalog version and PGSTAT_FILE_FORMAT_ID.
Author: Bertrand Drouvot
Reviewed-by: Álvaro Herrera, Kyotaro Horiguchi, Michael Paquier, Nazir
Bilal Yavuz
Discussion: https://postgr.es/m/ZtXR+CtkEVVE/LHF@ip-10-97-1-34.eu-west-3.compute.internal
pgstat_write_statsfile() discards any entries marked as dropped from
being written to the stats file at shutdown, and also included an
assertion based on the same condition.
The intention of the assertion is to track that no pgstats entries
should be left around as terminating backends should drop any entries
they still hold references on before the stats file is written by the
checkpointer, and it not worth taking down the server in this case if
there is a bug making that possible.
Let's improve the comment of this area to document clearly what's
intended.
Based on a discussion with Bertrand Drouvot and Anton A. Melnikov.
Author: Bertrand Drouvot
Discussion: https://postgr.es/m/a13e8cdf-b97a-4ecb-8f42-aaa367974e29@postgrespro.ru
Backpatch-through: 15
This new field controls if entries of a stats kind should be written or
not to the on-disk pgstats file when shutting down an instance. This
affects both fixed and variable-numbered kinds.
This is useful for custom statistics by itself, and a patch is under
discussion to add a new builtin stats kind where the write of the stats
is not necessary. All the built-in stats kinds, as well as the two
custom stats kinds in the test module injection_points, set this flag to
"true" for now, so as stats entries are written to the on-disk pgstats
file.
Author: Bertrand Drouvot
Reviewed-by: Nazir Bilal Yavuz
Discussion: https://postgr.es/m/Zz7T47nHwYgeYwOe@ip-10-97-1-34.eu-west-3.compute.internal
PgStat_HashKey is currently initialized in a way that could result in
random data if the structure has any padding bytes. The structure
has no padding bytes currently, fortunately, but it could become a
problem should the structure change at some point in the future.
The code is changed to use some memset(0) so as any padding would be
handled properly, as it would be surprising to see random failures in
the pgstats entry lookups. PgStat_HashKey is a structure internal to
pgstats, and an ABI change could be possible in the scope of a bug fix,
so backpatch down to 15 where this has been introduced.
Author: Bertrand Drouvot
Reviewed-by: Jelte Fennema-Nio, Michael Paquier
Discussion: https://postgr.es/m/Zyb7RW1y9dVfO0UH@ip-10-97-1-34.eu-west-3.compute.internal
Backpatch-through: 15
This opens the possibility to define keys for more types of statistics
kinds in PgStat_HashKey, the first case being 8-byte query IDs for
statistics like pg_stat_statements.
This increases the size of PgStat_HashKey from 12 to 16 bytes, while
PgStatShared_HashEntry, entry stored in the dshash for pgstats, keeps
the same size due to alignment.
xl_xact_stats_item, that tracks the stats items to drop in commit WAL
records, is increased from 12 to 16 bytes. Note that individual chunks
in commit WAL records should be multiples of sizeof(int), hence 8-byte
object IDs are stored as two uint32, based on a suggestion from Heikki
Linnakangas.
While on it, the field of PgStat_HashKey is renamed from "objoid" to
"objid", as for some stats kinds this field does not refer to OIDs but
just IDs, like for replication slot stats.
This commit bumps the following format variables:
- PGSTAT_FILE_FORMAT_ID, as PgStat_HashKey is written to the stats file
for non-serialized stats kinds in the dshash table.
- XLOG_PAGE_MAGIC for the changes in xl_xact_stats_item.
- Catalog version, for the SQL function pg_stat_have_stats().
Reviewed-by: Bertrand Drouvot
Discussion: https://postgr.es/m/ZsvTS9EW79Up8I62@paquier.xyz
This commit adds two callbacks in pgstats to have a better control of
the flush timing of pgstat_report_stat(), whose operation depends on the
three PGSTAT_*_INTERVAL variables:
- have_fixed_pending_cb(), to check if a stats kind has any pending
data waiting for a flush. This is used as a fast path if there are no
pending statistics to flush, and this check is done for fixed-numbered
statistics only if there are no variable-numbered statistics to flush.
A flush will need to happen if at least one callback reports any pending
data.
- flush_fixed_cb(), to do the actual flush.
These callbacks are currently used by the SLRU, WAL and IO statistics,
generalizing the concept for all stats kinds (builtin and custom).
The SLRU and IO stats relied each on one global variable to determine
whether a flush should happen; these are now local to pgstat_slru.c and
pgstat_io.c, cleaning up a bit how the pending flush states are tracked
in pgstat.c.
pgstat_flush_io() and pgstat_flush_wal() are still required, but we do
not need to check their return result anymore.
Reviewed-by: Bertrand Drouvot, Kyotaro Horiguchi
Discussion: https://postgr.es/m/ZtaVO0N-aTwiAk3w@paquier.xyz
pgstat_initialize() is currently used by the WAL stats as a code path to
take some custom actions when a backend starts. A callback is added to
generalize the concept so as all stats kinds can do the same, for
builtin and custom kinds, if set.
Reviewed-by: Bertrand Drouvot, Kyotaro Horiguchi
Discussion: https://postgr.es/m/ZtZr1K4PLdeWclXY@paquier.xyz
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 commit adds support in the backend for $subject, allowing
out-of-core extensions to plug their own custom kinds of cumulative
statistics. This feature has come up a few times into the lists, and
the first, original, suggestion came from Andres Freund, about
pg_stat_statements to use the cumulative statistics APIs in shared
memory rather than its own less efficient internals. The advantage of
this implementation is that this can be extended to any kind of
statistics.
The stats kinds are divided into two parts:
- The in-core "builtin" stats kinds, with designated initializers, able
to use IDs up to 128.
- The "custom" stats kinds, able to use a range of IDs from 128 to 256
(128 slots available as of this patch), with information saved in
TopMemoryContext. This can be made larger, if necessary.
There are two types of cumulative statistics in the backend:
- For fixed-numbered objects (like WAL, archiver, etc.). These are
attached to the snapshot and pgstats shmem control structures for
efficiency, and built-in stats kinds still do that to avoid any
redirection penalty. The data of custom kinds is stored in a first
array in snapshot structure and a second array in the shmem control
structure, both indexed by their ID, acting as an equivalent of the
builtin stats.
- For variable-numbered objects (like tables, functions, etc.). These
are stored in a dshash using the stats kind ID in the hash lookup key.
Internally, the handling of the builtin stats is unchanged, and both
fixed and variabled-numbered objects are supported. Structure
definitions for builtin stats kinds are renamed to reflect better the
differences with custom kinds.
Like custom RMGRs, custom cumulative statistics can only be loaded with
shared_preload_libraries at startup, and must allocate a unique ID
shared across all the PostgreSQL extension ecosystem with the following
wiki page to avoid conflicts:
https://wiki.postgresql.org/wiki/CustomCumulativeStats
This makes the detection of the stats kinds and their handling when
reading and writing stats much easier than, say, allocating IDs for
stats kinds from a shared memory counter, that may change the ID used by
a stats kind across restarts. When under development, extensions can
use PGSTAT_KIND_EXPERIMENTAL.
Two examples that can be used as templates for fixed-numbered and
variable-numbered stats kinds will be added in some follow-up commits,
with tests to provide coverage.
Some documentation is added to explain how to use this plugin facility.
Author: Michael Paquier
Reviewed-by: Dmitry Dolgov, Bertrand Drouvot
Discussion: https://postgr.es/m/Zmqm9j5EO0I4W8dx@paquier.xyz
These should have been switched from %d to %u in 3188a4582a in the
debugging elogs added in ca1ba50fcb. PgStat_Kind should never be
higher than INT32_MAX, but let's be clean.
Issue noticed while hacking more on this area.
A follow-up patch is planned to make cumulative statistics pluggable,
and using a type is useful in the internal routines used by pgstats as
PgStat_Kind may have a value that was not originally in the enum removed
here, once made pluggable.
While on it, this commit switches pgstat_is_kind_valid() to use
PgStat_Kind rather than an int, to be more consistent with its existing
callers. Some loops based on the stats kind IDs are switched to use
PgStat_Kind rather than int, for consistency with the new time.
Author: Michael Paquier
Reviewed-by: Dmitry Dolgov, Bertrand Drouvot
Discussion: https://postgr.es/m/Zmqm9j5EO0I4W8dx@paquier.xyz
This is used in the startup process to check that the pgstats file we
are reading includes the redo LSN referring to the shutdown checkpoint
where it has been written. The redo LSN in the pgstats file needs to
match with what the control file has.
This is intended to be used for an upcoming change that will extend the
write of the stats file to happen during checkpoints, rather than only
shutdown sequences.
Bump PGSTAT_FILE_FORMAT_ID.
Reviewed-by: Bertrand Drouvot
Discussion: https://postgr.es/m/Zp8o6_cl0KSgsnvS@paquier.xyz
This is useful to know which part of a stats file is corrupted when
reading it, adding to the server logs a WARNING with details about what
could not be read before giving up with the remaining data in the file.
Author: Michael Paquier
Reviewed-by: Bertrand Drouvot
Discussion: https://postgr.es/m/Zp8o6_cl0KSgsnvS@paquier.xyz
This switches the pgstats write code to use durable_rename() rather than
rename(). This ensures that the stats file's data is durable when the
statistics are written, which is something only happening at shutdown
now with the checkpointer doing the job.
This could cause the statistics to be lost even after PostgreSQL is shut
down, should a host failure happen, for example.
Suggested-by: Konstantin Knizhnik
Reviewed-by: Bertrand Drouvot
Discussion: https://postgr.es/m/ZpDQTZ0cAz0WEbh7@paquier.xyz
This routine can currently only be called from the postmaster in
single-user mode or the checkpointer, but there was no sanity check to
make sure that this was always the case.
This has proved to be useful when hacking the zone (at least to me), to
make sure that the write of the pgstats file happens at shutdown, as
wanted by design, in the correct process context.
Discussion: https://postgr.es/m/ZnEiqAITL-VgZDoY@paquier.xyz
