Hi Peter,
Peter Smith <smithpb2...@gmail.com>, 26 Tem 2023 Çar, 07:40 tarihinde şunu
yazdı:
> Here are some comments for patch v22-0001.
>
> ======
> 1. General -- naming conventions
>
> There is quite a lot of inconsistency with variable/parameter naming
> styles in this patch. I understand in most cases the names are copied
> unchanged from the original functions. Still, since this is a big
> refactor anyway, it can also be a good opportunity to clean up those
> inconsistencies instead of just propagating them to different places.
> IIUC, the usual reluctance to rename things because it would cause
> backpatch difficulties doesn't apply here (since everything is being
> refactored anyway).
>
> E.g. Consider using use snake_case names more consistently in the
> following places:
>
I can simply change the places you mentioned, that seems okay to me.
The reason why I did not change the namings in existing variables/functions
is because I did (and still do) not get what's the naming conventions in
those files. Is snake_case the convention for variables in those files (or
in general)?
2. SetupApplyOrSyncWorker
>
> -ApplyWorkerMain(Datum main_arg)
> +SetupApplyOrSyncWorker(int worker_slot)
> {
> - int worker_slot = DatumGetInt32(main_arg);
> - char originname[NAMEDATALEN];
> - XLogRecPtr origin_startpos = InvalidXLogRecPtr;
> - char *myslotname = NULL;
> - WalRcvStreamOptions options;
> - int server_version;
> -
> - InitializingApplyWorker = true;
> -
> /* Attach to slot */
> logicalrep_worker_attach(worker_slot);
>
> + Assert(am_tablesync_worker() || am_leader_apply_worker());
> +
>
> Why is the Assert not the very first statement of this function?
>
I would also prefer to assert in the very beginning but am_tablesync_worker
and am_leader_apply_worker require MyLogicalRepWorker to be not NULL.
And MyLogicalRepWorker is assigned in logicalrep_worker_attach. I can
change this if you think there is a better way to check the worker type.
Thanks,
--
Melih Mutlu
Microsoft
