Re: Replica Identity check of partition table on subscriber

On Mon, Jun 13, 2022 at 6:14 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
> On Mon, Jun 13, 2022 at 2:20 PM Amit Langote <amitlangote09@gmail.com> wrote:
> > On Sat, Jun 11, 2022 at 10:36 AM Amit Kapila <amit.kapila16@gmail.com> wrote:
> > > On Fri, Jun 10, 2022 at 2:26 PM Amit Langote <amitlangote09@gmail.com> wrote:
> > > >
> > > > +logicalrep_partmap_invalidate
> > > >
> > > > I wonder why not call this logicalrep_partmap_update() to go with
> > > > logicalrep_relmap_update()?  It seems confusing to have
> > > > logicalrep_partmap_invalidate() right next to
> > > > logicalrep_partmap_invalidate_cb().
> > > >
> > >
> > > I am thinking about why we need to update the relmap in this new
> > > function logicalrep_partmap_invalidate()? I think it may be better to
> > > do it in logicalrep_partition_open() when actually required,
> > > otherwise, we end up doing a lot of work that may not be of use unless
> > > the corresponding partition is accessed. Also, it seems awkward to me
> > > that we do the same thing in this new function
> > > logicalrep_partmap_invalidate() and then also in
> > > logicalrep_partition_open() under different conditions.
> >
> > Both logicalrep_rel_open() and logicalrel_partition_open() only ever
> > touch the local Relation, never the LogicalRepRelation.
>
> We do make the copy of remote rel in  logicalrel_partition_open() when
> the entry is not found. I feel the same should happen when remote
> relation is reset/invalidated by the RELATION message.

Hmm, the problem is that a RELATION message will only invalidate the
LogicalRepRelation portion of the target parent's entry, while any
copies that have been made for partitions that were opened till that
point will continue to have the old LogicalRepRelation information.
As things stand, logicalrep_partition_open() won't know that the
parent entry's LogicalRepRelation may have been modified due to a
RELATION message.  It will reconstruct the entry only if the partition
itself was modified locally, that is, if
logicalrep_partman_invalidate_cb() was called on the partition.

> >  Updating the
> > latter is the responsibility of logicalrep_relmap_update(), which is
> > there to support handling of the RELATION message by
> > apply_handle_relation().  Given that we make a separate copy of the
> > parent's LogicalRepRelMapEntry for each partition to put into the
> > corresponding LogicalRepPartMapEntry, those copies must be updated as
> > well when a RELATION message targeting the parent's entry arrives.  So
> > it seems fine that the patch is making it the
> > logicalrep_relmap_update()'s responsibility to update the partition
> > copies using the new logicalrep_partition_invalidate/update()
> > subroutine.
>
> I think we can do that way as well but do you see any benefit in it?
> The way I am suggesting will avoid the effort of updating the remote
> rel copy till we try to access that particular partition.

I don't see any benefit as such to doing it the way the patch does,
it's just that that seems to be the only way to go given the way
things are.

This would have been unnecessary, for example, if the relation map
entry had contained a LogicalRepRelation pointer instead of the
struct.  The partition entries would point to the same entry as the
parent's if that were the case and there would be no need to modify
the partitions' copies explicitly.

Am I missing something?

--
Thanks, Amit Langote
EDB: http://www.enterprisedb.com