Re: Inadequate executor locking of indexes

On Tue, 19 Feb 2019 at 12:13, David Rowley <david(dot)rowley(at)2ndquadrant(dot)com> wrote:
>
> On Tue, 12 Feb 2019 at 09:58, Julien Rouhaud <rjuju123(at)gmail(dot)com> wrote:
> >
> > On Mon, Feb 11, 2019 at 5:32 AM David Rowley
> > <david(dot)rowley(at)2ndquadrant(dot)com> wrote:
> > > 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 think I'd rather find some way to do it that didn't denormalise the
> parse nodes like that. It seems very strange to have a CmdType in the
> Query struct, and then another set of them in RangeTblEntry. Besides
> bloating the size of the RangeTblEntry struct a bit, it also could
> lead to inconsistency bugs where the two CmdTypes differ, for some
> reason.

I had another go at this patch and fixed the problem by just setting
the idxlockmode inside the planner just before the call to
expand_inherited_tables(). This allows the lockmode to be copied into
child RTEs. Doing it later in planning is more of a problem since we
don't really store all the result relations in PlannerInfo, we only
store the one that was written in the query. The others are stored in
the ModifyTable path and then later in the ModifyTable plan. The only
other place I could see to do it (without adding an extra field in
PlannerInfo) was during createplan... which does not at all seem like
the right place, and is also too late for get_relation_info(), see
below.

The finalize_lockmodes() now does the same thing for idxlockmode as it
does for rellockmode, i.e, just set the strongest lock for each unique
rel oid.

However, during my work on this, I saw a few things that made me
wonder if all this is over-engineered and worth the trouble. The
first thing I saw was that in get_relation_info() we obtain a
RowExclusiveLock if the relation is the result relation. This means we
obtain a RowExclusiveLock for DELETE targets too. This is
inconsistent with what the executor tries to do and results in us
taking a weaker lock during execution than we do during planning. It
also means we don't bump into the lock in the local lock table which
results in slower locking. That problem would be exaggerated with
partitioned tables with a large number of partitions or inheritance
parents with lots of children, however, likely that'll be drowned out
by all the other slow stuff around there.

Another thing that's on my mind is that in the patch in
nodeModifyTable.c we still do:

if (resultRelInfo->ri_RelationDesc->rd_rel->relhasindex &&
operation != CMD_DELETE &&
resultRelInfo->ri_IndexRelationDescs == NULL)
ExecOpenIndices(resultRelInfo,
node->onConflictAction != ONCONFLICT_NONE);

i.e don't open the indexes for DELETEs. I had ideas that maybe this
could be changed to check the idxlockmode and open the indexes if it's
above AccessSharedLock. There didn't seem to be a very nice way to
fetch the RangeTblEntry from the ResultRelInfo though, so I didn't do
that, but leaving it as is does not really work well with the patch as
I really want the planner be the thing that decides what happens here.

My final thoughts were that I see a lot of work going on to implement
pluggable storage. I ended up having an offlist conversation with
Thomas Munro about zheap and what its requirements were for locking
indexes during deletes. It seems modifying the index during delete is
not something that happens with the current version, but is planned
for future versions. In any case, we can't really assume zheap is
going to be the only additional storage engine implementation that's
going to get hooked into this.

Current thoughts are, do we:

1. Allow the storage engine to communicate its locking needs via an
API function call and add code to check that in the
determine_index_lockmode function (new in the attached patch)
2. Just get rid of the "operation != CMD_DELETE &&" line in the above
code and just always lock indexes for DELETE targets in
RowExclusiveLock mode.

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

I'm adding Andres, Robert, Thomas, Amit and Haribabu due to the
involvement with pluggable storage and zheap.

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