Re: Logical Replication of sequences

On Tue, 15 Apr 2025 at 12:03, Peter Smith <smithpb2250@gmail.com> wrote:
>
> Hi Vignesh,
>
> Some review comments for v20250525-0004.
>
> ======
> Commit message
> ProcessSyncingSequencesForApply:
>
> 10.
> + if (!started_tx)
> + {
> + StartTransactionCommand();
> + started_tx = true;
> + }
> +
> + Assert(get_rel_relkind(rstate->relid) == RELKIND_SEQUENCE);
>
> Maybe the Assert should come 1st before the tx stuff?

The existing is correct, get_rel_kind call requires transaction to be started.
>
> 13.
> + seq_params_match = seqform->seqtypid == seqtypid &&
> + seqform->seqmin == seqmin && seqform->seqmax == seqmax &&
> + seqform->seqcycle == seqcycle &&
> + seqform->seqstart == seqstart &&
> + seqform->seqincrement == seqincrement;
>
> By the time the WARNING for this mismatch gets logged, the knowledge
> of *what* differed seems lost. Maybe it is not possible, but I
> wondered if it would make the warning much more useful if you could
> somehow also log attribute values. That will help the user understand
> what caused the clash in the first place. Otherwise they will have to
> go to the trouble to try to figure it out for themselves.

I felt it might be ok for users to get this using the sequence name.
>
> 16.
> + *sequence_mismatch = !fetch_remote_sequence_data(conn, relid, remoteid,
> + nspname, relname,
> + &seq_log_cnt, &seq_is_called,
> + &seq_page_lsn, &seq_last_value);
> +
> + /* Update the sequence only if the parameters are identical. */
> + if (*sequence_mismatch == false)
> + SetSequence(RelationGetRelid(rel), seq_last_value, seq_is_called,
> + seq_log_cnt);
> +
> + /* Return the LSN when the sequence state was set. */
> + return seq_page_lsn;
>
> 16a.
> Is that a bug in the code? AFAICT the fetch_remote_sequence_data is
> going to overwrite the new 'seq_page_lsn' even if some mismatch is
> detected. Is that intentional?

In the caller we set the sequence lsn only if sequence_mismatch is
false, so there is no issue.

> 19.
> + /*
> + * In case sequence copy fails, throw a warning for the sequences that
> + * did not match before exiting.
> + */
> + PG_TRY();
> + {
> + sequence_lsn = copy_sequence(LogRepWorkerWalRcvConn, sequence_rel,
> + &sequence_mismatch);
> + }
> + PG_CATCH();
> + {
> + if (sequence_mismatch)
> + append_mismatched_sequences(mismatched_seqs, sequence_rel);
> +
> + report_mismatched_sequences(mismatched_seqs);
> + PG_RE_THROW();
> + }
>
> If we got to the CATCH then it means some ERROR happened, but at that
> point I really don't think  sequence_mismatch is likely to be set as
> true. Maybe it is you just being extra careful, "just in case" ?

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.

>
> 21.
> + /*
> + * Sequence synchronization failed due to a parameter mismatch. Setting
> + * the failure time to prevent repeated initiation of the sequencesync
> + * worker.
> + */
>
> /to prevent repeated initiation/to prevent immediate initiation/ (??)

I felt repeated is correct here as we don't want to repeatedly start
the sequence sync worker after every failure.

> 23.
> + if (!started_tx)
> + {
> + StartTransactionCommand();
> + started_tx = true;
> + }
> +
> + Assert(get_rel_relkind(rstate->relid) != RELKIND_SEQUENCE);
> +
>
> Should this Assert come before the other tx code?

The existing is correct, get_rel_kind call requires a transaction to be started.

> 25.
> -getSubscriptionTables(Archive *fout)
> +getSubscriptionRelations(Archive *fout)
>
> Although you changed the function command and the function name for ,
> there is still code within that function referring to tables. Should
> that also be changed to relations?

You are talking about the error message right, I have changed that.

> ======
> src/include/commands/sequence.h
>
> 26.
> +#define SEQ_LOG_CNT_INVALID 0
>
> Zero seemed like a curious value to use as the "invalid" count. I was
> wondering would it be better to define this as -1, but then in the
> SetSequence function do some explicit code like below:
>
> seq->log_cnt = log_cnt == SEQ_LOG_CNT_INVALID ? 0 : log_cnt;

I felt using 0 in this case is ok.

> ======
> src/test/subscription/t/036_sequences.pl
>
> 27.
> +# Check the initial data on subscriber
> +my $result = $node_subscriber->safe_psql(
> + 'postgres', qq(
> + SELECT last_value, log_cnt, is_called FROM regress_s1;
> +));
> +is($result, '100|32|t', 'initial test data replicated');
>
> I think this deserves some explanatory comment about the magic number
> 32? But, it may be better to have a general comment at the top of this
> TAP test to explain other magic numbers like 31 etc...

log_cnt is prefetched sequence values, it is not a special magic
value. I felt no need to add any comment for this.

Rest of the comments are fixed.

Regarding the comments from [1].
1. State code: i = initialize, d = data is being copied, f = finished
table copy, s = synchronized, r = ready (normal replication)
~
This is not part of the patch, but AFAIK not all of those states are
relevant if the srrelid was a SEQUENCE relation. Should there be 2
sets of states given here for what is possible for tables/sequences?

I have updated the states which are not applicable for sequences in
the same page.

3.     Publications may currently only contain tables and all tables in schema.
    Objects must be added explicitly, except when a publication is created for
-   <literal>ALL TABLES</literal>.
+   <literal>ALL TABLES</literal>. Publications can include sequences as well,
+   but their behavior differs from that of tables or groups of tables. Unlike
+   tables, sequences allow users to synchronize their current state at any
+   given time. For more information, refer to
+   <xref linkend="logical-replication-sequences"/>.
   </para>

This doesn't really make sense. The change seems too obviously just
tacked onto the end of the existing documentation. e.g It is strange
saying "Publications may currently only contain tables and all tables
in schema." and then later saying Oh, BTW "Publications can include
sequences as well". I think the whole paragraph may need some
reworking.

The preceding para on this page also still says "A publication is a
set of changes generated from a table or a group of tables" which also
failed to mention sequences.

OTOH, I think it would get too confusing to mush everything together,
so after saying  publish tables and sequences, then I think there
should be one paragraph that just talks about publishing tables,
followed by another paragraph that just talks about publishing
sequences. Probably this should mention ALL SEQUENCES too since it
already mentioned ALL TABLES.

I did not create a separate paragraph for tables/sequences as there is
a separate section for sequence with the reference.

Rest of the comments are fixed. The attached v20250416 version patch
has the changes for the same.

[1] - https://www.postgresql.org/message-id/CAHut%2BPtc0gG%3D4j_BVqxHRGa%3DTKY_PsYu0RdsT6YuWPiNkSRhOQ%40mail.gmail.com

Regards,
Vignesh