Re: [HACKERS] Proof of concept: auto updatable views [Review of Patch]
On 8 December 2012 23:29, Tom Lane t...@sss.pgh.pa.us wrote: Dean Rasheed dean.a.rash...@gmail.com writes: Attached is a rebased patch using new OIDs. Applied after a fair amount of hacking. Awesome! Thank you very much. Regards, Dean -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Proof of concept: auto updatable views [Review of Patch]
On 8 December 2012 23:29, Tom Lane t...@sss.pgh.pa.us wrote: Dean Rasheed dean.a.rash...@gmail.com writes: Attached is a rebased patch using new OIDs. Applied after a fair amount of hacking. One observation: There doesn't appear to be any tab-completion for view names after DML statement keywords in psql. Might we want to add this? -- Thom
Re: [HACKERS] Proof of concept: auto updatable views [Review of Patch]
Thom Brown t...@linux.com writes: One observation: There doesn't appear to be any tab-completion for view names after DML statement keywords in psql. Might we want to add this? Well, there is, but it only knows about INSTEAD OF trigger cases. I'm tempted to suggest that Query_for_list_of_insertables and friends be simplified to just include all views. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Proof of concept: auto updatable views [Review of Patch]
On 9 December 2012 16:53, Tom Lane t...@sss.pgh.pa.us wrote: Thom Brown t...@linux.com writes: One observation: There doesn't appear to be any tab-completion for view names after DML statement keywords in psql. Might we want to add this? Well, there is, but it only knows about INSTEAD OF trigger cases. I'm tempted to suggest that Query_for_list_of_insertables and friends be simplified to just include all views. Yeah, that's probably OK for tab-completion. It's a shame though that pg_view_is_updatable() and pg_view_is_insertable() are not really useful for identifying potentially updatable views (e.g., consider an auto-updatable view on top of a trigger-updatable view). I'm left wondering if I misinterpreted the SQL standard's intentions when separating out the concepts of updatable and trigger updatable. It seems like it would have been more useful to have trigger updatable imply updatable. Regards, Dean -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Proof of concept: auto updatable views [Review of Patch]
Dean Rasheed dean.a.rash...@gmail.com writes: It's a shame though that pg_view_is_updatable() and pg_view_is_insertable() are not really useful for identifying potentially updatable views (e.g., consider an auto-updatable view on top of a trigger-updatable view). I'm left wondering if I misinterpreted the SQL standard's intentions when separating out the concepts of updatable and trigger updatable. It seems like it would have been more useful to have trigger updatable imply updatable. I wondered about that too, but concluded that they were separate after noticing that the standard frequently writes things like updatable or trigger updatable. They wouldn't need to write that if the latter implied the former. But in any case, those functions are expensive enough that I can't see running them against every view in the DB anytime somebody hits tab. I think just allowing tab-completion to include all views is probably the best compromise. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Proof of concept: auto updatable views [Review of Patch]
On 9 December 2012 22:00, Tom Lane t...@sss.pgh.pa.us wrote: Dean Rasheed dean.a.rash...@gmail.com writes: It's a shame though that pg_view_is_updatable() and pg_view_is_insertable() are not really useful for identifying potentially updatable views (e.g., consider an auto-updatable view on top of a trigger-updatable view). I'm left wondering if I misinterpreted the SQL standard's intentions when separating out the concepts of updatable and trigger updatable. It seems like it would have been more useful to have trigger updatable imply updatable. I wondered about that too, but concluded that they were separate after noticing that the standard frequently writes things like updatable or trigger updatable. They wouldn't need to write that if the latter implied the former. Yeah, that was my reasoning too. But in any case, those functions are expensive enough that I can't see running them against every view in the DB anytime somebody hits tab. I think just allowing tab-completion to include all views is probably the best compromise. Agreed. Regards, Dean -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Proof of concept: auto updatable views [Review of Patch]
On 9 December 2012 22:00, Tom Lane t...@sss.pgh.pa.us wrote: Dean Rasheed dean.a.rash...@gmail.com writes: It's a shame though that pg_view_is_updatable() and pg_view_is_insertable() are not really useful for identifying potentially updatable views (e.g., consider an auto-updatable view on top of a trigger-updatable view). I'm left wondering if I misinterpreted the SQL standard's intentions when separating out the concepts of updatable and trigger updatable. It seems like it would have been more useful to have trigger updatable imply updatable. I wondered about that too, but concluded that they were separate after noticing that the standard frequently writes things like updatable or trigger updatable. They wouldn't need to write that if the latter implied the former. But in any case, those functions are expensive enough that I can't see running them against every view in the DB anytime somebody hits tab. I think just allowing tab-completion to include all views is probably the best compromise. I'm surprised to see that updateable and trigger updateable states aren't recorded in the catalog somewhere. ISTM a useful thing to be able to know about a view and not something we should be calculating on the fly. That has nothing much to do with tab completion, it just seems like a generally useful thing. -- Simon Riggs http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Proof of concept: auto updatable views [Review of Patch]
Simon Riggs si...@2ndquadrant.com writes: On 9 December 2012 22:00, Tom Lane t...@sss.pgh.pa.us wrote: But in any case, those functions are expensive enough that I can't see running them against every view in the DB anytime somebody hits tab. I think just allowing tab-completion to include all views is probably the best compromise. I'm surprised to see that updateable and trigger updateable states aren't recorded in the catalog somewhere. ISTM a useful thing to be able to know about a view and not something we should be calculating on the fly. That has nothing much to do with tab completion, it just seems like a generally useful thing. No, I don't find that a useful idea. These things are not that expensive to check given that you have an open relcache entry to look at, which would be the case anywhere in the backend that we wanted to know them. The reason that running the functions in a tab-completion query looks unpleasant is that it'd imply opening (and probably locking) a large number of views. If we did put an updatable flag into the catalogs then (1) we'd be giving up the ability to change the updatability conditions without an initdb, and (2) we'd have a problem with updating the flag for referencing views when a referenced view changed its state. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Proof of concept: auto updatable views [Review of Patch]
Continuing to work on this ... I found multiple things I didn't like
about the permission-field update code. Attached are some heavily
commented extracts from the code as I've changed it. Does anybody
object to either the code or the objectives given in the comments?
regards, tom lane
/*
* adjust_view_column_set - map a set of column numbers according to targetlist
*
* This is used with simply-updatable views to map column-permissions sets for
* the view columns onto the matching columns in the underlying base relation.
* The targetlist is expected to be a list of plain Vars of the underlying
* relation (as per the checks above in view_is_auto_updatable).
*/
static Bitmapset *
adjust_view_column_set(Bitmapset *cols, List *targetlist)
{
Bitmapset *result = NULL;
Bitmapset *tmpcols;
AttrNumber col;
tmpcols = bms_copy(cols);
while ((col = bms_first_member(tmpcols)) = 0)
{
/* bit numbers are offset by FirstLowInvalidHeapAttributeNumber
*/
AttrNumber attno = col +
FirstLowInvalidHeapAttributeNumber;
if (attno == InvalidAttrNumber)
{
/*
* There's a whole-row reference to the view. For
permissions
* purposes, treat it as a reference to each column
available from
* the view. (We should *not* convert this to a
whole-row
* reference to the base relation, since the view may
not touch
* all columns of the base relation.)
*/
ListCell *lc;
foreach(lc, targetlist)
{
TargetEntry *tle = (TargetEntry *) lfirst(lc);
Var*var;
if (tle-resjunk)
continue;
var = (Var *) tle-expr;
Assert(IsA(var, Var));
result = bms_add_member(result,
var-varattno -
FirstLowInvalidHeapAttributeNumber);
}
}
else
{
/*
* Views do not have system columns, so we do not
expect to see
* any other system attnos here. If we do find one,
the error
* case will apply.
*/
TargetEntry *tle = get_tle_by_resno(targetlist, attno);
if (tle != NULL IsA(tle-expr, Var))
{
Var*var = (Var *) tle-expr;
result = bms_add_member(result,
var-varattno -
FirstLowInvalidHeapAttributeNumber);
}
else
elog(ERROR, attribute number %d not found in
view targetlist,
attno);
}
}
bms_free(tmpcols);
return result;
}
/*
* Mark the new target RTE for the permissions checks that we want to
* enforce against the view owner, as distinct from the query caller.
At
* the relation level, require the same INSERT/UPDATE/DELETE permissions
* that the query caller needs against the view. We drop the ACL_SELECT
* bit that is presumably in new_rte-requiredPerms initially.
*
* Note: the original view RTE remains in the query's rangetable list.
* Although it will be unused in the query plan, we need it there so
that
* the executor still performs appropriate permissions checks for the
* query caller's use of the view.
*/
new_rte-checkAsUser = view-rd_rel-relowner;
new_rte-requiredPerms = view_rte-requiredPerms;
/*
* Now for the per-column permissions bits.
*
* Initially, new_rte contains selectedCols permission check bits for
all
* base-rel columns referenced by the view, but since the view is a
SELECT
* query its modifiedCols is empty. We set modifiedCols to include all
* the columns the outer query is trying to modify, adjusting the column
* numbers as needed. But we leave selectedCols as-is, so the view
owner
* must have read permission for all columns used in the view
definition,
* even if some of them are not read by the upper query. We could try
to
* limit selectedCols to only columns used in the transformed query, but
* that does not correspond to what happens in ordinary SELECT usage of
a
Re: [HACKERS] Proof of concept: auto updatable views [Review of Patch]
On 8 December 2012 15:21, Tom Lane t...@sss.pgh.pa.us wrote: Continuing to work on this ... I found multiple things I didn't like about the permission-field update code. Attached are some heavily commented extracts from the code as I've changed it. Does anybody object to either the code or the objectives given in the comments? Thanks for looking at this. The new, adjusted code looks good to me. Regards, Dean -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Proof of concept: auto updatable views [Review of Patch]
Dean Rasheed dean.a.rash...@gmail.com writes: Attached is a rebased patch using new OIDs. Applied after a fair amount of hacking. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Proof of concept: auto updatable views [Review of Patch]
[ finally getting back to this patch, after many distractions ] Dean Rasheed dean.a.rash...@gmail.com writes: On 8 November 2012 21:13, Tom Lane t...@sss.pgh.pa.us wrote: I think the most reasonable backwards-compatibility argument is that we shouldn't change the behavior if there are either INSTEAD rules or INSTEAD triggers. Otherwise we may be disturbing carefully constructed behavior (and no, I don't buy that throw an error couldn't be what the user intended). The current behaviour, if there is only a conditional instead rule, is to throw an error whether or not that condition is satisfied. It's hard to imagine that's an error the user intended. Well, the current definition is that a rule set with a conditional DO INSTEAD rule is incomplete and needs to be fixed. I don't see anything particularly wrong with that, and I remain hesitant to silently redefine the behavior of existing rule sets. However, given the niche nature of conditional instead rules, it doesn't seem so bad to say that auto-updatable views don't support them at the moment, so long as backwards compatibility is maintained in the table and trigger-updatable view cases. So I think the current behaviour to maintain is, for a relation with only a conditional instead rule: if the relation is a table: if the condition is satisfied: fire the rule action else: modify the table else if the relation is a view with triggers: if the condition is satisfied: fire the rule action else: modify the view using the triggers else: throw an error unconditionally That's backwards compatible and easy to document - views with conditional instead rules are not auto-updatable. If anyone cared enough about it, or could come up with a realistic use case, we could always add support for that case in the future. But if there's an unconditional INSTEAD rule, we aren't going to apply the transformation either, so it seems to me this comes out to the same thing I said above: any kind of INSTEAD rule disables application of the auto-update transformation. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Proof of concept: auto updatable views [Review of Patch]
On 8 November 2012 03:10, Tom Lane t...@sss.pgh.pa.us wrote: Dean Rasheed dean.a.rash...@gmail.com writes: On 7 November 2012 22:04, Tom Lane t...@sss.pgh.pa.us wrote: This seems to me to be dangerous and unintuitive, not to mention underdocumented. I think it would be better to just not do anything if there is any INSTEAD rule, period. Is this any more dangerous than what already happens with qualified rules? If we did nothing here then it would go on to either fire any INSTEAD OF triggers or raise an error if there aren't any. The problem with that is that it makes trigger-updatable views and auto-updatable views inconsistent in their behaviour with qualified INSTEAD rules. Well, as submitted it's already pretty thoroughly inconsistent. The way the existing code works is that if there's no INSTEAD rule, and there's no INSTEAD trigger, you get an error *at runtime*. The reason for that is that the INSTEAD trigger might be added (or removed) between planning and execution. This code tries to decide at plan time whether there's a relevant trigger, and that's just not very safe. I realize that you can't deliver the specific error messages that currently appear in view_is_auto_updatable if you don't throw the error at plan time. But if you're going to claim that this ought to be consistent with the existing behavior, then I'm going to say we need to give that up and just have the runtime error, same as now. If you want the better error reporting (which I agree would be nice) then we need to revisit the interaction between INSTEAD triggers and INSTEAD rules anyway, and one of the things we probably should look at twice is whether it's sane at all to permit both a trigger and a qualified rule. I'd bet long odds that nobody is using such a thing in the field, and I think disallowing it might be a good idea in order to disentangle these features a bit better. OK, yes I think we do need to be throwing the error at runtime rather than at plan time. That's pretty easy if we just keep the current error message, but I think it would be very nice to have the more specific DETAIL text to go along with the error. We could save the value of is_view_auto_updatable() so that it's available to the executor, but that seems very ugly. A better approach might be to just call is_view_auto_updatable() again from the executor. At the point where we would be calling it, we would already know that the view isn't updatable, so we would just be looking for friendlier DETAIL text to give to the user. There's a chance that the view might have been changed structurally between planning an execution, making that DETAIL text incorrect, or even changing the fact that the view isn't updatable, but that seems pretty unlikely, and no worse than similar risks with tables. I think the whole thing with qualified rules is a separate issue. I don't really have a strong opinion on it because I never use qualified rules, but I am wary of changing the existing behaviour on backwards-compatibility grounds. I don't much like the way qualified rules work, but if we're going to support them then why should trigger/auto-updatable views be an exception to the way they work? Regards, Dean -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Proof of concept: auto updatable views [Review of Patch]
On 8 November 2012 08:33, Dean Rasheed dean.a.rash...@gmail.com wrote: OK, yes I think we do need to be throwing the error at runtime rather than at plan time. That's pretty easy if we just keep the current error message... Oh wait, that's nonsense (not enough caffeine). The rewrite code needs to know whether there are INSTEAD OF triggers before it decides whether it's going to substitute the base relation. The fundamental problem is that the plans with and without triggers are completely different, and there's no way the executor is going to notice the addition of triggers if they weren't there when the query was rewritten and planned. Regards, Dean -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Proof of concept: auto updatable views [Review of Patch]
On 8 November 2012 14:38, Dean Rasheed dean.a.rash...@gmail.com wrote: On 8 November 2012 08:33, Dean Rasheed dean.a.rash...@gmail.com wrote: OK, yes I think we do need to be throwing the error at runtime rather than at plan time. That's pretty easy if we just keep the current error message... Oh wait, that's nonsense (not enough caffeine). The rewrite code needs to know whether there are INSTEAD OF triggers before it decides whether it's going to substitute the base relation. The fundamental problem is that the plans with and without triggers are completely different, and there's no way the executor is going to notice the addition of triggers if they weren't there when the query was rewritten and planned. In fact doesn't the existing plan invalidation mechanism already protect us from this? Consider for example: create table foo(a int); create view foo_v as select a+1 as a from foo; create function foo_trig_fn() returns trigger as $$ begin insert into foo values(new.a-1); return new; end $$ language plpgsql; create trigger foo_trig instead of insert on foo_v for each row execute procedure foo_trig_fn(); Then I can do: prepare f(int) as insert into foo_v values($1); PREPARE execute f(1); INSERT 0 1 drop trigger foo_trig on foo_v; DROP TRIGGER execute f(2); ERROR: cannot insert into view foo_v DETAIL: Views with columns that are not simple references to columns in the base relation are not updatable. HINT: You need an unconditional ON INSERT DO INSTEAD rule or an INSTEAD OF INSERT trigger. create trigger foo_trig instead of insert on foo_v for each row execute procedure foo_trig_fn(); CREATE TRIGGER execute f(3); INSERT 0 1 Regards, Dean -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Proof of concept: auto updatable views [Review of Patch]
On Wed, Nov 07, 2012 at 05:55:32PM -0500, Tom Lane wrote: David Fetter da...@fetter.org writes: On Wed, Nov 07, 2012 at 05:04:48PM -0500, Tom Lane wrote: Should we be doing something about such cases, or is playing dumb correct? The SQL standard handles deciding the behavior based on whether WITH CHECK OPTION is included in the view DDL. See the section 2 of the SQL standard (Foundation) for details. Ah, I see it. So as long as we don't support WITH CHECK OPTION, we can ignore the issue. I don't think it's as simple as all that. WITH CHECK OPTION is how the SQL standard allows for creating update-able views in the first place, so we want to be at least aware of what the standard mandates. Here's what I'm able to apprehend from the standard. There are three different WITH CHECK OPTION options: WITH CHECK OPTION WITH CASCADED CHECK OPTION WITH LOCAL CHECK OPTION - WITH CHECK OPTION means that the results of INSERTs and UPDATEs on the view must be consistent with the view definition, i.e. INSERTs any of whose rows would be outside the view or UPDATEs which would push a row a row out of the view are disallowed. - WITH CASCADED CHECK OPTION is like the above, but stricter in that they ensure by checking views which depend on the view where the write operation is happening. INSERTs and UPDATEs have to stay in the lines for those dependent views. - WITH LOCAL CHECK OPTION allows INSERTs or UPDATEs that violate the view definition so long as they comply with the WITH CHECK OPTION on any dependent views. Apparently the LOCAL here means, delegate any CHECK OPTION checking to the dependent view, i.e. check it only locally and not right here. Oh, and I'm guessing at least one well-known financial services company would just love to have these :) Cheers, David. -- David Fetter da...@fetter.org http://fetter.org/ Phone: +1 415 235 3778 AIM: dfetter666 Yahoo!: dfetter Skype: davidfetter XMPP: david.fet...@gmail.com iCal: webcal://www.tripit.com/feed/ical/people/david74/tripit.ics Remember to vote! Consider donating to Postgres: http://www.postgresql.org/about/donate -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Proof of concept: auto updatable views [Review of Patch]
David Fetter da...@fetter.org writes: There are three different WITH CHECK OPTION options: WITH CHECK OPTION WITH CASCADED CHECK OPTION WITH LOCAL CHECK OPTION No, there are four: the fourth case being if you leave off the phrase altogether. That's the only case we accept, and it corresponds to the patch's behavior, ie, don't worry about it. Oh, and I'm guessing at least one well-known financial services company would just love to have these :) It might be material for a future patch, but it's not happening in this iteration. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Proof of concept: auto updatable views [Review of Patch]
On Thu, Nov 08, 2012 at 11:33:47AM -0500, Tom Lane wrote: David Fetter da...@fetter.org writes: There are three different WITH CHECK OPTION options: WITH CHECK OPTION WITH CASCADED CHECK OPTION WITH LOCAL CHECK OPTION No, there are four: the fourth case being if you leave off the phrase altogether. That's the only case we accept, and it corresponds to the patch's behavior, ie, don't worry about it. Good point. I just wanted to get that out there in the archives, as it took a bit of cross-referencing, interpreting and contemplation to come up with something relatively concise. Oh, and I'm guessing at least one well-known financial services company would just love to have these :) It might be material for a future patch, but it's not happening in this iteration. Right. Cheers, David. -- David Fetter da...@fetter.org http://fetter.org/ Phone: +1 415 235 3778 AIM: dfetter666 Yahoo!: dfetter Skype: davidfetter XMPP: david.fet...@gmail.com iCal: webcal://www.tripit.com/feed/ical/people/david74/tripit.ics Remember to vote! Consider donating to Postgres: http://www.postgresql.org/about/donate -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Proof of concept: auto updatable views [Review of Patch]
Dean Rasheed dean.a.rash...@gmail.com writes: On 8 November 2012 14:38, Dean Rasheed dean.a.rash...@gmail.com wrote: Oh wait, that's nonsense (not enough caffeine). The rewrite code needs to know whether there are INSTEAD OF triggers before it decides whether it's going to substitute the base relation. The fundamental problem is that the plans with and without triggers are completely different, and there's no way the executor is going to notice the addition of triggers if they weren't there when the query was rewritten and planned. That's a good point: if we apply the transform, then the view isn't the plan's target table at all anymore, and so whether it has INSTEAD triggers or not isn't going to be noticed at runtime. In fact doesn't the existing plan invalidation mechanism already protect us from this? I'd prefer not to trust that completely, ie the behavior should be somewhat failsafe if invalidation doesn't happen. Thinking about that, we have these cases for the auto-updatable case as submitted: 1. INSTEAD triggers added after planning: they'll be ignored, as per above, but the update on the base table should go through without surprises. 2. INSTEAD triggers removed after planning: you get an error at runtime, which seems fine. However, for the case of only-a-conditional-INSTEAD-rule, INSTEAD triggers added after planning will be fired. So that's not entirely consistent, but maybe that's all right if we expect that plan invalidation will normally prevent the case from occurring. Basically what I'm wondering about is whether the plan should get marked somehow to tell the executor that INSTEAD triggers are expected or not. This doesn't seem terribly easy though, since the rewriter is doing this well upstream of where we create a ModifyTable plan node. Maybe it's not worth it given that invalidation should usually protect us. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Proof of concept: auto updatable views [Review of Patch]
Dean Rasheed dean.a.rash...@gmail.com writes: If we did nothing here then it would go on to either fire any INSTEAD OF triggers or raise an error if there aren't any. The problem with that is that it makes trigger-updatable views and auto-updatable views inconsistent in their behaviour with qualified INSTEAD rules. I don't think the existing interaction between trigger-updatable views and qualified INSTEAD rules is documented, so perhaps that's something that needs work. I'm still unhappy about this decision though, and after further thought I think I can explain why a bit better: it's actually *not* like the way rules work now. The current rule semantics are basically that: 1. The original query is done only if there are no unconditional INSTEAD rules and no conditional INSTEAD rule's condition is true. 2. Unconditional INSTEAD actions are done, well, unconditionally. 3. Each conditional INSTEAD action is done if its condition is true. I believe that the right way to think about the auto-update transformation is that it should act like a supplied-by-default unconditional INSTEAD rule. Which would mean that it happens unconditionally, per #2. As submitted, though, the auto-update query executes only if there are no unconditional INSTEAD rules *and* no conditional INSTEAD rule's condition is true. I do not think this is either consistent or useful. It's treating the auto-update replacement query as if it were the original, which it is not. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Proof of concept: auto updatable views [Review of Patch]
On 8 November 2012 17:37, Tom Lane t...@sss.pgh.pa.us wrote: Dean Rasheed dean.a.rash...@gmail.com writes: If we did nothing here then it would go on to either fire any INSTEAD OF triggers or raise an error if there aren't any. The problem with that is that it makes trigger-updatable views and auto-updatable views inconsistent in their behaviour with qualified INSTEAD rules. I don't think the existing interaction between trigger-updatable views and qualified INSTEAD rules is documented, so perhaps that's something that needs work. I'm still unhappy about this decision though, and after further thought I think I can explain why a bit better: it's actually *not* like the way rules work now. The current rule semantics are basically that: 1. The original query is done only if there are no unconditional INSTEAD rules and no conditional INSTEAD rule's condition is true. 2. Unconditional INSTEAD actions are done, well, unconditionally. 3. Each conditional INSTEAD action is done if its condition is true. I believe that the right way to think about the auto-update transformation is that it should act like a supplied-by-default unconditional INSTEAD rule. Which would mean that it happens unconditionally, per #2. As submitted, though, the auto-update query executes only if there are no unconditional INSTEAD rules *and* no conditional INSTEAD rule's condition is true. I do not think this is either consistent or useful. It's treating the auto-update replacement query as if it were the original, which it is not. But if you treat the auto-update transformation as a supplied-by-default unconditional INSTEAD rule, and the user defines their own conditional INSTEAD rule, if the condition is true it would execute both the conditional rule action and the auto-update action, making it an ALSO rule rather than the INSTEAD rule the user specified. Taking a concrete example: create table foo(a int); create table bar(a int); create view foo_v as select * from foo; create rule foo_r as on insert to foo_v where new.a 0 do instead insert into bar values(new.a); I would expect that to put all positive values into foo, and all negative values into bar, which is indeed what happens as it stands. Regards, Dean -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Proof of concept: auto updatable views [Review of Patch]
Dean Rasheed dean.a.rash...@gmail.com writes: On 8 November 2012 17:37, Tom Lane t...@sss.pgh.pa.us wrote: I believe that the right way to think about the auto-update transformation is that it should act like a supplied-by-default unconditional INSTEAD rule. But if you treat the auto-update transformation as a supplied-by-default unconditional INSTEAD rule, and the user defines their own conditional INSTEAD rule, if the condition is true it would execute both the conditional rule action and the auto-update action, making it an ALSO rule rather than the INSTEAD rule the user specified. Well, that's how things work if you specify both a conditional and an unconditional INSTEAD action, so I don't find this so surprising. What you're arguing for would make some sense if the auto-update feature could be seen as something that acts ahead of, and independently of, INSTEAD rules and triggers. But it can't be treated that way: in particular, the fact that it doesn't fire when there's an INSTEAD trigger pretty much breaks the fiction that it's an independent feature. I would rather be able to explain its interaction with rules by saying it's a default implementation of an INSTEAD rule than by saying well, it has these weird interactions with INSTEAD rules, which are different for conditional and unconditional INSTEAD rules. Or we could go back to what I suggested to start with, which is that the auto-update transformation doesn't fire if there are *either* conditional or unconditional INSTEAD rules. That still seems like the best way if you want an arms-length definition of behavior; it means we can explain the interaction with INSTEAD rules exactly the same as the interaction with INSTEAD triggers, ie, having one prevents the transformation from being used. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Proof of concept: auto updatable views [Review of Patch]
On 8 November 2012 19:29, Tom Lane t...@sss.pgh.pa.us wrote: Dean Rasheed dean.a.rash...@gmail.com writes: On 8 November 2012 17:37, Tom Lane t...@sss.pgh.pa.us wrote: I believe that the right way to think about the auto-update transformation is that it should act like a supplied-by-default unconditional INSTEAD rule. But if you treat the auto-update transformation as a supplied-by-default unconditional INSTEAD rule, and the user defines their own conditional INSTEAD rule, if the condition is true it would execute both the conditional rule action and the auto-update action, making it an ALSO rule rather than the INSTEAD rule the user specified. Well, that's how things work if you specify both a conditional and an unconditional INSTEAD action, so I don't find this so surprising. To me, it's very surprising, so I must be thinking about it differently. I think that I'm really expecting auto-updatable views to behave like tables, so I keep coming back to the question what would happen if you did that to a table?. Taking another concrete example, I could use a conditional DO INSTEAD NOTHING rule on a table to prevent certain values from being inserted: create table foo(a int); create rule foo_r as on insert to foo where new.a 0 do instead nothing; insert into foo values(-1),(1); select * from foo; a --- 1 (1 row) So I would expect the same behaviour from an auto-updatable view: create table bar(a int); create view bar_v as select * from bar; create rule bar_r as on insert to bar_v where new.a 0 do instead nothing; insert into bar_v values(-1),(1); select * from bar_v; a --- 1 (1 row) Having that put both -1 and 1 into bar seems completely wrong to me. I could live with it raising a you need an unconditional instead rule error, but that makes the auto-update view seem a bit half-baked. This also seems like a much more plausible case where users might have done something like this with a trigger-updatable view, so I don't think the backwards-compatibility argument can be ignored. Regards, Dean What you're arguing for would make some sense if the auto-update feature could be seen as something that acts ahead of, and independently of, INSTEAD rules and triggers. But it can't be treated that way: in particular, the fact that it doesn't fire when there's an INSTEAD trigger pretty much breaks the fiction that it's an independent feature. I would rather be able to explain its interaction with rules by saying it's a default implementation of an INSTEAD rule than by saying well, it has these weird interactions with INSTEAD rules, which are different for conditional and unconditional INSTEAD rules. Or we could go back to what I suggested to start with, which is that the auto-update transformation doesn't fire if there are *either* conditional or unconditional INSTEAD rules. That still seems like the best way if you want an arms-length definition of behavior; it means we can explain the interaction with INSTEAD rules exactly the same as the interaction with INSTEAD triggers, ie, having one prevents the transformation from being used. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Proof of concept: auto updatable views [Review of Patch]
Dean Rasheed dean.a.rash...@gmail.com writes: create table bar(a int); create view bar_v as select * from bar; create rule bar_r as on insert to bar_v where new.a 0 do instead nothing; insert into bar_v values(-1),(1); select * from bar_v; a --- 1 (1 row) Having that put both -1 and 1 into bar seems completely wrong to me. Right now, what you get from that is ERROR: cannot insert into view bar_v HINT: You need an unconditional ON INSERT DO INSTEAD rule or an INSTEAD OF INSERT trigger. and (modulo the contents of the HINT) I think that's still what you should get. If the user has got some DO INSTEAD rules we should not be second-guessing what should happen. This also seems like a much more plausible case where users might have done something like this with a trigger-updatable view, so I don't think the backwards-compatibility argument can be ignored. I think the most reasonable backwards-compatibility argument is that we shouldn't change the behavior if there are either INSTEAD rules or INSTEAD triggers. Otherwise we may be disturbing carefully constructed behavior (and no, I don't buy that throw an error couldn't be what the user intended). regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Proof of concept: auto updatable views [Review of Patch]
On 8 November 2012 21:13, Tom Lane t...@sss.pgh.pa.us wrote: Dean Rasheed dean.a.rash...@gmail.com writes: create table bar(a int); create view bar_v as select * from bar; create rule bar_r as on insert to bar_v where new.a 0 do instead nothing; insert into bar_v values(-1),(1); select * from bar_v; a --- 1 (1 row) Having that put both -1 and 1 into bar seems completely wrong to me. Right now, what you get from that is ERROR: cannot insert into view bar_v HINT: You need an unconditional ON INSERT DO INSTEAD rule or an INSTEAD OF INSERT trigger. and (modulo the contents of the HINT) I think that's still what you should get. If the user has got some DO INSTEAD rules we should not be second-guessing what should happen. You say it's second-guessing what should happen, but in every example I've been able to think of, it does exactly what I would expect, and exactly what already happens for a table or a trigger-updatable view. Clearly though, what I expect/find surprising is at odds with what you expect/find surprising. If I think about it, I would summarise my expectations something like this: Given 2 identical tables table1 and table2, and view view2 defined as select * from table2, I would expect view2 to behave identically to table1 for all operations supported by both tables and views. In particular, given any set of rules defined on table1, if the matching set of rules is defined on view2, I would expect all queries on view2 to behave the same as the matching queries on table1. This also seems like a much more plausible case where users might have done something like this with a trigger-updatable view, so I don't think the backwards-compatibility argument can be ignored. I think the most reasonable backwards-compatibility argument is that we shouldn't change the behavior if there are either INSTEAD rules or INSTEAD triggers. Otherwise we may be disturbing carefully constructed behavior (and no, I don't buy that throw an error couldn't be what the user intended). The current behaviour, if there is only a conditional instead rule, is to throw an error whether or not that condition is satisfied. It's hard to imagine that's an error the user intended. However, given the niche nature of conditional instead rules, it doesn't seem so bad to say that auto-updatable views don't support them at the moment, so long as backwards compatibility is maintained in the table and trigger-updatable view cases. So I think the current behaviour to maintain is, for a relation with only a conditional instead rule: if the relation is a table: if the condition is satisfied: fire the rule action else: modify the table else if the relation is a view with triggers: if the condition is satisfied: fire the rule action else: modify the view using the triggers else: throw an error unconditionally That's backwards compatible and easy to document - views with conditional instead rules are not auto-updatable. If anyone cared enough about it, or could come up with a realistic use case, we could always add support for that case in the future. Regards, Dean -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Proof of concept: auto updatable views [Review of Patch]
Dean Rasheed dean.a.rash...@gmail.com writes:
Thanks for looking at this.
Attached is a rebased patch using new OIDs.
I'm starting to look at this patch now. I don't understand the intended
interaction with qualified INSTEAD rules. The code looks like
+ if (!instead rt_entry_relation-rd_rel-relkind ==
RELKIND_VIEW)
+ {
+ Query *query = qual_product ? qual_product : parsetree;
+ Query *newquery = rewriteTargetView(query,
rt_entry_relation);
which has the effect that if there's a qualified INSTEAD rule, we'll
apply the substitution transformation to the
modified-by-addition-of-negated-qual query (ie, qual_product).
This seems to me to be dangerous and unintuitive, not to mention
underdocumented. I think it would be better to just not do anything if
there is any INSTEAD rule, period. (I don't see any problem with DO
ALSO rules, though, since they don't affect the behavior of the original
query.)
Also, I didn't see anything in the thread concerning the behavior with
selective views. If we have say
CREATE VIEW v AS SELECT id, data FROM t WHERE id 1000;
and we do
INSERT INTO v VALUES(1, 'foo');
the row will be inserted but will then be invisible through the view.
Is that okay? I can't find anything in the SQL standard that says it
isn't, but it seems pretty weird. A related example is
UPDATE v SET id = 0 WHERE id = 1;
which has the effect of making the row disappear from the view, which is
not what you'd expect an UPDATE to do. Should we be doing something
about such cases, or is playing dumb correct?
regards, tom lane
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Proof of concept: auto updatable views [Review of Patch]
On Wed, Nov 07, 2012 at 05:04:48PM -0500, Tom Lane wrote:
Dean Rasheed dean.a.rash...@gmail.com writes:
Thanks for looking at this.
Attached is a rebased patch using new OIDs.
I'm starting to look at this patch now. I don't understand the intended
interaction with qualified INSTEAD rules. The code looks like
+ if (!instead rt_entry_relation-rd_rel-relkind ==
RELKIND_VIEW)
+ {
+ Query *query = qual_product ? qual_product : parsetree;
+ Query *newquery = rewriteTargetView(query,
rt_entry_relation);
which has the effect that if there's a qualified INSTEAD rule, we'll
apply the substitution transformation to the
modified-by-addition-of-negated-qual query (ie, qual_product).
This seems to me to be dangerous and unintuitive, not to mention
underdocumented. I think it would be better to just not do anything if
there is any INSTEAD rule, period. (I don't see any problem with DO
ALSO rules, though, since they don't affect the behavior of the original
query.)
Also, I didn't see anything in the thread concerning the behavior with
selective views. If we have say
CREATE VIEW v AS SELECT id, data FROM t WHERE id 1000;
and we do
INSERT INTO v VALUES(1, 'foo');
the row will be inserted but will then be invisible through the view.
Is that okay? I can't find anything in the SQL standard that says it
isn't, but it seems pretty weird. A related example is
UPDATE v SET id = 0 WHERE id = 1;
which has the effect of making the row disappear from the view, which is
not what you'd expect an UPDATE to do. Should we be doing something
about such cases, or is playing dumb correct?
The SQL standard handles deciding the behavior based on whether WITH
CHECK OPTION is included in the view DDL. See the section 2 of the
SQL standard (Foundation) for details.
Cheers,
David.
--
David Fetter da...@fetter.org http://fetter.org/
Phone: +1 415 235 3778 AIM: dfetter666 Yahoo!: dfetter
Skype: davidfetter XMPP: david.fet...@gmail.com
iCal: webcal://www.tripit.com/feed/ical/people/david74/tripit.ics
Remember to vote!
Consider donating to Postgres: http://www.postgresql.org/about/donate
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Proof of concept: auto updatable views [Review of Patch]
David Fetter da...@fetter.org writes: On Wed, Nov 07, 2012 at 05:04:48PM -0500, Tom Lane wrote: Should we be doing something about such cases, or is playing dumb correct? The SQL standard handles deciding the behavior based on whether WITH CHECK OPTION is included in the view DDL. See the section 2 of the SQL standard (Foundation) for details. Ah, I see it. So as long as we don't support WITH CHECK OPTION, we can ignore the issue. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Proof of concept: auto updatable views [Review of Patch]
On 7 November 2012 22:04, Tom Lane t...@sss.pgh.pa.us wrote:
Dean Rasheed dean.a.rash...@gmail.com writes:
Thanks for looking at this.
Attached is a rebased patch using new OIDs.
I'm starting to look at this patch now. I don't understand the intended
interaction with qualified INSTEAD rules. The code looks like
+ if (!instead rt_entry_relation-rd_rel-relkind ==
RELKIND_VIEW)
+ {
+ Query *query = qual_product ? qual_product :
parsetree;
+ Query *newquery = rewriteTargetView(query,
rt_entry_relation);
which has the effect that if there's a qualified INSTEAD rule, we'll
apply the substitution transformation to the
modified-by-addition-of-negated-qual query (ie, qual_product).
Yes that's what I was aiming for. I thought that if the rule's
qualifier isn't satisfied and the rule isn't fired, it should attempt
to go ahead with the view update. This might result in an INSTEAD OF
trigger firing, substitution of the base relation, or an error being
raised.
This seems to me to be dangerous and unintuitive, not to mention
underdocumented. I think it would be better to just not do anything if
there is any INSTEAD rule, period. (I don't see any problem with DO
ALSO rules, though, since they don't affect the behavior of the original
query.)
Is this any more dangerous than what already happens with qualified rules?
If we did nothing here then it would go on to either fire any INSTEAD
OF triggers or raise an error if there aren't any. The problem with
that is that it makes trigger-updatable views and auto-updatable views
inconsistent in their behaviour with qualified INSTEAD rules. I don't
think the existing interaction between trigger-updatable views and
qualified INSTEAD rules is documented, so perhaps that's something
that needs work.
Regards,
Dean
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Proof of concept: auto updatable views [Review of Patch]
Dean Rasheed dean.a.rash...@gmail.com writes: On 7 November 2012 22:04, Tom Lane t...@sss.pgh.pa.us wrote: This seems to me to be dangerous and unintuitive, not to mention underdocumented. I think it would be better to just not do anything if there is any INSTEAD rule, period. Is this any more dangerous than what already happens with qualified rules? If we did nothing here then it would go on to either fire any INSTEAD OF triggers or raise an error if there aren't any. The problem with that is that it makes trigger-updatable views and auto-updatable views inconsistent in their behaviour with qualified INSTEAD rules. Well, as submitted it's already pretty thoroughly inconsistent. The way the existing code works is that if there's no INSTEAD rule, and there's no INSTEAD trigger, you get an error *at runtime*. The reason for that is that the INSTEAD trigger might be added (or removed) between planning and execution. This code tries to decide at plan time whether there's a relevant trigger, and that's just not very safe. I realize that you can't deliver the specific error messages that currently appear in view_is_auto_updatable if you don't throw the error at plan time. But if you're going to claim that this ought to be consistent with the existing behavior, then I'm going to say we need to give that up and just have the runtime error, same as now. If you want the better error reporting (which I agree would be nice) then we need to revisit the interaction between INSTEAD triggers and INSTEAD rules anyway, and one of the things we probably should look at twice is whether it's sane at all to permit both a trigger and a qualified rule. I'd bet long odds that nobody is using such a thing in the field, and I think disallowing it might be a good idea in order to disentangle these features a bit better. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Proof of concept: auto updatable views [Review of Patch]
Thanks for looking at this. Attached is a rebased patch using new OIDs. On 11 October 2012 02:39, Peter Eisentraut pete...@gmx.net wrote: Compiler warning needs to be fixed: rewriteHandler.c: In function 'RewriteQuery': rewriteHandler.c:2153:29: error: 'view_rte' may be used uninitialized in this function [-Werror=maybe-uninitialized] rewriteHandler.c:2015:17: note: 'view_rte' was declared here Ah, my version of gcc doesn't give that warning. Looking at the code afresh though, I think that code block is pretty ugly. The attached version rewrites that block in a more compact form, which I think is also much more readable, and should cure the compiler warning. Maybe we should distinguish updatable from insertable in error messages like this one: ERROR: cannot insert into view foov2 DETAIL: Views containing DISTINCT are not updatable. The SQL standard distinguishes the two, so there could be differences. I'm not sure what they are right now, though. This hint could use some refreshing: HINT: You need an unconditional ON INSERT DO INSTEAD rule or an INSTEAD OF INSERT trigger. Maybe something along the lines of HINT: To make the view insertable anyway, supply an unconditional ... etc. I've not updated the error messages - I need to think about that a bit more. Regards, Dean auto-update-views.patch.gz Description: GNU Zip compressed data -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Proof of concept: auto updatable views [Review of Patch]
On 24 September 2012 12:10, Amit Kapila amit.kap...@huawei.com wrote: On Sunday, September 23, 2012 12:33 AM Dean Rasheed wrote: On 18 September 2012 14:23, Amit kapila amit.kap...@huawei.com wrote: Please find the review of the patch. Thanks for the review. Attached is an updated patch, and I've include some responses to specific review comments below. I have verified your updated patch. It works fine and according to me it is ready for committer to check this patch. I have updated in CF page as Ready for Committer. This patch will need adjusting to account for the fact that OID 3172 is now assigned to lo_truncate64 as of 3 days ago. -- Thom -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Proof of concept: auto updatable views [Review of Patch]
Compiler warning needs to be fixed: rewriteHandler.c: In function 'RewriteQuery': rewriteHandler.c:2153:29: error: 'view_rte' may be used uninitialized in this function [-Werror=maybe-uninitialized] rewriteHandler.c:2015:17: note: 'view_rte' was declared here Duplicate OIDs need to be adjusted: FATAL: could not create unique index pg_proc_oid_index DETAIL: Key (oid)=(3172) is duplicated. Maybe we should distinguish updatable from insertable in error messages like this one: ERROR: cannot insert into view foov2 DETAIL: Views containing DISTINCT are not updatable. The SQL standard distinguishes the two, so there could be differences. I'm not sure what they are right now, though. This hint could use some refreshing: HINT: You need an unconditional ON INSERT DO INSTEAD rule or an INSTEAD OF INSERT trigger. Maybe something along the lines of HINT: To make the view insertable anyway, supply an unconditional ... etc. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Proof of concept: auto updatable views [Review of Patch]
On Sunday, September 23, 2012 12:33 AM Dean Rasheed wrote: On 18 September 2012 14:23, Amit kapila amit.kap...@huawei.com wrote: Please find the review of the patch. Thanks for the review. Attached is an updated patch, and I've include some responses to specific review comments below. I have verified your updated patch. It works fine and according to me it is ready for committer to check this patch. I have updated in CF page as Ready for Committer. With Regards, Amit Kapila. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Proof of concept: auto updatable views [Review of Patch]
On 18 September 2012 14:23, Amit kapila amit.kap...@huawei.com wrote: Please find the review of the patch. Thanks for the review. Attached is an updated patch, and I've include some responses to specific review comments below. Extra test cases that can be added to regression suite are as below: 1. where clause in view select statement I have modified my original test cases to include WHERE clauses in the view definitions and confirmed using EXPLAIN that they are picked up as expected by DML statements. 2. ORDER BY, FOR, FETCH. The parser turns FETCH FIRST/NEXT into LIMIT before the query reaches the rewriter, so I don't think there is much point having separate tests for those cases. 3. Temp views, views on temp tables. Yes that just works. 4. Target entry JOIN, VALUES, FUNCTION I added a test with VALUES in the rangetable. The JOIN case is already covered by the existing test with multiple base relations, and the FUNCTION case is covered by the ro_view12 test. 5. Toast column I see no reason why that would be a problem. It just works. 6. System view Most system views aren't updatable because they involve multiple base relations, expressions in the target list or functions in the rangetable. This doesn't seem like a particularly useful use-case. 7. Lateral and outer join This is covered by the existing test using multiple base relations. 8. auto increment columns 9. Triggers on tables 10.View with default values I've added these and they appear to work as I would expect. 11.Choosing base relation based on schema. 12.SECURITY DEFINER function execution These also work, but I'm not sure that the tests are proving anything useful. Code Review: 1. In test_auto_update_view function if (var-varattno == 0) return Views that refer to whole rows from the base relation are not updatable; I have a doubt that when the above scenario will cover? And the examples provided for whole row are working. This protects against a whole row reference in the target list (for example CREATE VIEW test_view AS SELECT base_tbl FROM base_tbl). The case that is allowed is a whole row reference in the WHERE clause. 2. In test_auto_update_view function if (base_rte-rtekind != RTE_RELATION) return Views that are not based on tables or views are not updatable; for view on sequences also the query is rewritten and giving error while executing. Is it possible to check for a particular relkind before rewriting query? Updated, so now it raises the error in the rewriter rather than the executor. 3. In function rewriteTargetView if (tle-resjunk || tle-resno = 0) continue; The above scenario is not possible as the junk is already removed in above condition and also the view which is refering to the system columns are not auto update views. OK, I've removed that check. The next test should catch anything unexpected that gets through. 4. In function rewriteTargetView if (view_tle == NULL) elog(ERROR, View column %d not found, tle-resno); The parsetree targetlist is already validated with view targetlist during transformstmt. Giving an ERROR is fine here? Shouldn't it be Assert? I think the elog(ERROR) is correct here, otherwise we'd be crashing. It ought to be impossible but it's not completely obvious that it can't somehow happen. 5. if any derived columns are present on the view, at least UPDATE operation can be allowed for columns other than derived columns. Yes, but I think that's the subject for another patch. In this patch, I'm just aiming to implement the SQL-92 feature. 6. name test_auto_update_view can be changed. The word test can be changed. OK, I've renamed it to is_view_auto_updatable(). 7. From function get_view_query(), error message : invalid _RETURN rule action specification might not make much sense to user who is inserting in a view. This is an internal elog() error, rather than a user-facing error. It should not happen in practice, unless perhaps the user has been messing with their system catalogs. Defects from test --- 1. With a old database and new binaries the following test code results in wrong way. CREATE TABLE base_tbl (a int PRIMARY KEY, b text DEFAULT 'Unspecified'); INSERT INTO base_tbl VALUES (1, 'Row 1'); INSERT INTO base_tbl VALUES (2, 'Row 2'); CREATE VIEW rw_view1 AS SELECT * FROM base_tbl; SELECT table_name, is_updatable, is_insertable_into FROM information_schema.views where table_name = 'rw_view1'; This will show is_insertable_into as 'no'. However below SQL statement is success INSERT INTO rw_view1 VALUES (3, 'Row 3'); That's because the information_schema needs updating in your old database, which I think means that the catalog version number needs to be bumped when/if it is committed. 2.
Re: [HACKERS] Proof of concept: auto updatable views [Review of Patch]
On 31 August 2012 07:59, Dean Rasheed dean(dot)a(dot)rasheed(at)gmail(dot)com wrote: On 30 August 2012 20:05, Robert Haas robertmhaas(at)gmail(dot)com wrote: On Sun, Aug 12, 2012 at 5:14 PM, Dean Rasheed dean(dot)a(dot)rasheed(at)gmail(dot)com wrote: Here's an updated patch for the base feature (without support for security barrier views) with updated docs and regression tests. Please find the review of the patch. Basic stuff: -- - Patch applies OK - Compiles cleanly with no warnings - Regression tests pass. - Documentation changes are mostly fine. What it does: The non-select DML operations on views which are simple updatable (The condition checks can be found in test section), can rewrite the query and execute it even if the view don't have rules and instead of triggers. Testing: The following test is carried out on the patch. 1. create a view which can be automatically updated. - no of view columns are not matching with actual base relation - column order should be different with base relation order - view column aliases as another column name - view with a where condition - column contains some constraints. - ORDER BY - FOR 2. create a view with all the features where automatically update is not possible. - Distinct clauses - Every TLE is not a column reference. - TLE appears more than once. - Refers to more than one base relation. - Other than relation in FROM clause. - GROUP BY or HAVING clauses. - set operations (UNION, INTERSECT or EXCEPT). - sub-queries in the WHERE clause. - DISTINCT ON clauses. - window functions. - CTEs (WITH or WITH RECURSIVE). - OFFSET or LIMIT clauses. - system columns. - security_barrier; - refers to whole rows of a table. - column permissions are not there for users. - refers to a sequence instead of relation. - 3. create a view which is having instead of triggers. 4. create a view which is having rules. 5. create a view on a base relation which is also a view of automatically updated. 6. create a view on a base relation which is also a view having instead of triggers. 7. create a view on a base relation which is also a view having rules. Extra test cases that can be added to regression suite are as below: 1. where clause in view select statement 2. ORDER BY, FOR, FETCH. 3. Temp views, views on temp tables. 4. Target entry JOIN, VALUES, FUNCTION 5. Toast column 6. System view 7. Lateral and outer join 8. auto increment columns 9. Triggers on tables 10.View with default values 11.Choosing base relation based on schema. 12.SECURITY DEFINER function execution The updated regression test sql file with extra test is attached with this mail, please check and add it to the patch. Code Review: 1. In test_auto_update_view function if (var-varattno == 0) return Views that refer to whole rows from the base relation are not updatable; I have a doubt that when the above scenario will cover? And the examples provided for whole row are working. 2. In test_auto_update_view function if (base_rte-rtekind != RTE_RELATION) return Views that are not based on tables or views are not updatable; for view on sequences also the query is rewritten and giving error while executing. Is it possible to check for a particular relkind before rewriting query? 3. In function rewriteTargetView if (tle-resjunk || tle-resno = 0) continue; The above scenario is not possible as the junk is already removed in above condition and also the view which is refering to the system columns are not auto update views. 4. In function rewriteTargetView if (view_tle == NULL) elog(ERROR, View column %d not found, tle-resno); The parsetree targetlist is already validated with view targetlist during transformstmt. Giving an ERROR is fine here? Shouldn't it be Assert? 5. if any derived columns are present on the view, at least UPDATE operation can be allowed for columns other than derived columns. 6. name test_auto_update_view can be changed. The word test can be changed. 7. From function get_view_query(), error message : invalid _RETURN rule action specification might not make much sense to user who is inserting in a view. Defects from test --- 1. With a old database and new binaries the following test code results in wrong way. CREATE TABLE base_tbl (a int PRIMARY KEY, b text DEFAULT 'Unspecified'); INSERT INTO base_tbl VALUES (1, 'Row 1'); INSERT INTO base_tbl VALUES (2, 'Row 2'); CREATE VIEW rw_view1 AS SELECT * FROM base_tbl; SELECT table_name, is_updatable, is_insertable_into FROM information_schema.views where table_name = 'rw_view1'; This will show is_insertable_into as 'no'. However below SQL statement is success INSERT
Re: [HACKERS] Proof of concept: auto updatable views
On 31 August 2012 07:59, Dean Rasheed dean.a.rash...@gmail.com wrote: On 30 August 2012 20:05, Robert Haas robertmh...@gmail.com wrote: On Sun, Aug 12, 2012 at 5:14 PM, Dean Rasheed dean.a.rash...@gmail.com wrote: None of this new code kicks in for non-security barrier views, so the kinds of plans I posted upthread remain unchanged in that case. But now a significant fraction of the patch is code added to handle security barrier views. Of course we could simply say that such views aren't updatable, but that seems like an annoying limitation if there is a feasible way round it. Maybe it'd be a good idea to split this into two patches: the first could implement the feature but exclude security_barrier views, and the second could lift that restriction. Yes, I think that makes sense. I should hopefully get some time to look at it over the weekend. Here's an updated patch for the base feature (without support for security barrier views) with updated docs and regression tests. Regards, Dean auto-update-views.patch.gz Description: GNU Zip compressed data -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Proof of concept: auto updatable views
On 30 August 2012 20:05, Robert Haas robertmh...@gmail.com wrote: On Sun, Aug 12, 2012 at 5:14 PM, Dean Rasheed dean.a.rash...@gmail.com wrote: None of this new code kicks in for non-security barrier views, so the kinds of plans I posted upthread remain unchanged in that case. But now a significant fraction of the patch is code added to handle security barrier views. Of course we could simply say that such views aren't updatable, but that seems like an annoying limitation if there is a feasible way round it. Maybe it'd be a good idea to split this into two patches: the first could implement the feature but exclude security_barrier views, and the second could lift that restriction. Yes, I think that makes sense. I should hopefully get some time to look at it over the weekend. Regards, Dean -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Proof of concept: auto updatable views
On Sun, Aug 12, 2012 at 5:14 PM, Dean Rasheed dean.a.rash...@gmail.com wrote: None of this new code kicks in for non-security barrier views, so the kinds of plans I posted upthread remain unchanged in that case. But now a significant fraction of the patch is code added to handle security barrier views. Of course we could simply say that such views aren't updatable, but that seems like an annoying limitation if there is a feasible way round it. Maybe it'd be a good idea to split this into two patches: the first could implement the feature but exclude security_barrier views, and the second could lift that restriction. Just a thought. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Proof of concept: auto updatable views
On 27 August 2012 20:26, Dean Rasheed dean.a.rash...@gmail.com wrote: Here's an updated WIP patch which I'll add to the next commitfest. Re-sending gzipped (apparently the mail system corrupted it last time). Regards, Dean auto-update-views.patch.gz Description: GNU Zip compressed data -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Proof of concept: auto updatable views
I've been looking at this further and I have made some improvements, but also found a problem. On 1 July 2012 23:35, Dean Rasheed dean.a.rash...@gmail.com wrote: I'm also aware that my new function ChangeVarAttnos() is almost identical to the function map_variable_attnos() that Tom recently added, but I couldn't immediately see a neat way to merge the two. My function handles whole-row references to the view by mapping them to a generic RowExpr based on the view definition. I don't think a ConvertRowtypeExpr can be used in this case, because the column names don't necessarily match. I improved on this by reusing the existing function ResolveNew() which reduced the size of the patch. The problem, however, is that the original patch is not safe for UPDATE/DELETE on security barrier views, because it mixes the user's quals with those on the view. For example a query like UPDATE some_view SET col=... WHERE user_quals will get rewritten as UPDATE base_table SET base_col=... WHERE user_quals AND view_quals which potentially leaks data hidden by the view's quals. So I think that it needs to use a subquery to isolate user_quals from view_quals. The least invasive way I could see to do that was to record the view quals in a new security barrier quals field on the RTE, and then turn that into a subquery at the end of the rewriting process. This approach avoids the extensive changes to the rewriter that I think would otherwise be needed if the RTE were changed into a subquery near the start of the rewriting process. It is also possible that this code might be reusable in the row-level security patch by Kohei KaiGai to handle modifications to tables with RLS quals, although I haven't looked too closely at that patch yet. Another complication is that the executor appears to have no rechecking code for subqueries in nodeSubqueryscan.c, so it looks like I need to lock rows coming from the base relation in the subquery. So for example, given the following setup CREATE TABLE private_t(a int, b text); INSERT INTO private_t VALUES (1, 'Private'); INSERT INTO private_t SELECT i, 'Public '||i FROM generate_series(2,1) g(i); CREATE INDEX private_t_a_idx ON private_t(a); ANALYSE private_t; CREATE VIEW public_v WITH (security_barrier=true) AS SELECT b AS bb, a AS aa FROM private_t WHERE a != 1; CREATE OR REPLACE FUNCTION snoop(b text) RETURNS boolean AS $$ BEGIN RAISE NOTICE 'b=%', b; RETURN false; END; $$ LANGUAGE plpgsql STRICT IMMUTABLE COST 0.01; an update on the view will get rewritten as an update on the base table from a subquery scan as follows: EXPLAIN (costs off) UPDATE public_v SET aa=aa WHERE snoop(bb) AND aa=2; Update on private_t - Subquery Scan on private_t Filter: snoop(private_t.b) - LockRows - Index Scan using private_t_a_idx on private_t Index Cond: (a = 2) Filter: (a 1) The LockRows node appears to give the expected behaviour for concurrently modified rows, but I'm not really sure if that's the right approach. None of this new code kicks in for non-security barrier views, so the kinds of plans I posted upthread remain unchanged in that case. But now a significant fraction of the patch is code added to handle security barrier views. Of course we could simply say that such views aren't updatable, but that seems like an annoying limitation if there is a feasible way round it. Thoughts? Regards, Dean auto-update-views.patch Description: Binary data -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Proof of concept: auto updatable views
On 2 July 2012 21:34, Tom Lane t...@sss.pgh.pa.us wrote: Robert Haas robertmh...@gmail.com writes: On Sun, Jul 1, 2012 at 6:35 PM, Dean Rasheed dean.a.rash...@gmail.com wrote: Basically what it does is this: in the first stage of query rewriting, just after any non-SELECT rules are applied, the new code kicks in - if the target relation is a view, and there were no unqualified INSTEAD rules, and there are no INSTEAD OF triggers, it tests if ... The consensus last time seemed to be that backwards compatibility should be offered through a new GUC variable to allow this feature to be disabled globally, which I've not implemented yet. I think the backward-compatibility concerns with this approach would be much less than with the previously-proposed approach, so I'm not 100% sure we need a backward compatibility knob. If the above description is correct, the behavior is changed only in cases that previously threw errors, so I tend to agree that no backwards compatibility knob is needed. Yeah, I think you're right - the default ACLs will typically give sensible behaviour. So unless users have been cavalier with the use of GRANT ALL, their existing views are only going to start becoming updatable to the owners of the views (and only then if the view owner already has write permission on the underlying table). Regards, Dean -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Proof of concept: auto updatable views
On Sun, Jul 1, 2012 at 6:35 PM, Dean Rasheed dean.a.rash...@gmail.com wrote: I've been playing around with the idea of supporting automatically updatable views, and I have a working proof of concept. I've taken a different approach than the previous attempts to implement this feature (e.g., http://archives.postgresql.org/pgsql-hackers/2009-01/msg01746.php), instead doing all the work in the rewriter, substituting the view for its base relation rather than attempting to auto-generate any rules or triggers. Basically what it does is this: in the first stage of query rewriting, just after any non-SELECT rules are applied, the new code kicks in - if the target relation is a view, and there were no unqualified INSTEAD rules, and there are no INSTEAD OF triggers, it tests if the view is simply updatable. If so, the target view is replaced by its base relation and columns are re-mapped. Then the remainder of the rewriting process continues, recursively handling any further non-SELECT rules or additional simply updatable views. This handles the case of views on top of views, with or without rules and/or triggers. Regrettably, I don't have time to look at this in detail right now, but please add it to the next CommitFest so it gets looked at. It sounds like it could be very cool. The consensus last time seemed to be that backwards compatibility should be offered through a new GUC variable to allow this feature to be disabled globally, which I've not implemented yet. I think the backward-compatibility concerns with this approach would be much less than with the previously-proposed approach, so I'm not 100% sure we need a backward compatibility knob. If we're going to have one, a reloption would probably be a better fit than a GUC, now that views support reloptions. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Proof of concept: auto updatable views
Robert Haas robertmh...@gmail.com writes: On Sun, Jul 1, 2012 at 6:35 PM, Dean Rasheed dean.a.rash...@gmail.com wrote: Basically what it does is this: in the first stage of query rewriting, just after any non-SELECT rules are applied, the new code kicks in - if the target relation is a view, and there were no unqualified INSTEAD rules, and there are no INSTEAD OF triggers, it tests if ... The consensus last time seemed to be that backwards compatibility should be offered through a new GUC variable to allow this feature to be disabled globally, which I've not implemented yet. I think the backward-compatibility concerns with this approach would be much less than with the previously-proposed approach, so I'm not 100% sure we need a backward compatibility knob. If the above description is correct, the behavior is changed only in cases that previously threw errors, so I tend to agree that no backwards compatibility knob is needed. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] Proof of concept: auto updatable views
Hi, I've been playing around with the idea of supporting automatically updatable views, and I have a working proof of concept. I've taken a different approach than the previous attempts to implement this feature (e.g., http://archives.postgresql.org/pgsql-hackers/2009-01/msg01746.php), instead doing all the work in the rewriter, substituting the view for its base relation rather than attempting to auto-generate any rules or triggers. Basically what it does is this: in the first stage of query rewriting, just after any non-SELECT rules are applied, the new code kicks in - if the target relation is a view, and there were no unqualified INSTEAD rules, and there are no INSTEAD OF triggers, it tests if the view is simply updatable. If so, the target view is replaced by its base relation and columns are re-mapped. Then the remainder of the rewriting process continues, recursively handling any further non-SELECT rules or additional simply updatable views. This handles the case of views on top of views, with or without rules and/or triggers. Here's a simple example: CREATE TABLE my_table(id int primary key, val text); CREATE VIEW my_view AS SELECT * FROM my_table WHERE id 0; then any modifications to the view get redirected to underlying table: EXPLAIN ANALYSE INSERT INTO my_view VALUES(1, 'Test row'); QUERY PLAN Insert on my_table (cost=0.00..0.01 rows=1 width=0) (actual time=0.208..0.208 rows=0 loops=1) - Result (cost=0.00..0.01 rows=1 width=0) (actual time=0.004..0.004 rows=1 loops=1) Total runtime: 0.327 ms (3 rows) EXPLAIN ANALYSE UPDATE my_view SET val='Updated' WHERE id=1; QUERY PLAN --- Update on my_table (cost=0.00..8.27 rows=1 width=10) (actual time=0.039..0.039 rows=0 loops=1) - Index Scan using my_table_pkey on my_table (cost=0.00..8.27 rows=1 width=10) (actual time=0.014..0.015 rows=1 loops=1) Index Cond: ((id 0) AND (id = 1)) Total runtime: 0.090 ms (4 rows) EXPLAIN ANALYSE DELETE FROM my_view; QUERY PLAN Delete on my_table (cost=0.00..1.01 rows=1 width=6) (actual time=0.030..0.030 rows=0 loops=1) - Seq Scan on my_table (cost=0.00..1.01 rows=1 width=6) (actual time=0.015..0.016 rows=1 loops=1) Filter: (id 0) Total runtime: 0.063 ms (4 rows) The patch is currently very strict about what kinds of views can be updated (based on SQL-92), and there is no support for WITH CHECK OPTION, because I wanted to keep this patch as simple as possible. The consensus last time seemed to be that backwards compatibility should be offered through a new GUC variable to allow this feature to be disabled globally, which I've not implemented yet. I'm also aware that my new function ChangeVarAttnos() is almost identical to the function map_variable_attnos() that Tom recently added, but I couldn't immediately see a neat way to merge the two. My function handles whole-row references to the view by mapping them to a generic RowExpr based on the view definition. I don't think a ConvertRowtypeExpr can be used in this case, because the column names don't necessarily match. Obviously there's still more work to do but the early signs seem to be encouraging. Thoughts? Regards, Dean auto-update-views.patch Description: Binary data -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Proof of concept: auto updatable views
My thoughts on this is that it would be a very valuable feature to have, and would make Postgres views behave more like they always were intended to behave, which is indistinguishible to users from tables in behavior where all possible, and that the reverse mapping would be automatic with the DBMS being given only the view-defining SELECT, where possible. -- Darren Duncan Dean Rasheed wrote: I've been playing around with the idea of supporting automatically updatable views, and I have a working proof of concept. I've taken a different approach than the previous attempts to implement this feature (e.g., http://archives.postgresql.org/pgsql-hackers/2009-01/msg01746.php), instead doing all the work in the rewriter, substituting the view for its base relation rather than attempting to auto-generate any rules or triggers. Basically what it does is this: in the first stage of query rewriting, just after any non-SELECT rules are applied, the new code kicks in - if the target relation is a view, and there were no unqualified INSTEAD rules, and there are no INSTEAD OF triggers, it tests if the view is simply updatable. If so, the target view is replaced by its base relation and columns are re-mapped. Then the remainder of the rewriting process continues, recursively handling any further non-SELECT rules or additional simply updatable views. This handles the case of views on top of views, with or without rules and/or triggers. snip Obviously there's still more work to do but the early signs seem to be encouraging. Thoughts? -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
