Date: Tue, 3 Sep 2024 14:52:50 -0500
From: Nathan Bossart <nathandbossart@gmail.com>
To: Michael Paquier <michael@paquier.xyz>
Cc: Robert Haas <robertmhaas@gmail.com>,
	Ayush Vatsa <ayushvatsa1810@gmail.com>,
	PostgreSQL Hackers <pgsql-hackers@postgresql.org>
Subject: Re: Pgstattuple on Sequences: Seeking Community Feedback on
 Potential Patch
Message-ID: <ZtdpEtY2MSB4N6iN@nathan>
References: 
 <CACX+KaP3i+i9tdPLjF5JCHVv93xobEdcd_eB+638VDvZ3i=cQA@mail.gmail.com>
 <Zsy6w_yW2QKYSo_m@nathan>
 <CA+TgmoYm0X=KuHry1A6prNzf3pUmAcXYgQEr25kCY8jSMJaVjg@mail.gmail.com>
 <ZszGBMK1D4hCsRnD@nathan>
 <CACX+KaNhg-ZwkqCSjsq7j_gRYDmHip2b4kwa6ohac0MspSeW9w@mail.gmail.com>
 <ZtCxo95K_bqGbmNQ@nathan>
 <CA+TgmoaHX9BYrr1u9c356ovGwVBy25Q9y_LdSYS8JW0pqd18JA@mail.gmail.com>
 <ZtI0O-bN90P02oQO@nathan>
 <ZtT8cfqTHfPGFRqJ@paquier.xyz>
MIME-Version: 1.0
Content-Type: multipart/mixed; boundary="eO/ZuwaqUo8v51O9"
Content-Disposition: inline
In-Reply-To: <ZtT8cfqTHfPGFRqJ@paquier.xyz>
Archived-At: <https://www.postgresql.org/message-id/ZtdpEtY2MSB4N6iN%40nathan>
Precedence: bulk


--eO/ZuwaqUo8v51O9
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline

Here is roughly what I had in mind to commit, but I'm not sure there's a
consensus on doing this.

-- 
nathan

--eO/ZuwaqUo8v51O9
Content-Type: text/plain; charset=us-ascii
Content-Disposition: attachment;
	filename="v3-0001-Reintroduce-support-for-sequences-in-pgstattuple-.patch"

From 97e670e2e0b53a9dc91c5932ccb34bb1fd7eae6b Mon Sep 17 00:00:00 2001
From: Nathan Bossart <nathan@postgresql.org>
Date: Tue, 3 Sep 2024 14:49:24 -0500
Subject: [PATCH v3 1/1] Reintroduce support for sequences in pgstattuple and
 pageinspect.

Commit 4b82664156 restricted a number of functions provided by
contrib modules to only relations that use the "heap" table access
method.  Sequences always use this table access method, but they do
not advertise as such in the pg_class system catalog, so the
aforementioned commit also (presumably unintentionally) removed
support for sequences from these functions.  This commit
reintroduces said support for sequences and adds a couple of
relevant tests.

Co-authored-by: Ayush Vatsa
Reviewed-by: FIXME
Discussion: https://postgr.es/m/CACX%2BKaP3i%2Bi9tdPLjF5JCHVv93xobEdcd_eB%2B638VDvZ3i%3DcQA%40mail.gmail.com
Backpatch-through: 12
---
 contrib/pageinspect/expected/page.out        |  9 +++++++
 contrib/pageinspect/heapfuncs.c              |  6 ++++-
 contrib/pageinspect/sql/page.sql             |  5 ++++
 contrib/pgstattuple/expected/pgstattuple.out | 25 ++++++++++++++++++++
 contrib/pgstattuple/pgstattuple.c            |  6 ++++-
 contrib/pgstattuple/sql/pgstattuple.sql      | 12 ++++++++++
 6 files changed, 61 insertions(+), 2 deletions(-)

diff --git a/contrib/pageinspect/expected/page.out b/contrib/pageinspect/expected/page.out
index 80ddb45a60..04fd9dee4b 100644
--- a/contrib/pageinspect/expected/page.out
+++ b/contrib/pageinspect/expected/page.out
@@ -239,3 +239,12 @@ SELECT page_checksum(decode(repeat('00', :block_size), 'hex'), 1);
               
 (1 row)
 
+-- tests for sequences
+create temporary sequence test_sequence;
+select tuple_data_split('test_sequence'::regclass, t_data, t_infomask, t_infomask2, t_bits)
+  from heap_page_items(get_raw_page('test_sequence', 0));
+                   tuple_data_split                    
+-------------------------------------------------------
+ {"\\x0100000000000000","\\x0000000000000000","\\x00"}
+(1 row)
+
diff --git a/contrib/pageinspect/heapfuncs.c b/contrib/pageinspect/heapfuncs.c
index 3faeabc711..38a539dad1 100644
--- a/contrib/pageinspect/heapfuncs.c
+++ b/contrib/pageinspect/heapfuncs.c
@@ -320,7 +320,11 @@ tuple_data_split_internal(Oid relid, char *tupdata,
 	raw_attrs = initArrayResult(BYTEAOID, CurrentMemoryContext, false);
 	nattrs = tupdesc->natts;
 
-	if (rel->rd_rel->relam != HEAP_TABLE_AM_OID)
+	/*
+	 * Sequences always use heap AM, but they don't show that in the catalogs.
+	 */
+	if (rel->rd_rel->relkind != RELKIND_SEQUENCE &&
+		rel->rd_rel->relam != HEAP_TABLE_AM_OID)
 		ereport(ERROR, (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
 						errmsg("only heap AM is supported")));
 
diff --git a/contrib/pageinspect/sql/page.sql b/contrib/pageinspect/sql/page.sql
index 5bff568d3b..59784fc7cc 100644
--- a/contrib/pageinspect/sql/page.sql
+++ b/contrib/pageinspect/sql/page.sql
@@ -98,3 +98,8 @@ SHOW block_size \gset
 SELECT fsm_page_contents(decode(repeat('00', :block_size), 'hex'));
 SELECT page_header(decode(repeat('00', :block_size), 'hex'));
 SELECT page_checksum(decode(repeat('00', :block_size), 'hex'), 1);
+
+-- tests for sequences
+create temporary sequence test_sequence;
+select tuple_data_split('test_sequence'::regclass, t_data, t_infomask, t_infomask2, t_bits)
+  from heap_page_items(get_raw_page('test_sequence', 0));
diff --git a/contrib/pgstattuple/expected/pgstattuple.out b/contrib/pgstattuple/expected/pgstattuple.out
index 283856e109..9176dc98b6 100644
--- a/contrib/pgstattuple/expected/pgstattuple.out
+++ b/contrib/pgstattuple/expected/pgstattuple.out
@@ -273,6 +273,31 @@ select pgstathashindex('test_partition_hash_idx');
  (4,8,0,1,0,0,0,100)
 (1 row)
 
+-- these should work for sequences
+create sequence test_sequence;
+select count(*) from pgstattuple('test_sequence');
+ count 
+-------
+     1
+(1 row)
+
+select pg_relpages('test_sequence');
+ pg_relpages 
+-------------
+           1
+(1 row)
+
+-- these should fail for sequences
+select pgstatindex('test_sequence');
+ERROR:  relation "test_sequence" is not a btree index
+select pgstatginindex('test_sequence');
+ERROR:  relation "test_sequence" is not a GIN index
+select pgstathashindex('test_sequence');
+ERROR:  relation "test_sequence" is not a hash index
+select pgstattuple_approx('test_sequence');
+ERROR:  relation "test_sequence" is of wrong relation kind
+DETAIL:  This operation is not supported for sequences.
+drop sequence test_sequence;
 drop table test_partitioned;
 drop view test_view;
 drop foreign table test_foreign_table;
diff --git a/contrib/pgstattuple/pgstattuple.c b/contrib/pgstattuple/pgstattuple.c
index 3bd8b96197..1d012233cb 100644
--- a/contrib/pgstattuple/pgstattuple.c
+++ b/contrib/pgstattuple/pgstattuple.c
@@ -323,7 +323,11 @@ pgstat_heap(Relation rel, FunctionCallInfo fcinfo)
 	pgstattuple_type stat = {0};
 	SnapshotData SnapshotDirty;
 
-	if (rel->rd_rel->relam != HEAP_TABLE_AM_OID)
+	/*
+	 * Sequences always use heap AM, but they don't show that in the catalogs.
+	 */
+	if (rel->rd_rel->relkind != RELKIND_SEQUENCE &&
+		rel->rd_rel->relam != HEAP_TABLE_AM_OID)
 		ereport(ERROR,
 				(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
 				 errmsg("only heap AM is supported")));
diff --git a/contrib/pgstattuple/sql/pgstattuple.sql b/contrib/pgstattuple/sql/pgstattuple.sql
index b08c31c21b..7e72c567a0 100644
--- a/contrib/pgstattuple/sql/pgstattuple.sql
+++ b/contrib/pgstattuple/sql/pgstattuple.sql
@@ -119,6 +119,18 @@ create index test_partition_hash_idx on test_partition using hash (a);
 select pgstatindex('test_partition_idx');
 select pgstathashindex('test_partition_hash_idx');
 
+-- these should work for sequences
+create sequence test_sequence;
+select count(*) from pgstattuple('test_sequence');
+select pg_relpages('test_sequence');
+
+-- these should fail for sequences
+select pgstatindex('test_sequence');
+select pgstatginindex('test_sequence');
+select pgstathashindex('test_sequence');
+select pgstattuple_approx('test_sequence');
+
+drop sequence test_sequence;
 drop table test_partitioned;
 drop view test_view;
 drop foreign table test_foreign_table;
-- 
2.39.3 (Apple Git-146)


--eO/ZuwaqUo8v51O9--