On Monday, June 13, 2022 1:53 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
>
> 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
> > attrmap->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.
Thanks for the comments.
I have separated out the bug-fix for the subscriber-side.
And fix the typo and function name.
Attach the new version patch set.
Best regards,
Hou zj