Subject: Removing log_cnt from pg_sequence_read_tuple()

From: Michael Paquier <[email-masked]>
Date: Mon, 26 Aug 2024 11:11:55 +0900

Hi all,
(Nathan in CC regarding c8b06bb969bf)

While rebasing my patch set for sequence AMs, I've looked at what has
been done with c8b06bb969bf and pg_sequence_read_tuple() because I can
reuse it in the sequence AM patch to grab the last value of a sequence
and if it has been called (the patch set implemented the same thing,
with a different function name), and got surprised that the function
also returns log_cnt, which is for the in-core sequence metadata an
internal counter to decide when a sequence should be WAL-logged.

Why do we need this field at all in this function?  pg_dump only cares
about the last value and is_called to be able to rebuilt its sequence
DDLs, and log_cnt is reset each time we restore or do a crash
recovery, bumping at the next sequence value based on an internal of
32.

I am also a bit dubious about the value it adds for debugging.  The
thing is that depending on the way sequences are computed, we may not
care about WAL at all, and log_cnt is something related to the way
in-core sequences are computed and how its data is persistent.  So
this makes the whole concept of sequence metadata a bit fuzzier
because we mix data necessary for the sequence command and more
things.  There is no need for it in pg_dump or pg_upgrade, either.

last_value and is_called are different and required all the time,
of course, because these define how the sequence DDLs should be
created.

It seems to me that we'd live better without it, at least it matters
for the sequence AM patch, because some of its callbacks are also
shaped around the fact that WAL may not be required for sequence value
computations.  Providing a function that should be rather generic does
not fit well in this context, still I agree that it comes down to how
the callbacks are defined, of course.  My point is that the use of WAL
is something that should be optional, but log_cnt in this function
makes it a mandatory concept that all sequence AMs would need to deal
with.

Comments?
--
Michael
diff --git a/src/include/catalog/pg_proc.dat b/src/include/catalog/pg_proc.dat
index 4abc6d9526..986df08c6e 100644
--- a/src/include/catalog/pg_proc.dat
+++ b/src/include/catalog/pg_proc.dat
@@ -3332,8 +3332,8 @@
 { oid => '9876', descr => 'return sequence tuple, for use by pg_dump',
   proname => 'pg_sequence_read_tuple', provolatile => 'v', proparallel => 'u',
   prorettype => 'record', proargtypes => 'regclass',
-  proallargtypes => '{regclass,int8,int8,bool}', proargmodes => '{i,o,o,o}',
-  proargnames => '{sequence_oid,last_value,log_cnt,is_called}',
+  proallargtypes => '{regclass,int8,bool}', proargmodes => '{i,o,o}',
+  proargnames => '{sequence_oid,last_value,is_called}',
   prosrc => 'pg_sequence_read_tuple' },
 
 { oid => '275', descr => 'return the next oid for a system table',
diff --git a/src/backend/commands/sequence.c b/src/backend/commands/sequence.c
index 8c1131f020..11000b97bd 100644
--- a/src/backend/commands/sequence.c
+++ b/src/backend/commands/sequence.c
@@ -1783,21 +1783,20 @@ pg_sequence_parameters(PG_FUNCTION_ARGS)
 Datum
 pg_sequence_read_tuple(PG_FUNCTION_ARGS)
 {
+#define SEQUENCE_READ_TUPLE_COLS	2
 	Oid			relid = PG_GETARG_OID(0);
 	SeqTable	elm;
 	Relation	seqrel;
-	Datum		values[SEQ_COL_LASTCOL] = {0};
-	bool		isnull[SEQ_COL_LASTCOL] = {0};
+	Datum		values[SEQUENCE_READ_TUPLE_COLS] = {0};
+	bool		isnull[SEQUENCE_READ_TUPLE_COLS] = {0};
 	TupleDesc	resultTupleDesc;
 	HeapTuple	resultHeapTuple;
 	Datum		result;
 
-	resultTupleDesc = CreateTemplateTupleDesc(SEQ_COL_LASTCOL);
+	resultTupleDesc = CreateTemplateTupleDesc(SEQUENCE_READ_TUPLE_COLS);
 	TupleDescInitEntry(resultTupleDesc, (AttrNumber) 1, "last_value",
 					   INT8OID, -1, 0);
-	TupleDescInitEntry(resultTupleDesc, (AttrNumber) 2, "log_cnt",
-					   INT8OID, -1, 0);
-	TupleDescInitEntry(resultTupleDesc, (AttrNumber) 3, "is_called",
+	TupleDescInitEntry(resultTupleDesc, (AttrNumber) 2, "is_called",
 					   BOOLOID, -1, 0);
 	resultTupleDesc = BlessTupleDesc(resultTupleDesc);
 
@@ -1818,8 +1817,7 @@ pg_sequence_read_tuple(PG_FUNCTION_ARGS)
 		seq = read_seq_tuple(seqrel, &buf, &seqtuple);
 
 		values[0] = Int64GetDatum(seq->last_value);
-		values[1] = Int64GetDatum(seq->log_cnt);
-		values[2] = BoolGetDatum(seq->is_called);
+		values[1] = BoolGetDatum(seq->is_called);
 
 		UnlockReleaseBuffer(buf);
 	}
@@ -1831,6 +1829,7 @@ pg_sequence_read_tuple(PG_FUNCTION_ARGS)
 	resultHeapTuple = heap_form_tuple(resultTupleDesc, values, isnull);
 	result = HeapTupleGetDatum(resultHeapTuple);
 	PG_RETURN_DATUM(result);
+#undef SEQUENCE_READ_TUPLE_COLS
 }

diff --git a/src/test/regress/expected/sequence.out b/src/test/regress/expected/sequence.out
index e749c4574e..6be5406ae9 100644
--- a/src/test/regress/expected/sequence.out
+++ b/src/test/regress/expected/sequence.out
@@ -841,9 +841,9 @@ SELECT nextval('test_seq1');
 
 -- pg_sequence_read_tuple
 SELECT * FROM pg_sequence_read_tuple('test_seq1');
- last_value | log_cnt | is_called 
-------------+---------+-----------
-         10 |      32 | t
+ last_value | is_called 
+------------+-----------
+         10 | t
 (1 row)
 
 DROP SEQUENCE test_seq1;