Re: Skipping logical replication transactions on subscriber side - Mailing list pgsql-hackers

On Tue, Aug 3, 2021 at 12:20 PM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
>
> On Mon, Aug 2, 2021 at 12:21 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
> >
> > On Mon, Aug 2, 2021 at 7:45 AM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
> > >
> > > On Fri, Jul 30, 2021 at 12:52 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
> > > >
> > > > On Thu, Jul 29, 2021 at 11:18 AM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
> > > >
> > > > Setting up logical rep error context in a generic function looks a bit
> > > > odd to me. Do we really need to set up error context here? I
> > > > understand we can't do this in caller but anyway I think we are not
> > > > sending this to logical replication view as well, so not sure we need
> > > > to do it here.
> > >
> > > Yeah, I'm not convinced of this part yet. I wanted to show relid also
> > > in truncate cases but I came up with only this idea.
> > >
> > > If an error happens during truncating the table (in
> > > ExecuteTruncateGuts()), relid set by
> > > set_logicalrep_error_context_rel() is actually sent to the view. If we
> > > don’t have it, the view always shows relid as NULL in truncate cases.
> > > On the other hand, it doesn’t cover all cases. For example, it doesn’t
> > > cover an error that the target table doesn’t exist on the subscriber,
> > > which happens when opening the target table. Anyway, in most cases,
> > > even if relid is NULL, the error message in the view helps users to
> > > know which relation the error happened on. What do you think?
> > >
> >
> > Yeah, I also think at this stage error message is sufficient in such cases.
>
> I've attached new patches that incorporate all comments I got so far.
> Please review them.

I had a look at the first patch, couple of minor comments:
1) Should we include this in typedefs.lst
+/* Struct for saving and restoring apply information */
+typedef struct ApplyErrCallbackArg
+{
+       LogicalRepMsgType command;      /* 0 if invalid */
+
+       /* Local relation information */
+       char       *nspname;

2)  We can keep the case statement in the same order as in the
LogicalRepMsgType enum, this will help in easily identifying if any
enum gets missed.
+               case LOGICAL_REP_MSG_RELATION:
+                       return "RELATION";
+               case LOGICAL_REP_MSG_TYPE:
+                       return "TYPE";
+               case LOGICAL_REP_MSG_ORIGIN:
+                       return "ORIGIN";
+               case LOGICAL_REP_MSG_MESSAGE:
+                       return "MESSAGE";
+               case LOGICAL_REP_MSG_STREAM_START:
+                       return "STREAM START";

Regards,
Vignesh