Обсуждение: Adding a LogicalRepWorker type field

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?

------
Kind Regards,
Peter Smith.
Fujitsu Australia

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.

> 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).

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?

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?

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;

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()?

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?

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.

--
Bharath Rupireddy
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com

On Mon, Jul 31, 2023 at 3:25 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.
>

+1. BTW, do we need the below functions (am_tablesync_worker(),
am_leader_apply_worker()) after this work?
static inline bool
 am_tablesync_worker(void)
 {
- return OidIsValid(MyLogicalRepWorker->relid);
+ return isTablesyncWorker(MyLogicalRepWorker);
 }

 static inline bool
 am_leader_apply_worker(void)
 {
- return (!am_tablesync_worker() &&
- !isParallelApplyWorker(MyLogicalRepWorker));
+ return isLeaderApplyWorker(MyLogicalRepWorker);
 }


--
With Regards,
Amit Kapila.

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

On Mon, Jul 31, 2023 at 11:11 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
>
> +1. BTW, do we need the below functions (am_tablesync_worker(),
> am_leader_apply_worker()) after this work?
> static inline bool
>  am_tablesync_worker(void)
>  {
> - return OidIsValid(MyLogicalRepWorker->relid);
> + return isTablesyncWorker(MyLogicalRepWorker);
>  }
>
>  static inline bool
>  am_leader_apply_worker(void)
>  {
> - return (!am_tablesync_worker() &&
> - !isParallelApplyWorker(MyLogicalRepWorker));
> + return isLeaderApplyWorker(MyLogicalRepWorker);
>  }
>

The am_xxx functions are removed now in the v2-0001 patch. See [1].

The replacement set of macros (the ones with no arg) are not strictly
necessary, except I felt it would make the code unnecessarily verbose
if we insist to pass MyLogicalRepWorker everywhere from the callers in
worker.c / tablesync.c / applyparallelworker.c.

------
Kind Regards,
Peter Smith.
Fujitsu Australia

On Wed, Aug 2, 2023 at 8:10 AM Peter Smith <smithpb2250@gmail.com> wrote:
>
>
> The am_xxx functions are removed now in the v2-0001 patch. See [1].
>
> The replacement set of macros (the ones with no arg) are not strictly
> necessary, except I felt it would make the code unnecessarily verbose
> if we insist to pass MyLogicalRepWorker everywhere from the callers in
> worker.c / tablesync.c / applyparallelworker.c.
>

Agreed but having a dual set of macros is also not clean. Can we
provide only a unique set of inline functions instead?

--
With Regards,
Amit Kapila.

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.

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.

Regards,

--
Masahiko Sawada
Amazon Web Services: https://aws.amazon.com

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

On Wed, Aug 2, 2023 at 1:00 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
>
> On Wed, Aug 2, 2023 at 8:10 AM Peter Smith <smithpb2250@gmail.com> wrote:
> >
> >
> > The am_xxx functions are removed now in the v2-0001 patch. See [1].
> >
> > The replacement set of macros (the ones with no arg) are not strictly
> > necessary, except I felt it would make the code unnecessarily verbose
> > if we insist to pass MyLogicalRepWorker everywhere from the callers in
> > worker.c / tablesync.c / applyparallelworker.c.
> >
>
> Agreed but having a dual set of macros is also not clean. Can we
> provide only a unique set of inline functions instead?
>

We can't use the same names for both with/without-parameter functions
because there is no function overloading in C. In patch v3-0001 I've
replaced the "dual set of macros", with a single inline function of a
different name, and one set of space-saving macros.

PSA v3

------
Kind Regards,
Peter Smith.
Fujitsu Australia

On Wed, Aug 2, 2023 at 12:14 PM Peter Smith <smithpb2250@gmail.com> wrote:
>
> We can't use the same names for both with/without-parameter functions
> because there is no function overloading in C. In patch v3-0001 I've
> replaced the "dual set of macros", with a single inline function of a
> different name, and one set of space-saving macros.
>
> PSA v3

Quick thoughts:

1.
+typedef enum LogicalRepWorkerType
+{
+    TYPE_UNKNOWN = 0,
+    TYPE_TABLESYNC_WORKER,
+    TYPE_APPLY_WORKER,
+    TYPE_PARALLEL_APPLY_WORKER
+} LogicalRepWorkerType;

-1 for these names starting with prefix TYPE_, in fact LR_ looked better.

2.
+is_worker_type(LogicalRepWorker *worker, LogicalRepWorkerType type)

This function name looks too generic, an element of  logical
replication is better in the name.

3.
+#define IsLeaderApplyWorker() is_worker_type(MyLogicalRepWorker,
TYPE_APPLY_WORKER)
+#define IsParallelApplyWorker() is_worker_type(MyLogicalRepWorker,
TYPE_PARALLEL_APPLY_WORKER)
+#define IsTablesyncWorker() is_worker_type(MyLogicalRepWorker,
TYPE_TABLESYNC_WORKER)

My thoughts were around removing am_XXX_worker() and
IsXXXWorker(&worker) functions and just have IsXXXWorker(&worker)
alone with using IsXXXWorker(MyLogicalRepWorker) in places where
am_XXX_worker() is used. If implementing this leads to something like
the above with is_worker_type, -1 from me.

> 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.

With is_worker_type, the calling code now looks even more verbose. I
don't think IsLeaderApplyWorker(MyLogicalRepWorker) is a bad idea.
This unifies multiple worker type functions into one and makes code
simple.

--
Bharath Rupireddy
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com

On Wed, Aug 2, 2023 at 2:46 PM Bharath Rupireddy
<bharath.rupireddyforpostgres@gmail.com> wrote:
>
> On Wed, Aug 2, 2023 at 12:14 PM Peter Smith <smithpb2250@gmail.com> wrote:
> >
> > We can't use the same names for both with/without-parameter functions
> > because there is no function overloading in C. In patch v3-0001 I've
> > replaced the "dual set of macros", with a single inline function of a
> > different name, and one set of space-saving macros.
> >
> > PSA v3
>
> Quick thoughts:
>
> 1.
> +typedef enum LogicalRepWorkerType
> +{
> +    TYPE_UNKNOWN = 0,
> +    TYPE_TABLESYNC_WORKER,
> +    TYPE_APPLY_WORKER,
> +    TYPE_PARALLEL_APPLY_WORKER
> +} LogicalRepWorkerType;
>
> -1 for these names starting with prefix TYPE_, in fact LR_ looked better.
>
> 2.
> +is_worker_type(LogicalRepWorker *worker, LogicalRepWorkerType type)
>
> This function name looks too generic, an element of  logical
> replication is better in the name.
>
> 3.
> +#define IsLeaderApplyWorker() is_worker_type(MyLogicalRepWorker,
> TYPE_APPLY_WORKER)
> +#define IsParallelApplyWorker() is_worker_type(MyLogicalRepWorker,
> TYPE_PARALLEL_APPLY_WORKER)
> +#define IsTablesyncWorker() is_worker_type(MyLogicalRepWorker,
> TYPE_TABLESYNC_WORKER)
>
> My thoughts were around removing am_XXX_worker() and
> IsXXXWorker(&worker) functions and just have IsXXXWorker(&worker)
> alone with using IsXXXWorker(MyLogicalRepWorker) in places where
> am_XXX_worker() is used. If implementing this leads to something like
> the above with is_worker_type, -1 from me.
>

What I was suggesting to Peter to have something like:
static inline bool
am_tablesync_worker(void)
{
return (MyLogicalRepWorker->type == TYPE_APPLY_WORKER);
}

--
With Regards,
Amit Kapila.

Thank you for the feedback received so far. Sorry, I have become busy
lately with other work.

IIUC there is a general +1 for the idea, but I still have some
juggling necessary to make the functions/macros of patch 0001
acceptable to everybody.

The difficulties arise from:
- no function overloading C
- ideally, we prefer the same names for everything (e.g. instead of
dual-set macros)
- but launcher code calls need to pass param (other code always uses
MyLogicalRepWorker)
- would be nice (although no mandatory) to not have to always pass
MyLogicalRepWorker as a param.

Anyway, I will work towards finding some compromise on this next week.
Currently, I feel the best choice is to go with what Bharath suggested
in the first place -- just always pass the parameter (including
passing MyLogicalRepWorker). I will think more about it...

------

Meanwhile, here are some replies to other feedback received:

Ashutosh --  "May be we should create a union of type specific members" [1].

Yes, I was wondering this myself, but I won't pursue this yet until
getting over the hurdle of finding acceptable functions/macros for
patch 0001. Hopefully, we can come back to this idea.

~~

Mellih -- "Isn't the apply worker type still decided by eliminating
the other choices even with the patch?".

AFAIK the only case of that now is the one-time check in the
logicalrep_worker_launch() function. IMO, that is quite a different
proposition to the HEAD macros that have to make that deduction
10,000s ot times in executing code. Actually, even the caller knows
exactly the kind of worker it wants to launch so we can pass the
LogicalRepWorkerType directly to logicalrep_worker_launch() and
eliminate even this one-time-check. I can code it that way in the next
patch version.

~~

Barath -- "-1 for these names starting with prefix TYPE_, in fact LR_
looked better."

Hmmm. I'm not sure what is best. Of the options below I prefer
"WORKER_TYPE_xxx", but I don't mind so long as there is a consensus.

LR_APPLY_WORKER
LR_PARALLEL_APPLY_WORKER
LR_TABLESYNC_WORKER

TYPE_APPLY_WORKERT
TYPE_PARALLEL_APPLY_WORKER
TYPE_TABLESYNC_WORKER

WORKER_TYPE_APPLY
WORKER_TYPE_PARALLEL_APPLY
WORKER_TYPE_TABLESYNC

APPLY_WORKER
PARALLEL_APPLY_WORKER
TABLESYNC_WORKER

APPLY
PARALLEL_APPLY
TABLESYNC

------
[1] Ashutosh -
https://www.postgresql.org/message-id/CAExHW5uALiimrdpdO0vwuDivD99TW%2B_9vvfFsErVNzq1ehYV9Q%40mail.gmail.com
[2] Melih -
https://www.postgresql.org/message-id/CAGPVpCRZ-zEOa2Lkvw%2BiTU3NhJzfbGwH1dU7Htreup--8e5nxg%40mail.gmail.com
[3] Bharath -
https://www.postgresql.org/message-id/CALj2ACVro6oCsTg_gpYu%2BV_LPMSgk4wjmSPDk8d5thArWNRoRQ%40mail.gmail.com

Kind Regards,
Peter Smith.
Fujitsu Australia

PSA v4 patches.

On Fri, Aug 4, 2023 at 5:45 PM Peter Smith <smithpb2250@gmail.com> wrote:
>
> Thank you for the feedback received so far. Sorry, I have become busy
> lately with other work.
>
> IIUC there is a general +1 for the idea, but I still have some
> juggling necessary to make the functions/macros of patch 0001
> acceptable to everybody.
>
> The difficulties arise from:
> - no function overloading C
> - ideally, we prefer the same names for everything (e.g. instead of
> dual-set macros)
> - but launcher code calls need to pass param (other code always uses
> MyLogicalRepWorker)
> - would be nice (although no mandatory) to not have to always pass
> MyLogicalRepWorker as a param.
>
> Anyway, I will work towards finding some compromise on this next week.
> Currently, I feel the best choice is to go with what Bharath suggested
> in the first place -- just always pass the parameter (including
> passing MyLogicalRepWorker). I will think more about it...

v4-0001 uses only 3 simple inline functions. Callers always pass
parameters as Bharath had suggested.

>
> ------
>
> Meanwhile, here are some replies to other feedback received:
>
> Ashutosh --  "May be we should create a union of type specific members" [1].
>
> Yes, I was wondering this myself, but I won't pursue this yet until
> getting over the hurdle of finding acceptable functions/macros for
> patch 0001. Hopefully, we can come back to this idea.
>

To be explored later.

> ~~
>
> Mellih -- "Isn't the apply worker type still decided by eliminating
> the other choices even with the patch?".
>
> AFAIK the only case of that now is the one-time check in the
> logicalrep_worker_launch() function. IMO, that is quite a different
> proposition to the HEAD macros that have to make that deduction
> 10,000s ot times in executing code. Actually, even the caller knows
> exactly the kind of worker it wants to launch so we can pass the
> LogicalRepWorkerType directly to logicalrep_worker_launch() and
> eliminate even this one-time-check. I can code it that way in the next
> patch version.
>

Now even the one-time checking to assign the worker type is removed.
The callers know the LogicalReWorkerType they want, so v4-0001 just
passes the type into logicalrep_worker_launch()

> ~~
>
> Barath -- "-1 for these names starting with prefix TYPE_, in fact LR_
> looked better."
>
> Hmmm. I'm not sure what is best. Of the options below I prefer
> "WORKER_TYPE_xxx", but I don't mind so long as there is a consensus.
>
> LR_APPLY_WORKER
> LR_PARALLEL_APPLY_WORKER
> LR_TABLESYNC_WORKER
>
> TYPE_APPLY_WORKERT
> TYPE_PARALLEL_APPLY_WORKER
> TYPE_TABLESYNC_WORKER
>
> WORKER_TYPE_APPLY
> WORKER_TYPE_PARALLEL_APPLY
> WORKER_TYPE_TABLESYNC
>
> APPLY_WORKER
> PARALLEL_APPLY_WORKER
> TABLESYNC_WORKER
>
> APPLY
> PARALLEL_APPLY
> TABLESYNC
>

For now, in v4-0001 these are called WORKERTYPE_xxx. Please do propose
better names if these are no good.

------
Kind Regards,
Peter Smith.
Fujitsu Australia

On Tue, Aug 8, 2023 at 1:39 PM Peter Smith <smithpb2250@gmail.com> wrote:
>
> On Fri, Aug 4, 2023 at 5:45 PM Peter Smith <smithpb2250@gmail.com> wrote:
>
> v4-0001 uses only 3 simple inline functions. Callers always pass
> parameters as Bharath had suggested.
>

*
- Assert(am_leader_apply_worker());
+ Assert(is_leader_apply_worker(MyLogicalRepWorker));
...
- if (am_leader_apply_worker())
+ if (is_leader_apply_worker(MyLogicalRepWorker))

Passing everywhere MyLogicalRepWorker not only increased the code
change footprint but doesn't appear any better to me. Instead, let
am_parallel_apply_worker() keep calling
isParallelApplyWorker(MyLogicalRepWorker) as it is doing now. I feel
even if you or others feel that is a better idea, we can debate it
separately after the main patch is done because as far as I understand
that is not the core idea of this proposal.

* If you do the above then there won't be a need to change the
variable name is_parallel_apply_worker in logicalrep_worker_launch.

--
With Regards,
Amit Kapila.

On Wed, Aug 9, 2023 at 4:18 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
>
> On Tue, Aug 8, 2023 at 1:39 PM Peter Smith <smithpb2250@gmail.com> wrote:
> >
> > On Fri, Aug 4, 2023 at 5:45 PM Peter Smith <smithpb2250@gmail.com> wrote:
> >
> > v4-0001 uses only 3 simple inline functions. Callers always pass
> > parameters as Bharath had suggested.
> >
>
> *
> - Assert(am_leader_apply_worker());
> + Assert(is_leader_apply_worker(MyLogicalRepWorker));
> ...
> - if (am_leader_apply_worker())
> + if (is_leader_apply_worker(MyLogicalRepWorker))
>
> Passing everywhere MyLogicalRepWorker not only increased the code
> change footprint but doesn't appear any better to me. Instead, let
> am_parallel_apply_worker() keep calling
> isParallelApplyWorker(MyLogicalRepWorker) as it is doing now. I feel
> even if you or others feel that is a better idea, we can debate it
> separately after the main patch is done because as far as I understand
> that is not the core idea of this proposal.

Right, those changes were not really core. Reverted as suggested. PSA v5.

>
> * If you do the above then there won't be a need to change the
> variable name is_parallel_apply_worker in logicalrep_worker_launch.
>

Done.

------
Kind Regards,
Peter Smith.
Fujitsu Australia

On Thu, Aug 10, 2023 at 7:50 AM Peter Smith <smithpb2250@gmail.com> wrote:
>
> >
> > * If you do the above then there won't be a need to change the
> > variable name is_parallel_apply_worker in logicalrep_worker_launch.
> >
>
> Done.
>

I don't think the addition of two new macros isTablesyncWorker() and
isLeaderApplyWorker() adds much value, so removed those and ran
pgindent. I am planning to commit this patch early next week unless
you or others have any comments.

--
With Regards,
Amit Kapila.

On Fri, Aug 11, 2023 at 7:33 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
>
> On Thu, Aug 10, 2023 at 7:50 AM Peter Smith <smithpb2250@gmail.com> wrote:
> >
> > >
> > > * If you do the above then there won't be a need to change the
> > > variable name is_parallel_apply_worker in logicalrep_worker_launch.
> > >
> >
> > Done.
> >
>
> I don't think the addition of two new macros isTablesyncWorker() and
> isLeaderApplyWorker() adds much value, so removed those and ran
> pgindent. I am planning to commit this patch early next week unless
> you or others have any comments.
>

Thanks for considering this patch fit for pushing.

Actually, I recently found 2 more overlooked places in the launcher.c
code which can benefit from using the isTablesyncWorker(w) macro that
was removed in patch v6-0001.

I have posted another v7.  (v7-0001 is identical to v6-0001). The
v7-0002 patch has the isTablesyncWorker changes. I think wherever
possible it is better to check the worker-type via macro instead of
deducing it by fields like 'relid', and patch v7-0002 makes the code
more consistent with other nearby isParallelApplyWorker checks in
launcher.c

------
Kind Regards,
Peter Smith.
Fujitsu Australia

On Fri, Aug 11, 2023 at 3:41 PM Peter Smith <smithpb2250@gmail.com> wrote:
>
> On Fri, Aug 11, 2023 at 7:33 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
> >
> > On Thu, Aug 10, 2023 at 7:50 AM Peter Smith <smithpb2250@gmail.com> wrote:
> > >
> > > >
> > > > * If you do the above then there won't be a need to change the
> > > > variable name is_parallel_apply_worker in logicalrep_worker_launch.
> > > >
> > >
> > > Done.
> > >
> >
> > I don't think the addition of two new macros isTablesyncWorker() and
> > isLeaderApplyWorker() adds much value, so removed those and ran
> > pgindent. I am planning to commit this patch early next week unless
> > you or others have any comments.
> >
>
> Thanks for considering this patch fit for pushing.
>
> Actually, I recently found 2 more overlooked places in the launcher.c
> code which can benefit from using the isTablesyncWorker(w) macro that
> was removed in patch v6-0001.
>

@@ -1301,7 +1301,7 @@ pg_stat_get_subscription(PG_FUNCTION_ARGS)
  worker_pid = worker.proc->pid;

  values[0] = ObjectIdGetDatum(worker.subid);
- if (OidIsValid(worker.relid))
+ if (isTablesyncWorker(&worker))
  values[1] = ObjectIdGetDatum(worker.relid);

I don't see this as a good fit for using isTablesyncWorker(). If we
were returning worker_type then using it would be okay.


--
With Regards,
Amit Kapila.

On Fri, Aug 11, 2023 at 9:13 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
>
> On Fri, Aug 11, 2023 at 3:41 PM Peter Smith <smithpb2250@gmail.com> wrote:
> >
> > On Fri, Aug 11, 2023 at 7:33 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
> > >
> > > On Thu, Aug 10, 2023 at 7:50 AM Peter Smith <smithpb2250@gmail.com> wrote:
> > > >
> > > > >
> > > > > * If you do the above then there won't be a need to change the
> > > > > variable name is_parallel_apply_worker in logicalrep_worker_launch.
> > > > >
> > > >
> > > > Done.
> > > >
> > >
> > > I don't think the addition of two new macros isTablesyncWorker() and
> > > isLeaderApplyWorker() adds much value, so removed those and ran
> > > pgindent. I am planning to commit this patch early next week unless
> > > you or others have any comments.
> > >
> >
> > Thanks for considering this patch fit for pushing.
> >
> > Actually, I recently found 2 more overlooked places in the launcher.c
> > code which can benefit from using the isTablesyncWorker(w) macro that
> > was removed in patch v6-0001.
> >
>
> @@ -1301,7 +1301,7 @@ pg_stat_get_subscription(PG_FUNCTION_ARGS)
>   worker_pid = worker.proc->pid;
>
>   values[0] = ObjectIdGetDatum(worker.subid);
> - if (OidIsValid(worker.relid))
> + if (isTablesyncWorker(&worker))
>   values[1] = ObjectIdGetDatum(worker.relid);
>
> I don't see this as a good fit for using isTablesyncWorker(). If we
> were returning worker_type then using it would be okay.

Yeah, I also wasn't very sure about that one, except it seems
analogous to the existing code immediately below it, where you could
say the same thing:
        if (isParallelApplyWorker(&worker))
            values[3] = Int32GetDatum(worker.leader_pid);

Whatever you think is best for that one is fine by me.

------
Kind Regards,
Peter Smith.
Fujitsu Australia

On Sat, Aug 12, 2023 at 5:01 AM Peter Smith <smithpb2250@gmail.com> wrote:
>
> On Fri, Aug 11, 2023 at 9:13 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
> >
> > On Fri, Aug 11, 2023 at 3:41 PM Peter Smith <smithpb2250@gmail.com> wrote:
> > >
> > > On Fri, Aug 11, 2023 at 7:33 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
> > > >
> > > > On Thu, Aug 10, 2023 at 7:50 AM Peter Smith <smithpb2250@gmail.com> wrote:
> > > > >
> > > > > >
> > > > > > * If you do the above then there won't be a need to change the
> > > > > > variable name is_parallel_apply_worker in logicalrep_worker_launch.
> > > > > >
> > > > >
> > > > > Done.
> > > > >
> > > >
> > > > I don't think the addition of two new macros isTablesyncWorker() and
> > > > isLeaderApplyWorker() adds much value, so removed those and ran
> > > > pgindent. I am planning to commit this patch early next week unless
> > > > you or others have any comments.
> > > >
> > >
> > > Thanks for considering this patch fit for pushing.
> > >
> > > Actually, I recently found 2 more overlooked places in the launcher.c
> > > code which can benefit from using the isTablesyncWorker(w) macro that
> > > was removed in patch v6-0001.
> > >
> >
> > @@ -1301,7 +1301,7 @@ pg_stat_get_subscription(PG_FUNCTION_ARGS)
> >   worker_pid = worker.proc->pid;
> >
> >   values[0] = ObjectIdGetDatum(worker.subid);
> > - if (OidIsValid(worker.relid))
> > + if (isTablesyncWorker(&worker))
> >   values[1] = ObjectIdGetDatum(worker.relid);
> >
> > I don't see this as a good fit for using isTablesyncWorker(). If we
> > were returning worker_type then using it would be okay.
>
> Yeah, I also wasn't very sure about that one, except it seems
> analogous to the existing code immediately below it, where you could
> say the same thing:
>         if (isParallelApplyWorker(&worker))
>             values[3] = Int32GetDatum(worker.leader_pid);
>

Fair point. I think it is better to keep the code consistent. So, I'll
merge your changes and push the patch early next week.

--
With Regards,
Amit Kapila.

The main patch for adding the worker type enum has been pushed [1].

Here is the remaining (rebased) patch for changing some previous
cascading if/else to switch on the LogicalRepWorkerType enum instead.

PSA v8.

------
[1] https://github.com/postgres/postgres/commit/2a8b40e3681921943a2989fd4ec6cdbf8766566c

Kind Regards,
Peter Smith.
Fujitsu Australia

On Mon, Aug 14, 2023 at 12:08 PM Peter Smith <smithpb2250@gmail.com> wrote:
>
> The main patch for adding the worker type enum has been pushed [1].
>
> Here is the remaining (rebased) patch for changing some previous
> cascading if/else to switch on the LogicalRepWorkerType enum instead.
>

I see this as being useful if we plan to add more worker types. Does
anyone else see this remaining patch as an improvement?

--
With Regards,
Amit Kapila.

On Fri, Aug 18, 2023 at 8:50 AM Amit Kapila <amit.kapila16@gmail.com> wrote:
>
> On Mon, Aug 14, 2023 at 12:08 PM Peter Smith <smithpb2250@gmail.com> wrote:
> >
> > The main patch for adding the worker type enum has been pushed [1].
> >
> > Here is the remaining (rebased) patch for changing some previous
> > cascading if/else to switch on the LogicalRepWorkerType enum instead.
> >
>
> I see this as being useful if we plan to add more worker types. Does
> anyone else see this remaining patch as an improvement?
>

I feel it does give a tad bit more clarity for cases where we have
'else' part with no clear comments or relevant keywords. As an
example, in function 'should_apply_changes_for_rel' , we have:
        else
                return (rel->state == SUBREL_STATE_READY ||
                                (rel->state == SUBREL_STATE_SYNCDONE &&
                                 rel->statelsn <= remote_final_lsn));

It is difficult to figure out which worker is this if I do not know
the concept completely;   'case WORKERTYPE_APPLY' makes it better for
the reader to understand.

thanks
Shveta

On Friday, August 18, 2023 11:20 AM Amit Kapila <amit.kapila16@gmail.com> wrote:
> 
> On Mon, Aug 14, 2023 at 12:08 PM Peter Smith <smithpb2250@gmail.com>
> wrote:
> >
> > The main patch for adding the worker type enum has been pushed [1].
> >
> > Here is the remaining (rebased) patch for changing some previous
> > cascading if/else to switch on the LogicalRepWorkerType enum instead.
> >
> 
> I see this as being useful if we plan to add more worker types. Does anyone else
> see this remaining patch as an improvement?

+1

I have one comment for the new error message.

+        case WORKERTYPE_UNKNOWN:
+            ereport(ERROR, errmsg_internal("should_apply_changes_for_rel: Unknown worker type"));

I think reporting an ERROR in this case is fine. However, I would suggest
refraining from mentioning the function name in the error message, as
recommended in the error style document [1]. Also, it appears we could use
elog() here.

[1] https://www.postgresql.org/docs/devel/error-style-guide.html

Best Regards,
Hou zj

On Fri, Aug 18, 2023 at 6:16 PM Zhijie Hou (Fujitsu)
<houzj.fnst@fujitsu.com> wrote:
>
> On Friday, August 18, 2023 11:20 AM Amit Kapila <amit.kapila16@gmail.com> wrote:
> >
> > On Mon, Aug 14, 2023 at 12:08 PM Peter Smith <smithpb2250@gmail.com>
> > wrote:
> > >
> > > The main patch for adding the worker type enum has been pushed [1].
> > >
> > > Here is the remaining (rebased) patch for changing some previous
> > > cascading if/else to switch on the LogicalRepWorkerType enum instead.
> > >
> >
> > I see this as being useful if we plan to add more worker types. Does anyone else
> > see this remaining patch as an improvement?
>
> +1
>
> I have one comment for the new error message.
>
> +               case WORKERTYPE_UNKNOWN:
> +                       ereport(ERROR, errmsg_internal("should_apply_changes_for_rel: Unknown worker type"));
>
> I think reporting an ERROR in this case is fine. However, I would suggest
> refraining from mentioning the function name in the error message, as
> recommended in the error style document [1]. Also, it appears we could use
> elog() here.
>
> [1] https://www.postgresql.org/docs/devel/error-style-guide.html

OK. Modified as suggested. Anyway, getting these errors should not
even be possible, so I added a comment to emphasise that.

PSA v9

------
Kind Regards,
Peter Smith.
Fujitsu Australia

On Mon, Aug 21, 2023 at 5:34 AM Peter Smith <smithpb2250@gmail.com> wrote:
>
> On Fri, Aug 18, 2023 at 6:16 PM Zhijie Hou (Fujitsu)
> <houzj.fnst@fujitsu.com> wrote:
> >
> > On Friday, August 18, 2023 11:20 AM Amit Kapila <amit.kapila16@gmail.com> wrote:
> > >
> > > On Mon, Aug 14, 2023 at 12:08 PM Peter Smith <smithpb2250@gmail.com>
> > > wrote:
> > > >
> > > > The main patch for adding the worker type enum has been pushed [1].
> > > >
> > > > Here is the remaining (rebased) patch for changing some previous
> > > > cascading if/else to switch on the LogicalRepWorkerType enum instead.
> > > >
> > >
> > > I see this as being useful if we plan to add more worker types. Does anyone else
> > > see this remaining patch as an improvement?
> >
> > +1
> >
> > I have one comment for the new error message.
> >
> > +               case WORKERTYPE_UNKNOWN:
> > +                       ereport(ERROR, errmsg_internal("should_apply_changes_for_rel: Unknown worker type"));
> >
> > I think reporting an ERROR in this case is fine. However, I would suggest
> > refraining from mentioning the function name in the error message, as
> > recommended in the error style document [1]. Also, it appears we could use
> > elog() here.
> >
> > [1] https://www.postgresql.org/docs/devel/error-style-guide.html
>
> OK. Modified as suggested. Anyway, getting these errors should not
> even be possible, so I added a comment to emphasise that.
>
> PSA v9
>

LGTM. I'll push this tomorrow unless there are any more comments.

--
With Regards,
Amit Kapila.

On Mon, Aug 21, 2023 at 3:48 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
>
> On Mon, Aug 21, 2023 at 5:34 AM Peter Smith <smithpb2250@gmail.com> wrote:
> >
> > PSA v9
> >
>
> LGTM. I'll push this tomorrow unless there are any more comments.
>

Pushed.

--
With Regards,
Amit Kapila.