Thread: Triggered Data Change check

I wanted to know if there was a decision
to remove the triggered data change violation checks
from trigger.c or to change them to a per statement
check?  I'm building a fix for some foreign key
problems, and want to cover some of those cases
in the regression test if we're going to allow it.

Stephan Szabo <sszabo@megazone23.bigpanda.com> writes:
>     I wanted to know if there was a decision
> to remove the triggered data change violation checks
> from trigger.c or to change them to a per statement
> check?

AFAIK no one is happy with the state of that code, but I'm not sure
if we've agreed on exactly how to change it.  Do you have a proposal?
        regards, tom lane

> 
>     I wanted to know if there was a decision
> to remove the triggered data change violation checks
> from trigger.c or to change them to a per statement
> check?  I'm building a fix for some foreign key
> problems, and want to cover some of those cases
> in the regression test if we're going to allow it.

I would like to do _something_ about that error message, not sure what.

--  Bruce Momjian                        |  http://candle.pha.pa.us pgman@candle.pha.pa.us               |  (610)
853-3000+  If your life is a hard drive,     |  830 Blythe Avenue +  Christ can be your backup.        |  Drexel Hill,
Pennsylvania19026
 

On Sun, 11 Nov 2001, Tom Lane wrote:

> Stephan Szabo <sszabo@megazone23.bigpanda.com> writes:
> >     I wanted to know if there was a decision
> > to remove the triggered data change violation checks
> > from trigger.c or to change them to a per statement
> > check?
>
> AFAIK no one is happy with the state of that code, but I'm not sure
> if we've agreed on exactly how to change it.  Do you have a proposal?

Well, I wonder if the check is so weak as to be fairly useless in the
first place really, even if applied to the statement as opposed to the
transaction.  It prevents cases like (not tested, but gives the idea):

create table foo1( a int unique);
create table foo2( a int unique default 2 references foo1(a)initially deferred on update set default);
alter table foo1 add foreign key(a) references foo2(a)initially deferred on update cascade);
begin;
insert into foo1 values (1);
insert into foo2 values (1);
end;
update foo1 set a=3;
-- I think it would have the following effect:
-- foo1 has "a" set to 3 which sets foo2's "a" to 2 which sets
-- foo1's "a" to 2 as well. And so the row in foo1 is changed
-- twice.

But, since you could do alot of this same work in your own triggers,
and doing so doesn't seem to trip the triggered data change check
(for example an after trigger on a non-fk table that updates the
same table), I wonder if we should either defend against neither
case or both.

As such, I'd say we should at least comment out the check and
error since it would fix alot of cases that people using the
system more normally run into at the expense of a more edge
condition.

One problem is that it opens up the foreign key stuff to a
bunch of cases that haven't been tested before and it may be
a little bit late for opening up that can of worms. I'm confident
that I've fixed related badness in the no action case and on
the base check(since my home copy had the check commented out and the
tests I ran worked in that case), but I haven't touched the referential
actions because they're a little more complicated.

Stephan Szabo <sszabo@megazone23.bigpanda.com> writes:
> Well, I wonder if the check is so weak as to be fairly useless in the
> first place really, even if applied to the statement as opposed to the
> transaction.

Looking back at our discussion around 24-Oct, I recall that I was
leaning to the idea that the correct interpretation of the spec's
"triggered data change" rule is that it prohibits scenarios that are
impossible anyway under MVCC, because of the MVCC tuple visibility
rules.  Therefore we don't need any explicit test for triggered data
change.  But I didn't hear anyone else supporting or disproving
that idea.

The code as-is is certainly wrong, since it prohibits multiple changes
within a transaction, not within a statement as the spec says.

Right at the moment I'd favor ripping the code out entirely ... but
it'd be good to hear some support for that approach.  Comments anyone?
        regards, tom lane

Tom Lane wrote:
> 
> Stephan Szabo <sszabo@megazone23.bigpanda.com> writes:
> > Well, I wonder if the check is so weak as to be fairly useless in the
> > first place really, even if applied to the statement as opposed to the
> > transaction.
> 
> Looking back at our discussion around 24-Oct, I recall that I was
> leaning to the idea that the correct interpretation of the spec's
> "triggered data change" rule is that it prohibits scenarios that are
> impossible anyway under MVCC, because of the MVCC tuple visibility
> rules. 

Strictly speaking MVCC is only for read-only queries.
Even under MVCC, update, delete and select .. for update have
to see the newest tuples. Constraints shouldn't ignore the
update/delete operations in the future from MVCC POV.

regards,
Hiroshi Inoue

Hiroshi Inoue <Inoue@tpf.co.jp> writes:
> Strictly speaking MVCC is only for read-only queries.
> Even under MVCC, update, delete and select .. for update have
> to see the newest tuples.

True.  But my point is that we already have mechanisms to deal with
that set of issues; the trigger code shouldn't concern itself with
the problem.
        regards, tom lane

Tom Lane wrote:
> 
> Hiroshi Inoue <Inoue@tpf.co.jp> writes:
> > Strictly speaking MVCC is only for read-only queries.
> > Even under MVCC, update, delete and select .. for update have
> > to see the newest tuples.
> 
> True.  But my point is that we already have mechanisms to deal with
> that set of issues; the trigger code shouldn't concern itself with
> the problem.

You are saying 
> Therefore we don't need any explicit test for triggered data
> change. 

ISTM your point is on the following.

> Functions can run new commands that get new command ID numbers within
> the current transaction --- but on return from the function, the current
> command number is restored.  I believe rows inserted by such a function
> would look "in the future" to us at the outer command, and would be
> ignored.

My point is why we could ignore the (future) changes. 

regards,
Hiroshi Inoue

On Sun, 11 Nov 2001, Tom Lane wrote:

> Hiroshi Inoue <Inoue@tpf.co.jp> writes:
> > Strictly speaking MVCC is only for read-only queries.
> > Even under MVCC, update, delete and select .. for update have
> > to see the newest tuples.
>
> True.  But my point is that we already have mechanisms to deal with
> that set of issues; the trigger code shouldn't concern itself with
> the problem.

This sequence on my system prints the numbers increasing by 1 which
I would assume means that the updates are going through:

create table foo1(a int);
create function f() returns opaque as 'begin update foo1 set a=a+1; raisenotice ''%'', NEW.a; return NEW; end;'
language'plpgsql';
 
create trigger tr after update on foo1 for each row executeprocedure f();
insert into foo1 values(1);
update foo1 set a=1;

I think that if this were an fk trigger, this would technically be illegal
behavior as soon as that row in foo1 was modified again during the
function execution from the "update foo1 set a=1" statement due to the
following (sql92, 11.8 General Rules -- I don't have the copy of sql99
on this machine to look at, but I'm guessing there's something similar)        7) If any attempt is made within an
SQL-statementto update some           data item to a value that is distinct from the value to which           that data
itemwas previously updated within the same SQL-           statement, then an exception condition is raised: triggered
       data change violation.
 
Given this is under the referential constraint definition, I'm guessing
it's about ri constraints even though the wording seems to say any
attempt.

Because its easy to get around with general triggers, I'm not sure the
check is meaningful, and it's alot less likely to occur than the normal
update/delete or update/update cases that currently error out in the
system, so I'm also for ripping out the check, although I think we
probably want to think about this for later.

Hiroshi Inoue <Inoue@tpf.co.jp> writes:
> My point is why we could ignore the (future) changes. 

We shouldn't.  My feeling is that the various places that consider
HeapTupleSelfUpdated to be an ignorable condition need more thought.
In some cases they should be raising a "data change violation" error,
instead.

It's still not special to triggers, however.  If you read the spec
closely, it's talking about any update not only trigger-caused updates:
        7) If any attempt is made within an SQL-statement to update some           data item to a value that is
distinctfrom the value to which           that data item was previously updated within the same SQL-
statement,then an exception condition is raised: triggered           data change violation.
 

It might be that a trigger is the only possible way to make that happen
within SQL92, but we have more ways to make it happen...
        regards, tom lane

 
> Looking back at our discussion around 24-Oct, I recall that I was
> leaning to the idea that the correct interpretation of the spec's
> "triggered data change" rule is that it prohibits scenarios that are
> impossible anyway under MVCC, because of the MVCC tuple visibility
> rules.  Therefore we don't need any explicit test for triggered data
> change.  But I didn't hear anyone else supporting or disproving
> that idea.
> 
> The code as-is is certainly wrong, since it prohibits multiple changes
> within a transaction, not within a statement as the spec says.
> 
> Right at the moment I'd favor ripping the code out entirely ... but
> it'd be good to hear some support for that approach.  Comments anyone?

If I read the code correctly, the "triggered data change" check is only
performed for keys, that have xmin == GetCurrentTransactionId().
Those are tuples that have already been modified by current session.
Since nobody else can touch those (since they are locked), I think the
check is not needed.

(Delete lines 2176 - 2200 and 2211 - 2229, that was your intent, Tom ?)
I think this would be correct.

I somehow wonder on the contrary why a check would not be necessary
for the exact opposite case, where oldtup->t_data->t_xmin != 
GetCurrentTransactionId(), since such a key might have been changed 
from another session. (Or does a referenced key always get a lock ?)

Andreas