Thread: [HACKERS] [PATCH] Lockable views
Hi, Attached is a patch to enable views to be locked. PostgreSQL has supported automatically updatable views since 9.3, so we can udpate simply defined views like regular tables. However, currently, table-level locks on views are not supported. We can not execute LOCK TABLE for views, while we can get row-level locks by FOR UPDATE/SHARE. In some situations that we need table-level locks on tables, we may also need table-level locks on automatically updatable views. Although we can lock base-relations manually, it would be useful if we can lock views without knowing the definition of the views. In the attached patch, only automatically-updatable views that do not have INSTEAD OF rules or INSTEAD OF triggers are lockable. It is assumed that those views definition have only one base-relation. When an auto-updatable view is locked, its base relation is also locked. If the base relation is a view again, base relations are processed recursively. For locking a view, the view owner have to have he priviledge to lock the base relation. * Example test=# CREATE TABLE tbl (i int); CREATE TABLE test=# CREATE VIEW v1 AS SELECT * FROM tbl; CREATE VIEW test=# BEGIN; BEGIN test=# LOCK TABLE v1; LOCK TABLE test=# SELECT relname, locktype, mode FROM pg_locks,pg_class c WHERE c.oid=relation AND relname NOT LIKE 'pg%'; relname | locktype | mode ---------+----------+--------------------- tbl | relation | AccessExclusiveLock v1 | relation | AccessExclusiveLock (2 rows) test=# END; COMMIT test=# CREATE VIEW v2 AS SELECT * FROM v1; CREATE VIEW test=# BEGIN; BEGIN test=# LOCK TABLE v2; LOCK TABLE test=# SELECT relname, locktype, mode FROM pg_locks,pg_class c WHERE c.oid=relation AND relname NOT LIKE 'pg%'; relname | locktype | mode ---------+----------+--------------------- v2 | relation | AccessExclusiveLock tbl | relation | AccessExclusiveLock v1 | relation | AccessExclusiveLock (3 rows) test=# END; COMMIT test=# CREATE VIEW v3 AS SELECT count(*) FROM v1; CREATE VIEW test=# BEGIN; BEGIN test=# LOCK TABLE v3; ERROR: cannot lock view "v3" DETAIL: Views that return aggregate functions are not automatically updatable. test=# END; ROLLBACK test=# CREATE FUNCTION fnc() RETURNS trigger AS $$ BEGIN RETURN NEW; END; $$ LANGUAGE plpgsql; CREATE FUNCTION test=# CREATE TRIGGER trg INSTEAD OF INSERT ON v1 FOR EACH ROW EXECUTE PROCEDURE fnc(); CREATE TRIGGER test=# BEGIN; BEGIN test=# LOCK TABLE v1; ERROR: cannot lock view "v1" DETAIL: views that have an INSTEAD OF trigger are not lockable test=# END; ROLLBACK Regards, -- Yugo Nagata <nagata@sraoss.co.jp> -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Attachment
> Hi, > > Attached is a patch to enable views to be locked. Nice. > PostgreSQL has supported automatically updatable views since 9.3, so we can > udpate simply defined views like regular tables. However, currently, > table-level locks on views are not supported. We can not execute LOCK TABLE > for views, while we can get row-level locks by FOR UPDATE/SHARE. In some > situations that we need table-level locks on tables, we may also need > table-level locks on automatically updatable views. Although we can lock > base-relations manually, it would be useful if we can lock views without > knowing the definition of the views. > > In the attached patch, only automatically-updatable views that do not have > INSTEAD OF rules or INSTEAD OF triggers are lockable. It is assumed that > those views definition have only one base-relation. When an auto-updatable > view is locked, its base relation is also locked. If the base relation is a > view again, base relations are processed recursively. For locking a view, > the view owner have to have he priviledge to lock the base relation. > > * Example > > test=# CREATE TABLE tbl (i int); > CREATE TABLE > > test=# CREATE VIEW v1 AS SELECT * FROM tbl; > CREATE VIEW > test=# BEGIN; > BEGIN > test=# LOCK TABLE v1; > LOCK TABLE > test=# SELECT relname, locktype, mode FROM pg_locks,pg_class c WHERE c.oid=relation AND relname NOT LIKE 'pg%'; > relname | locktype | mode > ---------+----------+--------------------- > tbl | relation | AccessExclusiveLock > v1 | relation | AccessExclusiveLock > (2 rows) > > test=# END; > COMMIT > > test=# CREATE VIEW v2 AS SELECT * FROM v1; > CREATE VIEW > test=# BEGIN; > BEGIN > test=# LOCK TABLE v2; > LOCK TABLE > test=# SELECT relname, locktype, mode FROM pg_locks,pg_class c WHERE c.oid=relation AND relname NOT LIKE 'pg%'; > relname | locktype | mode > ---------+----------+--------------------- > v2 | relation | AccessExclusiveLock > tbl | relation | AccessExclusiveLock > v1 | relation | AccessExclusiveLock > (3 rows) > > test=# END; > COMMIT > > test=# CREATE VIEW v3 AS SELECT count(*) FROM v1; > CREATE VIEW > test=# BEGIN; > BEGIN > test=# LOCK TABLE v3; > ERROR: cannot lock view "v3" > DETAIL: Views that return aggregate functions are not automatically updatable. It would be nice if the message would be something like: DETAIL: Views that return aggregate functions are not lockable > test=# END; > ROLLBACK > > test=# CREATE FUNCTION fnc() RETURNS trigger AS $$ BEGIN RETURN NEW; END; $$ LANGUAGE plpgsql; > CREATE FUNCTION > test=# CREATE TRIGGER trg INSTEAD OF INSERT ON v1 FOR EACH ROW EXECUTE PROCEDURE fnc(); > CREATE TRIGGER > test=# BEGIN; > BEGIN > test=# LOCK TABLE v1; > ERROR: cannot lock view "v1" > DETAIL: views that have an INSTEAD OF trigger are not lockable > test=# END; > ROLLBACK I wonder if we should lock tables in a subquery as well. For example, create view v1 as select * from t1 where i in (select i from t2); In this case should we lock t2 as well? Best regards, -- Tatsuo Ishii SRA OSS, Inc. Japan English: http://www.sraoss.co.jp/index_en.php Japanese:http://www.sraoss.co.jp -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
>> test=# CREATE VIEW v3 AS SELECT count(*) FROM v1; >> CREATE VIEW >> test=# BEGIN; >> BEGIN >> test=# LOCK TABLE v3; >> ERROR: cannot lock view "v3" >> DETAIL: Views that return aggregate functions are not automatically updatable. > > It would be nice if the message would be something like: > > DETAIL: Views that return aggregate functions are not lockable > >> test=# END; >> ROLLBACK >> >> test=# CREATE FUNCTION fnc() RETURNS trigger AS $$ BEGIN RETURN NEW; END; $$ LANGUAGE plpgsql; >> CREATE FUNCTION >> test=# CREATE TRIGGER trg INSTEAD OF INSERT ON v1 FOR EACH ROW EXECUTE PROCEDURE fnc(); >> CREATE TRIGGER >> test=# BEGIN; >> BEGIN >> test=# LOCK TABLE v1; >> ERROR: cannot lock view "v1" >> DETAIL: views that have an INSTEAD OF trigger are not lockable >> test=# END; >> ROLLBACK > > I wonder if we should lock tables in a subquery as well. For example, > > create view v1 as select * from t1 where i in (select i from t2); > > In this case should we lock t2 as well? Current the patch ignores t2 in the case above. So we have options below: - Leave as it is (ignore tables appearing in a subquery) - Lock all tables including in a subquery - Check subquery in the view definition. If there are some tables involved, emit an error and abort. The first one might be different from what users expect. There may be a risk that the second one could cause deadlock. So it seems the third one seems to be the safest IMO. Best regards, -- Tatsuo Ishii SRA OSS, Inc. Japan English: http://www.sraoss.co.jp/index_en.php Japanese:http://www.sraoss.co.jp -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
On Thu, 12 Oct 2017 13:11:45 +0900 (JST) Tatsuo Ishii <ishii@sraoss.co.jp> wrote: > >> test=# CREATE VIEW v3 AS SELECT count(*) FROM v1; > >> CREATE VIEW > >> test=# BEGIN; > >> BEGIN > >> test=# LOCK TABLE v3; > >> ERROR: cannot lock view "v3" > >> DETAIL: Views that return aggregate functions are not automatically updatable. > > > > It would be nice if the message would be something like: > > > > DETAIL: Views that return aggregate functions are not lockable This uses messages from view_query_is_auto_updatable() of the rewrite system directly. Although we can modify the messages, I think it is not necessary for now since we can lock only automatically updatable views. > > > >> test=# END; > >> ROLLBACK > >> > >> test=# CREATE FUNCTION fnc() RETURNS trigger AS $$ BEGIN RETURN NEW; END; $$ LANGUAGE plpgsql; > >> CREATE FUNCTION > >> test=# CREATE TRIGGER trg INSTEAD OF INSERT ON v1 FOR EACH ROW EXECUTE PROCEDURE fnc(); > >> CREATE TRIGGER > >> test=# BEGIN; > >> BEGIN > >> test=# LOCK TABLE v1; > >> ERROR: cannot lock view "v1" > >> DETAIL: views that have an INSTEAD OF trigger are not lockable > >> test=# END; > >> ROLLBACK > > > > I wonder if we should lock tables in a subquery as well. For example, > > > > create view v1 as select * from t1 where i in (select i from t2); > > > > In this case should we lock t2 as well? > > Current the patch ignores t2 in the case above. > > So we have options below: > > - Leave as it is (ignore tables appearing in a subquery) > > - Lock all tables including in a subquery > > - Check subquery in the view definition. If there are some tables > involved, emit an error and abort. > > The first one might be different from what users expect. There may be > a risk that the second one could cause deadlock. So it seems the third > one seems to be the safest IMO. Make sense. Even if the view is locked, when tables in a subquery is modified, the contents of view can change. To avoid it, we have to lock tables, or give up to lock such views. We can say the same thing for functions in a subquery. If the definition of the functions are changed, the result of the view can change. We cannot lock functions, but should we abtain row-level lock on pg_proc in such cases? (of cause, or give up to lock such views....) BTW, though you mentioned the risk of deadlocks, even when there are no subquery, deadlock can occur in the current patch. For example, lock a table T in Session1, and then lock a view V whose base relation is T in Session2. Session2 will wait for Session1 to release the lock on T. After this, when Session1 try to lock view V, the deadlock occurs and the query is canceled. Is this unacceptable behavior? > > Best regards, > -- > Tatsuo Ishii > SRA OSS, Inc. Japan > English: http://www.sraoss.co.jp/index_en.php > Japanese:http://www.sraoss.co.jp -- Yugo Nagata <nagata@sraoss.co.jp> -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
>> >> test=# CREATE VIEW v3 AS SELECT count(*) FROM v1; >> >> CREATE VIEW >> >> test=# BEGIN; >> >> BEGIN >> >> test=# LOCK TABLE v3; >> >> ERROR: cannot lock view "v3" >> >> DETAIL: Views that return aggregate functions are not automatically updatable. >> > >> > It would be nice if the message would be something like: >> > >> > DETAIL: Views that return aggregate functions are not lockable > > This uses messages from view_query_is_auto_updatable() of the rewrite system directly. > Although we can modify the messages, I think it is not necessary for now > since we can lock only automatically updatable views. You could add a flag to view_query_is_auto_updatable() to switch the message between DETAIL: Views that return aggregate functions are not automatically updatable. and DETAIL: Views that return aggregate functions are not lockable >> > I wonder if we should lock tables in a subquery as well. For example, >> > >> > create view v1 as select * from t1 where i in (select i from t2); >> > >> > In this case should we lock t2 as well? >> >> Current the patch ignores t2 in the case above. >> >> So we have options below: >> >> - Leave as it is (ignore tables appearing in a subquery) >> >> - Lock all tables including in a subquery >> >> - Check subquery in the view definition. If there are some tables >> involved, emit an error and abort. >> >> The first one might be different from what users expect. There may be >> a risk that the second one could cause deadlock. So it seems the third >> one seems to be the safest IMO. > > Make sense. Even if the view is locked, when tables in a subquery is > modified, the contents of view can change. To avoid it, we have to > lock tables, or give up to lock such views. > > We can say the same thing for functions in a subquery. If the definition > of the functions are changed, the result of the view can change. > We cannot lock functions, but should we abtain row-level lock on pg_proc > in such cases? (of cause, or give up to lock such views....) I think we don't need to care about function definition changes used in where clauses in views. None view queries using functions do not care about the definition changes of functions while executing the query. So why updatable views need to care them? > BTW, though you mentioned the risk of deadlocks, even when there > are no subquery, deadlock can occur in the current patch. > > For example, lock a table T in Session1, and then lock a view V > whose base relation is T in Session2. Session2 will wait for > Session1 to release the lock on T. After this, when Session1 try to > lock view V, the deadlock occurs and the query is canceled. You are right. Dealocks could occur in any case. Best regards, -- Tatsuo Ishii SRA OSS, Inc. Japan English: http://www.sraoss.co.jp/index_en.php Japanese:http://www.sraoss.co.jp -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
On Mon, 16 Oct 2017 10:07:48 +0900 (JST) Tatsuo Ishii <ishii@sraoss.co.jp> wrote: > >> >> test=# CREATE VIEW v3 AS SELECT count(*) FROM v1; > >> >> CREATE VIEW > >> >> test=# BEGIN; > >> >> BEGIN > >> >> test=# LOCK TABLE v3; > >> >> ERROR: cannot lock view "v3" > >> >> DETAIL: Views that return aggregate functions are not automatically updatable. > >> > > >> > It would be nice if the message would be something like: > >> > > >> > DETAIL: Views that return aggregate functions are not lockable > > > > This uses messages from view_query_is_auto_updatable() of the rewrite system directly. > > Although we can modify the messages, I think it is not necessary for now > > since we can lock only automatically updatable views. > > You could add a flag to view_query_is_auto_updatable() to switch the > message between > > DETAIL: Views that return aggregate functions are not automatically updatable. > > and > > DETAIL: Views that return aggregate functions are not lockable OK. I'll change view_query_is_auto_updatable() so. > > >> > I wonder if we should lock tables in a subquery as well. For example, > >> > > >> > create view v1 as select * from t1 where i in (select i from t2); > >> > > >> > In this case should we lock t2 as well? > >> > >> Current the patch ignores t2 in the case above. > >> > >> So we have options below: > >> > >> - Leave as it is (ignore tables appearing in a subquery) > >> > >> - Lock all tables including in a subquery > >> > >> - Check subquery in the view definition. If there are some tables > >> involved, emit an error and abort. > >> > >> The first one might be different from what users expect. There may be > >> a risk that the second one could cause deadlock. So it seems the third > >> one seems to be the safest IMO. > > > > Make sense. Even if the view is locked, when tables in a subquery is > > modified, the contents of view can change. To avoid it, we have to > > lock tables, or give up to lock such views. > > > > We can say the same thing for functions in a subquery. If the definition > > of the functions are changed, the result of the view can change. > > We cannot lock functions, but should we abtain row-level lock on pg_proc > > in such cases? (of cause, or give up to lock such views....) > > I think we don't need to care about function definition changes used > in where clauses in views. None view queries using functions do not > care about the definition changes of functions while executing the > query. So why updatable views need to care them? I'm a bit confused. What is difference between tables and functions in a subquery with regard to view locking? I think also none view queries using a subquery do not care about the changes of tables in the subquery while executing the query. I might be misnderstanding the problem you mentioned. BTW, I found that if we have to handle subqueries in where clause, we would also have to care about subqueries in target list... The view defined as below is also updatable. =# create view v7 as select (select * from tbl2 limit 1) from tbl; > > > BTW, though you mentioned the risk of deadlocks, even when there > > are no subquery, deadlock can occur in the current patch. > > > > For example, lock a table T in Session1, and then lock a view V > > whose base relation is T in Session2. Session2 will wait for > > Session1 to release the lock on T. After this, when Session1 try to > > lock view V, the deadlock occurs and the query is canceled. > > You are right. Dealocks could occur in any case. > > Best regards, > -- > Tatsuo Ishii > SRA OSS, Inc. Japan > English: http://www.sraoss.co.jp/index_en.php > Japanese:http://www.sraoss.co.jp -- Yugo Nagata <nagata@sraoss.co.jp> -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
> I'm a bit confused. What is difference between tables and functions > in a subquery with regard to view locking? I think also none view queries > using a subquery do not care about the changes of tables in the > subquery while executing the query. I might be misnderstanding > the problem you mentioned. The difference is in the function cases we concern the function definition. While the table cases need to care about table definitions *and* contents of the table. If we are changing the table definition, AccessExclusiveLock will be held for the table and the updation will be blocked anyway. So we don't need to care about the table definition changes. On the other hand, table contents changes need to be cared because no automatic locking are held in some cases. I think whether tables in the subquery need locking or not is depending on use cases. So I dug into the previous candidates a little bit more: 1) Leave as it is (ignore tables appearing in a subquery) 2) Lock all tables including in a subquery 3) Check subquery in the view definition. If there are some tables involved, emit an error and abort. I think one of the problems with #2 is, we will lock tables involved by the view in random order, which could cause unwanted dead locks. This is not good and I cannot see any easy way to avoid this. Also some tables may not need to be locked. Problem with #3 is, it does not help a user who wants to control lockings by himself/herself. So it seem #1 is the most reasonable way to deal with the problem assuming that it's user's responsibility to take appropriate locks on the tables in the subquery. > BTW, I found that if we have to handle subqueries in where clause, we would > also have to care about subqueries in target list... The view defined as > below is also updatable. > > =# create view v7 as select (select * from tbl2 limit 1) from tbl; The view is not updatable. You will get something like if you try to update v7: DETAIL: Views that have no updatable columns are not automatically updatable. On the other hand this: create view v7 as select i, (select j from tbl2 limit 1) from tbl; will be updatable. In this case column j of v7 will never be updatable. And you should do something like: insert into v7(i) values... In short, you don't need to care about a subquery appearing in the TLE as far as the view locking concerns. Best regards, -- Tatsuo Ishii SRA OSS, Inc. Japan English: http://www.sraoss.co.jp/index_en.php Japanese:http://www.sraoss.co.jp -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
On Wed, Oct 11, 2017 at 11:36 AM, Yugo Nagata <nagata@sraoss.co.jp> wrote: > In the attached patch, only automatically-updatable views that do not have > INSTEAD OF rules or INSTEAD OF triggers are lockable. It is assumed that > those views definition have only one base-relation. When an auto-updatable > view is locked, its base relation is also locked. If the base relation is a > view again, base relations are processed recursively. For locking a view, > the view owner have to have he priviledge to lock the base relation. Why is this the right behavior? I would have expected LOCK TABLE v to lock the view and nothing else. See http://postgr.es/m/AANLkTi=KupesJHRdEvGfbT30aU_iYRO6zwK+fwwY_sGd@mail.gmail.com for previous discussion of this topic. -- Robert Haas EnterpriseDB: 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 Fri, Oct 27, 2017 at 2:11 PM, Robert Haas <robertmhaas@gmail.com> wrote: > On Wed, Oct 11, 2017 at 11:36 AM, Yugo Nagata <nagata@sraoss.co.jp> wrote: >> In the attached patch, only automatically-updatable views that do not have >> INSTEAD OF rules or INSTEAD OF triggers are lockable. It is assumed that >> those views definition have only one base-relation. When an auto-updatable >> view is locked, its base relation is also locked. If the base relation is a >> view again, base relations are processed recursively. For locking a view, >> the view owner have to have he priviledge to lock the base relation. > > Why is this the right behavior? > > I would have expected LOCK TABLE v to lock the view and nothing else. > > See http://postgr.es/m/AANLkTi=KupesJHRdEvGfbT30aU_iYRO6zwK+fwwY_sGd@mail.gmail.com > for previous discussion of this topic. That's what I would expect as well.. But I may be missing something. I am marking the patch as returned with feedback as this has not been replied in one month. -- Michael
On Tue, 17 Oct 2017 11:59:05 +0900 (JST) Tatsuo Ishii <ishii@sraoss.co.jp> wrote: > > I'm a bit confused. What is difference between tables and functions > > in a subquery with regard to view locking? I think also none view queries > > using a subquery do not care about the changes of tables in the > > subquery while executing the query. I might be misnderstanding > > the problem you mentioned. > > The difference is in the function cases we concern the function > definition. While the table cases need to care about table > definitions *and* contents of the table. > > If we are changing the table definition, AccessExclusiveLock will be > held for the table and the updation will be blocked anyway. So we > don't need to care about the table definition changes. > > On the other hand, table contents changes need to be cared because no > automatic locking are held in some cases. I think whether tables in > the subquery need locking or not is depending on use cases. > > So I dug into the previous candidates a little bit more: > > 1) Leave as it is (ignore tables appearing in a subquery) > > 2) Lock all tables including in a subquery > > 3) Check subquery in the view definition. If there are some tables > involved, emit an error and abort. > > I think one of the problems with #2 is, we will lock tables involved > by the view in random order, which could cause unwanted dead > locks. This is not good and I cannot see any easy way to avoid > this. Also some tables may not need to be locked. > > Problem with #3 is, it does not help a user who wants to control > lockings by himself/herself. > > So it seem #1 is the most reasonable way to deal with the problem > assuming that it's user's responsibility to take appropriate locks on > the tables in the subquery. Thank you for your response. I agree to adopt #1. > > > BTW, I found that if we have to handle subqueries in where clause, we would > > also have to care about subqueries in target list... The view defined as > > below is also updatable. > > > > =# create view v7 as select (select * from tbl2 limit 1) from tbl; > > The view is not updatable. You will get something like if you try to update v7: > > DETAIL: Views that have no updatable columns are not automatically updatable. Although you can not insert into or update v7, you can delete tuples from v7 since it just delete tuples from table tbl regardless with any column. However, as disussed above, if it is user's responsibility to take appropriate locks on the tables in subqueries in the target list, we don't need to care about these. > > On the other hand this: > > create view v7 as select i, (select j from tbl2 limit 1) from tbl; > > will be updatable. In this case column j of v7 will never be > updatable. And you should do something like: > > insert into v7(i) values... > > In short, you don't need to care about a subquery appearing in the TLE > as far as the view locking concerns. > > Best regards, > -- > Tatsuo Ishii > SRA OSS, Inc. Japan > English: http://www.sraoss.co.jp/index_en.php > Japanese:http://www.sraoss.co.jp > > > -- > Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) > To make changes to your subscription: > http://www.postgresql.org/mailpref/pgsql-hackers -- Yugo Nagata <nagata@sraoss.co.jp>
On Fri, 27 Oct 2017 07:11:14 +0200 Robert Haas <robertmhaas@gmail.com> wrote: > On Wed, Oct 11, 2017 at 11:36 AM, Yugo Nagata <nagata@sraoss.co.jp> wrote: > > In the attached patch, only automatically-updatable views that do not have > > INSTEAD OF rules or INSTEAD OF triggers are lockable. It is assumed that > > those views definition have only one base-relation. When an auto-updatable > > view is locked, its base relation is also locked. If the base relation is a > > view again, base relations are processed recursively. For locking a view, > > the view owner have to have he priviledge to lock the base relation. > > Why is this the right behavior? > > I would have expected LOCK TABLE v to lock the view and nothing else. > > See http://postgr.es/m/AANLkTi=KupesJHRdEvGfbT30aU_iYRO6zwK+fwwY_sGd@mail.gmail.com > for previous discussion of this topic. This discussion is one about 7 years ago when automatically-updatable views are not supported. Since 9.3, simple views can be updated as well as tables, so now I think it is reasonable that LOCK TABLE for views locks their base tables. If we want to lock only the view, it seems to me that LOCK VIEW syntax is good. However, to realize this, changing the syntax to avoid a shift/reduce conflict will be needed as disucussed in the "LOCK for non-tables" thread. > > -- > Robert Haas > EnterpriseDB: 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 -- Yugo Nagata <nagata@sraoss.co.jp>
On Wed, 29 Nov 2017 11:29:36 +0900 Michael Paquier <michael.paquier@gmail.com> wrote: > On Fri, Oct 27, 2017 at 2:11 PM, Robert Haas <robertmhaas@gmail.com> wrote: > > On Wed, Oct 11, 2017 at 11:36 AM, Yugo Nagata <nagata@sraoss.co.jp> wrote: > >> In the attached patch, only automatically-updatable views that do not have > >> INSTEAD OF rules or INSTEAD OF triggers are lockable. It is assumed that > >> those views definition have only one base-relation. When an auto-updatable > >> view is locked, its base relation is also locked. If the base relation is a > >> view again, base relations are processed recursively. For locking a view, > >> the view owner have to have he priviledge to lock the base relation. > > > > Why is this the right behavior? > > > > I would have expected LOCK TABLE v to lock the view and nothing else. > > > > See http://postgr.es/m/AANLkTi=KupesJHRdEvGfbT30aU_iYRO6zwK+fwwY_sGd@mail.gmail.com > > for previous discussion of this topic. > > That's what I would expect as well.. But I may be missing something. I > am marking the patch as returned with feedback as this has not been > replied in one month. I was busy for and I could not work on this patch. After reading the previous discussion, I still think the behavior of this patch would be right. So, I would like to reregister to CF 2018-1. Do I need to create a new entry on CF? or should I change the status to "Moved to next CF"? > -- > Michael > -- Yugo Nagata <nagata@sraoss.co.jp>
On Fri, Dec 22, 2017 at 04:19:46PM +0900, Yugo Nagata wrote: > I was busy for and I could not work on this patch. After reading the > previous discussion, I still think the behavior of this patch would > be right. So, I would like to reregister to CF 2018-1. Do I need to > create a new entry on CF? or should I change the status to > "Moved to next CF"? This is entirely up to you. It may make sense as well to spawn a new thread and mark the new patch set as v2, based on the feedback received for v1, as well as it could make sense to continue with this thread. If the move involves a complete patch rewrite with a rather different concept, a new thread is more adapted in my opinion. -- Michael
Attachment
On Sat, 23 Dec 2017 09:44:30 +0900 Michael Paquier <michael.paquier@gmail.com> wrote: > On Fri, Dec 22, 2017 at 04:19:46PM +0900, Yugo Nagata wrote: > > I was busy for and I could not work on this patch. After reading the > > previous discussion, I still think the behavior of this patch would > > be right. So, I would like to reregister to CF 2018-1. Do I need to > > create a new entry on CF? or should I change the status to > > "Moved to next CF"? > > This is entirely up to you. It may make sense as well to spawn a new thread > and mark the new patch set as v2, based on the feedback received for v1, as > well as it could make sense to continue with this thread. If the move involves > a complete patch rewrite with a rather different concept, a new thread is more > adapted in my opinion. Thank you for your comment. I have created a new entry in CF-2017-1 and registered this thread again. > -- > Michael -- Yugo Nagata <nagata@sraoss.co.jp>
On Tue, Dec 26, 2017 at 06:37:06PM +0900, Yugo Nagata wrote: > I have created a new entry in CF-2017-1 and registered this thread again. Fine for me. Thanks for the update. And I guess that you are planning to send a new version before the beginning of the next commit fest using the feedback provided? -- Michael
Attachment
Yugo Nagata wrote: > On Fri, 27 Oct 2017 07:11:14 +0200 > Robert Haas <robertmhaas@gmail.com> wrote: > > > On Wed, Oct 11, 2017 at 11:36 AM, Yugo Nagata <nagata@sraoss.co.jp> wrote: > > > In the attached patch, only automatically-updatable views that do not have > > > INSTEAD OF rules or INSTEAD OF triggers are lockable. It is assumed that > > > those views definition have only one base-relation. When an auto-updatable > > > view is locked, its base relation is also locked. If the base relation is a > > > view again, base relations are processed recursively. For locking a view, > > > the view owner have to have he priviledge to lock the base relation. > > > > Why is this the right behavior? > > > > I would have expected LOCK TABLE v to lock the view and nothing else. > This discussion is one about 7 years ago when automatically-updatable views > are not supported. Since 9.3, simple views can be updated as well as tables, > so now I think it is reasonable that LOCK TABLE for views locks their base > tables. I agree with Yugo Nagata -- LOCK TABLE is in some cases necessary to provide the right isolation so that an operation can be carried out without interference from other processes that want to process the same data -- and if a view is provided on top of existing tables, preventing concurrent changes to the data returned by the view is done by locking the view and recursively the tables that the view are built on, as if the view were a table. This is why LOCK TABLE is the right command to do it. Also, if an application is designed using a table and concurrent changes are prevented via LOCK TABLE, then when the underlying schema is changed and the table is replaced by a view, the application continues to work unchanged; not only syntactically (no error because of table-locking a view) but also semantically because new application code that modifies data in underlying tables from paths other than the view will need to compete with those through the view, which is correct. > > See http://postgr.es/m/AANLkTi=KupesJHRdEvGfbT30aU_iYRO6zwK+fwwY_sGd@mail.gmail.com > > for previous discussion of this topic. > If we want to lock only the view, it seems to me that LOCK VIEW syntax is good. > However, to realize this, changing the syntax to avoid a shift/reduce > conflict will be needed as disucussed in the "LOCK for non-tables" thread. +1 on making TABLE mandatory in LOCK [TABLE], since that will support this new LOCK VIEW thing as well as locking other object types. -- Álvaro Herrera https://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On Tue, 26 Dec 2017 22:22:33 +0900 Michael Paquier <michael.paquier@gmail.com> wrote: > On Tue, Dec 26, 2017 at 06:37:06PM +0900, Yugo Nagata wrote: > > I have created a new entry in CF-2017-1 and registered this thread again. > > Fine for me. Thanks for the update. And I guess that you are planning to > send a new version before the beginning of the next commit fest using > the feedback provided? Yes. I'll update the patch according to Ishii-san's comments. > -- > Michael -- Yugo Nagata <nagata@sraoss.co.jp>
Hi, Attached is the updated patch. On Mon, 16 Oct 2017 10:07:48 +0900 (JST) Tatsuo Ishii <ishii@sraoss.co.jp> wrote: > >> > It would be nice if the message would be something like: > >> > > >> > DETAIL: Views that return aggregate functions are not lockable > You could add a flag to view_query_is_auto_updatable() to switch the > message between > > DETAIL: Views that return aggregate functions are not automatically updatable. > > and > > DETAIL: Views that return aggregate functions are not lockable I didn't want to change the interface of view_query_is_auto_updatable() because this might be called from other third-patry software, so I renamed this function to view_query_is_auto_updatable_or_lockable() and added the flag to this. I created view_query_is_auto_updatable() as a wrapper of this function. I also made view_query_is_lockable() that returns a other message than view_query_is_auto_updatable(). > On Tue, 17 Oct 2017 11:59:05 +0900 (JST) > Tatsuo Ishii <ishii@sraoss.co.jp> wrote: > > 1) Leave as it is (ignore tables appearing in a subquery) > > > > 2) Lock all tables including in a subquery > > > > 3) Check subquery in the view > > So it seem #1 is the most reasonable way to deal with the problem > > assuming that it's user's responsibility to take appropriate locks on > > the tables in the subquery. I adopted #1 and I didn't change anything about this. Regards, -- Yugo Nagata <nagata@sraoss.co.jp>
Attachment
> I didn't want to change the interface of view_query_is_auto_updatable() > because this might be called from other third-patry software, so I renamed > this function to view_query_is_auto_updatable_or_lockable() and added the flag > to this. I created view_query_is_auto_updatable() as a wrapper of this function. > I also made view_query_is_lockable() that returns a other message than > view_query_is_auto_updatable(). > >> On Tue, 17 Oct 2017 11:59:05 +0900 (JST) >> Tatsuo Ishii <ishii@sraoss.co.jp> wrote: >> > 1) Leave as it is (ignore tables appearing in a subquery) >> > >> > 2) Lock all tables including in a subquery >> > >> > 3) Check subquery in the view > >> > So it seem #1 is the most reasonable way to deal with the problem >> > assuming that it's user's responsibility to take appropriate locks on >> > the tables in the subquery. > > I adopted #1 and I didn't change anything about this. Looks good to me except that the patch lacks documents and the regression test needs more cases. For example, it needs a test for the case #1 above: probably using pg_locks to make sure that the tables appearing in the subquery do not hold locks. Best regards, -- Tatsuo Ishii SRA OSS, Inc. Japan English: http://www.sraoss.co.jp/index_en.php Japanese:http://www.sraoss.co.jp
On Thu, 28 Dec 2017 09:29:11 +0900 (JST) Tatsuo Ishii <ishii@sraoss.co.jp> wrote: > > I didn't want to change the interface of view_query_is_auto_updatable() > > because this might be called from other third-patry software, so I renamed > > this function to view_query_is_auto_updatable_or_lockable() and added the flag > > to this. I created view_query_is_auto_updatable() as a wrapper of this function. > > I also made view_query_is_lockable() that returns a other message than > > view_query_is_auto_updatable(). > > > >> On Tue, 17 Oct 2017 11:59:05 +0900 (JST) > >> Tatsuo Ishii <ishii@sraoss.co.jp> wrote: > >> > 1) Leave as it is (ignore tables appearing in a subquery) > >> > > >> > 2) Lock all tables including in a subquery > >> > > >> > 3) Check subquery in the view > > > >> > So it seem #1 is the most reasonable way to deal with the problem > >> > assuming that it's user's responsibility to take appropriate locks on > >> > the tables in the subquery. > > > > I adopted #1 and I didn't change anything about this. > > Looks good to me except that the patch lacks documents and the > regression test needs more cases. For example, it needs a test for the > case #1 above: probably using pg_locks to make sure that the tables > appearing in the subquery do not hold locks. Attached is the update patch, v3. I add some regression test and the documentation. > > Best regards, > -- > Tatsuo Ishii > SRA OSS, Inc. Japan > English: http://www.sraoss.co.jp/index_en.php > Japanese:http://www.sraoss.co.jp -- Yugo Nagata <nagata@sraoss.co.jp>
Attachment
>> >> > 1) Leave as it is (ignore tables appearing in a subquery) >> >> > >> >> > 2) Lock all tables including in a subquery >> >> > >> >> > 3) Check subquery in the view >> > >> >> > So it seem #1 is the most reasonable way to deal with the problem >> >> > assuming that it's user's responsibility to take appropriate locks on >> >> > the tables in the subquery. >> > >> > I adopted #1 and I didn't change anything about this. >> >> Looks good to me except that the patch lacks documents and the >> regression test needs more cases. For example, it needs a test for the >> case #1 above: probably using pg_locks to make sure that the tables >> appearing in the subquery do not hold locks. > > Attached is the update patch, v3. I add some regression test and > the documentation. The patch produces a warning. /home/t-ishii/lock_view-v3.patch:542: trailing whitespace. -- Verify that we can lock a auto-updatable views warning: 1 line adds whitespace errors. Your addition to the doc: + Automatically updatable views (see <xref linkend="sql-createview">) + that do not have <literal>INSTEAD OF</> triggers or <literal>INSTEAD</> + rules are also lockable. When a view is locked, its base relations are + also locked recursively with the same lock mode. does not mention about the point: >> >> > 1) Leave as it is (ignore tables appearing in a subquery) Best regards, -- Tatsuo Ishii SRA OSS, Inc. Japan English: http://www.sraoss.co.jp/index_en.php Japanese:http://www.sraoss.co.jp
Hi, The updated patch is attached. On Fri, 29 Dec 2017 23:39:39 +0900 (JST) Tatsuo Ishii <ishii@sraoss.co.jp> wrote: > The patch produces a warning. > > /home/t-ishii/lock_view-v3.patch:542: trailing whitespace. > -- Verify that we can lock a auto-updatable views > warning: 1 line adds whitespace errors. Fixed. > > Your addition to the doc: > + Automatically updatable views (see <xref linkend="sql-createview">) > + that do not have <literal>INSTEAD OF</> triggers or <literal>INSTEAD</> > + rules are also lockable. When a view is locked, its base relations are > + also locked recursively with the same lock mode. > > does not mention about the point: > > >> >> > 1) Leave as it is (ignore tables appearing in a subquery) I added this point to the documentation. Regards, > > Best regards, > -- > Tatsuo Ishii > SRA OSS, Inc. Japan > English: http://www.sraoss.co.jp/index_en.php > Japanese:http://www.sraoss.co.jp -- Yugo Nagata <nagata@sraoss.co.jp>
Attachment
On Sun, Dec 31, 2017 at 11:57 PM, Yugo Nagata <nagata@sraoss.co.jp> wrote: > On Fri, 29 Dec 2017 23:39:39 +0900 (JST) > Tatsuo Ishii <ishii@sraoss.co.jp> wrote: >> Your addition to the doc: >> + Automatically updatable views (see <xref linkend="sql-createview">) >> + that do not have <literal>INSTEAD OF</> triggers or <literal>INSTEAD</> >> + rules are also lockable. When a view is locked, its base relations are >> + also locked recursively with the same lock mode. > > I added this point to the documentation. + Automatically updatable views (see <xref linkend="sql-createview">) + that do not have <literal>INSTEAD OF</> triggers or <literal>INSTEAD</> Tthe documentation doesn't build: you now need to say </literal> instead of </>, and you need to say <xref ... />. -- Thomas Munro http://www.enterprisedb.com
On Thu, 25 Jan 2018 20:51:41 +1300 Thomas Munro <thomas.munro@enterprisedb.com> wrote: > On Sun, Dec 31, 2017 at 11:57 PM, Yugo Nagata <nagata@sraoss.co.jp> wrote: > > On Fri, 29 Dec 2017 23:39:39 +0900 (JST) > > Tatsuo Ishii <ishii@sraoss.co.jp> wrote: > >> Your addition to the doc: > >> + Automatically updatable views (see <xref linkend="sql-createview">) > >> + that do not have <literal>INSTEAD OF</> triggers or <literal>INSTEAD</> > >> + rules are also lockable. When a view is locked, its base relations are > >> + also locked recursively with the same lock mode. > > > > I added this point to the documentation. > > + Automatically updatable views (see <xref linkend="sql-createview">) > + that do not have <literal>INSTEAD OF</> triggers or <literal>INSTEAD</> Thank you for pointing out this. Also, I've noticed this patch cannot be applied to the HEAD, so I'll fix them together and update the patch. > > Tthe documentation doesn't build: you now need to say </literal> > instead of </>, and you need to say <xref ... />. > > -- > Thomas Munro > http://www.enterprisedb.com > -- Yugo Nagata <nagata@sraoss.co.jp>
On Fri, 26 Jan 2018 21:30:49 +0900 Yugo Nagata <nagata@sraoss.co.jp> wrote: > On Thu, 25 Jan 2018 20:51:41 +1300 > Thomas Munro <thomas.munro@enterprisedb.com> wrote: > > > On Sun, Dec 31, 2017 at 11:57 PM, Yugo Nagata <nagata@sraoss.co.jp> wrote: > > > On Fri, 29 Dec 2017 23:39:39 +0900 (JST) > > > Tatsuo Ishii <ishii@sraoss.co.jp> wrote: > > >> Your addition to the doc: > > >> + Automatically updatable views (see <xref linkend="sql-createview">) > > >> + that do not have <literal>INSTEAD OF</> triggers or <literal>INSTEAD</> > > >> + rules are also lockable. When a view is locked, its base relations are > > >> + also locked recursively with the same lock mode. > > > > > > I added this point to the documentation. > > > > + Automatically updatable views (see <xref linkend="sql-createview">) > > + that do not have <literal>INSTEAD OF</> triggers or <literal>INSTEAD</> > > Thank you for pointing out this. > > Also, I've noticed this patch cannot be applied to the HEAD, so I'll fix > them together and update the patch. Attached is the updated patch v5 including fixing SGML and rebase to HEAD. Regards, > > > > > Tthe documentation doesn't build: you now need to say </literal> > > instead of </>, and you need to say <xref ... />. > > > > -- > > Thomas Munro > > http://www.enterprisedb.com > > > > > -- > Yugo Nagata <nagata@sraoss.co.jp> > -- Yugo Nagata <nagata@sraoss.co.jp>
Attachment
> Attached is the updated patch v5 including fixing SGML and rebase to HEAD. You need to DROP VIEW lock_view4 and lock_view5 in the regression test as well. Best regards, -- Tatsuo Ishii SRA OSS, Inc. Japan English: http://www.sraoss.co.jp/index_en.php Japanese:http://www.sraoss.co.jp
On Tue, 30 Jan 2018 13:58:30 +0900 (JST) Tatsuo Ishii <ishii@sraoss.co.jp> wrote: > > Attached is the updated patch v5 including fixing SGML and rebase to HEAD. > > You need to DROP VIEW lock_view4 and lock_view5 in the regression > test as well. Thank you for reviewing the patch. I fixed this and attached a updated patch v6. Regards, > > Best regards, > -- > Tatsuo Ishii > SRA OSS, Inc. Japan > English: http://www.sraoss.co.jp/index_en.php > Japanese:http://www.sraoss.co.jp -- Yugo Nagata <nagata@sraoss.co.jp>
Attachment
>> You need to DROP VIEW lock_view4 and lock_view5 in the regression >> test as well. > > Thank you for reviewing the patch. > > I fixed this and attached a updated patch v6. Looks good to me. If there's no objection, especially from Thomas Munro, I will mark this as "ready for committer". Best regards, -- Tatsuo Ishii SRA OSS, Inc. Japan English: http://www.sraoss.co.jp/index_en.php Japanese:http://www.sraoss.co.jp
On Tue, Jan 30, 2018 at 6:48 PM, Tatsuo Ishii <ishii@sraoss.co.jp> wrote: >>> You need to DROP VIEW lock_view4 and lock_view5 in the regression >>> test as well. >> >> Thank you for reviewing the patch. >> >> I fixed this and attached a updated patch v6. > > Looks good to me. If there's no objection, especially from Thomas > Munro, I will mark this as "ready for committer". About the idea: it makes some kind of sense to me that we should lock the underlying table, in all the same cases that you could do DML on the view automatically. I wonder if this is a problem for the soundness: "Tables appearing in a subquery are ignored and not locked." I can imagine using this for making backwards-compatible schema changes, in which case the LOCK-based transaction isolation techniques might benefit from this behaviour. I'd be interested to hear about the ideal use case you have in mind. About the patch: I didn't study it in detail. It builds, has documentation and passes all tests. Would it be a good idea to add an isolation test to show that the underlying relation is actually locked? Typo: + /* Check permissions with the view owner's priviledge. */ s/priviledge/privilege/ Grammar: +/* + * Check whether the view is lockable. + * + * Currently, only auto-updatable views can be locked, that is, + * views whose definition are simple and that doesn't have + * instead of rules or triggers are lockable. s/definition are simple and that doesn't/definition is simple and that don't/ s/instead of/INSTEAD OF/ ? -- Thomas Munro http://www.enterprisedb.com
On Tue, 30 Jan 2018 19:21:04 +1300 Thomas Munro <thomas.munro@enterprisedb.com> wrote: > On Tue, Jan 30, 2018 at 6:48 PM, Tatsuo Ishii <ishii@sraoss.co.jp> wrote: > >>> You need to DROP VIEW lock_view4 and lock_view5 in the regression > >>> test as well. > >> > >> Thank you for reviewing the patch. > >> > >> I fixed this and attached a updated patch v6. > > > > Looks good to me. If there's no objection, especially from Thomas > > Munro, I will mark this as "ready for committer". > > About the idea: it makes some kind of sense to me that we should lock > the underlying table, in all the same cases that you could do DML on > the view automatically. I wonder if this is a problem for the > soundness: "Tables appearing in a subquery are ignored and not > locked." I can imagine using this for making backwards-compatible > schema changes, in which case the LOCK-based transaction isolation > techniques might benefit from this behaviour. I'd be interested to > hear about the ideal use case you have in mind. I think the use case is almost similar to that of auto-updatable views. There are some purpose to use views, for example 1) preventing from modifying application due to schema changes, 2) protecting the underlying table from users without proper privileges, 3) making a shorthand of a query with complex WHERE condition. When these are updatable views and users need transaction isolation during updating them, I think the lockable views feature is benefitical because users don't need to refer to the underlying table. Users might don't know the underlying table, or even might not have the privilege to lock this. > About the patch: I didn't study it in detail. It builds, has > documentation and passes all tests. Would it be a good idea to add an > isolation test to show that the underlying relation is actually > locked? Whether the underlying relation is actually locked or not is confirmed in the regression test using pg_locks, so I don't believe that we need to add an isolation test. > Typo: > > + /* Check permissions with the view owner's priviledge. */ > > s/priviledge/privilege/ > > Grammar: > > +/* > + * Check whether the view is lockable. > + * > + * Currently, only auto-updatable views can be locked, that is, > + * views whose definition are simple and that doesn't have > + * instead of rules or triggers are lockable. > > s/definition are simple and that doesn't/definition is simple and that don't/ > s/instead of/INSTEAD OF/ ? Thank you for pointing out these. I attached the fixed patch. Regards, > -- > Thomas Munro > http://www.enterprisedb.com -- Yugo Nagata <nagata@sraoss.co.jp>
Attachment
On Tue, Jan 30, 2018 at 6:48 PM, Tatsuo Ishii <ishii@sraoss.co.jp> wrote: > Looks good to me. If there's no objection, especially from Thomas > Munro, I will mark this as "ready for committer". No objection from me. -- Thomas Munro http://www.enterprisedb.com
> On Tue, Jan 30, 2018 at 6:48 PM, Tatsuo Ishii <ishii@sraoss.co.jp> wrote: >> Looks good to me. If there's no objection, especially from Thomas >> Munro, I will mark this as "ready for committer". > > No objection from me. I marked this as "Ready for Committer". Best regards, -- Tatsuo Ishii SRA OSS, Inc. Japan English: http://www.sraoss.co.jp/index_en.php Japanese:http://www.sraoss.co.jp
On Thu, 01 Feb 2018 09:48:49 +0900 (JST) Tatsuo Ishii <ishii@sraoss.co.jp> wrote: > > On Tue, Jan 30, 2018 at 6:48 PM, Tatsuo Ishii <ishii@sraoss.co.jp> wrote: > >> Looks good to me. If there's no objection, especially from Thomas > >> Munro, I will mark this as "ready for committer". > > > > No objection from me. > > I marked this as "Ready for Committer". Thank you for reviewing the patch! Regards, > > Best regards, > -- > Tatsuo Ishii > SRA OSS, Inc. Japan > English: http://www.sraoss.co.jp/index_en.php > Japanese:http://www.sraoss.co.jp -- Yugo Nagata <nagata@sraoss.co.jp>
On Tue, Jan 30, 2018 at 1:21 AM, Thomas Munro <thomas.munro@enterprisedb.com> wrote: > About the idea: it makes some kind of sense to me that we should lock > the underlying table, in all the same cases that you could do DML on > the view automatically. I wonder if this is a problem for the > soundness: "Tables appearing in a subquery are ignored and not > locked." Yeah, that seems like a pretty bad idea. It's exposing what is basically an implementation detail to users. I think that if we change the rules for which subqueries get flattened in a future release, then the behavior will also change. That seems bad. I also think that this is a bad idea for another reason, which is that it leaves us with no syntax to say that you want to lock the view itself, and pg_dump wants do that if only we had syntax for it. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
> On Tue, Jan 30, 2018 at 1:21 AM, Thomas Munro > <thomas.munro@enterprisedb.com> wrote: >> About the idea: it makes some kind of sense to me that we should lock >> the underlying table, in all the same cases that you could do DML on >> the view automatically. I wonder if this is a problem for the >> soundness: "Tables appearing in a subquery are ignored and not >> locked." > > Yeah, that seems like a pretty bad idea. It's exposing what is > basically an implementation detail to users. Initially I thought all base tables including ones in a subquery also should be locked like you. But after some discussions with Yugo, I agree that locking table in a subquery is less valuable for users (and I am afraid it may introduce more deadlock chances). See upthead discussions. > I think that if we > change the rules for which subqueries get flattened in a future > release, then the behavior will also change. That seems bad. I doubt it could happen in the future but if that happend we should disallow locking on such views. > I also think that this is a bad idea for another reason, which is that > it leaves us with no syntax to say that you want to lock the view > itself, and pg_dump wants do that if only we had syntax for it. I agree with Yugo and Alvaro. It's better to have a separate syntax for locking views itself. https://www.postgresql.org/message-id/20171226143407.6wjzjn42pt54qskm@alvherre.pgsql Best regards, -- Tatsuo Ishii SRA OSS, Inc. Japan English: http://www.sraoss.co.jp/index_en.php Japanese:http://www.sraoss.co.jp
On Thu, Feb 1, 2018 at 8:09 PM, Tatsuo Ishii <ishii@sraoss.co.jp> wrote: > Initially I thought all base tables including ones in a subquery also > should be locked like you. But after some discussions with Yugo, I > agree that locking table in a subquery is less valuable for users (and > I am afraid it may introduce more deadlock chances). See upthead > discussions. I just reread those discussions but I don't see that they really make any argument for the behavior the patch implements. I see no explanation on the thread for why locking a table inside of a subquery is more or less likely to cause deadlock than locking one outside of a subquery. >> I think that if we >> change the rules for which subqueries get flattened in a future >> release, then the behavior will also change. That seems bad. > > I doubt it could happen in the future but if that happend we should > disallow locking on such views. That doesn't make any sense to me. When someone migrates from PostgreSQL 11 to, say, PostgreSQL 14, the view definition is going to be recreated from an SQL query. Neither the user nor the database will know whether the query was optimized the same way on both databases, so how could we disallow locking only those views where there was a difference on the two releases? Even if we could, how does that help anything? Throwing an error is just as much a backward-incompatibility in the command as silently changing what gets locked. But my complaint may have been a little off base all the same -- I guess we're doing this based on the rewriter output, rather than the optimizer output, which makes it a lot less likely that we would decide to change anything here. >> I also think that this is a bad idea for another reason, which is that >> it leaves us with no syntax to say that you want to lock the view >> itself, and pg_dump wants do that if only we had syntax for it. > > I agree with Yugo and Alvaro. It's better to have a separate syntax > for locking views itself. > > https://www.postgresql.org/message-id/20171226143407.6wjzjn42pt54qskm@alvherre.pgsql Hmm, Alvaro's argument makes sense, I guess. I withdraw this complaint. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Robert, > I just reread those discussions but I don't see that they really make > any argument for the behavior the patch implements. I see no > explanation on the thread for why locking a table inside of a subquery > is more or less likely to cause deadlock than locking one outside of a > subquery. If we allow to lock a table in a subquery, following could happen: We have: CREATE VIEW v1 AS SELECT * FROM t1 WHERE i = (SELECT i FROM t2); 1. Session A tries to lock v1 (I suppose it tries to acquire lock in the order of t1, then t2). A acquires lock on t1 but yet on t2. 2. Another session B acquires lock on t2. 3. A continues to try to acquire lock on t2 (blocked). 4. B tries to acquire lock on t1. Deadlock occurs. So if a user mixes locking a view and a underlying base table, there's a possibility of deadlocks. If we regard that it is user's responsibility not to mix them or to be careful about the consequence the mixing of locks so that dealocks do not happen, then I would agree that we should lock a table inside a subquery. What do you think? >>> I think that if we >>> change the rules for which subqueries get flattened in a future >>> release, then the behavior will also change. That seems bad. >> >> I doubt it could happen in the future but if that happend we should >> disallow locking on such views. > > That doesn't make any sense to me. When someone migrates from > PostgreSQL 11 to, say, PostgreSQL 14, the view definition is going to > be recreated from an SQL query. Neither the user nor the database > will know whether the query was optimized the same way on both > databases, so how could we disallow locking only those views where > there was a difference on the two releases? Even if we could, how > does that help anything? Throwing an error is just as much a > backward-incompatibility in the command as silently changing what gets > locked. > > But my complaint may have been a little off base all the same -- I > guess we're doing this based on the rewriter output, rather than the > optimizer output, which makes it a lot less likely that we would > decide to change anything here. Best regards, -- Tatsuo Ishii SRA OSS, Inc. Japan English: http://www.sraoss.co.jp/index_en.php Japanese:http://www.sraoss.co.jp
On Mon, Feb 5, 2018 at 8:18 PM, Tatsuo Ishii <ishii@sraoss.co.jp> wrote: > We have: CREATE VIEW v1 AS SELECT * FROM t1 WHERE i = (SELECT i FROM t2); > > 1. Session A tries to lock v1 (I suppose it tries to acquire lock in > the order of t1, then t2). A acquires lock on t1 but yet on t2. > > 2. Another session B acquires lock on t2. > > 3. A continues to try to acquire lock on t2 (blocked). > > 4. B tries to acquire lock on t1. Deadlock occurs. True. But the same exact analysis also applies to this definition, which contains no subquery: CREATE VIEW v1 AS SELECT t1.* FROM t1, t2 WHERE t1.i = t2.i; -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
> True. But the same exact analysis also applies to this definition, > which contains no subquery: > > CREATE VIEW v1 AS SELECT t1.* FROM t1, t2 WHERE t1.i = t2.i; That's not an updatable view, thus cannot be locked according to the proposed implementation. Anyway do you want to allow to lock all base tables in a view definition if the view is updatable? Best regards, -- Tatsuo Ishii SRA OSS, Inc. Japan English: http://www.sraoss.co.jp/index_en.php Japanese:http://www.sraoss.co.jp
On Mon, Feb 5, 2018 at 10:26 PM, Tatsuo Ishii <ishii@sraoss.co.jp> wrote: >> True. But the same exact analysis also applies to this definition, >> which contains no subquery: >> >> CREATE VIEW v1 AS SELECT t1.* FROM t1, t2 WHERE t1.i = t2.i; > > That's not an updatable view, thus cannot be locked according to the > proposed implementation. Hmm, true. Why exactly are we imposing the restriction to updateable views, anyway? -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
> Hmm, true. Why exactly are we imposing the restriction to updateable > views, anyway? In my understanding, because of ambiguity to determine which rows in which base tables needs to be modified by just looking at the DML against a view. There could be multiple ways to modify the base tables. Best regards, -- Tatsuo Ishii SRA OSS, Inc. Japan English: http://www.sraoss.co.jp/index_en.php Japanese:http://www.sraoss.co.jp
On Mon, Feb 5, 2018 at 10:49 PM, Tatsuo Ishii <ishii@sraoss.co.jp> wrote: >> Hmm, true. Why exactly are we imposing the restriction to updateable >> views, anyway? > > In my understanding, because of ambiguity to determine which rows in > which base tables needs to be modified by just looking at the DML > against a view. There could be multiple ways to modify the base > tables. But what does that have to do with locking? -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
> But what does that have to do with locking? Well, if the view is not updatable, I think there will be less point to allow to lock the base tables in the view because locking is typically used in a case when updates are required. Of course we could add special triggers to allow to update views that are not automatically updatable but that kind of views are tend to complex and IMO there's less need the automatic view locking feature. Best regards, -- Tatsuo Ishii SRA OSS, Inc. Japan English: http://www.sraoss.co.jp/index_en.php Japanese:http://www.sraoss.co.jp
On Tue, Feb 6, 2018 at 1:28 AM, Tatsuo Ishii <ishii@sraoss.co.jp> wrote: >> But what does that have to do with locking? > > Well, if the view is not updatable, I think there will be less point > to allow to lock the base tables in the view because locking is > typically used in a case when updates are required. > > Of course we could add special triggers to allow to update views that > are not automatically updatable but that kind of views are tend to > complex and IMO there's less need the automatic view locking feature. Hmm. Well, I see now why you've designed the feature in the way that you have, but I guess it still seems somewhat arbitrary to me. If you ignore the deadlock consideration, then there's no reason not to define the feature as locking every table mentioned anywhere in the query, including subqueries, and it can work for all views whether updatable or not. If the deadlock consideration is controlling, then I guess we can't do better than what you have, but I'm not sure how future-proof it is. If in the future somebody makes views updateable that involve a join, say from the primary key of one table to a unique key of another so that no duplicate rows can be introduced, then they'll either have to write code to make this feature identify and lock the "main" table, which I'm not sure would be strong enough in all cases, or lock them all, which reintroduces the deadlock problem. Personally, I would be inclined to view the deadlock problem as not very important. I just don't see how that is going to come up very often. What I do think will be an issue is that if you start locking lots of tables, you might prevent the system from getting much work done, whether or not you also cause any deadlocks. But I don't see what we can do about that, really. If users want full control over which tables get locked, then they have to name them explicitly. Or alternatively, maybe they should avoid the need for full-table locks by using SSI, gaining the benefits of (1) optimistic rather than pessimistic concurrency control, (2) finer-grained locking, and (3) not needing to issue explicit LOCK commands. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
On Tue, 6 Feb 2018 11:12:37 -0500 Robert Haas <robertmhaas@gmail.com> wrote: > On Tue, Feb 6, 2018 at 1:28 AM, Tatsuo Ishii <ishii@sraoss.co.jp> wrote: > >> But what does that have to do with locking? > > > > Well, if the view is not updatable, I think there will be less point > > to allow to lock the base tables in the view because locking is > > typically used in a case when updates are required. > > > > Of course we could add special triggers to allow to update views that > > are not automatically updatable but that kind of views are tend to > > complex and IMO there's less need the automatic view locking feature. > > Hmm. Well, I see now why you've designed the feature in the way that > you have, but I guess it still seems somewhat arbitrary to me. If you > ignore the deadlock consideration, then there's no reason not to > define the feature as locking every table mentioned anywhere in the > query, including subqueries, and it can work for all views whether > updatable or not. If the deadlock consideration is controlling, then > I guess we can't do better than what you have, but I'm not sure how > future-proof it is. If in the future somebody makes views updateable > that involve a join, say from the primary key of one table to a unique > key of another so that no duplicate rows can be introduced, then > they'll either have to write code to make this feature identify and > lock the "main" table, which I'm not sure would be strong enough in > all cases, or lock them all, which reintroduces the deadlock problem. > > Personally, I would be inclined to view the deadlock problem as not > very important. I just don't see how that is going to come up very I agree that the deadlock won't occur very often and this is not so important. I have updated the lockable-view patch to v8. This new version doen't consider the deadlock problem, and all tables or views appearing in the view definition query are locked recursively. Also, this allows all kinds of views to be locked even if it is not auto-updatable view. > often. What I do think will be an issue is that if you start locking > lots of tables, you might prevent the system from getting much work > done, whether or not you also cause any deadlocks. But I don't see > what we can do about that, really. If users want full control over > which tables get locked, then they have to name them explicitly. Or > alternatively, maybe they should avoid the need for full-table locks > by using SSI, gaining the benefits of (1) optimistic rather than > pessimistic concurrency control, (2) finer-grained locking, and (3) > not needing to issue explicit LOCK commands. > -- > Robert Haas > EnterpriseDB: http://www.enterprisedb.com > The Enterprise PostgreSQL Company -- Yugo Nagata <nagata@sraoss.co.jp>
Attachment
On Tue, 27 Mar 2018 23:28:04 +0900 Yugo Nagata <nagata@sraoss.co.jp> wrote: I found the previous patch was broken and this can't handle views that has subqueries as bellow; CREATE VIEW lock_view6 AS SELECT * from (select * from lock_tbl1) sub; I fixed this and attached the updated version including additional tests. Regards, > On Tue, 6 Feb 2018 11:12:37 -0500 > Robert Haas <robertmhaas@gmail.com> wrote: > > > On Tue, Feb 6, 2018 at 1:28 AM, Tatsuo Ishii <ishii@sraoss.co.jp> wrote: > > >> But what does that have to do with locking? > > > > > > Well, if the view is not updatable, I think there will be less point > > > to allow to lock the base tables in the view because locking is > > > typically used in a case when updates are required. > > > > > > Of course we could add special triggers to allow to update views that > > > are not automatically updatable but that kind of views are tend to > > > complex and IMO there's less need the automatic view locking feature. > > > > Hmm. Well, I see now why you've designed the feature in the way that > > you have, but I guess it still seems somewhat arbitrary to me. If you > > ignore the deadlock consideration, then there's no reason not to > > define the feature as locking every table mentioned anywhere in the > > query, including subqueries, and it can work for all views whether > > updatable or not. If the deadlock consideration is controlling, then > > I guess we can't do better than what you have, but I'm not sure how > > future-proof it is. If in the future somebody makes views updateable > > that involve a join, say from the primary key of one table to a unique > > key of another so that no duplicate rows can be introduced, then > > they'll either have to write code to make this feature identify and > > lock the "main" table, which I'm not sure would be strong enough in > > all cases, or lock them all, which reintroduces the deadlock problem. > > > > Personally, I would be inclined to view the deadlock problem as not > > very important. I just don't see how that is going to come up very > > I agree that the deadlock won't occur very often and this is not > so important. > > I have updated the lockable-view patch to v8. > > This new version doen't consider the deadlock problem, and all tables > or views appearing in the view definition query are locked recursively. > Also, this allows all kinds of views to be locked even if it is not > auto-updatable view. > > > > often. What I do think will be an issue is that if you start locking > > lots of tables, you might prevent the system from getting much work > > done, whether or not you also cause any deadlocks. But I don't see > > what we can do about that, really. If users want full control over > > which tables get locked, then they have to name them explicitly. Or > > alternatively, maybe they should avoid the need for full-table locks > > by using SSI, gaining the benefits of (1) optimistic rather than > > pessimistic concurrency control, (2) finer-grained locking, and (3) > > not needing to issue explicit LOCK commands. > > > > > -- > > Robert Haas > > EnterpriseDB: http://www.enterprisedb.com > > The Enterprise PostgreSQL Company > > > -- > Yugo Nagata <nagata@sraoss.co.jp> -- Yugo Nagata <nagata@sraoss.co.jp>
Attachment
> On Tue, 27 Mar 2018 23:28:04 +0900 > Yugo Nagata <nagata@sraoss.co.jp> wrote: > > I found the previous patch was broken and this can't handle > views that has subqueries as bellow; > > CREATE VIEW lock_view6 AS SELECT * from (select * from lock_tbl1) sub; > > I fixed this and attached the updated version including additional tests. This patch gives a warning while compiling: lockcmds.c:186:1: warning: no semicolon at end of struct or union } LockViewRecurse_context; ^ Best regards, -- Tatsuo Ishii SRA OSS, Inc. Japan English: http://www.sraoss.co.jp/index_en.php Japanese:http://www.sraoss.co.jp
>> I found the previous patch was broken and this can't handle >> views that has subqueries as bellow; >> >> CREATE VIEW lock_view6 AS SELECT * from (select * from lock_tbl1) sub; >> >> I fixed this and attached the updated version including additional tests. > > This patch gives a warning while compiling: > > lockcmds.c:186:1: warning: no semicolon at end of struct or union > } LockViewRecurse_context; > ^ Also I get a regression test failure: *** /usr/local/src/pgsql/current/postgresql/src/test/regress/expected/lock.out 2018-03-28 15:24:13.805314577 +0900 --- /usr/local/src/pgsql/current/postgresql/src/test/regress/results/lock.out 2018-03-28 15:42:07.975592594 +0900 *************** *** 118,124 **** ------------ lock_tbl1 lock_view6 ! (2 rows) ROLLBACK; -- Verify that we can lock a table with inheritance children. --- 118,125 ---- ------------ lock_tbl1 lock_view6 ! mvtest_tm ! (3 rows) Best regards, -- Tatsuo Ishii SRA OSS, Inc. Japan English: http://www.sraoss.co.jp/index_en.php Japanese:http://www.sraoss.co.jp
On Wed, 28 Mar 2018 15:45:09 +0900 (JST) Tatsuo Ishii <ishii@sraoss.co.jp> wrote: > >> I found the previous patch was broken and this can't handle > >> views that has subqueries as bellow; > >> > >> CREATE VIEW lock_view6 AS SELECT * from (select * from lock_tbl1) sub; > >> > >> I fixed this and attached the updated version including additional tests. > > > > This patch gives a warning while compiling: > > > > lockcmds.c:186:1: warning: no semicolon at end of struct or union > > } LockViewRecurse_context; > > ^ > > Also I get a regression test failure: Thank you for your reviewing my patch. I attached the updated patch, v10. Regards, > > *** /usr/local/src/pgsql/current/postgresql/src/test/regress/expected/lock.out 2018-03-28 15:24:13.805314577 +0900 > --- /usr/local/src/pgsql/current/postgresql/src/test/regress/results/lock.out 2018-03-28 15:42:07.975592594 +0900 > *************** > *** 118,124 **** > ------------ > lock_tbl1 > lock_view6 > ! (2 rows) > > ROLLBACK; > -- Verify that we can lock a table with inheritance children. > --- 118,125 ---- > ------------ > lock_tbl1 > lock_view6 > ! mvtest_tm > ! (3 rows) > > Best regards, > -- > Tatsuo Ishii > SRA OSS, Inc. Japan > English: http://www.sraoss.co.jp/index_en.php > Japanese:http://www.sraoss.co.jp -- Yugo Nagata <nagata@sraoss.co.jp>
Attachment
> On Wed, 28 Mar 2018 15:45:09 +0900 (JST) > Tatsuo Ishii <ishii@sraoss.co.jp> wrote: > >> >> I found the previous patch was broken and this can't handle >> >> views that has subqueries as bellow; >> >> >> >> CREATE VIEW lock_view6 AS SELECT * from (select * from lock_tbl1) sub; >> >> >> >> I fixed this and attached the updated version including additional tests. >> > >> > This patch gives a warning while compiling: >> > >> > lockcmds.c:186:1: warning: no semicolon at end of struct or union >> > } LockViewRecurse_context; >> > ^ >> >> Also I get a regression test failure: > > Thank you for your reviewing my patch. > I attached the updated patch, v10. Thanks. Looks good to me. I marked the patch as "Ready for Committer". Unless there's an objection, especially from Robert or Thomas Munro, I am going to commit/push it. Best regards, -- Tatsuo Ishii SRA OSS, Inc. Japan English: http://www.sraoss.co.jp/index_en.php Japanese:http://www.sraoss.co.jp
Hi,
On 2018-03-28 20:26:48 +0900, Yugo Nagata wrote:
> diff --git a/doc/src/sgml/ref/lock.sgml b/doc/src/sgml/ref/lock.sgml
> index b2c7203..96d477c 100644
> --- a/doc/src/sgml/ref/lock.sgml
> +++ b/doc/src/sgml/ref/lock.sgml
> @@ -46,6 +46,11 @@ LOCK [ TABLE ] [ ONLY ] <replaceable class="parameter">name</replaceable> [ * ]
> </para>
>
> <para>
> + When a view is specified to be locked, all relations appearing in the view
> + definition query are also locked recursively with the same lock mode.
> + </para>
Trailing space added. I'd remove "specified to be" from the sentence.
I think mentioning how this interacts with permissions would be a good
idea too. Given how operations use the view's owner to recurse, that's
not obvious. Should also explain what permissions are required to do the
operation on the view.
> @@ -86,15 +92,17 @@ RangeVarCallbackForLockTable(const RangeVar *rv, Oid relid, Oid oldrelid,
> return; /* woops, concurrently dropped; no permissions
> * check */
>
> - /* Currently, we only allow plain tables to be locked */
> - if (relkind != RELKIND_RELATION && relkind != RELKIND_PARTITIONED_TABLE)
> +
This newline looks spurious to me.
> /*
> + * Apply LOCK TABLE recursively over a view
> + *
> + * All tables and views appearing in the view definition query are locked
> + * recursively with the same lock mode.
> + */
> +
> +typedef struct
> +{
> + Oid root_reloid;
> + LOCKMODE lockmode;
> + bool nowait;
> + Oid viewowner;
> + Oid viewoid;
> +} LockViewRecurse_context;
Probably wouldn't hurt to pgindent the larger changes in the patch.
> +static bool
> +LockViewRecurse_walker(Node *node, LockViewRecurse_context *context)
> +{
> + if (node == NULL)
> + return false;
> +
> + if (IsA(node, Query))
> + {
> + Query *query = (Query *) node;
> + ListCell *rtable;
> +
> + foreach(rtable, query->rtable)
> + {
> + RangeTblEntry *rte = lfirst(rtable);
> + AclResult aclresult;
> +
> + Oid relid = rte->relid;
> + char relkind = rte->relkind;
> + char *relname = get_rel_name(relid);
> +
> + /* The OLD and NEW placeholder entries in the view's rtable are skipped. */
> + if (relid == context->viewoid &&
> + (!strcmp(rte->eref->aliasname, "old") || !strcmp(rte->eref->aliasname, "new")))
> + continue;
> +
> + /* Currently, we only allow plain tables or views to be locked. */
> + if (relkind != RELKIND_RELATION && relkind != RELKIND_PARTITIONED_TABLE &&
> + relkind != RELKIND_VIEW)
> + continue;
> +
> + /* Check infinite recursion in the view definition. */
> + if (relid == context->root_reloid)
> + ereport(ERROR,
> + (errcode(ERRCODE_INVALID_OBJECT_DEFINITION),
> + errmsg("infinite recursion detected in rules for relation \"%s\"",
> + get_rel_name(context->root_reloid))));
Hm, how can that happen? And if it can happen, why can it only happen
with the root relation?
Greetings,
Andres Freund
Andres, I have just pushed the v10 patch. Yugo will reply back to your point but I will look into your review as well. Thanks. Best regards, -- Tatsuo Ishii SRA OSS, Inc. Japan English: http://www.sraoss.co.jp/index_en.php Japanese:http://www.sraoss.co.jp > Hi, > > On 2018-03-28 20:26:48 +0900, Yugo Nagata wrote: >> diff --git a/doc/src/sgml/ref/lock.sgml b/doc/src/sgml/ref/lock.sgml >> index b2c7203..96d477c 100644 >> --- a/doc/src/sgml/ref/lock.sgml >> +++ b/doc/src/sgml/ref/lock.sgml >> @@ -46,6 +46,11 @@ LOCK [ TABLE ] [ ONLY ] <replaceable class="parameter">name</replaceable> [ * ] >> </para> >> >> <para> >> + When a view is specified to be locked, all relations appearing in the view >> + definition query are also locked recursively with the same lock mode. >> + </para> > > Trailing space added. I'd remove "specified to be" from the sentence. > > I think mentioning how this interacts with permissions would be a good > idea too. Given how operations use the view's owner to recurse, that's > not obvious. Should also explain what permissions are required to do the > operation on the view. > > >> @@ -86,15 +92,17 @@ RangeVarCallbackForLockTable(const RangeVar *rv, Oid relid, Oid oldrelid, >> return; /* woops, concurrently dropped; no permissions >> * check */ >> >> - /* Currently, we only allow plain tables to be locked */ >> - if (relkind != RELKIND_RELATION && relkind != RELKIND_PARTITIONED_TABLE) >> + > > This newline looks spurious to me. > > >> /* >> + * Apply LOCK TABLE recursively over a view >> + * >> + * All tables and views appearing in the view definition query are locked >> + * recursively with the same lock mode. >> + */ >> + >> +typedef struct >> +{ >> + Oid root_reloid; >> + LOCKMODE lockmode; >> + bool nowait; >> + Oid viewowner; >> + Oid viewoid; >> +} LockViewRecurse_context; > > Probably wouldn't hurt to pgindent the larger changes in the patch. > > >> +static bool >> +LockViewRecurse_walker(Node *node, LockViewRecurse_context *context) >> +{ >> + if (node == NULL) >> + return false; >> + >> + if (IsA(node, Query)) >> + { >> + Query *query = (Query *) node; >> + ListCell *rtable; >> + >> + foreach(rtable, query->rtable) >> + { >> + RangeTblEntry *rte = lfirst(rtable); >> + AclResult aclresult; >> + >> + Oid relid = rte->relid; >> + char relkind = rte->relkind; >> + char *relname = get_rel_name(relid); >> + >> + /* The OLD and NEW placeholder entries in the view's rtable are skipped. */ >> + if (relid == context->viewoid && >> + (!strcmp(rte->eref->aliasname, "old") || !strcmp(rte->eref->aliasname, "new"))) >> + continue; >> + >> + /* Currently, we only allow plain tables or views to be locked. */ >> + if (relkind != RELKIND_RELATION && relkind != RELKIND_PARTITIONED_TABLE && >> + relkind != RELKIND_VIEW) >> + continue; >> + >> + /* Check infinite recursion in the view definition. */ >> + if (relid == context->root_reloid) >> + ereport(ERROR, >> + (errcode(ERRCODE_INVALID_OBJECT_DEFINITION), >> + errmsg("infinite recursion detected in rules for relation \"%s\"", >> + get_rel_name(context->root_reloid)))); > > Hm, how can that happen? And if it can happen, why can it only happen > with the root relation? > > Greetings, > > Andres Freund
Tatsuo Ishii <ishii@sraoss.co.jp> writes:
> I have just pushed the v10 patch.
The buildfarm is fairly unhappy, and I think it's because of this patch.
regards, tom lane
>> I have just pushed the v10 patch. > > The buildfarm is fairly unhappy, and I think it's because of this patch. Thanks for the info. Yes, at least prion is unhappy because of the patch. I will look into this. Best regards, -- Tatsuo Ishii SRA OSS, Inc. Japan English: http://www.sraoss.co.jp/index_en.php Japanese:http://www.sraoss.co.jp
>> The buildfarm is fairly unhappy, and I think it's because of this patch. > > Thanks for the info. Yes, at least prion is unhappy because of the > patch. I will look into this. Done. See if the buildarm becomes happy. Best regards, -- Tatsuo Ishii SRA OSS, Inc. Japan English: http://www.sraoss.co.jp/index_en.php Japanese:http://www.sraoss.co.jp
On Thu, 29 Mar 2018 17:26:36 -0700
Andres Freund <andres@anarazel.de> wrote:
Thank you for your comments. I attach a patch to fix issues
you've pointed out.
> Hi,
>
> On 2018-03-28 20:26:48 +0900, Yugo Nagata wrote:
> > diff --git a/doc/src/sgml/ref/lock.sgml b/doc/src/sgml/ref/lock.sgml
> > index b2c7203..96d477c 100644
> > --- a/doc/src/sgml/ref/lock.sgml
> > +++ b/doc/src/sgml/ref/lock.sgml
> > @@ -46,6 +46,11 @@ LOCK [ TABLE ] [ ONLY ] <replaceable class="parameter">name</replaceable> [ * ]
> > </para>
> >
> > <para>
> > + When a view is specified to be locked, all relations appearing in the view
> > + definition query are also locked recursively with the same lock mode.
> > + </para>
>
> Trailing space added. I'd remove "specified to be" from the sentence.
Fixed.
>
> I think mentioning how this interacts with permissions would be a good
> idea too. Given how operations use the view's owner to recurse, that's
> not obvious. Should also explain what permissions are required to do the
> operation on the view.
Added a description about permissions into the documentation.
>
>
> > @@ -86,15 +92,17 @@ RangeVarCallbackForLockTable(const RangeVar *rv, Oid relid, Oid oldrelid,
> > return; /* woops, concurrently dropped; no permissions
> > * check */
> >
> > - /* Currently, we only allow plain tables to be locked */
> > - if (relkind != RELKIND_RELATION && relkind != RELKIND_PARTITIONED_TABLE)
> > +
>
> This newline looks spurious to me.
Removed.
>
>
> > /*
> > + * Apply LOCK TABLE recursively over a view
> > + *
> > + * All tables and views appearing in the view definition query are locked
> > + * recursively with the same lock mode.
> > + */
> > +
> > +typedef struct
> > +{
> > + Oid root_reloid;
> > + LOCKMODE lockmode;
> > + bool nowait;
> > + Oid viewowner;
> > + Oid viewoid;
> > +} LockViewRecurse_context;
>
> Probably wouldn't hurt to pgindent the larger changes in the patch.
>
>
> > +static bool
> > +LockViewRecurse_walker(Node *node, LockViewRecurse_context *context)
> > +{
> > + if (node == NULL)
> > + return false;
> > +
> > + if (IsA(node, Query))
> > + {
> > + Query *query = (Query *) node;
> > + ListCell *rtable;
> > +
> > + foreach(rtable, query->rtable)
> > + {
> > + RangeTblEntry *rte = lfirst(rtable);
> > + AclResult aclresult;
> > +
> > + Oid relid = rte->relid;
> > + char relkind = rte->relkind;
> > + char *relname = get_rel_name(relid);
> > +
> > + /* The OLD and NEW placeholder entries in the view's rtable are skipped. */
> > + if (relid == context->viewoid &&
> > + (!strcmp(rte->eref->aliasname, "old") || !strcmp(rte->eref->aliasname, "new")))
> > + continue;
> > +
> > + /* Currently, we only allow plain tables or views to be locked. */
> > + if (relkind != RELKIND_RELATION && relkind != RELKIND_PARTITIONED_TABLE &&
> > + relkind != RELKIND_VIEW)
> > + continue;
> > +
> > + /* Check infinite recursion in the view definition. */
> > + if (relid == context->root_reloid)
> > + ereport(ERROR,
> > + (errcode(ERRCODE_INVALID_OBJECT_DEFINITION),
> > + errmsg("infinite recursion detected in rules for relation \"%s\"",
> > + get_rel_name(context->root_reloid))));
>
> Hm, how can that happen? And if it can happen, why can it only happen
> with the root relation?
For example, the following queries cause the infinite recursion of views.
This is detected and the error is raised.
create table t (i int);
create view v1 as select 1;
create view v2 as select * from v1;
create or replace view v1 as select * from v2;
begin;
lock v1;
abort;
However, I found that the previous patch could not handle the following
situation in which the root relation itself doesn't have infinite recursion.
create view v3 as select * from v1;
begin;
lock v3;
abort;
This fixed in the attached patch.
Regards,
>
> Greetings,
>
> Andres Freund
--
Yugo Nagata <nagata@sraoss.co.jp>
Attachment
On Mon, 2 Apr 2018 18:32:53 +0900
Yugo Nagata <nagata@sraoss.co.jp> wrote:
> On Thu, 29 Mar 2018 17:26:36 -0700
> Andres Freund <andres@anarazel.de> wrote:
>
> Thank you for your comments. I attach a patch to fix issues
> you've pointed out.
I found a typo in the documentation and attach the updated patch.
Regards,
>
> > Hi,
> >
> > On 2018-03-28 20:26:48 +0900, Yugo Nagata wrote:
> > > diff --git a/doc/src/sgml/ref/lock.sgml b/doc/src/sgml/ref/lock.sgml
> > > index b2c7203..96d477c 100644
> > > --- a/doc/src/sgml/ref/lock.sgml
> > > +++ b/doc/src/sgml/ref/lock.sgml
> > > @@ -46,6 +46,11 @@ LOCK [ TABLE ] [ ONLY ] <replaceable class="parameter">name</replaceable> [ * ]
> > > </para>
> > >
> > > <para>
> > > + When a view is specified to be locked, all relations appearing in the view
> > > + definition query are also locked recursively with the same lock mode.
> > > + </para>
> >
> > Trailing space added. I'd remove "specified to be" from the sentence.
>
> Fixed.
>
> >
> > I think mentioning how this interacts with permissions would be a good
> > idea too. Given how operations use the view's owner to recurse, that's
> > not obvious. Should also explain what permissions are required to do the
> > operation on the view.
>
> Added a description about permissions into the documentation.
>
> >
> >
> > > @@ -86,15 +92,17 @@ RangeVarCallbackForLockTable(const RangeVar *rv, Oid relid, Oid oldrelid,
> > > return; /* woops, concurrently dropped; no permissions
> > > * check */
> > >
> > > - /* Currently, we only allow plain tables to be locked */
> > > - if (relkind != RELKIND_RELATION && relkind != RELKIND_PARTITIONED_TABLE)
> > > +
> >
> > This newline looks spurious to me.
>
> Removed.
>
> >
> >
> > > /*
> > > + * Apply LOCK TABLE recursively over a view
> > > + *
> > > + * All tables and views appearing in the view definition query are locked
> > > + * recursively with the same lock mode.
> > > + */
> > > +
> > > +typedef struct
> > > +{
> > > + Oid root_reloid;
> > > + LOCKMODE lockmode;
> > > + bool nowait;
> > > + Oid viewowner;
> > > + Oid viewoid;
> > > +} LockViewRecurse_context;
> >
> > Probably wouldn't hurt to pgindent the larger changes in the patch.
> >
> >
> > > +static bool
> > > +LockViewRecurse_walker(Node *node, LockViewRecurse_context *context)
> > > +{
> > > + if (node == NULL)
> > > + return false;
> > > +
> > > + if (IsA(node, Query))
> > > + {
> > > + Query *query = (Query *) node;
> > > + ListCell *rtable;
> > > +
> > > + foreach(rtable, query->rtable)
> > > + {
> > > + RangeTblEntry *rte = lfirst(rtable);
> > > + AclResult aclresult;
> > > +
> > > + Oid relid = rte->relid;
> > > + char relkind = rte->relkind;
> > > + char *relname = get_rel_name(relid);
> > > +
> > > + /* The OLD and NEW placeholder entries in the view's rtable are skipped. */
> > > + if (relid == context->viewoid &&
> > > + (!strcmp(rte->eref->aliasname, "old") || !strcmp(rte->eref->aliasname, "new")))
> > > + continue;
> > > +
> > > + /* Currently, we only allow plain tables or views to be locked. */
> > > + if (relkind != RELKIND_RELATION && relkind != RELKIND_PARTITIONED_TABLE &&
> > > + relkind != RELKIND_VIEW)
> > > + continue;
> > > +
> > > + /* Check infinite recursion in the view definition. */
> > > + if (relid == context->root_reloid)
> > > + ereport(ERROR,
> > > + (errcode(ERRCODE_INVALID_OBJECT_DEFINITION),
> > > + errmsg("infinite recursion detected in rules for relation \"%s\"",
> > > + get_rel_name(context->root_reloid))));
> >
> > Hm, how can that happen? And if it can happen, why can it only happen
> > with the root relation?
>
> For example, the following queries cause the infinite recursion of views.
> This is detected and the error is raised.
>
> create table t (i int);
> create view v1 as select 1;
> create view v2 as select * from v1;
> create or replace view v1 as select * from v2;
> begin;
> lock v1;
> abort;
>
> However, I found that the previous patch could not handle the following
> situation in which the root relation itself doesn't have infinite recursion.
>
> create view v3 as select * from v1;
> begin;
> lock v3;
> abort;
>
> This fixed in the attached patch.
>
> Regards,
>
> >
> > Greetings,
> >
> > Andres Freund
>
>
> --
> Yugo Nagata <nagata@sraoss.co.jp>
--
Yugo Nagata <nagata@sraoss.co.jp>
Attachment
>> > > +typedef struct
>> > > +{
>> > > + Oid root_reloid;
>> > > + LOCKMODE lockmode;
>> > > + bool nowait;
>> > > + Oid viewowner;
>> > > + Oid viewoid;
>> > > +} LockViewRecurse_context;
>> >
>> > Probably wouldn't hurt to pgindent the larger changes in the patch.
Yeah. Also, each struct member needs a comment.
>> > Hm, how can that happen? And if it can happen, why can it only happen
>> > with the root relation?
>>
>> For example, the following queries cause the infinite recursion of views.
>> This is detected and the error is raised.
>>
>> create table t (i int);
>> create view v1 as select 1;
>> create view v2 as select * from v1;
>> create or replace view v1 as select * from v2;
>> begin;
>> lock v1;
>> abort;
>>
>> However, I found that the previous patch could not handle the following
>> situation in which the root relation itself doesn't have infinite recursion.
>>
>> create view v3 as select * from v1;
>> begin;
>> lock v3;
>> abort;
Shouldn't they be in the regression test?
It's shame that create_view test does not include the cases, but it's
another story.
Best regards,
--
Tatsuo Ishii
SRA OSS, Inc. Japan
English: http://www.sraoss.co.jp/index_en.php
Japanese:http://www.sraoss.co.jp
On Thu, 05 Apr 2018 07:53:42 +0900 (JST)
Tatsuo Ishii <ishii@sraoss.co.jp> wrote:
I update the patch to fix the lockable view issues.
> >> > > +typedef struct
> >> > > +{
> >> > > + Oid root_reloid;
> >> > > + LOCKMODE lockmode;
> >> > > + bool nowait;
> >> > > + Oid viewowner;
> >> > > + Oid viewoid;
> >> > > +} LockViewRecurse_context;
> >> >
> >> > Probably wouldn't hurt to pgindent the larger changes in the patch.
>
> Yeah. Also, each struct member needs a comment.
I applied pgindent and added comments to struct members.
>
> >> > Hm, how can that happen? And if it can happen, why can it only happen
> >> > with the root relation?
> >>
> >> For example, the following queries cause the infinite recursion of views.
> >> This is detected and the error is raised.
> >>
> >> create table t (i int);
> >> create view v1 as select 1;
> >> create view v2 as select * from v1;
> >> create or replace view v1 as select * from v2;
> >> begin;
> >> lock v1;
> >> abort;
> >>
> >> However, I found that the previous patch could not handle the following
> >> situation in which the root relation itself doesn't have infinite recursion.
> >>
> >> create view v3 as select * from v1;
> >> begin;
> >> lock v3;
> >> abort;
>
> Shouldn't they be in the regression test?
Added tests for the infinite recursion detection.
Regards,
>
> It's shame that create_view test does not include the cases, but it's
> another story.
>
> Best regards,
> --
> Tatsuo Ishii
> SRA OSS, Inc. Japan
> English: http://www.sraoss.co.jp/index_en.php
> Japanese:http://www.sraoss.co.jp
--
Yugo Nagata <nagata@sraoss.co.jp>