Re: Single transaction in the tablesync worker?

On Tue, Jan 5, 2021 at 10:41 PM Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
> > > 1.
> > > + /* Drop the tablesync slot. */
> > > + {
> > > + char *syncslotname = ReplicationSlotNameForTablesync(subid, relid);
> > > +
> > > + /*
> > > + * If the subscription slotname is NONE/NULL and the connection to publisher is
> > > + * broken, but the DropSubscription should still be allowed to complete.
> > > + * But without a connection it is not possible to drop any tablesync slots.
> > > + */
> > > + if (!wrconn)
> > > + {
> > > + /* FIXME - OK to just log a warning? */
> > > + elog(WARNING, "!!>> DropSubscription: no connection. Cannot drop
> > > tablesync slot \"%s\".",
> > > + syncslotname);
> > > + }
> > >
> > > Why is this not an ERROR? We don't want to keep the table slots
> > > lingering after DropSubscription. If there is any tablesync slot that
> > > needs to be dropped and the publisher is not available then we should
> > > raise an error.
> >
> > Previously there was only the subscription slot. If the connection was
> > broken and caused an error then it was still possible for the user to
> > disassociate the subscription from the slot using ALTER SUBSCRIPTION
> > ... SET (slot_name = NONE). And then (when the slotname is NULL) the
> > DropSubscription could complete OK. I expect in that case the Admin
> > still had some slot clean-up they would need to do on the Publisher
> > machine.
> >
>
> I think such an option could probably be used for user-created slots
> but it would be difficult for even Admin to know about these
> internally created slots associated with the particular subscription.
> I would say it is better to ERROR out.

I am having doubts that ERROR is the best choice here. There is a long
note in https://www.postgresql.org/docs/devel/sql-dropsubscription.html
which describes this problem for the subscription slot and how to
disassociate the name to give a workaround “To proceed in this
situation”.

OTOH if we make the tablesync slot unconditionally ERROR for a broken
connection then there is no way to proceed, and the whole (slot_name =
NONE) workaround becomes ineffectual. Note - the current patch code is
only logging when the user has already disassociated the slot name; of
course normally (when the slot name was not disassociated) table slots
will give ERROR for broken connections.

IMO, if the user has disassociated the slot name then they have
already made their decision that they REALLY DO want to “proceed in
this situation”. So I thought we should let them proceed.

What do you think?

----
Kind Regards,
Peter Smith.
Fujitsu Australia.