On Tue, Aug 13, 2024 at 10:00 PM vignesh C <vignes...@gmail.com> wrote: > > On Tue, 13 Aug 2024 at 09:19, Peter Smith <smithpb2...@gmail.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
