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
--
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 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
Attachments:
0001-debug-lock-on-index-without-heap-lock.patchtext/x-patch; charset=us-asciiDownload
>From bf329af4eb6d839ae2a75c4f8a2d6867877510f4 Mon Sep 17 00:00:00 2001
From: Andres Freund <andres@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 ConditionalLockPage(Relation relation, BlockNumber blkno, LOCKMODE lockmode);
diff --git a/src/include/storage/lock.h b/src/include/storage/lock.h
index 7bcaf7c..ea915c8 100644
--- a/src/include/storage/lock.h
+++ b/src/include/storage/lock.h
@@ -495,6 +495,8 @@ extern void LockReleaseAll(LOCKMETHODID lockmethodid, bool allLocks);
extern void LockReleaseSession(LOCKMETHODID lockmethodid);
extern void LockReleaseCurrentOwner(LOCALLOCK **locallocks, int nlocks);
extern void LockReassignCurrentOwner(LOCALLOCK **locallocks, int nlocks);
+extern LOCKMODE StrongestLocalLock(const LOCKTAG* locktag);
+
extern bool LockHasWaiters(const LOCKTAG *locktag,
LOCKMODE lockmode, bool sessionLock);
extern VirtualTransactionId *GetLockConflicts(const LOCKTAG *locktag,
--
1.8.3.251.g1462b67
0002-Acquire-appropriate-locks-when-rewriting-during-REFR.patchtext/x-patch; charset=us-asciiDownload
>From 61ad53f8e1d48264fab3a90f4104ffc308bd1f73 Mon Sep 17 00:00:00 2001
From: Andres Freund <andres@anarazel.de>
Date: Wed, 23 Oct 2013 01:25:49 +0200
Subject: [PATCH 2/4] Acquire appropriate locks when rewriting during REFRESH
MATERIALIZED VIEW
---
src/backend/commands/matview.c | 11 +++++++++--
1 file changed, 9 insertions(+), 2 deletions(-)
diff --git a/src/backend/commands/matview.c b/src/backend/commands/matview.c
index fcfc678..1c02eab 100644
--- a/src/backend/commands/matview.c
+++ b/src/backend/commands/matview.c
@@ -283,6 +283,7 @@ refresh_matview_datafill(DestReceiver *dest, Query *query,
Oid save_userid;
int save_sec_context;
int save_nestlevel;
+ Query *copied_query;
/*
* Switch to the owner's userid, so that any functions are run as that
@@ -294,8 +295,14 @@ refresh_matview_datafill(DestReceiver *dest, Query *query,
save_sec_context | SECURITY_RESTRICTED_OPERATION);
save_nestlevel = NewGUCNestLevel();
- /* Rewrite, copying the given Query to make sure it's not changed */
- rewritten = QueryRewrite((Query *) copyObject(query));
+ /* copy the given Query to make sure it's not changed */
+ copied_query = copyObject(query);
+
+ /* acquire locks for rewriting */
+ AcquireRewriteLocks(copied_query, false);
+
+ /* Rewrite */
+ rewritten = QueryRewrite(copied_query);
/* SELECT should never rewrite to more or less than one SELECT query */
if (list_length(rewritten) != 1)
--
1.8.3.251.g1462b67
0003-hack-INSERT-INTO-some_view-to-lock-rels-in-the-under.patchtext/x-patch; charset=us-asciiDownload
>From e49502bca39c70ebadc7cbe2fdacf68c33be627b Mon Sep 17 00:00:00 2001
From: Andres Freund <andres@anarazel.de>
Date: Wed, 23 Oct 2013 02:35:05 +0200
Subject: [PATCH 3/4] hack INSERT INTO some_view; to lock rels in the
underlying view
This is not an acceptable patch!
---
src/backend/parser/parse_relation.c | 18 ++++++++++++++++++
1 file changed, 18 insertions(+)
diff --git a/src/backend/parser/parse_relation.c b/src/backend/parser/parse_relation.c
index 5f46913..731ac7d 100644
--- a/src/backend/parser/parse_relation.c
+++ b/src/backend/parser/parse_relation.c
@@ -27,6 +27,7 @@
#include "parser/parsetree.h"
#include "parser/parse_relation.h"
#include "parser/parse_type.h"
+#include "rewrite/rewriteHandler.h"
#include "utils/builtins.h"
#include "utils/lsyscache.h"
#include "utils/rel.h"
@@ -1009,6 +1010,23 @@ parserOpenTable(ParseState *pstate, const RangeVar *relation, int lockmode)
relation->relname)));
}
}
+ if (rel->rd_rel->relkind == RELKIND_VIEW)
+ {
+ int i;
+ for (i = 0; i < rel->rd_rules->numLocks; i++)
+ {
+ Query *v;
+ RewriteRule *r;
+ ListCell *c;
+
+ r = rel->rd_rules->rules[i];
+ foreach(c, r->actions)
+ {
+ v = (Query *) lfirst(c);
+ AcquireRewriteLocks(copyObject(v), false);
+ }
+ }
+ }
cancel_parser_errposition_callback(&pcbstate);
return rel;
}
--
1.8.3.251.g1462b67
0004-matview-lock-temp-table-till-tx-end-during-refresh-f.patchtext/x-patch; charset=us-asciiDownload
>From b77ea7e33e0b2d9ae02559a52ac9f0326eac69fe Mon Sep 17 00:00:00 2001
From: Andres Freund <andres@anarazel.de>
Date: Wed, 23 Oct 2013 03:08:45 +0200
Subject: [PATCH 4/4] matview: lock temp table till tx end during refresh for
easier debugging
---
src/backend/commands/matview.c | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/src/backend/commands/matview.c b/src/backend/commands/matview.c
index 1c02eab..8e8981a 100644
--- a/src/backend/commands/matview.c
+++ b/src/backend/commands/matview.c
@@ -243,6 +243,10 @@ ExecRefreshMatView(RefreshMatViewStmt *stmt, const char *queryString,
/* Create the transient table that will receive the regenerated data. */
OIDNewHeap = make_new_heap(matviewOid, tableSpace, concurrent,
ExclusiveLock);
+
+ /* lock temporary table till end of transaction */
+ heap_close(heap_open(OIDNewHeap, lockmode), NoLock);
+
dest = CreateTransientRelDestReceiver(OIDNewHeap);
/* Generate the data, if wanted. */
--
1.8.3.251.g1462b67
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
--
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 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
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
--
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 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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
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
Attachments:
updatable-view-locking.patchtext/x-diff; charset=US-ASCII; name=updatable-view-locking.patchDownload
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
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
--
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-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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
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
Attachments:
matview-refresh-lock-newrel-v1.difftext/x-diff; name=matview-refresh-lock-newrel-v1.diffDownload
diff --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. */
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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
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
--
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-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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
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
--
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-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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
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 theThese 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
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 theThese 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
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.deAre 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
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
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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
* 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