On Thu, 15 May 2025 at 12:27, Peter Smith <smithpb2250@gmail.com> wrote:
>
> Hi Vignesh.
>
> Some minor review comments for the patches in set v20250514.
>
> ======
>
> Patch 0001.
>
> 1.1
> For function 'pg_sequence_state', the DOCS call the 2nd parameter
> 'sequence_name', but the pg_proc.dat file calls it 'seq_name'. Should
> these be made the same?
>
> ////////////////////
>
> Patch 0004.
>
> pg_sequence_state:
>
> 4.1
>
> - errmsg("sequence \"%s.%s\" does not exist",
> + errmsg("logical replication sequence \"%s.%s\" does not exist",
>
> Why isn't this change already be done in an early patch when this
> function was first implemented?
>
> ~~~
>
> copy_sequences:
>
> 4.2
>
> +/*
> + * Copy existing data of sequnces from the publisher.
> + *
>
> Typo: "sequnces"
>
> ~~~
>
> 4.3
> +{
> + int total_seq = list_length(remotesequences);
> + int curr_seq = 0;
> +
> +/*
> + * We batch synchronize multiple sequences per transaction, because the
> + * alternative of synchronizing each sequence individually incurs overhead of
> + * starting and committing transactions repeatedly. On the other hand, we want
> + * to avoid keeping this batch transaction open for extended periods so it is
> + * currently limited to 100 sequences per batch.
> + */
> +#define MAX_SEQUENCES_SYNC_PER_BATCH 100
>
> Wrong indent for block comment.
>
> ~~~
>
> 4.4
> + if (res->status != WALRCV_OK_TUPLES)
> + ereport(ERROR,
> + errcode(ERRCODE_CONNECTION_FAILURE),
> + errmsg("could not receive list of sequences information from the
> publisher: %s",
> + res->err));
>
> Should this say /sequences information/sequence information/
>
> ~~~
>
> 4.5
> + ereport(LOG,
> + errmsg("Logical replication sequence synchronization - total
> unsynchronized: %d, attempted in this batch: %d; succeeded in this
> batch: %d; mismatched in this batch: %d for subscription: \"%s\"",
> + total_seq, batch_seq_count, batch_success_count,
> + batch_mismatch_count, get_subscription_name(subid, false)));
> +
>
>
> This errmsg seems backwards. I think it should be expressed like the
> other one immediately above. Also I think the information can be made
> shorter -- e.g. no need to say "in this batch" multiple times.
>
> SUGGESTION
> "Logical replication sequence synchronization for subscription \"%s\"
> - total unsynchronized: %d; batch #%d = %d attempted, %d succeeded, %d
> mismatched"
Thanks for the comments, these are handled in the attached v20250516
version patch.
Regards,
Vignesh
