Обсуждение: 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