Re: executor relation handling

Hi,

On Sun, Sep 30, 2018 at 7:18 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>
> I think that the call sites should ultimately look like
>
>         Assert(CheckRelationLockedByMe(...));
>
> but for hunting down the places where the assertion currently fails,
> it's more convenient if it's just an elog(WARNING).

I just hit one of the asserts (in relation_open()) added in
b04aeb0a053.  Here's a simple reproducer:

DROP TABLE IF EXISTS test;
CREATE TABLE test (id integer primary key);
PREPARE s AS DELETE FROM test WHERE id = 1;
EXECUTE s;
EXECUTE s;


This comes from ExecInitIndexScan() and ExecInitBitmapIndexScan(),
which open the index without lock if the parent table is a target
relation:

    /*
     * Open the index relation.
     *
     * If the parent table is one of the target relations of the query, then
     * InitPlan already opened and write-locked the index, so we can avoid
     * taking another lock here.  Otherwise we need a normal reader's lock.
     */
    relistarget = ExecRelationIsTargetRelation(estate, node->scan.scanrelid);
    indexstate->iss_RelationDesc = index_open(node->indexid,
                                              relistarget ? NoLock :
AccessShareLock);

And digging into InitPlan() up to ExecInitModifyTable():

        /*
         * If there are indices on the result relation, open them and save
         * descriptors in the result relation info, so that we can add new
         * index entries for the tuples we add/update.  We need not do this
         * for a DELETE, however, since deletion doesn't affect indexes. Also,
         * inside an EvalPlanQual operation, the indexes might be open
         * already, since we share the resultrel state with the original
         * query.
         */
        if (resultRelInfo->ri_RelationDesc->rd_rel->relhasindex &&
            operation != CMD_DELETE &&
            resultRelInfo->ri_IndexRelationDescs == NULL)
            ExecOpenIndices(resultRelInfo,
                            node->onConflictAction != ONCONFLICT_NONE);


So, this is problematic with a cached plan on a DELETE query since the
lock on the target relation's index will never be acquired (the lock
is acquired on the first execution in get_relation_info()).  This
doesn't seem unsafe though, since DROP INDEX [CONCURRENTLY] will still
acquire lock on the index relation or query xid before dropping the
index.

I'm not sure of what's the best way to fix this problem.  I wanted to
modify ExecInitIndexScan() and ExecInitBitmapIndexScan() to acquire
the lock for a DELETE on the target relation, however I don't think
that we have that information at this point.  Maybe just
unconditionally acquire an AccessShareLock on the index in those
functions?