Re: Logical Replication of sequences

On Tue, Sep 23, 2025 at 6:39 PM vignesh C <vignesh21@gmail.com> wrote:
>
> On Mon, 22 Sept 2025 at 12:03, shveta malik <shveta.malik@gmail.com> wrote:
> >
> > On Thu, Sep 18, 2025 at 4:07 PM vignesh C <vignesh21@gmail.com> wrote:
> > >
> > > Thanks for the comments, these are handled in the attached patch.
> > >
> >
> > Please find a few comments:
> >
> >
> > patch005:
> > 1)
> > GetSubscriptionRelations:
> > + /* Skip sequences if they were not requested */
> > + if (!get_sequences && (relkind == RELKIND_SEQUENCE))
> > + continue;
> > +
> > + /* Skip tables if they were not requested */
> > + if (!get_tables && (relkind != RELKIND_SEQUENCE))
> > + continue;
> >
> > The use of negated conditions makes the logic harder to follow,
> > especially in the second if block.
> >
> > Can we write it as:
> >    bool is_sequence = (relkind == RELKIND_SEQUENCE);
> >
> >    /* Skip if the relation type is not requested */
> >    if ((get_tables && is_sequence) ||
> >        (get_sequences && !is_sequence))
> >        continue;
> >
> > Or at-least:
> > /* Skip sequences if they were not requested */
> > if (get_tables && (relkind == RELKIND_SEQUENCE))
> >   continue;
> >
> > /* Skip tables if they were not requested */
> > if (get_sequences && (relkind != RELKIND_SEQUENCE))
> >   continue;
>
> I felt this would not work. Say we want both sequences and tables,
> won't it skip the sequence this way from:
> if (get_tables && (relkind == RELKIND_SEQUENCE))
>   continue;
>

Okay, I see your point. In that case, could we reverse the conditions
instead? That seems like the more obvious choice in terms of
readability :
if ((relkind == RELKIND_SEQUENCE && !get_sequences))
    continue;

if ((relkind != RELKIND_SEQUENCE && !get_tables))
    continue;

For second if-block, more understandable conditions will be:
(relkind == RELKIND_RELATION || relkind == RELKIND_PARTITIONED_TABLE)
&& !get_tables

But here we will have to check relkind twice, so I leave the decision to you.

~~

Few comments on latest patch:

LogicalRepSyncSequences():
1)
+ seq_entry = hash_search(sequences_to_copy, &key,
+ HASH_ENTER, &found);
+ Assert(seq_entry != NULL);

Since we are using HASH_ENTER, it will be good to add Assert(!found) as well.

2)
+ sequences_to_copy = hash_create("Logical replication sequence sync
worker sequences",
+ 256, &ctl, HASH_ELEM | HASH_FUNCTION | HASH_COMPARE | HASH_CONTEXT);
+

The name of the hash-table looks odd. Shall it simply be 'Logical
replication sequences'. 'Logical Replication' is good enough to give
an indication that these are sequences being synchronized by seq-sync
worker.

~~

copy_sequences():
3)
+ if (batch_size >= MAX_SEQUENCES_SYNC_PER_BATCH ||
+ (current_index + batch_size == total_seqs))
+ break;

In the first condition, I think a better comparison will be equality
one (batch_size == MAX_SEQUENCES_SYNC_PER_BATCH). We are not letting
batch_size go beyond MAX_SEQUENCES_SYNC_PER_BATCH.

4)
+ if (batch_size == 0)
+ {
+ CommitTransactionCommand();
+ break;
+ }

I could not think of a scenario when this will be hit. The outer loop
condition has 'while (current_index < total_seqs)', so batch_size has
to be something. Even if we skip some entries due to
remote_seq_queried being set to true already, that should not make
batch_size as 0 as the entries with remote_seq_queried=true are
already accounted for in current_index. Or am I missing something
here?

~~

sequencesync_list_invalidate_cb():
5)

+ /* invalidate all entries */
+ hash_seq_init(&status, sequences_to_copy);
+ while ((entry = (LogicalRepSequenceInfo *) hash_seq_search(&status)) != NULL)
+ entry->entry_valid = false;

Can you please elaborate when this case can be hit? I see such logic
in all such invalidation functions registered with
CacheRegisterRelcacheCallback(), but could not find any relevant
comment.

~~

Reviewing further.

thanks
Shveta