Re: Skipping logical replication transactions on subscriber side

From: Amit Kapila <amit(dot)kapila16(at)gmail(dot)com>
To: Masahiko Sawada <sawada(dot)mshk(at)gmail(dot)com>
Cc: Greg Nancarrow <gregn4422(at)gmail(dot)com>, "tanghy(dot)fnst(at)fujitsu(dot)com" <tanghy(dot)fnst(at)fujitsu(dot)com>, "osumi(dot)takamichi(at)fujitsu(dot)com" <osumi(dot)takamichi(at)fujitsu(dot)com>, "houzj(dot)fnst(at)fujitsu(dot)com" <houzj(dot)fnst(at)fujitsu(dot)com>, Alexey Lesovsky <lesovsky(at)gmail(dot)com>, Peter Eisentraut <peter(dot)eisentraut(at)enterprisedb(dot)com>, PostgreSQL-development <pgsql-hackers(at)postgresql(dot)org>
Subject: Re: Skipping logical replication transactions on subscriber side
Date: 2021-08-26 06:30:23
Message-ID: CAA4eK1JyMN5GU5amd22yxOLiRLZ300gZd-3sWvSbjeQfNphhtQ@mail.gmail.com
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

On Thu, Aug 26, 2021 at 11:39 AM Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
>
> On Thu, Aug 26, 2021 at 9:50 AM Masahiko Sawada <sawada(dot)mshk(at)gmail(dot)com> wrote:
> >
> > On Thu, Aug 26, 2021 at 12:51 PM Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
> >
> > Yeah, I agree that it's a handy way to detect missing a switch case
> > but I think that we don't necessarily need it in this case. Because
> > there are many places in the code where doing similar things and when
> > it comes to apply_dispatch() it's the entry function to handle the
> > incoming message so it will be unlikely that we miss adding a switch
> > case until the patch gets committed. If we don't move it, we would end
> > up either adding the code resetting the
> > apply_error_callback_arg.command to every message type, adding a flag
> > indicating the message is handled and checking later, or having a big
> > if statement checking if the incoming message type is valid etc.
> >
>
> I was reviewing and making minor edits to your v11-0001* patch and
> noticed that the below parts of the code could be improved:
> 1.
> + if (errarg->rel)
> + appendStringInfo(&buf, _(" for replication target relation \"%s.%s\""),
> + errarg->rel->remoterel.nspname,
> + errarg->rel->remoterel.relname);
> +
> + if (errarg->remote_attnum >= 0)
> + appendStringInfo(&buf, _(" column \"%s\""),
> + errarg->rel->remoterel.attnames[errarg->remote_attnum]);
>
> Isn't it better if 'remote_attnum' check is inside if (errargrel)
> check? It will be weird to print column information without rel
> information and in the current code, we don't set remote_attnum
> without rel. The other possibility could be to have an Assert for rel
> in 'remote_attnum' check.
>
> 2.
> + /* Reset relation for error callback */
> + apply_error_callback_arg.rel = NULL;
> +
> logicalrep_rel_close(rel, NoLock);
>
> end_replication_step();
>
> Isn't it better to reset relation info as the last thing in
> apply_handle_insert/update/delete as you do for a few other
> parameters? There is very little chance of error from those two
> functions but still, it will be good if they ever throw an error and
> it might be clear for future edits in this function that this needs to
> be set as the last thing in these functions.
>

I see that resetting it before logicalrep_rel_close has an advantage
that we might not accidentally access some information after close
which is not there in rel. I am not sure if that is the reason you
have in mind for resetting it before close.

--
With Regards,
Amit Kapila.

In response to

Browse pgsql-hackers by date

  From Date Subject
Next Message Denis Laxalde 2021-08-26 07:15:25 Re: [PATCH] Disable bgworkers during servers start in pg_upgrade
Previous Message Dilip Kumar 2021-08-26 06:17:34 Re: Separate out FileSet from SharedFileSet (was Re: pgsql: pgstat: Bring up pgstat in BaseInit() to fix uninitialized use o)