Few more concerns:
4)
In UpdateSubscriptionRelState():
if (!HeapTupleIsValid(tup))
elog(ERROR, "subscription table %u in subscription %u
does not exist",
relid, subid);
table-->relation as now it can be hit for both sequence and table.
5)
In LogicalRepSyncSequences, why are we allocating it in a permanent
memory context?
+ /* Allocate the tracking info in a permanent memory context. */
+ oldctx = MemoryContextSwitchTo(CacheMemoryContext);
+ foreach_ptr(SubscriptionRelState, seq_state, sequences)
+ {
+ SubscriptionRelState *rstate = palloc(sizeof(SubscriptionRelState));
+
+ memcpy(rstate, seq_state, sizeof(SubscriptionRelState));
+ sequences_not_synced = lappend(sequences_not_synced, rstate);
+ }
+ MemoryContextSwitchTo(oldctx);
Same for 'seq_info' allocation.
6)
In LogicalRepSyncSequences, can you please help me understand this:
Why have we first created new list 'sequences_not_synced' from
'sequences' list, both have same elements of type
SubscriptionRelState; and then used that newly created
'sequences_not_synced list' to create remotesequences list having
element type LogicalRepSequenceInfo. Why didn't we use 'sequences'
list directly to create 'remotesequences' list?
7)
In this function we have 2 variables: seqinfo and seq_info. We are
using seqinfo to create seq_info. The names are very confusing.
Difficult to differentiate between the two.
In copy_sequences() too we have similarly named variables: seqinfo and
sequence_info.
Can we choose different names here?
8)
Why have we named argument of copy_sequences() as remotesequences?
IIUC, these are the sequences fetched from pg_subscription_rel and its
elements even maintain a field 'localrelid'. The name is thus
confusing as it has local data. Perhaps we can name it as
candidate_sequences or init_sequences (i.e. sequences in init state)
or any other suitable name.
thanks
Shveta
