From: | David Rowley <david(dot)rowley(at)2ndquadrant(dot)com> |
---|---|
To: | Amit Langote <Langote_Amit_f8(at)lab(dot)ntt(dot)co(dot)jp> |
Cc: | Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>, Julien Rouhaud <rjuju123(at)gmail(dot)com>, Robert Haas <robertmhaas(at)gmail(dot)com>, Andres Freund <andres(at)anarazel(dot)de>, Thomas Munro <thomas(dot)munro(at)gmail(dot)com>, Amit Kapila <amit(dot)kapila16(at)gmail(dot)com>, Haribabu Kommi <kommi(dot)haribabu(at)gmail(dot)com>, 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-04-04 13:22:42 |
Message-ID: | CAKJS1f9JLYyTLb==1gRtFe2QU7pMWnqhug_bt03aCAEUT7Gerg@mail.gmail.com |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
On Thu, 4 Apr 2019 at 15:35, Amit Langote <Langote_Amit_f8(at)lab(dot)ntt(dot)co(dot)jp> wrote:
> BTW, get_actual_variable_range is static to selfuncs.c and other public
> functions that are entry point to get_actual_variable_range's
> functionality appear to require having built a RelOptInfo first, which
> means (even for third party code) having gone through get_relation_info
> and opened the indexes. That may be too much wishful thinking though.
I think we should do this. We have the CheckRelationLockedByMe Asserts
to alert us if this ever gets broken. I think even if something added
a get_relation_info_hook to alter the indexes somehow, say to remove
one then it should still be okay as get_actual_variable_range() only
looks at index that are in that list and the other two functions are
dealing with Paths, which couldn't exist for an index that was removed
from RelOptInfo->indexlist.
> >> Also, I noticed that there is infer_arbiter_indexes() too, which opens the
> >> target table's indexes with RowExclusiveLock. I thought for a second
> >> that's a index-locking site in the planner that you may have missed, but
> >> turns out it might very well be the first time those indexes are locked in
> >> a given insert query's processing, because query_planner doesn't need to
> >> plan access to the result relation, so get_relation_info is not called.
> >
> > I skimmed over that and thought that the rellockmode would always be
> > RowExclusiveLock anyway, so didn't change it. However, it would make
> > sense to use rellockmode for consistency. We're already looking up the
> > RangeTblEntry to get the relid, so getting the rellockmode is about
> > free.
>
> Yeah, maybe it'd be a good idea, if only for consistency, to fetch the
> rellockmode of the resultRelation RTE and use it for index_open.
I've changed infer_arbiter_indexes() too, but decided to use
rellockmode rather than NoLock since we're not using
RelOptInfo->indexlist. If anything uses get_relation_info_hook to
remove indexes and then closes removed ones releasing the lock, then
we could end up with problems here.
v2 attached.
--
David Rowley http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services
Attachment | Content-Type | Size |
---|---|---|
use_rellockmode_for_indexes_too_v2.patch | application/octet-stream | 10.3 KB |
From | Date | Subject | |
---|---|---|---|
Next Message | David Rowley | 2019-04-04 13:25:21 | Re: Inadequate executor locking of indexes |
Previous Message | Julien Rouhaud | 2019-04-04 13:06:47 | Re: Re: reloption to prevent VACUUM from truncating empty pages at the end of relation |