Re: WIP: Automatic view update rules

Please find attached my current patch for automatic view update rules.
This is a stripped down version of my former patch discussed before 8.3
without CHECK OPTION support and less invasive changes to the rewriter
itself.

I'm currently cleaning up the code with all occurences of multiple base
relations (in progress) hence supporting SQL92 at the moment, only

If we decide to go further with this approach i would like to work on the
CHECK OPTION implementation based on some ideas we had in the past (e.g.
rewriter/executor support).

Note that i'm still working on this (for example, RETURNING is missing
yet), As always, discussion welcome ;)

--
Thanks

Bernd

--On Donnerstag, Oktober 30, 2008 21:24:08 +0100 Bernd Helmle
<mailings(at)oopsware(dot)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(at)gmail(dot)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(at)decibel(dot)org
Give your computer some brain candy! www.distributed.net Team #1828

Decibel! <decibel(at)decibel(dot)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(at)gmail(dot)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(at)oopsware(dot)de> wrote:

> --On Dienstag, November 25, 2008 23:43:02 -0500 Robert Haas
> <robertmhaas(at)gmail(dot)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(at)oopsware(dot)de> wrote:
> --On Mittwoch, November 26, 2008 10:54:01 +0100 Bernd Helmle
> <mailings(at)oopsware(dot)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(at)oopsware(dot)de>
> wrote:
>> --On Mittwoch, November 26, 2008 10:54:01 +0100 Bernd Helmle
>> <mailings(at)oopsware(dot)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(at)oopsware(dot)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(at)oopsware(dot)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(at)oopsware(dot)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(at)oopsware(dot)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(at)oopsware(dot)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(at)oopsware(dot)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(at)gmx(dot)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

Here is my updated patch based on yours.

Outstanding issues, as far as I can see, are:

Critical:

* Updatability check must reject views where the select list references
the same column more than once.

* Various scenarios of REPLACE VIEW misbehave. I have marked these as
FIXME in the regression test. I think all this would behave better if
REPLACE VIEW dropped all automatic rules and reassembled them from
scratch for the new view. The infrastructure for this is already there,
so it should be a small change.

Important:

* Array support should be clarified. checkTree() appears to reject most
variants of array references, but other parts of the code try to handle
it. Should be cleaned up.

* It is not clear how automatic rules and manual DO ALSO rules should
interact. A manual DO ALSO rule will currently clear out an automatic
INSTEAD rule, which I find to be illogical.

Optional:

* The use of must_replace is create_update_rule() seems a bit useless.
You might as well just always pass replace = true.

* You may want to consider writing the rule qualifications

WHERE ((CASE WHEN (old.a IS NOT NULL) THEN (old.a = vutestv20.x) ELSE
(vutestv20.x IS NULL) END))

more like

WHERE ((old.a = vutestv20.x) OR (old IS NULL AND vutestv20.x IS NULL))

for better optimizability.

CASE will be quite bad for optimization, and then you might as well go
with IS DISTINCT FROM, which is just as bad but simpler.

Peter Eisentraut <peter_e(at)gmx(dot)net> writes:
> CASE will be quite bad for optimization, and then you might as well go
> with IS DISTINCT FROM, which is just as bad but simpler.

Definitely avoid CASE if you can. Although the planner currently knows
nothing about IS DISTINCT FROM, that's fixable in principle. We'll
probably never be able to optimize CASE conditions very much because of
their special evaluation rules.

regards, tom lane

--On Samstag, Januar 17, 2009 02:01:15 +0200 Peter Eisentraut
<peter_e(at)gmx(dot)net> wrote:

> * It is not clear how automatic rules and manual DO ALSO rules should
> interact. A manual DO ALSO rule will currently clear out an automatic
> INSTEAD rule, which I find to be illogical.

My intentional feeling was that it would be a bad idea to leave any
implicit rule when someone is going to create his own rule set on a view,
at least to avoid any confusion or side effects. Consider someone having
his own rules upgrading from an older version. He must have at least his
own DO INSTEAD Rule, it doesn't make any sense to have his own DO ALSO Rule
without an INSTEAD one. Thus, doing it this way will leave the view as
expected from the original setup.

*thinking more*...if we teach explicit DO ALSO rules *not* to clean out
implicit ones, we will have the following workflows:

a) View is updatable, has its own automatic DO INSTEAD rule: if someone is
restoring his old update rules, he will have at least his own DO INSTEAD
rule. This will drop any corresponding automatically created rule, adding
his own DO INSTEAD rule and any DO ALSO rule.

b) View is updatable, has its own automatic DO INSTEAD rule: The user is
able to create any additional DO ALSO rule.

I don't see any problems here, as long as the implicit DO INSTEAD rule gets
replaced.

Opinions?

--
Thanks

Bernd

Bernd Helmle wrote:
> --On Samstag, Januar 17, 2009 02:01:15 +0200 Peter Eisentraut
> <peter_e(at)gmx(dot)net> wrote:
>
>> * It is not clear how automatic rules and manual DO ALSO rules should
>> interact. A manual DO ALSO rule will currently clear out an automatic
>> INSTEAD rule, which I find to be illogical.
>
> My intentional feeling was that it would be a bad idea to leave any
> implicit rule when someone is going to create his own rule set on a
> view, at least to avoid any confusion or side effects. Consider someone
> having his own rules upgrading from an older version. He must have at
> least his own DO INSTEAD Rule, it doesn't make any sense to have his own
> DO ALSO Rule without an INSTEAD one. Thus, doing it this way will leave
> the view as expected from the original setup.
>
> *thinking more*...if we teach explicit DO ALSO rules *not* to clean out
> implicit ones, we will have the following workflows:
>
> a) View is updatable, has its own automatic DO INSTEAD rule: if someone
> is restoring his old update rules, he will have at least his own DO
> INSTEAD rule. This will drop any corresponding automatically created
> rule, adding his own DO INSTEAD rule and any DO ALSO rule.
>
> b) View is updatable, has its own automatic DO INSTEAD rule: The user is
> able to create any additional DO ALSO rule.
>
> I don't see any problems here, as long as the implicit DO INSTEAD rule
> gets replaced.

Someone who previously had a DO INSTEAD rule to effect updatable views
as well as a DO ALSO rule to create some side effect (e.g.,
auditing/logging), will after the upgrade perhaps want to rely on the
automatic view update rules but will still want to create his own DO
ALSO rule. In the current patch, the creation of the DO ALSO rule will
delete the automatic view update rules, thus breaking the entire scenario.

Considering that the updatable views feature deals only with DO INSTEAD
rules, we should leave DO ALSO rules completely alone.