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)