Re: executor relation handling

Hi,

On Sun, Sep 30, 2018 at 7:18 PM Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)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?