Re: Logical Replication of sequences

[Peter Smith]

Hi Vignesh.

Here are some review comments for patch v20241211-0001.

======
src/backend/commands/sequence.c

pg_sequence_state:


1.
+ TupleDesc tupdesc;
+ HeapTuple tuple;
+ Datum values[4];
+ bool nulls[4] = {false, false, false, false};

SUGGESTION
bool nulls[4] = {0};

The above achieves the same thing, but more succinctly, and I think it
is a common enough pattern in the PG source code.

~~~

2.
+ seq = read_seq_tuple(seqrel, &buf, &seqtuple);
+ page = BufferGetPage(buf);
+ lsn = PageGetLSN(page);
+
+ last_value = seq->last_value;
+ log_cnt = seq->log_cnt;
+ is_called = seq->is_called;

Move the blank line, so the 'lsn' assignment will be grouped with the
other 3 assignments which are part of the return tuple.

~~~

3.
+ /* How many fetches remain before a new WAL record has to be written */
+ values[2] = Int64GetDatum(log_cnt);

Trivial change to use the same wording as the documentation.

/has to/must/

======

(The attached NITPICKS patch includes the above suggestions)

======
Kind Regards,
Peter Smith.
Fujitsu Australia

diff --git a/src/backend/commands/sequence.c b/src/backend/commands/sequence.c
index 8b6c34a..aff2c1a 100644
--- a/src/backend/commands/sequence.c
+++ b/src/backend/commands/sequence.c
@@ -1912,7 +1912,7 @@ pg_sequence_state(PG_FUNCTION_ARGS)
        TupleDesc       tupdesc;
        HeapTuple       tuple;
        Datum           values[4];
-       bool            nulls[4] = {false, false, false, false};
+       bool            nulls[4] = {0};
 
        if (get_call_result_type(fcinfo, NULL, &tupdesc) != TYPEFUNC_COMPOSITE)
                elog(ERROR, "return type must be a row type");
@@ -1929,8 +1929,8 @@ pg_sequence_state(PG_FUNCTION_ARGS)
 
        seq = read_seq_tuple(seqrel, &buf, &seqtuple);
        page = BufferGetPage(buf);
-       lsn = PageGetLSN(page);
 
+       lsn = PageGetLSN(page);
        last_value = seq->last_value;
        log_cnt = seq->log_cnt;
        is_called = seq->is_called;
@@ -1944,7 +1944,7 @@ pg_sequence_state(PG_FUNCTION_ARGS)
        /* The value most recently returned by nextval in the current session */
        values[1] = Int64GetDatum(last_value);
 
-       /* How many fetches remain before a new WAL record has to be written */
+       /* How many fetches remain before a new WAL record must be written */
        values[2] = Int64GetDatum(log_cnt);
 
        /* Indicates whether the sequence has been used */