Обсуждение: INSTEAD rule bug?

Here is a problem a ran into:

testdb=# create table test (x int);
CREATE TABLE
testdb=# create table test_view as select * from test;
SELECT
testdb=# create rule insert_test as on insert to test_view do instead
insert into test values (new.*);
CREATE RULE
testdb=# create rule skip_test as on insert to test_view where x is null
do instead nothing;
CREATE RULE
testdb=# insert into test_view values (null);
INSERT 17259 1
testdb=# select * from test;
 x
---

(1 row)

According to the last rule the insert should not have happened, right?
How come it got ignored?

Thanks!

Dima

Dmitry Tkach <dmitry@openratings.com> writes:
> testdb=# create table test_view as select * from test;
> SELECT

That is not a view, it's only a static copy of the original table.

            regards, tom lane

Tom Lane wrote:

>Dmitry Tkach <dmitry@openratings.com> writes:
>
>
>>testdb=# create table test_view as select * from test;
>>SELECT
>>
>>
>
>That is not a view, it's only a static copy of the original table.
>
>            regards, tom lane
>
>
I know... That was a typo in my sql :-)
But for this example it doesn't matter - that view/table is only needed
to illustrate the rules behaviour on insert.
You can just replace 'table' with 'view' in that statement - the
behaviour of the insert is exactly the same anyway.

Dima

Dmitry Tkach <dmitry@openratings.com> writes:
> I know... That was a typo in my sql :-)
> But for this example it doesn't matter - that view/table is only needed
> to illustrate the rules behaviour on insert.

Oh, I see what you're on about.  Sorry, a "DO INSTEAD NOTHING" only
suppresses the original command, it does not suppress other rules.
I think what you want is to make the insert_test rule conditional
on x being not null.

            regards, tom lane

Tom Lane wrote:

>Oh, I see what you're on about.  Sorry, a "DO INSTEAD NOTHING" only
>suppresses the original command, it does not suppress other rules.
>I think what you want is to make the insert_test rule conditional
>on x being not null.
>
>
>
Yeah... that's what I was afraid of :-(
The problem is that in the 'real life' situation the condition is a lot
more complicated than this simple is null test... I hate having to
duplicate it, and I hate even more having to evaluate it twice on every
insert :-(

Dima

Dmitry Tkach <dmitry@openratings.com> writes:
> The problem is that in the 'real life' situation the condition is a lot
> more complicated than this simple is null test... I hate having to
> duplicate it, and I hate even more having to evaluate it twice on every
> insert :-(

Why evaluate it twice?  The DO INSTEAD NOTHING rule should be
unconditional.

            regards, tom lane

Tom Lane wrote:

>Dmitry Tkach <dmitry@openratings.com> writes:
>
>
>>The problem is that in the 'real life' situation the condition is a lot
>>more complicated than this simple is null test... I hate having to
>>duplicate it, and I hate even more having to evaluate it twice on every
>>insert :-(
>>
>>
>
>Why evaluate it twice?  The DO INSTEAD NOTHING rule should be
>unconditional.
>
>
>
Right. But the problem is I don't want to discard the invalid entries
completely...
So, it would have to be *three* rules, not just two - like:

create rule skip_null as on insert to test_view where x is null do instead
insert into invalid_entries ('NULL DATA', new.*);
create rule insert_test as on insert to test_view where is is not null
do instead
insert into test values (new.*);
create rule dummy_insert as on insert to test_view do instead nothing;

... so x is null ends up being evaluated twice...

Dima

>
>
>Oh, I see what you're on about.  Sorry, a "DO INSTEAD NOTHING" only
>suppresses the original command, it does not suppress other rules.
>I think what you want is to make the insert_test rule conditional
>on x being not null.
>
>            regards, tom lane
>
>
Ok... What's wrong with this one then (three rules are conditional - if
x is null or y is null, the entry is rejected, if x is not null and y is
not null, it is inserted, the fourth rull is a dummy to prevent the
planner from complaining about not being able to insert into a view):



testdb=# create table test (x int not null, y int not null);
CREATE TABLE
testdb=# create view test_view as select * from test;
CREATE VIEW
testdb=# create rule reject_x as on insert to test_view where new.x is
null do instead
testdb=# create table test_reject (x int, y int, reason text);
CREATE TABLE
testdb=# create rule reject_x as on insert to test_view where x is null
do instead insert into test_reject values (new.*, 'NULL x');
CREATE RULE
testdb=# create rule reject_y as on insert to test_view where y is null
do instead insert into test_reject values (new.*, 'NULL y');
CREATE RULE
testdb=# create rule insert_test as on insert to test_view where x is
not null and y is not null do instead insert into test values (new.*);
CREATE RULE
testdb=# create rule insert_dummy as on insert to test_view do instead
nothing;
CREATE RULE
testdb=# insert into test values (null, null);
ERROR:  ExecInsert: Fail to add null value in not null attribute x

Now, why does this fail?
The only rule that attempts to insert anything into test has 'x is not
null and y is not null' as the qualifier.
What's wrong?

Thanks!

Dima

Dmitry Tkach <dmitry@openratings.com> writes:
> Ok... What's wrong with this one then

> testdb=# insert into test values (null, null);
> ERROR:  ExecInsert: Fail to add null value in not null attribute x

Try inserting into test_view ...

            regards, tom lane

Tom Lane wrote:

>Dmitry Tkach <dmitry@openratings.com> writes:
>
>
>>Ok... What's wrong with this one then
>>
>>
>
>
>
>>testdb=# insert into test values (null, null);
>>ERROR:  ExecInsert: Fail to add null value in not null attribute x
>>
>>
>
>Try inserting into test_view ...
>
>            regards, tom lane
>
>
Damn!
Sorry about that....

Actually, believe it or not, I do actually have a similar problem on
7.2.4...
I was just trying to reproduce it with a simple example, and screwed up
the insert statement... :-(
Sorry...

I just checked this example on 7.2, and it works fine...

But what the hell is my problem then??? I swear, I do insert into the
view there :-)
It's a really huge view, looking at a whole bunch of different tables...
I'd hate having to post the whole thing...


Dima

Dmitry Tkach <dmitry@openratings.com> writes:
> But what the hell is my problem then??? I swear, I do insert into the
> view there :-)
> It's a really huge view, looking at a whole bunch of different tables...
> I'd hate having to post the whole thing...

All I can guess is a bug (or pilot error) that's triggered by the more
complex view.  I think you'll just have to try to whittle down the
failure to something you can post.

            regards, tom lane

Aha!

I got it.
This generates the 'cannot insert null...' error:

create table test (x int not null, y int not null);
create table test_reject (x int, y int, reason text);

create view test_view as select * from test;

create rule reject_x as on insert to test_view where new.x is null do
instead insert into test_reject values (new.*, 'NULL x');

create rule reject_y as on insert to test_view where new.y is null do
instead insert into test_reject values (new.*, 'NULL y');

create rule insert_test as on insert to test_view where new.x is not
null and new.y is not null do instead
(
   insert into test
   select new.* union
   select new.*;
);

create rule insert_dummy as on insert to test_view do instead nothing;

-- insert into test_reject values (new.*,
-- case when new.x is null then 'NULL x' else 'NULL y' end);

insert into test_view values (null, null);


It looks like the UNION in the 'not null' rule is the problem.
If I change it to just insert ... select (without the union), or to two
inserts, then it works.
But union always fails, even if I add a 'where false' to the end, so
that it only returns one row...

Dima


Tom Lane wrote:

>Dmitry Tkach <dmitry@openratings.com> writes:
>
>
>>But what the hell is my problem then??? I swear, I do insert into the
>>view there :-)
>>It's a really huge view, looking at a whole bunch of different tables...
>>I'd hate having to post the whole thing...
>>
>>
>
>All I can guess is a bug (or pilot error) that's triggered by the more
>complex view.  I think you'll just have to try to whittle down the
>failure to something you can post.
>
>            regards, tom lane
>
>

Dmitry Tkach <dmitry@openratings.com> writes:
> create rule insert_test as on insert to test_view where new.x is not
> null and new.y is not null do instead
> (
>    insert into test
>    select new.* union
>    select new.*;
> );

Mmm.  In CVS tip that throws

ERROR:  UNION/INTERSECT/EXCEPT member statement may not refer to other relations of same query level

which was a check added as a result of this discussion thread:
http://archives.postgresql.org/pgsql-general/2003-02/msg00693.php

I am sure you are running into some misbehavior associated with the
fact that the rule transformation generates a bogusly-structured SQL
query, and 7.2 isn't noticing.

I'd like to support this case someday, but it's not clear how...

            regards, tom lane

Tom Lane wrote:

>Dmitry Tkach <dmitry@openratings.com> writes:
>
>
>>create rule insert_test as on insert to test_view where new.x is not
>>null and new.y is not null do instead
>>(
>>   insert into test
>>   select new.* union
>>   select new.*;
>>);
>>
>>
>
>Mmm.  In CVS tip that throws
>
>ERROR:  UNION/INTERSECT/EXCEPT member statement may not refer to other relations of same query level
>
>
Actually, I just used that new.* as an example (if I understand this
error message correctly, that's what it refers to, right?)
Something like
insert into test
select null,null union select 1,2 where false

has the same problem... and it doesn't refer to any relations.

>which was a check added as a result of this discussion thread:
>http://archives.postgresql.org/pgsql-general/2003-02/msg00693.php
>
>
I'll take a look at that thread, thanks!

>I am sure you are running into some misbehavior associated with the
>fact that the rule transformation generates a bogusly-structured SQL
>query, and 7.2 isn't noticing.
>
>
Not just 7.2... I was testing this in 7.3 - it has the same problem

Dima

>I'd like to support this case someday, but it's not clear how...
>
>
>
I don't know if it helps, but somehow if I do

insert into test select * from (select null,null union select 1,2 where
false) as dummy

... that works fine.

Thanks!

Dima

Dmitry Tkach <dmitry@openratings.com> writes:
> Something like
> insert into test
> select null,null union select 1,2 where false
> has the same problem... and it doesn't refer to any relations.

But that's parsed as

insert into test
(select null,null) union (select 1,2 where false)

so I'd expect it to bomb if test has NOT NULL constraints.

> Not just 7.2... I was testing this in 7.3 - it has the same problem

Yeah, the change is post-7.3.

> insert into test select * from (select null,null union select 1,2 where
> false) as dummy
> ... that works fine.

I get
ERROR:  ExecInsert: Fail to add null value in not null attribute x
which is what I'd expect.

            regards, tom lane

Tom Lane wrote:

>Dmitry Tkach <dmitry@openratings.com> writes:
>
>
>>Something like
>>insert into test
>>select null,null union select 1,2 where false
>>has the same problem... and it doesn't refer to any relations.
>>
>>
>
>But that's parsed as
>
>insert into test
>(select null,null) union (select 1,2 where false)
>
>so I'd expect it to bomb if test has NOT NULL constraints.
>
>
Sure, but it is inside the rule that has 'where x is not null and y is
not null' on it as a qualifier, so
with my test example it should just never get executed in the first place.

>
>
>
>>Not just 7.2... I was testing this in 7.3 - it has the same problem
>>
>>
>
>Yeah, the change is post-7.3.
>
>
>
>>insert into test select * from (select null,null union select 1,2 where
>>false) as dummy
>>... that works fine.
>>
>>
>
>I get
>ERROR:  ExecInsert: Fail to add null value in not null attribute x
>which is what I'd expect.
>
>
Really? In 7.3?
That's weird...
Here is what I am getting exactly:

testdb=# drop table test cascade;
NOTICE:  Drop cascades to rule insert_test on view test_view
NOTICE:  Drop cascades to rule _RETURN on view test_view
NOTICE:  Drop cascades to view test_view
DROP TABLE
testdb=# drop table test_reject cascade;
DROP TABLE
testdb=#
testdb=# create table test (x int not null, y int not null);
CREATE TABLE
testdb=# create table test_reject (x int, y int, reason text);
CREATE TABLE
testdb=#
testdb=# create view test_view as select * from test;
CREATE VIEW
testdb=#
testdb=# create rule reject_x as on insert to test_view where new.x is
null do instead insert into test_reject values (new.*, 'NULL x');
CREATE RULE
testdb=#
testdb=# create rule reject_y as on insert to test_view where new.y is
null do instead insert into test_reject values (new.*, 'NULL y');
CREATE RULE
testdb=#
testdb=# create rule insert_test as on insert to test_view where new.x
is not null and new.y is not null do instead
testdb-# (
testdb(#    insert into test select * from
testdb(#    (select null,null union select 1,2 where false) as dummy
testdb(# );
CREATE RULE
testdb=#
testdb=# create rule dummy_insert as on insert to test_view do instead
nothing;
CREATE RULE
testdb=#
testdb=#
testdb=# insert into test_view values (null, null);
INSERT 17648 1
testdb=# select * from test;
 x | y
---+---
(0 rows)

testdb=# select * from test_reject;
 x | y | reason
---+---+--------
   |   | NULL x
   |   | NULL y
(2 rows)

Dmitry Tkach <dmitry@openratings.com> writes:
> Sure, but it is inside the rule that has 'where x is not null and y is
> not null' on it as a qualifier, so
> with my test example it should just never get executed in the first place.

You're confusing rules with triggers.  The INSERT *will* get executed;
the rule's qualifier gets moved to the WHERE of the INSERT...SELECT,
and the way you get no effect is for the qual to fail on every row the
SELECT generates.

One way to think about the problem (though I'm not sure this is right in
detail) is that there's no place to hang a top-level WHERE on a UNION.

            regards, tom lane

Tom Lane wrote:

>Dmitry Tkach <dmitry@openratings.com> writes:
>
>
>>Sure, but it is inside the rule that has 'where x is not null and y is
>>not null' on it as a qualifier, so
>>with my test example it should just never get executed in the first place.
>>
>>
>
>You're confusing rules with triggers.  The INSERT *will* get executed;
>the rule's qualifier gets moved to the WHERE of the INSERT...SELECT,
>and the way you get no effect is for the qual to fail on every row the
>SELECT generates.
>
>One way to think about the problem (though I'm not sure this is right in
>detail) is that there's no place to hang a top-level WHERE on a UNION.
>
>
>
Ok. If so, should UNION not be disallowed entirely inside (at least
conditional) rules, regadless of whether it has those 'cross-from'
references or not?
What you are saying makes sense to me (and I have already rewritten that
rule, and it is working now)... but it's unfortunate that I had to spend
half a day trying to figure out why the damn thing doesn't work... (even
worse really - I've written that rule a while ago, and it already made
it into the production database before anyone noticed that it did not
really work) :-(
It would have saved a lot of trouble if it just complained about that
union thing right away and refuse to create the rule...

On a different note, I think there *is* a way to add a where clause to
the union - that's exactly what I did in that last example - by
converting it into a subselect...
Can that not be done automatically for conditional rules?
(I doubt that would be very useful though... since it's no longer
possible to use old and new there... I can't really think of any useful
application of a union inside a rule, except for my obscure 'select 1,2'
example :-)

Dima

I said:
> Dmitry Tkach <dmitry@openratings.com> writes:
>> I thought you said it was only complaining about references to new and
>> old, not about *any* union clause...

> I don't see a need to reject "any" union clause.  AFAIK the cases that
> don't work are just the ones where NEW or OLD are referenced.

After further study I've realized that the problem is that we have no
support for attaching qual conditions directly to a UNION node, and
thus INSERT ... SELECT ... UNION in a rule will misbehave if either
the rule's own WHERE condition or the WHERE of the invoking query is
nonempty.  This is a separate problem from the question of whether there
is a reference to OLD or NEW.

For 7.4 I've added code to reject such cases.  CVS tip will now do this,
given your example test and test_view:

regression=# create rule z2 as on update to test_view where false
regression-# do instead insert into test (select null, null union select 1,2);
ERROR:  Conditional UNION/INTERSECT/EXCEPT statements are not implemented
regression=# create rule z2 as on update to test_view
regression-# do instead insert into test (select null, null union select 1,2);
CREATE RULE
regression=# update test_view set y = 1 where x is null;
ERROR:  Conditional UNION/INTERSECT/EXCEPT statements are not implemented
regression=# update test_view set y = 1 ;
ERROR:  ExecInsert: Fail to add null value in not null attribute x

Further down the pike we should try to support the cases that can be
made to work, but that sounds too much like a new feature to me to
consider for 7.4 at this stage.

            regards, tom lane

Dmitry Tkach <dmitry@openratings.com> writes:
> It would have saved a lot of trouble if it just complained about that
> union thing right away and refuse to create the rule...

That's what happens in CVS tip.

> On a different note, I think there *is* a way to add a where clause to
> the union - that's exactly what I did in that last example - by
> converting it into a subselect...
> Can that not be done automatically for conditional rules?

Send a patch... or at least convince us it can be done ... I'm not
convinced yet.

            regards, tom lane

Dmitry Tkach <dmitry@openratings.com> writes:
> Tom Lane wrote:
>> Dmitry Tkach <dmitry@openratings.com> writes:
>>> It would have saved a lot of trouble if it just complained about that
>>> union thing right away and refuse to create the rule...
>>
>> That's what happens in CVS tip.
>>
> I thought you said it was only complaining about references to new and
> old, not about *any* union clause...

I don't see a need to reject "any" union clause.  AFAIK the cases that
don't work are just the ones where NEW or OLD are referenced.

            regards, tom lane

Tom Lane wrote:

>Dmitry Tkach <dmitry@openratings.com> writes:
>
>
>>It would have saved a lot of trouble if it just complained about that
>>union thing right away and refuse to create the rule...
>>
>>
>
>That's what happens in CVS tip.
>
>
I thought you said it was only complaining about references to new and
old, not about *any* union clause...
Did I get it wrong?

>
>
>>On a different note, I think there *is* a way to add a where clause to
>>the union - that's exactly what I did in that last example - by
>>converting it into a subselect...
>>Can that not be done automatically for conditional rules?
>>
>>
>
>Send a patch... or at least convince us it can be done ... I'm not
>convinced yet.
>
>
>
I am afraid, that's too complicated for me :-)
I tried to dig through the source a little bit when I was struggling to
figure out why the damn thing did not work, but I could not even find
the place where those qualifiers are evaluated. :-(

Besides, as I said earlier, I don't really think that such an
improvement would be of much use anyway, unless at the same time we find
away to allow referencing new and old (or at least new, which, I suspect
is much easier) from inside the union... I don't really understand the
reason why that cannot be supported (but I am sure, it's a good one)
:-), and without that I just can't think of any example where using the
union inside a rule would be useful for anything anyway, so, unless we
want to consider allowing new and old at the same time, it looks like
trying to make unions work isn't worth the effort... Just indicating
properly that they don't would be good enough


Dima

DeJuan Jackson <djackson@speedfc.com> writes:
>   Or is it simply any conditional rule using UNION/EXCEPT/INTERSECT/...?

Yeah, that's about the size of it :-(.  Note though that you could
probably work around the problem by pushing the UNION etc. down into a
sub-select:
    SELECT * FROM (SELECT ... UNION ...) foo;

At some point we could look at automatically transforming the query in
that way, but I'm not planning to worry about it now.

            regards, tom lane

Dmitry Tkach <dmitry@openratings.com> writes: 

Tom Lane wrote:   

Dmitry Tkach <dmitry@openratings.com> writes:     

It would have saved a lot of trouble if it just complained about that 
union thing right away and refuse to create the rule...       

That's what happens in CVS tip.
     

I thought you said it was only complaining about references to new and 
old, not about *any* union clause...   

I don't see a need to reject "any" union clause.  AFAIK the cases that
don't work are just the ones where NEW or OLD are referenced.
		regards, tom lane

---------------------------(end of broadcast)---------------------------
TIP 1: subscribe and unsubscribe commands go to majordomo@postgresql.org 

Tom Lane wrote:
> DeJuan Jackson <djackson@speedfc.com> writes:
> >   Or is it simply any conditional rule using UNION/EXCEPT/INTERSECT/...?
>
> Yeah, that's about the size of it :-(.  Note though that you could
> probably work around the problem by pushing the UNION etc. down into a
> sub-select:
>     SELECT * FROM (SELECT ... UNION ...) foo;
>
> At some point we could look at automatically transforming the query in
> that way, but I'm not planning to worry about it now.

What does the code do now after you changed it?  Fail on the UNION
query?

--
  Bruce Momjian                        |  http://candle.pha.pa.us
  pgman@candle.pha.pa.us               |  (610) 359-1001
  +  If your life is a hard drive,     |  13 Roberts Road
  +  Christ can be your backup.        |  Newtown Square, Pennsylvania 19073

Bruce Momjian <pgman@candle.pha.pa.us> writes:
> What does the code do now after you changed it?  Fail on the UNION
> query?

Yeah.  You get something about "union can't be conditionally executed"
(I forget the exact wording, but it's comparable to what we say about
NOTIFY under similar circumstances).

            regards, tom lane