On Mon, Jan 27, 2020 at 01:44:13PM +0900, Kyotaro Horiguchi wrote:
> At Sun, 26 Jan 2020 20:22:01 -0800, Noah Misch <noah@leadboat.com> wrote in
> > Diffing the two latest versions of one patch:
> > > --- v32-0002-Fix-the-defect-1.patch 2020-01-18 14:32:47.499129940 -0800
> > > +++ v33-0002-Fix-the-defect-1.patch 2020-01-26 16:23:52.846391035 -0800
> > > +@@ -2978,8 +3054,8 @@ AssertPendingSyncs_RelationCache(void)
> > > + LOCKTAG_RELATION)
> > > + continue;
> > > + relid = ObjectIdGetDatum(locallock->tag.lock.locktag_field2);
> > > +- r = RelationIdGetRelation(relid);
> > > +- if (r == NULL)
> > > ++ r = RelationIdGetRelationCache(relid);
> >
> > The purpose of this loop is to create relcache entries for rels locked in the
> > current transaction. (The "r == NULL" case happens for rels no longer visible
> > in catalogs. It is harmless.) Since RelationIdGetRelationCache() never
> > creates a relcache entry, calling it defeats that purpose.
> > RelationIdGetRelation() is the right function to call.
>
> I thought that the all required entry exist in the cache but actually
> it's safer that recreate dropped caches. Does the following works?
>
> r = RelationIdGetRelation(relid);
> + /* if not found, fetch a "dropped" entry if any */
> + if (r == NULL)
> + r = RelationIdGetRelationCache(relid);
> if (r == NULL)
> continue;
That does not materially change the function's behavior. Notice that the
function does one thing with "r", which is to call RelationClose(r). The
function calls RelationIdGetRelation() for its side effects, not for its
return value.