Re: Added schema level support for publication. - Mailing list pgsql-hackers

On Fri, Aug 6, 2021 at 4:39 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
>
> On Fri, Aug 6, 2021 at 2:02 PM vignesh C <vignesh21@gmail.com> wrote:
> >
> > On Wed, Aug 4, 2021 at 4:10 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
> > >
> > > On Tue, Aug 3, 2021 at 8:38 PM vignesh C <vignesh21@gmail.com> wrote:
> >
> > > 6.
> > > + {PublicationSchemaRelationId, /* PUBLICATIONSCHEMAMAP */
> > > + PublicationSchemaPsnspcidPspubidIndexId,
> > > + 2,
> > > + {
> > > + Anum_pg_publication_schema_psnspcid,
> > > + Anum_pg_publication_schema_pspubid,
> > > + 0,
> > > + 0
> > > + },
> > >
> > > Why don't we keep pubid as the first column in this index?
> >
> > I wanted to keep it similar to PUBLICATIONRELMAP, should we keep it as
> > it is, thoughts?
> >
>
> Okay, I see your point. I think for PUBLICATIONRELMAP, we need it
> because it is searched using the only relid in
> GetRelationPublications, so, similarly, in the patch, you are using
> schema_oid in GetSchemaPublications, so probably that will help. I was
> wondering why you haven't directly used the cache in
> GetSchemaPublications similar to GetRelationPublications?

Both of the approaches work, I was not sure which one is better, If
the approach in GetRelationPublications is better, I will change it to
something similar to GetRelationPublications. Thoughts?

It seems
> there is a danger for concurrent object drop. Can you please check how
> the safety is ensured say when either one wants to drop the
> corresponding relation/schema or publication?

If a table is dropped concurrently from another session during logical
replication of some operation in that table, while we get
get_rel_sync_entry the cache invalidations(rel_sync_cache_relation_cb)
happen.  The cache entry will be marked as false, also the schema_sent
will be marked as false. It will resend the relation using the
relation that was prepared while processing this transaction from
ReorderBufferProcessTXN. I felt this is ok since the relation is
dropped after the operation on the table. Similarly if the publication
is dropped concurrently from another session during logical
replication of some operation in that table, while we get
get_rel_sync_entry the cache
invalidations(publication_invalidation_cb) happen. The publications
will be reloaded and validated again, the data will be replicated to
the server. I felt this behavior is fine since the publication is
dropped after the operation on the table.

 Another point is why
> can't we use the other index (where the index is on relid or
> schema_oid (PublicationSchemaObjectIndexId))?

I felt this cannot be used because In this case the index is in the
oid column of pg_publication_schema and not on psnspcid column.

Regards,
Vignesh