Re: Skipping logical replication transactions on subscriber side

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.

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.

--
With Regards,
Amit Kapila.