From: | Peter Smith <smithpb2250(at)gmail(dot)com> |
---|---|
To: | Bharath Rupireddy <bharath(dot)rupireddyforpostgres(at)gmail(dot)com> |
Cc: | PostgreSQL Hackers <pgsql-hackers(at)lists(dot)postgresql(dot)org> |
Subject: | Re: Adding a LogicalRepWorker type field |
Date: | 2023-08-02 02:31:56 |
Message-ID: | CAHut+Pu9p2=KxTJ1gdcTqOitP6Hgk37JPqtBAAzA19xq+mtNTw@mail.gmail.com |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | 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(dot)rupireddyforpostgres(at)gmail(dot)com> wrote:
>
> On Mon, Jul 31, 2023 at 7:17 AM Peter Smith <smithpb2250(at)gmail(dot)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
Attachment | Content-Type | Size |
---|---|---|
v2-0002-Switch-on-worker-type.patch | application/octet-stream | 3.7 KB |
v2-0001-Add-LogicalRepWorkerType-enum.patch | application/octet-stream | 15.0 KB |
From | Date | Subject | |
---|---|---|---|
Next Message | David G. Johnston | 2023-08-02 02:36:01 | Re: Documentation of psql's \df no longer matches reality |
Previous Message | Amit Kapila | 2023-08-02 02:20:49 | Re: Inaccurate comments in ReorderBufferCheckMemoryLimit() |