On Sun, Mar 20, 2022 at 12:58 PM Andres Freund <and...@anarazel.de> 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
From c521ba1dcb13ba236181adce06893c42ec439877 Mon Sep 17 00:00:00 2001 From: Melanie Plageman <melanieplage...@gmail.com> 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
From 5761e7436ea0df8ef6f56011a20085fb52c832b1 Mon Sep 17 00:00:00 2001 From: Melanie Plageman <melanieplage...@gmail.com> 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