Re: Logical Replication of sequences

Hi Vignesh,

No comments for patch v20250416-0001
No comments for patch v20250416-0002
No comments for patch v20250416-0003

Here are some comments for patch v20250416-0004

======
src/backend/catalog/system_views.sql

1.
+CREATE VIEW pg_publication_sequences AS
+ SELECT
+ P.pubname AS pubname,
+ N.nspname AS schemaname,
+ C.relname AS sequencename
+ FROM pg_publication P,
+ LATERAL pg_get_publication_sequences(P.pubname) GPS,
+ pg_class C JOIN pg_namespace N ON (N.oid = C.relnamespace)
+ WHERE C.oid = GPS.relid;
+

Should we have some regression tests for this view?

SUGGESTION
test_pub=# CREATE SEQUENCE S1;
test_pub=# CREATE SEQUENCE S2;
test_pub=# CREATE PUBLICATION PUB1 FOR ALL SEQUENCES;
test_pub=# SELECT * FROM pg_publication_sequences;
pubname | schemaname | sequencename
---------+------------+--------------
pub1 | public | s1
pub1 | public | s2
(2 rows)

======
.../replication/logical/sequencesync.c

copy_sequence:

2.
+ res = walrcv_exec(conn, cmd.data,
+ lengthof(seqRow), seqRow);

Unnecessary wrapping.

~~~

Vignesh 16/4 answered my previous review comment #16
In the caller we set the sequence lsn only if sequence_mismatch is
false, so there is no issue.

PS REPLY 17/4. No, I don’t see that. I think
fetch_remote_sequesnce_data is unconditionally assigning to the
*page_lsn output parameter (aka seq_page_lsn). Anyway, it does not
matter anymore since the return from copy_sequence function is now
fixed.

~~~

3.
+ /* Update the sequence only if the parameters are identical. */
+ if (!*sequence_mismatch)
+ SetSequence(RelationGetRelid(rel), seq_last_value, seq_is_called,
+ seq_log_cnt);
+
+ /* Return the LSN when the sequence state was set. */
+ return *sequence_mismatch ? InvalidXLogRecPtr : seq_page_lsn;

It might be simpler to have a single condition instead checking
*sequence_mismatch twice.

SUGGESTION
/* Update the sequence only if the parameters are identical. */
if (*sequence_mismatch)
return InvalidXLogRecPtr;
else
{
SetSequence(RelationGetRelid(rel), seq_last_value, seq_is_called,
seq_log_cnt);
return seq_page_lsn;
}

~~~

LogicalRepSyncSequences:

Vignesh 16/4 answered my previous comment #19:
In this function we copy the sequences_not_synced sequences one by
one, while copying the sequence if there is an error like sequence
type or min or max etc don't match , sequence_mismatch will be set.
Later while copying another sequence if an exception is raised and we
reach catch block, we report an error this case.

PS REPLY 17/4. I didn’t understand your explanation. I think anything
that causes sequence_mismatch to be assigned true is just an internal
logic state. It is not something that will be “thrown” and caught by
the PG_CATCH. Therefore, I did not understand why the “if
(sequence_mismatch)” needed to be within the PG_CATCH block.

~~~

Vignesh 16/4 answered my previous review comment #21:
I felt repeated is correct here as we don't want to repeatedly start
the sequence sync worker after every failure.

PS REPLY 17/4
Hm. Is that correct? AFAIK we still will "repeatedly" start the
sequence syn worker after a failure. I think the failure only *slows
down* the respawn of the worker because it will use the
TimestampDifferenceExceeds check if there had been a failure. That's
why I suggested s/to prevent repeated initiation/to prevent immediate
initiation/.

======
src/bin/pg_dump/pg_dump.c

getSubscriptionRelations:

Vignesh 16/4 answered my previous review comment #25:
You are talking about the error message right, I have changed that.

PS REPLY 17/4
Yes, the error message, but also I thought 'tblinfo' var and
FindTableByOid function name should refer to relations instead of
tables?

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