pg_stat_have_stats() returns true for dropped indexes (or for index creation transaction rolled back)

Started by Drouvot, Bertrandover 3 years ago12 messages

bdrouvot@amazon.com

over 3 years ago

1 attachment(s)

Hi hackers,

While working on the relation stats split into table and index stats
[1]: /messages/by-id/5bfcf1a5-4224-9324-594b-725e704c95b1@amazon.com
dropped indexes (or for index creation transaction rolled back).

Example:

postgres=# create table bdt as select a from generate_series(1,1000) a;
SELECT 1000
postgres=# create index bdtidx on bdt(a);
CREATE INDEX
postgres=# select * from bdt where a = 30;
a
----
30
(1 row)

postgres=# SELECT 'bdtidx'::regclass::oid;
oid
-------
16395
(1 row)

postgres=# select pg_stat_have_stats('relation', 5, 16395);
pg_stat_have_stats
--------------------
t
(1 row)

postgres=# drop index bdtidx;
DROP INDEX
postgres=# select pg_stat_have_stats('relation', 5, 16395);
pg_stat_have_stats
--------------------
t
(1 row)

Please find attached a patch proposal to fix it.

It does contain additional calls to pgstat_create_relation() and
pgstat_drop_relation() as well as additional TAP tests.

[1]: /messages/by-id/5bfcf1a5-4224-9324-594b-725e704c95b1@amazon.com
/messages/by-id/5bfcf1a5-4224-9324-594b-725e704c95b1@amazon.com

Regards,

--
Bertrand Drouvot
Amazon Web Services: https://aws.amazon.com

Attachments:

v1-0001-pgstat_drop_relation-for-indexes.patchtext/plain; charset=UTF-8; name=v1-0001-pgstat_drop_relation-for-indexes.patchDownload

diff --git a/src/backend/catalog/index.c b/src/backend/catalog/index.c
index d7192f35e3..de10d2ade8 100644
--- a/src/backend/catalog/index.c
+++ b/src/backend/catalog/index.c
@@ -1248,6 +1248,8 @@ index_create(Relation heapRelation,
 	}
 	else
 	{
+		/* ensure that stats are dropped if transaction aborts */
+		pgstat_create_relation(indexRelation);
 		index_build(heapRelation, indexRelation, indexInfo, false, true);
 	}
 
@@ -2325,6 +2327,9 @@ index_drop(Oid indexId, bool concurrent, bool concurrent_lock_mode)
 	if (RELKIND_HAS_STORAGE(userIndexRelation->rd_rel->relkind))
 		RelationDropStorage(userIndexRelation);
 
+	/* ensure that stats are dropped if transaction commits */
+	pgstat_drop_relation(userIndexRelation);
+
 	/*
 	 * Close and flush the index's relcache entry, to ensure relcache doesn't
 	 * try to rebuild it while we're deleting catalog entries. We keep the
@@ -2349,6 +2354,7 @@ index_drop(Oid indexId, bool concurrent, bool concurrent_lock_mode)
 	CatalogTupleDelete(indexRelation, &tuple->t_self);
 
 	ReleaseSysCache(tuple);
+
 	table_close(indexRelation, RowExclusiveLock);
 
 	/*
diff --git a/src/test/recovery/t/030_stats_cleanup_replica.pl b/src/test/recovery/t/030_stats_cleanup_replica.pl
index cc92ddbb52..04fe8a0997 100644
--- a/src/test/recovery/t/030_stats_cleanup_replica.pl
+++ b/src/test/recovery/t/030_stats_cleanup_replica.pl
@@ -29,11 +29,12 @@ $node_standby->start;
 
 my $sect = 'initial';
 
-my ($dboid, $tableoid, $funcoid) =
+my ($dboid, $tableoid, $indexoid, $funcoid) =
   populate_standby_stats('postgres', 'public');
 test_standby_func_tab_stats_status('postgres',
-	$dboid, $tableoid, $funcoid, 't');
+	$dboid, $tableoid, $indexoid, $funcoid, 't');
 
+drop_index_by_oid('postgres', $indexoid);
 drop_table_by_oid('postgres', $tableoid);
 drop_function_by_oid('postgres', $funcoid);
 
@@ -41,7 +42,7 @@ $sect = 'post drop';
 my $primary_lsn = $node_primary->lsn('flush');
 $node_primary->wait_for_catchup($node_standby, 'replay', $primary_lsn);
 test_standby_func_tab_stats_status('postgres',
-	$dboid, $tableoid, $funcoid, 'f');
+	$dboid, $tableoid, $indexoid, $funcoid, 'f');
 
 
 ## Test that stats are cleaned up on standby after dropping indirectly
@@ -52,11 +53,11 @@ $node_primary->safe_psql('postgres', "CREATE SCHEMA drop_schema_test1");
 $primary_lsn = $node_primary->lsn('flush');
 $node_primary->wait_for_catchup($node_standby, 'replay', $primary_lsn);
 
-($dboid, $tableoid, $funcoid) =
+($dboid, $tableoid, $indexoid, $funcoid) =
   populate_standby_stats('postgres', 'drop_schema_test1');
 
 test_standby_func_tab_stats_status('postgres',
-	$dboid, $tableoid, $funcoid, 't');
+	$dboid, $tableoid, $indexoid, $funcoid, 't');
 $node_primary->safe_psql('postgres', "DROP SCHEMA drop_schema_test1 CASCADE");
 
 $sect = "post schema drop";
@@ -66,7 +67,7 @@ $node_primary->wait_for_catchup($node_standby, 'replay', $primary_lsn);
 
 # verify table and function stats removed from standby
 test_standby_func_tab_stats_status('postgres',
-	$dboid, $tableoid, $funcoid, 'f');
+	$dboid, $tableoid, $indexoid, $funcoid, 'f');
 
 
 ## Test that stats are cleaned up on standby after dropping database
@@ -77,10 +78,10 @@ $node_primary->safe_psql('postgres', "CREATE DATABASE test");
 $primary_lsn = $node_primary->lsn('flush');
 $node_primary->wait_for_catchup($node_standby, 'replay', $primary_lsn);
 
-($dboid, $tableoid, $funcoid) = populate_standby_stats('test', 'public');
+($dboid, $tableoid, $indexoid, $funcoid) = populate_standby_stats('test', 'public');
 
 # verify stats are present
-test_standby_func_tab_stats_status('test', $dboid, $tableoid, $funcoid, 't');
+test_standby_func_tab_stats_status('test', $dboid, $tableoid, $indexoid, $funcoid, 't');
 test_standby_db_stats_status('test', $dboid, 't');
 
 $node_primary->safe_psql('postgres', "DROP DATABASE test");
@@ -92,7 +93,7 @@ $node_primary->wait_for_catchup($node_standby, 'replay', $primary_lsn);
 # Note that this connects to 'postgres' but provides the dboid of dropped db
 # 'test' which we acquired previously
 test_standby_func_tab_stats_status('postgres',
-	$dboid, $tableoid, $funcoid, 'f');
+	$dboid, $tableoid, $indexoid, $funcoid, 'f');
 
 test_standby_db_stats_status('postgres', $dboid, 'f');
 
@@ -102,16 +103,16 @@ test_standby_db_stats_status('postgres', $dboid, 'f');
 # NB: Can't test database stats, they're immediately repopulated when
 # reconnecting...
 $sect = "pre restart";
-($dboid, $tableoid, $funcoid) = populate_standby_stats('postgres', 'public');
+($dboid, $tableoid, $indexoid, $funcoid) = populate_standby_stats('postgres', 'public');
 test_standby_func_tab_stats_status('postgres',
-	$dboid, $tableoid, $funcoid, 't');
+	$dboid, $tableoid, $indexoid, $funcoid, 't');
 
 $node_standby->restart();
 
 $sect = "post non-immediate";
 
 test_standby_func_tab_stats_status('postgres',
-	$dboid, $tableoid, $funcoid, 't');
+	$dboid, $tableoid, $indexoid, $funcoid, 't');
 
 # but gone after an immediate restart
 $node_standby->stop('immediate');
@@ -120,7 +121,7 @@ $node_standby->start();
 $sect = "post immediate restart";
 
 test_standby_func_tab_stats_status('postgres',
-	$dboid, $tableoid, $funcoid, 'f');
+	$dboid, $tableoid, $indexoid, $funcoid, 'f');
 
 
 done_testing();
@@ -134,6 +135,9 @@ sub populate_standby_stats
 	$node_primary->safe_psql($connect_db,
 		"CREATE TABLE $schema.drop_tab_test1 AS SELECT generate_series(1,100) AS a"
 	);
+	$node_primary->safe_psql($connect_db,
+		"CREATE INDEX drop_tab_test1_idx on $schema.drop_tab_test1(a);"
+	);
 	$node_primary->safe_psql($connect_db,
 		"CREATE FUNCTION $schema.drop_func_test1() RETURNS VOID AS 'select 2;' LANGUAGE SQL IMMUTABLE"
 	);
@@ -145,6 +149,8 @@ sub populate_standby_stats
 		"SELECT oid FROM pg_database WHERE datname = '$connect_db'");
 	my $tableoid = $node_standby->safe_psql($connect_db,
 		"SELECT '$schema.drop_tab_test1'::regclass::oid");
+	my $indexoid = $node_standby->safe_psql($connect_db,
+		"SELECT '$schema.drop_tab_test1_idx'::regclass::oid");
 	my $funcoid = $node_standby->safe_psql($connect_db,
 		"SELECT '$schema.drop_func_test1()'::regprocedure::oid");
 
@@ -153,7 +159,7 @@ sub populate_standby_stats
 		"SELECT * FROM $schema.drop_tab_test1");
 	$node_standby->safe_psql($connect_db, "SELECT $schema.drop_func_test1()");
 
-	return ($dboid, $tableoid, $funcoid);
+	return ($dboid, $tableoid, $indexoid, $funcoid);
 }
 
 sub drop_function_by_oid
@@ -176,14 +182,26 @@ sub drop_table_by_oid
 	$node_primary->safe_psql($connect_db, "DROP TABLE $table_name");
 }
 
+sub drop_index_by_oid
+{
+	my ($connect_db, $indexoid) = @_;
+
+	# Get index name from returned oid
+	my $index_name =
+	  $node_primary->safe_psql($connect_db, "SELECT '$indexoid'::regclass");
+	$node_primary->safe_psql($connect_db, "DROP index $index_name");
+}
+
 sub test_standby_func_tab_stats_status
 {
 	local $Test::Builder::Level = $Test::Builder::Level + 1;
-	my ($connect_db, $dboid, $tableoid, $funcoid, $present) = @_;
+	my ($connect_db, $dboid, $tableoid, $indexoid, $funcoid, $present) = @_;
 
-	my %expected = (rel => $present, func => $present);
+	my %expected = (idx=> $present, rel => $present, func => $present);
 	my %stats;
 
+	$stats{idx} = $node_standby->safe_psql($connect_db,
+		"SELECT pg_stat_have_stats('relation', $dboid, $indexoid)");
 	$stats{rel} = $node_standby->safe_psql($connect_db,
 		"SELECT pg_stat_have_stats('relation', $dboid, $tableoid)");
 	$stats{func} = $node_standby->safe_psql($connect_db,

Andres Freund

andres@anarazel.de

over 3 years ago

In reply to: Drouvot, Bertrand (#1)

Re: pg_stat_have_stats() returns true for dropped indexes (or for index creation transaction rolled back)

Hi,

On 2022-08-22 18:39:07 +0200, Drouvot, Bertrand wrote:

While working on the relation stats split into table and index stats [1], I
noticed that currently pg_stat_have_stats() returns true for dropped indexes
(or for index creation transaction rolled back).

Good catch.

I guess Horiguchi-san and/or I wrongly assumed it'd be taken care of by the
pgstat_create_relation() in heap_create_with_catalog(), but index_create()
doesn't use that.

Please find attached a patch proposal to fix it.

Perhaps a better fix would be to move the pgstat_create_relation() from
heap_create_with_catalog() into heap_create()? Although I guess it's a bit
pointless to deduplicate given that you're going to split it up again...

It does contain additional calls to pgstat_create_relation() and
pgstat_drop_relation() as well as additional TAP tests.

Would be good to add a test for CREATE INDEX / DROP INDEX / REINDEX
CONCURRENTLY as well.

Might be worth adding a test to stats.sql or stats.spec in the main regression
tests. Perhaps that's best where the aforementioned things should be tested?

@@ -2349,6 +2354,7 @@ index_drop(Oid indexId, bool concurrent, bool concurrent_lock_mode)
CatalogTupleDelete(indexRelation, &tuple->t_self);

ReleaseSysCache(tuple);
+
table_close(indexRelation, RowExclusiveLock);

/*

Assume this was just an accident?

Greetings,

Andres Freund

Drouvot, Bertrand

bdrouvot@amazon.com

over 3 years ago

In reply to: Andres Freund (#2)

1 attachment(s)

Re: pg_stat_have_stats() returns true for dropped indexes (or for index creation transaction rolled back)

Hi,

On 8/22/22 7:36 PM, Andres Freund wrote:

On 2022-08-22 18:39:07 +0200, Drouvot, Bertrand wrote:

Please find attached a patch proposal to fix it.

Perhaps a better fix would be to move the pgstat_create_relation() from
heap_create_with_catalog() into heap_create()? Although I guess it's a bit
pointless to deduplicate given that you're going to split it up again...

Thanks for looking at it!

Agree it's better to move it to heap_create(): it's done in the new
version attached.

We'll see later on if it needs to be duplicated for the table/index
split work.

It does contain additional calls to pgstat_create_relation() and
pgstat_drop_relation() as well as additional TAP tests.

Would be good to add a test for CREATE INDEX / DROP INDEX / REINDEX
CONCURRENTLY as well.

Might be worth adding a test to stats.sql or stats.spec in the main regression
tests. Perhaps that's best where the aforementioned things should be tested?

Yeah that sounds better, I'm also adding more tests around table
creation while at it.

I ended up adding the new tests in stats.sql.

@@ -2349,6 +2354,7 @@ index_drop(Oid indexId, bool concurrent, bool concurrent_lock_mode)
CatalogTupleDelete(indexRelation, &tuple->t_self);

ReleaseSysCache(tuple);
+
table_close(indexRelation, RowExclusiveLock);

/*

Assume this was just an accident?

Oops, thanks!

New version attached.

Regards,

--
Bertrand Drouvot
Amazon Web Services: https://aws.amazon.com

Attachments:

v1-0002-pgstat_drop_relation-for-indexes.patchtext/plain; charset=UTF-8; name=v1-0002-pgstat_drop_relation-for-indexes.patchDownload

diff --git a/src/backend/catalog/heap.c b/src/backend/catalog/heap.c
index 9b03579e6e..9a80ccdccd 100644
--- a/src/backend/catalog/heap.c
+++ b/src/backend/catalog/heap.c
@@ -403,6 +403,9 @@ heap_create(const char *relname,
 		recordDependencyOnTablespace(RelationRelationId, relid,
 									 reltablespace);
 
+	/* ensure that stats are dropped if transaction aborts */
+	pgstat_create_relation(rel);
+
 	return rel;
 }
 
@@ -1477,9 +1480,6 @@ heap_create_with_catalog(const char *relname,
 	if (oncommit != ONCOMMIT_NOOP)
 		register_on_commit_action(relid, oncommit);
 
-	/* ensure that stats are dropped if transaction aborts */
-	pgstat_create_relation(new_rel_desc);
-
 	/*
 	 * ok, the relation has been cataloged, so close our relations and return
 	 * the OID of the newly created relation.
diff --git a/src/backend/catalog/index.c b/src/backend/catalog/index.c
index d7192f35e3..61f1d3926a 100644
--- a/src/backend/catalog/index.c
+++ b/src/backend/catalog/index.c
@@ -2325,6 +2325,9 @@ index_drop(Oid indexId, bool concurrent, bool concurrent_lock_mode)
 	if (RELKIND_HAS_STORAGE(userIndexRelation->rd_rel->relkind))
 		RelationDropStorage(userIndexRelation);
 
+	/* ensure that stats are dropped if transaction commits */
+	pgstat_drop_relation(userIndexRelation);
+
 	/*
 	 * Close and flush the index's relcache entry, to ensure relcache doesn't
 	 * try to rebuild it while we're deleting catalog entries. We keep the
diff --git a/src/test/regress/expected/stats.out b/src/test/regress/expected/stats.out
index 6b233ff4c0..5b01ef68fe 100644
--- a/src/test/regress/expected/stats.out
+++ b/src/test/regress/expected/stats.out
@@ -783,6 +783,160 @@ SELECT pg_stat_have_stats('database', (SELECT oid FROM pg_database WHERE datname
  t
 (1 row)
 
+-- pg_stat_have_stats returns true for table creation inserting data
+CREATE table stats_test_tab1 as select generate_series(1,10) a;
+SELECT 'stats_test_tab1'::regclass::oid AS stats_test_tab1_oid \gset
+SELECT oid AS dboid from pg_database where datname = current_database() \gset
+SELECT pg_stat_have_stats('relation', :dboid, :stats_test_tab1_oid);
+ pg_stat_have_stats 
+--------------------
+ t
+(1 row)
+
+-- pg_stat_have_stats returns true for a rolled back drop table with stats
+BEGIN;
+SELECT pg_stat_have_stats('relation', :dboid, :stats_test_tab1_oid);
+ pg_stat_have_stats 
+--------------------
+ t
+(1 row)
+
+DROP table stats_test_tab1;
+ROLLBACK;
+SELECT pg_stat_have_stats('relation', :dboid, :stats_test_tab1_oid);
+ pg_stat_have_stats 
+--------------------
+ t
+(1 row)
+
+-- pg_stat_have_stats returns false for a dropped table with stats
+SELECT pg_stat_have_stats('relation', :dboid, :stats_test_tab1_oid);
+ pg_stat_have_stats 
+--------------------
+ t
+(1 row)
+
+DROP table stats_test_tab1;
+SELECT pg_stat_have_stats('relation', :dboid, :stats_test_tab1_oid);
+ pg_stat_have_stats 
+--------------------
+ f
+(1 row)
+
+-- pg_stat_have_stats returns false for rolled back table creation inserting data
+BEGIN;
+CREATE table stats_test_tab1 as select generate_series(1,10) a;
+SELECT 'stats_test_tab1'::regclass::oid AS stats_test_tab1_oid \gset
+ROLLBACK;
+SELECT pg_stat_have_stats('relation', :dboid, :stats_test_tab1_oid);
+ pg_stat_have_stats 
+--------------------
+ f
+(1 row)
+
+-- pg_stat_have_stats returns true for committed index creation
+CREATE table stats_test_tab1 as select generate_series(1,10) a;
+CREATE index stats_test_idx1 on stats_test_tab1(a);
+SELECT 'stats_test_idx1'::regclass::oid AS stats_test_idx1_oid \gset
+SET enable_seqscan TO off;
+select a from stats_test_tab1 where a = 3;
+ a 
+---
+ 3
+(1 row)
+
+SELECT pg_stat_have_stats('relation', :dboid, :stats_test_idx1_oid);
+ pg_stat_have_stats 
+--------------------
+ t
+(1 row)
+
+-- pg_stat_have_stats returns false for dropped index with stats
+SELECT pg_stat_have_stats('relation', :dboid, :stats_test_idx1_oid);
+ pg_stat_have_stats 
+--------------------
+ t
+(1 row)
+
+DROP index stats_test_idx1;
+SELECT pg_stat_have_stats('relation', :dboid, :stats_test_idx1_oid);
+ pg_stat_have_stats 
+--------------------
+ f
+(1 row)
+
+-- pg_stat_have_stats returns false for rolled back index creation
+BEGIN;
+CREATE index stats_test_idx1 on stats_test_tab1(a);
+SELECT 'stats_test_idx1'::regclass::oid AS stats_test_idx1_oid \gset
+select a from stats_test_tab1 where a = 3;
+ a 
+---
+ 3
+(1 row)
+
+SELECT pg_stat_have_stats('relation', :dboid, :stats_test_idx1_oid);
+ pg_stat_have_stats 
+--------------------
+ t
+(1 row)
+
+ROLLBACK;
+SELECT pg_stat_have_stats('relation', :dboid, :stats_test_idx1_oid);
+ pg_stat_have_stats 
+--------------------
+ f
+(1 row)
+
+-- pg_stat_have_stats returns true for reindex CONCURRENTLY
+CREATE index stats_test_idx1 on stats_test_tab1(a);
+SELECT 'stats_test_idx1'::regclass::oid AS stats_test_idx1_oid \gset
+select a from stats_test_tab1 where a = 3;
+ a 
+---
+ 3
+(1 row)
+
+SELECT pg_stat_have_stats('relation', :dboid, :stats_test_idx1_oid);
+ pg_stat_have_stats 
+--------------------
+ t
+(1 row)
+
+REINDEX index CONCURRENTLY stats_test_idx1;
+-- false for previous oid
+SELECT pg_stat_have_stats('relation', :dboid, :stats_test_idx1_oid);
+ pg_stat_have_stats 
+--------------------
+ f
+(1 row)
+
+-- true for new oid
+SELECT 'stats_test_idx1'::regclass::oid AS stats_test_idx1_oid \gset
+SELECT pg_stat_have_stats('relation', :dboid, :stats_test_idx1_oid);
+ pg_stat_have_stats 
+--------------------
+ t
+(1 row)
+
+-- pg_stat_have_stats returns true for a rolled back drop index with stats
+BEGIN;
+SELECT pg_stat_have_stats('relation', :dboid, :stats_test_idx1_oid);
+ pg_stat_have_stats 
+--------------------
+ t
+(1 row)
+
+DROP index stats_test_idx1;
+ROLLBACK;
+SELECT pg_stat_have_stats('relation', :dboid, :stats_test_idx1_oid);
+ pg_stat_have_stats 
+--------------------
+ t
+(1 row)
+
+-- put enable_seqscan back to on
+SET enable_seqscan TO on;
 -- ensure that stats accessors handle NULL input correctly
 SELECT pg_stat_get_replication_slot(NULL);
  pg_stat_get_replication_slot 
diff --git a/src/test/regress/sql/stats.sql b/src/test/regress/sql/stats.sql
index 096f00ce8b..7cfefb2abc 100644
--- a/src/test/regress/sql/stats.sql
+++ b/src/test/regress/sql/stats.sql
@@ -390,6 +390,74 @@ SELECT pg_stat_have_stats('zaphod', 0, 0);
 SELECT pg_stat_have_stats('database', (SELECT oid FROM pg_database WHERE datname = current_database()), 1);
 SELECT pg_stat_have_stats('database', (SELECT oid FROM pg_database WHERE datname = current_database()), 0);
 
+-- pg_stat_have_stats returns true for table creation inserting data
+CREATE table stats_test_tab1 as select generate_series(1,10) a;
+SELECT 'stats_test_tab1'::regclass::oid AS stats_test_tab1_oid \gset
+SELECT oid AS dboid from pg_database where datname = current_database() \gset
+SELECT pg_stat_have_stats('relation', :dboid, :stats_test_tab1_oid);
+
+-- pg_stat_have_stats returns true for a rolled back drop table with stats
+BEGIN;
+SELECT pg_stat_have_stats('relation', :dboid, :stats_test_tab1_oid);
+DROP table stats_test_tab1;
+ROLLBACK;
+SELECT pg_stat_have_stats('relation', :dboid, :stats_test_tab1_oid);
+
+-- pg_stat_have_stats returns false for a dropped table with stats
+SELECT pg_stat_have_stats('relation', :dboid, :stats_test_tab1_oid);
+DROP table stats_test_tab1;
+SELECT pg_stat_have_stats('relation', :dboid, :stats_test_tab1_oid);
+
+-- pg_stat_have_stats returns false for rolled back table creation inserting data
+BEGIN;
+CREATE table stats_test_tab1 as select generate_series(1,10) a;
+SELECT 'stats_test_tab1'::regclass::oid AS stats_test_tab1_oid \gset
+ROLLBACK;
+SELECT pg_stat_have_stats('relation', :dboid, :stats_test_tab1_oid);
+
+-- pg_stat_have_stats returns true for committed index creation
+CREATE table stats_test_tab1 as select generate_series(1,10) a;
+CREATE index stats_test_idx1 on stats_test_tab1(a);
+SELECT 'stats_test_idx1'::regclass::oid AS stats_test_idx1_oid \gset
+SET enable_seqscan TO off;
+select a from stats_test_tab1 where a = 3;
+SELECT pg_stat_have_stats('relation', :dboid, :stats_test_idx1_oid);
+
+-- pg_stat_have_stats returns false for dropped index with stats
+SELECT pg_stat_have_stats('relation', :dboid, :stats_test_idx1_oid);
+DROP index stats_test_idx1;
+SELECT pg_stat_have_stats('relation', :dboid, :stats_test_idx1_oid);
+
+-- pg_stat_have_stats returns false for rolled back index creation
+BEGIN;
+CREATE index stats_test_idx1 on stats_test_tab1(a);
+SELECT 'stats_test_idx1'::regclass::oid AS stats_test_idx1_oid \gset
+select a from stats_test_tab1 where a = 3;
+SELECT pg_stat_have_stats('relation', :dboid, :stats_test_idx1_oid);
+ROLLBACK;
+SELECT pg_stat_have_stats('relation', :dboid, :stats_test_idx1_oid);
+
+-- pg_stat_have_stats returns true for reindex CONCURRENTLY
+CREATE index stats_test_idx1 on stats_test_tab1(a);
+SELECT 'stats_test_idx1'::regclass::oid AS stats_test_idx1_oid \gset
+select a from stats_test_tab1 where a = 3;
+SELECT pg_stat_have_stats('relation', :dboid, :stats_test_idx1_oid);
+REINDEX index CONCURRENTLY stats_test_idx1;
+-- false for previous oid
+SELECT pg_stat_have_stats('relation', :dboid, :stats_test_idx1_oid);
+-- true for new oid
+SELECT 'stats_test_idx1'::regclass::oid AS stats_test_idx1_oid \gset
+SELECT pg_stat_have_stats('relation', :dboid, :stats_test_idx1_oid);
+
+-- pg_stat_have_stats returns true for a rolled back drop index with stats
+BEGIN;
+SELECT pg_stat_have_stats('relation', :dboid, :stats_test_idx1_oid);
+DROP index stats_test_idx1;
+ROLLBACK;
+SELECT pg_stat_have_stats('relation', :dboid, :stats_test_idx1_oid);
+
+-- put enable_seqscan back to on
+SET enable_seqscan TO on;
 
 -- ensure that stats accessors handle NULL input correctly
 SELECT pg_stat_get_replication_slot(NULL);

Kyotaro Horiguchi

horikyota.ntt@gmail.com

over 3 years ago

In reply to: Drouvot, Bertrand (#3)

Re: pg_stat_have_stats() returns true for dropped indexes (or for index creation transaction rolled back)

Good catch, and thanks for the patch!

(The file name would correctly be v2-0001-...:)

At Tue, 23 Aug 2022 09:58:03 +0200, "Drouvot, Bertrand" <bdrouvot@amazon.com> wrote in

Agree it's better to move it to heap_create(): it's done in the new
version attached.

+1 (not considering stats splitting)

We'll see later on if it needs to be duplicated for the table/index
split work.

The code changes looks good to me.

It does contain additional calls to pgstat_create_relation() and
pgstat_drop_relation() as well as additional TAP tests.

Would be good to add a test for CREATE INDEX / DROP INDEX / REINDEX
CONCURRENTLY as well.

Might be worth adding a test to stats.sql or stats.spec in the main
regression
tests. Perhaps that's best where the aforementioned things should be
tested?

Yeah that sounds better, I'm also adding more tests around table
creation while at it.

I ended up adding the new tests in stats.sql.

+-- pg_stat_have_stats returns true for table creation inserting data
+-- pg_stat_have_stats returns true for committed index creation
+

Not sure we need this, as we check that already in the same file. (In
other words, if we failed this, the should have failed earlier.) Maybe
we only need check for drop operations and reindex cases?

We have other variable-numbered stats kinds
FUNCTION/REPLSLOT/SUBSCRIPTION. Don't we need the same for these?

regards.

--
Kyotaro Horiguchi
NTT Open Source Software Center

Drouvot, Bertrand

bdrouvot@amazon.com

over 3 years ago

In reply to: Kyotaro Horiguchi (#4)

1 attachment(s)

Re: pg_stat_have_stats() returns true for dropped indexes (or for index creation transaction rolled back)

Hi,

On 8/25/22 4:47 AM, Kyotaro Horiguchi wrote:

Good catch, and thanks for the patch!

(The file name would correctly be v2-0001-...:)

At Tue, 23 Aug 2022 09:58:03 +0200, "Drouvot, Bertrand" <bdrouvot@amazon.com> wrote in

Agree it's better to move it to heap_create(): it's done in the new
version attached.

+1 (not considering stats splitting)

We'll see later on if it needs to be duplicated for the table/index
split work.

The code changes looks good to me.

Thanks for looking at it!

+-- pg_stat_have_stats returns true for table creation inserting data
+-- pg_stat_have_stats returns true for committed index creation
+
Not sure we need this, as we check that already in the same file. (In
other words, if we failed this, the should have failed earlier.)

That's right.

Maybe
we only need check for drop operations

Looking closer at it, I think we are already good for the drop case on
the tables (by making direct use of the pg_stat_get_* functions on the
before dropped oid).

So I think we can remove all the "table" new checks: new patch attached
is doing so.

On the other hand, for the index case, I think it's better to keep the
"committed index creation one".

Indeed, to check that the drop behaves correctly I think it's better in
"the same test" to ensure we've had the desired result before the drop
(I mean having pg_stat_have_stats() returning false after a drop does
not really help if we are not 100% sure it was returning true for the
exact same index before the drop).

We have other variable-numbered stats kinds
FUNCTION/REPLSLOT/SUBSCRIPTION. Don't we need the same for these?

I don't think we need more tests for the FUNCTION case (as it looks to
me it is already covered in stat.sql by the pg_stat_get_function_calls()
calls on the dropped functions oids).

For SUBSCRIPTION, i think this is covered in 026_stats.pl:

# Subscription stats for sub1 should be gone
is( $node_subscriber->safe_psql(
        $db, qq(SELECT pg_stat_have_stats('subscription', 0, $sub1_oid))),
    qq(f),
    qq(Subscription stats for subscription '$sub1_name' should be
removed.));

For REPLSLOT, I agree that we can add one test: I added it in
contrib/test_decoding/sql/stats.sql. It relies on pg_stat_have_stats()
(as relying on pg_stat_replication_slots and/or
pg_stat_get_replication_slot() would not help that much for this test
given that the slot has been removed from ReplicationSlotCtl)

Attaching v3-0001 (with the right "numbering" this time ;-) )

Regards,

--
Bertrand Drouvot
Amazon Web Services: https://aws.amazon.com

Attachments:

v3-0001-pgstat_drop_relation-for-indexes.patchtext/plain; charset=UTF-8; name=v3-0001-pgstat_drop_relation-for-indexes.patchDownload

diff --git a/contrib/test_decoding/expected/stats.out b/contrib/test_decoding/expected/stats.out
index 78d36429c8..4526c7e87f 100644
--- a/contrib/test_decoding/expected/stats.out
+++ b/contrib/test_decoding/expected/stats.out
@@ -139,6 +139,14 @@ SELECT slot_name FROM pg_stat_replication_slots;
 
 COMMIT;
 DROP TABLE stats_test;
+-- pg_stat_have_stats returns true for regression_slot_stats1
+-- Its index is 1 in ReplicationSlotCtl->replication_slots
+select pg_stat_have_stats('replslot', 0, 1);
+ pg_stat_have_stats 
+--------------------
+ t
+(1 row)
+
 SELECT pg_drop_replication_slot('regression_slot_stats1'),
     pg_drop_replication_slot('regression_slot_stats2'),
     pg_drop_replication_slot('regression_slot_stats3');
@@ -147,3 +155,10 @@ SELECT pg_drop_replication_slot('regression_slot_stats1'),
                           |                          | 
 (1 row)
 
+-- pg_stat_have_stats returns false for regression_slot_stats1
+select pg_stat_have_stats('replslot', 0, 1);
+ pg_stat_have_stats 
+--------------------
+ f
+(1 row)
+
diff --git a/contrib/test_decoding/sql/stats.sql b/contrib/test_decoding/sql/stats.sql
index 630371f147..036039b966 100644
--- a/contrib/test_decoding/sql/stats.sql
+++ b/contrib/test_decoding/sql/stats.sql
@@ -51,6 +51,12 @@ SELECT slot_name FROM pg_stat_replication_slots;
 COMMIT;
 
 DROP TABLE stats_test;
+
+-- pg_stat_have_stats returns true for regression_slot_stats1
+-- Its index is 1 in ReplicationSlotCtl->replication_slots
+select pg_stat_have_stats('replslot', 0, 1);
 SELECT pg_drop_replication_slot('regression_slot_stats1'),
     pg_drop_replication_slot('regression_slot_stats2'),
     pg_drop_replication_slot('regression_slot_stats3');
+-- pg_stat_have_stats returns false for regression_slot_stats1
+select pg_stat_have_stats('replslot', 0, 1);
diff --git a/src/backend/catalog/heap.c b/src/backend/catalog/heap.c
index 9b03579e6e..9a80ccdccd 100644
--- a/src/backend/catalog/heap.c
+++ b/src/backend/catalog/heap.c
@@ -403,6 +403,9 @@ heap_create(const char *relname,
 		recordDependencyOnTablespace(RelationRelationId, relid,
 									 reltablespace);
 
+	/* ensure that stats are dropped if transaction aborts */
+	pgstat_create_relation(rel);
+
 	return rel;
 }
 
@@ -1477,9 +1480,6 @@ heap_create_with_catalog(const char *relname,
 	if (oncommit != ONCOMMIT_NOOP)
 		register_on_commit_action(relid, oncommit);
 
-	/* ensure that stats are dropped if transaction aborts */
-	pgstat_create_relation(new_rel_desc);
-
 	/*
 	 * ok, the relation has been cataloged, so close our relations and return
 	 * the OID of the newly created relation.
diff --git a/src/backend/catalog/index.c b/src/backend/catalog/index.c
index d7192f35e3..61f1d3926a 100644
--- a/src/backend/catalog/index.c
+++ b/src/backend/catalog/index.c
@@ -2325,6 +2325,9 @@ index_drop(Oid indexId, bool concurrent, bool concurrent_lock_mode)
 	if (RELKIND_HAS_STORAGE(userIndexRelation->rd_rel->relkind))
 		RelationDropStorage(userIndexRelation);
 
+	/* ensure that stats are dropped if transaction commits */
+	pgstat_drop_relation(userIndexRelation);
+
 	/*
 	 * Close and flush the index's relcache entry, to ensure relcache doesn't
 	 * try to rebuild it while we're deleting catalog entries. We keep the
diff --git a/src/test/regress/expected/stats.out b/src/test/regress/expected/stats.out
index 6b233ff4c0..94758224dc 100644
--- a/src/test/regress/expected/stats.out
+++ b/src/test/regress/expected/stats.out
@@ -783,6 +783,110 @@ SELECT pg_stat_have_stats('database', (SELECT oid FROM pg_database WHERE datname
  t
 (1 row)
 
+-- pg_stat_have_stats returns true for committed index creation
+CREATE table stats_test_tab1 as select generate_series(1,10) a;
+CREATE index stats_test_idx1 on stats_test_tab1(a);
+SELECT oid AS dboid from pg_database where datname = current_database() \gset
+SELECT 'stats_test_idx1'::regclass::oid AS stats_test_idx1_oid \gset
+SET enable_seqscan TO off;
+select a from stats_test_tab1 where a = 3;
+ a 
+---
+ 3
+(1 row)
+
+SELECT pg_stat_have_stats('relation', :dboid, :stats_test_idx1_oid);
+ pg_stat_have_stats 
+--------------------
+ t
+(1 row)
+
+-- pg_stat_have_stats returns false for dropped index with stats
+SELECT pg_stat_have_stats('relation', :dboid, :stats_test_idx1_oid);
+ pg_stat_have_stats 
+--------------------
+ t
+(1 row)
+
+DROP index stats_test_idx1;
+SELECT pg_stat_have_stats('relation', :dboid, :stats_test_idx1_oid);
+ pg_stat_have_stats 
+--------------------
+ f
+(1 row)
+
+-- pg_stat_have_stats returns false for rolled back index creation
+BEGIN;
+CREATE index stats_test_idx1 on stats_test_tab1(a);
+SELECT 'stats_test_idx1'::regclass::oid AS stats_test_idx1_oid \gset
+select a from stats_test_tab1 where a = 3;
+ a 
+---
+ 3
+(1 row)
+
+SELECT pg_stat_have_stats('relation', :dboid, :stats_test_idx1_oid);
+ pg_stat_have_stats 
+--------------------
+ t
+(1 row)
+
+ROLLBACK;
+SELECT pg_stat_have_stats('relation', :dboid, :stats_test_idx1_oid);
+ pg_stat_have_stats 
+--------------------
+ f
+(1 row)
+
+-- pg_stat_have_stats returns true for reindex CONCURRENTLY
+CREATE index stats_test_idx1 on stats_test_tab1(a);
+SELECT 'stats_test_idx1'::regclass::oid AS stats_test_idx1_oid \gset
+select a from stats_test_tab1 where a = 3;
+ a 
+---
+ 3
+(1 row)
+
+SELECT pg_stat_have_stats('relation', :dboid, :stats_test_idx1_oid);
+ pg_stat_have_stats 
+--------------------
+ t
+(1 row)
+
+REINDEX index CONCURRENTLY stats_test_idx1;
+-- false for previous oid
+SELECT pg_stat_have_stats('relation', :dboid, :stats_test_idx1_oid);
+ pg_stat_have_stats 
+--------------------
+ f
+(1 row)
+
+-- true for new oid
+SELECT 'stats_test_idx1'::regclass::oid AS stats_test_idx1_oid \gset
+SELECT pg_stat_have_stats('relation', :dboid, :stats_test_idx1_oid);
+ pg_stat_have_stats 
+--------------------
+ t
+(1 row)
+
+-- pg_stat_have_stats returns true for a rolled back drop index with stats
+BEGIN;
+SELECT pg_stat_have_stats('relation', :dboid, :stats_test_idx1_oid);
+ pg_stat_have_stats 
+--------------------
+ t
+(1 row)
+
+DROP index stats_test_idx1;
+ROLLBACK;
+SELECT pg_stat_have_stats('relation', :dboid, :stats_test_idx1_oid);
+ pg_stat_have_stats 
+--------------------
+ t
+(1 row)
+
+-- put enable_seqscan back to on
+SET enable_seqscan TO on;
 -- ensure that stats accessors handle NULL input correctly
 SELECT pg_stat_get_replication_slot(NULL);
  pg_stat_get_replication_slot 
diff --git a/src/test/regress/sql/stats.sql b/src/test/regress/sql/stats.sql
index 096f00ce8b..1c340ffc03 100644
--- a/src/test/regress/sql/stats.sql
+++ b/src/test/regress/sql/stats.sql
@@ -390,6 +390,50 @@ SELECT pg_stat_have_stats('zaphod', 0, 0);
 SELECT pg_stat_have_stats('database', (SELECT oid FROM pg_database WHERE datname = current_database()), 1);
 SELECT pg_stat_have_stats('database', (SELECT oid FROM pg_database WHERE datname = current_database()), 0);
 
+-- pg_stat_have_stats returns true for committed index creation
+CREATE table stats_test_tab1 as select generate_series(1,10) a;
+CREATE index stats_test_idx1 on stats_test_tab1(a);
+SELECT oid AS dboid from pg_database where datname = current_database() \gset
+SELECT 'stats_test_idx1'::regclass::oid AS stats_test_idx1_oid \gset
+SET enable_seqscan TO off;
+select a from stats_test_tab1 where a = 3;
+SELECT pg_stat_have_stats('relation', :dboid, :stats_test_idx1_oid);
+
+-- pg_stat_have_stats returns false for dropped index with stats
+SELECT pg_stat_have_stats('relation', :dboid, :stats_test_idx1_oid);
+DROP index stats_test_idx1;
+SELECT pg_stat_have_stats('relation', :dboid, :stats_test_idx1_oid);
+
+-- pg_stat_have_stats returns false for rolled back index creation
+BEGIN;
+CREATE index stats_test_idx1 on stats_test_tab1(a);
+SELECT 'stats_test_idx1'::regclass::oid AS stats_test_idx1_oid \gset
+select a from stats_test_tab1 where a = 3;
+SELECT pg_stat_have_stats('relation', :dboid, :stats_test_idx1_oid);
+ROLLBACK;
+SELECT pg_stat_have_stats('relation', :dboid, :stats_test_idx1_oid);
+
+-- pg_stat_have_stats returns true for reindex CONCURRENTLY
+CREATE index stats_test_idx1 on stats_test_tab1(a);
+SELECT 'stats_test_idx1'::regclass::oid AS stats_test_idx1_oid \gset
+select a from stats_test_tab1 where a = 3;
+SELECT pg_stat_have_stats('relation', :dboid, :stats_test_idx1_oid);
+REINDEX index CONCURRENTLY stats_test_idx1;
+-- false for previous oid
+SELECT pg_stat_have_stats('relation', :dboid, :stats_test_idx1_oid);
+-- true for new oid
+SELECT 'stats_test_idx1'::regclass::oid AS stats_test_idx1_oid \gset
+SELECT pg_stat_have_stats('relation', :dboid, :stats_test_idx1_oid);
+
+-- pg_stat_have_stats returns true for a rolled back drop index with stats
+BEGIN;
+SELECT pg_stat_have_stats('relation', :dboid, :stats_test_idx1_oid);
+DROP index stats_test_idx1;
+ROLLBACK;
+SELECT pg_stat_have_stats('relation', :dboid, :stats_test_idx1_oid);
+
+-- put enable_seqscan back to on
+SET enable_seqscan TO on;
 
 -- ensure that stats accessors handle NULL input correctly
 SELECT pg_stat_get_replication_slot(NULL);

Kyotaro Horiguchi

horikyota.ntt@gmail.com

over 3 years ago

In reply to: Drouvot, Bertrand (#5)

Re: pg_stat_have_stats() returns true for dropped indexes (or for index creation transaction rolled back)

At Thu, 25 Aug 2022 11:44:34 +0200, "Drouvot, Bertrand" <bdrouvot@amazon.com> wrote in

Looking closer at it, I think we are already good for the drop case on
the tables (by making direct use of the pg_stat_get_* functions on the
before dropped oid).

So I think we can remove all the "table" new checks: new patch
attached is doing so.

On the other hand, for the index case, I think it's better to keep the
"committed index creation one".

I agree.

Indeed, to check that the drop behaves correctly I think it's better
in "the same test" to ensure we've had the desired result before the
drop (I mean having pg_stat_have_stats() returning false after a drop
does not really help if we are not 100% sure it was returning true for
the exact same index before the drop).

Sounds reasonable.

We have other variable-numbered stats kinds
FUNCTION/REPLSLOT/SUBSCRIPTION. Don't we need the same for these?

I don't think we need more tests for the FUNCTION case (as it looks to
me it is already covered in stat.sql by the
pg_stat_get_function_calls() calls on the dropped functions oids).

Right.

For SUBSCRIPTION, i think this is covered in 026_stats.pl:

# Subscription stats for sub1 should be gone
is( $node_subscriber->safe_psql(
        $db, qq(SELECT pg_stat_have_stats('subscription', 0,
$sub1_oid))),
    qq(f),
    qq(Subscription stats for subscription '$sub1_name' should be
removed.));

For REPLSLOT, I agree that we can add one test: I added it in
contrib/test_decoding/sql/stats.sql. It relies on pg_stat_have_stats()
(as relying on pg_stat_replication_slots and/or
pg_stat_get_replication_slot() would not help that much for this test
given that the slot has been removed from ReplicationSlotCtl)

Thanks for the searching.

+-- pg_stat_have_stats returns true for regression_slot_stats1
+-- Its index is 1 in ReplicationSlotCtl->replication_slots
+select pg_stat_have_stats('replslot', 0, 1);

This is wrong. The index is actually 0. We cannot know the id
reliably since we don't expose it at all. We could slightly increase
robustness by assuming the range of the id but that is just moving the
problem to another place. If the test is broken by a change of
replslot id assignment policy, it would be easily found and fixed.

So is it fine simply fixing the comment with the correct ID?

Or, contrarily we can be more sensitive to the change of ID assignment
policy by checking all the replication slots.

select count(n) from generate_series(0, 2) as n where pg_stat_have_stats('replslot', 0, n);

The number changes from 3 to 0 across the slots drop.. If any of the
slots has gone out of the range, the number before the drop decreases.

Attaching v3-0001 (with the right "numbering" this time ;-) )

Yeah, Looks fine:p

regards.

--
Kyotaro Horiguchi
NTT Open Source Software Center

Andres Freund

andres@anarazel.de

over 3 years ago

In reply to: Drouvot, Bertrand (#5)

Re: pg_stat_have_stats() returns true for dropped indexes (or for index creation transaction rolled back)

Hiu,

On 2022-08-25 11:44:34 +0200, Drouvot, Bertrand wrote:

For REPLSLOT, I agree that we can add one test: I added it in
contrib/test_decoding/sql/stats.sql. It relies on pg_stat_have_stats() (as
relying on pg_stat_replication_slots and/or pg_stat_get_replication_slot()
would not help that much for this test given that the slot has been removed
from ReplicationSlotCtl)

As Horiguchi-san noted, we can't rely on specific indexes being used. I feel
ok with the current coverage in that area, but if we *really* feel we need to
test it, we'd need to count the number of indexes with slots before dropping
the slot and after the drop.

+-- pg_stat_have_stats returns true for committed index creation

Maybe another test for an uncommitted index creation would be good too?

Could you try running this test with debug_discard_caches = 1 - it's pretty
easy to write tests in this area that aren't reliable timing wise.

+CREATE table stats_test_tab1 as select generate_series(1,10) a;
+CREATE index stats_test_idx1 on stats_test_tab1(a);
+SELECT oid AS dboid from pg_database where datname = current_database() \gset

Since you introduced this, maybe convert the other instance of this query at
the end of the file as well?

Greetings,

Andres Freund

Drouvot, Bertrand

bdrouvot@amazon.com

over 3 years ago

In reply to: Kyotaro Horiguchi (#6)

Re: pg_stat_have_stats() returns true for dropped indexes (or for index creation transaction rolled back)

Hi,

On 8/31/22 9:10 AM, Kyotaro Horiguchi wrote:

Thanks for the searching.
+-- pg_stat_have_stats returns true for regression_slot_stats1
+-- Its index is 1 in ReplicationSlotCtl->replication_slots
+select pg_stat_have_stats('replslot', 0, 1);

This is wrong. The index is actually 0.

Right, thanks for pointing out.

(gdb) p get_replslot_index("regression_slot_stats1")
$1 = 0
(gdb) p get_replslot_index("regression_slot_stats2")
$2 = 1
(gdb) p get_replslot_index("regression_slot_stats3")

$3 = 2

We cannot know the id
reliably since we don't expose it at all.

Right.

We could slightly increase
robustness by assuming the range of the id but that is just moving the
problem to another place. If the test is broken by a change of
replslot id assignment policy, it would be easily found and fixed.

So is it fine simply fixing the comment with the correct ID?

Or, contrarily we can be more sensitive to the change of ID assignment
policy by checking all the replication slots.

select count(n) from generate_series(0, 2) as n where pg_stat_have_stats('replslot', 0, n);

The number changes from 3 to 0 across the slots drop.. If any of the
slots has gone out of the range, the number before the drop decreases.

Thanks for the ideas! I'm coming up with a slightly different one (also
based on Andre's feedback in [1]/messages/by-id/20220831192657.jqhphpud2mxbzbom@awork3.anarazel.de) in the upcoming v4.

[1]: /messages/by-id/20220831192657.jqhphpud2mxbzbom@awork3.anarazel.de
/messages/by-id/20220831192657.jqhphpud2mxbzbom@awork3.anarazel.de

Regards,

--
Bertrand Drouvot
Amazon Web Services:https://aws.amazon.com

Drouvot, Bertrand

bdrouvot@amazon.com

over 3 years ago

In reply to: Andres Freund (#7)

1 attachment(s)

Re: pg_stat_have_stats() returns true for dropped indexes (or for index creation transaction rolled back)

Hi,

On 8/31/22 9:26 PM, Andres Freund wrote:

Hiu,

On 2022-08-25 11:44:34 +0200, Drouvot, Bertrand wrote:

For REPLSLOT, I agree that we can add one test: I added it in
contrib/test_decoding/sql/stats.sql. It relies on pg_stat_have_stats() (as
relying on pg_stat_replication_slots and/or pg_stat_get_replication_slot()
would not help that much for this test given that the slot has been removed
from ReplicationSlotCtl)

As Horiguchi-san noted, we can't rely on specific indexes being used.

Yeah.

I feel
ok with the current coverage in that area, but if we *really* feel we need to
test it, we'd need to count the number of indexes with slots before dropping
the slot and after the drop.

Thanks for the suggestion, I'm coming up with this proposal in v4 attached:

* count the number of slots
* ensure we have at least one for which pg_stat_have_stats() returns true
* get the list of ids (true_ids) for which pg_stat_have_stats()
returns true
* drop all the slots
* get the list of ids (false_ids) for which pg_stat_have_stats()
returns false
* ensure that both lists (true_ids and false_ids) are the same

I don't "really" feel the need we need to test it but i think that this
thread was a good opportunity to try to test it.

That said, that's also fine for me if this test is not part of the patch.

Maybe a better/simpler option could be to expose a function to get the
slot id based on its name and then write a "simple" test with it? (If so
I think that would better to start another patch/thread).

+-- pg_stat_have_stats returns true for committed index creation

Maybe another test for an uncommitted index creation would be good too?

You mean in addition to the "-- pg_stat_have_stats returns false for
rolled back index creation" one?

Could you try running this test with debug_discard_caches = 1 - it's pretty
easy to write tests in this area that aren't reliable timing wise.

Thanks for the suggestion. I did and it passed without any issues.

+CREATE table stats_test_tab1 as select generate_series(1,10) a;
+CREATE index stats_test_idx1 on stats_test_tab1(a);
+SELECT oid AS dboid from pg_database where datname = current_database() \gset
Since you introduced this, maybe convert the other instance of this query at
the end of the file as well?

yeah good point. In v4, I moved the dboid recording at the top and use
it when appropriate.

Regards,

Bertrand Drouvot
Amazon Web Services:https://aws.amazon.com

Attachments:

v4-0001-pgstat_drop_relation-for-indexes.patchtext/plain; charset=UTF-8; name=v4-0001-pgstat_drop_relation-for-indexes.patchDownload

diff --git a/contrib/test_decoding/expected/stats.out b/contrib/test_decoding/expected/stats.out
index 78d36429c8..fa18fba7ba 100644
--- a/contrib/test_decoding/expected/stats.out
+++ b/contrib/test_decoding/expected/stats.out
@@ -139,6 +139,19 @@ SELECT slot_name FROM pg_stat_replication_slots;
 
 COMMIT;
 DROP TABLE stats_test;
+-- Count the number of slots
+select count(1) as nb_slots from pg_stat_replication_slots \gset
+-- We want pg_stat_have_stats() to return true at least one time for the slots id range
+select count(1) > 0 from generate_series(0, :nb_slots - 1) as n where pg_stat_have_stats('replslot', 0, n);
+ ?column? 
+----------
+ t
+(1 row)
+
+-- Record the replication_slots id(s) for which pg_stat_have_stats() returns true
+select array(select n from generate_series(0, :nb_slots - 1) as n where pg_stat_have_stats('replslot', 0, n) order by n asc) as true_ids \gset
+\set true_ids '''' :true_ids ''''
+-- Drop the 3 slots
 SELECT pg_drop_replication_slot('regression_slot_stats1'),
     pg_drop_replication_slot('regression_slot_stats2'),
     pg_drop_replication_slot('regression_slot_stats3');
@@ -147,3 +160,12 @@ SELECT pg_drop_replication_slot('regression_slot_stats1'),
                           |                          | 
 (1 row)
 
+-- We want the ones that were true to be false after dropping the slots
+select array(select n from generate_series(0, :nb_slots - 1) as n where not pg_stat_have_stats('replslot', 0, n) order by n asc) as false_ids \gset
+\set false_ids '''' :false_ids ''''
+select :true_ids = :false_ids;
+ ?column? 
+----------
+ t
+(1 row)
+
diff --git a/contrib/test_decoding/sql/stats.sql b/contrib/test_decoding/sql/stats.sql
index 630371f147..ffb021f76d 100644
--- a/contrib/test_decoding/sql/stats.sql
+++ b/contrib/test_decoding/sql/stats.sql
@@ -51,6 +51,23 @@ SELECT slot_name FROM pg_stat_replication_slots;
 COMMIT;
 
 DROP TABLE stats_test;
+
+-- Count the number of slots
+select count(1) as nb_slots from pg_stat_replication_slots \gset
+
+-- We want pg_stat_have_stats() to return true at least one time for the slots id range
+select count(1) > 0 from generate_series(0, :nb_slots - 1) as n where pg_stat_have_stats('replslot', 0, n);
+
+-- Record the replication_slots id(s) for which pg_stat_have_stats() returns true
+select array(select n from generate_series(0, :nb_slots - 1) as n where pg_stat_have_stats('replslot', 0, n) order by n asc) as true_ids \gset
+\set true_ids '''' :true_ids ''''
+
+-- Drop the 3 slots
 SELECT pg_drop_replication_slot('regression_slot_stats1'),
     pg_drop_replication_slot('regression_slot_stats2'),
     pg_drop_replication_slot('regression_slot_stats3');
+
+-- We want the ones that were true to be false after dropping the slots
+select array(select n from generate_series(0, :nb_slots - 1) as n where not pg_stat_have_stats('replslot', 0, n) order by n asc) as false_ids \gset
+\set false_ids '''' :false_ids ''''
+select :true_ids = :false_ids;
\ No newline at end of file
diff --git a/src/backend/catalog/heap.c b/src/backend/catalog/heap.c
index 9b03579e6e..9a80ccdccd 100644
--- a/src/backend/catalog/heap.c
+++ b/src/backend/catalog/heap.c
@@ -403,6 +403,9 @@ heap_create(const char *relname,
 		recordDependencyOnTablespace(RelationRelationId, relid,
 									 reltablespace);
 
+	/* ensure that stats are dropped if transaction aborts */
+	pgstat_create_relation(rel);
+
 	return rel;
 }
 
@@ -1477,9 +1480,6 @@ heap_create_with_catalog(const char *relname,
 	if (oncommit != ONCOMMIT_NOOP)
 		register_on_commit_action(relid, oncommit);
 
-	/* ensure that stats are dropped if transaction aborts */
-	pgstat_create_relation(new_rel_desc);
-
 	/*
 	 * ok, the relation has been cataloged, so close our relations and return
 	 * the OID of the newly created relation.
diff --git a/src/backend/catalog/index.c b/src/backend/catalog/index.c
index d7192f35e3..61f1d3926a 100644
--- a/src/backend/catalog/index.c
+++ b/src/backend/catalog/index.c
@@ -2325,6 +2325,9 @@ index_drop(Oid indexId, bool concurrent, bool concurrent_lock_mode)
 	if (RELKIND_HAS_STORAGE(userIndexRelation->rd_rel->relkind))
 		RelationDropStorage(userIndexRelation);
 
+	/* ensure that stats are dropped if transaction commits */
+	pgstat_drop_relation(userIndexRelation);
+
 	/*
 	 * Close and flush the index's relcache entry, to ensure relcache doesn't
 	 * try to rebuild it while we're deleting catalog entries. We keep the
diff --git a/src/test/regress/expected/stats.out b/src/test/regress/expected/stats.out
index 6b233ff4c0..fa0a2c6f7a 100644
--- a/src/test/regress/expected/stats.out
+++ b/src/test/regress/expected/stats.out
@@ -18,6 +18,8 @@ SET enable_indexscan TO on;
 SET enable_indexonlyscan TO off;
 -- not enabled by default, but we want to test it...
 SET track_functions TO 'all';
+-- record dboid for later use
+SELECT oid AS dboid from pg_database where datname = current_database() \gset
 -- save counters
 BEGIN;
 SET LOCAL stats_fetch_consistency = snapshot;
@@ -771,18 +773,121 @@ SELECT pg_stat_have_stats('bgwriter', 0, 0);
 SELECT pg_stat_have_stats('zaphod', 0, 0);
 ERROR:  invalid statistics kind: "zaphod"
 -- db stats have objoid 0
-SELECT pg_stat_have_stats('database', (SELECT oid FROM pg_database WHERE datname = current_database()), 1);
+SELECT pg_stat_have_stats('database', :dboid, 1);
  pg_stat_have_stats 
 --------------------
  f
 (1 row)
 
-SELECT pg_stat_have_stats('database', (SELECT oid FROM pg_database WHERE datname = current_database()), 0);
+SELECT pg_stat_have_stats('database', :dboid, 0);
  pg_stat_have_stats 
 --------------------
  t
 (1 row)
 
+-- pg_stat_have_stats returns true for committed index creation
+CREATE table stats_test_tab1 as select generate_series(1,10) a;
+CREATE index stats_test_idx1 on stats_test_tab1(a);
+SELECT 'stats_test_idx1'::regclass::oid AS stats_test_idx1_oid \gset
+SET enable_seqscan TO off;
+select a from stats_test_tab1 where a = 3;
+ a 
+---
+ 3
+(1 row)
+
+SELECT pg_stat_have_stats('relation', :dboid, :stats_test_idx1_oid);
+ pg_stat_have_stats 
+--------------------
+ t
+(1 row)
+
+-- pg_stat_have_stats returns false for dropped index with stats
+SELECT pg_stat_have_stats('relation', :dboid, :stats_test_idx1_oid);
+ pg_stat_have_stats 
+--------------------
+ t
+(1 row)
+
+DROP index stats_test_idx1;
+SELECT pg_stat_have_stats('relation', :dboid, :stats_test_idx1_oid);
+ pg_stat_have_stats 
+--------------------
+ f
+(1 row)
+
+-- pg_stat_have_stats returns false for rolled back index creation
+BEGIN;
+CREATE index stats_test_idx1 on stats_test_tab1(a);
+SELECT 'stats_test_idx1'::regclass::oid AS stats_test_idx1_oid \gset
+select a from stats_test_tab1 where a = 3;
+ a 
+---
+ 3
+(1 row)
+
+SELECT pg_stat_have_stats('relation', :dboid, :stats_test_idx1_oid);
+ pg_stat_have_stats 
+--------------------
+ t
+(1 row)
+
+ROLLBACK;
+SELECT pg_stat_have_stats('relation', :dboid, :stats_test_idx1_oid);
+ pg_stat_have_stats 
+--------------------
+ f
+(1 row)
+
+-- pg_stat_have_stats returns true for reindex CONCURRENTLY
+CREATE index stats_test_idx1 on stats_test_tab1(a);
+SELECT 'stats_test_idx1'::regclass::oid AS stats_test_idx1_oid \gset
+select a from stats_test_tab1 where a = 3;
+ a 
+---
+ 3
+(1 row)
+
+SELECT pg_stat_have_stats('relation', :dboid, :stats_test_idx1_oid);
+ pg_stat_have_stats 
+--------------------
+ t
+(1 row)
+
+REINDEX index CONCURRENTLY stats_test_idx1;
+-- false for previous oid
+SELECT pg_stat_have_stats('relation', :dboid, :stats_test_idx1_oid);
+ pg_stat_have_stats 
+--------------------
+ f
+(1 row)
+
+-- true for new oid
+SELECT 'stats_test_idx1'::regclass::oid AS stats_test_idx1_oid \gset
+SELECT pg_stat_have_stats('relation', :dboid, :stats_test_idx1_oid);
+ pg_stat_have_stats 
+--------------------
+ t
+(1 row)
+
+-- pg_stat_have_stats returns true for a rolled back drop index with stats
+BEGIN;
+SELECT pg_stat_have_stats('relation', :dboid, :stats_test_idx1_oid);
+ pg_stat_have_stats 
+--------------------
+ t
+(1 row)
+
+DROP index stats_test_idx1;
+ROLLBACK;
+SELECT pg_stat_have_stats('relation', :dboid, :stats_test_idx1_oid);
+ pg_stat_have_stats 
+--------------------
+ t
+(1 row)
+
+-- put enable_seqscan back to on
+SET enable_seqscan TO on;
 -- ensure that stats accessors handle NULL input correctly
 SELECT pg_stat_get_replication_slot(NULL);
  pg_stat_get_replication_slot 
diff --git a/src/test/regress/sql/stats.sql b/src/test/regress/sql/stats.sql
index 096f00ce8b..a63bde9e67 100644
--- a/src/test/regress/sql/stats.sql
+++ b/src/test/regress/sql/stats.sql
@@ -16,6 +16,9 @@ SET enable_indexonlyscan TO off;
 -- not enabled by default, but we want to test it...
 SET track_functions TO 'all';
 
+-- record dboid for later use
+SELECT oid AS dboid from pg_database where datname = current_database() \gset
+
 -- save counters
 BEGIN;
 SET LOCAL stats_fetch_consistency = snapshot;
@@ -387,9 +390,52 @@ SELECT pg_stat_have_stats('bgwriter', 0, 0);
 -- unknown stats kinds error out
 SELECT pg_stat_have_stats('zaphod', 0, 0);
 -- db stats have objoid 0
-SELECT pg_stat_have_stats('database', (SELECT oid FROM pg_database WHERE datname = current_database()), 1);
-SELECT pg_stat_have_stats('database', (SELECT oid FROM pg_database WHERE datname = current_database()), 0);
+SELECT pg_stat_have_stats('database', :dboid, 1);
+SELECT pg_stat_have_stats('database', :dboid, 0);
+
+-- pg_stat_have_stats returns true for committed index creation
+CREATE table stats_test_tab1 as select generate_series(1,10) a;
+CREATE index stats_test_idx1 on stats_test_tab1(a);
+SELECT 'stats_test_idx1'::regclass::oid AS stats_test_idx1_oid \gset
+SET enable_seqscan TO off;
+select a from stats_test_tab1 where a = 3;
+SELECT pg_stat_have_stats('relation', :dboid, :stats_test_idx1_oid);
+
+-- pg_stat_have_stats returns false for dropped index with stats
+SELECT pg_stat_have_stats('relation', :dboid, :stats_test_idx1_oid);
+DROP index stats_test_idx1;
+SELECT pg_stat_have_stats('relation', :dboid, :stats_test_idx1_oid);
+
+-- pg_stat_have_stats returns false for rolled back index creation
+BEGIN;
+CREATE index stats_test_idx1 on stats_test_tab1(a);
+SELECT 'stats_test_idx1'::regclass::oid AS stats_test_idx1_oid \gset
+select a from stats_test_tab1 where a = 3;
+SELECT pg_stat_have_stats('relation', :dboid, :stats_test_idx1_oid);
+ROLLBACK;
+SELECT pg_stat_have_stats('relation', :dboid, :stats_test_idx1_oid);
+
+-- pg_stat_have_stats returns true for reindex CONCURRENTLY
+CREATE index stats_test_idx1 on stats_test_tab1(a);
+SELECT 'stats_test_idx1'::regclass::oid AS stats_test_idx1_oid \gset
+select a from stats_test_tab1 where a = 3;
+SELECT pg_stat_have_stats('relation', :dboid, :stats_test_idx1_oid);
+REINDEX index CONCURRENTLY stats_test_idx1;
+-- false for previous oid
+SELECT pg_stat_have_stats('relation', :dboid, :stats_test_idx1_oid);
+-- true for new oid
+SELECT 'stats_test_idx1'::regclass::oid AS stats_test_idx1_oid \gset
+SELECT pg_stat_have_stats('relation', :dboid, :stats_test_idx1_oid);
+
+-- pg_stat_have_stats returns true for a rolled back drop index with stats
+BEGIN;
+SELECT pg_stat_have_stats('relation', :dboid, :stats_test_idx1_oid);
+DROP index stats_test_idx1;
+ROLLBACK;
+SELECT pg_stat_have_stats('relation', :dboid, :stats_test_idx1_oid);
 
+-- put enable_seqscan back to on
+SET enable_seqscan TO on;
 
 -- ensure that stats accessors handle NULL input correctly
 SELECT pg_stat_get_replication_slot(NULL);

#10

Andres Freund

andres@anarazel.de

over 3 years ago

In reply to: Drouvot, Bertrand (#9)

Re: pg_stat_have_stats() returns true for dropped indexes (or for index creation transaction rolled back)

Hi,

On 2022-09-01 08:40:54 +0200, Drouvot, Bertrand wrote:

Thanks for the suggestion, I'm coming up with this proposal in v4 attached:

I pushed the bugfix / related test portion to 15, master. Thanks!

I left the replication stuff out - it seemed somewhat independent. Probably
will just push that to master, unless somebody thinks it should be in both?

Greetings,

Andres Freund

#11

Drouvot, Bertrand

bertranddrouvot.pg@gmail.com

over 3 years ago

In reply to: Andres Freund (#10)

Re: pg_stat_have_stats() returns true for dropped indexes (or for index creation transaction rolled back)

Hi,

On 9/23/22 10:45 PM, Andres Freund wrote:

Hi,

On 2022-09-01 08:40:54 +0200, Drouvot, Bertrand wrote:

Thanks for the suggestion, I'm coming up with this proposal in v4 attached:

I pushed the bugfix / related test portion to 15, master. Thanks!

Thanks!

I left the replication stuff out - it seemed somewhat independent.

Yeah.

Probably
will just push that to master, unless somebody thinks it should be in both?

Sounds good to me as this is not a bug and that seems unlikely to me
that an issue in this area will be introduced later on on 15 without
being introduced on master too.

Regards,

--
Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com

#12

Drouvot, Bertrand

bertranddrouvot.pg@gmail.com

over 3 years ago

In reply to: Drouvot, Bertrand (#11)

Re: pg_stat_have_stats() returns true for dropped indexes (or for index creation transaction rolled back)

Hi,

On 9/26/22 10:23 AM, Drouvot, Bertrand wrote:

Hi,

On 9/23/22 10:45 PM, Andres Freund wrote:

Hi,

On 2022-09-01 08:40:54 +0200, Drouvot, Bertrand wrote:

Thanks for the suggestion, I'm coming up with this proposal in v4
attached:

I pushed the bugfix / related test portion to 15, master. Thanks!

Thanks!

Forgot to say that with that being fixed, I'll come back with a patch
proposal for the tables/indexes stats split (discovered the issue fixed
in this current thread while working on the split patch.)

Regards,

--
Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com