WIP: Automatic view update rules

--On Donnerstag, Oktober 30, 2008 21:24:08 +0100 Bernd Helmle
<mailings@oopsware.de> 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

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

--On Dienstag, November 11, 2008 23:06:08 -0500 Robert Haas
<robertmhaas@gmail.com> 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

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 decibel@decibel.org
Give your computer some brain candy! www.distributed.net Team #1828

Decibel! <decibel@decibel.org> 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

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

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

--On Dienstag, November 25, 2008 23:43:02 -0500 Robert Haas
<robertmhaas@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'.

--
Thanks

Bernd

--On Mittwoch, November 26, 2008 10:54:01 +0100 Bernd Helmle
<mailings@oopsware.de> wrote:

--On Dienstag, November 25, 2008 23:43:02 -0500 Robert Haas
<robertmhaas@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

On Mon, Dec 22, 2008 at 8:53 AM, Bernd Helmle <mailings@oopsware.de> wrote:

--On Mittwoch, November 26, 2008 10:54:01 +0100 Bernd Helmle
<mailings@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 null primary key,
name varchar(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

On Mon, Dec 22, 2008 at 8:53 AM, Bernd Helmle <mailings@oopsware.de>
wrote:

--On Mittwoch, November 26, 2008 10:54:01 +0100 Bernd Helmle
<mailings@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 null primary key,
name varchar(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

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

On Sun, Dec 28, 2008 at 9:29 AM, Bernd Helmle <bernd@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

On Sun, Dec 28, 2008 at 9:29 AM, Bernd Helmle <bernd@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

--On Sonntag, Dezember 28, 2008 15:29:58 +0100 Bernd Helmle
<bernd@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

--On Freitag, Januar 09, 2009 13:20:57 +0100 Bernd Helmle
<mailings@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

--On Freitag, Januar 09, 2009 17:53:31 +0100 Bernd Helmle
<mailings@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

Bernd Helmle wrote:

--On Freitag, Januar 09, 2009 17:53:31 +0100 Bernd Helmle
<mailings@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

--On Montag, Januar 12, 2009 14:48:46 +0200 Peter Eisentraut
<peter_e@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