Re: Relcache refactoring

From: Heikki Linnakangas <hlinnaka(at)iki(dot)fi>
To: jian he <jian(dot)universality(at)gmail(dot)com>
Cc: pgsql-hackers <pgsql-hackers(at)postgresql(dot)org>, Alexander Lakhin <exclusion(at)gmail(dot)com>
Subject: Re: Relcache refactoring
Date: 2024-10-31 08:14:19
Message-ID: ddb86292-49be-4ec8-b38e-918fb24c2cec@iki.fi
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

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)

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Anthonin Bonnefoy 2024-10-31 08:23:47 Re: Segfault in jit tuple deforming on arm64 due to LLVM issue
Previous Message Soumyadeep Chakraborty 2024-10-31 08:03:06 Re: InstallXLogFileSegment() vs concurrent WAL flush