Re: Hitting CheckRelationLockedByMe() ASSERT with force_generic_plan

On Fri, Nov 23, 2018 at 3:33 AM David Rowley <david(dot)rowley(at)2ndquadrant(dot)com>
wrote:

> On Thu, 22 Nov 2018 at 22:33, Rushabh Lathia <rushabh(dot)lathia(at)gmail(dot)com>
> wrote:
> > 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
>
>
> > 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.
>
> I'd say the following comment and code in nodeIndexscan.c is outdated:
>
> /*
> * 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);
>
> Despite what the above comment claims, these indexes have not been
> opened in InitPlan since 389af951552ff2. As you mentioned, they're
> opened in nodeModifyTable.c for non-delete statements.
>
>
+1.

I tried the same approach and with further testing I haven't found
any issues.

> I think we either need to update the above code to align it to what's
> now in nodeModifyTable.c, or just obtain an AccessShareLock
> regardless. I kinda think we should just take the lock regardless as
> determining if the relation is a result relation may be much more
> expensive than just taking another lower-level lock on the index.
>
> Anyway. I've attached a small patch to update the above fragment.
>
> --
> David Rowley http://www.2ndQuadrant.com/
> PostgreSQL Development, 24x7 Support, Training & Services
>

--
Rushabh Lathia