Re: Inadequate executor locking of indexes

Hi,

On Tue, Feb 5, 2019 at 5:16 AM David Rowley
<david(dot)rowley(at)2ndquadrant(dot)com> wrote:
>
> I've changed a couple of things:
>
> 1. Changed nodeBitmapIndexscan.c now properly uses the RangeTblEntry's
> idxlockmode field.
> 2. Renamed a few variables in finalize_lockmodes().
>
> I'm keen to get some feedback if we should go about fixing things this
> way. One thing that's still on my mind is that the parser is still at
> risk of lock upgrade hazards. This patch only fixes the executor. I
> don't quite see how it would be possible to fix the same in the
> parser.

+ /*
+ * If there are multiple instances of the same rel with varying lock
+ * strengths then set the strongest lock level to each instance of
+ * that relation.
+ */
+ if (applystrongest)
[...]

The patch is quite straightforward, so I don't have general comments
on it. However, I think that the idxlockmode initialization is
problematic: you're using the statement's commandType so this doesn't
work with CTE. For instance, with this artificial query

WITH s AS (UPDATE test set id = 1 WHERE id =1) select 1;

will take an AccessShareLock on test's index while it should have an
RowExclusiveLock. I guess that you have to code similar lock upgrade
logic for the indexes, inspecting the planTree and subplans to find
the correct command type.

>
> I was also looking at each call site that calls ExecOpenIndices(). I
> don't think it's great that ExecInitModifyTable() has its own logic to
> skip calling that function for DELETE. I wondered if it shouldn't
> somehow depend on what the idxlockmode is set to.

I don't think that it's great either. However for DELETE we shouldn't
simply call ExecOpenIndices(), but open only the used indexes right?