Re: Adding a LogicalRepWorker type field

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.