Hi, On Tue, Feb 5, 2019 at 5:16 AM David Rowley <david.row...@2ndquadrant.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?
