Re: Inadequate executor locking of indexes

On Mon, Feb 11, 2019 at 5:32 AM David Rowley
<david(dot)rowley(at)2ndquadrant(dot)com> wrote:
>
> On Mon, 11 Feb 2019 at 01:22, Julien Rouhaud <rjuju123(at)gmail(dot)com> wrote:
> > 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.
>
> Good catch. I'm a bit stuck on the best way to fix this. So far I can
> only think of, either;
>
> 1. Adding a new field to RangeTblEntry to indicate the operation type
> that's being performed on the relation; or
> 2. Adding a Bitmapset field to PlannerGlobal that sets the rtable
> indexes of RangeTblEntry items that belong to DELETEs and ignore these
> when setting resultRelids in finalize_lockmodes().
>
> For #2, the only place I can see to do this is
> add_rtes_to_flat_rtable(), which would require either passing the
> PlannerInfo into the function, or at least its parse's commandType.
>
> I don't really like either, but don't have any other ideas at the moment.

But we would still need the same lock level upgrade logic on indexes
for cases like CTE with a mix of INSERT, UPDATE and DELETE on the same
relation I think. #1 seems like a better solution to me.

> > > 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?
>
> No, I don't think so. The "used" index(es) will be opened in the scan
> node(s). The reason I didn't like it much is that I wanted to keep
> the logic for deciding what lock level to use in the planner. The
> executor seems to have more knowledge than I think maybe it should.

Ah, I see. Thanks.