Re: 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 INTO rw_view1 VALUES (3, 'Row 3');

2. User-1:
Table and views are created under user-1.
Grant select and insert permission to user-2 on table and view.
Alter the view owner as user-3.

User-2:
Try to insert into the view is failing because of permission's even though user-2 has rights on both table and view.

Documentation Improvements
--------------------------------------------
1. Under title Updateable Views -

If the view satisifes all these conditions then it will be automatically updatable.
This seems to be appearing both before and after the conditions of non-updateable views.
2. Grant of rights on views should be mentioned separatly.

With Regards,

Amit Kapila.

On 18 September 2012 14:23, Amit kapila <amit(dot)kapila(at)huawei(dot)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. User-1:
> Table and views are created under user-1.
> Grant select and insert permission to user-2 on table and view.
> Alter the view owner as user-3.
>
> User-2:
> Try to insert into the view is failing because of permission's even
> though user-2 has rights on both table and view.
>

Yes that's correct behaviour, and is consistent with a SELECT from the
view. The user that executes the query (user_2) needs permissions on
the view, and the owner of the view/rule (user_3) needs permissions on
the underlying table. It is not sufficient for user_2 to have
permissions on the table because the permissions in the rewritten
query are checked against the view/rule owner (user_3). See
http://www.postgresql.org/docs/current/static/rules-privileges.html.

> Documentation Improvements
> --------------------------------------------
> 1. Under title Updateable Views -
>
> If the view satisifes all these conditions then it will be automatically
> updatable.
> This seems to be appearing both before and after the conditions of
> non-updateable views.
> 2. Grant of rights on views should be mentioned separatly.
>

I've tidied that up a bit and added a new paragraph with a reference
to the section on rules and privileges.

Regards,
Dean

On Sunday, September 23, 2012 12:33 AM Dean Rasheed wrote:
> On 18 September 2012 14:23, Amit kapila <amit(dot)kapila(at)huawei(dot)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.

On 24 September 2012 12:10, Amit Kapila <amit(dot)kapila(at)huawei(dot)com> wrote:
> On Sunday, September 23, 2012 12:33 AM Dean Rasheed wrote:
>> On 18 September 2012 14:23, Amit kapila <amit(dot)kapila(at)huawei(dot)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

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.

Thanks for looking at this.
Attached is a rebased patch using new OIDs.

On 11 October 2012 02:39, Peter Eisentraut <peter_e(at)gmx(dot)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

Dean Rasheed <dean(dot)a(dot)rasheed(at)gmail(dot)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 = 10000;

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

On Wed, Nov 07, 2012 at 05:04:48PM -0500, Tom Lane wrote:
> Dean Rasheed <dean(dot)a(dot)rasheed(at)gmail(dot)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 = 10000;
>
> 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 <david(at)fetter(dot)org> http://fetter.org/
Phone: +1 415 235 3778 AIM: dfetter666 Yahoo!: dfetter
Skype: davidfetter XMPP: david(dot)fetter(at)gmail(dot)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

David Fetter <david(at)fetter(dot)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

On 7 November 2012 22:04, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Dean Rasheed <dean(dot)a(dot)rasheed(at)gmail(dot)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

Dean Rasheed <dean(dot)a(dot)rasheed(at)gmail(dot)com> writes:
> On 7 November 2012 22:04, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)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

On 8 November 2012 03:10, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Dean Rasheed <dean(dot)a(dot)rasheed(at)gmail(dot)com> writes:
>> On 7 November 2012 22:04, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)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

On 8 November 2012 08:33, Dean Rasheed <dean(dot)a(dot)rasheed(at)gmail(dot)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

On 8 November 2012 14:38, Dean Rasheed <dean(dot)a(dot)rasheed(at)gmail(dot)com> wrote:
> On 8 November 2012 08:33, Dean Rasheed <dean(dot)a(dot)rasheed(at)gmail(dot)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

On Wed, Nov 07, 2012 at 05:55:32PM -0500, Tom Lane wrote:
> David Fetter <david(at)fetter(dot)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 <david(at)fetter(dot)org> http://fetter.org/
Phone: +1 415 235 3778 AIM: dfetter666 Yahoo!: dfetter
Skype: davidfetter XMPP: david(dot)fetter(at)gmail(dot)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

David Fetter <david(at)fetter(dot)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

On Thu, Nov 08, 2012 at 11:33:47AM -0500, Tom Lane wrote:
> David Fetter <david(at)fetter(dot)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 <david(at)fetter(dot)org> http://fetter.org/
Phone: +1 415 235 3778 AIM: dfetter666 Yahoo!: dfetter
Skype: davidfetter XMPP: david(dot)fetter(at)gmail(dot)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

Dean Rasheed <dean(dot)a(dot)rasheed(at)gmail(dot)com> writes:
> On 8 November 2012 14:38, Dean Rasheed <dean(dot)a(dot)rasheed(at)gmail(dot)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

Dean Rasheed <dean(dot)a(dot)rasheed(at)gmail(dot)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

On 8 November 2012 17:37, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Dean Rasheed <dean(dot)a(dot)rasheed(at)gmail(dot)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

Dean Rasheed <dean(dot)a(dot)rasheed(at)gmail(dot)com> writes:
> On 8 November 2012 17:37, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)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

On 8 November 2012 19:29, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Dean Rasheed <dean(dot)a(dot)rasheed(at)gmail(dot)com> writes:
>> On 8 November 2012 17:37, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)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

Dean Rasheed <dean(dot)a(dot)rasheed(at)gmail(dot)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

On 8 November 2012 21:13, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Dean Rasheed <dean(dot)a(dot)rasheed(at)gmail(dot)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

[ finally getting back to this patch, after many distractions ]

Dean Rasheed <dean(dot)a(dot)rasheed(at)gmail(dot)com> writes:
> On 8 November 2012 21:13, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)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

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
* view: all referenced columns must have read permission, even if
* optimization finds that some of them can be discarded during query
* transformation. The flattening we're doing here is an optional
* optimization, too. (If you are unpersuaded and want to change this,
* note that applying adjust_view_column_set to view_rte->selectedCols is
* clearly *not* the right answer, since that neglects base-rel columns
* used in the view's WHERE quals.)
*/
Assert(bms_is_empty(new_rte->modifiedCols));
new_rte->modifiedCols = adjust_view_column_set(view_rte->modifiedCols,
view_targetlist);

On 8 December 2012 15:21, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)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

Dean Rasheed <dean(dot)a(dot)rasheed(at)gmail(dot)com> writes:
> Attached is a rebased patch using new OIDs.

Applied after a fair amount of hacking.

regards, tom lane

On 8 December 2012 23:29, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Dean Rasheed <dean(dot)a(dot)rasheed(at)gmail(dot)com> writes:
>> Attached is a rebased patch using new OIDs.
>
> Applied after a fair amount of hacking.
>

Awesome! Thank you very much.

Regards,
Dean

On 8 December 2012 23:29, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:

> Dean Rasheed <dean(dot)a(dot)rasheed(at)gmail(dot)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

Thom Brown <thom(at)linux(dot)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

On 9 December 2012 16:53, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Thom Brown <thom(at)linux(dot)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

Dean Rasheed <dean(dot)a(dot)rasheed(at)gmail(dot)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

On 9 December 2012 22:00, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Dean Rasheed <dean(dot)a(dot)rasheed(at)gmail(dot)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

On 9 December 2012 22:00, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Dean Rasheed <dean(dot)a(dot)rasheed(at)gmail(dot)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

Simon Riggs <simon(at)2ndQuadrant(dot)com> writes:
> On 9 December 2012 22:00, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)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