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
