Re: Logical Replication of sequences

On Tue, Aug 13, 2024 at 10:00 PM vignesh C <vignesh21(at)gmail(dot)com> wrote:
>
> On Tue, 13 Aug 2024 at 09:19, Peter Smith <smithpb2250(at)gmail(dot)com> wrote:
> >
> > 3.1. GENERAL
> >
> > Hmm. I am guessing this was provided as a separate patch to aid review
> > by showing that existing functions are moved? OTOH you can't really
> > judge this patch properly without already knowing details of what will
> > come next in the sequencesync. i.e. As a *standalone* patch without
> > the sequencesync.c the refactoring doesn't make much sense.
> >
> > Maybe it is OK later to combine patches 0003 and 0004. Alternatively,
> > keep this patch separated but give greater emphasis in the comment
> > header to say this patch only exists separately in order to help the
> > review.
>
> I have kept this patch only to show that this patch as such has no
> code changes. If we move this to the next patch it will be difficult
> for reviewers to know which is new code and which is old code. During
> commit we can merge this with the next one. I felt it is better to add
> it in the commit message instead of comment header so updated the
> commit message.
>

Yes, I wrote "comment header" but it was a typo; I meant "commit
header". What you did looks good now. Thanks.

> > ~
> >
> > 3.4. function names
> >
> > With the re-shuffling that this patch does, and changing several from
> > static to not-static, should the function names remain as they are?
> > They look random to me.
> > - finish_sync_worker(void)
> > - invalidate_syncing_table_states(Datum arg, int cacheid, uint32 hashvalue)
> > - FetchTableStates(bool *started_tx)
> > - process_syncing_tables(XLogRecPtr current_lsn)
> >
> > I think using a consistent naming convention would be better. e.g.
> > SyncFinishWorker
> > SyncInvalidateTableStates
> > SyncFetchTableStates
> > SyncProcessTables
>
> One advantage with keeping the existing names the same wherever
> possible will help while merging the changes to back-branches. So I'm
> not making this change.
>

According to my understanding, the logical replication code tries to
maintain name conventions for static functions (snake_case) and for
non-static functions (CamelCase) as an aid for code readability. I
think we should either do our best to abide by those conventions, or
we might as well just forget them and have a naming free-for-all.
Since the new syncutils.c module is being introduced by this patch, my
guess is that any future merging to back-branches will be affected
regardless. IMO this is an ideal opportunity to try to nudge the
function names in the right direction. YMMV.

======
Kind Regards,
Peter Smith.
Fujitsu Australia