Re: Adding a LogicalRepWorker type field

On Wed, Aug 2, 2023 at 3:35 PM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
>
> On Mon, Jul 31, 2023 at 10:47 AM Peter Smith <smithpb2250@gmail.com> wrote:
> >
> > Hi hackers,
> >
> > BACKGROUND:
> >
> > The logical replication has different worker "types" (e.g. apply
> > leader, apply parallel, tablesync).
> >
> > They all use a common structure called LogicalRepWorker, but at times
> > it is necessary to know what "type" of worker a given LogicalRepWorker
> > represents.
> >
> > Once, there were just apply workers and tablesync workers - these were
> > easily distinguished because only tablesync workers had a valid
> > 'relid' field. Next, parallel-apply workers were introduced - these
> > are distinguishable from the apply leaders by the value of
> > 'leader_pid' field.
> >
> >
> > 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.
> >
> > SOLUTION:
> >
> > AFAIK none of the complications is necessary anyway; the worker type
> > is already known at launch time, and it never changes during the life
> > of the process.
> >
> > The attached Patch 0001 introduces a new enum 'type' field, which is
> > assigned during the launch.
> >
> > ~
> >
> > This change not only simplifies the code, but it also permits other
> > code optimizations, because now we can switch on the worker enum type,
> > instead of using cascading if/else.  (see Patch 0002).
> >
> > Thoughts?
>
> I can see the problem you stated and it's true that the worker's type
> never changes during its lifetime. But I'm not sure we really need to
> add a new variable to LogicalRepWorker since we already have enough
> information there to distinguish the worker's type.
>
> Currently, we deduce the worker's type by checking some fields that
> never change such as relid and leader_piid. It's fine to me as long as
> we encapcel the deduction of worker's type by macros (or inline
> functions). Even with the proposed patch (0001 patch), we still need a
> similar logic to determine the worker's type.

Thanks for the feedback.

I agree that the calling code will not look very different
with/without this patch 0001, because everything is hidden by the
macros/functions. But those HEAD macros/functions are also hiding the
inefficiencies of the type deduction -- e.g. IMO it is quite strange
that we can only recognize the worker is an "apply worker" by
eliminating the other 2 possibilities.

As further background, the idea for this patch came from considering
another thread to "re-use" workers [1]. For example, when thinking
about re-using tablesync workers the HEAD am_tablesync_worker()
function begins to fall apart -- ie. it does not let you sensibly
disassociate a tablesync worker from its relid (which you might want
to do if tablesync workers were "pooled") because in the HEAD code any
tablesync worker without a relid is (by definition) NOT recognized as
a tablesync worker anymore!

Those above issues just disappear when a type field is added.

>
> If we want to change both process_syncing_tables() and
> should_apply_changes_for_rel() to use the switch statement instead of
> using cascading if/else, I think we can have a function, say
> LogicalRepWorkerType(), that returns LogicalRepWorkerType of the given
> worker.
>

The point of that "switch" in patch 0002 was to demonstrate that with
a worker-type enum we can write more *efficient* code in some places
than the current cascading if/else. I think making a "switch" just for
the sake of it, by adding more functions that hide yet more work, is
going in the opposite direction.

------
[1] reuse workers -
https://www.postgresql.org/message-id/flat/CAGPVpCTq%3DrUDd4JUdaRc1XUWf4BrH2gdSNf3rtOMUGj9rPpfzQ%40mail.gmail.com

Kind Regards,
Peter Smith.
Fujitsu Australia