On Thu, Mar 17, 2022 at 3:36 AM Andres Freund <and...@anarazel.de> 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
From 2cb108fadf656de9cc998c0b2123a184881ef4e0 Mon Sep 17 00:00:00 2001
From: Melanie Plageman <melanieplage...@gmail.com>
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
