On Mon, 11 Feb 2019 at 01:22, Julien Rouhaud <rjuju...@gmail.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. > > 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. -- David Rowley http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services
