Re: Skipping logical replication transactions on subscriber side

On Tue, Aug 3, 2021 at 7:54 PM vignesh C <vignesh21(at)gmail(dot)com> wrote:
>
> On Tue, Aug 3, 2021 at 12:20 PM Masahiko Sawada <sawada(dot)mshk(at)gmail(dot)com> wrote:
> >
> > On Mon, Aug 2, 2021 at 12:21 PM Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
> > >
> > > On Mon, Aug 2, 2021 at 7:45 AM Masahiko Sawada <sawada(dot)mshk(at)gmail(dot)com> wrote:
> > > >
> > > > On Fri, Jul 30, 2021 at 12:52 PM Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
> > > > >
> > > > > On Thu, Jul 29, 2021 at 11:18 AM Masahiko Sawada <sawada(dot)mshk(at)gmail(dot)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";
>

Thank you for reviewing the patch!

I agreed with all comments and will fix them in the next version patch.

Regards,

--
Masahiko Sawada
EDB: https://www.enterprisedb.com/