SEO

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

---------------------------(end of broadcast)---------------------------
TIP 4: Have you searched our list archives?

http://archives.postgresql.org

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

---------------------------(end of broadcast)---------------------------
TIP 2: Don't 'kill -9' the postmaster

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/

---------------------------(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

--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

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

---------------------------(end of broadcast)---------------------------
TIP 6: explain analyze is your friend

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/

---------------------------(end of broadcast)---------------------------
TIP 4: Have you searched our list archives?

http://archives.postgresql.org

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

---------------------------(end of broadcast)---------------------------
TIP 3: Have you checked our extensive FAQ?

http://www.postgresql.org/docs/faq

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. +

---------------------------(end of broadcast)---------------------------
TIP 3: Have you checked our extensive FAQ?

http://www.postgresql.org/docs/faq

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


--
Sent via pgsql-patches mailing list (pgsql-patches@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-patches

--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/p...log;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

--
Sent via pgsql-patches mailing list (pgsql-patches@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-patches