Thread: Updatable views
Hi, This is the patch for updatable views I've been able to come up with. A nasty bug was just discovered in the upcoming Mammoth Replicator release so I'm not sure if I'm going to have time to work more on it soon. So, I'll appreciate if somebody else takes the responsability to fix the remaining issues. I've put a lot of XXX's and some FIXME's. Some functions are in need of some comments as well. The new files are src/backend/rewrite/viewUpdate.c and src/include/rewrite/viewUpdate.h. The third file, upd-views.sql, is intended to be a new regression test. Extra points if the table therein is completed correctly. I haven't tested the array stuff at all. Comments from Bernd and Jaime are especially welcome if I've broken something that used to work on their patch :-) -- Alvaro Herrera http://www.CommandPrompt.com/ The PostgreSQL Company - Command Prompt, Inc.
Attachment
--On Montag, August 21, 2006 02:07:41 -0400 Alvaro Herrera
<alvherre@commandprompt.com> wrote:
> Hi,
>
> This is the patch for updatable views I've been able to come up with. A
> nasty bug was just discovered in the upcoming Mammoth Replicator release
> so I'm not sure if I'm going to have time to work more on it soon.
>
> So, I'll appreciate if somebody else takes the responsability to fix the
> remaining issues. I've put a lot of XXX's and some FIXME's.
> Some
> functions are in need of some comments as well.
>
I'll try to complete the missing comments and to make some statements
cleaner.
> The new files are src/backend/rewrite/viewUpdate.c and
> src/include/rewrite/viewUpdate.h. The third file, upd-views.sql, is
> intended to be a new regression test. Extra points if the table therein
> is completed correctly.
>
> I haven't tested the array stuff at all.
>
> Comments from Bernd and Jaime are especially welcome if I've broken
> something that used to work on their patch :-)
I see that the current patch doesn't support subqueries in the WHERE-clause
anymore.
You can find one example in the attached SQL-script. Is there a reason why
you
dropped this?
--
Thanks
Bernd
Attachment
Bernd Helmle wrote: > I'll try to complete the missing comments and to make some statements > cleaner. Thanks. > --On Montag, August 21, 2006 02:07:41 -0400 Alvaro Herrera > <alvherre@commandprompt.com> wrote: > >The new files are src/backend/rewrite/viewUpdate.c and > >src/include/rewrite/viewUpdate.h. The third file, upd-views.sql, is > >intended to be a new regression test. Extra points if the table therein > >is completed correctly. > > > >I haven't tested the array stuff at all. > > > >Comments from Bernd and Jaime are especially welcome if I've broken > >something that used to work on their patch :-) > > I see that the current patch doesn't support subqueries in the WHERE-clause > anymore. > You can find one example in the attached SQL-script. Is there a reason why > you dropped this? Nope, no reason. If you can find _where_ I broke it, that would be the first clue to unbreaking it ;-) -- Alvaro Herrera http://www.CommandPrompt.com/ PostgreSQL Replication, Consulting, Custom Development, 24x7 support
Bernd Helmle wrote:
> >Comments from Bernd and Jaime are especially welcome if I've broken
> >something that used to work on their patch :-)
>
> I see that the current patch doesn't support subqueries in the WHERE-clause
> anymore.
> You can find one example in the attached SQL-script. Is there a reason
> why you dropped this?
Hum, turns out that you had this code in the patch:
/*
if ( query->hasSubLinks == true )
{
elog( WARNING, "Subqueries violates SQL92 view update rules!" );
return false;
}
*/
I figured I'd remove the comments, and then forgot ... If you remove
the piece of code from the viewUpdate.c file, your sample script works.
--
Alvaro Herrera http://www.CommandPrompt.com/
The PostgreSQL Company - Command Prompt, Inc.
--On Montag, August 21, 2006 02:07:41 -0400 Alvaro Herrera
<alvherre@commandprompt.com> wrote:
> So, I'll appreciate if somebody else takes the responsability to fix the
> remaining issues. I've put a lot of XXX's and some FIXME's. Some
> functions are in need of some comments as well.
While working on Alvaro's suggestions to fix the code i got the opinion
that we need to reject any attempts to name a user defined rule
as
"_INSERT"
"_NOTHING_INSERT"
"_DELETE"
"_NOTHING_DELETE"
"_UPDATE"
"_NOTHING_UPDATE"
because this confuses the code when replacing an existing implicit
rule with its own user defined one:
bernd@[local]:bernd #= create or replace view v_second as select id, name,
usr from second where usr =
current_user with check option;
NOTICE: CREATE VIEW will create implicit INSERT/UPDATE/DELETE rules
CREATE VIEW
bernd@[local]:bernd #= CREATE OR REPLACE RULE "_INSERT" AS ON INSERT TO
v_second DO INSTEAD NOTHING;
ERROR: tuple already updated by self
This is because the code tries to drop the existing implicit insert rule
from pg_rewrite
and then to replace it with the new one (note the "_INSERT" caption of the
rule, any
other labeled rule works as expected).
We could avoid this by using a CommandCounterIncrement() (brute force
method),
but it seems to me that we should do the same here as with "_RETURN" rules
at the moment.
Any comments?
--
Thanks
Bernd
Bernd Helmle <mailings@oopsware.de> writes:
> While working on Alvaro's suggestions to fix the code i got the opinion
> that we need to reject any attempts to name a user defined rule
> as
> "_INSERT"
> "_NOTHING_INSERT"
> "_DELETE"
> "_NOTHING_DELETE"
> "_UPDATE"
> "_NOTHING_UPDATE"
If the code is dependent on recognizing names to know what it's doing,
then I'd say you have a fundamentally broken approach. Consider adding
a flag column to pg_rewrite to distinguish these rules, instead.
regards, tom lane
On 8/24/06, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Bernd Helmle <mailings@oopsware.de> writes:
> > While working on Alvaro's suggestions to fix the code i got the opinion
> > that we need to reject any attempts to name a user defined rule
> > as
>
> > "_INSERT"
> > "_NOTHING_INSERT"
> > "_DELETE"
> > "_NOTHING_DELETE"
> > "_UPDATE"
> > "_NOTHING_UPDATE"
>
> If the code is dependent on recognizing names to know what it's doing,
> then I'd say you have a fundamentally broken approach. Consider adding
> a flag column to pg_rewrite to distinguish these rules, instead.
>
Actually the code delete implicit rules based on a field added to
pg_rewrite but that catalog has a unique index on ev_class, rulename:
"pg_rewrite_rel_rulename_index" UNIQUE, btree (ev_class, rulename)
i guess bernd's comment is about this index giving an error if we try
to insert the new rule with the same name on the same event...
--
regards,
Jaime Casanova
"Programming today is a race between software engineers striving to
build bigger and better idiot-proof programs and the universe trying
to produce bigger and better idiots.
So far, the universe is winning."
Richard Cook
--On Donnerstag, August 24, 2006 11:00:45 -0400 Tom Lane
<tgl@sss.pgh.pa.us> wrote:
> If the code is dependent on recognizing names to know what it's doing,
> then I'd say you have a fundamentally broken approach. Consider adding
> a flag column to pg_rewrite to distinguish these rules, instead.
This is the approach the code already follows (it uses an additional
ev_kind column which distinguishes rules between implicit rules with
no, local or cascaded check option and explicit ones).
Turns out that i was thinking too difficult when looking at the code which
drops implicit rules....sorry for the noise.
--
Thanks
Bernd
--On Donnerstag, August 24, 2006 11:02:43 -0500 Jaime Casanova
<systemguards@gmail.com> wrote:
> Actually the code delete implicit rules based on a field added to
> pg_rewrite but that catalog has a unique index on ev_class, rulename:
> "pg_rewrite_rel_rulename_index" UNIQUE, btree (ev_class, rulename)
>
> i guess bernd's comment is about this index giving an error if we try
> to insert the new rule with the same name on the same event...
No, this wasn't the problem, since we are going to drop any implicit
rule that collides with an user defined one (however, this approach is
discussable, but nobody has put his comments on this yet and i think this
is important for backwards compatibility). I don't think we need ev_kind in
the
index at all, in my opinion implicit and user defined rules of the same
event
shouldn't live together (_RETURN rules are marked as implicit ones now, too)
--
Thanks
Bernd
--On Montag, August 21, 2006 02:07:41 -0400 Alvaro Herrera
<alvherre@commandprompt.com> wrote:
> Hi,
>
> This is the patch for updatable views I've been able to come up with. A
> nasty bug was just discovered in the upcoming Mammoth Replicator release
> so I'm not sure if I'm going to have time to work more on it soon.
>
> So, I'll appreciate if somebody else takes the responsability to fix the
> remaining issues. I've put a lot of XXX's and some FIXME's. Some
> functions are in need of some comments as well.
>
> The new files are src/backend/rewrite/viewUpdate.c and
> src/include/rewrite/viewUpdate.h. The third file, upd-views.sql, is
> intended to be a new regression test. Extra points if the table therein
> is completed correctly.
>
> I haven't tested the array stuff at all.
>
> Comments from Bernd and Jaime are especially welcome if I've broken
> something that used to work on their patch :-)
Here's the current reworked version of the patch. I've fixed some broken
code in the patch
and ifdef'ed the DEFAULT stuff out (Jaime is working on that), some
functions got
some more detailed comments and i've dropped some functions which aren't
used
anymore due to some redesign of the code.
If someone wants to look at the current updatable view patch, please look
at this current
version.
--
Thanks
Bernd
Attachment
----- Original Message -----
From: alvherre@commandprompt.com <Alvaro Herrera>
To: mailings@oopsware.de
Date: 25.08.2006 00:50:59
Subject: Re: [PATCHES] Updatable views
>
> Minor suggestion: change get_view_qualification_function to look the
> function by Oid rather than name. I wasn't sure it was actually a good
> idea to use a function that way, but if it's going to stay ...
>
> Another: remove create_nothing_rule, replace with call to
> create_rule_stmt.
>
> Another: change hasRule to return a bool instead of an Oid.
>
> Another: instead of a comment like this:
>
> /*
> * XXX It seems to me that these checks are not necessary; and further,
> * they are useless. This is because the view is just being created,
> * thus it cannot have any rules before the ones we are going to
> * create.
> *
> * XXX What about CREATE OR REPLACE VIEW ???
> */
>
> have a single paragraph explaining why the replace flag is needed.
>
Okay, i'll sent a reworked version asap, but can't get to it before monday.
I'm away from my machine this weekend and have only sporadic access
to my email.
Bernd
--On Donnerstag, August 24, 2006 22:25:46 +0200 Bernd Helmle
<mailings@oopsware.de> wrote:
> --On Montag, August 21, 2006 02:07:41 -0400 Alvaro Herrera
> <alvherre@commandprompt.com> wrote:
>
>
> If someone wants to look at the current updatable view patch, please look
> at this current
> version.
I did some more rework based on additional suggestions from Alvaro, so
please
find the current updatable view patch attached. I've rewritten functions to
use expression_tree_walker() and added additional comments. If you find
anything more to improve, feel free to drop your comments. Thanks to
Alvarro for its comments again.
--
Thanks
Bernd
Attachment
Bernd Helmle wrote: > --On Donnerstag, August 24, 2006 22:25:46 +0200 Bernd Helmle > <mailings@oopsware.de> wrote: > > >--On Montag, August 21, 2006 02:07:41 -0400 Alvaro Herrera > ><alvherre@commandprompt.com> wrote: > > > > > >If someone wants to look at the current updatable view patch, please look > >at this current > >version. > > I did some more rework based on additional suggestions from Alvaro, so > please > find the current updatable view patch attached. I've rewritten functions to > use expression_tree_walker() and added additional comments. If you find > anything more to improve, feel free to drop your comments. Thanks to > Alvarro for its comments again. Note that pg_rewrite.h does not match catalogs.sgml. -- Alvaro Herrera http://www.CommandPrompt.com/ The PostgreSQL Company - Command Prompt, Inc.
--On Dienstag, August 29, 2006 13:15:25 -0400 Alvaro Herrera
<alvherre@commandprompt.com> wrote:
> Note that pg_rewrite.h does not match catalogs.sgml.
Oops, fixed.
--
Thanks
Bernd
Attachment
Bernd Helmle <mailings@oopsware.de> writes:
> [ latest views patch ]
This is the first time I've actually looked at this patch, and I am
dismayed. viewUpdate.c looks like nothing so much as a large program
with a small program struggling to get out. What is all the stuff about
handling multiple base rels? SQL92, at least, does not say that a join
is updatable, and AFAICT this patch is rejecting that too ... though
it's hard to tell with the conditions for allowing the join to be
updatable scattered through a lot of different functions. And some of
the code seems to be expecting multiple implicit rules and other parts
not. I get the impression that a lot of this code is left over from a
more ambitious first draft and ought to be removed in the name of
readability/maintainability.
I'm unclear as to why you've got DO INSTEAD NOTHING rules in there ---
the spec says that a WITH CHECK OPTION violation results in an error,
not in nothing happening, so it doesn't seem to me that we should need
any NOTHING rules to implement the spec. It would probably help if
there were some header documentation that explained exactly how the
module intends to transform a SELECT to create the various action rules.
The pg_dump changes seem pretty odd too. Why wouldn't you just
ignore implicit rules during a dump, expecting the system to
regenerate them when the view is reloaded?
regards, tom lane
On Wed, Aug 30, 2006 at 12:01:25PM -0400, Tom Lane wrote: > Bernd Helmle <mailings@oopsware.de> writes: > > [ latest views patch ] > > This is the first time I've actually looked at this patch, and I am > dismayed. viewUpdate.c looks like nothing so much as a large program > with a small program struggling to get out. What is all the stuff about > handling multiple base rels? SQL92, at least, does not say that a join > is updatable, and AFAICT this patch is rejecting that too ... though > it's hard to tell with the conditions for allowing the join to be > updatable scattered through a lot of different functions. And some of > the code seems to be expecting multiple implicit rules and other parts > not. I get the impression that a lot of this code is left over from a > more ambitious first draft and ought to be removed in the name of > readability/maintainability. If that code is on the right path to allowing things like updates to the many side of a join then it would be worth adding comments to that effect. Or maybe a comment referencing whatever version of the file the code was yanked out of. -- Jim C. Nasby, Sr. Engineering Consultant jnasby@pervasive.com Pervasive Software http://pervasive.com work: 512-231-6117 vcard: http://jim.nasby.net/pervasive.vcf cell: 512-569-9461
Am Mittwoch, 30. August 2006 18:01 schrieb Tom Lane: > This is the first time I've actually looked at this patch, and I am > dismayed. viewUpdate.c looks like nothing so much as a large program > with a small program struggling to get out. What is all the stuff about > handling multiple base rels? SQL92, at least, does not say that a join > is updatable, and AFAICT this patch is rejecting that too ... But later SQL versions allow some of that, so at least it shouldn't hurt to have some parts of the code to be more general in preparation of that. > I'm unclear as to why you've got DO INSTEAD NOTHING rules in there --- You need to have one unconditional rule if you have a bunch of conditional ones. The system does not see through the fact that the conditional ones cover all cases. > The pg_dump changes seem pretty odd too. Why wouldn't you just > ignore implicit rules during a dump, expecting the system to > regenerate them when the view is reloaded? Right. -- Peter Eisentraut http://developer.postgresql.org/~petere/
--On Mittwoch, August 30, 2006 12:01:25 -0400 Tom Lane <tgl@sss.pgh.pa.us>
wrote:
> Bernd Helmle <mailings@oopsware.de> writes:
>> [ latest views patch ]
>
> This is the first time I've actually looked at this patch, and I am
> dismayed. viewUpdate.c looks like nothing so much as a large program
> with a small program struggling to get out. What is all the stuff about
> handling multiple base rels? SQL92, at least, does not say that a join
> is updatable, and AFAICT this patch is rejecting that too ... though
> it's hard to tell with the conditions for allowing the join to be
> updatable scattered through a lot of different functions. And some of
> the code seems to be expecting multiple implicit rules and other parts
> not. I get the impression that a lot of this code is left over from a
> more ambitious first draft and ought to be removed in the name of
> readability/maintainability.
>
I not sure what parts of the code you are refering to exactly, but I admit
that
there are code parts that could deal with multiple base relations and
rules.
get_base_base_relation() is an example, it is used to create lookup tables
for reversed columns so we could break them down to the correct position in
their base tables. Restricting that to only one base relation wouldn't make
any
difference. Furthermore, SQL99 allows at least updatable views with joined
relations which preserve their keys in the view definition. So i don't
think it's that
bad to leave parts of the code that way for future improvements.
> I'm unclear as to why you've got DO INSTEAD NOTHING rules in there ---
> the spec says that a WITH CHECK OPTION violation results in an error,
> not in nothing happening, so it doesn't seem to me that we should need
> any NOTHING rules to implement the spec. It would probably help if
Well, instead of something like
"ERROR: cannot insert into a view
HINT: You need an unconditional ON INSERT DO INSTEAD rule."
you will get
"ERROR: view update commands violates rule condition"
with the correct error code set, because the view update check function is
fired before.
The first one isn't very useful for someone who simply wants to insert data
into the
view which isn't allowed to get in. You never get the view update check
function fired
without the DO INSTEAD rule applied to a view created with a check option.
> there were some header documentation that explained exactly how the
> module intends to transform a SELECT to create the various action rules.
>
I agree with you, maybe it's a good to add a README to src/backend/rewrite?
> The pg_dump changes seem pretty odd too. Why wouldn't you just
> ignore implicit rules during a dump, expecting the system to
> regenerate them when the view is reloaded?
Uhm, you're right. It's easier to exclude them in the SELECT query directly
instead
of selecting them, iterating over and filter them out. I'll fix that.
(Looks like this is a
"cannot see the wood for the trees"-mistake....)
--
Thanks
Bernd
Peter Eisentraut <peter_e@gmx.net> writes:
> Am Mittwoch, 30. August 2006 18:01 schrieb Tom Lane:
>> This is the first time I've actually looked at this patch, and I am
>> dismayed. viewUpdate.c looks like nothing so much as a large program
>> with a small program struggling to get out.
> But later SQL versions allow some of that, so at least it shouldn't hurt to
> have some parts of the code to be more general in preparation of that.
If it bloats the code to unreadability, it's bad.
>> I'm unclear as to why you've got DO INSTEAD NOTHING rules in there ---
> You need to have one unconditional rule if you have a bunch of conditional
> ones. The system does not see through the fact that the conditional ones
> cover all cases.
AFAICS, for the cases we are able to implement within the existing rule
mechanism, there should be exactly one unconditional rule. If you
propose more, then you are going to have insurmountable problems with
the usual sorts of multiple-evaluation risks.
The proposed WITH CHECK OPTION implementation is unworkable for exactly
this reason --- it will give the wrong answers in the presence of
volatile functions such as nextval(). I believe that we cannot
implement WITH CHECK OPTION as a rule. It's a constraint, instead,
and will have to be checked the way the executor presently checks
constraints, ie after forming the finished new tuple(s).
(Someday we're going to have to look into redesigning the rule system
so that it can cope better with the kinds of situations that give rise
to multiple-evaluation problems. But today is not that day.)
It's possible that if we strip the patch down to SQL92-equivalent
functionality (no multiple base rels) without WITH CHECK OPTION,
we would have something that would work reliably atop the existing
rule mechanism. It's getting mighty late in the 8.2 cycle to be
doing major rework though.
regards, tom lane
Am Donnerstag, 31. August 2006 15:55 schrieb Tom Lane: > >> I'm unclear as to why you've got DO INSTEAD NOTHING rules in there --- > > > > You need to have one unconditional rule if you have a bunch of > > conditional ones. The system does not see through the fact that the > > conditional ones cover all cases. > > AFAICS, for the cases we are able to implement within the existing rule > mechanism, there should be exactly one unconditional rule. If you > propose more, then you are going to have insurmountable problems with > the usual sorts of multiple-evaluation risks. I'm not sure what you are saying here ... The implementation creates, for each of the three actions INSERT, UPDATE, DELETE, one conditional rule that redirects the action from the view into the unterlying table, conditional on the view condition being fulfilled. The unconditional DO INSTEAD NOTHING rule then catches the cases where the view condition is not fulfilled. So there is, for each action, exactly one conditional and one unconditional rule. Which is consistent with what you said above, so I don't see the problem. > The proposed WITH CHECK OPTION implementation is unworkable for exactly > this reason --- it will give the wrong answers in the presence of > volatile functions such as nextval(). I'm not sure why anyone would want to define a view condition containing a volatile function. At least it wouldn't put a major dent into this feature if such views were decreed not updatable. -- Peter Eisentraut http://developer.postgresql.org/~petere/
Peter Eisentraut <peter_e@gmx.net> writes:
> Am Donnerstag, 31. August 2006 15:55 schrieb Tom Lane:
>> The proposed WITH CHECK OPTION implementation is unworkable for exactly
>> this reason --- it will give the wrong answers in the presence of
>> volatile functions such as nextval().
> I'm not sure why anyone would want to define a view condition containing a
> volatile function. At least it wouldn't put a major dent into this feature
> if such views were decreed not updatable.
The problem is not with the view condition. Consider
CREATE TABLE data (id serial primary key, ...);
CREATE VIEW only_new_data AS SELECT * FROM data WHERE id > 12345
WITH CHECK OPTION;
INSERT INTO only_new_data VALUES(nextval('data_id_seq'), ...);
The proposed implementation will execute nextval twice (bad), and will
apply the WITH CHECK OPTION test to the value that isn't the one stored
(much worse). It doesn't help if the id is defaulted.
regards, tom lane
Where are we on this feature? --------------------------------------------------------------------------- Bernd Helmle wrote: > --On Mittwoch, August 30, 2006 12:01:25 -0400 Tom Lane <tgl@sss.pgh.pa.us> > wrote: > > > Bernd Helmle <mailings@oopsware.de> writes: > >> [ latest views patch ] > > > > This is the first time I've actually looked at this patch, and I am > > dismayed. viewUpdate.c looks like nothing so much as a large program > > with a small program struggling to get out. What is all the stuff about > > handling multiple base rels? SQL92, at least, does not say that a join > > is updatable, and AFAICT this patch is rejecting that too ... though > > it's hard to tell with the conditions for allowing the join to be > > updatable scattered through a lot of different functions. And some of > > the code seems to be expecting multiple implicit rules and other parts > > not. I get the impression that a lot of this code is left over from a > > more ambitious first draft and ought to be removed in the name of > > readability/maintainability. > > > > I not sure what parts of the code you are refering to exactly, but I admit > that > there are code parts that could deal with multiple base relations and > rules. > get_base_base_relation() is an example, it is used to create lookup tables > for reversed columns so we could break them down to the correct position in > their base tables. Restricting that to only one base relation wouldn't make > any > difference. Furthermore, SQL99 allows at least updatable views with joined > relations which preserve their keys in the view definition. So i don't > think it's that > bad to leave parts of the code that way for future improvements. > > > I'm unclear as to why you've got DO INSTEAD NOTHING rules in there --- > > the spec says that a WITH CHECK OPTION violation results in an error, > > not in nothing happening, so it doesn't seem to me that we should need > > any NOTHING rules to implement the spec. It would probably help if > > Well, instead of something like > > "ERROR: cannot insert into a view > HINT: You need an unconditional ON INSERT DO INSTEAD rule." > > you will get > > "ERROR: view update commands violates rule condition" > > with the correct error code set, because the view update check function is > fired before. > The first one isn't very useful for someone who simply wants to insert data > into the > view which isn't allowed to get in. You never get the view update check > function fired > without the DO INSTEAD rule applied to a view created with a check option. > > > there were some header documentation that explained exactly how the > > module intends to transform a SELECT to create the various action rules. > > > > I agree with you, maybe it's a good to add a README to src/backend/rewrite? > > > The pg_dump changes seem pretty odd too. Why wouldn't you just > > ignore implicit rules during a dump, expecting the system to > > regenerate them when the view is reloaded? > > Uhm, you're right. It's easier to exclude them in the SELECT query directly > instead > of selecting them, iterating over and filter them out. I'll fix that. > (Looks like this is a > "cannot see the wood for the trees"-mistake....) > > > -- > Thanks > > Bernd > > ---------------------------(end of broadcast)--------------------------- > TIP 1: if posting/reading through Usenet, please send an appropriate > subscribe-nomail command to majordomo@postgresql.org so that your > message can get through to the mailing list cleanly -- Bruce Momjian <bruce@momjian.us> http://momjian.us EnterpriseDB http://www.enterprisedb.com + If your life is a hard drive, Christ can be your backup. +
On Thu, 2007-02-08 at 18:21 -0500, Bruce Momjian wrote: > Where are we on this feature? Any update, Bernd? -- Simon Riggs 2ndQuadrant http://www.2ndQuadrant.com
--On Mittwoch, Mai 07, 2008 20:38:59 +0100 Simon Riggs <simon@2ndquadrant.com> wrote: >> Where are we on this feature? > > Any update, Bernd? I've merged the patch into current -HEAD and updated some parts. My current *working* state can be reviewed at <http://git.postgresql.org/?p=~psoo/postgresql.git;a=shortlog;h=updatable_views> I'm still not sure how to implement a reliable CHECK OPTION, but short on time i haven't done a very deep investigation yet. Next idea was to look at the updatable cursor stuff, maybe something there can be reused. -- Thanks Bernd
On Thu, 2008-05-08 at 13:48 +0200, Bernd Helmle wrote: > --On Mittwoch, Mai 07, 2008 20:38:59 +0100 Simon Riggs > <simon@2ndquadrant.com> wrote: > > >> Where are we on this feature? > > > > Any update, Bernd? > > I've merged the patch into current -HEAD and updated some parts. My current > *working* state can be reviewed at > > <http://git.postgresql.org/?p=~psoo/postgresql.git;a=shortlog;h=updatable_views> > > I'm still not sure how to implement a reliable CHECK OPTION, but short on > time i haven't done a very deep investigation yet. Next idea was to look at > the updatable cursor stuff, maybe something there can be reused. Your earlier patch seemed to add two rules if the view had a with check option? One with a pass through and another one with a do-nothing and a where clause. As I understand it CREATE VIEW x AS SELECT * FROM foo WHERE where-clause WITH CHECK OPTION should generate an INSERT rule like this CREATE RULE somename AS ON INSERT TO x WHERE where-clause DO INSERT ... which seems straightforward, no? The SQLStandard default is CASCADED and it seems easier not to worry too much about the LOCAL option until we have the basics working. I'm not even sure that we *want* the LOCAL option anyway having read what it means, plus it isn't supported by many other DBMS. Do you store anything in the catalog to mark the view as updatable or not? I couldn't see that but it seemed easier than trying to resolve all of the updatability characteristics at run-time. I may be able to help some with the patch, if you'd like? -- Simon Riggs 2ndQuadrant http://www.2ndQuadrant.com
Am Donnerstag, 8. Mai 2008 schrieb Simon Riggs: > CREATE RULE somename AS ON INSERT TO x WHERE where-clause DO INSERT ... > > which seems straightforward, no? Double evaluation is the key word. The conclusion was more or less that you can't implement check constraints using the rules system. You need to check them in the executor.
--On Donnerstag, Mai 08, 2008 13:28:14 +0100 Simon Riggs <simon@2ndquadrant.com> wrote: > On Thu, 2008-05-08 at 13:48 +0200, Bernd Helmle wrote: >> --On Mittwoch, Mai 07, 2008 20:38:59 +0100 Simon Riggs >> <simon@2ndquadrant.com> wrote: >> >> >> Where are we on this feature? >> > >> > Any update, Bernd? >> >> I've merged the patch into current -HEAD and updated some parts. My >> current *working* state can be reviewed at >> >> <http://git.postgresql.org/?p=~psoo/postgresql.git;a=shortlog;h=updatabl >> e_views> >> >> I'm still not sure how to implement a reliable CHECK OPTION, but short >> on time i haven't done a very deep investigation yet. Next idea was to >> look at the updatable cursor stuff, maybe something there can be reused. > > Your earlier patch seemed to add two rules if the view had a with check > option? One with a pass through and another one with a do-nothing and a > where clause. > > As I understand it > > CREATE VIEW x AS SELECT * FROM foo WHERE where-clause WITH CHECK OPTION > > should generate an INSERT rule like this > > CREATE RULE somename AS ON INSERT TO x WHERE where-clause DO INSERT ... > This was indeed the implementation i've proposed. We have rejected this idea then because it doesn't work with volatile functions reliable due to double evaluation: <http://archives.postgresql.org/pgsql-patches/2006-08/msg00483.php> Tom's example even demonstrates a serious constraint in rule based updates, since you get side effects in such conditions you won't expect, even without a CHECK OPTION. > which seems straightforward, no? > > The SQLStandard default is CASCADED and it seems easier not to worry too > much about the LOCAL option until we have the basics working. I'm not > even sure that we *want* the LOCAL option anyway having read what it > means, plus it isn't supported by many other DBMS. > > Do you store anything in the catalog to mark the view as updatable or > not? I couldn't see that but it seemed easier than trying to resolve all > of the updatability characteristics at run-time. I'm not sure want you mean, but pg_rewrite.ev_kind stores the nature of the rule. Updatability is determined by the checkTree() function internally. It's easy to query pg_rewrite to examine wether a view is updatable or not. > > I may be able to help some with the patch, if you'd like? > You're welcome ;) > -- > Simon Riggs > 2ndQuadrant http://www.2ndQuadrant.com -- Thanks Bernd
On Thu, 2008-05-08 at 14:56 +0200, Peter Eisentraut wrote: > Am Donnerstag, 8. Mai 2008 schrieb Simon Riggs: > > CREATE RULE somename AS ON INSERT TO x WHERE where-clause DO INSERT ... > > > > which seems straightforward, no? > > Double evaluation is the key word. The conclusion was more or less that you > can't implement check constraints using the rules system. You need to check > them in the executor. That makes sense. I can't see how we would make LOCAL CHECK CONSTRAINTs work with rules anyhow. So that means WITH CHECK CONSTRAINT is going to end up executed in a similar place to constraint evaluation on underlying tables. That leaves me in a difficult position with MERGE though. MERGE does something similar with conditional-WHEN clause evaluation, plus transformation of the sub-statements is only sensible when we have updatable views. :-( -- Simon Riggs 2ndQuadrant http://www.2ndQuadrant.com
--On Donnerstag, Mai 08, 2008 14:42:50 +0100 Simon Riggs
<simon@2ndquadrant.com> wrote:
> That makes sense. I can't see how we would make LOCAL CHECK CONSTRAINTs
> work with rules anyhow.
One of the idea's that came up through the discussion was to make the
rewriter responsible for collecting check constraints such as the local
check condition. They would be pushed down to the executor then where the
correct constraints would be applied. However, i'm currently not in the
position to say if this is doable right now.
The original updatable views patch tracked the state of required and
applied rule conditions during rewrite. This way it applied only the rule
conditions of the specified view in cascading updates.
--
Thanks
Bernd
On Thu, 2008-05-08 at 17:20 +0200, Bernd Helmle wrote: > --On Donnerstag, Mai 08, 2008 14:42:50 +0100 Simon Riggs > <simon@2ndquadrant.com> wrote: > > > That makes sense. I can't see how we would make LOCAL CHECK CONSTRAINTs > > work with rules anyhow. > > One of the idea's that came up through the discussion was to make the > rewriter responsible for collecting check constraints such as the local > check condition. They would be pushed down to the executor then where the > correct constraints would be applied. However, i'm currently not in the > position to say if this is doable right now. That's what I was thinking too. > The original updatable views patch tracked the state of required and > applied rule conditions during rewrite. This way it applied only the rule > conditions of the specified view in cascading updates. Yes, seems like the only way we'll get LOCAL CHECK CONSTRAINTS to work. Are you planning to work on this? -- Simon Riggs 2ndQuadrant http://www.2ndQuadrant.com
--On Donnerstag, Mai 08, 2008 16:34:39 +0100 Simon Riggs <simon@2ndquadrant.com> wrote: > Are you planning to work on this? Yes, i do. But i have to finish other things first until i can get back full attention to it, hopefully very soon.
On Thu, 2008-05-08 at 21:37 +0200, Bernd Helmle wrote: > --On Donnerstag, Mai 08, 2008 16:34:39 +0100 Simon Riggs > <simon@2ndquadrant.com> wrote: > > > Are you planning to work on this? > > > Yes, i do. But i have to finish other things first until i can get back > full attention to it, hopefully very soon. OK, cool. It looks to me that the way MERGE processes WHEN clauses is almost identical to the way updatable views process WITH CHECK OPTION. I'll assist you with at least that part of it, if I can. In the meantime, I'll assume its there and get on with the rest of MERGE. -- Simon Riggs 2ndQuadrant http://www.2ndQuadrant.com