Removing log_cnt from pg_sequence_read_tuple()
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
Attachments:
seq-read-tuple-logcnt.patchtext/x-diff; charset=us-asciiDownload
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;
On Mon, Aug 26, 2024 at 11:11:55AM +0900, Michael Paquier wrote:
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.
I am fine with changes to this function that would allow it to be
generically useful for all sequence AMs, provided that it can still be used
for dumpSequenceData(). I only included log_cnt because
pg_sequence_read_tuple() is intended to be a substitute for SELECT from the
sequence, but I'm not aware of any real use-case for that column besides
debugging, which you've already addressed. Even if we remove log_cnt, you
can still find it with SELECT, too.
The patch looks reasonable to me. Do you think the name of the function
still makes sense now that 1) we might have different sequence AMs in the
near future and 2) it no longer returns everything in the sequence tuple?
--
nathan
On Mon, Aug 26, 2024 at 09:19:06AM -0500, Nathan Bossart wrote:
I am fine with changes to this function that would allow it to be
generically useful for all sequence AMs, provided that it can still be used
for dumpSequenceData(). I only included log_cnt because
pg_sequence_read_tuple() is intended to be a substitute for SELECT from the
sequence, but I'm not aware of any real use-case for that column besides
debugging, which you've already addressed.
Okay, thanks.
Even if we remove log_cnt, you can still find it with SELECT, too.
The design used in the sequence AM patch makes it possible to assign
custom attributes to the sequence "relation" used for storage, with a
table AM used underneath that may not be heap. The AM callback
plugged into the path used by pg_sequence_read_tuple() (previous
version for pg_sequence_last_value) returns the pair is_called and
last_value, to map to the row of the function used to rebuild the
commands in dumps and upgrades.
The patch looks reasonable to me. Do you think the name of the function
still makes sense now that 1) we might have different sequence AMs in the
near future and 2) it no longer returns everything in the sequence tuple?
Indeed, pg_sequence_read_tuple() would not reflect the reality, some
ideas:
- pg_sequence_read_data
- pg_sequence_get_data
- pg_sequence_data
- More consistent with other catalog functions: pg_get_sequence_data,
as we have already in the tree a lot of pg_get_* functions.
--
Michael
On Thu, Aug 29, 2024 at 08:00:52AM +0900, Michael Paquier wrote:
On Mon, Aug 26, 2024 at 09:19:06AM -0500, Nathan Bossart wrote:
The patch looks reasonable to me. Do you think the name of the function
still makes sense now that 1) we might have different sequence AMs in the
near future and 2) it no longer returns everything in the sequence tuple?Indeed, pg_sequence_read_tuple() would not reflect the reality, some
ideas:
- pg_sequence_read_data
- pg_sequence_get_data
- pg_sequence_data
- More consistent with other catalog functions: pg_get_sequence_data,
as we have already in the tree a lot of pg_get_* functions.
pg_get_sequence_data() sounds fine to me.
--
nathan
On Wed, Aug 28, 2024 at 08:28:03PM -0500, Nathan Bossart wrote:
On Thu, Aug 29, 2024 at 08:00:52AM +0900, Michael Paquier wrote:
Indeed, pg_sequence_read_tuple() would not reflect the reality, some
ideas:
- pg_sequence_read_data
- pg_sequence_get_data
- pg_sequence_data
- More consistent with other catalog functions: pg_get_sequence_data,
as we have already in the tree a lot of pg_get_* functions.pg_get_sequence_data() sounds fine to me.
Okay, here is a v2 of the patch using this name for the function.
--
Michael
Attachments:
seq-read-tuple-logcnt-v2.patchtext/x-diff; charset=us-asciiDownload
diff --git a/src/include/catalog/pg_proc.dat b/src/include/catalog/pg_proc.dat
index 4abc6d9526..85f42be1b3 100644
--- a/src/include/catalog/pg_proc.dat
+++ b/src/include/catalog/pg_proc.dat
@@ -3330,11 +3330,11 @@
prorettype => 'int8', proargtypes => 'regclass',
prosrc => 'pg_sequence_last_value' },
{ oid => '9876', descr => 'return sequence tuple, for use by pg_dump',
- proname => 'pg_sequence_read_tuple', provolatile => 'v', proparallel => 'u',
+ proname => 'pg_get_sequence_data', 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}',
- prosrc => 'pg_sequence_read_tuple' },
+ proallargtypes => '{regclass,int8,bool}', proargmodes => '{i,o,o}',
+ proargnames => '{sequence_oid,last_value,is_called}',
+ prosrc => 'pg_get_sequence_data' },
{ oid => '275', descr => 'return the next oid for a system table',
proname => 'pg_nextoid', provolatile => 'v', proparallel => 'u',
diff --git a/src/backend/commands/sequence.c b/src/backend/commands/sequence.c
index 8c1131f020..5e41088d01 100644
--- a/src/backend/commands/sequence.c
+++ b/src/backend/commands/sequence.c
@@ -1781,23 +1781,22 @@ pg_sequence_parameters(PG_FUNCTION_ARGS)
* without needing to individually query each sequence relation.
*/
Datum
-pg_sequence_read_tuple(PG_FUNCTION_ARGS)
+pg_get_sequence_data(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/bin/pg_dump/pg_dump.c b/src/bin/pg_dump/pg_dump.c
index b6e01d3d29..f7720ad53b 100644
--- a/src/bin/pg_dump/pg_dump.c
+++ b/src/bin/pg_dump/pg_dump.c
@@ -17381,7 +17381,7 @@ collectSequences(Archive *fout)
* versions, but for now it seems unlikely to be worth it.
*
* Since version 18, we can gather the sequence data in this query with
- * pg_sequence_read_tuple(), but we only do so for non-schema-only dumps.
+ * pg_get_sequence_data(), but we only do so for non-schema-only dumps.
*/
if (fout->remoteVersion < 100000)
return;
@@ -17401,7 +17401,7 @@ collectSequences(Archive *fout)
"seqcache, seqcycle, "
"last_value, is_called "
"FROM pg_catalog.pg_sequence, "
- "pg_sequence_read_tuple(seqrelid) "
+ "pg_get_sequence_data(seqrelid) "
"ORDER BY seqrelid;";
res = ExecuteSqlQuery(fout, query, PGRES_TUPLES_OK);
diff --git a/src/test/regress/expected/sequence.out b/src/test/regress/expected/sequence.out
index e749c4574e..15925d99c8 100644
--- a/src/test/regress/expected/sequence.out
+++ b/src/test/regress/expected/sequence.out
@@ -839,11 +839,11 @@ SELECT nextval('test_seq1');
3
(1 row)
--- pg_sequence_read_tuple
-SELECT * FROM pg_sequence_read_tuple('test_seq1');
- last_value | log_cnt | is_called
-------------+---------+-----------
- 10 | 32 | t
+-- pg_get_sequence_data
+SELECT * FROM pg_get_sequence_data('test_seq1');
+ last_value | is_called
+------------+-----------
+ 10 | t
(1 row)
DROP SEQUENCE test_seq1;
diff --git a/src/test/regress/sql/sequence.sql b/src/test/regress/sql/sequence.sql
index ea447938ae..2c220b6074 100644
--- a/src/test/regress/sql/sequence.sql
+++ b/src/test/regress/sql/sequence.sql
@@ -413,7 +413,7 @@ SELECT nextval('test_seq1');
SELECT nextval('test_seq1');
SELECT nextval('test_seq1');
--- pg_sequence_read_tuple
-SELECT * FROM pg_sequence_read_tuple('test_seq1');
+-- pg_get_sequence_data
+SELECT * FROM pg_get_sequence_data('test_seq1');
DROP SEQUENCE test_seq1;
On Thu, Aug 29, 2024 at 02:11:22PM +0900, Michael Paquier wrote:
Okay, here is a v2 of the patch using this name for the function.
LGTM
--
nathan
On Thu, Aug 29, 2024 at 09:28:49AM -0500, Nathan Bossart wrote:
On Thu, Aug 29, 2024 at 02:11:22PM +0900, Michael Paquier wrote:
Okay, here is a v2 of the patch using this name for the function.
LGTM
Thanks, applied that, after one tweak for the #define name.
--
Michael