Horton

RE: Adding a LogicalRepWorker type field

Zhijie Hou (Fujitsu) <houzj.fnst@fujitsu.com>

View thread View on postgr.es
From: "Zhijie Hou (Fujitsu)" <houzj.fnst@fujitsu.com>
To: Amit Kapila <amit.kapila16@gmail.com>, Peter Smith <smithpb2250@gmail.com>
Cc: Ashutosh Bapat <ashutosh.bapat.oss@gmail.com>, Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com>, PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
Date: 2023-08-18T08:16:41Z
Lists: pgsql-hackers 
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

Commits

  1. Simplify the logical worker type checks by using the switch on worker type.

  2. Simplify determining logical replication worker types.