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
