Re: Relcache refactoring

On Wed, Jun 5, 2024 at 9:56 PM Heikki Linnakangas <hlinnaka(at)iki(dot)fi> wrote:
>
> ## Patch 3: Split RelationClearRelation into three different functions
>
> RelationClearRelation() is complicated. Depending on the 'rebuild'
> argument and the circumstances, like if it's called in a transaction and
> whether the relation is an index, a nailed relation, a regular table, or
> a relation dropped in the same xact, it does different things:
>
> - Remove the relation completely from the cache (rebuild == false),
> - Mark the entry as invalid (rebuild == true, but not in xact), or
> - Rebuild the entry (rebuild == true).
>
> The callers have expectations on what they want it to do. Mostly the
> callers with 'rebuild == false' expect the entry to be removed, and
> callers with 'rebuild == true' expect it to be rebuilt or invalidated,
> but there are exceptions. RelationForgetRelation() for example sets
> rd_droppedSubid and expects RelationClearRelation() to then merely
> invalidate it, and the call from RelationIdGetRelation() expects it to
> rebuild, not merely invalidate it.
>
> I propose to split RelationClearRelation() into three functions:
>
> RelationInvalidateRelation: mark the relcache entry as invalid, so that
> it it is rebuilt on next access.
> RelationRebuildRelation: rebuild the relcache entry in-place.
> RelationClearRelation: Remove the entry from the relcache.
>
> This moves the responsibility of deciding the right action to the
> callers. Which they were mostly already doing. Each of those actions
> have different preconditions, e.g. RelationRebuildRelation() can only be
> called in a valid transaction, and RelationClearRelation() can only be
> called if the reference count is zero. Splitting them makes those
> preconditions more clear, we can have assertions to document them in each.
>
>
one minor issue.

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?

* 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.