Re: [HACKERS] WIP: Automatic view update rules
Here is my latest reworked patch that fixes all outstanding issues. view_update-petere-20090121.patch.bz2 Description: Binary data -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] WIP: Automatic view update rules
On Wed, Jan 21, 2009 at 11:09 AM, Peter Eisentraut pete...@gmx.net wrote: Here is my latest reworked patch that fixes all outstanding issues. 1) there's a regression failure, it's just a message that changes... attached regression.diffs 2) this comment on src/backend/rewrite/viewUpdate.c:form_where_for_updrule() is no longer true /* * Finally, create the OpExpr node, as part of a CaseExpr and * include the OpExpr as part of the Case for managing NULL's * we will do this everytime. That way we will have no * problem with: * * ALTER TABLE ... ALTER COLUMN ... DROP NOT NULL; */ -- Atentamente, Jaime Casanova Soporte y capacitación de PostgreSQL Asesoría y desarrollo de sistemas Guayaquil - Ecuador Cel. +59387171157 regression.diffs Description: Binary data -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] WIP: Automatic view update rules
--On Samstag, Januar 17, 2009 02:01:15 +0200 Peter Eisentraut pete...@gmx.net wrote: * It is not clear how automatic rules and manual DO ALSO rules should interact. A manual DO ALSO rule will currently clear out an automatic INSTEAD rule, which I find to be illogical. My intentional feeling was that it would be a bad idea to leave any implicit rule when someone is going to create his own rule set on a view, at least to avoid any confusion or side effects. Consider someone having his own rules upgrading from an older version. He must have at least his own DO INSTEAD Rule, it doesn't make any sense to have his own DO ALSO Rule without an INSTEAD one. Thus, doing it this way will leave the view as expected from the original setup. *thinking more*...if we teach explicit DO ALSO rules *not* to clean out implicit ones, we will have the following workflows: a) View is updatable, has its own automatic DO INSTEAD rule: if someone is restoring his old update rules, he will have at least his own DO INSTEAD rule. This will drop any corresponding automatically created rule, adding his own DO INSTEAD rule and any DO ALSO rule. b) View is updatable, has its own automatic DO INSTEAD rule: The user is able to create any additional DO ALSO rule. I don't see any problems here, as long as the implicit DO INSTEAD rule gets replaced. Opinions? -- Thanks Bernd -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] WIP: Automatic view update rules
Bernd Helmle wrote: --On Samstag, Januar 17, 2009 02:01:15 +0200 Peter Eisentraut pete...@gmx.net wrote: * It is not clear how automatic rules and manual DO ALSO rules should interact. A manual DO ALSO rule will currently clear out an automatic INSTEAD rule, which I find to be illogical. My intentional feeling was that it would be a bad idea to leave any implicit rule when someone is going to create his own rule set on a view, at least to avoid any confusion or side effects. Consider someone having his own rules upgrading from an older version. He must have at least his own DO INSTEAD Rule, it doesn't make any sense to have his own DO ALSO Rule without an INSTEAD one. Thus, doing it this way will leave the view as expected from the original setup. *thinking more*...if we teach explicit DO ALSO rules *not* to clean out implicit ones, we will have the following workflows: a) View is updatable, has its own automatic DO INSTEAD rule: if someone is restoring his old update rules, he will have at least his own DO INSTEAD rule. This will drop any corresponding automatically created rule, adding his own DO INSTEAD rule and any DO ALSO rule. b) View is updatable, has its own automatic DO INSTEAD rule: The user is able to create any additional DO ALSO rule. I don't see any problems here, as long as the implicit DO INSTEAD rule gets replaced. Someone who previously had a DO INSTEAD rule to effect updatable views as well as a DO ALSO rule to create some side effect (e.g., auditing/logging), will after the upgrade perhaps want to rely on the automatic view update rules but will still want to create his own DO ALSO rule. In the current patch, the creation of the DO ALSO rule will delete the automatic view update rules, thus breaking the entire scenario. Considering that the updatable views feature deals only with DO INSTEAD rules, we should leave DO ALSO rules completely alone. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] WIP: Automatic view update rules
--On Samstag, Januar 17, 2009 02:01:15 +0200 Peter Eisentraut pete...@gmx.net wrote: Here is my updated patch based on yours. Outstanding issues, as far as I can see, are: Critical: * Updatability check must reject views where the select list references the same column more than once. checkTree() will recheck this against reused varattno's within the query tree and will reject such view definitions as non-updatable now. * Various scenarios of REPLACE VIEW misbehave. I have marked these as FIXME in the regression test. I think all this would behave better if REPLACE VIEW dropped all automatic rules and reassembled them from scratch for the new view. The infrastructure for this is already there, so it should be a small change. DefineViewRules() will drop all implicit rules when REPLACE is used now. Important: * Array support should be clarified. checkTree() appears to reject most variants of array references, but other parts of the code try to handle it. Should be cleaned up. I'm currently working on this. * It is not clear how automatic rules and manual DO ALSO rules should interact. A manual DO ALSO rule will currently clear out an automatic INSTEAD rule, which I find to be illogical. What i've done so far is to replace implicit DO INSTEAD rules only, when the new explicit one is DO INSTEAD, too. An additional DO ALSO rule will be added to the view without dropping any automatic rules now. Optional: * The use of must_replace is create_update_rule() seems a bit useless. You might as well just always pass replace = true. Fixed * You may want to consider writing the rule qualifications WHERE ((CASE WHEN (old.a IS NOT NULL) THEN (old.a = vutestv20.x) ELSE (vutestv20.x IS NULL) END)) more like WHERE ((old.a = vutestv20.x) OR (old IS NULL AND vutestv20.x IS NULL)) for better optimizability. Done. Please note that i haven't fixed the regression test yet. Thanks very much for reviewing! -- Thanks Bernd view_update_20090119.patch.bz2 Description: Binary data -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] WIP: Automatic view update rules
Here is my updated patch based on yours. Outstanding issues, as far as I can see, are: Critical: * Updatability check must reject views where the select list references the same column more than once. * Various scenarios of REPLACE VIEW misbehave. I have marked these as FIXME in the regression test. I think all this would behave better if REPLACE VIEW dropped all automatic rules and reassembled them from scratch for the new view. The infrastructure for this is already there, so it should be a small change. Important: * Array support should be clarified. checkTree() appears to reject most variants of array references, but other parts of the code try to handle it. Should be cleaned up. * It is not clear how automatic rules and manual DO ALSO rules should interact. A manual DO ALSO rule will currently clear out an automatic INSTEAD rule, which I find to be illogical. Optional: * The use of must_replace is create_update_rule() seems a bit useless. You might as well just always pass replace = true. * You may want to consider writing the rule qualifications WHERE ((CASE WHEN (old.a IS NOT NULL) THEN (old.a = vutestv20.x) ELSE (vutestv20.x IS NULL) END)) more like WHERE ((old.a = vutestv20.x) OR (old IS NULL AND vutestv20.x IS NULL)) for better optimizability. CASE will be quite bad for optimization, and then you might as well go with IS DISTINCT FROM, which is just as bad but simpler. view_update-petere-20090117.patch.bz2 Description: Binary data -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] WIP: Automatic view update rules
Peter Eisentraut pete...@gmx.net writes: CASE will be quite bad for optimization, and then you might as well go with IS DISTINCT FROM, which is just as bad but simpler. Definitely avoid CASE if you can. Although the planner currently knows nothing about IS DISTINCT FROM, that's fixable in principle. We'll probably never be able to optimize CASE conditions very much because of their special evaluation rules. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] WIP: Automatic view update rules
Bernd Helmle wrote: --On Freitag, Januar 09, 2009 17:53:31 +0100 Bernd Helmle maili...@oopsware.de wrote: I've decided to check updatability of all involved views during view creation. Please find attached a new version with all other open issues adressed. Oops, forgot to track some files in my new git branch and so the new files viewUpdate.c und viewUpdate.h are missing...please find attached a corrected patch file. Sorry for that. gcc -no-cpp-precomp -O2 -Wall -Wmissing-prototypes -Wpointer-arith -Wdeclaration-after-statement -Wendif-labels -fno-strict-aliasing -fwrapv -g -I../../../src/include -I/sw/include/libxml2 -I/sw/include -c -o viewUpdate.o viewUpdate.c -MMD -MP -MF .deps/viewUpdate.Po viewUpdate.c: In function 'transform_select_to_insert': viewUpdate.c:1407: error: 'qual' undeclared (first use in this function) viewUpdate.c:1407: error: (Each undeclared identifier is reported only once viewUpdate.c:1407: error: for each function it appears in.) viewUpdate.c:1407: error: 'viewDef' undeclared (first use in this function) viewUpdate.c: In function 'CreateViewUpdateRules': viewUpdate.c:1731: warning: unused variable 'view_qual' make: *** [viewUpdate.o] Error 1 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] WIP: Automatic view update rules
--On Montag, Januar 12, 2009 14:48:46 +0200 Peter Eisentraut pete...@gmx.net wrote: gcc -no-cpp-precomp -O2 -Wall -Wmissing-prototypes -Wpointer-arith -Wdeclaration-after-statement -Wendif-labels -fno-strict-aliasing -fwrapv -g -I../../../src/include -I/sw/include/libxml2 -I/sw/include -c -o viewUpdate.o viewUpdate.c -MMD -MP -MF .deps/viewUpdate.Po viewUpdate.c: In function 'transform_select_to_insert': viewUpdate.c:1407: error: 'qual' undeclared (first use in this function) viewUpdate.c:1407: error: (Each undeclared identifier is reported only once viewUpdate.c:1407: error: for each function it appears in.) viewUpdate.c:1407: error: 'viewDef' undeclared (first use in this function) viewUpdate.c: In function 'CreateViewUpdateRules': viewUpdate.c:1731: warning: unused variable 'view_qual' make: *** [viewUpdate.o] Error 1 Fixed. -- Thanks Bernd view_update.patch.bz2 Description: Binary data -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] WIP: Automatic view update rules
--On Freitag, Januar 09, 2009 17:53:31 +0100 Bernd Helmle maili...@oopsware.de wrote: I've decided to check updatability of all involved views during view creation. Please find attached a new version with all other open issues adressed. Oops, forgot to track some files in my new git branch and so the new files viewUpdate.c und viewUpdate.h are missing...please find attached a corrected patch file. Sorry for that. -- Thanks Bernd view_update.patch.bz2 Description: Binary data -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] WIP: Automatic view update rules
--On Sonntag, Dezember 28, 2008 15:29:58 +0100 Bernd Helmle be...@oopsware.de wrote: maybe the better solution is to not allow such a view to be updatable Yes, it seems we have to check for target lists having negative attnums in checkTree(). Another solution would be to simply ignore those columns (extract them from the target list and include all updatable columns only). While looking at this it turned out that the problem of updatability is more complex than i originally thought: Consider a relation tree of the following views/relations: View1 - View2 - View3 - Table1 That means, View1 consists of View2 and so on. What happens now if someone is going to change View3, so that it's not updatable anymore? What the patch actually does is, scanning all relations/views involved in a current view (and cascading updates) und reject update rules as soon as it finds more than one relation within a view definition. Unfortunately this seems not to be enough, we had really check all involved views for updatability recursively. The infrastructure for this is already there, but i wonder if it could be made easier when we are going to maintain a separate is_updatable flag somewhere in the catalog, which would make checking the relation tree for updatability more easier. Comments? -- Thanks Bernd -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] WIP: Automatic view update rules
--On Freitag, Januar 09, 2009 13:20:57 +0100 Bernd Helmle maili...@oopsware.de wrote: That means, View1 consists of View2 and so on. What happens now if someone is going to change View3, so that it's not updatable anymore? What the patch actually does is, scanning all relations/views involved in a current view (and cascading updates) und reject update rules as soon as it finds more than one relation within a view definition. Unfortunately this seems not to be enough, we had really check all involved views for updatability recursively. The infrastructure for this is already there, but i wonder if it could be made easier when we are going to maintain a separate is_updatable flag somewhere in the catalog, which would make checking the relation tree for updatability more easier. I've decided to check updatability of all involved views during view creation. Please find attached a new version with all other open issues adressed. -- Thanks Bernd view_update.patch.bz2 Description: Binary data -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] WIP: Automatic view update rules
On Sun, Dec 28, 2008 at 9:29 AM, Bernd Helmle be...@oopsware.de wrote: i would say check for negative attnums and deny that view to be updateable because of SQL92 says in 11.19 view definition syntax rule 6: 6) If the query expression is updatable, then the viewed table is an updatable table. Otherwise, it is a read-only table. wich i understand as deny updatability in any view that constains non updateable query expression in the target list Yes, but afaiks SQL99 allows partial updatable column lists, so we could argue that we can follow this. However, it seems your approach is better for now. yes. if we didn't do that we will be against spec. syntax rule 12 (again in 11.19 view definition ) says: 12)If WITH CHECK OPTION is specified, then the viewed table shall be updatable. Okay. I'm currently travelling (visiting my parents during turn of the year), checking my mail sporadically. I'll provide an updated patch ASAP. Bernd -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] WIP: Automatic view update rules
2) Another less important bug, the WITH CHECK OPTION is accepted even when that functionality is not implemented. updatable_views=# create or replace view v2 as select * from foo where id 10 with check option; NOTICE: CREATE VIEW will create implicit INSERT/UPDATE/DELETE rules CREATE VIEW What do we want in this case? We can throw an error telling that CHECK OPTION isn't supported yet or simply issueing a warning. +1 for an error. ...Robert -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] WIP: Automatic view update rules
On Mon, Dec 22, 2008 at 8:53 AM, Bernd Helmle maili...@oopsware.de wrote: --On Mittwoch, November 26, 2008 10:54:01 +0100 Bernd Helmle maili...@oopsware.de wrote: Okay, i've finally managed to create an updated version with (hopefully) all issues mentioned by Robert adressed. Hi Bernd, 1) i found a crash type bug, try this: create table foo ( id integer not nullprimary key, namevarchar(30) ) with oids; create view foo_view as select oid, * from foo; with this you will get an error like this one: ERROR: RETURNING list's entry 1 has different type from column oid Hrm, seems i've introduced a bug while implementing RETURNING support. but if you make this: alter table foo add column description text; create view foo_view as select oid, * from foo; then, the server crash. STATEMENT: create or replace view v_foo as select oid, * from foo; LOG: server process (PID 16320) was terminated by signal 11: Segmentation fault LOG: terminating any other active server processes maybe the better solution is to not allow such a view to be updatable Yes, it seems we have to check for target lists having negative attnums in checkTree(). Another solution would be to simply ignore those columns (extract them from the target list and include all updatable columns only). 2) Another less important bug, the WITH CHECK OPTION is accepted even when that functionality is not implemented. updatable_views=# create or replace view v2 as select * from foo where id 10 with check option; NOTICE: CREATE VIEW will create implicit INSERT/UPDATE/DELETE rules CREATE VIEW What do we want in this case? We can throw an error telling that CHECK OPTION isn't supported yet or simply issueing a warning. 3) one final point: seems like you'll have to update the rules regression test (attached the regression.diffs) okay Thanks for the review so far. Bernd -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] WIP: Automatic view update rules
On Sun, Dec 28, 2008 at 9:29 AM, Bernd Helmle be...@oopsware.de wrote: Yes, it seems we have to check for target lists having negative attnums in checkTree(). Another solution would be to simply ignore those columns (extract them from the target list and include all updatable columns only). i would say check for negative attnums and deny that view to be updateable because of SQL92 says in 11.19 view definition syntax rule 6: 6) If the query expression is updatable, then the viewed table is an updatable table. Otherwise, it is a read-only table. wich i understand as deny updatability in any view that constains non updateable query expression in the target list 2) Another less important bug, the WITH CHECK OPTION is accepted even when that functionality is not implemented. updatable_views=# create or replace view v2 as select * from foo where id 10 with check option; NOTICE: CREATE VIEW will create implicit INSERT/UPDATE/DELETE rules CREATE VIEW What do we want in this case? We can throw an error telling that CHECK OPTION isn't supported yet or simply issueing a warning. yes. if we didn't do that we will be against spec. syntax rule 12 (again in 11.19 view definition ) says: 12)If WITH CHECK OPTION is specified, then the viewed table shall be updatable. -- Atentamente, Jaime Casanova Soporte y capacitación de PostgreSQL Asesoría y desarrollo de sistemas Guayaquil - Ecuador Cel. +59387171157 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] WIP: Automatic view update rules
On Mon, Dec 22, 2008 at 8:53 AM, Bernd Helmle maili...@oopsware.de wrote: --On Mittwoch, November 26, 2008 10:54:01 +0100 Bernd Helmle maili...@oopsware.de wrote: Okay, i've finally managed to create an updated version with (hopefully) all issues mentioned by Robert adressed. Hi Bernd, 1) i found a crash type bug, try this: create table foo ( id integer not nullprimary key, namevarchar(30) ) with oids; create view foo_view as select oid, * from foo; with this you will get an error like this one: ERROR: RETURNING list's entry 1 has different type from column oid but if you make this: alter table foo add column description text; create view foo_view as select oid, * from foo; then, the server crash. STATEMENT: create or replace view v_foo as select oid, * from foo; LOG: server process (PID 16320) was terminated by signal 11: Segmentation fault LOG: terminating any other active server processes maybe the better solution is to not allow such a view to be updatable 2) Another less important bug, the WITH CHECK OPTION is accepted even when that functionality is not implemented. updatable_views=# create or replace view v2 as select * from foo where id 10 with check option; NOTICE: CREATE VIEW will create implicit INSERT/UPDATE/DELETE rules CREATE VIEW 3) one final point: seems like you'll have to update the rules regression test (attached the regression.diffs) -- Atentamente, Jaime Casanova Soporte y capacitación de PostgreSQL Asesoría y desarrollo de sistemas Guayaquil - Ecuador Cel. +59387171157 regression.diffs Description: Binary data -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] WIP: Automatic view update rules
--On Mittwoch, November 26, 2008 10:54:01 +0100 Bernd Helmle maili...@oopsware.de wrote: --On Dienstag, November 25, 2008 23:43:02 -0500 Robert Haas robertmh...@gmail.com wrote: Do you intend to submit an updated version of this patch for this commitfest? I'll do asap, i've updated the status to 'waiting on author'. Okay, i've finally managed to create an updated version with (hopefully) all issues mentioned by Robert adressed. -- Thanks Bernd view_update.patch.bz2 Description: Binary data -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] WIP: Automatic view update rules
--On Dienstag, November 25, 2008 23:43:02 -0500 Robert Haas [EMAIL PROTECTED] wrote: Do you intend to submit an updated version of this patch for this commitfest? I'll do asap, i've updated the status to 'waiting on author'. -- Thanks Bernd -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] WIP: Automatic view update rules
Bernd, Do you intend to submit an updated version of this patch for this commitfest? If not, I will move this to Returned with feedback. Thanks, ...Robert -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] WIP: Automatic view update rules
--On Dienstag, November 11, 2008 23:06:08 -0500 Robert Haas [EMAIL PROTECTED] wrote: Thanks for your look at this. Unfortunately i was travelling the last 2 days, so i didn't have time to reply earlier, sorry for that. I haven't done a full review of this patch, but here are some thoughts based on a quick read-through: - make check fails 16 of 118 tests for me with this patch applied. Most of them are caused by additional NOTICE messages or unexpected additional rules in the rewriter regression tests. I don't see anything critical here. - There are some unnecessary hunks in this diff. For example, some of the gram.y changes appear to move curly braces around, adjust spacing, hmm i didn't see any changes to brackets or adjusting spaces... and replace true and false with TRUE and FALSE (I'm not 100% sure that the last of these isn't substantive... but I hope it's not). The changes to rewriteDefine.c contain some commented-out declarations that need to be cleaned up, and at least one place where a blank line has been added. - The doc changes for ev_kind describe 'e' as meaning rule was created by user, which confused me because why would you pick e for that? Then I realized that e was probably intended to mean explicit; it would probably be good to work that word into the documentation of that value somehow. okay - Should this be an optional behavior? What if I don't WANT my view to be updateable? I didn't see anything like this in the SQL92 draft...i thought if a view is updatable, it is, without any further adjustments. If you don't want your view updatable you have to REVOKE or GRANT your specific ACLs. - I am wondering if the old_tup_is_implicit variable started off its life as a boolean and morphed into a char. It seems misnamed, now, at any rate. agreed - The capitalization of deleteImplicitRulesOnEvent is inconsistent with the functions that immediately precede it in rewriteRemove.h. I think the d should be capitalized. checkTree() also uses this style of capitalization, which I haven't seen elsewhere in the source tree. agreed - This: rhaas=# create table baz (a integer, b integer); CREATE TABLE rhaas=# create or replace view bar as select * from baz; NOTICE: CREATE VIEW will create implicit INSERT/UPDATE/DELETE rules CREATE VIEW Generates this update rule: ON UPDATE TO bar DO INSTEAD UPDATE ONLY foo SET a = new.a, b = new.b WHERE CASE WHEN old.a IS NOT NULL THEN old.a = foo.a ELSE foo.a IS NULL END AND CASE WHEN old.b IS NOT NULL THEN old.b = foo.b ELSE foo.b IS NULL END RETURNING new.a, new.b It seems like this could be simplified using IS NOT DISTINCT FROM. I'll look at this. -- Thanks Bernd -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] WIP: Automatic view update rules
On Nov 11, 2008, at 10:06 PM, Robert Haas wrote: - Should this be an optional behavior? What if I don't WANT my view to be updateable? That seems like a deal-breaker to me... many users could easily be depending on views not being updateable. Views are generally always thought of as read-only, so you should need to explicitly mark a view as being updateable/insertable/deleteable. It's tempting to try and use permissions to try and handle this, but I don't think that's safe either: nothing prevents you from doing GRANT ALL on a view with no rules, and such a view would suddenly become updateable. -- Decibel!, aka Jim C. Nasby, Database Architect [EMAIL PROTECTED] Give your computer some brain candy! www.distributed.net Team #1828 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] WIP: Automatic view update rules
Decibel! [EMAIL PROTECTED] writes: That seems like a deal-breaker to me... many users could easily be depending on views not being updateable. Views are generally always thought of as read-only, so you should need to explicitly mark a view as being updateable/insertable/deleteable. The SQL standard says differently ... regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] WIP: Automatic view update rules
- make check fails 16 of 118 tests for me with this patch applied. Most of them are caused by additional NOTICE messages or unexpected additional rules in the rewriter regression tests. I don't see anything critical here. Possible; in that case you should patch the expected regression output appropriately. But I seem to remember thinking that \d was producing the wrong column list on one of the system catalogs you modified, so you might want to double check that part... maybe I'm all wet. ...Robert -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] WIP: Automatic view update rules
I haven't done a full review of this patch, but here are some thoughts based on a quick read-through: - make check fails 16 of 118 tests for me with this patch applied. - There are some unnecessary hunks in this diff. For example, some of the gram.y changes appear to move curly braces around, adjust spacing, and replace true and false with TRUE and FALSE (I'm not 100% sure that the last of these isn't substantive... but I hope it's not). The changes to rewriteDefine.c contain some commented-out declarations that need to be cleaned up, and at least one place where a blank line has been added. - The doc changes for ev_kind describe 'e' as meaning rule was created by user, which confused me because why would you pick e for that? Then I realized that e was probably intended to mean explicit; it would probably be good to work that word into the documentation of that value somehow. - Should this be an optional behavior? What if I don't WANT my view to be updateable? - I am wondering if the old_tup_is_implicit variable started off its life as a boolean and morphed into a char. It seems misnamed, now, at any rate. - The capitalization of deleteImplicitRulesOnEvent is inconsistent with the functions that immediately precede it in rewriteRemove.h. I think the d should be capitalized. checkTree() also uses this style of capitalization, which I haven't seen elsewhere in the source tree. - This: rhaas=# create table baz (a integer, b integer); CREATE TABLE rhaas=# create or replace view bar as select * from baz; NOTICE: CREATE VIEW will create implicit INSERT/UPDATE/DELETE rules CREATE VIEW Generates this update rule: ON UPDATE TO bar DO INSTEAD UPDATE ONLY foo SET a = new.a, b = new.b WHERE CASE WHEN old.a IS NOT NULL THEN old.a = foo.a ELSE foo.a IS NULL END AND CASE WHEN old.b IS NOT NULL THEN old.b = foo.b ELSE foo.b IS NULL END RETURNING new.a, new.b It seems like this could be simplified using IS NOT DISTINCT FROM. ...Robert -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] WIP: Automatic view update rules
--On Donnerstag, Oktober 30, 2008 21:24:08 +0100 Bernd Helmle [EMAIL PROTECTED] wrote: Note that i'm still working on this (for example, RETURNING is missing yet), As always, discussion welcome ;) This new version implements RETURNING support for implicit view update rules and does some further cleanups. -- Thanks Bernd view_update.patch.bz2 Description: Binary data -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
