Re: Inadequate executor locking of indexes

On 2019/03/14 7:12, David Rowley wrote:
> Thanks for having a look at this.
>
> On Wed, 13 Mar 2019 at 22:45, Amit Langote
> <Langote_Amit_f8(at)lab(dot)ntt(dot)co(dot)jp> wrote:
>> I have one question about the relation between idxlockmode and
>> rellockmode? From skimming the patch, it appears that they're almost
>> always set to the same value. If so, why not use rellockmode for index
>> locking too?
>
> Maybe that's possible, but it would mean giving up on locking indexes
> during DELETE with AccessShareLock. That would become
> RowExclusiveLock. Also FOR UPDATE would lock indexes with RowShareLock
> instead of AccessShareLock.

If the correct lock is taken in both cases by the current code, then maybe
there's no need to change anything? What does idxlockmode improve about
the existing situation? One thing I can imagine it improves is that we
don't need the potentially expensive ExecRelationIsTargetRelation() check
anymore, because idxlockmode gives that information for free.

>>> #2 would not address Tom's mention of there possibly being some way to
>>> have the index scan node initialise before the modify table node
>>> (currently we pass NoLock for indexes belonging to result rels in the
>>> index scan nodes). I can't quite imagine at the moment how that's
>>> possible, but maybe my imagination is just not good enough. We could
>>> fix that by passing RowExclusiveLock instead of NoLock. It just means
>>> we'd discover the lock already exists in the local lock table...
>>> unless of course there is a case where the index scan gets initialised
>>> before modify table is.
>>
>> Tom gave an example upthread that looked like this:
>
> [...]
>
>> But if finalize_lockmodes() in your patch set lockmodes correctly,
>> ExecInitBitmapIndexScan() called for x1 ought to lock the index with
>> RowExclusiveLock, no?
>
> Yeah, in the executor the bitmap index scan uses a RowExclusiveLock.
> There's still the issue in the planner that get_relation_info() will
> still take an AccessShareLock when planning the 1st CTE and then
> upgrade it to RowExclusiveLock once it gets to the 2nd. It's not
> really very obvious how that can be fixed, but at least we don't start
> using indexes without any locks.

If we're to fix that upgrade hazard, maybe we'll need a separate phase to
determine the strongest lock to take on each table referenced in different
parts of the query (across CTEs) that runs before
pg_analyze_and_rewrite()? We surely can't use table OIDs as keys though.

Thanks,
Amit