Re: Single transaction in the tablesync worker?

On Wed, Jan 6, 2021 at 4:32 AM Peter Smith <smithpb2250(at)gmail(dot)com> wrote:
>
> 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.
>

Okay, if we want to go that way then we should add some documentation
about it. Currently, the slot name used by apply worker is known to
the user because either it is specified by the user or the default is
subscription name, so the user can manually remove that slot later but
that is not true for tablesync slots. I think we need to update both
the Drop Subscription page [1] and logical-replication-subscription
page [2] where we have mentioned temporary slots and in the end "Here
are some scenarios: .." to mention about these slots and probably how
their names are generated so that in such special situations users can
drop them manually.

[1] - https://www.postgresql.org/docs/devel/sql-dropsubscription.html
[2] - https://www.postgresql.org/docs/devel/logical-replication-subscription.html

--
With Regards,
Amit Kapila.