On Mon, Jan 25, 2021 at 6:15 AM Peter Smith <smithpb2...@gmail.com> wrote:
>
> On Sun, Jan 24, 2021 at 5:54 PM Peter Smith <smithpb2...@gmail.com> wrote:
> > > 4.
> > > - /*
> > > - * To build a slot name for the sync work, we are limited to NAMEDATALEN
> > > -
> > > - * 1 characters. We cut the original slot name to NAMEDATALEN - 28 chars
> > > - * and append _%u_sync_%u (1 + 10 + 6 + 10 + '\0'). (It's actually the
> > > - * NAMEDATALEN on the remote that matters, but this scheme will also work
> > > - * reasonably if that is different.)
> > > - */
> > > - StaticAssertStmt(NAMEDATALEN >= 32, "NAMEDATALEN too small"); /* for
> > > sanity */
> > > - slotname = psprintf("%.*s_%u_sync_%u",
> > > - NAMEDATALEN - 28,
> > > - MySubscription->slotname,
> > > - MySubscription->oid,
> > > - MyLogicalRepWorker->relid);
> > > + /* Calculate the name of the tablesync slot. */
> > > + slotname = ReplicationSlotNameForTablesync(
> > > + MySubscription->oid,
> > > + MyLogicalRepWorker->relid);
> > >
> > > What is the reason for changing the slot name calculation? If there is
> > > any particular reasons, then we can add a comment to indicate why we
> > > can't include the subscription's slotname in this calculation.
> > >
> >
> > The subscription slot name may be changed (e.g. ALTER SUBSCRIPTION)
> > and so including the subscription slot name as part of the tablesync
> > slot name was considered to be:
> > a) possibly risky/undefined, if the subscription slot_name = NONE
> > b) confusing, if we end up using 2 different slot names for the same
> > tablesync (e.g. if the subscription slot name is changed before a sync
> > worker is re-launched).
> > And since this subscription slot name part is not necessary for
> > uniqueness anyway, it was removed from the tablesync slot name to
> > eliminate those concerns.
> >
> > Also, the tablesync slot name calculation was encapsulated as a
> > separate function because previously (i.e. before v18) it was used by
> > various other cleanup codes. I still like it better as a function, but
> > now it is only called from one place so we could put that code back
> > inline if you prefer it how it was..
>
> It turns out those (a/b) concerns I wrote above are maybe unfounded,
> because it seems not possible to alter the slot_name = NONE unless the
> subscription is first DISABLED.
>
Yeah, but I think the user can still change to some other predefined
slot_name. However, I guess it doesn't matter unless it can lead what
you have mentioned in (a). As that can't happen, it is probably better
to take out that change from the patch. I see your point of moving
this calculation to a separate function but not sure if it is worth it
unless we have to call it from multiple places or it simplifies the
existing code.
--
With Regards,
Amit Kapila.
