Re: Adding a LogicalRepWorker type field

On Wed, Aug 2, 2023 at 12:14 PM Peter Smith <smithpb2250(at)gmail(dot)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