Here are my initial review comments for the first patch v20240625-0001.
======
General
1. Missing docs?
Section 9.17. "Sequence Manipulation Functions" [1] describes some
functions. Shouldn't your new function be documented here also?
~~~
2. Missing tests?
Shouldn't there be some test code that at least executes your new
pg_sequence_state function to verify that sane values are returned?
======
Commit Message
3.
This patch introduces new functionalities to PostgreSQL:
- pg_sequence_state allows retrieval of sequence values using LSN.
- SetSequence enables updating sequences with user-specified values.
~
3a.
I didn't understand why this says "using LSN" because IIUC 'lsn' is an
output parameter of that function. Don't you mean "... retrieval of
sequence values including LSN"?
~
3b.
Does "user-specified" make sense? Is this going to be exposed to a
user? How about just "specified"?
======
src/backend/commands/sequence.c
4. SetSequence:
+void
+SetSequence(Oid seq_relid, int64 value)
Would 'new_last_value' be a better parameter name here?
~~~
5.
This new function logic looks pretty similar to the do_setval()
function. Can you explain (maybe in the function comment) some info
about how and why it differs from that other function?
~~~
6.
I saw that RelationNeedsWAL() is called 2 times. It may make no sense,
but is it possible to assign that to a variable 1st time so you don't
need to call it 2nd time within the critical section?
~~~
NITPICK - remove junk (') char in comment
NITPICK - missing periods (.) in multi-sentence comment
~~~
7.
-read_seq_tuple(Relation rel, Buffer *buf, HeapTuple seqdatatuple)
+read_seq_tuple(Relation rel, Buffer *buf, HeapTuple seqdatatuple,
+ XLogRecPtr *lsn)
7a.
The existing parameters were described in the function comment. So,
the new 'lsn' parameter should be described here also.
~
7b.
Maybe the new parameter name should be 'lsn_res' or 'lsn_out' or
similar to emphasise that this is a returned value.
~~
NITPICK - tweaked comment. YMMV.
~~~
8. pg_sequence_state:
Should you give descriptions of the output parameters in the function
header comment? Otherwise, where are they described so called knows
what they mean?
~~~
NITPICK - /relid/seq_relid/
NITPICK - declare the variables in the same order as the output parameters
NITPICK - An alternative to the memset for nulls is just to use static
initialisation
"bool nulls[4] = {false, false, false, false};"
======
+extern void SetSequence(Oid seq_relid, int64 value);
9.
Would 'SetSequenceLastValue' be a better name for what this function is doing?
======
99.
See also my attached diff which is a top-up patch implementing those
nitpicks mentioned above. Please apply any of these that you agree
with.
======
[1] https://www.postgresql.org/docs/devel/functions-sequence.html
Kind Regards,
Peter Smith.
Fujitsu Australia
diff --git a/src/backend/commands/sequence.c b/src/backend/commands/sequence.c
index 57453a7..9bad121 100644
--- a/src/backend/commands/sequence.c
+++ b/src/backend/commands/sequence.c
@@ -349,7 +349,7 @@ SetSequence(Oid seq_relid, int64 value)
/* open and lock sequence */
init_sequence(seq_relid, &elm, &seqrel);
- /* lock page' buffer and read tuple */
+ /* lock page buffer and read tuple */
seq = read_seq_tuple(seqrel, &buf, &seqdatatuple, NULL);
/* check the comment above nextval_internal()'s equivalent call. */
@@ -397,8 +397,10 @@ SetSequence(Oid seq_relid, int64 value)
UnlockReleaseBuffer(buf);
- /* Clear local cache so that we don't think we have cached numbers */
- /* Note that we do not change the currval() state */
+ /*
+ * Clear local cache so that we don't think we have cached numbers.
+ * Note that we do not change the currval() state.
+ */
elm->cached = elm->last;
relation_close(seqrel, NoLock);
@@ -1275,8 +1277,9 @@ read_seq_tuple(Relation rel, Buffer *buf, HeapTuple
seqdatatuple,
RelationGetRelationName(rel), sm->magic);
/*
- * If the caller requested it, set the page LSN. This allows deciding
- * which sequence changes are before/after the returned sequence state.
+ * If the caller requested it, return the page LSN. This allows the
+ * caller to determine which sequence changes are before/after the
+ * returned sequence state.
*/
if (lsn)
*lsn = PageGetLSN(page);
@@ -1912,7 +1915,7 @@ pg_sequence_last_value(PG_FUNCTION_ARGS)
Datum
pg_sequence_state(PG_FUNCTION_ARGS)
{
- Oid relid = PG_GETARG_OID(0);
+ Oid seq_relid = PG_GETARG_OID(0);
SeqTable elm;
Relation seqrel;
Buffer buf;
@@ -1920,21 +1923,21 @@ pg_sequence_state(PG_FUNCTION_ARGS)
Form_pg_sequence_data seq;
Datum result;
+ XLogRecPtr lsn;
int64 last_value;
int64 log_cnt;
bool is_called;
- XLogRecPtr lsn;
TupleDesc tupdesc;
HeapTuple tuple;
Datum values[4];
- bool nulls[4];
+ bool nulls[4] = {false, false, false, false};
if (get_call_result_type(fcinfo, NULL, &tupdesc) != TYPEFUNC_COMPOSITE)
elog(ERROR, "return type must be a row type");
/* open and lock sequence */
- init_sequence(relid, &elm, &seqrel);
+ init_sequence(seq_relid, &elm, &seqrel);
if (pg_class_aclcheck(elm->relid, GetUserId(),
ACL_SELECT | ACL_USAGE) !=
ACLCHECK_OK)
@@ -1957,8 +1960,6 @@ pg_sequence_state(PG_FUNCTION_ARGS)
values[2] = Int64GetDatum(log_cnt);
values[3] = BoolGetDatum(is_called);
- memset(nulls, 0, sizeof(nulls));
-
tuple = heap_form_tuple(tupdesc, values, nulls);
result = HeapTupleGetDatum(tuple);
