Re: Replica Identity check of partition table on subscriber - Mailing list pgsql-hackers

On Sat, Jun 11, 2022 at 2:36 PM houzj.fnst@fujitsu.com
<houzj.fnst@fujitsu.com> wrote:
>
> On Saturday, June 11, 2022 9: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.
> >
> > One more point about the 0001, it doesn't seem to have a test that validates
> > logicalrep_partmap_invalidate_cb() functionality. I think for that we need to Alter
> > the local table (table on the subscriber side). Can we try to write a test for it?
>
>
> Thanks for Amit. L and Amit. K for your comments ! I agree with this point.
> Here is the version patch set which try to address all these comments.
>
> In addition, when reviewing the code, I found some other related
> problems in the code.
>
> 1)
>                 entry->attrmap = make_attrmap(map->maplen);
>                 for (attno = 0; attno < entry->attrmap->maplen; attno++)
>                 {
>                         AttrNumber      root_attno = map->attnums[attno];
>
>                         entry->attrmap->attnums[attno] = attrmap->attnums[root_attno - 1];
>                 }
>
> In this case, It’s possible that 'attno' points to a dropped column in which
> case the root_attno would be '0'. I think in this case we should just set the
> entry->attrmap->attnums[attno] to '-1' instead of accessing the
> attrmap->attnums[]. I included this change in 0001 because the testcase which
> can reproduce these problems are related(we need to ALTER the partition on
> subscriber to reproduce it).
>

Hmm, this appears to be a different issue. Can we separate out the
bug-fix code for the subscriber-side issue caused by the DDL on the
subscriber?

Few other comments:
+ * Note that we don't update the remoterel information in the entry here,
+ * we will update the information in logicalrep_partition_open to save
+ * unnecessary work.
+ */
+void
+logicalrep_partmap_invalidate(LogicalRepRelation *remoterel)

/to save/to avoid

Also, I agree with Amit L. that it is confusing to have
logicalrep_partmap_invalidate() right next to
logicalrep_partmap_invalidate_cb() and both have somewhat different
kinds of logic. So, we can either name it as
logicalrep_partmap_reset_relmap() or logicalrep_partmap_update()
unless you have any other better suggestions? Accordingly, change the
comment atop this function.

--
With Regards,
Amit Kapila.