Re: Relcache refactoring

On Wed, Jun 5, 2024 at 9:56 PM Heikki Linnakangas <hlinnaka@iki.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.