public inbox for [email protected]
help / color / mirror / Atom feed[PATCH 07/18] function comment: get_am_name
21+ messages / 4 participants
[nested] [flat]
* [PATCH 07/18] function comment: get_am_name
@ 2020-12-30 15:39 Justin Pryzby <[email protected]>
0 siblings, 0 replies; 21+ messages in thread
From: Justin Pryzby @ 2020-12-30 15:39 UTC (permalink / raw)
---
src/backend/commands/amcmds.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/backend/commands/amcmds.c b/src/backend/commands/amcmds.c
index eff9535ed0..188109e474 100644
--- a/src/backend/commands/amcmds.c
+++ b/src/backend/commands/amcmds.c
@@ -186,7 +186,7 @@ get_am_oid(const char *amname, bool missing_ok)
}
/*
- * get_am_name - given an access method OID name and type, look up its name.
+ * get_am_name - given an access method OID, look up its name.
*/
char *
get_am_name(Oid amOid)
--
2.17.0
--lc9FT7cWel8HagAv
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment; filename="0008-an-exclusive.patch"
^ permalink raw reply [nested|flat] 21+ messages in thread
* Re: shared-memory based stats collector - v66
@ 2022-03-17 07:36 Andres Freund <[email protected]>
0 siblings, 4 replies; 21+ messages in thread
From: Andres Freund @ 2022-03-17 07:36 UTC (permalink / raw)
To: Kyotaro Horiguchi <[email protected]>; Melanie Plageman <[email protected]>; +Cc: [email protected]; [email protected]; [email protected]; [email protected]
Hi,
I've attached a substantially improved version of the shared memory stats
patch.
The biggest changes are:
- chopped the bigger patches into smaller chunks. Most importantly the
"transactional stats creation / drop" part is now its own commit. Also the
tests I added.
- put in a defense against the danger of loosing function stats entries due to
the lack of cache invalidation during function calls. See long comment in
pgstat_init_function_usage()
- split up pgstat_global.c into pgstat_{archiver, bgwriter, checkpoint,
replslot, slru, wal}. While each individually not large, there were enough
of them to make the file confusing. Feels a lot better to work with.
- replication slot stats used the slot "index" in a dangerous way, fixed
- implemented a few omitted features like resetting all subscriptions and
setting reset timestamps (Melanie)
- loads of code and comment polishing
I think the first few patches are basically ready to be applied and are
independently worthwhile:
- 0001-pgstat-run-pgindent-on-pgstat.c-h.patch
- 0002-pgstat-split-relation-database-stats-handling-ou.patch
- 0003-pgstat-split-out-WAL-handling-from-pgstat_-initi.patch
- 0004-pgstat-introduce-pgstat_relation_should_count.patch
- 0005-pgstat-xact-level-cleanups-consolidation.patch
Might not be worth having separately, should probably just be part of
0014:
- 0006-pgstat-wip-pgstat-relation-init-assoc.patch
A pain to maintain, needs mostly a bit of polishing of file headers. Perhaps I
should rename pgstat_checkpoint.c to pgstat_checkpointer.c, fits better with
function names:
- 0007-pgstat-split-different-types-of-stats-into-separ.patch
This is also painful to maintain. Mostly kept separate from 0007 for easier
reviewing:
- 0009-pgstat-reorder-file-pgstat.c-pgstat.h-contents.patch
Everything after isn't yet quite there / depend on patches that aren't yet
there:
- 0010-pgstat-add-pgstat_copy_relation_stats.patch
- 0011-pgstat-remove-superflous-comments-from-pgstat.h.patch
- 0012-pgstat-stats-collector-references-in-comments.patch
- 0013-pgstat-scaffolding-for-transactional-stats-creat.patch
- 0014-pgstat-store-statistics-in-shared-memory.patch
- 0015-pgstat-add-pg_stat_force_next_flush.patch
Notably this patch makes stat.sql a test that can safely be run concurrent
with other tests:
- 0016-pgstat-utilize-pg_stat_force_next_flush-to-simpl.patch
Needs a tap test as well, but already covers a lot of things that aren't
covered today. Unfortunately it can't really be applied before because it's
too hard to write / slow to run without 0015
- 0017-pgstat-extend-pgstat-test-coverage.patch
I don't yet know what we should do with other users of
PG_STAT_TMP_DIR. There's no need for it for pgstat.c et al anymore. Not sure
that pg_stat_statement is enough of a reason to keep the stats_temp_directory
GUC around?
- 0019-pgstat-wip-remove-stats_temp_directory.patch
Right now we reset stats for replicas, even if we start from a shutdown
checkpoint. That seems pretty unnecessary with this patch:
- 0021-pgstat-wip-only-reset-pgstat-data-after-crash-re.patch
Starting to feel more optimistic about this! There's loads more to do, but now
the TODOs just seem to require elbow grease, rather than deep thinking.
The biggest todos are:
- Address all the remaining AFIXMEs and XXXs
- add longer explanation of architecture to pgstat.c (or a README)
- Further improve our stats test coverage - there's a crapton not covered,
despite 0017:
- test WAL replay with stats (stats for dropped tables are removed etc)
- test crash recovery and "invalid stats file" paths
- lot of the pg_stat_ views like bgwriter, pg_stat_database have zero coverage today
- make naming not "a pain in the neck": [1]
- lots of polishing
- revise docs
- run benchmarks - I've done so in the past, but not recently
- perhaps 0014 can be further broken down - it's still uncomfortably large
It's worth noting that the patchset, leaving new tests aside, has a
substantially negative diffstat, even if one includes all the new file
headers... Once a bunch more cleanup is done, I bet it'll improve further.
with new tests:
80 files changed, 9759 insertions(+), 8051 deletions(-)
without tests (mildly inaccurate):
71 files changed, 6991 insertions(+), 7814 deletions(-)
just shared memory stats patch, not all the code movement, new file headers:
49 files changed, 4079 insertions(+), 5472 deletions(-)
Comments and reviews welcome!
I regularly push to https://github.com/anarazel/postgres/tree/shmstat fwiw -
the series is way too big to spam the list all the time.
Greetings,
Andres Freund
[1] https://postgr.es/m/20220303.170412.1542007127371857370.horikyota.ntt%40gmail.com
Attachments:
[application/x-patch-gzip] v66-0001-pgstat-run-pgindent-on-pgstat.c-h.patch.gz (2.2K, ../../[email protected]/2-v66-0001-pgstat-run-pgindent-on-pgstat.c-h.patch.gz)
download
[application/x-patch-gzip] v66-0002-pgstat-split-relation-database-stats-handling-ou.patch.gz (1.9K, ../../[email protected]/3-v66-0002-pgstat-split-relation-database-stats-handling-ou.patch.gz)
download
[application/x-patch-gzip] v66-0003-pgstat-split-out-WAL-handling-from-pgstat_-initi.patch.gz (1.4K, ../../[email protected]/4-v66-0003-pgstat-split-out-WAL-handling-from-pgstat_-initi.patch.gz)
download
[application/x-patch-gzip] v66-0004-pgstat-introduce-pgstat_relation_should_count.patch.gz (1.6K, ../../[email protected]/5-v66-0004-pgstat-introduce-pgstat_relation_should_count.patch.gz)
download
[application/x-patch-gzip] v66-0005-pgstat-xact-level-cleanups-consolidation.patch.gz (1.8K, ../../[email protected]/6-v66-0005-pgstat-xact-level-cleanups-consolidation.patch.gz)
download
[application/x-patch-gzip] v66-0006-pgstat-wip-pgstat-relation-init-assoc.patch.gz (2.0K, ../../[email protected]/7-v66-0006-pgstat-wip-pgstat-relation-init-assoc.patch.gz)
download
[application/x-patch-gzip] v66-0007-pgstat-split-different-types-of-stats-into-separ.patch.gz (30.0K, ../../[email protected]/8-v66-0007-pgstat-split-different-types-of-stats-into-separ.patch.gz)
download
[application/x-patch-gzip] v66-0008-pgstat-fix-function-name.patch.gz (516B, ../../[email protected]/9-v66-0008-pgstat-fix-function-name.patch.gz)
download
[application/x-patch-gzip] v66-0009-pgstat-reorder-file-pgstat.c-pgstat.h-contents.patch.gz (11.0K, ../../[email protected]/10-v66-0009-pgstat-reorder-file-pgstat.c-pgstat.h-contents.patch.gz)
download
[application/x-patch-gzip] v66-0010-pgstat-add-pgstat_copy_relation_stats.patch.gz (1.4K, ../../[email protected]/11-v66-0010-pgstat-add-pgstat_copy_relation_stats.patch.gz)
download
[application/x-patch-gzip] v66-0011-pgstat-remove-superflous-comments-from-pgstat.h.patch.gz (1.2K, ../../[email protected]/12-v66-0011-pgstat-remove-superflous-comments-from-pgstat.h.patch.gz)
download
[application/x-patch-gzip] v66-0012-pgstat-stats-collector-references-in-comments.patch.gz (8.3K, ../../[email protected]/13-v66-0012-pgstat-stats-collector-references-in-comments.patch.gz)
download
[application/x-patch-gzip] v66-0013-pgstat-scaffolding-for-transactional-stats-creat.patch.gz (8.0K, ../../[email protected]/14-v66-0013-pgstat-scaffolding-for-transactional-stats-creat.patch.gz)
download
[application/x-patch-gzip] v66-0014-pgstat-store-statistics-in-shared-memory.patch.gz (88.5K, ../../[email protected]/15-v66-0014-pgstat-store-statistics-in-shared-memory.patch.gz)
download
[application/x-patch-gzip] v66-0015-pgstat-add-pg_stat_force_next_flush.patch.gz (1.6K, ../../[email protected]/16-v66-0015-pgstat-add-pg_stat_force_next_flush.patch.gz)
download
[application/x-patch-gzip] v66-0016-pgstat-utilize-pg_stat_force_next_flush-to-simpl.patch.gz (4.4K, ../../[email protected]/17-v66-0016-pgstat-utilize-pg_stat_force_next_flush-to-simpl.patch.gz)
download
[application/x-patch-gzip] v66-0017-pgstat-extend-pgstat-test-coverage.patch.gz (5.6K, ../../[email protected]/18-v66-0017-pgstat-extend-pgstat-test-coverage.patch.gz)
download
[application/x-patch-gzip] v66-0018-pgstat-move-pgstat.c-to-utils-activity.patch.gz (916B, ../../[email protected]/19-v66-0018-pgstat-move-pgstat.c-to-utils-activity.patch.gz)
download
[application/x-patch-gzip] v66-0019-pgstat-wip-remove-stats_temp_directory.patch.gz (3.0K, ../../[email protected]/20-v66-0019-pgstat-wip-remove-stats_temp_directory.patch.gz)
download
[application/x-patch-gzip] v66-0020-pgstat-rename-STATS_COLLECTOR-GUC-group-to-STATS.patch.gz (1.4K, ../../[email protected]/21-v66-0020-pgstat-rename-STATS_COLLECTOR-GUC-group-to-STATS.patch.gz)
download
[application/x-patch-gzip] v66-0021-pgstat-wip-only-reset-pgstat-data-after-crash-re.patch.gz (1.1K, ../../[email protected]/22-v66-0021-pgstat-wip-only-reset-pgstat-data-after-crash-re.patch.gz)
download
^ permalink raw reply [nested|flat] 21+ messages in thread
* Re: shared-memory based stats collector - v66
@ 2022-03-20 16:32 Melanie Plageman <[email protected]>
parent: Andres Freund <[email protected]>
3 siblings, 1 reply; 21+ messages in thread
From: Melanie Plageman @ 2022-03-20 16:32 UTC (permalink / raw)
To: Andres Freund <[email protected]>; +Cc: Kyotaro Horiguchi <[email protected]>; Ibrar Ahmed <[email protected]>; [email protected]; [email protected]; PostgreSQL Hackers <[email protected]>
On Thu, Mar 17, 2022 at 3:36 AM Andres Freund <[email protected]> wrote:
>
> Starting to feel more optimistic about this! There's loads more to do, but now
> the TODOs just seem to require elbow grease, rather than deep thinking.
>
> The biggest todos are:
> - Address all the remaining AFIXMEs and XXXs
>
> - add longer explanation of architecture to pgstat.c (or a README)
>
> - Further improve our stats test coverage - there's a crapton not covered,
> despite 0017:
> - test WAL replay with stats (stats for dropped tables are removed etc)
Attached is a TAP test to check that stats are cleaned up on a physical
replica after the objects they concern are dropped on the primary.
I'm not sure that the extra force next flush on standby is needed after
drop on primary since drop should report stats and I wait for catchup.
Also, I don't think the tests with DROP SCHEMA actually exercise another
code path, so it might be worth cutting those.
- Melanie
Attachments:
[text/x-patch] add_replica_stats_cleanup_tests.patch (12.5K, ../../CAAKRu_aNxL1WegCa45r=VAViCLnpOU7uNC7bTtGw+=QAPyYivw@mail.gmail.com/2-add_replica_stats_cleanup_tests.patch)
download | inline diff:
From 2cb108fadf656de9cc998c0b2123a184881ef4e0 Mon Sep 17 00:00:00 2001
From: Melanie Plageman <[email protected]>
Date: Thu, 17 Mar 2022 21:54:16 -0400
Subject: [PATCH] add replica cleanup tests
---
src/backend/utils/activity/pgstat.c | 32 +++
src/backend/utils/adt/pgstatfuncs.c | 22 ++
src/include/catalog/pg_proc.dat | 6 +
src/include/pgstat.h | 3 +
.../recovery/t/029_stats_cleanup_replica.pl | 238 ++++++++++++++++++
5 files changed, 301 insertions(+)
create mode 100644 src/test/recovery/t/029_stats_cleanup_replica.pl
diff --git a/src/backend/utils/activity/pgstat.c b/src/backend/utils/activity/pgstat.c
index e9fab35a46..4c0fea62b4 100644
--- a/src/backend/utils/activity/pgstat.c
+++ b/src/backend/utils/activity/pgstat.c
@@ -159,6 +159,7 @@ static void pgstat_shared_reset_timestamp(PgStatKind kind, PgStatShm_StatEntryHe
static void pgstat_pending_delete(PgStatSharedRef *shared_ref);
static bool pgstat_pending_flush_stats(bool nowait);
+static PgStatKind pgstat_kind_for(char *kind_str);
static inline const PgStatKindInfo *pgstat_kind_info_for(PgStatKind kind);
@@ -1315,6 +1316,23 @@ pgstat_get_stat_snapshot_timestamp(bool *have_snapshot)
return 0;
}
+bool
+pgstat_shared_stat_exists(char *kind_str, Oid dboid, Oid objoid)
+{
+ PgStatKind kind = pgstat_kind_for(kind_str);
+
+ /*
+ * Ignore dboid or objoid for subscription and db stats respectively.
+ */
+ if (kind == PGSTAT_KIND_SUBSCRIPTION)
+ dboid = InvalidOid;
+
+ if (kind == PGSTAT_KIND_DB)
+ objoid = InvalidOid;
+
+ return (bool) pgstat_fetch_entry(kind, dboid, objoid);
+}
+
/* ------------------------------------------------------------
* Helper functions
@@ -1343,6 +1361,20 @@ pgstat_setup_memcxt(void)
ALLOCSET_SMALL_SIZES);
}
+static PgStatKind
+pgstat_kind_for(char *kind_str)
+{
+ for (int kind = 0; kind <= PGSTAT_KIND_LAST; kind++)
+ {
+ if (pg_strcasecmp(kind_str, pgstat_kind_infos[kind].name) == 0)
+ return kind;
+ }
+
+ ereport(ERROR,
+ (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+ errmsg("invalid statistic kind: \"%s\"", kind_str)));
+}
+
static inline const PgStatKindInfo *
pgstat_kind_info_for(PgStatKind kind)
{
diff --git a/src/backend/utils/adt/pgstatfuncs.c b/src/backend/utils/adt/pgstatfuncs.c
index 5d843cde24..62b2df856f 100644
--- a/src/backend/utils/adt/pgstatfuncs.c
+++ b/src/backend/utils/adt/pgstatfuncs.c
@@ -2420,3 +2420,25 @@ pg_stat_get_subscription_stats(PG_FUNCTION_ARGS)
/* Returns the record as Datum */
PG_RETURN_DATUM(HeapTupleGetDatum(heap_form_tuple(tupdesc, values, nulls)));
}
+
+
+/*
+ * Checks for presence of stats for object with provided object oid of kind
+ * specified in the type string in database of provided database oid.
+ *
+ * For subscription stats, only the objoid will be used. For database stats,
+ * only the dboid will be used. The value passed in for the unused parameter is
+ * discarded.
+ * TODO: should it be 'pg_stat_stats_present' instead of 'pg_stat_stats_exist'?
+ */
+Datum
+pg_stat_stats_exist(PG_FUNCTION_ARGS)
+{
+ Oid dboid = PG_GETARG_OID(0);
+ Oid objoid = PG_GETARG_OID(1);
+ char *stats_type = text_to_cstring(PG_GETARG_TEXT_P(2));
+
+ PG_RETURN_BOOL((bool) pgstat_shared_stat_exists(stats_type, dboid,
+ objoid));
+
+}
diff --git a/src/include/catalog/pg_proc.dat b/src/include/catalog/pg_proc.dat
index 75dae94c49..3f3c4e0427 100644
--- a/src/include/catalog/pg_proc.dat
+++ b/src/include/catalog/pg_proc.dat
@@ -5376,6 +5376,12 @@
proargmodes => '{i,o,o,o,o,o,o,o,o,o,o}',
proargnames => '{slot_name,slot_name,spill_txns,spill_count,spill_bytes,stream_txns,stream_count,stream_bytes,total_txns,total_bytes,stats_reset}',
prosrc => 'pg_stat_get_replication_slot' },
+
+{ oid => '8384', descr => 'statistics: checks if stats exist for provided object of provided type in provided database',
+ proname => 'pg_stat_stats_exist', provolatile => 's', proparallel => 'r',
+ prorettype => 'bool', proargtypes => 'oid oid text',
+ prosrc => 'pg_stat_stats_exist' },
+
{ oid => '8523', descr => 'statistics: information about subscription stats',
proname => 'pg_stat_get_subscription_stats', proisstrict => 'f',
provolatile => 's', proparallel => 'r',
diff --git a/src/include/pgstat.h b/src/include/pgstat.h
index 981f3e9e83..604c01312a 100644
--- a/src/include/pgstat.h
+++ b/src/include/pgstat.h
@@ -430,6 +430,9 @@ extern void pgstat_reset_single_counter(PgStatKind kind, Oid objectid);
extern void pgstat_reset_shared_counters(PgStatKind kind);
extern TimestampTz pgstat_get_stat_snapshot_timestamp(bool *have_snapshot);
+/* Helpers for testing */
+extern bool pgstat_shared_stat_exists(char *kind_str, Oid dboid, Oid objoid);
+
/* GUC parameters */
extern PGDLLIMPORT bool pgstat_track_counts;
extern PGDLLIMPORT int pgstat_track_functions;
diff --git a/src/test/recovery/t/029_stats_cleanup_replica.pl b/src/test/recovery/t/029_stats_cleanup_replica.pl
new file mode 100644
index 0000000000..e66d72a60e
--- /dev/null
+++ b/src/test/recovery/t/029_stats_cleanup_replica.pl
@@ -0,0 +1,238 @@
+# Copyright (c) 2021-2022, PostgreSQL Global Development Group
+
+# Tests that statistics are removed from a physical replica after being dropped
+# on the primary
+
+use strict;
+use warnings;
+use PostgreSQL::Test::Cluster;
+use PostgreSQL::Test::Utils;
+use Test::More;
+
+# Initialize primary node
+my $node_primary = PostgreSQL::Test::Cluster->new('primary');
+# A specific role is created to perform some tests related to replication,
+# and it needs proper authentication configuration.
+$node_primary->init(
+ allows_streaming => 1,
+ auth_extra => [ '--create-role', 'repl_role' ]);
+$node_primary->start;
+my $backup_name = 'my_backup';
+
+# Take backup
+$node_primary->backup($backup_name);
+
+# Set track_functions to all on primary
+$node_primary->append_conf('postgresql.conf', "track_functions = 'all'");
+$node_primary->restart;
+
+# Create streaming standby linking to primary
+my $node_standby = PostgreSQL::Test::Cluster->new('standby');
+$node_standby->init_from_backup($node_primary, $backup_name,
+ has_streaming => 1);
+$node_standby->start;
+
+# Set track_functions to all on standby
+$node_standby->append_conf('postgresql.conf', "track_functions = 'all'");
+$node_standby->restart;
+
+sub populate_standby_stats
+{
+ my ($node_primary, $node_standby, $connect_db, $schema) = @_;
+ # Create table on primary
+ $node_primary->safe_psql($connect_db,
+ "CREATE TABLE $schema.drop_tab_test1 AS SELECT generate_series(1,100) AS a");
+
+ # Create function on primary
+ $node_primary->safe_psql($connect_db,
+ "CREATE FUNCTION $schema.drop_func_test1() RETURNS VOID AS 'select 2;' LANGUAGE SQL IMMUTABLE");
+
+ # Wait for catchup
+ my $primary_lsn = $node_primary->lsn('write');
+ $node_primary->wait_for_catchup($node_standby, 'replay', $primary_lsn);
+
+ # Get database oid
+ my $dboid = $node_standby->safe_psql($connect_db,
+ "SELECT oid FROM pg_database WHERE datname = '$connect_db'");
+
+ # Get table oid
+ my $tableoid = $node_standby->safe_psql($connect_db,
+ "SELECT '$schema.drop_tab_test1'::regclass::oid");
+
+ # Do scan on standby
+ $node_standby->safe_psql($connect_db,
+ "SELECT * FROM $schema.drop_tab_test1");
+
+ # Get function oid
+ my $funcoid = $node_standby->safe_psql($connect_db,
+ "SELECT '$schema.drop_func_test1()'::regprocedure::oid");
+
+ # Call function on standby
+ $node_standby->safe_psql($connect_db,
+ "SELECT $schema.drop_func_test1()");
+
+ # Force flush of stats on standby
+ $node_standby->safe_psql($connect_db,
+ "SELECT pg_stat_force_next_flush()");
+
+ return ($dboid, $tableoid, $funcoid);
+}
+
+sub drop_function_by_oid
+{
+ my ($node_primary, $connect_db, $funcoid) = @_;
+
+ # Get function name from returned oid
+ my $func_name = $node_primary->safe_psql($connect_db,
+ "SELECT '$funcoid'::regprocedure");
+
+ # Drop function on primary
+ $node_primary->safe_psql($connect_db,
+ "DROP FUNCTION $func_name");
+}
+
+sub drop_table_by_oid
+{
+ my ($node_primary, $connect_db, $tableoid) = @_;
+
+ # Get table name from returned oid
+ my $table_name = $node_primary->safe_psql($connect_db,
+ "SELECT '$tableoid'::regclass");
+
+ # Drop table on primary
+ $node_primary->safe_psql($connect_db,
+ "DROP TABLE $table_name");
+}
+
+sub test_standby_func_tab_stats_status
+{
+ my ($node_standby, $connect_db, $dboid, $tableoid, $funcoid, $present) = @_;
+
+ is($node_standby->safe_psql($connect_db,
+ "SELECT pg_stat_stats_exist($dboid, $tableoid, 'table')"), $present,
+ "Check that table stats exist is '$present' on standby");
+
+ is($node_standby->safe_psql($connect_db,
+ "SELECT pg_stat_stats_exist($dboid, $funcoid, 'function')"), $present,
+ "Check that function stats exist is '$present' on standby");
+
+ return;
+}
+
+sub test_standby_db_stats_status
+{
+ my ($node_standby, $connect_db, $dboid, $present) = @_;
+
+ is($node_standby->safe_psql($connect_db,
+ "SELECT pg_stat_stats_exist($dboid, $dboid, 'db')"), $present,
+ "Check that db stats exist is '$present' on standby");
+}
+
+# Test that stats are cleaned up on standby after dropping table or function
+
+# Populate test objects
+my ($dboid, $tableoid, $funcoid) =
+ populate_standby_stats($node_primary, $node_standby, 'postgres', 'public');
+
+# Test that the stats are present
+test_standby_func_tab_stats_status($node_standby, 'postgres',
+ $dboid, $tableoid, $funcoid, 't');
+
+# Drop test objects
+drop_table_by_oid($node_primary, 'postgres', $tableoid);
+
+drop_function_by_oid($node_primary, 'postgres', $funcoid);
+
+# Wait for catchup
+my $primary_lsn = $node_primary->lsn('write');
+$node_primary->wait_for_catchup($node_standby, 'replay', $primary_lsn);
+
+# TODO: I don't think this is needed because DROP should have resulted in
+# pg_report_stat()?
+# Force flush of stats on standby
+$node_standby->safe_psql('postgres',
+ "SELECT pg_stat_force_next_flush()");
+
+# Check table and function stats removed from standby
+test_standby_func_tab_stats_status($node_standby, 'postgres',
+ $dboid, $tableoid, $funcoid, 'f');
+
+# Check that stats are cleaned up on standby after dropping schema
+
+# Create schema
+$node_primary->safe_psql('postgres',
+ "CREATE SCHEMA drop_schema_test1");
+
+# Wait for catchup
+$primary_lsn = $node_primary->lsn('write');
+$node_primary->wait_for_catchup($node_standby, 'replay', $primary_lsn);
+
+# Populate test objects
+($dboid, $tableoid, $funcoid) = populate_standby_stats($node_primary,
+ $node_standby, 'postgres', 'drop_schema_test1');
+
+# Test that the stats are present
+test_standby_func_tab_stats_status($node_standby, 'postgres',
+ $dboid, $tableoid, $funcoid, 't');
+
+# Drop schema
+$node_primary->safe_psql('postgres',
+ "DROP SCHEMA drop_schema_test1 CASCADE");
+
+# Wait for catchup
+$primary_lsn = $node_primary->lsn('write');
+$node_primary->wait_for_catchup($node_standby, 'replay', $primary_lsn);
+
+# TODO: I don't think this is needed because DROP should have resulted in
+# pg_report_stat()?
+# Force flush of stats on standby
+$node_standby->safe_psql('postgres',
+ "SELECT pg_stat_force_next_flush()");
+
+# Check table and function stats removed from standby
+test_standby_func_tab_stats_status($node_standby, 'postgres',
+ $dboid, $tableoid, $funcoid, 'f');
+
+# Test that stats are cleaned up on standby after dropping database
+
+# Create the database
+$node_primary->safe_psql('postgres',
+ "CREATE DATABASE test");
+
+# Wait for catchup
+$primary_lsn = $node_primary->lsn('write');
+$node_primary->wait_for_catchup($node_standby, 'replay', $primary_lsn);
+
+# Populate test objects
+($dboid, $tableoid, $funcoid) = populate_standby_stats($node_primary,
+ $node_standby, 'test', 'public');
+
+# Test that the stats are present
+test_standby_func_tab_stats_status($node_standby, 'test',
+ $dboid, $tableoid, $funcoid, 't');
+
+test_standby_db_stats_status($node_standby, 'test', $dboid, 't');
+
+# Drop db 'test' on primary
+$node_primary->safe_psql('postgres',
+ "DROP DATABASE test");
+
+# Wait for catchup
+$primary_lsn = $node_primary->lsn('write');
+$node_primary->wait_for_catchup($node_standby, 'replay', $primary_lsn);
+
+# TODO: I don't think this is needed because DROP should have resulted in
+# pg_report_stat()?
+# Force flush of stats on standby
+$node_standby->safe_psql('postgres',
+ "SELECT pg_stat_force_next_flush()");
+
+# Test that the stats were cleaned up on standby
+# Note that this connects to 'postgres' but provides the dboid of dropped db
+# 'test' which was returned by previous routine
+test_standby_func_tab_stats_status($node_standby, 'postgres',
+ $dboid, $tableoid, $funcoid, 'f');
+
+test_standby_db_stats_status($node_standby, 'postgres', $dboid, 'f');
+
+done_testing();
--
2.30.2
^ permalink raw reply [nested|flat] 21+ messages in thread
* Re: shared-memory based stats collector - v66
@ 2022-03-20 16:58 Andres Freund <[email protected]>
parent: Melanie Plageman <[email protected]>
0 siblings, 1 reply; 21+ messages in thread
From: Andres Freund @ 2022-03-20 16:58 UTC (permalink / raw)
To: Melanie Plageman <[email protected]>; +Cc: Kyotaro Horiguchi <[email protected]>; Ibrar Ahmed <[email protected]>; [email protected]; [email protected]; PostgreSQL Hackers <[email protected]>
Hi,
On 2022-03-20 12:32:39 -0400, Melanie Plageman wrote:
> Attached is a TAP test to check that stats are cleaned up on a physical
> replica after the objects they concern are dropped on the primary.
Thanks!
> I'm not sure that the extra force next flush on standby is needed after
> drop on primary since drop should report stats and I wait for catchup.
A drop doesn't force stats in other sessions to be flushed immediately, so
unless I misunderstand, yes, it's needed.
> Also, I don't think the tests with DROP SCHEMA actually exercise another
> code path, so it might be worth cutting those.
> +/*
> + * Checks for presence of stats for object with provided object oid of kind
> + * specified in the type string in database of provided database oid.
> + *
> + * For subscription stats, only the objoid will be used. For database stats,
> + * only the dboid will be used. The value passed in for the unused parameter is
> + * discarded.
> + * TODO: should it be 'pg_stat_stats_present' instead of 'pg_stat_stats_exist'?
> + */
> +Datum
> +pg_stat_stats_exist(PG_FUNCTION_ARGS)
Should we revoke stats for this one from PUBLIC (similar to the reset functions)?
> +# Set track_functions to all on standby
> +$node_standby->append_conf('postgresql.conf', "track_functions = 'all'");
That should already be set, cloning from the primary includes the
configuration from that point in time.
> +$node_standby->restart;
FWIW, it'd also only require a reload....
Greetings,
Andres Freund
^ permalink raw reply [nested|flat] 21+ messages in thread
* Re: shared-memory based stats collector - v66
@ 2022-03-20 20:56 Melanie Plageman <[email protected]>
parent: Andres Freund <[email protected]>
0 siblings, 1 reply; 21+ messages in thread
From: Melanie Plageman @ 2022-03-20 20:56 UTC (permalink / raw)
To: Andres Freund <[email protected]>; +Cc: Kyotaro Horiguchi <[email protected]>; Ibrar Ahmed <[email protected]>; [email protected]; [email protected]; PostgreSQL Hackers <[email protected]>
On Sun, Mar 20, 2022 at 12:58 PM Andres Freund <[email protected]> wrote:
>
> Hi,
>
> On 2022-03-20 12:32:39 -0400, Melanie Plageman wrote:
> > Attached is a TAP test to check that stats are cleaned up on a physical
> > replica after the objects they concern are dropped on the primary.
>
> Thanks!
>
>
> > I'm not sure that the extra force next flush on standby is needed after
> > drop on primary since drop should report stats and I wait for catchup.
>
> A drop doesn't force stats in other sessions to be flushed immediately, so
> unless I misunderstand, yes, it's needed.
>
>
> > Also, I don't think the tests with DROP SCHEMA actually exercise another
> > code path, so it might be worth cutting those.
>
> > +/*
> > + * Checks for presence of stats for object with provided object oid of kind
> > + * specified in the type string in database of provided database oid.
> > + *
> > + * For subscription stats, only the objoid will be used. For database stats,
> > + * only the dboid will be used. The value passed in for the unused parameter is
> > + * discarded.
> > + * TODO: should it be 'pg_stat_stats_present' instead of 'pg_stat_stats_exist'?
> > + */
> > +Datum
> > +pg_stat_stats_exist(PG_FUNCTION_ARGS)
>
> Should we revoke stats for this one from PUBLIC (similar to the reset functions)?
>
>
> > +# Set track_functions to all on standby
> > +$node_standby->append_conf('postgresql.conf', "track_functions = 'all'");
>
> That should already be set, cloning from the primary includes the
> configuration from that point in time.
>
> > +$node_standby->restart;
>
> FWIW, it'd also only require a reload....
>
Addressed all of these points in
v2-0001-add-replica-cleanup-tests.patch
also added a new test file in
v2-0002-Add-TAP-test-for-discarding-stats-after-crash.patch
testing correct behavior after a crash and when stats file is invalid
- Melanie
Attachments:
[text/x-patch] v2-0001-add-replica-cleanup-tests.patch (12.7K, ../../CAAKRu_Yf3TqApws5O_+uWWhoOD=a=9XjzNgOE_ufvWqHCpFKWQ@mail.gmail.com/2-v2-0001-add-replica-cleanup-tests.patch)
download | inline diff:
From c521ba1dcb13ba236181adce06893c42ec439877 Mon Sep 17 00:00:00 2001
From: Melanie Plageman <[email protected]>
Date: Thu, 17 Mar 2022 21:54:16 -0400
Subject: [PATCH v2 1/2] add replica cleanup tests
---
src/backend/catalog/system_functions.sql | 2 +
src/backend/utils/activity/pgstat.c | 32 +++
src/backend/utils/adt/pgstatfuncs.c | 22 ++
src/include/catalog/pg_proc.dat | 6 +
src/include/pgstat.h | 3 +
.../recovery/t/029_stats_cleanup_replica.pl | 228 ++++++++++++++++++
6 files changed, 293 insertions(+)
create mode 100644 src/test/recovery/t/029_stats_cleanup_replica.pl
diff --git a/src/backend/catalog/system_functions.sql b/src/backend/catalog/system_functions.sql
index 81bac6f581..4f2c80b72e 100644
--- a/src/backend/catalog/system_functions.sql
+++ b/src/backend/catalog/system_functions.sql
@@ -639,6 +639,8 @@ REVOKE EXECUTE ON FUNCTION pg_stat_reset_single_function_counters(oid) FROM publ
REVOKE EXECUTE ON FUNCTION pg_stat_reset_replication_slot(text) FROM public;
+REVOKE EXECUTE ON FUNCTION pg_stat_stats_exist(oid, oid, text) FROM public;
+
REVOKE EXECUTE ON FUNCTION pg_stat_reset_subscription_stats(oid) FROM public;
REVOKE EXECUTE ON FUNCTION lo_import(text) FROM public;
diff --git a/src/backend/utils/activity/pgstat.c b/src/backend/utils/activity/pgstat.c
index e9fab35a46..4c0fea62b4 100644
--- a/src/backend/utils/activity/pgstat.c
+++ b/src/backend/utils/activity/pgstat.c
@@ -159,6 +159,7 @@ static void pgstat_shared_reset_timestamp(PgStatKind kind, PgStatShm_StatEntryHe
static void pgstat_pending_delete(PgStatSharedRef *shared_ref);
static bool pgstat_pending_flush_stats(bool nowait);
+static PgStatKind pgstat_kind_for(char *kind_str);
static inline const PgStatKindInfo *pgstat_kind_info_for(PgStatKind kind);
@@ -1315,6 +1316,23 @@ pgstat_get_stat_snapshot_timestamp(bool *have_snapshot)
return 0;
}
+bool
+pgstat_shared_stat_exists(char *kind_str, Oid dboid, Oid objoid)
+{
+ PgStatKind kind = pgstat_kind_for(kind_str);
+
+ /*
+ * Ignore dboid or objoid for subscription and db stats respectively.
+ */
+ if (kind == PGSTAT_KIND_SUBSCRIPTION)
+ dboid = InvalidOid;
+
+ if (kind == PGSTAT_KIND_DB)
+ objoid = InvalidOid;
+
+ return (bool) pgstat_fetch_entry(kind, dboid, objoid);
+}
+
/* ------------------------------------------------------------
* Helper functions
@@ -1343,6 +1361,20 @@ pgstat_setup_memcxt(void)
ALLOCSET_SMALL_SIZES);
}
+static PgStatKind
+pgstat_kind_for(char *kind_str)
+{
+ for (int kind = 0; kind <= PGSTAT_KIND_LAST; kind++)
+ {
+ if (pg_strcasecmp(kind_str, pgstat_kind_infos[kind].name) == 0)
+ return kind;
+ }
+
+ ereport(ERROR,
+ (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+ errmsg("invalid statistic kind: \"%s\"", kind_str)));
+}
+
static inline const PgStatKindInfo *
pgstat_kind_info_for(PgStatKind kind)
{
diff --git a/src/backend/utils/adt/pgstatfuncs.c b/src/backend/utils/adt/pgstatfuncs.c
index 5d843cde24..62b2df856f 100644
--- a/src/backend/utils/adt/pgstatfuncs.c
+++ b/src/backend/utils/adt/pgstatfuncs.c
@@ -2420,3 +2420,25 @@ pg_stat_get_subscription_stats(PG_FUNCTION_ARGS)
/* Returns the record as Datum */
PG_RETURN_DATUM(HeapTupleGetDatum(heap_form_tuple(tupdesc, values, nulls)));
}
+
+
+/*
+ * Checks for presence of stats for object with provided object oid of kind
+ * specified in the type string in database of provided database oid.
+ *
+ * For subscription stats, only the objoid will be used. For database stats,
+ * only the dboid will be used. The value passed in for the unused parameter is
+ * discarded.
+ * TODO: should it be 'pg_stat_stats_present' instead of 'pg_stat_stats_exist'?
+ */
+Datum
+pg_stat_stats_exist(PG_FUNCTION_ARGS)
+{
+ Oid dboid = PG_GETARG_OID(0);
+ Oid objoid = PG_GETARG_OID(1);
+ char *stats_type = text_to_cstring(PG_GETARG_TEXT_P(2));
+
+ PG_RETURN_BOOL((bool) pgstat_shared_stat_exists(stats_type, dboid,
+ objoid));
+
+}
diff --git a/src/include/catalog/pg_proc.dat b/src/include/catalog/pg_proc.dat
index 75dae94c49..3f3c4e0427 100644
--- a/src/include/catalog/pg_proc.dat
+++ b/src/include/catalog/pg_proc.dat
@@ -5376,6 +5376,12 @@
proargmodes => '{i,o,o,o,o,o,o,o,o,o,o}',
proargnames => '{slot_name,slot_name,spill_txns,spill_count,spill_bytes,stream_txns,stream_count,stream_bytes,total_txns,total_bytes,stats_reset}',
prosrc => 'pg_stat_get_replication_slot' },
+
+{ oid => '8384', descr => 'statistics: checks if stats exist for provided object of provided type in provided database',
+ proname => 'pg_stat_stats_exist', provolatile => 's', proparallel => 'r',
+ prorettype => 'bool', proargtypes => 'oid oid text',
+ prosrc => 'pg_stat_stats_exist' },
+
{ oid => '8523', descr => 'statistics: information about subscription stats',
proname => 'pg_stat_get_subscription_stats', proisstrict => 'f',
provolatile => 's', proparallel => 'r',
diff --git a/src/include/pgstat.h b/src/include/pgstat.h
index 981f3e9e83..604c01312a 100644
--- a/src/include/pgstat.h
+++ b/src/include/pgstat.h
@@ -430,6 +430,9 @@ extern void pgstat_reset_single_counter(PgStatKind kind, Oid objectid);
extern void pgstat_reset_shared_counters(PgStatKind kind);
extern TimestampTz pgstat_get_stat_snapshot_timestamp(bool *have_snapshot);
+/* Helpers for testing */
+extern bool pgstat_shared_stat_exists(char *kind_str, Oid dboid, Oid objoid);
+
/* GUC parameters */
extern PGDLLIMPORT bool pgstat_track_counts;
extern PGDLLIMPORT int pgstat_track_functions;
diff --git a/src/test/recovery/t/029_stats_cleanup_replica.pl b/src/test/recovery/t/029_stats_cleanup_replica.pl
new file mode 100644
index 0000000000..650d662031
--- /dev/null
+++ b/src/test/recovery/t/029_stats_cleanup_replica.pl
@@ -0,0 +1,228 @@
+# Copyright (c) 2021-2022, PostgreSQL Global Development Group
+
+# Tests that statistics are removed from a physical replica after being dropped
+# on the primary
+
+use strict;
+use warnings;
+use PostgreSQL::Test::Cluster;
+use PostgreSQL::Test::Utils;
+use Test::More;
+
+# Initialize primary node
+my $node_primary = PostgreSQL::Test::Cluster->new('primary');
+# A specific role is created to perform some tests related to replication,
+# and it needs proper authentication configuration.
+$node_primary->init(
+ allows_streaming => 1,
+ auth_extra => [ '--create-role', 'repl_role' ]);
+$node_primary->start;
+my $backup_name = 'my_backup';
+
+# Set track_functions to all on primary
+$node_primary->append_conf('postgresql.conf', "track_functions = 'all'");
+$node_primary->reload;
+
+# Take backup
+$node_primary->backup($backup_name);
+
+# Create streaming standby linking to primary
+my $node_standby = PostgreSQL::Test::Cluster->new('standby');
+$node_standby->init_from_backup($node_primary, $backup_name,
+ has_streaming => 1);
+$node_standby->start;
+
+sub populate_standby_stats
+{
+ my ($node_primary, $node_standby, $connect_db, $schema) = @_;
+ # Create table on primary
+ $node_primary->safe_psql($connect_db,
+ "CREATE TABLE $schema.drop_tab_test1 AS SELECT generate_series(1,100) AS a");
+
+ # Create function on primary
+ $node_primary->safe_psql($connect_db,
+ "CREATE FUNCTION $schema.drop_func_test1() RETURNS VOID AS 'select 2;' LANGUAGE SQL IMMUTABLE");
+
+ # Wait for catchup
+ my $primary_lsn = $node_primary->lsn('write');
+ $node_primary->wait_for_catchup($node_standby, 'replay', $primary_lsn);
+
+ # Get database oid
+ my $dboid = $node_standby->safe_psql($connect_db,
+ "SELECT oid FROM pg_database WHERE datname = '$connect_db'");
+
+ # Get table oid
+ my $tableoid = $node_standby->safe_psql($connect_db,
+ "SELECT '$schema.drop_tab_test1'::regclass::oid");
+
+ # Do scan on standby
+ $node_standby->safe_psql($connect_db,
+ "SELECT * FROM $schema.drop_tab_test1");
+
+ # Get function oid
+ my $funcoid = $node_standby->safe_psql($connect_db,
+ "SELECT '$schema.drop_func_test1()'::regprocedure::oid");
+
+ # Call function on standby
+ $node_standby->safe_psql($connect_db,
+ "SELECT $schema.drop_func_test1()");
+
+ # Force flush of stats on standby
+ $node_standby->safe_psql($connect_db,
+ "SELECT pg_stat_force_next_flush()");
+
+ return ($dboid, $tableoid, $funcoid);
+}
+
+sub drop_function_by_oid
+{
+ my ($node_primary, $connect_db, $funcoid) = @_;
+
+ # Get function name from returned oid
+ my $func_name = $node_primary->safe_psql($connect_db,
+ "SELECT '$funcoid'::regprocedure");
+
+ # Drop function on primary
+ $node_primary->safe_psql($connect_db,
+ "DROP FUNCTION $func_name");
+}
+
+sub drop_table_by_oid
+{
+ my ($node_primary, $connect_db, $tableoid) = @_;
+
+ # Get table name from returned oid
+ my $table_name = $node_primary->safe_psql($connect_db,
+ "SELECT '$tableoid'::regclass");
+
+ # Drop table on primary
+ $node_primary->safe_psql($connect_db,
+ "DROP TABLE $table_name");
+}
+
+sub test_standby_func_tab_stats_status
+{
+ my ($node_standby, $connect_db, $dboid, $tableoid, $funcoid, $present) = @_;
+
+ is($node_standby->safe_psql($connect_db,
+ "SELECT pg_stat_stats_exist($dboid, $tableoid, 'table')"), $present,
+ "Check that table stats exist is '$present' on standby");
+
+ is($node_standby->safe_psql($connect_db,
+ "SELECT pg_stat_stats_exist($dboid, $funcoid, 'function')"), $present,
+ "Check that function stats exist is '$present' on standby");
+
+ return;
+}
+
+sub test_standby_db_stats_status
+{
+ my ($node_standby, $connect_db, $dboid, $present) = @_;
+
+ is($node_standby->safe_psql($connect_db,
+ "SELECT pg_stat_stats_exist($dboid, $dboid, 'db')"), $present,
+ "Check that db stats exist is '$present' on standby");
+}
+
+# Test that stats are cleaned up on standby after dropping table or function
+
+# Populate test objects
+my ($dboid, $tableoid, $funcoid) =
+ populate_standby_stats($node_primary, $node_standby, 'postgres', 'public');
+
+# Test that the stats are present
+test_standby_func_tab_stats_status($node_standby, 'postgres',
+ $dboid, $tableoid, $funcoid, 't');
+
+# Drop test objects
+drop_table_by_oid($node_primary, 'postgres', $tableoid);
+
+drop_function_by_oid($node_primary, 'postgres', $funcoid);
+
+# Wait for catchup
+my $primary_lsn = $node_primary->lsn('write');
+$node_primary->wait_for_catchup($node_standby, 'replay', $primary_lsn);
+
+# Force flush of stats on standby
+$node_standby->safe_psql('postgres',
+ "SELECT pg_stat_force_next_flush()");
+
+# Check table and function stats removed from standby
+test_standby_func_tab_stats_status($node_standby, 'postgres',
+ $dboid, $tableoid, $funcoid, 'f');
+
+# Check that stats are cleaned up on standby after dropping schema
+
+# Create schema
+$node_primary->safe_psql('postgres',
+ "CREATE SCHEMA drop_schema_test1");
+
+# Wait for catchup
+$primary_lsn = $node_primary->lsn('write');
+$node_primary->wait_for_catchup($node_standby, 'replay', $primary_lsn);
+
+# Populate test objects
+($dboid, $tableoid, $funcoid) = populate_standby_stats($node_primary,
+ $node_standby, 'postgres', 'drop_schema_test1');
+
+# Test that the stats are present
+test_standby_func_tab_stats_status($node_standby, 'postgres',
+ $dboid, $tableoid, $funcoid, 't');
+
+# Drop schema
+$node_primary->safe_psql('postgres',
+ "DROP SCHEMA drop_schema_test1 CASCADE");
+
+# Wait for catchup
+$primary_lsn = $node_primary->lsn('write');
+$node_primary->wait_for_catchup($node_standby, 'replay', $primary_lsn);
+
+# Force flush of stats on standby
+$node_standby->safe_psql('postgres',
+ "SELECT pg_stat_force_next_flush()");
+
+# Check table and function stats removed from standby
+test_standby_func_tab_stats_status($node_standby, 'postgres',
+ $dboid, $tableoid, $funcoid, 'f');
+
+# Test that stats are cleaned up on standby after dropping database
+
+# Create the database
+$node_primary->safe_psql('postgres',
+ "CREATE DATABASE test");
+
+# Wait for catchup
+$primary_lsn = $node_primary->lsn('write');
+$node_primary->wait_for_catchup($node_standby, 'replay', $primary_lsn);
+
+# Populate test objects
+($dboid, $tableoid, $funcoid) = populate_standby_stats($node_primary,
+ $node_standby, 'test', 'public');
+
+# Test that the stats are present
+test_standby_func_tab_stats_status($node_standby, 'test',
+ $dboid, $tableoid, $funcoid, 't');
+
+test_standby_db_stats_status($node_standby, 'test', $dboid, 't');
+
+# Drop db 'test' on primary
+$node_primary->safe_psql('postgres',
+ "DROP DATABASE test");
+
+# Wait for catchup
+$primary_lsn = $node_primary->lsn('write');
+$node_primary->wait_for_catchup($node_standby, 'replay', $primary_lsn);
+
+# Force flush of stats on standby
+$node_standby->safe_psql('postgres',
+ "SELECT pg_stat_force_next_flush()");
+
+# Test that the stats were cleaned up on standby
+# Note that this connects to 'postgres' but provides the dboid of dropped db
+# 'test' which was returned by previous routine
+test_standby_func_tab_stats_status($node_standby, 'postgres',
+ $dboid, $tableoid, $funcoid, 'f');
+
+test_standby_db_stats_status($node_standby, 'postgres', $dboid, 'f');
+
+done_testing();
--
2.30.2
[text/x-patch] v2-0002-Add-TAP-test-for-discarding-stats-after-crash.patch (5.9K, ../../CAAKRu_Yf3TqApws5O_+uWWhoOD=a=9XjzNgOE_ufvWqHCpFKWQ@mail.gmail.com/3-v2-0002-Add-TAP-test-for-discarding-stats-after-crash.patch)
download | inline diff:
From 5761e7436ea0df8ef6f56011a20085fb52c832b1 Mon Sep 17 00:00:00 2001
From: Melanie Plageman <[email protected]>
Date: Sun, 20 Mar 2022 16:33:40 -0400
Subject: [PATCH v2 2/2] Add TAP test for discarding stats after crash
Stats should not be restored if the db crashed. Also, ensure invalid
stats files are handled gracefully.
---
.../recovery/t/030_stats_cleanup_crash.pl | 184 ++++++++++++++++++
1 file changed, 184 insertions(+)
create mode 100644 src/test/recovery/t/030_stats_cleanup_crash.pl
diff --git a/src/test/recovery/t/030_stats_cleanup_crash.pl b/src/test/recovery/t/030_stats_cleanup_crash.pl
new file mode 100644
index 0000000000..dde3eaf04c
--- /dev/null
+++ b/src/test/recovery/t/030_stats_cleanup_crash.pl
@@ -0,0 +1,184 @@
+# Copyright (c) 2021-2022, PostgreSQL Global Development Group
+
+# Tests that statistics are discarded after a crash and that invalid stats
+# files are handled gracefully.
+
+use strict;
+use warnings;
+use PostgreSQL::Test::Cluster;
+use PostgreSQL::Test::Utils;
+use Test::More;
+use File::Copy;
+
+my $node = PostgreSQL::Test::Cluster->new('primary');
+$node->init(allows_streaming => 1);
+$node->start;
+$node->append_conf('postgresql.conf', "track_functions = 'all'");
+$node->reload;
+
+my $connect_db = 'postgres';
+my $db_under_test = 'test';
+
+$node->safe_psql($connect_db,
+ "CREATE DATABASE $db_under_test");
+
+# Create table in test db
+$node->safe_psql($db_under_test,
+ "CREATE TABLE tab_stats_crash_discard_test1 AS SELECT generate_series(1,100) AS a");
+
+# Create function in test db
+$node->safe_psql($db_under_test,
+ "CREATE FUNCTION func_stats_crash_discard1() RETURNS VOID AS 'select 2;' LANGUAGE SQL IMMUTABLE");
+
+# Get database oid
+my $dboid = $node->safe_psql($db_under_test,
+ "SELECT oid FROM pg_database WHERE datname = '$db_under_test'");
+
+# Get function oid
+my $funcoid = $node->safe_psql($db_under_test,
+ "SELECT 'func_stats_crash_discard1()'::regprocedure::oid");
+
+# Get table oid
+my $tableoid = $node->safe_psql($db_under_test,
+ "SELECT 'tab_stats_crash_discard_test1'::regclass::oid");
+
+# Do scan
+$node->safe_psql($db_under_test,
+ "SELECT * FROM tab_stats_crash_discard_test1");
+
+# Call function
+$node->safe_psql($db_under_test,
+ "SELECT func_stats_crash_discard1()");
+
+# Force flush of stats
+$node->safe_psql($db_under_test,
+ "SELECT pg_stat_force_next_flush()");
+
+is($node->safe_psql($connect_db,
+ "SELECT pg_stat_stats_exist($dboid, $dboid, 'db')"), 't',
+ "Check that db stats exist.");
+
+is($node->safe_psql($connect_db,
+ "SELECT pg_stat_stats_exist($dboid, $funcoid, 'function')"), 't',
+ "Check that function stats exist.");
+
+is($node->safe_psql($connect_db,
+ "SELECT pg_stat_stats_exist($dboid, $tableoid, 'table')"), 't',
+ "Check that table stats exist.");
+
+# Regular shutdown
+$node->stop();
+
+# Backup stats files
+my $statsfile = $PostgreSQL::Test::Utils::tmp_check . '/' . "discard_stats1";
+ok( !-f "$statsfile",
+ "Backup statsfile cannot already exist");
+
+my $datadir = $node->data_dir();
+my $og_stats = $datadir . '/' . "pg_stat" . '/' . "pgstat.stat";
+ok( -f "$og_stats",
+ "Origin stats file must exist");
+copy($og_stats, $statsfile) or die "Copy failed: $!";
+
+# Start the server
+$node->start;
+
+is($node->safe_psql($connect_db,
+ "SELECT pg_stat_stats_exist($dboid, $dboid, 'db')"), 't',
+ "Check that db stats exist.");
+
+is($node->safe_psql($connect_db,
+ "SELECT pg_stat_stats_exist($dboid, $funcoid, 'function')"), 't',
+ "Check that function stats exist.");
+
+is($node->safe_psql($connect_db,
+ "SELECT pg_stat_stats_exist($dboid, $tableoid, 'table')"), 't',
+ "Check that table stats exist.");
+
+# Fast shutdown
+$node->stop('immediate');
+
+ok( !-f "$og_stats",
+ "No stats file should exist after immediate shutdown.");
+
+# Copy the old stats back
+copy($statsfile, $og_stats) or die "Copy failed: $!";
+
+# Start the server
+$node->start;
+
+# Stats should have been discarded
+is($node->safe_psql($connect_db,
+ "SELECT pg_stat_stats_exist($dboid, $dboid, 'db')"), 'f',
+ "Check that db stats do not exist.");
+
+is($node->safe_psql($connect_db,
+ "SELECT pg_stat_stats_exist($dboid, $tableoid, 'table')"), 'f',
+ "Check that table stats do not exist.");
+
+is($node->safe_psql($connect_db,
+ "SELECT pg_stat_stats_exist($dboid, $funcoid, 'function')"), 'f',
+ "Check that function stats do not exist.");
+
+# Get rid of backup statsfile
+unlink $statsfile or die "cannot unlink $statsfile $!";
+
+# Start generating new stats
+
+# Do scan
+$node->safe_psql($db_under_test,
+ "SELECT * FROM tab_stats_crash_discard_test1");
+
+# Call function
+$node->safe_psql($db_under_test,
+ "SELECT func_stats_crash_discard1()");
+
+# Force flush of stats
+$node->safe_psql($db_under_test,
+ "SELECT pg_stat_force_next_flush()");
+
+is($node->safe_psql($connect_db,
+ "SELECT pg_stat_stats_exist($dboid, $dboid, 'db')"), 't',
+ "Check that db stats exist.");
+
+is($node->safe_psql($connect_db,
+ "SELECT pg_stat_stats_exist($dboid, $funcoid, 'function')"), 't',
+ "Check that function stats exist.");
+
+is($node->safe_psql($connect_db,
+ "SELECT pg_stat_stats_exist($dboid, $tableoid, 'table')"), 't',
+ "Check that table stats exist.");
+
+# Regular shutdown
+$node->stop();
+
+sub overwrite_file
+{
+ my $fh;
+ my ($filename, $str) = @_;
+ open my $fh, ">", $filename
+ or die "could not write \"$filename\": $!";
+ print $fh $str;
+ close $fh;
+ return;
+}
+
+overwrite_file($og_stats, "ZZZZZZZZZZZZZ");
+
+# Normal startup and no issues despite invalid stats file
+$node->start;
+
+# No stats present due to invalid stats file
+is($node->safe_psql($connect_db,
+ "SELECT pg_stat_stats_exist($dboid, $dboid, 'db')"), 'f',
+ "Check that db stats do not exist.");
+
+is($node->safe_psql($connect_db,
+ "SELECT pg_stat_stats_exist($dboid, $tableoid, 'table')"), 'f',
+ "Check that table stats do not exist.");
+
+is($node->safe_psql($connect_db,
+ "SELECT pg_stat_stats_exist($dboid, $funcoid, 'function')"), 'f',
+ "Check that function stats do not exist.");
+
+done_testing();
--
2.30.2
^ permalink raw reply [nested|flat] 21+ messages in thread
* Re: shared-memory based stats collector - v67
@ 2022-03-21 21:30 Andres Freund <[email protected]>
parent: Andres Freund <[email protected]>
3 siblings, 2 replies; 21+ messages in thread
From: Andres Freund @ 2022-03-21 21:30 UTC (permalink / raw)
To: Kyotaro Horiguchi <[email protected]>; Melanie Plageman <[email protected]>; +Cc: [email protected]; [email protected]; [email protected]; [email protected]
Hi,
Attached is v67 of the patch. Changes:
- I've committed a number of the earlier patches after polishing them some more
- lots of small cleanups, particularly around reducing unnecessary diff noise
- included Melanie's tests
On 2022-03-17 00:36:52 -0700, Andres Freund wrote:
> I think the first few patches are basically ready to be applied and are
> independently worthwhile:
> - 0001-pgstat-run-pgindent-on-pgstat.c-h.patch
> - 0002-pgstat-split-relation-database-stats-handling-ou.patch
> - 0003-pgstat-split-out-WAL-handling-from-pgstat_-initi.patch
> - 0004-pgstat-introduce-pgstat_relation_should_count.patch
> - 0005-pgstat-xact-level-cleanups-consolidation.patch
Committed.
> Might not be worth having separately, should probably just be part of
> 0014:
> - 0006-pgstat-wip-pgstat-relation-init-assoc.patch
Committed parts, the "assoc" stuff was moved into the main shared memory stats
patch.
> A pain to maintain, needs mostly a bit of polishing of file headers. Perhaps I
> should rename pgstat_checkpoint.c to pgstat_checkpointer.c, fits better with
> function names:
> - 0007-pgstat-split-different-types-of-stats-into-separ.patch
Committed.
> This is also painful to maintain. Mostly kept separate from 0007 for easier
> reviewing:
> - 0009-pgstat-reorder-file-pgstat.c-pgstat.h-contents.patch
Planning to commit this soon (it's now 0001). Doing a last few passes of
readthrough / polishing.
> I don't yet know what we should do with other users of
> PG_STAT_TMP_DIR. There's no need for it for pgstat.c et al anymore. Not sure
> that pg_stat_statement is enough of a reason to keep the stats_temp_directory
> GUC around?
> - 0019-pgstat-wip-remove-stats_temp_directory.patch
Still unclear. Might raise this separately for higher visibility.
> Right now we reset stats for replicas, even if we start from a shutdown
> checkpoint. That seems pretty unnecessary with this patch:
> - 0021-pgstat-wip-only-reset-pgstat-data-after-crash-re.patch
Might raise this in another thread for higher visibility.
> The biggest todos are:
> - Address all the remaining AFIXMEs and XXXs
> - add longer explanation of architecture to pgstat.c (or a README)
> - make naming not "a pain in the neck": [1]
> - lots of polishing
> - run benchmarks - I've done so in the past, but not recently
Still TBD
> - revise docs
Kyotaro-san, maybe you could do a first pass?
> - Further improve our stats test coverage - there's a crapton not covered,
> despite 0017:
> - test WAL replay with stats (stats for dropped tables are removed etc)
> - test crash recovery and "invalid stats file" paths
> - lot of the pg_stat_ views like bgwriter, pg_stat_database have zero coverage today
That's gotten a lot better with Melanie's tests, still a bit further to go. I
think she's found at least one more small bug that's not yet fixed here.
> - perhaps 0014 can be further broken down - it's still uncomfortably large
Things that I think can be split out:
- Encapsulate "if (pgStatSock == PGINVALID_SOCKET || !pgstat_track_counts)"
style tests in a helper function. Then just the body needs to be changed,
rather than a lot of places needing such checks.
Yep, that's it. I don't really see anything else that wouldn't be too
awkward. Would welcome suggestions!.
Greetings,
Andres Freund
Attachments:
[application/x-patch-gzip] v67-0001-pgstat-reorder-pgstat.-ch-contents.patch.gz (11.8K, ../../[email protected]/2-v67-0001-pgstat-reorder-pgstat.-ch-contents.patch.gz)
download
[application/x-patch-gzip] v67-0002-pgstat-add-pgstat_copy_relation_stats.patch.gz (1.4K, ../../[email protected]/3-v67-0002-pgstat-add-pgstat_copy_relation_stats.patch.gz)
download
[application/x-patch-gzip] v67-0003-pgstat-fix-function-name-in-comment.patch.gz (609B, ../../[email protected]/4-v67-0003-pgstat-fix-function-name-in-comment.patch.gz)
download
[application/x-patch-gzip] v67-0004-pgstat-remove-some-superflous-comments-from-pgst.patch.gz (1.3K, ../../[email protected]/5-v67-0004-pgstat-remove-some-superflous-comments-from-pgst.patch.gz)
download
[application/x-patch-gzip] v67-0005-pgstat-stats-collector-references-in-comments.patch.gz (8.6K, ../../[email protected]/6-v67-0005-pgstat-stats-collector-references-in-comments.patch.gz)
download
[application/x-patch-gzip] v67-0006-pgstat-scaffolding-for-transactional-stats-creat.patch.gz (8.1K, ../../[email protected]/7-v67-0006-pgstat-scaffolding-for-transactional-stats-creat.patch.gz)
download
[application/x-patch-gzip] v67-0007-pgstat-store-statistics-in-shared-memory.patch.gz (89.3K, ../../[email protected]/8-v67-0007-pgstat-store-statistics-in-shared-memory.patch.gz)
download
[application/x-patch-gzip] v67-0008-pgstat-add-pg_stat_force_next_flush.patch.gz (1.6K, ../../[email protected]/9-v67-0008-pgstat-add-pg_stat_force_next_flush.patch.gz)
download
[application/x-patch-gzip] v67-0009-pgstat-utilize-pg_stat_force_next_flush-to-simpl.patch.gz (4.4K, ../../[email protected]/10-v67-0009-pgstat-utilize-pg_stat_force_next_flush-to-simpl.patch.gz)
download
[application/x-patch-gzip] v67-0010-pgstat-test-transaction-behaviour-2PC-function-s.patch.gz (5.6K, ../../[email protected]/11-v67-0010-pgstat-test-transaction-behaviour-2PC-function-s.patch.gz)
download
[application/x-patch-gzip] v67-0011-pgstat-wip-only-reset-pgstat-data-after-crash-re.patch.gz (1.1K, ../../[email protected]/12-v67-0011-pgstat-wip-only-reset-pgstat-data-after-crash-re.patch.gz)
download
[application/x-patch-gzip] v67-0012-pgstat-test-test-stats-interactions-with-streami.patch.gz (3.7K, ../../[email protected]/13-v67-0012-pgstat-test-test-stats-interactions-with-streami.patch.gz)
download
[application/x-patch-gzip] v67-0013-pgstat-test-discarding-stats-after-crash.patch.gz (1.7K, ../../[email protected]/14-v67-0013-pgstat-test-discarding-stats-after-crash.patch.gz)
download
[application/x-patch-gzip] v67-0014-pgstat-test-subscriber-stats-reset-and-drop.patch.gz (2.6K, ../../[email protected]/15-v67-0014-pgstat-test-subscriber-stats-reset-and-drop.patch.gz)
download
[application/x-patch-gzip] v67-0015-pgstat-wip-remove-stats_temp_directory.patch.gz (3.0K, ../../[email protected]/16-v67-0015-pgstat-wip-remove-stats_temp_directory.patch.gz)
download
[application/x-patch-gzip] v67-0016-pgstat-rename-STATS_COLLECTOR-GUC-group-to-STATS.patch.gz (1.4K, ../../[email protected]/17-v67-0016-pgstat-rename-STATS_COLLECTOR-GUC-group-to-STATS.patch.gz)
download
[application/x-patch-gzip] v67-0017-pgstat-move-pgstat.c-to-utils-activity.patch.gz (918B, ../../[email protected]/18-v67-0017-pgstat-move-pgstat.c-to-utils-activity.patch.gz)
download
^ permalink raw reply [nested|flat] 21+ messages in thread
* Re: shared-memory based stats collector - v66
@ 2022-03-21 21:41 Melanie Plageman <[email protected]>
parent: Melanie Plageman <[email protected]>
0 siblings, 0 replies; 21+ messages in thread
From: Melanie Plageman @ 2022-03-21 21:41 UTC (permalink / raw)
To: Andres Freund <[email protected]>; +Cc: Kyotaro Horiguchi <[email protected]>; Ibrar Ahmed <[email protected]>; [email protected]; [email protected]; PostgreSQL Hackers <[email protected]>
On Sun, Mar 20, 2022 at 4:56 PM Melanie Plageman
<[email protected]> wrote:
>
> Addressed all of these points in
> v2-0001-add-replica-cleanup-tests.patch
>
> also added a new test file in
> v2-0002-Add-TAP-test-for-discarding-stats-after-crash.patch
> testing correct behavior after a crash and when stats file is invalid
>
Attached is the last of the tests confirming clean up for stats in the
shared stats hashtable (these are for the subscription stats).
I thought that maybe these tests could now use
pg_stat_force_next_flush() instead of poll_query_until() but I wasn't
sure how to ensure that the error has happened and the pending entry has
been added before setting force_next_flush.
I also added in tests that resetting subscription stats works as
expected.
- Melanie
Attachments:
[text/x-patch] add_subscriber_stats_tests.patch (11.3K, ../../CAAKRu_Zu=8WD3sHyiU-b_DPmP5ic_9CHw9S1vAex44VcNUUNQA@mail.gmail.com/2-add_subscriber_stats_tests.patch)
download | inline diff:
From ffb83cc6ad2941f1d01b42b55dd0615a011d59cf Mon Sep 17 00:00:00 2001
From: Melanie Plageman <[email protected]>
Date: Mon, 21 Mar 2022 14:52:55 -0400
Subject: [PATCH] add subscriber stats reset and drop tests
---
src/test/subscription/t/026_stats.pl | 303 ++++++++++++++++++++-------
1 file changed, 230 insertions(+), 73 deletions(-)
diff --git a/src/test/subscription/t/026_stats.pl b/src/test/subscription/t/026_stats.pl
index a42ea3170e..e86bfb4fea 100644
--- a/src/test/subscription/t/026_stats.pl
+++ b/src/test/subscription/t/026_stats.pl
@@ -18,83 +18,240 @@ my $node_subscriber = PostgreSQL::Test::Cluster->new('subscriber');
$node_subscriber->init(allows_streaming => 'logical');
$node_subscriber->start;
-# Initial table setup on both publisher and subscriber. On subscriber we
-# create the same tables but with primary keys. Also, insert some data that
-# will conflict with the data replicated from publisher later.
-$node_publisher->safe_psql(
- 'postgres',
- qq[
-BEGIN;
-CREATE TABLE test_tab1 (a int);
-INSERT INTO test_tab1 VALUES (1);
-COMMIT;
-]);
-$node_subscriber->safe_psql(
- 'postgres',
- qq[
-BEGIN;
-CREATE TABLE test_tab1 (a int primary key);
-INSERT INTO test_tab1 VALUES (1);
-COMMIT;
-]);
-
-# Setup publication.
-my $publisher_connstr = $node_publisher->connstr . ' dbname=postgres';
-$node_publisher->safe_psql('postgres',
- "CREATE PUBLICATION tap_pub FOR TABLE test_tab1;");
+
+sub create_sub_pub_w_errors
+{
+ my ($publisher, $subscriber, $db, $table_name) = @_;
+ # Initial table setup on both publisher and subscriber. On subscriber we
+ # create the same tables but with primary keys. Also, insert some data that
+ # will conflict with the data replicated from publisher later.
+ $publisher->safe_psql(
+ $db,
+ qq[
+ BEGIN;
+ CREATE TABLE $table_name(a int);
+ INSERT INTO $table_name VALUES (1);
+ COMMIT;
+ ]);
+ $subscriber->safe_psql(
+ $db,
+ qq[
+ BEGIN;
+ CREATE TABLE $table_name(a int primary key);
+ INSERT INTO $table_name VALUES (1);
+ COMMIT;
+ ]);
+
+ # Set up publication.
+ my $pub_name = $table_name . '_pub';
+ my $publisher_connstr = $publisher->connstr . qq( dbname=$db);
+
+ $publisher->safe_psql($db,
+ qq(CREATE PUBLICATION $pub_name FOR TABLE $table_name));
+
+ # Create subscription. The tablesync for table on subscription will enter into
+ # infinite error loop due to violating the unique constraint.
+ my $sub_name = $table_name . '_sub';
+ $subscriber->safe_psql($db,
+ qq(CREATE SUBSCRIPTION $sub_name CONNECTION '$publisher_connstr' PUBLICATION $pub_name)
+ );
+
+ $publisher->wait_for_catchup($sub_name);
+
+ # TODO: can this be replaced with pg_stat_force_next_flush() and a test
+ # that sync error > 0?
+ # Wait for the tablesync error to be reported.
+ $node_subscriber->poll_query_until(
+ $db,
+ qq[
+ SELECT sync_error_count > 0
+ FROM pg_stat_subscription_stats
+ WHERE subname = '$sub_name'
+ ]) or die qq(Timed out while waiting for tablesync errors for subscription '$sub_name');
+
+ # Truncate test_tab1 so that tablesync worker can continue.
+ $subscriber->safe_psql($db, qq(TRUNCATE $table_name));
+
+ # Wait for initial tablesync to finish.
+ $subscriber->poll_query_until(
+ $db,
+ qq[
+ SELECT count(1) = 1 FROM pg_subscription_rel
+ WHERE srrelid = '$table_name'::regclass AND srsubstate in ('r', 's')
+ ]) or die qq(Timed out while waiting for subscriber to synchronize data for table '$table_name'.);
+
+ # Check test table on the subscriber has one row.
+ my $result = $subscriber->safe_psql($db, qq(SELECT a FROM $table_name));
+ is($result, qq(1), qq(Check that table '$table_name' now has 1 row.));
+
+ # Insert data to test table on the publisher, raising an error on the
+ # subscriber due to violation of the unique constraint on test table.
+ $publisher->safe_psql($db, qq(INSERT INTO $table_name VALUES (1)));
+
+ # TODO: can this be replaced with a pg_stat_force_next_flush() and a test
+ # that apply error > 0?
+ # Wait for the apply error to be reported.
+ $subscriber->poll_query_until(
+ $db,
+ qq[
+ SELECT apply_error_count > 0
+ FROM pg_stat_subscription_stats
+ WHERE subname = '$sub_name'
+ ]) or die qq(Timed out while waiting for apply error for subscription '$sub_name');
+
+ # Truncate test table so that apply worker can continue.
+ $subscriber->safe_psql($db, qq(TRUNCATE $table_name));
+
+ return ($pub_name, $sub_name);
+}
+
+my $db = 'postgres';
# There shouldn't be any subscription errors before starting logical replication.
-my $result = $node_subscriber->safe_psql('postgres',
- "SELECT count(1) FROM pg_stat_subscription_stats");
-is($result, qq(0), 'check no subscription error');
-
-# Create subscription. The tablesync for test_tab1 on tap_sub will enter into
-# infinite error loop due to violating the unique constraint.
-$node_subscriber->safe_psql('postgres',
- "CREATE SUBSCRIPTION tap_sub CONNECTION '$publisher_connstr' PUBLICATION tap_pub;"
+my $result = $node_subscriber->safe_psql($db,
+ qq(SELECT count(1) FROM pg_stat_subscription_stats));
+is($result, qq(0), 'Check that there are no subscription errors before starting logical replication.');
+
+# Create the publication and subscription with sync and apply errors
+my $table1_name = 'test_tab1';
+my ($pub1_name, $sub1_name) = create_sub_pub_w_errors($node_publisher, $node_subscriber, $db, $table1_name);
+
+# Apply and Sync errors are > 0 and reset timestamp is NULL
+is($node_subscriber->safe_psql(
+ $db,
+ qq(SELECT apply_error_count > 0,
+ sync_error_count > 0,
+ stats_reset IS NULL
+ FROM pg_stat_subscription_stats
+ WHERE subname = '$sub1_name')
+), qq(t|t|t),
+qq(Check that apply errors and sync errors are both > 0 and stats_reset is NULL for subscription '$sub1_name'.));
+
+# Reset a single subscription
+$node_subscriber->safe_psql(
+ $db,
+ qq(SELECT pg_stat_reset_subscription_stats((SELECT subid FROM pg_stat_subscription_stats WHERE subname = '$sub1_name')))
+);
+
+# Apply and Sync errors are 0 and stats reset is not NULL
+is($node_subscriber->safe_psql(
+ $db,
+ qq(SELECT apply_error_count = 0,
+ sync_error_count = 0,
+ stats_reset IS NOT NULL
+ FROM pg_stat_subscription_stats
+ WHERE subname = '$sub1_name')
+), qq(t|t|t),
+qq(Confirm that apply errors and sync errors are both 0 and stats_reset is not NULL after reset for subscription '$sub1_name'.));
+
+# Get reset timestamp
+my $reset_time1 = $node_subscriber->safe_psql(
+ $db,
+ qq(SELECT stats_reset FROM pg_stat_subscription_stats WHERE subname = '$sub1_name')
+);
+
+# Reset single sub again
+$node_subscriber->safe_psql(
+ $db,
+ qq(SELECT pg_stat_reset_subscription_stats((SELECT subid FROM
+ pg_stat_subscription_stats WHERE subname = '$sub1_name')))
+);
+
+# check reset timestamp is newer after reset
+is($node_subscriber->safe_psql(
+ $db,
+ qq(SELECT stats_reset > '$reset_time1'::timestamptz FROM
+ pg_stat_subscription_stats WHERE subname = '$sub1_name')
+), qq(t), qq(Check reset timestamp for '$sub1_name' is newer after second reset.));
+
+# Make second subscription and publication
+my $table2_name = 'test_tab2';
+my ($pub2_name, $sub2_name) = create_sub_pub_w_errors($node_publisher, $node_subscriber, $db, $table2_name);
+
+# Apply and Sync errors are > 0 and reset timestamp is NULL
+is($node_subscriber->safe_psql(
+ $db,
+ qq(SELECT apply_error_count > 0,
+ sync_error_count > 0,
+ stats_reset IS NULL
+ FROM pg_stat_subscription_stats
+ WHERE subname = '$sub2_name')
+), qq(t|t|t), qq(Confirm that apply errors and sync errors are both > 0 and stats_reset is NULL for sub '$sub2_name'.));
+
+# Reset all subscriptions
+$node_subscriber->safe_psql(
+ $db,
+ qq(SELECT pg_stat_reset_subscription_stats(NULL))
+);
+
+# Apply and Sync errors are 0 and stats reset is not NULL
+is($node_subscriber->safe_psql(
+ $db,
+ qq(SELECT apply_error_count = 0,
+ sync_error_count = 0,
+ stats_reset IS NOT NULL
+ FROM pg_stat_subscription_stats
+ WHERE subname = '$sub1_name')
+), qq(t|t|t), qq(Confirm that apply errors and sync errors are both 0 and stats_reset is not NULL for sub '$sub1_name' after reset.));
+
+is($node_subscriber->safe_psql(
+ $db,
+ qq(SELECT apply_error_count = 0,
+ sync_error_count = 0,
+ stats_reset IS NOT NULL
+ FROM pg_stat_subscription_stats
+ WHERE subname = '$sub2_name')
+), qq(t|t|t), qq(Confirm that apply errors and sync errors are both 0 and stats_reset is not NULL for sub '$sub2_name' after reset.));
+
+# Get reset timestamp 1
+$reset_time1 = $node_subscriber->safe_psql(
+ $db,
+ qq(SELECT stats_reset FROM pg_stat_subscription_stats WHERE subname = '$sub1_name')
+);
+
+# Get reset timestamp 2
+my $reset_time2 = $node_subscriber->safe_psql(
+ $db,
+ qq(SELECT stats_reset FROM pg_stat_subscription_stats WHERE subname = '$sub2_name')
+);
+
+# Reset all subscriptions
+$node_subscriber->safe_psql(
+ $db,
+ qq(SELECT pg_stat_reset_subscription_stats(NULL))
+);
+
+# check reset timestamp for sub1 is newer after reset
+is($node_subscriber->safe_psql(
+ $db,
+ qq(SELECT stats_reset > '$reset_time1'::timestamptz FROM
+ pg_stat_subscription_stats WHERE subname = '$sub1_name')
+), qq(t), qq(Confirm that reset timestamp for '$sub1_name' is newer after second reset.));
+
+# check reset timestamp for sub2 is newer after reset
+is($node_subscriber->safe_psql(
+ $db,
+ qq(SELECT stats_reset > '$reset_time2'::timestamptz FROM
+ pg_stat_subscription_stats WHERE subname = '$sub2_name')
+), qq(t), qq(Confirm that reset timestamp for '$sub2_name' is newer after second reset.));
+
+# Get subscription 1 oid
+my $sub1_oid = $node_subscriber->safe_psql($db,
+ qq(SELECT oid FROM pg_subscription WHERE subname = '$sub1_name')
+);
+
+# Drop subscription 1
+$node_subscriber->safe_psql(
+ $db,
+ qq(DROP SUBSCRIPTION $sub1_name)
);
-$node_publisher->wait_for_catchup('tap_sub');
-
-# Wait for the tablesync error to be reported.
-$node_subscriber->poll_query_until(
- 'postgres',
- qq[
-SELECT sync_error_count > 0
-FROM pg_stat_subscription_stats
-WHERE subname = 'tap_sub'
-]) or die "Timed out while waiting for tablesync error";
-
-# Truncate test_tab1 so that tablesync worker can continue.
-$node_subscriber->safe_psql('postgres', "TRUNCATE test_tab1;");
-
-# Wait for initial tablesync for test_tab1 to finish.
-$node_subscriber->poll_query_until(
- 'postgres',
- qq[
-SELECT count(1) = 1 FROM pg_subscription_rel
-WHERE srrelid = 'test_tab1'::regclass AND srsubstate in ('r', 's')
-]) or die "Timed out while waiting for subscriber to synchronize data";
-
-# Check test_tab1 on the subscriber has one row.
-$result = $node_subscriber->safe_psql('postgres', "SELECT a FROM test_tab1");
-is($result, qq(1), 'check the table has now row');
-
-# Insert data to test_tab1 on the publisher, raising an error on the subscriber
-# due to violation of the unique constraint on test_tab1.
-$node_publisher->safe_psql('postgres', "INSERT INTO test_tab1 VALUES (1)");
-
-# Wait for the apply error to be reported.
-$node_subscriber->poll_query_until(
- 'postgres',
- qq[
-SELECT apply_error_count > 0
-FROM pg_stat_subscription_stats
-WHERE subname = 'tap_sub'
-]) or die "Timed out while waiting for apply error";
-
-# Truncate test_tab1 so that apply worker can continue.
-$node_subscriber->safe_psql('postgres', "TRUNCATE test_tab1;");
+# Subscription stats for sub1 should be gone
+is($node_subscriber->safe_psql(
+ $db,
+ qq(SELECT pg_stat_stats_exist($sub1_oid, $sub1_oid, 'subscription'))
+ ), qq(f), qq(Subscription stats for subscription '$sub1_name' should be removed.));
+
$node_subscriber->stop('fast');
$node_publisher->stop('fast');
--
2.30.2
^ permalink raw reply [nested|flat] 21+ messages in thread
* Re: shared-memory based stats collector - v67
@ 2022-03-22 02:56 Kyotaro Horiguchi <[email protected]>
parent: Andres Freund <[email protected]>
1 sibling, 1 reply; 21+ messages in thread
From: Kyotaro Horiguchi @ 2022-03-22 02:56 UTC (permalink / raw)
To: [email protected]; +Cc: [email protected]; [email protected]; [email protected]; [email protected]; [email protected]
At Mon, 21 Mar 2022 14:30:17 -0700, Andres Freund <[email protected]> wrote in
> Hi,
>
> Attached is v67 of the patch. Changes:
Thanks for the lot of work on this.
> > This is also painful to maintain. Mostly kept separate from 0007 for easier
> > reviewing:
> > - 0009-pgstat-reorder-file-pgstat.c-pgstat.h-contents.patch
>
> Planning to commit this soon (it's now 0001). Doing a last few passes of
> readthrough / polishing.
This looks like committed.
> > I don't yet know what we should do with other users of
> > PG_STAT_TMP_DIR. There's no need for it for pgstat.c et al anymore. Not sure
> > that pg_stat_statement is enough of a reason to keep the stats_temp_directory
> > GUC around?
> > - 0019-pgstat-wip-remove-stats_temp_directory.patch
>
> Still unclear. Might raise this separately for higher visibility.
>
>
> > Right now we reset stats for replicas, even if we start from a shutdown
> > checkpoint. That seems pretty unnecessary with this patch:
> > - 0021-pgstat-wip-only-reset-pgstat-data-after-crash-re.patch
>
> Might raise this in another thread for higher visibility.
>
>
> > The biggest todos are:
> > - Address all the remaining AFIXMEs and XXXs
> > - add longer explanation of architecture to pgstat.c (or a README)
> > - make naming not "a pain in the neck": [1]
> > - lots of polishing
> > - run benchmarks - I've done so in the past, but not recently
>
> Still TBD
> > - revise docs
>
> Kyotaro-san, maybe you could do a first pass?
Docs.. Yeah I'll try it.
> > - Further improve our stats test coverage - there's a crapton not covered,
> > despite 0017:
> > - test WAL replay with stats (stats for dropped tables are removed etc)
> > - test crash recovery and "invalid stats file" paths
> > - lot of the pg_stat_ views like bgwriter, pg_stat_database have zero coverage today
>
> That's gotten a lot better with Melanie's tests, still a bit further to go. I
> think she's found at least one more small bug that's not yet fixed here.
>
>
> > - perhaps 0014 can be further broken down - it's still uncomfortably large
>
> Things that I think can be split out:
> - Encapsulate "if (pgStatSock == PGINVALID_SOCKET || !pgstat_track_counts)"
> style tests in a helper function. Then just the body needs to be changed,
> rather than a lot of places needing such checks.
>
> Yep, that's it. I don't really see anything else that wouldn't be too
> awkward. Would welcome suggestions!.
I'm overwhelmed by the amout, but I'm going to look into them.
regards.
--
Kyotaro Horiguchi
NTT Open Source Software Center
^ permalink raw reply [nested|flat] 21+ messages in thread
* Re: shared-memory based stats collector - v66
@ 2022-03-23 00:43 Melanie Plageman <[email protected]>
parent: Andres Freund <[email protected]>
3 siblings, 0 replies; 21+ messages in thread
From: Melanie Plageman @ 2022-03-23 00:43 UTC (permalink / raw)
To: Andres Freund <[email protected]>; +Cc: Kyotaro Horiguchi <[email protected]>; Ibrar Ahmed <[email protected]>; [email protected]; [email protected]; PostgreSQL Hackers <[email protected]>
On Thu, Mar 17, 2022 at 3:36 AM Andres Freund <[email protected]> wrote:
> I've attached a substantially improved version of the shared memory stats
> patch.
...
> - lot of the pg_stat_ views like bgwriter, pg_stat_database have zero coverage today
Attached are some tests including tests that reset of stats works for
all views having a reset timestamp as well as a basic test for at least
one column in all of the following stats views:
pg_stat_archiver, pg_stat_bgwriter, pg_stat_wal, pg_stat_slru,
pg_stat_replication_slots, pg_stat_database
It might be nice to have a test for one of the columns fetched from the
PgStatBgwriter data structure since those and the Checkpointer stats are
stored separately despite being displayed in the same view currently.
but, alas...
- Melanie
Attachments:
[text/x-patch] add_more_basic_stats_tests.patch (20.2K, ../../CAAKRu_Z13Pbatko5AQbCm+wwYK=3Yix1_=x3QoD-0Ket9h47og@mail.gmail.com/2-add_more_basic_stats_tests.patch)
download | inline diff:
From 37e5ba3b7743309b00c81dbfe65cfd481d4859a6 Mon Sep 17 00:00:00 2001
From: Melanie Plageman <[email protected]>
Date: Tue, 22 Mar 2022 16:53:32 -0400
Subject: [PATCH] test
---
src/test/isolation/expected/stats.out | 35 ++++
src/test/isolation/specs/stats.spec | 31 +++
src/test/recovery/t/006_logical_decoding.pl | 63 ++++++
src/test/regress/expected/stats.out | 208 ++++++++++++++++++++
src/test/regress/sql/stats.sql | 107 ++++++++++
5 files changed, 444 insertions(+)
diff --git a/src/test/isolation/expected/stats.out b/src/test/isolation/expected/stats.out
index ca553be3bf..7a156e1da2 100644
--- a/src/test/isolation/expected/stats.out
+++ b/src/test/isolation/expected/stats.out
@@ -1895,3 +1895,38 @@ name |pg_stat_get_function_calls|total_above_zero|self_above_zero
test_stat_func| 5|t |t
(1 row)
+
+starting permutation: s1_slru_save_stats s1_listen s1_big_notify s1_ff s1_slru_check_stats
+step s1_slru_save_stats:
+ INSERT INTO test_slru_stats VALUES('Notify', 'blks_zeroed',
+ (SELECT blks_zeroed FROM pg_stat_slru WHERE name = 'Notify'));
+
+step s1_listen: LISTEN big_notify;
+step s1_big_notify: SELECT pg_notify('big_notify',
+ repeat('0', current_setting('block_size')::int / 2)) FROM generate_series(1, 2);
+
+pg_notify
+---------
+
+
+(2 rows)
+
+s1: NOTIFY "big_notify" with payload "0000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000" from s1
+step s1_ff: SELECT pg_stat_force_next_flush();
+pg_stat_force_next_flush
+------------------------
+
+(1 row)
+
+step s1_slru_check_stats:
+ SELECT current.blks_zeroed > before.value
+ FROM test_slru_stats before
+ INNER JOIN pg_stat_slru current
+ ON before.slru = current.name
+ WHERE before.stat = 'blks_zeroed';
+
+?column?
+--------
+t
+(1 row)
+
diff --git a/src/test/isolation/specs/stats.spec b/src/test/isolation/specs/stats.spec
index 752a71c478..4970b5f9d4 100644
--- a/src/test/isolation/specs/stats.spec
+++ b/src/test/isolation/specs/stats.spec
@@ -11,11 +11,14 @@ setup
CREATE FUNCTION test_stat_func2() RETURNS VOID LANGUAGE plpgsql AS $$BEGIN END;$$;
INSERT INTO test_stat_oid(name, oid) VALUES('test_stat_func2', 'test_stat_func2'::regproc);
+
+ CREATE TABLE test_slru_stats(slru TEXT, stat TEXT, value INT);
}
teardown
{
DROP TABLE test_stat_oid;
+ DROP TABLE test_slru_stats;
DROP TABLE IF EXISTS test_stat_tab;
DROP FUNCTION IF EXISTS test_stat_func();
@@ -58,8 +61,25 @@ step "s1_func_stats2" {
FROM test_stat_oid AS tso
WHERE tso.name = 'test_stat_func2'
}
+step "s1_slru_save_stats" {
+ INSERT INTO test_slru_stats VALUES('Notify', 'blks_zeroed',
+ (SELECT blks_zeroed FROM pg_stat_slru WHERE name = 'Notify'));
+}
+step "s1_listen" { LISTEN big_notify; }
+step "s1_big_notify" { SELECT pg_notify('big_notify',
+ repeat('0', current_setting('block_size')::int / 2)) FROM generate_series(1, 2);
+ }
+
+step "s1_slru_check_stats" {
+ SELECT current.blks_zeroed > before.value
+ FROM test_slru_stats before
+ INNER JOIN pg_stat_slru current
+ ON before.slru = current.name
+ WHERE before.stat = 'blks_zeroed';
+ }
#step "s1_func_stats_debug" {SELECT * FROM pg_stat_user_functions;}
+
session "s2"
setup { SET stats_fetch_consistency = 'none'; }
step "s2_track_funcs_all" { SET track_functions = 'all'; }
@@ -285,3 +305,14 @@ permutation
"s1_func_stats"
"s2_rollback_prepared_a"
"s1_func_stats"
+
+######################
+# SLRU stats tests
+######################
+
+permutation
+ "s1_slru_save_stats"
+ "s1_listen"
+ "s1_big_notify"
+ "s1_ff"
+ "s1_slru_check_stats"
diff --git a/src/test/recovery/t/006_logical_decoding.pl b/src/test/recovery/t/006_logical_decoding.pl
index 9cec2792fc..3ccced2ea2 100644
--- a/src/test/recovery/t/006_logical_decoding.pl
+++ b/src/test/recovery/t/006_logical_decoding.pl
@@ -200,6 +200,69 @@ chomp($logical_restart_lsn_post);
ok(($logical_restart_lsn_pre cmp $logical_restart_lsn_post) == 0,
"logical slot advance persists across restarts");
+my $stats_test_slot1 = 'test_slot';
+my $stats_test_slot2 = 'logical_slot';
+
+# Test that reset works for pg_stat_replication_slots
+
+# Stats exist for stats test slot 1
+is($node_primary->safe_psql(
+ 'postgres',
+ qq(SELECT total_bytes > 0, stats_reset IS NULL FROM pg_stat_replication_slots WHERE slot_name = '$stats_test_slot1')
+), qq(t|t), qq(Total bytes is > 0 and stats_reset is NULL for slot '$stats_test_slot1'.));
+
+# Do reset of stats for stats test slot 1
+$node_primary->safe_psql(
+ 'postgres',
+ qq(SELECT pg_stat_reset_replication_slot('$stats_test_slot1'))
+);
+
+# Get reset value after reset
+my $reset1 = $node_primary->safe_psql(
+ 'postgres',
+ qq(SELECT stats_reset FROM pg_stat_replication_slots WHERE slot_name = '$stats_test_slot1')
+);
+
+# Do reset again
+$node_primary->safe_psql(
+ 'postgres',
+ qq(SELECT pg_stat_reset_replication_slot('$stats_test_slot1'))
+);
+
+is($node_primary->safe_psql(
+ 'postgres',
+ qq(SELECT stats_reset > '$reset1'::timestamptz, total_bytes = 0 FROM pg_stat_replication_slots WHERE slot_name = '$stats_test_slot1')
+), qq(t|t), qq(Check that reset timestamp is later after the second reset of stats for slot '$stats_test_slot1' and confirm total_bytes was set to 0.));
+
+# Check that test slot 2 has NULL in reset timestamp
+is($node_primary->safe_psql(
+ 'postgres',
+ qq(SELECT stats_reset IS NULL FROM pg_stat_replication_slots WHERE slot_name = '$stats_test_slot2')
+), qq(t), qq(Stats_reset is NULL for slot '$stats_test_slot2' before reset.));
+
+# Get reset value again for test slot 1
+$reset1 = $node_primary->safe_psql(
+ 'postgres',
+ qq(SELECT stats_reset FROM pg_stat_replication_slots WHERE slot_name = '$stats_test_slot1')
+);
+
+# Reset stats for all replication slots
+$node_primary->safe_psql(
+ 'postgres',
+ qq(SELECT pg_stat_reset_replication_slot(NULL))
+);
+
+# Check that test slot 2 reset timestamp is no longer NULL after reset
+is($node_primary->safe_psql(
+ 'postgres',
+ qq(SELECT stats_reset IS NOT NULL FROM pg_stat_replication_slots WHERE slot_name = '$stats_test_slot2')
+), qq(t), qq(Stats_reset is not NULL for slot '$stats_test_slot2' after reset all.));
+
+is($node_primary->safe_psql(
+ 'postgres',
+ qq(SELECT stats_reset > '$reset1'::timestamptz FROM pg_stat_replication_slots WHERE slot_name = '$stats_test_slot1')
+), qq(t), qq(Check that reset timestamp is later after resetting stats for slot '$stats_test_slot1' again.));
+
# done with the node
$node_primary->stop;
diff --git a/src/test/regress/expected/stats.out b/src/test/regress/expected/stats.out
index 741c2d3e4f..da81038f8c 100644
--- a/src/test/regress/expected/stats.out
+++ b/src/test/regress/expected/stats.out
@@ -515,4 +515,212 @@ SELECT pg_stat_get_function_calls(:stats_test_func2_oid);
DROP TABLE trunc_stats_test, trunc_stats_test1, trunc_stats_test2, trunc_stats_test3, trunc_stats_test4;
DROP TABLE prevstats;
+-----
+-- Test that various stats views are being properly populated
+-----
+-- Test that sessions is incremented when a new session is started in pg_stat_database
+SELECT sessions AS db_stat_sessions FROM pg_stat_database WHERE datname = (SELECT current_database()) \gset
+\c
+SELECT sessions > :db_stat_sessions FROM pg_stat_database WHERE datname = (SELECT current_database());
+ ?column?
+----------
+ t
+(1 row)
+
+-- Test pg_stat_bgwriter checkpointer-related stat
+SELECT checkpoints_req AS rqst_ckpts_before FROM pg_stat_bgwriter \gset
+-- Test pg_stat_wal
+SELECT wal_bytes AS wal_bytes_before FROM pg_stat_wal \gset
+CREATE TABLE test_stats_temp(a int);
+INSERT INTO test_stats_temp SELECT 1 FROM generate_series(1,1000)i;
+CHECKPOINT;
+SELECT checkpoints_req > :rqst_ckpts_before FROM pg_stat_bgwriter;
+ ?column?
+----------
+ t
+(1 row)
+
+SELECT wal_bytes > :wal_bytes_before FROM pg_stat_wal;
+ ?column?
+----------
+ t
+(1 row)
+
+-----
+-- Test that resetting stats works for reset timestamp
+-----
+SELECT stats_reset AS slru_commit_ts_reset_ts FROM pg_stat_slru WHERE name = 'CommitTs' \gset
+SELECT stats_reset AS slru_notify_reset_ts FROM pg_stat_slru WHERE name = 'Notify' \gset
+-- Test that reset_slru with a specified SLRU works.
+SELECT pg_stat_reset_slru('CommitTs');
+ pg_stat_reset_slru
+--------------------
+
+(1 row)
+
+SELECT pg_stat_force_next_flush();
+ pg_stat_force_next_flush
+--------------------------
+
+(1 row)
+
+SELECT stats_reset > :'slru_commit_ts_reset_ts'::timestamptz FROM pg_stat_slru WHERE name = 'CommitTs';
+ ?column?
+----------
+ t
+(1 row)
+
+SELECT stats_reset AS slru_commit_ts_reset_ts FROM pg_stat_slru WHERE name = 'CommitTs' \gset
+-- Test that multiple SLRUs are reset when no specific SLRU provided to reset function
+SELECT pg_stat_reset_slru(NULL);
+ pg_stat_reset_slru
+--------------------
+
+(1 row)
+
+SELECT pg_stat_force_next_flush();
+ pg_stat_force_next_flush
+--------------------------
+
+(1 row)
+
+SELECT stats_reset > :'slru_commit_ts_reset_ts'::timestamptz FROM pg_stat_slru WHERE name = 'CommitTs';
+ ?column?
+----------
+ t
+(1 row)
+
+SELECT stats_reset > :'slru_notify_reset_ts'::timestamptz FROM pg_stat_slru WHERE name = 'Notify';
+ ?column?
+----------
+ t
+(1 row)
+
+SELECT stats_reset AS archiver_reset_ts FROM pg_stat_archiver \gset
+-- Test that reset_shared with archiver specified as the stats type works
+SELECT pg_stat_reset_shared('archiver');
+ pg_stat_reset_shared
+----------------------
+
+(1 row)
+
+SELECT pg_stat_force_next_flush();
+ pg_stat_force_next_flush
+--------------------------
+
+(1 row)
+
+SELECT stats_reset > :'archiver_reset_ts'::timestamptz FROM pg_stat_archiver;
+ ?column?
+----------
+ t
+(1 row)
+
+SELECT stats_reset AS archiver_reset_ts FROM pg_stat_archiver \gset
+SELECT stats_reset AS bgwriter_reset_ts FROM pg_stat_bgwriter \gset
+-- Test that reset_shared with bgwriter specified as the stats type works
+SELECT pg_stat_reset_shared('bgwriter');
+ pg_stat_reset_shared
+----------------------
+
+(1 row)
+
+SELECT pg_stat_force_next_flush();
+ pg_stat_force_next_flush
+--------------------------
+
+(1 row)
+
+SELECT stats_reset > :'bgwriter_reset_ts'::timestamptz FROM pg_stat_bgwriter;
+ ?column?
+----------
+ t
+(1 row)
+
+SELECT stats_reset AS bgwriter_reset_ts FROM pg_stat_bgwriter \gset
+SELECT stats_reset AS wal_reset_ts FROM pg_stat_wal \gset
+-- Test that reset_shared with wal specified as the stats type works
+SELECT pg_stat_reset_shared('wal');
+ pg_stat_reset_shared
+----------------------
+
+(1 row)
+
+SELECT pg_stat_force_next_flush();
+ pg_stat_force_next_flush
+--------------------------
+
+(1 row)
+
+SELECT stats_reset > :'wal_reset_ts'::timestamptz FROM pg_stat_wal;
+ ?column?
+----------
+ t
+(1 row)
+
+SELECT stats_reset AS wal_reset_ts FROM pg_stat_wal \gset
+-- Test that reset_shared with no specified stats type resets all of them
+SELECT pg_stat_reset_shared(NULL);
+ pg_stat_reset_shared
+----------------------
+
+(1 row)
+
+SELECT pg_stat_force_next_flush();
+ pg_stat_force_next_flush
+--------------------------
+
+(1 row)
+
+SELECT stats_reset > :'archiver_reset_ts'::timestamptz FROM pg_stat_archiver;
+ ?column?
+----------
+ f
+(1 row)
+
+SELECT stats_reset > :'bgwriter_reset_ts'::timestamptz FROM pg_stat_bgwriter;
+ ?column?
+----------
+ f
+(1 row)
+
+SELECT stats_reset > :'wal_reset_ts'::timestamptz FROM pg_stat_wal;
+ ?column?
+----------
+ f
+(1 row)
+
+-- Test that reset works for pg_stat_database
+-- Since pg_stat_database stats_reset starts out as NULL, reset it once first so we have something to compare it to
+SELECT pg_stat_reset();
+ pg_stat_reset
+---------------
+
+(1 row)
+
+SELECT pg_stat_force_next_flush();
+ pg_stat_force_next_flush
+--------------------------
+
+(1 row)
+
+SELECT stats_reset AS db_reset_ts FROM pg_stat_database WHERE datname = (SELECT current_database()) \gset
+SELECT pg_stat_reset();
+ pg_stat_reset
+---------------
+
+(1 row)
+
+SELECT pg_stat_force_next_flush();
+ pg_stat_force_next_flush
+--------------------------
+
+(1 row)
+
+SELECT stats_reset > :'db_reset_ts'::timestamptz FROM pg_stat_database WHERE datname = (SELECT current_database());
+ ?column?
+----------
+ t
+(1 row)
+
-- End of Stats Test
diff --git a/src/test/regress/sql/stats.sql b/src/test/regress/sql/stats.sql
index 1e156ca5e4..cd3dc70edb 100644
--- a/src/test/regress/sql/stats.sql
+++ b/src/test/regress/sql/stats.sql
@@ -281,4 +281,111 @@ SELECT pg_stat_get_function_calls(:stats_test_func2_oid);
DROP TABLE trunc_stats_test, trunc_stats_test1, trunc_stats_test2, trunc_stats_test3, trunc_stats_test4;
DROP TABLE prevstats;
+-----
+-- Test that various stats views are being properly populated
+-----
+
+-- Test that sessions is incremented when a new session is started in pg_stat_database
+SELECT sessions AS db_stat_sessions FROM pg_stat_database WHERE datname = (SELECT current_database()) \gset
+\c
+SELECT sessions > :db_stat_sessions FROM pg_stat_database WHERE datname = (SELECT current_database());
+
+-- Test pg_stat_bgwriter checkpointer-related stat
+
+SELECT checkpoints_req AS rqst_ckpts_before FROM pg_stat_bgwriter \gset
+
+-- Test pg_stat_wal
+
+SELECT wal_bytes AS wal_bytes_before FROM pg_stat_wal \gset
+
+CREATE TABLE test_stats_temp(a int);
+INSERT INTO test_stats_temp SELECT 1 FROM generate_series(1,1000)i;
+
+CHECKPOINT;
+
+SELECT checkpoints_req > :rqst_ckpts_before FROM pg_stat_bgwriter;
+SELECT wal_bytes > :wal_bytes_before FROM pg_stat_wal;
+
+-----
+-- Test that resetting stats works for reset timestamp
+-----
+
+SELECT stats_reset AS slru_commit_ts_reset_ts FROM pg_stat_slru WHERE name = 'CommitTs' \gset
+
+SELECT stats_reset AS slru_notify_reset_ts FROM pg_stat_slru WHERE name = 'Notify' \gset
+
+-- Test that reset_slru with a specified SLRU works.
+SELECT pg_stat_reset_slru('CommitTs');
+
+SELECT pg_stat_force_next_flush();
+
+SELECT stats_reset > :'slru_commit_ts_reset_ts'::timestamptz FROM pg_stat_slru WHERE name = 'CommitTs';
+
+SELECT stats_reset AS slru_commit_ts_reset_ts FROM pg_stat_slru WHERE name = 'CommitTs' \gset
+
+-- Test that multiple SLRUs are reset when no specific SLRU provided to reset function
+SELECT pg_stat_reset_slru(NULL);
+
+SELECT pg_stat_force_next_flush();
+
+SELECT stats_reset > :'slru_commit_ts_reset_ts'::timestamptz FROM pg_stat_slru WHERE name = 'CommitTs';
+SELECT stats_reset > :'slru_notify_reset_ts'::timestamptz FROM pg_stat_slru WHERE name = 'Notify';
+
+SELECT stats_reset AS archiver_reset_ts FROM pg_stat_archiver \gset
+
+-- Test that reset_shared with archiver specified as the stats type works
+SELECT pg_stat_reset_shared('archiver');
+
+SELECT pg_stat_force_next_flush();
+
+SELECT stats_reset > :'archiver_reset_ts'::timestamptz FROM pg_stat_archiver;
+
+SELECT stats_reset AS archiver_reset_ts FROM pg_stat_archiver \gset
+
+SELECT stats_reset AS bgwriter_reset_ts FROM pg_stat_bgwriter \gset
+
+-- Test that reset_shared with bgwriter specified as the stats type works
+SELECT pg_stat_reset_shared('bgwriter');
+
+SELECT pg_stat_force_next_flush();
+
+SELECT stats_reset > :'bgwriter_reset_ts'::timestamptz FROM pg_stat_bgwriter;
+
+SELECT stats_reset AS bgwriter_reset_ts FROM pg_stat_bgwriter \gset
+
+SELECT stats_reset AS wal_reset_ts FROM pg_stat_wal \gset
+
+-- Test that reset_shared with wal specified as the stats type works
+SELECT pg_stat_reset_shared('wal');
+
+SELECT pg_stat_force_next_flush();
+
+SELECT stats_reset > :'wal_reset_ts'::timestamptz FROM pg_stat_wal;
+
+SELECT stats_reset AS wal_reset_ts FROM pg_stat_wal \gset
+
+-- Test that reset_shared with no specified stats type resets all of them
+SELECT pg_stat_reset_shared(NULL);
+
+SELECT pg_stat_force_next_flush();
+
+SELECT stats_reset > :'archiver_reset_ts'::timestamptz FROM pg_stat_archiver;
+SELECT stats_reset > :'bgwriter_reset_ts'::timestamptz FROM pg_stat_bgwriter;
+SELECT stats_reset > :'wal_reset_ts'::timestamptz FROM pg_stat_wal;
+
+-- Test that reset works for pg_stat_database
+
+-- Since pg_stat_database stats_reset starts out as NULL, reset it once first so we have something to compare it to
+SELECT pg_stat_reset();
+
+SELECT pg_stat_force_next_flush();
+
+SELECT stats_reset AS db_reset_ts FROM pg_stat_database WHERE datname = (SELECT current_database()) \gset
+
+SELECT pg_stat_reset();
+
+SELECT pg_stat_force_next_flush();
+
+SELECT stats_reset > :'db_reset_ts'::timestamptz FROM pg_stat_database WHERE datname = (SELECT current_database());
+
-- End of Stats Test
--
2.30.2
^ permalink raw reply [nested|flat] 21+ messages in thread
* Re: shared-memory based stats collector - v67
@ 2022-03-23 07:38 Kyotaro Horiguchi <[email protected]>
parent: Kyotaro Horiguchi <[email protected]>
0 siblings, 1 reply; 21+ messages in thread
From: Kyotaro Horiguchi @ 2022-03-23 07:38 UTC (permalink / raw)
To: [email protected]; +Cc: [email protected]; [email protected]; [email protected]; [email protected]; [email protected]
At Tue, 22 Mar 2022 11:56:40 +0900 (JST), Kyotaro Horiguchi <[email protected]> wrote in
> Docs.. Yeah I'll try it.
This is the first cut, based on the earlier patchset.
monitoring.sgml:
> When using the statistics to monitor collected data, it is important
I failed to read this clearly. I modified the part assuming that it
means "the statistics" means "the statistics views and functions".
I didn't mention pgstat_force_next_flush() since I think it is a
developer-only feature.
In the attached diff, I refrained to reindent for easy review.
regards.
--
Kyotaro Horiguchi
NTT Open Source Software Center
^ permalink raw reply [nested|flat] 21+ messages in thread
* Re: shared-memory based stats collector - v67
@ 2022-03-23 08:27 Kyotaro Horiguchi <[email protected]>
parent: Andres Freund <[email protected]>
1 sibling, 1 reply; 21+ messages in thread
From: Kyotaro Horiguchi @ 2022-03-23 08:27 UTC (permalink / raw)
To: [email protected]; +Cc: [email protected]; [email protected]; [email protected]; [email protected]; [email protected]
At Mon, 21 Mar 2022 14:30:17 -0700, Andres Freund <[email protected]> wrote in
> > Right now we reset stats for replicas, even if we start from a shutdown
> > checkpoint. That seems pretty unnecessary with this patch:
> > - 0021-pgstat-wip-only-reset-pgstat-data-after-crash-re.patch
>
> Might raise this in another thread for higher visibility.
+ /*
+ * When starting with crash recovery, reset pgstat data - it might not be
+ * valid. Otherwise restore pgstat data. It's safe to do this here,
+ * because postmaster will not yet have started any other processes
+ *
+ * TODO: With a bit of extra work we could just start with a pgstat file
+ * associated with the checkpoint redo location we're starting from.
+ */
+ if (ControlFile->state == DB_SHUTDOWNED ||
+ ControlFile->state == DB_SHUTDOWNED_IN_RECOVERY)
+ pgstat_restore_stats();
+ else
+ pgstat_discard_stats();
+
Before there, InitWalRecovery changes the state to
DB_IN_ARCHIVE_RECOVERY if it was either DB_SHUTDOWNED or
DB_IN_PRODUCTION. So the stat seems like always discarded on standby.
In the first place, I'm not sure it is valid that a standby from a
cold backup takes over the stats from the primary.
regards.
--
Kyotaro Horiguchi
NTT Open Source Software Center
^ permalink raw reply [nested|flat] 21+ messages in thread
* Re: shared-memory based stats collector - v67
@ 2022-03-23 17:42 Andres Freund <[email protected]>
parent: Kyotaro Horiguchi <[email protected]>
0 siblings, 2 replies; 21+ messages in thread
From: Andres Freund @ 2022-03-23 17:42 UTC (permalink / raw)
To: Kyotaro Horiguchi <[email protected]>; +Cc: [email protected]; [email protected]; [email protected]; [email protected]; [email protected]
Hi,
On 2022-03-23 17:27:50 +0900, Kyotaro Horiguchi wrote:
> At Mon, 21 Mar 2022 14:30:17 -0700, Andres Freund <[email protected]> wrote in
> > > Right now we reset stats for replicas, even if we start from a shutdown
> > > checkpoint. That seems pretty unnecessary with this patch:
> > > - 0021-pgstat-wip-only-reset-pgstat-data-after-crash-re.patch
> >
> > Might raise this in another thread for higher visibility.
>
> + /*
> + * When starting with crash recovery, reset pgstat data - it might not be
> + * valid. Otherwise restore pgstat data. It's safe to do this here,
> + * because postmaster will not yet have started any other processes
> + *
> + * TODO: With a bit of extra work we could just start with a pgstat file
> + * associated with the checkpoint redo location we're starting from.
> + */
> + if (ControlFile->state == DB_SHUTDOWNED ||
> + ControlFile->state == DB_SHUTDOWNED_IN_RECOVERY)
> + pgstat_restore_stats();
> + else
> + pgstat_discard_stats();
> +
>
> Before there, InitWalRecovery changes the state to
> DB_IN_ARCHIVE_RECOVERY if it was either DB_SHUTDOWNED or
> DB_IN_PRODUCTION. So the stat seems like always discarded on standby.
Hm. I though it worked at some point. I guess there's a reason this commit is
a separate commit marked WIP ;)
> In the first place, I'm not sure it is valid that a standby from a
> cold backup takes over the stats from the primary.
I don't really see a reason not to use the stats in that case - we have a
correct stats file after all. But it doesn't seem too important. What I
actually find worth addressing is the case of standbys starting in
DB_SHUTDOWNED_IN_RECOVERY. Right now we always throw stats away after a
*graceful* restart of a standby, which doesn't seem great.
Greetings,
Andres Freund
^ permalink raw reply [nested|flat] 21+ messages in thread
* Re: shared-memory based stats collector - v67
@ 2022-03-24 06:00 Kyotaro Horiguchi <[email protected]>
parent: Andres Freund <[email protected]>
1 sibling, 0 replies; 21+ messages in thread
From: Kyotaro Horiguchi @ 2022-03-24 06:00 UTC (permalink / raw)
To: [email protected]; +Cc: [email protected]; [email protected]; [email protected]; [email protected]; [email protected]
(nice work about ubsan)
At Wed, 23 Mar 2022 10:42:03 -0700, Andres Freund <[email protected]> wrote in
> Hi,
>
> On 2022-03-23 17:27:50 +0900, Kyotaro Horiguchi wrote:
> > At Mon, 21 Mar 2022 14:30:17 -0700, Andres Freund <[email protected]> wrote in
> > Before there, InitWalRecovery changes the state to
> > DB_IN_ARCHIVE_RECOVERY if it was either DB_SHUTDOWNED or
> > DB_IN_PRODUCTION. So the stat seems like always discarded on standby.
>
> Hm. I though it worked at some point. I guess there's a reason this commit is
> a separate commit marked WIP ;)
Yeah, I know:p
> > In the first place, I'm not sure it is valid that a standby from a
> > cold backup takes over the stats from the primary.
>
> I don't really see a reason not to use the stats in that case - we have a
> correct stats file after all. But it doesn't seem too important. What I
> actually find worth addressing is the case of standbys starting in
> DB_SHUTDOWNED_IN_RECOVERY. Right now we always throw stats away after a
> *graceful* restart of a standby, which doesn't seem great.
It undoubtfully an improvement if stats preserved after a graceful
restart on standbys. I just wonder if there's way to detect the
first-start of a standby from a cold backup. But even if there isn't,
it's an improvement.
regards.
--
Kyotaro Horiguchi
NTT Open Source Software Center
^ permalink raw reply [nested|flat] 21+ messages in thread
* Re: shared-memory based stats collector - v66
@ 2022-03-24 17:21 Melanie Plageman <[email protected]>
parent: Andres Freund <[email protected]>
3 siblings, 1 reply; 21+ messages in thread
From: Melanie Plageman @ 2022-03-24 17:21 UTC (permalink / raw)
To: Andres Freund <[email protected]>; +Cc: Kyotaro Horiguchi <[email protected]>; Ibrar Ahmed <[email protected]>; [email protected]; [email protected]; PostgreSQL Hackers <[email protected]>
On Thu, Mar 17, 2022 at 3:36 AM Andres Freund <[email protected]> wrote:
>
> The biggest todos are:
> - Address all the remaining AFIXMEs and XXXs
Attached is a patch that addresses three of the existing AFIXMEs.
Attachments:
[text/x-patch] address3fixmes.patch (12.2K, ../../CAAKRu_YsFzW3jsQUD7MVUY8c+87OiKW_Y93TsPAfLr3zi_jX9w@mail.gmail.com/2-address3fixmes.patch)
download | inline diff:
From 2a975cdb5d10ec365ca2ced39b9f99a9385b6268 Mon Sep 17 00:00:00 2001
From: Melanie Plageman <[email protected]>
Date: Wed, 23 Mar 2022 19:43:12 -0400
Subject: [PATCH] Address 3 AFIXMEs
- reset timestamp callback
- schedule stat internl warn and reset if exists
- pending delete callback
---
src/backend/utils/activity/pgstat.c | 117 +++++++++---------
src/backend/utils/activity/pgstat_database.c | 7 ++
src/backend/utils/activity/pgstat_relation.c | 16 +++
.../utils/activity/pgstat_subscription.c | 7 ++
src/include/utils/pgstat_internal.h | 16 +++
5 files changed, 105 insertions(+), 58 deletions(-)
diff --git a/src/backend/utils/activity/pgstat.c b/src/backend/utils/activity/pgstat.c
index 7f86bc29ee..9841447a8b 100644
--- a/src/backend/utils/activity/pgstat.c
+++ b/src/backend/utils/activity/pgstat.c
@@ -154,10 +154,8 @@ static void pgstat_shared_refs_release_all(void);
static void pgstat_perform_drop(xl_xact_stats_item *drop);
static bool pgstat_drop_stats_entry(dshash_seq_status *hstat);
-static void pgstat_shared_reset_timestamp(PgStatKind kind, PgStatShm_StatEntryHeader *header);
static void pgstat_reset_all_stats(TimestampTz ts);
-static void pgstat_pending_delete(PgStatSharedRef *shared_ref);
static bool pgstat_pending_flush_stats(bool nowait);
static PgStatKind pgstat_kind_for(char *kind_str);
@@ -281,6 +279,8 @@ static const PgStatKindInfo pgstat_kind_infos[PGSTAT_KIND_LAST + 1] = {
.pending_size = sizeof(PgStat_StatDBEntry),
.flush_pending_cb = pgstat_flush_database,
+ .pending_delete_cb = pgstat_pending_delete,
+ .reset_timestamp_cb = pgstat_database_reset_timestamp,
},
[PGSTAT_KIND_TABLE] = {
@@ -294,6 +294,8 @@ static const PgStatKindInfo pgstat_kind_infos[PGSTAT_KIND_LAST + 1] = {
.pending_size = sizeof(PgStat_TableStatus),
.flush_pending_cb = pgstat_flush_relation,
+ .pending_delete_cb = pgstat_relation_pending_delete,
+ .reset_timestamp_cb = pgstat_shared_reset_timestamp_noop,
},
[PGSTAT_KIND_FUNCTION] = {
@@ -307,6 +309,8 @@ static const PgStatKindInfo pgstat_kind_infos[PGSTAT_KIND_LAST + 1] = {
.pending_size = sizeof(PgStat_BackendFunctionEntry),
.flush_pending_cb = pgstat_flush_function,
+ .pending_delete_cb = pgstat_pending_delete,
+ .reset_timestamp_cb = pgstat_shared_reset_timestamp_noop,
},
[PGSTAT_KIND_SUBSCRIPTION] = {
@@ -322,6 +326,8 @@ static const PgStatKindInfo pgstat_kind_infos[PGSTAT_KIND_LAST + 1] = {
.pending_size = sizeof(PgStat_BackendSubEntry),
.flush_pending_cb = pgstat_flush_subscription,
+ .pending_delete_cb = pgstat_pending_delete,
+ .reset_timestamp_cb = pgstat_subscription_reset_timestamp,
},
@@ -1040,12 +1046,18 @@ pgstat_schedule_stat_internal(PgStatKind kind, Oid dboid, Oid objoid, bool is_cr
void
pgstat_schedule_stat_create(PgStatKind kind, Oid dboid, Oid objoid)
{
- pgstat_schedule_stat_internal(kind, dboid, objoid, /* create */ true);
+ if (pgstat_shared_ref_get(kind, dboid, objoid, false, NULL))
+ {
+ Oid msg_oid = (kind == PGSTAT_KIND_DB) ? dboid : objoid;
- /*
- * AFIXME: It would be a good idea to check if an object with that key
- * already exists. WARN if so, and reset the stats to 0.
- */
+ ereport(WARNING,
+ errmsg("Resetting existing stats for %s with OID %d.",
+ (pgstat_kind_info_for(kind))->name, msg_oid));
+
+ pgstat_reset_one(kind, dboid, objoid);
+ }
+
+ pgstat_schedule_stat_internal(kind, dboid, objoid, /* create */ true);
}
/*
@@ -2178,6 +2190,28 @@ pgstat_shared_refs_release_all(void)
* ------------------------------------------------------------
*/
+/*
+ * Delete pending stats for variable number stats in the general case. Some
+ * types of stats require additional steps and have dedicated callbacks.
+ */
+void
+pgstat_pending_delete(PgStatSharedRef *shared_ref)
+{
+ void *pending_data = shared_ref->pending;
+ PgStatKind kind = shared_ref->shared_entry->key.kind;
+
+ Assert(pending_data != NULL);
+ Assert(!pgstat_kind_info_for(kind)->fixed_amount);
+
+ /* PGSTAT_KIND_TABLE has its own callback */
+ Assert(kind != PGSTAT_KIND_TABLE);
+
+ pfree(pending_data);
+ shared_ref->pending = NULL;
+
+ dlist_delete(&shared_ref->pending_node);
+}
+
/*
* Returns the appropriate PgStatSharedRef, preparing it to receive pending
* stats if not already done.
@@ -2222,38 +2256,6 @@ pgstat_pending_fetch(PgStatKind kind, Oid dboid, Oid objoid)
return shared_ref;
}
-static void
-pgstat_pending_delete(PgStatSharedRef *shared_ref)
-{
- void *pending_data = shared_ref->pending;
- PgStatKind kind;
-
- Assert(pending_data != NULL);
-
- /* AFIXME: Move into a PgStatKindInfo callback */
- kind = shared_ref->shared_entry->key.kind;
- switch (kind)
- {
- case PGSTAT_KIND_TABLE:
- pgstat_relation_unlink(((PgStat_TableStatus *) pending_data)->relation);
- break;
- case PGSTAT_KIND_DB:
- case PGSTAT_KIND_FUNCTION:
- case PGSTAT_KIND_SUBSCRIPTION:
- break;
- default:
- /* !fixed_amount stats should be handled explicitly */
- Assert(pgstat_kind_info_for(kind)->fixed_amount);
- elog(ERROR, "unexpected");
- break;
- }
-
- pfree(pending_data);
- shared_ref->pending = NULL;
-
- dlist_delete(&shared_ref->pending_node);
-}
-
/*
* Flush out pending stats for database objects (databases, relations,
* functions).
@@ -2288,6 +2290,7 @@ pgstat_pending_flush_stats(bool nowait)
Assert(!kind_info->fixed_amount);
Assert(kind_info->flush_pending_cb != NULL);
+ Assert(kind_info->pending_delete_cb != NULL);
/* flush the stats, if possible */
remove = kind_info->flush_pending_cb(shared_ref, nowait);
@@ -2302,7 +2305,7 @@ pgstat_pending_flush_stats(bool nowait)
/* if successfully flushed, remove entry */
if (remove)
- pgstat_pending_delete(shared_ref);
+ kind_info->pending_delete_cb(shared_ref);
else
have_pending = true;
@@ -2438,11 +2441,16 @@ pgstat_perform_drop(xl_xact_stats_item *drop)
{
PgStatShmHashEntry *shent;
PgStatHashKey key;
+ const PgStatKindInfo *kind_info;
key.kind = drop->kind;
key.dboid = drop->dboid;
key.objoid = drop->objoid;
+ kind_info = pgstat_kind_info_for(drop->kind);
+ Assert(!kind_info->fixed_amount);
+ Assert(kind_info->pending_delete_cb != NULL);
+
if (pgStatSharedRefHash)
{
PgStatSharedRefHashEntry *lohashent;
@@ -2452,7 +2460,7 @@ pgstat_perform_drop(xl_xact_stats_item *drop)
if (lohashent)
{
if (lohashent->shared_ref && lohashent->shared_ref->pending)
- pgstat_pending_delete(lohashent->shared_ref);
+ kind_info->pending_delete_cb(lohashent->shared_ref);
pgstat_shared_ref_release(lohashent->key, lohashent->shared_ref);
}
@@ -2498,9 +2506,12 @@ pgstat_perform_drop(xl_xact_stats_item *drop)
static void
pgstat_shared_stat_reset_one(PgStatKind kind, PgStatShm_StatEntryHeader *header)
{
+ const PgStatKindInfo *kind_info = pgstat_kind_info_for(kind);
+
memset(shared_stat_entry_data(kind, header), 0,
shared_stat_entry_len(kind));
- pgstat_shared_reset_timestamp(kind, header);
+
+ kind_info->reset_timestamp_cb(header);
}
/*
@@ -2555,26 +2566,16 @@ pgstat_reset_matching(bool (*do_reset) (PgStatShmHashEntry *))
dshash_seq_term(&hstat);
}
-static void
-pgstat_shared_reset_timestamp(PgStatKind kind, PgStatShm_StatEntryHeader *header)
+/*
+ * For variable number stats with no stats reset timestamp member
+ */
+void
+pgstat_shared_reset_timestamp_noop(PgStatShm_StatEntryHeader *header)
{
- /* AFIXME: Move into a PgStatKindInfo callback */
- switch (kind)
- {
- case PGSTAT_KIND_DB:
- ((PgStatShm_StatDBEntry *) header)->stats.stat_reset_timestamp =
- GetCurrentTimestamp();
- break;
- case PGSTAT_KIND_SUBSCRIPTION:
- ((PgStatShm_StatSubEntry *) header)->stats.stat_reset_timestamp =
- GetCurrentTimestamp();
- break;
- default:
- return;
- }
}
+
/* ------------------------------------------------------------
* Fetching of stats
* ------------------------------------------------------------
diff --git a/src/backend/utils/activity/pgstat_database.c b/src/backend/utils/activity/pgstat_database.c
index 49037d345a..92ef8f1974 100644
--- a/src/backend/utils/activity/pgstat_database.c
+++ b/src/backend/utils/activity/pgstat_database.c
@@ -319,6 +319,13 @@ pgstat_should_report_connstat(void)
return MyBackendType == B_BACKEND;
}
+void
+pgstat_database_reset_timestamp(PgStatShm_StatEntryHeader *header)
+{
+ ((PgStatShm_StatDBEntry *) header)->stats.stat_reset_timestamp =
+ GetCurrentTimestamp();
+}
+
/*
* pgstat_flush_db - flush out a local database stats entry
*
diff --git a/src/backend/utils/activity/pgstat_relation.c b/src/backend/utils/activity/pgstat_relation.c
index a96760f5b1..e3eb960659 100644
--- a/src/backend/utils/activity/pgstat_relation.c
+++ b/src/backend/utils/activity/pgstat_relation.c
@@ -885,6 +885,22 @@ pgstat_flush_relation(PgStatSharedRef *shared_ref, bool nowait)
return true;
}
+void
+pgstat_relation_pending_delete(PgStatSharedRef *shared_ref)
+{
+ void *pending_data = shared_ref->pending;
+
+ Assert(pending_data != NULL);
+
+ pgstat_relation_unlink(((PgStat_TableStatus *) pending_data)->relation);
+
+ pfree(pending_data);
+ shared_ref->pending = NULL;
+
+ dlist_delete(&shared_ref->pending_node);
+}
+
+
/*
* Find or create a PgStat_TableStatus entry for rel. New entry is created and
* initialized if not exists.
diff --git a/src/backend/utils/activity/pgstat_subscription.c b/src/backend/utils/activity/pgstat_subscription.c
index f7f5698c5d..5882016177 100644
--- a/src/backend/utils/activity/pgstat_subscription.c
+++ b/src/backend/utils/activity/pgstat_subscription.c
@@ -43,6 +43,13 @@ pgstat_reset_subscription_counter(Oid subid)
pgstat_reset_one(PGSTAT_KIND_SUBSCRIPTION, InvalidOid, subid);
}
+void
+pgstat_subscription_reset_timestamp(PgStatShm_StatEntryHeader *header)
+{
+ ((PgStatShm_StatSubEntry *) header)->stats.stat_reset_timestamp =
+ GetCurrentTimestamp();
+}
+
/* ----------
* pgstat_report_subscription_error() -
*
diff --git a/src/include/utils/pgstat_internal.h b/src/include/utils/pgstat_internal.h
index b312993d1f..718e848866 100644
--- a/src/include/utils/pgstat_internal.h
+++ b/src/include/utils/pgstat_internal.h
@@ -193,6 +193,16 @@ typedef struct PgStatKindInfo
*/
bool (*flush_pending_cb) (PgStatSharedRef *sr, bool nowait);
+ /*
+ * For variable number stats: delete pending stats.
+ */
+ void (*pending_delete_cb) (PgStatSharedRef *sr);
+
+ /*
+ * For variable number stats: reset the reset timestamp.
+ */
+ void (*reset_timestamp_cb) (PgStatShm_StatEntryHeader *header);
+
/*
* For global statistics: Reset All.
*/
@@ -390,7 +400,9 @@ extern PgStatSharedRef *pgstat_shared_stat_locked(PgStatKind kind,
bool nowait);
extern void pgstat_reset_one(PgStatKind kind, Oid dboid, Oid objoid);
extern void pgstat_reset_matching(bool (*do_reset) (PgStatShmHashEntry *));
+extern void pgstat_shared_reset_timestamp_noop(PgStatShm_StatEntryHeader *header);
+extern void pgstat_pending_delete(PgStatSharedRef *shared_ref);
extern PgStatSharedRef *pgstat_pending_prepare(PgStatKind kind, Oid dboid, Oid objoid, bool *created_shared);
extern PgStatSharedRef *pgstat_pending_fetch(PgStatKind kind, Oid dboid, Oid objoid);
@@ -441,6 +453,7 @@ extern void pgstat_checkpointer_snapshot_cb(void);
extern void pgstat_report_disconnect(Oid dboid);
extern void pgstat_update_dbstats(TimestampTz now);
extern void AtEOXact_PgStat_Database(bool isCommit, bool parallel);
+extern void pgstat_database_reset_timestamp(PgStatShm_StatEntryHeader *header);
extern bool pgstat_flush_database(PgStatSharedRef *shared_ref, bool nowait);
extern PgStat_StatDBEntry *pgstat_pending_db_prepare(Oid dboid);
@@ -460,7 +473,9 @@ extern void AtEOXact_PgStat_Relations(PgStat_SubXactStatus *xact_state, bool isC
extern void AtEOSubXact_PgStat_Relations(PgStat_SubXactStatus *xact_state, bool isCommit, int nestDepth);
extern void AtPrepare_PgStat_Relations(PgStat_SubXactStatus *xact_state);
extern void PostPrepare_PgStat_Relations(PgStat_SubXactStatus *xact_state);
+
extern bool pgstat_flush_relation(PgStatSharedRef *shared_ref, bool nowait);
+extern void pgstat_relation_pending_delete(PgStatSharedRef *shared_ref);
/*
@@ -497,6 +512,7 @@ extern void pgstat_wal_snapshot_cb(void);
*/
extern bool pgstat_flush_subscription(PgStatSharedRef *shared_ref, bool nowait);
+extern void pgstat_subscription_reset_timestamp(PgStatShm_StatEntryHeader *header);
/*
--
2.30.2
^ permalink raw reply [nested|flat] 21+ messages in thread
* Re: shared-memory based stats collector - v66
@ 2022-03-25 05:22 Kyotaro Horiguchi <[email protected]>
parent: Melanie Plageman <[email protected]>
0 siblings, 1 reply; 21+ messages in thread
From: Kyotaro Horiguchi @ 2022-03-25 05:22 UTC (permalink / raw)
To: [email protected]; +Cc: [email protected]; [email protected]; [email protected]; [email protected]; [email protected]
At Thu, 24 Mar 2022 13:21:33 -0400, Melanie Plageman <[email protected]> wrote in
> On Thu, Mar 17, 2022 at 3:36 AM Andres Freund <[email protected]> wrote:
> >
> > The biggest todos are:
> > - Address all the remaining AFIXMEs and XXXs
>
> Attached is a patch that addresses three of the existing AFIXMEs.
Thanks!
+ .reset_timestamp_cb = pgstat_shared_reset_timestamp_noop,
(I once misunderstood that the "shared" means shared memory area..)
The reset function is type-specific and it must be set. So don't we
provide all to-be-required reset functions?
+ if (pgstat_shared_ref_get(kind, dboid, objoid, false, NULL))
+ {
+ Oid msg_oid = (kind == PGSTAT_KIND_DB) ? dboid : objoid;
Explicitly using PGSTAT_KIND_DB here is a kind of annoyance. Since we
always give InvalidOid correctly as the parameters, and objoid alone
is not specific enough, do we warn using both dboid and objoid without
a special treat?
Concretely, I propose to do the following instead.
+ if (pgstat_shared_ref_get(kind, dboid, objoid, false, NULL))
+ {
+ ereport(WARNING,
+ errmsg("resetting existing stats for type %s, db=%d, oid=%d",
+ pgstat_kind_info_for(kind)->name, dboid, objoid);
+pgstat_pending_delete(PgStatSharedRef *shared_ref)
+{
+ void *pending_data = shared_ref->pending;
+ PgStatKind kind = shared_ref->shared_entry->key.kind;
+
+ Assert(pending_data != NULL);
+ Assert(!pgstat_kind_info_for(kind)->fixed_amount);
+
+ /* PGSTAT_KIND_TABLE has its own callback */
+ Assert(kind != PGSTAT_KIND_TABLE);
+
"kind" is used only in assertion, which requires PG_USED_FOR_ASSERTS_ONLY.
regards.
--
Kyotaro Horiguchi
NTT Open Source Software Center
^ permalink raw reply [nested|flat] 21+ messages in thread
* Re: shared-memory based stats collector - v66
@ 2022-03-25 08:24 Kyotaro Horiguchi <[email protected]>
parent: Kyotaro Horiguchi <[email protected]>
0 siblings, 2 replies; 21+ messages in thread
From: Kyotaro Horiguchi @ 2022-03-25 08:24 UTC (permalink / raw)
To: [email protected]; +Cc: [email protected]; [email protected]; [email protected]; [email protected]; [email protected]
At Fri, 25 Mar 2022 14:22:56 +0900 (JST), Kyotaro Horiguchi <[email protected]> wrote in
> At Thu, 24 Mar 2022 13:21:33 -0400, Melanie Plageman <[email protected]> wrote in
> > On Thu, Mar 17, 2022 at 3:36 AM Andres Freund <[email protected]> wrote:
> > >
> > > The biggest todos are:
> > > - Address all the remaining AFIXMEs and XXXs
> >
> > Attached is a patch that addresses three of the existing AFIXMEs.
I'd like to dump out my humble thoughts about other AFIXMEs..
> AFIXME: Isn't PGSTAT_MIN_INTERVAL way too long? What is the justification
> for increasing it?
It is 1000ms in the comment just above but actually 10000ms. The
number came from a discussion that if we have 1000 clients and each
backend writes stats once per 0.5 seconds, totally we flush pending
data to shared area at 2000 times per second which is too frequent. I
raised it to 5000ms, then 10000ms. So the expected maximum flush
frequency is reduces to 100 times per second. Of course it is
assuming the worst case and the 10000ms is apparently too long for the
average cases.
The current implement of pgstat postpones flushing if lock collision
happens then postpone by at most 60s. This is a kind of
auto-averaging mechanishm. It might be enough and we can reduce the
PGSTAT_MIN_INTERVAL to 500ms or so.
> AFIXME: architecture explanation.
Mmm. next, please:p
( [PGSTAT_KIND_REPLSLOT] = {)
> * AFIXME: With a bit of extra work this could now be a !fixed_amount
> * stats kind.
Yeah. The most bothersome point is the slot index is not persistent
at all and the relationship between the index and name (or identity)
is not stable even within a process life. It can be resolved by
allocating an object id to every replication slot. I faintly remember
of a discussion like that but I don't have a clear memory of the
discussion.
> static Size
> pgstat_dsa_init_size(void)
> {
> /*
> * AFIXME: What should we choose as an initial size? Should we make this
> * configurable? Maybe tune based on NBuffers?
> StatsShmemInit(void)
> * AFIXME: we need to guarantee this can be allocated in plain shared
> * memory, rather than allocating dsm segments.
I'm not sure that NBuffers is the ideal base for deciding the required
size since it doesn't seem to be generally in proportion with the
number of database objects. If we made it manually-tunable, we will
be able to emit a log when DSM segment allocation happens for this use
as as the tuning aid..
WARNING: dsa allocation happened for activity statistics
HINT: You might want to increase stat_dsa_initial_size if you see slow
down blah..
> * AFIXME: Should all the stats drop code be moved into pgstat_drop.c?
Or pgstat_xact.c?
> * AFIXME: comment
> * AFIXME: see notes about race conditions for functions in
> * pgstat_drop_function().
> */
> void
> pgstat_schedule_stat_drop(PgStatKind kind, Oid dboid, Oid objoid)
pgstat_drop_function() doesn't seem to have such a note.
I suppose the "race condition" means the case a stats entry for an
object is created just after the same object is dropped on another
backend. It seems to me such a race condition is eliminated by the
transactional drop mechanism. Are you intending to write an
explanation of that?
> /*
> * pgStatSharedRefAge increments quite slowly than the time the following
> * loop takes so this is expected to iterate no more than twice.
> *
> * AFIXME: Why is this a good place to do this?
> */
> while (pgstat_shared_refs_need_gc())
> pgstat_shared_refs_gc();
Is the reason for the AFIXME is you think that GC-check happens too
frequently?
> pgstat_shared_ref_release(PgStatHashKey key, PgStatSharedRef *shared_ref)
> {
...
> * AFIXME: this probably is racy. Another backend could look up the
> * stat, bump the refcount, as we free it.
> if (pg_atomic_fetch_sub_u32(&shared_ref->shared_entry->refcount, 1) == 1)
> {
...
> /* only dropped entries can reach a 0 refcount */
> Assert(shared_ref->shared_entry->dropped);
I didn't deeply examined, but is that race condition avoidable by
prevent pgstat_shared_ref_get from incrementing the refcount of
dropped entries?
> * AFIXME: This needs to be deduplicated with pgstat_shared_ref_release(). But
> * it's not entirely trivial, because we can't use plain dshash_delete_entry()
> * (but have to use dshash_delete_current()).
> */
> static bool
> pgstat_drop_stats_entry(dshash_seq_status *hstat)
...
> * AFIXME: don't do this while holding the dshash lock.
Is the AFIXMEs mean that we should move the call to
pgstat_shared_ref_release() out of the dshash-loop (in
pgstat_drop_database_and_contents) that calls this function? Is it
sensible if we store the (key, ref) pairs for to-be released
shared_refs then clean up them after exiting the loop?
> * Database stats contain other stats. Drop those as well when
> * dropping the database. AFIXME: Perhaps this should be done in a
> * slightly more principled way?
> */
> if (key.kind == PGSTAT_KIND_DB)
> pgstat_drop_database_and_contents(key.dboid);
I tend to agree to that and it is possible that we have
PgStatKindInfo.drop_cascade_cb(PgStatShm_StatEntryHeader *header). But
it is really needed only by PGSTAT_KIND_DB..
> * AFIXME: consistent naming
> * AFIXME: deduplicate some of this code with pgstat_fetch_snapshot_build().
> *
> * AFIXME: it'd be nicer if we passed .snapshot_cb() the target memory
> * location, instead of putting PgStatSnapshot into pgstat_internal.h
> */
> void
> pgstat_snapshot_global(PgStatKind kind)
Does having PGSTAT_KIND_NONE in PgStatKind or InvalidPgStatKind work
for deduplication? But I'm afraid that harms in some way.
For the memory location, it seems like a matter of taste, but if we
don't need a multiple copies of a global snapshot, I think
.snapshot_cb() doesn't need to take the target memory location.
regards.
--
Kyotaro Horiguchi
NTT Open Source Software Center
^ permalink raw reply [nested|flat] 21+ messages in thread
* Re: shared-memory based stats collector - v66
@ 2022-03-29 07:24 Andres Freund <[email protected]>
parent: Kyotaro Horiguchi <[email protected]>
1 sibling, 0 replies; 21+ messages in thread
From: Andres Freund @ 2022-03-29 07:24 UTC (permalink / raw)
To: Kyotaro Horiguchi <[email protected]>; +Cc: [email protected]; [email protected]; [email protected]; [email protected]; [email protected]
Hi,
On 2022-03-25 17:24:18 +0900, Kyotaro Horiguchi wrote:
> I'd like to dump out my humble thoughts about other AFIXMEs..
Thanks! Please have another look at the code in
https://github.com/anarazel/postgres/tree/shmstat I just pushed a revised
version with a lot of [a]fixmes removed.
Most importantly I did move replication slot stats into the hash table, and
just generally revised the replication slot stats code substantially. I
think it does look better now.
But also there's a new commit allowing dsm use in single user mode. To be able
to rely on stats drops we need to perform them even in single user mode. The
only reason this didn't previously fail was that we allocated enough "static"
shared memory for single user mode to never need DSMs.
Thanks to Melanie's tests, and a few more additions by myself, the code is now
reasonably well covered. The big exception to that is recovery conflict stats,
and as Melanie noticed, that was broken (somehow pgstat_database_flush_cb()
didn't sum them up)). I think she has some WIP tests...
Re the added tests: I did fix a few timing issues there. There's probably a
few more hiding somewhere.
I also found that unfortunately dshash_seq_next() as is isn't correct. I
included a workaround commit, but it's not correct. What we need to do is to
just always lock partition 0 in the initialization branch. Before we call
ensure_valid_bucket_pointers() status->hash_table->size_log2 isn't valid. And
ensure_valid_bucket_pointers can only be called with a lock...
Horiguchi-san, if you have time to look at the "XXX: The following could now be
generalized" in pgstat_read_statsfile(), pgstat_write_statsfile()... I think
that'd be nice to clean up.
> > AFIXME: Isn't PGSTAT_MIN_INTERVAL way too long? What is the justification
> > for increasing it?
>
> It is 1000ms in the comment just above but actually 10000ms. The
> number came from a discussion that if we have 1000 clients and each
> backend writes stats once per 0.5 seconds, totally we flush pending
> data to shared area at 2000 times per second which is too frequent.
Have you measured this (recently)? I tried to cause contention with a workload
targeted towards that, but couldn't see a problem with 1000ms. Of course
there's a problem with 1ms...
I think it's confusing to not report stats for 10s without a need.
> The current implement of pgstat postpones flushing if lock collision
> happens then postpone by at most 60s. This is a kind of
> auto-averaging mechanishm. It might be enough and we can reduce the
> PGSTAT_MIN_INTERVAL to 500ms or so.
Yea, I think the 60s part under contention is fine. I'd expect that to be
rarely reached.
>
> > AFIXME: architecture explanation.
>
> Mmm. next, please:p
Working on it. There's one more AFIXME that I want to resolve before, so I
don't end up with old type names strewn around (the one in pgstat_internal.h).
>
> ( [PGSTAT_KIND_REPLSLOT] = {)
> > * AFIXME: With a bit of extra work this could now be a !fixed_amount
> > * stats kind.
>
> Yeah. The most bothersome point is the slot index is not persistent
> at all and the relationship between the index and name (or identity)
> is not stable even within a process life. It can be resolved by
> allocating an object id to every replication slot. I faintly remember
> of a discussion like that but I don't have a clear memory of the
> discussion.
I think it's resolved now. pgstat_report_replslot* all get the ReplicationSlot
as a parameter. They use the new ReplicationSlotIndex() to get an index from
that. pgstat_report_replslot_(create|acquire) ensure that the relevant index
doesn't somehow contain old stats.
To deal with indexes changing / slots getting removed during restart, there's
now a new callback made during pgstat_read_statsfile() to build the key from
the serialized NameStr. That can return false if a slot of that name is not
know, or use ReplicationSlotIndex() to get the index to store in-memory stats.
> > static Size
> > pgstat_dsa_init_size(void)
> > {
> > /*
> > * AFIXME: What should we choose as an initial size? Should we make this
> > * configurable? Maybe tune based on NBuffers?
>
> > StatsShmemInit(void)
> > * AFIXME: we need to guarantee this can be allocated in plain shared
> > * memory, rather than allocating dsm segments.
>
> I'm not sure that NBuffers is the ideal base for deciding the required
> size since it doesn't seem to be generally in proportion with the
> number of database objects. If we made it manually-tunable, we will
> be able to emit a log when DSM segment allocation happens for this use
> as as the tuning aid..
>
> WARNING: dsa allocation happened for activity statistics
> HINT: You might want to increase stat_dsa_initial_size if you see slow
> down blah..
FWIW, I couldn't find any performance impact from using DSM. Because of the
"PgStatSharedRef" layer, there's not actually that much interaction with the
dsm code...
I reduced the initial allocation to 256kB. Unfortunately that's currently the
minimum that allows dshash_create() to succeed (due to dsa.c pre-allocating 16
of each allocation). I was a bit worried about that for a while, but memory
usage is still lower with the patch than before in the scenarios I tested. We
can probably improve upon that fairly easily in the future (move
dshash_table_control into static shared memory, call dsa_trim() when resizing
dshash table).
> > * AFIXME: Should all the stats drop code be moved into pgstat_drop.c?
>
> Or pgstat_xact.c?
Maybe. Somehow it doesn't seem *great* either.
> > * AFIXME: comment
> > * AFIXME: see notes about race conditions for functions in
> > * pgstat_drop_function().
> > */
> > void
> > pgstat_schedule_stat_drop(PgStatKind kind, Oid dboid, Oid objoid)
>
>
> pgstat_drop_function() doesn't seem to have such a note.
Yea, I fixed it in pgstat_init_function_usage(), forgetting about the node in
pgstat_schedule_stat_drop(). There's a decently long comment in
pgstat_init_function_usage() explaining the problem.
> I suppose the "race condition" means the case a stats entry for an
> object is created just after the same object is dropped on another
> backend. It seems to me such a race condition is eliminated by the
> transactional drop mechanism. Are you intending to write an
> explanation of that?
Yes, I definitely plan to write a bit more about that.
>
> > /*
> > * pgStatSharedRefAge increments quite slowly than the time the following
> > * loop takes so this is expected to iterate no more than twice.
> > *
> > * AFIXME: Why is this a good place to do this?
> > */
> > while (pgstat_shared_refs_need_gc())
> > pgstat_shared_refs_gc();
>
> Is the reason for the AFIXME is you think that GC-check happens too
> frequently?
Well, the while () loop makes me "suspicious" when looking at the code. I've
now made it an if (), I can't see a reason why we'd need a while()?
I just moved a bunch of that code around, there's probably a bit more polish
needed.
> > pgstat_shared_ref_release(PgStatHashKey key, PgStatSharedRef *shared_ref)
> > {
> ...
> > * AFIXME: this probably is racy. Another backend could look up the
> > * stat, bump the refcount, as we free it.
> > if (pg_atomic_fetch_sub_u32(&shared_ref->shared_entry->refcount, 1) == 1)
> > {
> ...
> > /* only dropped entries can reach a 0 refcount */
> > Assert(shared_ref->shared_entry->dropped);
>
> I didn't deeply examined, but is that race condition avoidable by
> prevent pgstat_shared_ref_get from incrementing the refcount of
> dropped entries?
I don't think the race exists anymore. I've now revised the relevant code.
> > * AFIXME: This needs to be deduplicated with pgstat_shared_ref_release(). But
> > * it's not entirely trivial, because we can't use plain dshash_delete_entry()
> > * (but have to use dshash_delete_current()).
> > */
> > static bool
> > pgstat_drop_stats_entry(dshash_seq_status *hstat)
> ...
> > * AFIXME: don't do this while holding the dshash lock.
>
> Is the AFIXMEs mean that we should move the call to
> pgstat_shared_ref_release() out of the dshash-loop (in
> pgstat_drop_database_and_contents) that calls this function? Is it
> sensible if we store the (key, ref) pairs for to-be released
> shared_refs then clean up them after exiting the loop?
I think this is now resolved. The release now happens separately, without
nested locks. See pgstat_shared_refs_release_db() call in
pgstat_drop_database_and_contents().
>
> > * Database stats contain other stats. Drop those as well when
> > * dropping the database. AFIXME: Perhaps this should be done in a
> > * slightly more principled way?
> > */
> > if (key.kind == PGSTAT_KIND_DB)
> > pgstat_drop_database_and_contents(key.dboid);
>
> I tend to agree to that and it is possible that we have
> PgStatKindInfo.drop_cascade_cb(PgStatShm_StatEntryHeader *header). But
> it is really needed only by PGSTAT_KIND_DB..
Yea, I came to the same conclusion, namely that we don't need something better
for now.
> > * AFIXME: consistent naming
> > * AFIXME: deduplicate some of this code with pgstat_fetch_snapshot_build().
> > *
> > * AFIXME: it'd be nicer if we passed .snapshot_cb() the target memory
> > * location, instead of putting PgStatSnapshot into pgstat_internal.h
> > */
> > void
> > pgstat_snapshot_global(PgStatKind kind)
>
>
> Does having PGSTAT_KIND_NONE in PgStatKind or InvalidPgStatKind work
> for deduplication? But I'm afraid that harms in some way.
I think I made it a bit nicer now, without needing either of those. I'd like
to remove "global" from those functions, it's not actually that obvious what
it means.
> For the memory location, it seems like a matter of taste, but if we
> don't need a multiple copies of a global snapshot, I think
> .snapshot_cb() doesn't need to take the target memory location.
I think it's ok for now. It'd be a bit nicer if we didn't need PgStatSnapshot
/ stats_snapshot in pgstat_internal.h, but it's ok that way I think.
Greetings,
Andres Freund
^ permalink raw reply [nested|flat] 21+ messages in thread
* Re: shared-memory based stats collector - v67
@ 2022-03-29 07:26 Andres Freund <[email protected]>
parent: Kyotaro Horiguchi <[email protected]>
0 siblings, 0 replies; 21+ messages in thread
From: Andres Freund @ 2022-03-29 07:26 UTC (permalink / raw)
To: Kyotaro Horiguchi <[email protected]>; +Cc: [email protected]; [email protected]; [email protected]; [email protected]; [email protected]
Hi,
On 2022-03-23 16:38:33 +0900, Kyotaro Horiguchi wrote:
> At Tue, 22 Mar 2022 11:56:40 +0900 (JST), Kyotaro Horiguchi <[email protected]> wrote in
> > Docs.. Yeah I'll try it.
>
> This is the first cut, based on the earlier patchset.
Thanks!
> I didn't mention pgstat_force_next_flush() since I think it is a
> developer-only feature.
Yes, that makes sense.
Sorry for not yet getting back to looking at this.
One thing we definitely need to add documentation for is the
stats_fetch_consistency GUC. I think we should change its default to 'cache',
because that still gives the ability to "self-join", without the cost of the
current method.
Greetings,
Andres Freund
^ permalink raw reply [nested|flat] 21+ messages in thread
* Re: shared-memory based stats collector - v66
@ 2022-04-02 08:16 Andres Freund <[email protected]>
parent: Kyotaro Horiguchi <[email protected]>
1 sibling, 1 reply; 21+ messages in thread
From: Andres Freund @ 2022-04-02 08:16 UTC (permalink / raw)
To: Kyotaro Horiguchi <[email protected]>; +Cc: [email protected]; [email protected]; [email protected]; [email protected]; [email protected]
Hi,
On 2022-03-25 17:24:18 +0900, Kyotaro Horiguchi wrote:
> > AFIXME: Isn't PGSTAT_MIN_INTERVAL way too long? What is the justification
> > for increasing it?
>
> It is 1000ms in the comment just above but actually 10000ms. The
> number came from a discussion that if we have 1000 clients and each
> backend writes stats once per 0.5 seconds, totally we flush pending
> data to shared area at 2000 times per second which is too frequent. I
> raised it to 5000ms, then 10000ms. So the expected maximum flush
> frequency is reduces to 100 times per second. Of course it is
> assuming the worst case and the 10000ms is apparently too long for the
> average cases.
>
> The current implement of pgstat postpones flushing if lock collision
> happens then postpone by at most 60s. This is a kind of
> auto-averaging mechanishm. It might be enough and we can reduce the
> PGSTAT_MIN_INTERVAL to 500ms or so.
I just noticed that the code doesn't appear to actually work like that right
now. Whenever the timeout is reached, pgstat_report_stat() is called with
force = true.
And even if the backend is busy running queries, once there's contention, the
next invocation of pgstat_report_stat() will return the timeout relative to
pendin_since, which then will trigger a force report via a very short timeout
soon.
It might actually make sense to only ever return PGSTAT_RETRY_MIN_INTERVAL
(with a slightly different name) from pgstat_report_stat() when blocked
(limiting the max reporting delay for an idle connection) and to continue
calling pgstat_report_stat(force = true). But to only trigger force
"internally" in pgstat_report_stat() when PGSTAT_MAX_INTERVAL is reached.
I think that'd mean we'd report after max PGSTAT_RETRY_MIN_INTERVAL in an idle
connection, and try reporting every PGSTAT_RETRY_MIN_INTERVAL (increasing up
to PGSTAT_MAX_INTERVAL when blocked) on busy connections.
Makes sense?
I think we need to do something with the pgstat_report_stat() calls outside of
postgres.c. Otherwise there's nothing limiting their reporting delay, because
they don't have the timeout logic postgres.c has. None of them is ever hot
enough to be problematic, so I think we should just make them pass force=true?
Greetings,
Andres Freund
^ permalink raw reply [nested|flat] 21+ messages in thread
* Re: shared-memory based stats collector - v67
@ 2022-04-04 23:31 Andres Freund <[email protected]>
parent: Andres Freund <[email protected]>
1 sibling, 0 replies; 21+ messages in thread
From: Andres Freund @ 2022-04-04 23:31 UTC (permalink / raw)
To: Kyotaro Horiguchi <[email protected]>; +Cc: [email protected]; [email protected]; [email protected]; [email protected]; [email protected]
Hi,
On 2022-03-23 10:42:03 -0700, Andres Freund wrote:
> On 2022-03-23 17:27:50 +0900, Kyotaro Horiguchi wrote:
> > + /*
> > + * When starting with crash recovery, reset pgstat data - it might not be
> > + * valid. Otherwise restore pgstat data. It's safe to do this here,
> > + * because postmaster will not yet have started any other processes
> > + *
> > + * TODO: With a bit of extra work we could just start with a pgstat file
> > + * associated with the checkpoint redo location we're starting from.
> > + */
> > + if (ControlFile->state == DB_SHUTDOWNED ||
> > + ControlFile->state == DB_SHUTDOWNED_IN_RECOVERY)
> > + pgstat_restore_stats();
> > + else
> > + pgstat_discard_stats();
> > +
> >
> > Before there, InitWalRecovery changes the state to
> > DB_IN_ARCHIVE_RECOVERY if it was either DB_SHUTDOWNED or
> > DB_IN_PRODUCTION. So the stat seems like always discarded on standby.
>
> Hm. I though it worked at some point. I guess there's a reason this commit is
> a separate commit marked WIP ;)
FWIW, it had gotten broken by
commit be1c00ab13a7c2c9299d60cb5a9d285c40e2506c
Author: Heikki Linnakangas <[email protected]>
Date: 2022-02-16 09:22:44 +0200
Move code around in StartupXLOG().
because that moved the spot where
ControlFile->state = DB_IN_CRASH_RECOVERY
is set to an earlier location.
Greetings,
Andres Freund
^ permalink raw reply [nested|flat] 21+ messages in thread
* Re: shared-memory based stats collector - v66
@ 2022-04-05 20:40 Andres Freund <[email protected]>
parent: Andres Freund <[email protected]>
0 siblings, 0 replies; 21+ messages in thread
From: Andres Freund @ 2022-04-05 20:40 UTC (permalink / raw)
To: Kyotaro Horiguchi <[email protected]>; +Cc: [email protected]; [email protected]; [email protected]; [email protected]; [email protected]
Hi,
On 2022-04-02 01:16:48 -0700, Andres Freund wrote:
> I just noticed that the code doesn't appear to actually work like that right
> now. Whenever the timeout is reached, pgstat_report_stat() is called with
> force = true.
>
> And even if the backend is busy running queries, once there's contention, the
> next invocation of pgstat_report_stat() will return the timeout relative to
> pendin_since, which then will trigger a force report via a very short timeout
> soon.
>
> It might actually make sense to only ever return PGSTAT_RETRY_MIN_INTERVAL
> (with a slightly different name) from pgstat_report_stat() when blocked
> (limiting the max reporting delay for an idle connection) and to continue
> calling pgstat_report_stat(force = true). But to only trigger force
> "internally" in pgstat_report_stat() when PGSTAT_MAX_INTERVAL is reached.
>
> I think that'd mean we'd report after max PGSTAT_RETRY_MIN_INTERVAL in an idle
> connection, and try reporting every PGSTAT_RETRY_MIN_INTERVAL (increasing up
> to PGSTAT_MAX_INTERVAL when blocked) on busy connections.
>
> Makes sense?
I tried to come up with a workload producing a *lot* of stats (multiple
function calls within a transaction, multiple transactions pipelined) and ran
it with 1000 clients (on a machine with 2 x (10 cores / 20 threads)). To
reduce overhead I set
default_transaction_isolation=repeatable read
track_activities=false
MVCC Snapshot acquisition is the clear bottleneck otherwise, followed by
pgstat_report_activity() (which, as confusing as it may sound, is independent
of this patch).
I do see a *small* amount of contention if I lower PGSTAT_MIN_INTERVAL to
1ms. Too small to ever be captured in pg_stat_activity.wait_event, but just
about visible in a profiler.
Which leads me to conclude we can simplify the logic significantly. Here's my
current comment explaining the logic:
* Unless called with 'force', pending stats updates are flushed happen once
* per PGSTAT_MIN_INTERVAL (1000ms). When not forced, stats flushes do not
* block on lock acquisition, except if stats updates have been pending for
* longer than PGSTAT_MAX_INTERVAL (60000ms).
*
* Whenever pending stats updates remain at the end of pgstat_report_stat() a
* suggested idle timeout is returned. Currently this is always
* PGSTAT_IDLE_INTERVAL (10000ms). Callers can use the returned time to set up
* a timeout after which to call pgstat_report_stat(true), but are not
* required to to do so.
Comments?
Greetings,
Andres Freund
^ permalink raw reply [nested|flat] 21+ messages in thread
end of thread, other threads:[~2022-04-05 20:40 UTC | newest]
Thread overview: 21+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2020-12-30 15:39 [PATCH 07/18] function comment: get_am_name Justin Pryzby <[email protected]>
2022-03-17 07:36 Re: shared-memory based stats collector - v66 Andres Freund <[email protected]>
2022-03-20 16:32 ` Re: shared-memory based stats collector - v66 Melanie Plageman <[email protected]>
2022-03-20 16:58 ` Re: shared-memory based stats collector - v66 Andres Freund <[email protected]>
2022-03-20 20:56 ` Re: shared-memory based stats collector - v66 Melanie Plageman <[email protected]>
2022-03-21 21:41 ` Re: shared-memory based stats collector - v66 Melanie Plageman <[email protected]>
2022-03-21 21:30 ` Re: shared-memory based stats collector - v67 Andres Freund <[email protected]>
2022-03-22 02:56 ` Re: shared-memory based stats collector - v67 Kyotaro Horiguchi <[email protected]>
2022-03-23 07:38 ` Re: shared-memory based stats collector - v67 Kyotaro Horiguchi <[email protected]>
2022-03-29 07:26 ` Re: shared-memory based stats collector - v67 Andres Freund <[email protected]>
2022-03-23 08:27 ` Re: shared-memory based stats collector - v67 Kyotaro Horiguchi <[email protected]>
2022-03-23 17:42 ` Re: shared-memory based stats collector - v67 Andres Freund <[email protected]>
2022-03-24 06:00 ` Re: shared-memory based stats collector - v67 Kyotaro Horiguchi <[email protected]>
2022-04-04 23:31 ` Re: shared-memory based stats collector - v67 Andres Freund <[email protected]>
2022-03-23 00:43 ` Re: shared-memory based stats collector - v66 Melanie Plageman <[email protected]>
2022-03-24 17:21 ` Re: shared-memory based stats collector - v66 Melanie Plageman <[email protected]>
2022-03-25 05:22 ` Re: shared-memory based stats collector - v66 Kyotaro Horiguchi <[email protected]>
2022-03-25 08:24 ` Re: shared-memory based stats collector - v66 Kyotaro Horiguchi <[email protected]>
2022-03-29 07:24 ` Re: shared-memory based stats collector - v66 Andres Freund <[email protected]>
2022-04-02 08:16 ` Re: shared-memory based stats collector - v66 Andres Freund <[email protected]>
2022-04-05 20:40 ` Re: shared-memory based stats collector - v66 Andres Freund <[email protected]>
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox