On Saturday, June 11, 2022 9:36 AM Amit Kapila <amit.kapil...@gmail.com> wrote: > > On Fri, Jun 10, 2022 at 2:26 PM Amit Langote <amitlangot...@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
v3-0002-Check-partition-table-replica-identity-on-subscriber.patch
Description: v3-0002-Check-partition-table-replica-identity-on-subscriber.patch
v3-0003-fix-memory-leak-about-attrmap.patch
Description: v3-0003-fix-memory-leak-about-attrmap.patch
v3-0001-Fix-partition-map-cache-issues.patch
Description: v3-0001-Fix-partition-map-cache-issues.patch
