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
