Re: Single transaction in the tablesync worker?

Amit Kapila <amit.kapila16@gmail.com>

From: Amit Kapila <amit.kapila16@gmail.com>
To: Peter Smith <smithpb2250@gmail.com>
Cc: Petr Jelinek <petr.jelinek@enterprisedb.com>, Ajin Cherian <itsajin@gmail.com>, PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
Date: 2021-02-06T05:07:51Z
Lists: pgsql-hackers
On Sat, Feb 6, 2021 at 6:22 AM Peter Smith <smithpb2250@gmail.com> wrote:
>
> On Sat, Feb 6, 2021 at 2:10 AM Petr Jelinek
> <petr.jelinek@enterprisedb.com> wrote:
> >
> > > +ReplicationSlotNameForTablesync(Oid suboid, Oid relid, char syncslotname[NAMEDATALEN])
> > > +{
> > > +     if (syncslotname)
> > > +             sprintf(syncslotname, "pg_%u_sync_%u", suboid, relid);
> > > +     else
> > > +             syncslotname = psprintf("pg_%u_sync_%u", suboid, relid);
> > > +
> > > +     return syncslotname;
> > > +}
> >
> > Given that we are now explicitly dropping slots, what happens here if we
> > have 2 different downstreams that happen to get same suboid and reloid,
> > will one of the drop the slot of the other one? Previously with the
> > cleanup being left to temp slot we'd at maximum got error when creating
> > it but with the new logic in LogicalRepSyncTableStart it feels like we
> > could get into situation where 2 downstreams are fighting over slot no?
> >

I think so. See, if the alternative suggested below works or if you
have any other suggestions for the same?

>
> The PG docs [1] says "there is only one copy of pg_subscription per
> cluster, not one per database". IIUC that means it is not possible for
> 2 different subscriptions to have the same suboid.
>

I think he is talking about two different clusters having separate
subscriptions but point to the same publisher. In different clusters,
we can get the same subid/relid. I think we need a cluster-wide unique
identifier to distinguish among different subscribers. How about using
the system_identifier stored in the control file (we can use
GetSystemIdentifier to retrieve it).  I think one concern could be
that adding that to slot name could exceed the max length of slot
(NAMEDATALEN -1) but I don't think that is the case here
(pg_%u_sync_%u_UINT64_FORMAT (3 + 10 + 6 + 10 + 20 + '\0')). Note last
is system_identifier in this scheme.

Do you guys think that works or let me know if you have any other
better idea? Petr, is there a reason why such an identifier is not
considered originally, is there any risk in it?

-- 
With Regards,
Amit Kapila.



Commits

  1. Fix relcache reference leak introduced by ce0fdbfe97.

  2. Fix Subscription test added by commit ce0fdbfe97.

  3. Allow multiple xacts during table sync in logical replication.

  4. Logical replication support for initial data copy