Hitting CheckRelationLockedByMe() ASSERT with force_generic_plan

Hi,

Commit b04aeb0a053e7cf7faad89f7d47844d8ba0dc839 add assertions that we hold
some relevant lock during relation open as opening a relation with no lock
at all is unsafe;

With above commit below test case is failing and hitting the newly added
Assert().

Test case:
===========

CREATE TABLE foo (x int primary key);
INSERT INTO foo VALUES (1), (2), (3), (4), (5);

CREATE OR REPLACE FUNCTION f1(a int) RETURNS int
AS $$
BEGIN
DELETE FROM foo where x = a;
return 0;
END;
$$ LANGUAGE plpgsql;

postgres(at)100858=#set plan_cache_mode = force_generic_plan;
SET
postgres(at)100858=#select f1(4);
f1
----
0
(1 row)

postgres(at)100858=#select f1(4);
server closed the connection unexpectedly
This probably means the server terminated abnormally
before or while processing the request.
The connection to the server was lost. Attempting reset: Failed.

Call stack:
=============

#0 0x00007f230b2d61d7 in raise () from /lib64/libc.so.6
#1 0x00007f230b2d78c8 in abort () from /lib64/libc.so.6
#2 0x0000000000a26ce5 in ExceptionalCondition (
conditionName=0xabdca8 "!(lockmode != 0 || (Mode ==
BootstrapProcessing) || CheckRelationLockedByMe(r, 1, 1))",
errorType=0xabc529 "FailedAssertion",
fileName=0xabc668 "heapam.c", lineNumber=1146) at assert.c:54
#3 0x00000000004dacd8 in relation_open (relationId=16387, lockmode=0) at
heapam.c:1144
#4 0x00000000004f94bf in index_open (relationId=16387, lockmode=0) at
indexam.c:155
#5 0x0000000000701403 in ExecInitIndexScan (node=0x1cf8fd8,
estate=0x1cec418, eflags=0) at nodeIndexscan.c:994
#6 0x00000000006dde31 in ExecInitNode (node=0x1cf8fd8, estate=0x1cec418,
eflags=0) at execProcnode.c:217
#7 0x000000000070d7b8 in ExecInitModifyTable (node=0x1cf8da8,
estate=0x1cec418, eflags=0) at nodeModifyTable.c:2190
#8 0x00000000006ddd39 in ExecInitNode (node=0x1cf8da8, estate=0x1cec418,
eflags=0) at execProcnode.c:174
#9 0x00000000006d48a5 in InitPlan (queryDesc=0x1cdc378, eflags=0) at
execMain.c:1024
#10 0x00000000006d36e9 in standard_ExecutorStart (queryDesc=0x1cdc378,
eflags=0) at execMain.c:265
#11 0x00000000006d3485 in ExecutorStart (queryDesc=0x1cdc378, eflags=0) at
execMain.c:147
#12 0x0000000000725880 in _SPI_pquery (queryDesc=0x1cdc378,
fire_triggers=true, tcount=0) at spi.c:2469
#13 0x00000000007252d4 in _SPI_execute_plan (plan=0x1cc4748,
paramLI=0x1ce23c8, snapshot=0x0, crosscheck_snapshot=0x0, read_only=false,
fire_triggers=true,
tcount=0) at spi.c:2235
#14 0x00000000007222a1 in SPI_execute_plan_with_paramlist (plan=0x1cc4748,
params=0x1ce23c8, read_only=false, tcount=0) at spi.c:516
#15 0x00007f23004a5f9a in exec_stmt_execsql (estate=0x7fff0bcaa4d0,
stmt=0x1cc78f0) at pl_exec.c:4114

Here DELETE statement pick the IndexScan plan, and later during execution
when it tries to open index relation, ExecInitIndexScan() assumes that index
relation should already hold require a lock. But in this case, that's not
the
case - which got exposed through the newly added ASSERT() for NoLock.

When query get's execute first time build_simple_rel() ->
get_relation_info(),
takes a lock on the indexes of a relation (this do happen from the
BuildCachePlan()). When we execute the same query second time, a plan is
caches
and CheckCachePlan() calls the AquireExecutorLocks() take the require locks
needed for execution of the caches plan. Here it takes a lock on the rtable
list - but doesn't take any lock on the index relation.

I observed that ExecInitModifyTable() to open the index relation if indices
on
the result relation. But here also - it doesn't do anything for CMD_DELETE:

/*
* 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);

Now, to fix this issue either we need to hold proper lock before reaching
to ExecInitIndexScan() or teach ExecInitIndexScan() to take AccessShareLock
on the scan coming from CMD_DELETE.

Thoughts/Comments?

Thanks,
Rushabh Lathia
www.EnterpriseDB.com