Horton

Re: executor relation handling

Amit Langote <langote_amit_f8@lab.ntt.co.jp>

View thread View on postgr.es
From: Amit Langote <Langote_Amit_f8@lab.ntt.co.jp>
To: Tom Lane <tgl@sss.pgh.pa.us>, David Rowley <david.rowley@2ndquadrant.com>
Cc: Alvaro Herrera <alvherre@2ndquadrant.com>, Jesper Pedersen <jesper.pedersen@redhat.com>, Amit Langote <amitlangote09@gmail.com>, Pg Hackers <pgsql-hackers@postgresql.org>
Date: 2018-10-01T10:20:29Z
Lists: pgsql-hackers 
On 2018/10/01 2:18, Tom Lane wrote:
> I wrote:
>> 1. You set up transformRuleStmt to insert AccessExclusiveLock into
>> the "OLD" and "NEW" RTEs for a view.  This is surely wrong; we do
>> not want to take exclusive lock on a view just to run a query using
>> the view.  It should (usually, anyway) just be AccessShareLock.
>> However, because addRangeTableEntryForRelation insists that you
>> hold the requested lock type *now*, just changing the parameter
>> to AccessShareLock doesn't work.
>> I hacked around this for the moment by passing NoLock to
>> addRangeTableEntryForRelation and then changing rte->lockmode
>> after it returns, but man that's ugly.  It makes me wonder whether
>> addRangeTableEntryForRelation should be checking the lockmode at all.
> 
> It occurred to me that it'd be reasonable to insist that the caller
> holds a lock *at least as strong* as the one being recorded in the RTE,
> and that there's also been discussions about verifying that some lock
> is held when something like heap_open(foo, NoLock) is attempted.
> So I dusted off the part of 0001 that did that, producing the
> attached delta patch.
> 
> Unfortunately, I can't commit this, because it exposes at least two
> pre-existing bugs :-(.  So we'll need to fix those first, which seems
> like it should be a separate thread.  I'm just parking this here for
> the moment.
> 
> 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).

Should this check that we're not in a parallel worker process?

Thanks,
Amit

Commits

  1. Avoid O(N^2) cost in ExecFindRowMark().

  2. Remove some unnecessary fields from Plan trees.

  3. Restore sane locking behavior during parallel query.

  4. Remove more redundant relation locking during executor startup.

  5. In the executor, use an array of pointers to access the rangetable.

  6. Centralize executor's opening/closing of Relations for rangetable entries.

  7. Change executor to just Assert that table locks were already obtained.

  8. Change rewriter/planner/executor/plancache to depend on RTE rellockmode.

  9. Add assertions that we hold some relevant lock during relation open.

  10. Create an RTE field to record the query's lock mode for each relation.