Re: Inadequate executor locking of indexes

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.

> > #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.

--
David Rowley http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services