From: | David Rowley <david(dot)rowley(at)2ndquadrant(dot)com> |
---|---|
To: | Julien Rouhaud <rjuju123(at)gmail(dot)com> |
Cc: | Amit Langote <Langote_Amit_f8(at)lab(dot)ntt(dot)co(dot)jp>, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>, PostgreSQL Hackers <pgsql-hackers(at)lists(dot)postgresql(dot)org>, Rushabh Lathia <rushabh(dot)lathia(at)gmail(dot)com> |
Subject: | Re: Inadequate executor locking of indexes |
Date: | 2019-02-11 04:31:54 |
Message-ID: | CAKJS1f-sUgU2BLwfyv9ZQCR5cVbsOSBKDk--gCr6WGhGfJe2pQ@mail.gmail.com |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
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.
> > 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
From | Date | Subject | |
---|---|---|---|
Next Message | Noah Misch | 2019-02-11 05:13:49 | Re: Spurious "apparent wraparound" via SimpleLruTruncate() rounding |
Previous Message | Tom Lane | 2019-02-11 04:08:13 | Re: anole's failed timeouts test |