Re: [HACKERS] missing locking in at least INSERT INTO view WITH CHECK
* Andres Freund (and...@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 signature.asc Description: Digital signature
Re: [HACKERS] missing locking in at least INSERT INTO view WITH CHECK
On 27 August 2015 at 18:44, Andres Freund and...@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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] missing locking in at least INSERT INTO view WITH CHECK
On 2015-08-27 18:37:10 +0100, Dean Rasheed wrote: On 27 August 2015 at 12:18, Andres Freund and...@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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] missing locking in at least INSERT INTO view WITH CHECK
On 27 August 2015 at 12:18, Andres Freund and...@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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] missing locking in at least INSERT INTO view WITH CHECK
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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] missing locking in at least INSERT INTO view WITH CHECK
On 27 August 2015 at 19:29, Andres Freund and...@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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] missing locking in at least INSERT INTO view WITH CHECK
On 2013-10-24 20:58:55 +0100, Dean Rasheed wrote: On 24 October 2013 18:28, Andres Freund and...@2ndquadrant.com wrote: On 2013-10-23 21:20:58 +0100, Dean Rasheed wrote: On 23 October 2013 21:08, Andres Freund and...@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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] missing locking in at least INSERT INTO view WITH CHECK
Andres Freund and...@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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] missing locking in at least INSERT INTO view WITH CHECK
Andres Freund and...@2ndquadrant.com wrote: On 2013-11-02 17:05:24 -0700, Kevin Grittner wrote: Andres Freund and...@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 Companydiff --git a/src/backend/commands/matview.c b/src/backend/commands/matview.c index d5a10ad..d5e58ae 100644 --- a/src/backend/commands/matview.c +++ b/src/backend/commands/matview.c @@ -30,6 +30,7 @@ #include miscadmin.h #include parser/parse_relation.h #include rewrite/rewriteHandler.h +#include storage/lmgr.h #include storage/smgr.h #include tcop/tcopprot.h #include utils/builtins.h @@ -243,6 +244,7 @@ ExecRefreshMatView(RefreshMatViewStmt *stmt, const char *queryString, /* Create the transient table that will receive the regenerated data. */ OIDNewHeap = make_new_heap(matviewOid, tableSpace, concurrent, ExclusiveLock); + LockRelationOid(OIDNewHeap, AccessExclusiveLock); dest = CreateTransientRelDestReceiver(OIDNewHeap); /* Generate the data, if wanted. */ -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] missing locking in at least INSERT INTO view WITH CHECK
On 2013-11-05 11:56:25 -0800, Kevin Grittner wrote: Andres Freund and...@2ndquadrant.com wrote: On 2013-11-02 17:05:24 -0700, Kevin Grittner wrote: Andres Freund and...@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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] missing locking in at least INSERT INTO view WITH CHECK
On 2013-11-05 12:21:23 -0800, Kevin Grittner wrote: Andres Freund and...@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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] missing locking in at least INSERT INTO view WITH CHECK
Andres Freund and...@2ndquadrant.com wrote: On 2013-11-05 12:21:23 -0800, Kevin Grittner wrote: Andres Freund and...@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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] missing locking in at least INSERT INTO view WITH CHECK
On 2013-11-02 17:05:24 -0700, Kevin Grittner wrote: Andres Freund and...@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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] missing locking in at least INSERT INTO view WITH CHECK
Andres Freund and...@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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] missing locking in at least INSERT INTO view WITH CHECK
On 2013-10-23 21:20:58 +0100, Dean Rasheed wrote: On 23 October 2013 21:08, Andres Freund and...@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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] missing locking in at least INSERT INTO view WITH CHECK
On 24 October 2013 18:28, Andres Freund and...@2ndquadrant.com wrote: On 2013-10-23 21:20:58 +0100, Dean Rasheed wrote: On 23 October 2013 21:08, Andres Freund and...@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 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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] missing locking in at least INSERT INTO view WITH CHECK
On 23 October 2013 02:18, Andres Freund and...@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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] missing locking in at least INSERT INTO view WITH CHECK
On 2013-10-23 20:51:27 +0100, Dean Rasheed wrote: On 23 October 2013 02:18, Andres Freund and...@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. 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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] missing locking in at least INSERT INTO view WITH CHECK
On 23 October 2013 21:08, Andres Freund and...@2ndquadrant.com wrote: On 2013-10-23 20:51:27 +0100, Dean Rasheed wrote: On 23 October 2013 02:18, Andres Freund and...@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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] missing locking in at least INSERT INTO view WITH CHECK
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
From bf329af4eb6d839ae2a75c4f8a2d6867877510f4 Mon Sep 17 00:00:00 2001
From: Andres Freund and...@anarazel.de
Date: Wed, 23 Oct 2013 02:34:51 +0200
Subject: [PATCH 1/4] debug-lock-on-index-without-heap-lock
---
src/backend/access/heap/heapam.c | 10 ++
src/backend/access/index/indexam.c | 14 ++
src/backend/storage/lmgr/lmgr.c| 9 +
src/backend/storage/lmgr/lock.c| 16
src/backend/utils/cache/relcache.c | 10 ++
src/include/storage/lmgr.h | 2 ++
src/include/storage/lock.h | 2 ++
7 files changed, 63 insertions(+)
diff --git a/src/backend/access/heap/heapam.c b/src/backend/access/heap/heapam.c
index a21f31b..3eecd5f 100644
--- a/src/backend/access/heap/heapam.c
+++ b/src/backend/access/heap/heapam.c
@@ -1203,6 +1203,16 @@ heap_open(Oid relationId, LOCKMODE lockmode)
errmsg(\%s\ is a composite type,
RelationGetRelationName(r;
+ if (IsNormalProcessingMode() lockmode == NoLock)
+ {
+ LOCKMODE mode;
+
+ mode = StrongestLocalRelationLock(relationId);
+ if (mode == NoLock)
+ elog(WARNING, no relation lock on rel %s,
+ RelationGetRelationName(r));
+ }
+
return r;
}
diff --git a/src/backend/access/index/indexam.c b/src/backend/access/index/indexam.c
index b878155..10c3991 100644
--- a/src/backend/access/index/indexam.c
+++ b/src/backend/access/index/indexam.c
@@ -65,6 +65,8 @@
#include postgres.h
+#include miscadmin.h
+
#include access/relscan.h
#include access/transam.h
#include catalog/index.h
@@ -142,6 +144,8 @@ static IndexScanDesc index_beginscan_internal(Relation indexRelation,
*
*/
+#include catalog/catalog.h
+
/*
* index_open - open an index relation by relation OID
*
@@ -169,6 +173,16 @@ index_open(Oid relationId, LOCKMODE lockmode)
errmsg(\%s\ is not an index,
RelationGetRelationName(r;
+ if (IsNormalProcessingMode())
+ {
+ LOCKMODE mode;
+
+ mode = StrongestLocalRelationLock(r-rd_index-indrelid);
+ if (mode == NoLock)
+ elog(WARNING, no relation lock on rel %u held while opening index %s,
+ r-rd_index-indrelid,
+ RelationGetRelationName(r));
+ }
return r;
}
diff --git a/src/backend/storage/lmgr/lmgr.c b/src/backend/storage/lmgr/lmgr.c
index a978172..32d7bba 100644
--- a/src/backend/storage/lmgr/lmgr.c
+++ b/src/backend/storage/lmgr/lmgr.c
@@ -232,6 +232,15 @@ UnlockRelation(Relation relation, LOCKMODE lockmode)
LockRelease(tag, lockmode, false);
}
+LOCKMODE
+StrongestLocalRelationLock(Oid relid)
+{
+ LOCKTAG tag;
+ SetLocktagRelationOid(tag, relid);
+ return StrongestLocalLock(tag);
+}
+
+
/*
* LockHasWaitersRelation
*
diff --git a/src/backend/storage/lmgr/lock.c b/src/backend/storage/lmgr/lock.c
index f4f32e9..2f068be 100644
--- a/src/backend/storage/lmgr/lock.c
+++ b/src/backend/storage/lmgr/lock.c
@@ -2365,6 +2365,22 @@ LockReassignOwner(LOCALLOCK *locallock, ResourceOwner parent)
ResourceOwnerForgetLock(CurrentResourceOwner, locallock);
}
+LOCKMODE
+StrongestLocalLock(const LOCKTAG* locktag)
+{
+ HASH_SEQ_STATUS status;
+ LOCALLOCK *locallock;
+ LOCKMODE strongest = NoLock;
+ hash_seq_init(status, LockMethodLocalHash);
+
+ while ((locallock = (LOCALLOCK *) hash_seq_search(status)) != NULL)
+ {
+ if (memcmp(locallock-tag.lock, locktag, sizeof(LOCKTAG)) == 0)
+ strongest = Max(strongest, locallock-tag.mode);
+ }
+ return strongest;
+}
+
/*
* FastPathGrantRelationLock
* Grant lock using per-backend fast-path array, if there is space.
diff --git a/src/backend/utils/cache/relcache.c b/src/backend/utils/cache/relcache.c
index b4cc6ad..f74c6af 100644
--- a/src/backend/utils/cache/relcache.c
+++ b/src/backend/utils/cache/relcache.c
@@ -1577,6 +1577,16 @@ RelationIdGetRelation(Oid relationId)
{
Relation rd;
+ if (IsNormalProcessingMode())
+ {
+ LOCKMODE mode;
+
+ mode = StrongestLocalRelationLock(relationId);
+ if (mode == NoLock)
+ elog(WARNING, no relation lock for descriptor of oid %u,
+ relationId);
+ }
+
/*
* first try to find reldesc in the cache
*/
diff --git a/src/include/storage/lmgr.h b/src/include/storage/lmgr.h
index 1a8c018..c2c6026 100644
--- a/src/include/storage/lmgr.h
+++ b/src/include/storage/lmgr.h
@@ -40,6 +40,8 @@ extern void UnlockRelationIdForSession(LockRelId *relid, LOCKMODE lockmode);
extern void LockRelationForExtension(Relation relation, LOCKMODE lockmode);
extern void UnlockRelationForExtension(Relation relation, LOCKMODE lockmode);
+extern LOCKMODE StrongestLocalRelationLock(Oid relid);
+
/* Lock a page (currently only used within indexes) */
extern void LockPage(Relation relation, BlockNumber blkno, LOCKMODE lockmode);
extern bool
