Re: Adding a LogicalRepWorker type field - Mailing list pgsql-hackers

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.rupireddyforpostgres@gmail.com> wrote:
>
> On Mon, Jul 31, 2023 at 7:17 AM Peter Smith <smithpb2250@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