Re: Logical Replication of sequences

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