Thanks for your detailed code review. Most comments are addressed in the attached v2 patches. Details inline below:
On Mon, Jul 31, 2023 at 7:55 PM Bharath Rupireddy
<bharath.rupireddyforpostg...@gmail.com> wrote:
>
> On Mon, Jul 31, 2023 at 7:17 AM Peter Smith <smithpb2...@gmail.com> wrote:
> >
> > PROBLEM:
> >
> > IMO, deducing the worker's type by examining multiple different field
> > values seems a dubious way to do it. This maybe was reasonable enough
> > when there were only 2 types, but as more get added it becomes
> > increasingly complicated.
>
> +1 for being more explicit in worker types. I also think that we must
> move away from am_{parallel_apply, tablesync,
> leader_apply}_worker(void) to Is{ParallelApply, TableSync,
> LeaderApply}Worker(MyLogicalRepWorker), just to be a little more
> consistent and less confusion around different logical replication
> worker type related functions.
>
Done. I converged everything to the macro-naming style as suggested,
but I chose not to pass MyLogicalRepWorker as a parameter for the
normal (am_xxx case) otherwise I felt it would make the calling code
unnecessarily verbose.
> Some comments:
> 1.
> +/* Different kinds of workers */
> +typedef enum LogicalRepWorkerType
> +{
> + LR_WORKER_TABLESYNC,
> + LR_WORKER_APPLY,
> + LR_WORKER_APPLY_PARALLEL
> +} LogicalRepWorkerType;
>
> Can these names be readable? How about something like
> LR_TABLESYNC_WORKER, LR_APPLY_WORKER, LR_PARALLEL_APPLY_WORKER?
Done. Renamed similar to your suggestion.
>
> 2.
> -#define isParallelApplyWorker(worker) ((worker)->leader_pid != InvalidPid)
> +#define isLeaderApplyWorker(worker) ((worker)->type == LR_WORKER_APPLY)
> +#define isParallelApplyWorker(worker) ((worker)->type ==
> LR_WORKER_APPLY_PARALLEL)
> +#define isTablesyncWorker(worker) ((worker)->type == LR_WORKER_TABLESYNC)
>
> Can the above start with "Is" instead of "is" similar to
> IsLogicalWorker and IsLogicalParallelApplyWorker?
>
Done as suggested.
> 3.
> + worker->type =
> + OidIsValid(relid) ? LR_WORKER_TABLESYNC :
> + is_parallel_apply_worker ? LR_WORKER_APPLY_PARALLEL :
> + LR_WORKER_APPLY;
>
> Perhaps, an if-else is better for readability?
>
> if (OidIsValid(relid))
> worker->type = LR_WORKER_TABLESYNC;
> else if (is_parallel_apply_worker)
> worker->type = LR_WORKER_APPLY_PARALLEL;
> else
> worker->type = LR_WORKER_APPLY;
>
Done as suggested.
> 4.
> +/* Different kinds of workers */
> +typedef enum LogicalRepWorkerType
> +{
> + LR_WORKER_TABLESYNC,
> + LR_WORKER_APPLY,
> + LR_WORKER_APPLY_PARALLEL
> +} LogicalRepWorkerType;
>
> Have a LR_WORKER_UNKNOWN = 0 and set it in logicalrep_worker_cleanup()?
>
Done as suggested.
> 5.
> + case LR_WORKER_APPLY:
> + return (rel->state == SUBREL_STATE_READY ||
> + (rel->state == SUBREL_STATE_SYNCDONE &&
> + rel->statelsn <= remote_final_lsn));
>
> Not necessary, but a good idea to have a default: clause and error out
> saying wrong logical replication worker type?
>
Not done. Switching on the enum gives a compiler warning if the case
is not handled. All enums are handled.
> 6.
> + case LR_WORKER_APPLY_PARALLEL:
> + /*
> + * Skip for parallel apply workers because they only
> operate on tables
> + * that are in a READY state. See pa_can_start() and
> + * should_apply_changes_for_rel().
> + */
> + break;
>
> I'd better keep this if-else as-is instead of a switch case with
> nothing for parallel apply worker.
>
Not done. IIUC using a switch, the compiler can optimize the code to a
single "jump" thereby saving the extra type-check the HEAD code is
doing. Admittedly, it’s only a nanosecond saving, so it is no problem
to revert this change, but why waste any CPU time unless there is a
reason to?
------
Kind Regards,
Peter Smith.
Fujitsu Australia
v2-0002-Switch-on-worker-type.patch
Description: Binary data
v2-0001-Add-LogicalRepWorkerType-enum.patch
Description: Binary data
