Re: Single transaction in the tablesync worker?

On Tue, Jan 5, 2021 at 3:32 PM Peter Smith <smithpb2250(at)gmail(dot)com> wrote:
>
> On Mon, Jan 4, 2021 at 10:48 PM Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
> >
> > Few more comments on v9:
> > ======================
> > 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.

>
> > 2.
> > + /*
> > + * Tablesync resource cleanup (slots and origins).
> > + *
> > + * Any READY-state relations would already have dealt with clean-ups.
> > + */
> > + {
> >
> > There is no need to start a separate block '{' here.
>
> Written this way so I can declare variables only at the scope they are
> needed. I didn’t see anything in the PG code conventions discouraging
> doing this practice: https://www.postgresql.org/docs/devel/source.html
>

But, do we encourage such a coding convention to declare variables. I
find it difficult to read such a code. I guess as a one-off we can do
this but I don't see a compelling need here.

> > 3.
> > +#define SUBREL_STATE_COPYDONE 'C' /* tablesync copy phase is completed */
> >
> > You can mention in the comments that sublsn will be NULL for this
> > state as it is mentioned for other similar states. Can we think of
> > using any letter in lower case for this as all other states are in
> > lower-case except for this which makes it a look bit odd? We can use
> > 'f' or 'e' and describe it as 'copy finished' or 'copy end'. I am fine
> > if you have any better ideas.
> >
>
> Fixed in latest patch [v11]
>

It is still not reflected in the docs. See below:
--- a/doc/src/sgml/catalogs.sgml
+++ b/doc/src/sgml/catalogs.sgml
@@ -7651,6 +7651,7 @@ SCRAM-SHA-256$<replaceable>&lt;iteration
count&gt;</replaceable>:<replaceable>&l
State code:
<literal>i</literal> = initialize,
<literal>d</literal> = data is being copied,
+ <literal>C</literal> = table data has been copied,
<literal>s</literal> = synchronized,

--
With Regards,
Amit Kapila.