Thread: Re: [COMMITTERS] pgsql: Automatic view update rules Bernd Helmle

petere@postgresql.org (Peter Eisentraut) writes:
> Automatic view update rules

This patch is still a few bricks shy of a load ...  within a few moments
of starting to look at it I'd noticed two different failure conditions

regression=# \d box_tbl Table "public.box_tbl"Column | Type | Modifiers 
--------+------+-----------f1     | box  | 

regression=# create view v1 as select * from box_tbl;
ERROR:  could not identify an equality operator for type box
regression=# create view v1 as select box_tbl from box_tbl;
server closed the connection unexpectedly       This probably means the server terminated abnormally       before or
whileprocessing the request.
 
The connection to the server was lost. Attempting reset: Failed.

and I'm sure there are quite a few more.  These things are not that hard
to fix in themselves, but what disturbs me more is the basic nature of
the generated rules.

regression=# create view v1 as select * from int8_tbl where q1 > 1000;
NOTICE:  CREATE VIEW has created automatic view update rules
CREATE VIEW
regression=# \d v1     View "public.v1"Column |  Type  | Modifiers 
--------+--------+-----------q1     | bigint | q2     | bigint | 
View definition:SELECT int8_tbl.q1, int8_tbl.q2  FROM int8_tbl WHERE int8_tbl.q1 > 1000;
Rules:"_DELETE" AS   ON DELETE TO v1 DO INSTEAD  DELETE FROM int8_tbl WHERE (old.q1 IS NULL AND int8_tbl.q1 IS NULL OR
old.q1= int8_tbl.q1) AND (old.q2 IS NULL AND int8_tbl.q2 IS NULL OR old.q2 = int8_tbl.q2) RETURNING old.q1,
old.q2"_INSERT"AS   ON INSERT TO v1 DO INSTEAD  INSERT INTO int8_tbl (q1, q2)  VALUES (new.q1, new.q2) RETURNING
new.q1,new.q2"_UPDATE" AS   ON UPDATE TO v1 DO INSTEAD  UPDATE int8_tbl SET q1 = new.q1, q2 = new.q2 WHERE (old.q1 IS
NULLAND int8_tbl.q1 IS NULL OR old.q1 = int8_tbl.q1) AND (old.q2 IS NULL AND int8_tbl.q2 IS NULL OR old.q2 =
int8_tbl.q2)RETURNING new.q1, new.q2
 

This has got two big problems.  The first is the incredibly inefficient
nature of the resulting plans, e.g,

regression=# explain update v1 set q1 = q1 + 1000 where q1 = 42;
                                                                 QUERY PLAN
                                                                      
 

------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
----------------------------------Nested Loop  (cost=0.00..2.20 rows=1 width=22)  Join Filter: ((((public.int8_tbl.q1
ISNULL) AND (public.int8_tbl.q1 IS NULL)) OR (public.int8_tbl.q1 = public.int8_tbl.q1)) AND (((public.int8_tbl.q2 IS
NULL)AND (public.int8_tbl.q2 IS NULL)) OR (public.int8_tbl.q2 = public.int8_tbl.q2)))  ->  Seq Scan on int8_tbl
(cost=0.00..1.07rows=1 width=16)        Filter: ((q1 > 1000) AND (q1 = 42))  ->  Seq Scan on int8_tbl  (cost=0.00..1.05
rows=5width=22)
 
(5 rows)

If we ship this, we will be a laughingstock.  The other problem (which
is related to the first failure condition exhibited above) is the
assumption that the default btree equality operator for a data type is
"real" equality.  Even if it exists, that's a bad assumption --- it
falls down for float8 and numeric let alone any more-interesting
datatypes such as the geometric types.

It would probably be better if we insisted that the view's base be a
plain relation and used ctid equality in the update rules (which will in
turn require supporting TidScan as an inner join indexscan, but that's
doable).

In short, I don't feel that this was ready to be applied.  It's probably
fixable with a week or so's work, but do we want to be expending that
kind of effort on it at this stage of the release cycle?
        regards, tom lane

--On 23. Januar 2009 13:28:27 -0500 Tom Lane <tgl@sss.pgh.pa.us> wrote:

>
> In short, I don't feel that this was ready to be applied.  It's probably
> fixable with a week or so's work, but do we want to be expending that
> kind of effort on it at this stage of the release cycle?
>

Uh well, i'd be happier if such review comments would have been made 
earlier in the CommitFest.

If i understand you correctly we have the choice between

a) revert this patch, fix all remaining issues which will likely postpone 
this for 8.5
b) don't revert, but try to fix the issues currently existing in HEAD.

It seems you're unsure wether b) is an option at all, because the amount of
remaining work exceeds the time left for this release cycle?

To be honest: I'm disappointed. If it tooks only a few steps to identify 
those (obviously important) issues, i get the opinion that there's very few 
motivating interest in this functionality (And yes, i'm annoyed about 
myself to not consider those operator issues).
Bernd

Bernd Helmle wrote:

> If i understand you correctly we have the choice between
>
> a) revert this patch, fix all remaining issues which will likely postpone 
> this for 8.5
> b) don't revert, but try to fix the issues currently existing in HEAD.

c) revert and expect an updated patch to apply very soon before this
commitfest is finished

-- 
Alvaro Herrera                                http://www.CommandPrompt.com/
The PostgreSQL Company - Command Prompt, Inc.

Bernd Helmle <mailings@oopsware.de> writes:
> --On 23. Januar 2009 13:28:27 -0500 Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> In short, I don't feel that this was ready to be applied.

> Uh well, i'd be happier if such review comments would have been made 
> earlier in the CommitFest.

[ shrug... ]  I've been busting my butt since 1 November to try to
review everything.  Some things are going to get left to the end.
I have to admit having ranked this one lower because it was marked
WIP for a good part of the commitfest, and so I'd assumed it was not
really a serious candidate to get applied.

Anyway, it's here now, and what we have to figure out is whether it's
fixable on a time scale that's realistic for 8.4.  I would really rather
sidestep the whole btree-equality issue if possible, but that doesn't
seem possible without some amount of changes to the rule mechanism
itself.  The idea I was toying with when I posted earlier is that the
rules should look more like
on update to view do insteadupdate base_table set c1 = new.c1, etcwhere base_table.ctid = old.ctid

but of course that doesn't work as-is because views don't expose
old.ctid, and even if they did (which doesn't seem impossible) we'd need
some planner fixes in order to get a non-silly plan out of it, because
joins on ctid aren't implemented very well today.

Another gotcha is that read-committed updates wouldn't work properly.
If the row first identified by the view has been outdated by someone
else's update, we're supposed to try to apply the update to the newest
version of the row, if it still passes the update's WHERE clause.
This would fail a priori with the ctid-based approach since the new row
version is guaranteed not to have the same ctid.  Even in the current
equate-all-the-visible-fields approach it doesn't work if the someone
else updated any of the visible fields: the row would now fail one of
the added where conditions, which have got nothing to do with anything
that the user wrote, so it's not expected behavior.

I'm inclined to think that this is all pretty much insoluble within the
current rule mechanism.  The existing definition of rules makes it
basically impossible to do INSTEAD UPDATE or INSTEAD DELETE without
creating a self-join; if we don't get around that somehow we're never
going to be very satisfied with either the performance or the
corner-case semantics of this thing.  What we get now from a rewritten
view update is something that looks like
UPDATE base_table new SET ... FROM base_table oldWHERE view's-conditions-on-old AND user's-conditions-on-old    AND
exposed-fields-of-new-and-old-are-equal

and just replacing the last part of that with a ctid equality is only
nibbling at the margins of its suckiness.  What we really want is that
the rewritten query is just
UPDATE base_table SET ...WHERE view's-conditions AND user's-conditions

with no join at all.

Perhaps the right answer is to invent some new rule syntax to "redirect"
inserts/updates/deletes, say something like
on update to foo do instead redirect to bar

and then put some logic that's not so much different from what you've
got here into the rule engine itself ... or maybe better, just have the
rule engine automatically try to redirect if it's faced with having to
raise error for lack of a rule?  It seems to me that the rule engine
has probably got all the infrastructure needed to convert the query the
way we'd like, we just don't have a suitable API to tell it to do that.
        regards, tom lane

On Fri, 2009-01-23 at 17:32 -0500, Tom Lane wrote:
> Bernd Helmle <mailings@oopsware.de> writes:
> > --On 23. Januar 2009 13:28:27 -0500 Tom Lane <tgl@sss.pgh.pa.us> wrote:
> >> In short, I don't feel that this was ready to be applied.
> 
> > Uh well, i'd be happier if such review comments would have been made 
> > earlier in the CommitFest.
> 
> [ shrug... ]  I've been busting my butt since 1 November to try to
> review everything.  Some things are going to get left to the end.

I don't think anyone is suggesting differently and if they are I will be
happy to go all JD on them. I think the author is just (rightfully)
frustrated at the process in general. We lack certain resources. *shrug*

The good news is :) this release cycle has been much better than any
previous release cycle that I have been a part of.

Sincerely,

Joshua D. Drake

-- 
PostgreSQL - XMPP: jdrake@jabber.postgresql.org  Consulting, Development, Support, Training  503-667-4564 -
http://www.commandprompt.com/ The PostgreSQL Company, serving since 1997
 

Bernd,

> To be honest: I'm disappointed. If it tooks only a few steps to identify 
> those (obviously important) issues, i get the opinion that there's very 
> few motivating interest in this functionality (And yes, i'm annoyed 
> about myself to not consider those operator issues).

Well, that *is* the problem with getting your patch into the last 
commitfest, and why I tried to get everyone to submit earlier.  You only 
have to miss one difficult problem to miss the release.

If it makes you feel any better, I certainly didn't think of the 
operator issue, and neither did Robert.

--Josh

On Fri, Jan 23, 2009 at 5:52 PM, Josh Berkus <josh@agliodbs.com> wrote:
> Bernd,
>
> If it makes you feel any better, I certainly didn't think of the operator
> issue, and neither did Robert.
>

to be honest, i feel like that was commented in the last (or the last
before the last) release cycle well this patch originally appears.
but i have no time in this moment to confirm that

--
Atentamente,
Jaime Casanova
Soporte y capacitación de PostgreSQL
Asesoría y desarrollo de sistemas
Guayaquil - Ecuador
Cel. +59387171157

--On 23. Januar 2009 17:32:55 -0500 Tom Lane <tgl@sss.pgh.pa.us> wrote:

> Bernd Helmle <mailings@oopsware.de> writes:
>> --On 23. Januar 2009 13:28:27 -0500 Tom Lane <tgl@sss.pgh.pa.us> wrote:
>>> In short, I don't feel that this was ready to be applied.
>
>> Uh well, i'd be happier if such review comments would have been made
>> earlier in the CommitFest.
>
> [ shrug... ]  I've been busting my butt since 1 November to try to
> review everything.  Some things are going to get left to the end.
> I have to admit having ranked this one lower because it was marked
> WIP for a good part of the commitfest, and so I'd assumed it was not
> really a serious candidate to get applied.
>

Oh, please, don't get me wrong: i never intended to attack you personally. 
I can imagine how much of work you are faced with this release. I got the 
feeling that it's simply the wrong way chosen, a little bit frustrating, 
isn't it?

Apologize for that.

> Anyway, it's here now, and what we have to figure out is whether it's
> fixable on a time scale that's realistic for 8.4.  I would really rather
> sidestep the whole btree-equality issue if possible, but that doesn't
> seem possible without some amount of changes to the rule mechanism
> itself.  The idea I was toying with when I posted earlier is that the
> rules should look more like
>
>     on update to view do instead
>     update base_table set c1 = new.c1, etc
>     where base_table.ctid = old.ctid
>
> but of course that doesn't work as-is because views don't expose
> old.ctid, and even if they did (which doesn't seem impossible) we'd need
> some planner fixes in order to get a non-silly plan out of it, because
> joins on ctid aren't implemented very well today.
>
> Another gotcha is that read-committed updates wouldn't work properly.
> If the row first identified by the view has been outdated by someone
> else's update, we're supposed to try to apply the update to the newest
> version of the row, if it still passes the update's WHERE clause.
> This would fail a priori with the ctid-based approach since the new row
> version is guaranteed not to have the same ctid.  Even in the current
> equate-all-the-visible-fields approach it doesn't work if the someone
> else updated any of the visible fields: the row would now fail one of
> the added where conditions, which have got nothing to do with anything
> that the user wrote, so it's not expected behavior.
>

Yeah, that's exactly the same feeling i got when reading your last mail. 
I'm very uncomfortable now that we know the "real" gotchas with the whole 
rule approach. Normally you'll get some ideas when thinking about a 
solution, but instead i have to think "omg, is that really doable within 
the rewriter in any ways?" getting disappointed.

> What we get now from a rewritten
> view update is something that looks like
>
>     UPDATE base_table new SET ... FROM base_table old
>     WHERE view's-conditions-on-old AND user's-conditions-on-old
>         AND exposed-fields-of-new-and-old-are-equal
>
> and just replacing the last part of that with a ctid equality is only
> nibbling at the margins of its suckiness.  What we really want is that
> the rewritten query is just
>
>     UPDATE base_table SET ...
>     WHERE view's-conditions AND user's-conditions
>
> with no join at all.
>
> Perhaps the right answer is to invent some new rule syntax to "redirect"
> inserts/updates/deletes, say something like
>
>     on update to foo do instead redirect to bar
>

Hmm this would mean that the rewriter bypasses all the rule stuff itself 
when faced with a view update and completely replacing the original query? 
Looks kinda of it. Oracle has INSTEAD OF triggers which are going to do 
nearly the same thing, afaiks.
Bernd

On Fri, Jan 23, 2009 at 5:32 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>
> Perhaps the right answer is to invent some new rule syntax to "redirect"
> inserts/updates/deletes, say something like
>
>        on update to foo do instead redirect to bar
>
> and then put some logic that's not so much different from what you've
> got here into the rule engine itself ... or maybe better, just have the
> rule engine automatically try to redirect if it's faced with having to
> raise error for lack of a rule?  It seems to me that the rule engine
> has probably got all the infrastructure needed to convert the query the
> way we'd like, we just don't have a suitable API to tell it to do that.
>

and what about default values? if we redirect we will have to use the
table's default (something i like) and AFAIU we won't have the ability
to change it for the view at least not without manually create a new
DO INSTEAD rule (something i don't like)...

i'm missing something? or can we implement such "REDIRECT" with the
ability to respect view's own defaults?

--
Atentamente,
Jaime Casanova
Soporte y capacitación de PostgreSQL
Asesoría y desarrollo de sistemas
Guayaquil - Ecuador
Cel. +59387171157

--On 23. Januar 2009 18:02:55 -0500 Jaime Casanova 
<jcasanov@systemguards.com.ec> wrote:

> to be honest, i feel like that was commented in the last (or the last
> before the last) release cycle well this patch originally appears.

I know that i've changed something in the operator lookup code regarding 
some discussions last year, but i can't remember. Anyways, we have to fix 
it in some other way.

And of course, CTID looks like a mess (and i'm sure there are much more 
issues with them than we can imagine now, because it's in some direction 
theoretically much the same problem as multi action rules).
Bernd

--On 23. Januar 2009 18:07:38 -0500 Jaime Casanova 
<jcasanov@systemguards.com.ec> wrote:

> and what about default values? if we redirect we will have to use the
> table's default (something i like) and AFAIU we won't have the ability
> to change it for the view at least not without manually create a new
> DO INSTEAD rule (something i don't like)...
>
> i'm missing something? or can we implement such "REDIRECT" with the
> ability to respect view's own defaults?

It's too late for me to think technically about it, but you're right, 
something to keep this behavior would be nice. I don't know wether the 
standard has a notion about such behavior, too (have to look at it).

Note that a possible solution obviously has to allow the old behavior, so 
in the first place this behavior can be easily restored.

Bernd

Jaime Casanova <jcasanov@systemguards.com.ec> writes:
> On Fri, Jan 23, 2009 at 5:32 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> Perhaps the right answer is to invent some new rule syntax to "redirect"
>> inserts/updates/deletes, say something like
>> on update to foo do instead redirect to bar

> and what about default values?

I don't see the issue.  View defaults would get inserted the same way
they are now.


There is another thing that's bothering me, though, which is that the
present approach to dumping rules isn't adequate.  Consider the
following scenario:

1. You create a view that the system considers updatable, so it creates
some automatic rules.

2. You don't want those rules, so you delete them, leaving you with the
traditional behavior where attempted inserts etc on the view fail.

3. All is well until you dump and restore, whereupon you'll be swearing
at those ^$^@#! rules having come back.

I think that we probably want the rules to show up automatically during
an upgrade from an older version, but it does not follow that they
should come back after being intentionally removed from an 8.4
installation.  (This is *particularly* true if we are unable to squash
every last one of the semantic gotchas; but even if we can, it's not
impossible that someone might want the no-update behavior for some
views.)

We could imagine attaching a "no auto rules please" property to views
(hm, perhaps this is an application for reloptions for a view).
Or we could invent a new rule action type "DO INSTEAD ERROR",
so that you could get the failure behavior as the result of a rule
manually substituted for the automatic ones.  But right now there's
a hole in the definition.
        regards, tom lane

> Uh well, i'd be happier if such review comments would have been made earlier
> in the CommitFest.

Well, as one of original reviewers of this patch, I feel a little bad
that I didn't consider these issues - the rules looked messy to me,
but I didn't consider that the whole approach might be wrong.  But...
I have to admit I didn't look at this patch very hard.  When I first
reviewed it on November 11th, it didn't even pass regression tests,
and you didn't submit a new version until December 26th, by which time
I had long since moved onto other things.

In the future, I think we should have an expectation that resubmits
within the same CommitFest should happen within a week, and that if no
revision is forthcoming within two weeks the patch is declared dead
(and the submitter can add it to the next CommitFest when they
resubmit).  Don't think I'm picking on you, either: there was quite a
bit of it this CommitFest, and it's bad, because:

- reviewers are afraid to move on to new patches, because they don't
know when or if they'll suddenly be called upon to go re-review old
patches, and
- the commitfest takes forever, which is probably hard on the
committers as well as the reviewers, and
- when the FINAL commitfest takes this long, it creates an extremely
long window during which it's hard to get started on any new work for
8.5.

On the flip side, as I've said before, some of the big patches were
not reviewed until quite late.  I think next time we should focus on
assigning reviewers to the big patches first (maybe two reviewers each
just to make sure we get good coverage...) and then review the smaller
patches afterwards.  But that's a separate issue from how long the
submitter takes to respond to feedback once it's given.

...Robert

I wrote:
> ... It seems to me that the rule engine
> has probably got all the infrastructure needed to convert the query the
> way we'd like, we just don't have a suitable API to tell it to do that.

I have in mind a couple of quite different approaches to this, and wanted
to solicit some feedback about which direction to pursue.

The idea I'd originally had was something along the lines of

ON UPDATE DO INSTEAD SUBSTITUTE base_table [ (base_column_name, ...) ]

where the intended transformation is that you take the update command
on the view as-written, substitute base_table for the view name and
appropriate base_column_names for each view column name, and presto
you have your update command for the base table.  The list of column names
would be there to let you specify the correspondence between base columns
and view columns.  One thing this is lacking is anything corresponding
to the view's WHERE clause to ensure that the update is restricted to
rows that are visible through the view.  We could just have the rewriter
copy over the view's WHERE clause, or we could insist that the clause
be repeated in the rule, ie

ON UPDATE DO INSTEAD SUBSTITUTE base_table [ (base_column_name, ...) ][ WHERE ... ]

That would be pretty tedious to write or maintain by hand, but in simple
cases the automatic rewriter should do it for you.

(Note: I'm focusing on UPDATE here because that's the hardest case.
DELETE is easier because there's no new column values to compute,
and INSERT is easy because there's no need to worry about matching
to an existing view row.)

Plan B was to not have any of this syntax exposed at all, but just have
the rewriter try to do it automatically when no update rule exists for
a view.  I think the main argument in favor of exposing syntax would be if
the syntax allows you to do things above and beyond the cases that we're
willing to take care of automatically.  Some examples of that would be
ignoring attempted updates on derived columns of a view, or reversing
invertible functions in the view.  (A trivial example of that: if the
view exposes "base_col + 1", you could allow updates that subtract one
from the value the user tries to store.)  The above syntax doesn't work
very well for doing such things, though.

I came up with a Plan C, which is to keep mostly the current syntax for
update rules but invent some notation that says "apply the update to the
view's underlying row".  There's an obvious candidate for existing syntax
to abuse for this purpose: WHERE CURRENT OF.  So we'd write something like

ON UPDATE DO INSTEAD UPDATE base_table SET base_col_1 = new.derived_col_1, base_col_2 ... WHERE CURRENT OF VIEW;

and the rewriter would interpret this appropriately.  You'd end up with
essentially the same results as with the other syntax, but there is more
flexibility here to omit columns, store results computed from columns,
etc.

This is a bit ugly because of the potential conflict with regular
"WHERE CURRENT OF cursor", but I find it hard to see a use-case for that
in a rule, since cursors are so much shorter-lived than rules.  Anyway
you could avoid the conflict by not naming your cursor "view".  A bigger
objection is that the semantics would be just a little bit different from
regular WHERE CURRENT OF cursor, because our implementation of that is
effectively a ctid match; and as I explained before, that's not what we
want for an updatable view.

Does anyone find any of these examples particularly attractive or
horrific?  Got any better ideas?
        regards, tom lane

Tom Lane <tgl@sss.pgh.pa.us> writes:

> ON UPDATE DO INSTEAD SUBSTITUTE base_table [ (base_column_name, ...) ]
>     [ WHERE ... ]
>
> ON UPDATE DO INSTEAD
>   UPDATE base_table SET base_col_1 = new.derived_col_1, base_col_2 ...
>   WHERE CURRENT OF VIEW;

What would happen with these if the view is defined with "SELECT *" and I add
a new column or drop columns from the table? It seems like the former with the
optional list of columns would magically apply to the new columns which would
make it behave differently from the normal select rule. Or would you expand an
ommitted column list like we do with "select *"

In any case the fact that the latter allows you to extend things with computed
values seems pretty attractive. We could always allow shortcuts like "SET *
WHERE CURRENT OF VIEW" analogous to "SELECT *" for manually created views. We
could also allow the rhs of the expressions to be skipped so you could do

UPDATE base_table SET col1, col2, col, base_col = new.derived_col - 1WHERE CURRENT OF VIEW

This same machinery isn't present in the normal executor is it? I mean, if I
can update a view then ISTM I should be able to update a view written inline
in the query like:
UPDATE (select * from a where x=1) set y=2

just like I can with SELECTs. This does incidentally work in Oracle and is its
way of doing what we do with UPDATE...FROM. It's the only way AFAIK to get
merge join update plans out of it.

--  Gregory Stark EnterpriseDB          http://www.enterprisedb.com Get trained by Bruce Momjian - ask me about
EnterpriseDB'sPostgreSQL training!
 

--On Samstag, Januar 24, 2009 14:17:58 -0500 Tom Lane <tgl@sss.pgh.pa.us> 
wrote:

> ON UPDATE DO INSTEAD
>   UPDATE base_table SET base_col_1 = new.derived_col_1, base_col_2 ...
>   WHERE CURRENT OF VIEW;
>
> and the rewriter would interpret this appropriately.  You'd end up with
> essentially the same results as with the other syntax, but there is more
> flexibility here to omit columns, store results computed from columns,
> etc.

I like this idea more than Plan A or B, since it's much closer to the 
current rule syntax. What i'm missing is some notion about CHECK OPTION. We 
surely want to support that in way.

--  Thanks
                   Bernd

Gregory Stark <stark@enterprisedb.com> writes:
> Tom Lane <tgl@sss.pgh.pa.us> writes:
>> ON UPDATE DO INSTEAD SUBSTITUTE base_table [ (base_column_name, ...) ]
>> [ WHERE ... ]
>> 
>> ON UPDATE DO INSTEAD
>> UPDATE base_table SET base_col_1 = new.derived_col_1, base_col_2 ...
>> WHERE CURRENT OF VIEW;

> What would happen with these if the view is defined with "SELECT *" and I add
> a new column or drop columns from the table?

Nothing, just as happens now, because the * got expanded to a set column
list by the parser before the view ever got defined.

> This same machinery isn't present in the normal executor is it? I mean, if I
> can update a view then ISTM I should be able to update a view written inline
> in the query like:
>  UPDATE (select * from a where x=1) set y=2

That is not a view; the primary reason why not being that there are no
applicable rules.
        regards, tom lane

Bernd Helmle <mailings@oopsware.de> writes:
> What i'm missing is some notion about CHECK OPTION. We 
> surely want to support that in way.

Feel free to insist on that, if you want to make dead certain that
updatable views don't make it into 8.4 ;-)

My recollection of the discussion two years ago is that we concluded
that WITH CHECK OPTION is simply not implementable using a rule-based
infrastructure, because of the multiple-evaluation problem.  Perhaps
it could be done with some kind of extension to the constraint-checking
logic, but I freely admit I don't see how to do it in any detail.
That seems like something to tackle later on.
        regards, tom lane

On Fri, Jan 23, 2009 at 7:17 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>
> I think that we probably want the rules to show up automatically during
> an upgrade from an older version, but it does not follow that they
> should come back after being intentionally removed from an 8.4
> installation.
>
[...]
>
> We could imagine attaching a "no auto rules please" property to views
> (hm, perhaps this is an application for reloptions for a view).
>

+1 for reloptions (the other way i think is to invent new syntax and i
think the reloptions are exactly to avoid that)


--
Atentamente,
Jaime Casanova
Soporte y capacitación de PostgreSQL
Asesoría y desarrollo de sistemas
Guayaquil - Ecuador
Cel. +59387171157

On Sat, Jan 24, 2009 at 1:17 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> I think that we probably want the rules to show up automatically during
> an upgrade from an older version

I'm really not convinced by that. Is it required by the standard? It's
really far from being compliant with the principle of least surprise.
Personnally, I don't expect my views to become updatable.

There should be an easy way to make a view become updatable but making
all of them updatable automagically on upgrade seems weird.

Another question related to the choice of explicit rules for the
implementation: if we change the way these rules are generated in 8.5,
will we upgrade all the existing rules? What if the user modified one
of them on purpose?

-- 
Guillaume

--On Sonntag, Januar 25, 2009 09:41:14 +0100 Guillaume Smet 
<guillaume.smet@gmail.com> wrote:

>> I think that we probably want the rules to show up automatically during
>> an upgrade from an older version
>
> I'm really not convinced by that. Is it required by the standard? It's
> really far from being compliant with the principle of least surprise.
> Personnally, I don't expect my views to become updatable.
>
> There should be an easy way to make a view become updatable but making
> all of them updatable automagically on upgrade seems weird.

I didn't find such a notion in the standard. A view is automatically 
updatable if it meets the criteria of updatability). If you don't want your 
view writable, you have to GRANT the necessary ACLs.

I originally had the idea of a GUC which controls wether automatic rules 
will be generated or not. But I abonded this idea, since this has some kind 
of "parametrized SQL standard functionality".


--  Thanks
                   Bernd

On Sun, Jan 25, 2009 at 1:07 PM, Bernd Helmle <mailings@oopsware.de> wrote:
> I didn't find such a notion in the standard. A view is automatically
> updatable if it meets the criteria of updatability). If you don't want your
> view writable, you have to GRANT the necessary ACLs.

Perhaps I'm a bit old school on this one but I don't see the point of
creating a bunch of rules on every view I create, even if I don't
especially want them to be updatable (and I think it's a very common
use case - especially because we're used to it).
Yes, I can remove them but I don't see the point of going through
every view to remove the rules.

Especially, creating these rules on upgrade seems really weird as
there is no chance the application is using updatable views: they
didn't exist in prior versions and if the application is using it, the
view already has its own set of rules for that.

> I originally had the idea of a GUC which controls wether automatic rules
> will be generated or not. But I abonded this idea, since this has some kind
> of "parametrized SQL standard functionality".

I'm more for some syntactical sugar which allows to create view with
the "updatable" property and remove this property from the view.

I don't know if it's possible though and it's just MVHO on this subject.

-- 
Guillaume

Bernd Helmle <mailings@oopsware.de> writes:
> I originally had the idea of a GUC which controls wether automatic rules 
> will be generated or not. But I abonded this idea, since this has some kind 
> of "parametrized SQL standard functionality".

We have GUCs like that already, for exactly the same reason: backwards
compatibility with prior releases in which some feature didn't work as
per SQL standard.  I think the argument that "no existing application
is going to be expecting these auto rules to appear" is pretty strong.
Arguably, pg_dump from an older version should make sure that the auto
rules should NOT get created, else it is failing to preserve an older
view's behavior.

The main question in my mind is whether we should have a turn-off
feature that is global (GUC) or per-view (reloption).  One difficulty
with a reloption is that there's no way to set it on a view until after
you've done CREATE VIEW, whereupon it's too late --- the auto rules
are already there, and surely the reloption mechanism isn't going to
know how to make them go away.

This would all be a little easier to accomplish if the behavior were
made to be implicit in the rewriter (ie, rewrite instead of throwing a
"no rule" error), since then there is no persistent state that a GUC or
reloption would have to try to add or get rid of.  However, I do rather
like the idea that the auto rules are just a special case of some syntax
with wider usage than the auto rules themselves.  So it's a tradeoff.
        regards, tom lane

Tom Lane wrote:
> Bernd Helmle <mailings@oopsware.de> writes:
>   
>> I originally had the idea of a GUC which controls wether automatic rules 
>> will be generated or not. But I abonded this idea, since this has some kind 
>> of "parametrized SQL standard functionality".
>>     
>
> We have GUCs like that already, for exactly the same reason: backwards
> compatibility with prior releases in which some feature didn't work as
> per SQL standard.  I think the argument that "no existing application
> is going to be expecting these auto rules to appear" is pretty strong.
> Arguably, pg_dump from an older version should make sure that the auto
> rules should NOT get created, else it is failing to preserve an older
> view's behavior.
>   

+1

We certainly can't just throw old apps to the wolves in the name of 
standards compliance.

> The main question in my mind is whether we should have a turn-off
> feature that is global (GUC) or per-view (reloption).  One difficulty
> with a reloption is that there's no way to set it on a view until after
> you've done CREATE VIEW, whereupon it's too late --- the auto rules
> are already there, and surely the reloption mechanism isn't going to
> know how to make them go away.
>   

Maybe something like CREATE VIEW .... WITHOUT UPDATE;

I actually like the idea of being able to turn update on and off for a view.

cheers

andrew

--On 25. Januar 2009 12:16:56 -0500 Andrew Dunstan <andrew@dunslane.net> 
wrote:

> Maybe something like CREATE VIEW .... WITHOUT UPDATE;
>
> I actually like the idea of being able to turn update on and off for a
> view.
>

Or what about

CREATE [OR REPLACE] [UPDATABLE] VIEW ... ?

This looks closer to TEMP|TEMPORARY VIEW, which we already have.
Bernd

2009/1/25 Bernd Helmle <mailings@oopsware.de>:
>
>
> --On 25. Januar 2009 12:16:56 -0500 Andrew Dunstan <andrew@dunslane.net>
> wrote:
>
>> Maybe something like CREATE VIEW .... WITHOUT UPDATE;
>>
>> I actually like the idea of being able to turn update on and off for a
>> view.
>>
>
> Or what about
>
> CREATE [OR REPLACE] [UPDATABLE] VIEW ... ?
>
> This looks closer to TEMP|TEMPORARY VIEW, which we already have.
>
>        Bernd

+ 1
regards
Pavel Stehule
>
> --
> Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
> To make changes to your subscription:
> http://www.postgresql.org/mailpref/pgsql-hackers
>

Bernd Helmle <mailings@oopsware.de> writes:
> Or what about
> CREATE [OR REPLACE] [UPDATABLE] VIEW ... ?
> This looks closer to TEMP|TEMPORARY VIEW, which we already have.

But per spec, UPDATABLE should be the default (if not now, then
eventually).  Are you proposingCREATE [OR REPLACE] [[NOT] UPDATABLE] VIEW ...
?  Seems confusing.
        regards, tom lane

Tom Lane escribió:

> The main question in my mind is whether we should have a turn-off
> feature that is global (GUC) or per-view (reloption).  One difficulty
> with a reloption is that there's no way to set it on a view until after
> you've done CREATE VIEW, whereupon it's too late --- the auto rules
> are already there, and surely the reloption mechanism isn't going to
> know how to make them go away.

Hmm, is there no way to do CREATE VIEW ... WITH (reloption)?  Isn't it
just a matter of another case in opt_check_option?

-- 
Alvaro Herrera                                http://www.CommandPrompt.com/
The PostgreSQL Company - Command Prompt, Inc.

On Sun, Jan 25, 2009 at 11:47 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>
> This would all be a little easier to accomplish if the behavior were
> made to be implicit in the rewriter (ie, rewrite instead of throwing a
> "no rule" error), since then there is no persistent state that a GUC or
> reloption would have to try to add or get rid of.

why we don't follow this path from the beggining?
what are the pros and cons of this?

--
Atentamente,
Jaime Casanova
Soporte y capacitación de PostgreSQL
Asesoría y desarrollo de sistemas
Guayaquil - Ecuador
Cel. +59387171157

--On 25. Januar 2009 13:36:35 -0500 Tom Lane <tgl@sss.pgh.pa.us> wrote:

> But per spec, UPDATABLE should be the default (if not now, then
> eventually).  Are you proposing
>     CREATE [OR REPLACE] [[NOT] UPDATABLE] VIEW ...
> ?  Seems confusing.

Good point. We need a better phrasing to restore the old behavior, maybe

CREATE [OR REPLACE] [READ ONLY|UPDATABLE] VIEW ...?

I think this looks less confusing.
Bernd

> There is another thing that's bothering me, though, which is that the
> present approach to dumping rules isn't adequate.  Consider the
> following scenario:
>
> 1. You create a view that the system considers updatable, so
> it creates
> some automatic rules.
>
> 2. You don't want those rules, so you delete them, leaving
> you with the
> traditional behavior where attempted inserts etc on the view fail.

Is that why other db's only make views updateable, that are created
"WITH CHECK OPTION" ? Should we also follow that path ?

Andreas

On Mon, Jan 26, 2009 at 6:48 AM, Zeugswetter Andreas OSB sIT
<Andreas.Zeugswetter@s-itsolutions.at> wrote:
>
>
> Is that why other db's only make views updateable, that are created
> "WITH CHECK OPTION" ? Should we also follow that path ?
>

no, the standard says that if the query expression is updatable the
view is updatable



--
Atentamente,
Jaime Casanova
Soporte y capacitación de PostgreSQL
Asesoría y desarrollo de sistemas
Guayaquil - Ecuador
Cel. +59387171157

Tom Lane wrote:
> Bernd Helmle <mailings@oopsware.de> writes:
> > Or what about
> > CREATE [OR REPLACE] [UPDATABLE] VIEW ... ?
> > This looks closer to TEMP|TEMPORARY VIEW, which we already have.
> 
> But per spec, UPDATABLE should be the default (if not now, then
> eventually).  Are you proposing
>     CREATE [OR REPLACE] [[NOT] UPDATABLE] VIEW ...
> ?  Seems confusing.

UNUPDATABLE?  :-)

BTW, how do we handle cases where the query cannot be updatable, e.g.
aggregates?  Do we throw a warning?

--  Bruce Momjian  <bruce@momjian.us>        http://momjian.us EnterpriseDB
http://enterprisedb.com
 + If your life is a hard drive, Christ can be your backup. +

On Mon, Jan 26, 2009 at 5:18 PM, Bruce Momjian <bruce@momjian.us> wrote:
> Tom Lane wrote:
>> Bernd Helmle <mailings@oopsware.de> writes:
>> > Or what about
>> > CREATE [OR REPLACE] [UPDATABLE] VIEW ... ?
>> > This looks closer to TEMP|TEMPORARY VIEW, which we already have.
>>
>> But per spec, UPDATABLE should be the default (if not now, then
>> eventually).  Are you proposing
>>       CREATE [OR REPLACE] [[NOT] UPDATABLE] VIEW ...
>> ?  Seems confusing.
>
> UNUPDATABLE?  :-)
>
> BTW, how do we handle cases where the query cannot be updatable, e.g.
> aggregates?  Do we throw a warning?
>

yes. we detect that and send a warning saying that there not be any rules


--
Atentamente,
Jaime Casanova
Soporte y capacitación de PostgreSQL
Asesoría y desarrollo de sistemas
Guayaquil - Ecuador
Cel. +59387171157

Bruce Momjian wrote:
> Tom Lane wrote:
>> Bernd Helmle <mailings@oopsware.de> writes:
>>> Or what about
>>> CREATE [OR REPLACE] [UPDATABLE] VIEW ... ?
>>> This looks closer to TEMP|TEMPORARY VIEW, which we already have.
>> But per spec, UPDATABLE should be the default (if not now, then
>> eventually).  Are you proposing
>>     CREATE [OR REPLACE] [[NOT] UPDATABLE] VIEW ...
>> ?  Seems confusing.

I'd frankly look at WITH, which is where we've historically stuck 
non-SQL extensions.

> BTW, how do we handle cases where the query cannot be updatable, e.g.
> aggregates?  Do we throw a warning?
> 

Error if "updatable" is specified, warning if not.

--Josh

Jaime Casanova wrote:
> On Mon, Jan 26, 2009 at 5:18 PM, Bruce Momjian <bruce@momjian.us> wrote:
> > Tom Lane wrote:
> >> Bernd Helmle <mailings@oopsware.de> writes:
> >> > Or what about
> >> > CREATE [OR REPLACE] [UPDATABLE] VIEW ... ?
> >> > This looks closer to TEMP|TEMPORARY VIEW, which we already have.
> >>
> >> But per spec, UPDATABLE should be the default (if not now, then
> >> eventually).  Are you proposing
> >>       CREATE [OR REPLACE] [[NOT] UPDATABLE] VIEW ...
> >> ?  Seems confusing.
> >
> > UNUPDATABLE?  :-)
> >
> > BTW, how do we handle cases where the query cannot be updatable, e.g.
> > aggregates?  Do we throw a warning?
> >
> 
> yes. we detect that and send a warning saying that there not be any rules

OK, so we are going to need an option to suppress that warning, even
without the problems of upgrades and customization.

We already use READ ONLY in SET TRANSACTION and START TRANSACTION, so it
would be logical to use READ ONLY to control this.

--  Bruce Momjian  <bruce@momjian.us>        http://momjian.us EnterpriseDB
http://enterprisedb.com
 + If your life is a hard drive, Christ can be your backup. +

Bruce,

>> yes. we detect that and send a warning saying that there not be any rules
> 
> OK, so we are going to need an option to suppress that warning, even
> without the problems of upgrades and customization.

Per my response earlier, I think we really logically need an error if 
the user specifies UPDATABLE.

--Josh

On Mon, Jan 26, 2009 at 8:47 PM, Josh Berkus <josh@agliodbs.com> wrote:
> Bruce,
>
>>> yes. we detect that and send a warning saying that there not be any rules
>>
>> OK, so we are going to need an option to suppress that warning, even
>> without the problems of upgrades and customization.
>
> Per my response earlier, I think we really logically need an error if the
> user specifies UPDATABLE.
>

a view should be updatable by default if the query expression is
updatable... what we need is something to make a view READ ONLY even
if it should be updatable by spec...

having said that, i don't think that inventing new syntax is the way
to go... a reloption seems better (thinking a little more, it could be
a problem if the user changes the reloptions of an already created
view)

--
Atentamente,
Jaime Casanova
Soporte y capacitación de PostgreSQL
Asesoría y desarrollo de sistemas
Guayaquil - Ecuador
Cel. +59387171157

Jaime, Bernd,

> having said that, i don't think that inventing new syntax is the way
> to go... a reloption seems better (thinking a little more, it could be
> a problem if the user changes the reloptions of an already created
> view)

There's also the issue with backup/restore: we need some kind of syntax 
for restoring a read-only view which doesn't depend on command ordering.  So we need a ALTER VIEW SET READ ONLY or
similar.

Overall, I'm starting to think there's a lot of syntax and issues that 
need to be worked out, and I'm thinking it's too late in the cycle to do 
that for 8.4.

--Josh

--On Montag, Januar 26, 2009 20:03:41 -0800 Josh Berkus <josh@agliodbs.com> 
wrote:

> Jaime, Bernd,
>
>> having said that, i don't think that inventing new syntax is the way
>> to go... a reloption seems better (thinking a little more, it could be
>> a problem if the user changes the reloptions of an already created
>> view)
>
> There's also the issue with backup/restore: we need some kind of syntax
> for restoring a read-only view which doesn't depend on command ordering.
> So we need a ALTER VIEW SET READ ONLY or similar.
>

Hence my proposal with

CREATE [OR REPLACE] [READ ONLY|UPDATABLE] VIEW

This can easily be extended to ALTER VIEW SET [READ ONLY|UPDATABLE]. 
Besides other issues already mentioned, this looks more logical to me, 
since this is going to change the behavior of a view completely.


--  Thanks
                   Bernd

On Saturday 24 January 2009 02:17:13 Tom Lane wrote:
> 2. You don't want those rules, so you delete them, leaving you with the
> traditional behavior where attempted inserts etc on the view fail.

This was never meant to be supported.  If you don't want updates on the rules 
to succeed, don't grant privileges.

On Tuesday 27 January 2009 05:39:48 Jaime Casanova wrote:
> a view should be updatable by default if the query expression is
> updatable... what we need is something to make a view READ ONLY even
> if it should be updatable by spec...

A view is read-only if you don't grant any write permissions on it.

On Tuesday 27 January 2009 00:21:08 Jaime Casanova wrote:
> On Mon, Jan 26, 2009 at 5:18 PM, Bruce Momjian <bruce@momjian.us> wrote:
> > Tom Lane wrote:
> >> Bernd Helmle <mailings@oopsware.de> writes:
> >> > Or what about
> >> > CREATE [OR REPLACE] [UPDATABLE] VIEW ... ?
> >> > This looks closer to TEMP|TEMPORARY VIEW, which we already have.
> >>
> >> But per spec, UPDATABLE should be the default (if not now, then
> >> eventually).  Are you proposing
> >>       CREATE [OR REPLACE] [[NOT] UPDATABLE] VIEW ...
> >> ?  Seems confusing.
> >
> > UNUPDATABLE?  :-)
> >
> > BTW, how do we handle cases where the query cannot be updatable, e.g.
> > aggregates?  Do we throw a warning?
>
> yes. we detect that and send a warning saying that there not be any rules

No, you get a notice *if* the view is updatable.  You don't get anything if 
the view is not.

--On Dienstag, Januar 27, 2009 14:04:05 +0200 Peter Eisentraut 
<peter_e@gmx.net> wrote:

>> a view should be updatable by default if the query expression is
>> updatable... what we need is something to make a view READ ONLY even
>> if it should be updatable by spec...
>
> A view is read-only if you don't grant any write permissions on it.

What i'm seeing here is a very divergent understanding what a "read-only" 
view is:

old-school PostgreSQL-Users would expect a "read-only" view to have no 
"write action" installed. If we want to follow the standard closely, they 
need to be installed automatically, changing this behavior, hence the wish 
to have a syntax to restore the old behavior (e.g. for pg_dump). I'm unsure 
what the correct approach looks like, but it seems we need a compromise.

--  Thanks
                   Bernd

On Tue, Jan 27, 2009 at 8:49 AM, Bernd Helmle <mailings@oopsware.de> wrote:
> --On Dienstag, Januar 27, 2009 14:04:05 +0200 Peter Eisentraut
> <peter_e@gmx.net> wrote:
>
>>> a view should be updatable by default if the query expression is
>>> updatable... what we need is something to make a view READ ONLY even
>>> if it should be updatable by spec...
>>
>> A view is read-only if you don't grant any write permissions on it.
>
> What i'm seeing here is a very divergent understanding what a "read-only"
> view is:
>
> old-school PostgreSQL-Users would expect a "read-only" view to have no
> "write action" installed. If we want to follow the standard closely, they
> need to be installed automatically, changing this behavior, hence the wish
> to have a syntax to restore the old behavior (e.g. for pg_dump). I'm unsure
> what the correct approach looks like, but it seems we need a compromise.

Do we REALLY think there are people out there who are writing INSERT
or UPDATE actions on views on which they haven't installed rules and
counting on the fact that those operations fail for correctness?

Personally, I usually write my code so it inserts into something that
is, uh... insertable.

...Robert

Robert Haas wrote:
> Do we REALLY think there are people out there who are writing INSERT
> or UPDATE actions on views on which they haven't installed rules and
> counting on the fact that those operations fail for correctness?
>
> Personally, I usually write my code so it inserts into something that
> is, uh... insertable.
>
>
>   

Not everybody has control over what clients might try to do. This is a 
very legitimate concern, ISTM.

cheers

andrew

On Tue, Jan 27, 2009 at 10:14 AM, Andrew Dunstan <andrew@dunslane.net> wrote:
>> Do we REALLY think there are people out there who are writing INSERT
>> or UPDATE actions on views on which they haven't installed rules and
>> counting on the fact that those operations fail for correctness?
>>
>> Personally, I usually write my code so it inserts into something that
>> is, uh... insertable.
>>
> Not everybody has control over what clients might try to do. This is a very
> legitimate concern, ISTM.

Can you flesh out the scenario you're concerned about a bit more?

...Robert

Peter Eisentraut <peter_e@gmx.net> writes:
> On Saturday 24 January 2009 02:17:13 Tom Lane wrote:
>> 2. You don't want those rules, so you delete them, leaving you with the
>> traditional behavior where attempted inserts etc on the view fail.

> This was never meant to be supported.  If you don't want updates on the rules
> to succeed, don't grant privileges.

If we'd had the SQL-spec behavior from day one, it wouldn't be a
problem, but you can't just blow off the old behavior like that.
It's a potential security hole, since GRANT ALL on a view used to
be de facto the same as GRANT SELECT, if you hadn't bothered to
create any rules.
        regards, tom lane

Robert Haas wrote:
> On Tue, Jan 27, 2009 at 10:14 AM, Andrew Dunstan <andrew@dunslane.net> wrote:
>   
>>> Do we REALLY think there are people out there who are writing INSERT
>>> or UPDATE actions on views on which they haven't installed rules and
>>> counting on the fact that those operations fail for correctness?
>>>
>>> Personally, I usually write my code so it inserts into something that
>>> is, uh... insertable.
>>>
>>>       
>> Not everybody has control over what clients might try to do. This is a very
>> legitimate concern, ISTM.
>>     
>
> Can you flesh out the scenario you're concerned about a bit more?
>
>
>   

See Tom's response to Peter nearby.

cheers

andrew

On Tuesday 27 January 2009 17:19:28 Tom Lane wrote:
> If we'd had the SQL-spec behavior from day one, it wouldn't be a
> problem, but you can't just blow off the old behavior like that.
> It's a potential security hole, since GRANT ALL on a view used to
> be de facto the same as GRANT SELECT, if you hadn't bothered to
> create any rules.

That is a good point.  But the only clean solution would be to make views 
never updatable by default, and invent a nonstandard syntax to make them so, 
which seems very unattractive to me.  A GUC variable as a transition measure 
could work, though.

Peter Eisentraut <peter_e@gmx.net> writes:
> On Tuesday 27 January 2009 17:19:28 Tom Lane wrote:
>> It's a potential security hole, since GRANT ALL on a view used to
>> be de facto the same as GRANT SELECT, if you hadn't bothered to
>> create any rules.

> That is a good point.  But the only clean solution would be to make views 
> never updatable by default, and invent a nonstandard syntax to make them so, 
> which seems very unattractive to me.  A GUC variable as a transition measure 
> could work, though.

Yeah, I tend to prefer the GUC approach over nonstandard syntax too.
We'd need a GUC anyway to determine the default behavior if no
nonstandard clause appeared; so we might as well just do that and not
bother with the syntax options.
        regards, tom lane

On Tue, Jan 27, 2009 at 3:36 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Peter Eisentraut <peter_e@gmx.net> writes:
>> On Tuesday 27 January 2009 17:19:28 Tom Lane wrote:
>>> It's a potential security hole, since GRANT ALL on a view used to
>>> be de facto the same as GRANT SELECT, if you hadn't bothered to
>>> create any rules.
>
>> That is a good point.  But the only clean solution would be to make views
>> never updatable by default, and invent a nonstandard syntax to make them so,
>> which seems very unattractive to me.  A GUC variable as a transition measure
>> could work, though.
>
> Yeah, I tend to prefer the GUC approach over nonstandard syntax too.
> We'd need a GUC anyway to determine the default behavior if no
> nonstandard clause appeared; so we might as well just do that and not
> bother with the syntax options.
>

+1


--
Atentamente,
Jaime Casanova
Soporte y capacitación de PostgreSQL
Asesoría y desarrollo de sistemas
Guayaquil - Ecuador
Cel. +59387171157