Updatable views

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

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

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

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

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