Thread: Re: [PATCHES] Updatable views
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. +