RE: Replica Identity check of partition table on subscriber

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).

2)
    if (entry->attrmap)
        pfree(entry->attrmap);

I think we should use free_attrmap instead of pfree here to avoid memory leak.
And we also need to check the attrmap in logicalrep_rel_open() and
logicalrep_partition_open() and free it if needed. I am not sure shall we put this
in the 0001 patch, so attach a separate patch for this. We can merge later this if needed.

Best regards,
Hou zj