Re: Relcache refactoring

On 23/09/2024 04:39, jian he wrote:
> static void
> RelationClearRelation(Relation relation)
> {
>      Assert(RelationHasReferenceCountZero(relation));
>      Assert(!relation->rd_isnailed);
> 
>      /*
>       * Relations created in the same transaction must never be removed, see
>       * RelationFlushRelation.
>       */
>      Assert(relation->rd_createSubid == InvalidSubTransactionId);
>      Assert(relation->rd_firstRelfilelocatorSubid == InvalidSubTransactionId);
>      Assert(relation->rd_droppedSubid == InvalidSubTransactionId);
> 
>      /* Ensure it's closed at smgr level */
>      RelationCloseSmgr(relation);
> 
>      /* Free AM cached data, if any */
>      if (relation->rd_amcache)
>          pfree(relation->rd_amcache);
>      relation->rd_amcache = NULL;
> 
>      /* Mark it as invalid (just pro forma since we will free it below) */
>      relation->rd_isvalid = false;
> 
>      /* Remove it from the hash table */
>      RelationCacheDelete(relation);
> 
>      /* And release storage */
>      RelationDestroyRelation(relation, false);
> }
> 
> 
> can be simplified as
> 
> 
> static void
> RelationClearRelation(Relation relation)
> {
>      ---bunch of Asserts
> 
>     /* first mark it as invalid */
>     RelationInvalidateRelation(relation);
> 
>      /* Remove it from the hash table */
>      RelationCacheDelete(relation);
> 
>      /* And release storage */
>      RelationDestroyRelation(relation, false);
> }
> ?
> 
> 
> in RelationRebuildRelation
> we can also use RelationInvalidateRelation?

Yep, that makes sense, changed them that way.

>   *    We assume that at the time we are called, we have at least AccessShareLock
>   *    on the target index.  (Note: in the calls from RelationClearRelation,
>   *    this is legitimate because we know the rel has positive refcount.)
> 
> calling RelationClearRelation, rel->rd_refcnt == 0
> seems conflicted with the above comments in RelationReloadIndexInfo.
> so i am confused with the above comments.

That comment is outdated, because after these patches, there's only once 
caller, in RelationRebuildRelation. Fixed that, thanks for noticing!

Hmm, in case of a nailed index, this will get called with refcnt == 1, 
even though we have no AccessShareLock on the index. But I guess that's 
OK for a nailed index.

Committed with those fixes, thanks for the review!

-- 
Heikki Linnakangas
Neon (https://neon.tech)