Re: Logical Replication of sequences

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.

> ======
> src/backend/replication/logical/syncutils.c
>
> 3.3. "common code" ??
>
> FYI - There are multiple code comments mentioning "common code..."
> which, in the absence of the sequencesync worker (which comes in the
> next patch), have nothing "common" about them at all. Fixing them and
> then fixing them again in the next patch might cause unnecessary code
> churn, but OTOH they aren't correct as-is either. I have left them
> alone for now.

We can ignore this as this will get merged to the next one. If you
have any comments you can give it on top of the next(0004) patch.

> ~
>
> 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.

> ~~~
>
> nit - file header comment
>
> ======
> src/backend/replication/logical/tablesync.c
>
> 3.5.
> -static void
> +void
> process_syncing_tables_for_sync(XLogRecPtr current_lsn)
>
> -static void
> +void
> process_syncing_tables_for_apply(XLogRecPtr current_lsn)
>
> Since these functions are no longer static should those function names
> be changed to use the CamelCase convention for non-static API?

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.

The rest of the comments were fixed, the attached v20240813 has the
changes for the same.

Regards,
Vignesh