Re: Replica Identity check of partition table on subscriber

On Tue, Jun 21, 2022 at 5:08 PM houzj.fnst@fujitsu.com
<houzj.fnst@fujitsu.com> wrote:
> On Tuesday, June 21, 2022 3:21 PM Amit Langote <amitlangote09@gmail.com> wrote:
> > Thanks for the patch.
> >
> > I agree it's an old bug.  A partition map entry's localrel may point
> > to a stale Relation pointer, because once the caller had closed the
> > relation, the relcache subsystem is free to "clear" it, like in the
> > case of a RELCACHE_FORCE_RELEASE build.
>
> Hi,
>
> Thanks for replying.
>
> > Fixing it the way patch does seems fine, though it feels like
> > localrelvalid will lose some of its meaning for the partition map
> > entries -- we will now overwrite localrel even if localrelvalid is
> > true.
>
> To me, it seems localrelvalid doesn't have the meaning that the cached relation
> pointer is valid. In logicalrep_rel_open(), we also reopen and update the
> relation even if the localrelvalid is true.

Ah, right.  I guess only the localrelvalid=false case is really
interesting then.  Only in that case, we need to (re-)build other
fields that are computed using localrel.  In the localrelvalid=true
case, we don't need to worry about other fields, but still need to
make sure that localrel points to an up to date relcache entry of the
relation.

> > +   /*
> > +    * Relation is opened and closed by caller, so we need to always update the
> > +    * partrel in case the cached relation was closed.
> > +    */
> > +   entry->localrel = partrel;
> > +
> > +   if (entry->localrelvalid)
> >         return entry;
> >
> > Maybe we should add a comment here about why it's okay to overwrite
> > localrel even if localrelvalid is true.  How about the following hunk:
> >
> > @@ -596,8 +596,20 @@ logicalrep_partition_open(LogicalRepRelMapEntry
> > *root,
> >
> >     entry = &part_entry->relmapentry;
> >
> > +   /*
> > +    * We must always overwrite entry->localrel with the latest partition
> > +    * Relation pointer, because the Relation pointed to by the old value may
> > +    * have been cleared after the caller would have closed the partition
> > +    * relation after the last use of this entry.  Note that localrelvalid is
> > +    * only updated by the relcache invalidation callback, so it may still be
> > +    * true irrespective of whether the Relation pointed to by localrel has
> > +    * been cleared or not.
> > +    */
> >     if (found && entry->localrelvalid)
> > +   {
> > +       entry->localrel = partrel;
> >         return entry;
> > +   }
> >
> > Attached a patch containing the above to consider as an alternative.
>
> This looks fine to me as well.

Thank you.

--
Thanks, Amit Langote
EDB: http://www.enterprisedb.com