Re: Skipping logical replication transactions on subscriber side

On Thu, Aug 26, 2021 at 3:09 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
>
> On Thu, Aug 26, 2021 at 9:50 AM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
> >
> > On Thu, Aug 26, 2021 at 12:51 PM Amit Kapila <amit.kapila16@gmail.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:

Thank you for the comments!

> 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.

Agreed to check 'remote_attnum' inside "if(errargrel)".

>
> 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.

On Thu, Aug 26, 2021 at 3:30 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
>
> 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.

Yes, that's why I reset the apply_error_callback_arg.rel before
logicalrep_rel_close(), not at the end of the function.

Since the callback function refers to apply_error_callback_arg.rel it
still needs to be valid when an error occurs. Moving it to the end of
the function is no problem for now, but if we always reset relation
info as the last thing, I think that we cannot allow adding changes
between setting relation info and the end of the function (i.g.,
resetting relation info) that could lead to invalidate fields of
apply_error_callback_arg.rel (e.g, freeing a string value etc).

> Note - I can take care of the above points based on whatever we agree
> with, you don't need to send a new version for this.

Thanks!

Regards,

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