Обсуждение: missing locking in at least INSERT INTO view WITH CHECK
Hi, Using the same debugging hack^Wpatch (0001) as in the matview patch (0002) an hour or so ago I noticed that INSERT INTO view WITH CHECK doesn't lock the underlying relations properly. I've attached a sort-of-working (0003) hack but I really doubt it's the correct approach, I don't really know enough about that area of the code. This looks like something that needs to be fixed. Also attached is 0004 which just adds a heap_lock() around a newly created temporary table in the matview code which shouldn't be required for correctness but gives warm and fuzzy feelings as well as less debugging noise. Wouldn't it be a good idea to tack such WARNINGs (in a proper and clean form) to index_open (checking the underlying relation is locked), relation_open(..., NoLock) (checking the relation has previously been locked) and maybe RelationIdGetRelation() when cassert is enabled? ISTM we frequently had bugs around this. Greetings, Andres Freund -- Andres Freund http://www.2ndQuadrant.com/PostgreSQL Development, 24x7 Support, Training & Services
On 2013-10-23 03:18:55 +0200, Andres Freund wrote: > (000[1-4]) Attached. Greetings, Andres Freund -- Andres Freund http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services
Вложения
On 23 October 2013 02:18, Andres Freund <andres@2ndquadrant.com> wrote: > Hi, > > Using the same debugging hack^Wpatch (0001) as in the matview patch > (0002) an hour or so ago I noticed that INSERT INTO view WITH CHECK > doesn't lock the underlying relations properly. > > I've attached a sort-of-working (0003) hack but I really doubt it's the > correct approach, I don't really know enough about that area of the > code. > This looks like something that needs to be fixed. > Hmm, my first thought is that rewriteTargetView() should be calling AcquireRewriteLocks() on viewquery, before doing too much with it. There may be sub-queries in viewquery's quals (and also now in its targetlist) and I don't think the relations referred to by those sub-queries are getting locked. I think that any code that is doing anything significant with a rule action's query needs to think about locking the query's relations. I did a quick search and the only suspicious code I found was the matview and auto-updatable view code. Regards, Dean > Also attached is 0004 which just adds a heap_lock() around a newly > created temporary table in the matview code which shouldn't be required > for correctness but gives warm and fuzzy feelings as well as less > debugging noise. > > Wouldn't it be a good idea to tack such WARNINGs (in a proper and clean > form) to index_open (checking the underlying relation is locked), > relation_open(..., NoLock) (checking the relation has previously been > locked) and maybe RelationIdGetRelation() when cassert is enabled? ISTM > we frequently had bugs around this. > > Greetings, > > Andres Freund > > -- > Andres Freund http://www.2ndQuadrant.com/ > PostgreSQL Development, 24x7 Support, Training & Services > > > -- > Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) > To make changes to your subscription: > http://www.postgresql.org/mailpref/pgsql-hackers
On 2013-10-23 20:51:27 +0100, Dean Rasheed wrote: > On 23 October 2013 02:18, Andres Freund <andres@2ndquadrant.com> wrote: > > Hi, > > > > Using the same debugging hack^Wpatch (0001) as in the matview patch > > (0002) an hour or so ago I noticed that INSERT INTO view WITH CHECK > > doesn't lock the underlying relations properly. > > > > I've attached a sort-of-working (0003) hack but I really doubt it's the > > correct approach, I don't really know enough about that area of the > > code. > > This looks like something that needs to be fixed. > > > > Hmm, my first thought is that rewriteTargetView() should be calling > AcquireRewriteLocks() on viewquery, before doing too much with it. > There may be sub-queries in viewquery's quals (and also now in its > targetlist) and I don't think the relations referred to by those > sub-queries are getting locked. Well, that wouldn't follow the currently documented rule ontop of QueryRewrite:* NOTE: the parsetree must either have come straight from the parser,* or have been scanned by AcquireRewriteLocksto acquire suitable locks. It might still be the right thing to do, but it seems suspicious that the rules need to be tweaked like that. > I think that any code that is doing anything significant with a rule > action's query needs to think about locking the query's relations. I > did a quick search and the only suspicious code I found was the > matview and auto-updatable view code. Yea, that were the locations the debugging patch cried on... Greetings, Andres Freund -- Andres Freund http://www.2ndQuadrant.com/PostgreSQL Development, 24x7 Support, Training & Services
On 23 October 2013 21:08, Andres Freund <andres@2ndquadrant.com> wrote: > On 2013-10-23 20:51:27 +0100, Dean Rasheed wrote: >> On 23 October 2013 02:18, Andres Freund <andres@2ndquadrant.com> wrote: >> > Hi, >> > >> > Using the same debugging hack^Wpatch (0001) as in the matview patch >> > (0002) an hour or so ago I noticed that INSERT INTO view WITH CHECK >> > doesn't lock the underlying relations properly. >> > >> > I've attached a sort-of-working (0003) hack but I really doubt it's the >> > correct approach, I don't really know enough about that area of the >> > code. >> > This looks like something that needs to be fixed. >> > >> >> Hmm, my first thought is that rewriteTargetView() should be calling >> AcquireRewriteLocks() on viewquery, before doing too much with it. >> There may be sub-queries in viewquery's quals (and also now in its >> targetlist) and I don't think the relations referred to by those >> sub-queries are getting locked. > > Well, that wouldn't follow the currently documented rule ontop > of QueryRewrite: > * NOTE: the parsetree must either have come straight from the parser, > * or have been scanned by AcquireRewriteLocks to acquire suitable locks. > > It might still be the right thing to do, but it seems suspicious that > the rules need to be tweaked like that. > Well it matches what already happens in other places in the rewriter --- see rewriteRuleAction() and ApplyRetrieveRule(). It's precisely because the rule action's query hasn't come from the parser that it needs to be processed in this way. Regards, Dean
On 2013-10-23 21:20:58 +0100, Dean Rasheed wrote: > On 23 October 2013 21:08, Andres Freund <andres@2ndquadrant.com> wrote: > > On 2013-10-23 20:51:27 +0100, Dean Rasheed wrote: > >> Hmm, my first thought is that rewriteTargetView() should be calling > >> AcquireRewriteLocks() on viewquery, before doing too much with it. > >> There may be sub-queries in viewquery's quals (and also now in its > >> targetlist) and I don't think the relations referred to by those > >> sub-queries are getting locked. > > > > Well, that wouldn't follow the currently documented rule ontop > > of QueryRewrite: > > * NOTE: the parsetree must either have come straight from the parser, > > * or have been scanned by AcquireRewriteLocks to acquire suitable locks. > > > > It might still be the right thing to do, but it seems suspicious that > > the rules need to be tweaked like that. > > > > Well it matches what already happens in other places in the rewriter > --- see rewriteRuleAction() and ApplyRetrieveRule(). It's precisely > because the rule action's query hasn't come from the parser that it > needs to be processed in this way. I really don't know that are of code that well, fortunately I never had to wade around it much so far... Reading your explanation and a bit of the code your plan sound sane. Are you going to propose a patch? Greetings, Andres Freund -- Andres Freund http://www.2ndQuadrant.com/PostgreSQL Development, 24x7 Support, Training & Services
On 24 October 2013 18:28, Andres Freund <andres@2ndquadrant.com> wrote: > On 2013-10-23 21:20:58 +0100, Dean Rasheed wrote: >> On 23 October 2013 21:08, Andres Freund <andres@2ndquadrant.com> wrote: >> > On 2013-10-23 20:51:27 +0100, Dean Rasheed wrote: >> >> Hmm, my first thought is that rewriteTargetView() should be calling >> >> AcquireRewriteLocks() on viewquery, before doing too much with it. >> >> There may be sub-queries in viewquery's quals (and also now in its >> >> targetlist) and I don't think the relations referred to by those >> >> sub-queries are getting locked. >> > >> > Well, that wouldn't follow the currently documented rule ontop >> > of QueryRewrite: >> > * NOTE: the parsetree must either have come straight from the parser, >> > * or have been scanned by AcquireRewriteLocks to acquire suitable locks. >> > >> > It might still be the right thing to do, but it seems suspicious that >> > the rules need to be tweaked like that. >> > >> >> Well it matches what already happens in other places in the rewriter >> --- see rewriteRuleAction() and ApplyRetrieveRule(). It's precisely >> because the rule action's query hasn't come from the parser that it >> needs to be processed in this way. > > I really don't know that are of code that well, fortunately I never had > to wade around it much so far... > > Reading your explanation and a bit of the code your plan sound sane. Are > you going to propose a patch? > I thought about making rewriteTargetView() call AcquireRewriteLocks(), but on closer inspection I think that is probably over the top. 90% of the code in AcquireRewriteLocks() is to process the query's RTEs, but rewriteTargetView() is already locking the single RTE in viewquery anyway. Also AcquireRewriteLocks() would acquire the wrong kind of lock on that RTE, since viewquery is a SELECT, but we want an exclusive lock because the RTE is about to become the outer query's result relation. AcquireRewriteLocks() also processes CTEs, but we know that we won't have any of those in rewriteTargetView(). So I think the only thing missing from rewriteTargetView() is to lock any relations from any sublink subqueries in viewquery using acquireLocksOnSubLinks() -- patch attached. Regards, Dean
Вложения
Andres Freund <andres@2ndquadrant.com> wrote: > the matview patch (0002) This is definitely needed as a bug fix. Will adjust comments and commit, back-patched to 9.3. Thanks for spotting this, and thanks for the fix! > Also attached is 0004 which just adds a heap_lock() around a > newly created temporary table in the matview code which shouldn't > be required for correctness but gives warm and fuzzy feelings as > well as less debugging noise. Will think about this. I agree is is probably worth doing something to reduce the noise when looking for cases that actually matter. > Wouldn't it be a good idea to tack such WARNINGs (in a proper and > clean form) to index_open (checking the underlying relation is > locked), relation_open(..., NoLock) (checking the relation has > previously been locked) and maybe RelationIdGetRelation() when > cassert is enabled? ISTM we frequently had bugs around this. It would be nice to have such omissions pointed out during early testing. -- Kevin Grittner EDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
On 2013-11-02 17:05:24 -0700, Kevin Grittner wrote: > Andres Freund <andres@2ndquadrant.com> wrote: > > > the matview patch (0002) > > This is definitely needed as a bug fix. Will adjust comments and > commit, back-patched to 9.3. Thanks. > > Also attached is 0004 which just adds a heap_lock() around a > > newly created temporary table in the matview code which shouldn't > > be required for correctness but gives warm and fuzzy feelings as > > well as less debugging noise. > > Will think about this. I agree is is probably worth doing > something to reduce the noise when looking for cases that actually > matter. It's pretty much free, so I don't think there really is any reason to deviate from other parts of the code. Note how e.g. copy_heap_data(), DefineRelation() and ATRewriteTable() all lock the new relation, even if it just has been created and is (and in the latter two cases will always be) invisible. > > Wouldn't it be a good idea to tack such WARNINGs (in a proper and > > clean form) to index_open (checking the underlying relation is > > locked), relation_open(..., NoLock) (checking the relation has > > previously been locked) and maybe RelationIdGetRelation() when > > cassert is enabled? ISTM we frequently had bugs around this. > > It would be nice to have such omissions pointed out during early > testing. Will try to come up with something. Greetings, Andres Freund -- Andres Freund http://www.2ndQuadrant.com/PostgreSQL Development, 24x7 Support, Training & Services
Andres Freund <andres@2ndquadrant.com> wrote: > On 2013-11-02 17:05:24 -0700, Kevin Grittner wrote: >> Andres Freund <andres@2ndquadrant.com> wrote: >>> Also attached is 0004 which just adds a heap_lock() around a >>> newly created temporary table in the matview code which >>> shouldn't be required for correctness but gives warm and fuzzy >>> feelings as well as less debugging noise. >> >> Will think about this. I agree is is probably worth doing >> something to reduce the noise when looking for cases that >> actually matter. > > It's pretty much free, so I don't think there really is any > reason to deviate from other parts of the code. Note how e.g. > copy_heap_data(), DefineRelation() and ATRewriteTable() all lock > the new relation, even if it just has been created and is (and in > the latter two cases will always be) invisible. None of those locations are using heap_open() as the first parameter to heap_close(). That looks kinda iffy, and the fact that it is not yet done anywhere in the code gives me pause. You probably had a reason for preferring that to a simple call to LockRelationOid(), but I'm not seeing what that reason is. I also don't understand the use of the lockmode variable here. I'm thinking of something like the attached instead. -- Kevin Grittner EDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Вложения
On 2013-11-05 11:56:25 -0800, Kevin Grittner wrote: > Andres Freund <andres@2ndquadrant.com> wrote: > > On 2013-11-02 17:05:24 -0700, Kevin Grittner wrote: > >> Andres Freund <andres@2ndquadrant.com> wrote: > > >>> Also attached is 0004 which just adds a heap_lock() around a > >>> newly created temporary table in the matview code which > >>> shouldn't be required for correctness but gives warm and fuzzy > >>> feelings as well as less debugging noise. > >> > >> Will think about this. I agree is is probably worth doing > >> something to reduce the noise when looking for cases that > >> actually matter. > > > > It's pretty much free, so I don't think there really is any > > reason to deviate from other parts of the code. Note how e.g. > > copy_heap_data(), DefineRelation() and ATRewriteTable() all lock > > the new relation, even if it just has been created and is (and in > > the latter two cases will always be) invisible. > > None of those locations are using heap_open() as the first > parameter to heap_close(). Oh! Sure, what I'd posted was just an absolutely crude hack that surely isn't committable. > I'm thinking of something like the attached instead. Looks fine to me, maybe add a short comment? Greetings, Andres Freund -- Andres Freund http://www.2ndQuadrant.com/PostgreSQL Development, 24x7 Support, Training & Services
On 2013-11-05 12:21:23 -0800, Kevin Grittner wrote: > Andres Freund <andres@2ndquadrant.com> > > > Looks fine to me > > Any thoughts on whether this should be back-patched to 9.3? I can > see arguments both ways, and don't have a particularly strong > feeling one way or the other. Hehe. I was wondering myself. I'd tentatively say no - unless we also backpatch the debugging patch there doesn't seem to be good reason to since the likelihood of conficts due to it doesn't seem very high. Greetings, Andres Freund -- Andres Freund http://www.2ndQuadrant.com/PostgreSQL Development, 24x7 Support, Training & Services
Andres Freund <andres@2ndquadrant.com> wrote: > On 2013-11-05 12:21:23 -0800, Kevin Grittner wrote: >> Andres Freund <andres@2ndquadrant.com> >> >>> Looks fine to me >> >> Any thoughts on whether this should be back-patched to 9.3? I >> can see arguments both ways, and don't have a particularly >> strong feeling one way or the other. > > Hehe. I was wondering myself. I'd tentatively say no - unless we > also backpatch the debugging patch there doesn't seem to be good > reason to since the likelihood of conficts due to it doesn't seem > very high. Works for me. Done. -- Kevin Grittner EDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Andres Freund <andres@2ndquadrant.com> > Looks fine to me Any thoughts on whether this should be back-patched to 9.3? I can see arguments both ways, and don't have a particularly strong feeling one way or the other. -- Kevin Grittner EDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
On 2013-10-24 20:58:55 +0100, Dean Rasheed wrote: > On 24 October 2013 18:28, Andres Freund <andres@2ndquadrant.com> wrote: > > On 2013-10-23 21:20:58 +0100, Dean Rasheed wrote: > >> On 23 October 2013 21:08, Andres Freund <andres@2ndquadrant.com> wrote: > >> > On 2013-10-23 20:51:27 +0100, Dean Rasheed wrote: > >> >> Hmm, my first thought is that rewriteTargetView() should be calling > >> >> AcquireRewriteLocks() on viewquery, before doing too much with it. > >> >> There may be sub-queries in viewquery's quals (and also now in its > >> >> targetlist) and I don't think the relations referred to by those > >> >> sub-queries are getting locked. > >> > > >> > Well, that wouldn't follow the currently documented rule ontop > >> > of QueryRewrite: > >> > * NOTE: the parsetree must either have come straight from the parser, > >> > * or have been scanned by AcquireRewriteLocks to acquire suitable locks. > >> > > >> > It might still be the right thing to do, but it seems suspicious that > >> > the rules need to be tweaked like that. > >> > > >> > >> Well it matches what already happens in other places in the rewriter > >> --- see rewriteRuleAction() and ApplyRetrieveRule(). It's precisely > >> because the rule action's query hasn't come from the parser that it > >> needs to be processed in this way. > > > > I really don't know that are of code that well, fortunately I never had > > to wade around it much so far... > > > > Reading your explanation and a bit of the code your plan sound sane. Are > > you going to propose a patch? > > > > I thought about making rewriteTargetView() call AcquireRewriteLocks(), > but on closer inspection I think that is probably over the top. 90% of > the code in AcquireRewriteLocks() is to process the query's RTEs, but > rewriteTargetView() is already locking the single RTE in viewquery > anyway. Also AcquireRewriteLocks() would acquire the wrong kind of > lock on that RTE, since viewquery is a SELECT, but we want an > exclusive lock because the RTE is about to become the outer query's > result relation. AcquireRewriteLocks() also processes CTEs, but we > know that we won't have any of those in rewriteTargetView(). So I > think the only thing missing from rewriteTargetView() is to lock any > relations from any sublink subqueries in viewquery using > acquireLocksOnSubLinks() -- patch attached. This is still an open problem :( > diff --git a/src/backend/rewrite/rewriteHandler.c b/src/backend/rewrite/rewriteHandler.c > new file mode 100644 > index c52a374..e6a9e7b > *** a/src/backend/rewrite/rewriteHandler.c > --- b/src/backend/rewrite/rewriteHandler.c > *************** rewriteTargetView(Query *parsetree, Rela > *** 2589,2594 **** > --- 2589,2604 ---- > heap_close(base_rel, NoLock); > > /* > + * If the view query contains any sublink subqueries, we should also > + * acquire locks on any relations they refer to. We know that there won't > + * be any subqueries in the range table or CTEs, so we can skip those, as > + * in AcquireRewriteLocks. > + */ > + if (viewquery->hasSubLinks) > + query_tree_walker(viewquery, acquireLocksOnSubLinks, NULL, > + QTW_IGNORE_RC_SUBQUERIES); > + > + /* > * Create a new target RTE describing the base relation, and add it to the > * outer query's rangetable. (What's happening in the next few steps is > * very much like what the planner would do to "pull up" the view into the These days this seems to require a context parameter being passed down. Other than that this patch still fixes the problem. Greetings, Andres Freund
On 27 August 2015 at 12:18, Andres Freund <andres@anarazel.de> wrote: > On 2013-10-24 20:58:55 +0100, Dean Rasheed wrote: >> So I >> think the only thing missing from rewriteTargetView() is to lock any >> relations from any sublink subqueries in viewquery using >> acquireLocksOnSubLinks() -- patch attached. > > This is still an open problem :( > Oh yes, I forgot to follow up on this. >> diff --git a/src/backend/rewrite/rewriteHandler.c b/src/backend/rewrite/rewriteHandler.c >> new file mode 100644 >> index c52a374..e6a9e7b >> *** a/src/backend/rewrite/rewriteHandler.c >> --- b/src/backend/rewrite/rewriteHandler.c >> *************** rewriteTargetView(Query *parsetree, Rela >> *** 2589,2594 **** >> --- 2589,2604 ---- >> heap_close(base_rel, NoLock); >> >> /* >> + * If the view query contains any sublink subqueries, we should also >> + * acquire locks on any relations they refer to. We know that there won't >> + * be any subqueries in the range table or CTEs, so we can skip those, as >> + * in AcquireRewriteLocks. >> + */ >> + if (viewquery->hasSubLinks) >> + query_tree_walker(viewquery, acquireLocksOnSubLinks, NULL, >> + QTW_IGNORE_RC_SUBQUERIES); >> + >> + /* >> * Create a new target RTE describing the base relation, and add it to the >> * outer query's rangetable. (What's happening in the next few steps is >> * very much like what the planner would do to "pull up" the view into the > > These days this seems to require a context parameter being passed > down. Other than that this patch still fixes the problem. > Yes, I concur. It now needs an acquireLocksOnSubLinks_context with for_execute = true, but otherwise it should work. I have a feeling that RLS might suffer from the same issue, but I haven't looked yet. Regards, Dean
On 2015-08-27 18:37:10 +0100, Dean Rasheed wrote: > On 27 August 2015 at 12:18, Andres Freund <andres@anarazel.de> wrote: > >> > >> /* > >> + * If the view query contains any sublink subqueries, we should also > >> + * acquire locks on any relations they refer to. We know that there won't > >> + * be any subqueries in the range table or CTEs, so we can skip those, as > >> + * in AcquireRewriteLocks. > >> + */ > >> + if (viewquery->hasSubLinks) > >> + query_tree_walker(viewquery, acquireLocksOnSubLinks, NULL, > >> + QTW_IGNORE_RC_SUBQUERIES); > >> + > >> + /* > >> * Create a new target RTE describing the base relation, and add it to the > >> * outer query's rangetable. (What's happening in the next few steps is > >> * very much like what the planner would do to "pull up" the view into the > > > > These days this seems to require a context parameter being passed > > down. Other than that this patch still fixes the problem. > > > > Yes, I concur. It now needs an acquireLocksOnSubLinks_context with > for_execute = true, but otherwise it should work. Ok, I'll push that then, unless somebody else wants to do the honors. > I have a feeling that RLS might suffer from the same issue, but I > haven't looked yet. There's a similar issue there, yes: http://archives.postgresql.org/message-id/20150827124931.GD15922%40awork2.anarazel.de Are you thinking of that angle, or yet another one? Regards, Andres
On 27 August 2015 at 18:44, Andres Freund <andres@anarazel.de> wrote: > On 2015-08-27 18:37:10 +0100, Dean Rasheed wrote: >> I have a feeling that RLS might suffer from the same issue, but I >> haven't looked yet. > > There's a similar issue there, yes: > http://archives.postgresql.org/message-id/20150827124931.GD15922%40awork2.anarazel.de > > Are you thinking of that angle, or yet another one? > Yeah, that's what I was thinking of - I just hadn't caught up on all my mail. It also seems to me that this warning has proved its worth, although I don't think it's something a production build should be producing. Perhaps it could be an Assert? Regards, Dean
On 2015-08-27 19:19:35 +0100, Dean Rasheed wrote: > It also seems to me that this warning has proved its worth Same here - I plan to re-submit it. Perhaps the number of bugs it found convinces Tom, after I address some of his points. > although I don't think it's something a production build should be > producing. Perhaps it could be an Assert? It's currently protected by a #ifdef USE_ASSERT_CHECKING. A warning seems to make it easier to actually run the whole regression test, and it's consistent with what we do in a bunch of other places. Greetings, Andres Freund
On 27 August 2015 at 19:29, Andres Freund <andres@anarazel.de> wrote: > On 2015-08-27 19:19:35 +0100, Dean Rasheed wrote: >> It also seems to me that this warning has proved its worth > > Same here - I plan to re-submit it. Perhaps the number of bugs it found > convinces Tom, after I address some of his points. > >> although I don't think it's something a production build should be >> producing. Perhaps it could be an Assert? > > It's currently protected by a #ifdef USE_ASSERT_CHECKING. A warning > seems to make it easier to actually run the whole regression test, and > it's consistent with what we do in a bunch of other places. > OK, that seems reasonable. Regards, Dean
* Andres Freund (andres@anarazel.de) wrote: > On 2015-08-27 18:37:10 +0100, Dean Rasheed wrote: > > Yes, I concur. It now needs an acquireLocksOnSubLinks_context with > > for_execute = true, but otherwise it should work. > > Ok, I'll push that then, unless somebody else wants to do the honors. Done. Apologies about it taking so long, the TAP test failure issue with 'make check' had me tied up for a bit. Thanks! Stephen