Re: Remembering bug #6123

"Kevin Grittner" <Kevin(dot)Grittner(at)wicourts(dot)gov> writes:
> You were right. One of the isolation tests failed an assertion;
> things pass with the attached change, setting the cmax
> conditionally. Some comments updated. I hope this is helpful.

I worked over this patch a bit, mostly cosmetically; updated version
attached. However, we're not there yet. I have a couple of remaining
concerns:

1. I think the error message needs more thought. I believe it's
possible to trigger this error not only with BEFORE triggers, but also
with a volatile function used in the query that reaches around and
updates row(s) of the target table. Now, I don't have the slightest
qualms about breaking any apps that do anything so dirty, but should
we try to generalize the message text to cope with that?

2. The HINT needs work too, as it seems pretty useless as it stands.
I'd have expected some short reference to the technique of re-executing
the update in the trigger and then returning NULL. (BTW, does this
patch require any documentation changes, and if so where?)

3. I modified heap_lock_tuple to also return a HeapUpdateFailureData,
principally on the grounds that its API should be largely parallel to
heap_update, but having done that I can't help wondering whether its
callers are okay with skipping self-updated tuples. I see that you
changed EvalPlanQualFetch, but I'm not entirely sure that that is right;
shouldn't it continue to ignore rows modified by the current command
itself? And you did not touch the other two callers, which definitely
have got issues. Here is an example, which is basically the reference
count case refactored into a single self-referencing table, so that we
can hit both parent and child rows in one operation:

create table test (
id int primary key,
parent int references test,
data text,
nchildren int not null default 0
);

create function test_ins_func()
returns trigger language plpgsql as
$$
begin
if new.parent is not null then
update test set nchildren = nchildren + 1 where id = new.parent;
end if;
return new;
end;
$$;
create trigger test_ins_trig before insert on test
for each row execute procedure test_ins_func();

create function test_del_func()
returns trigger language plpgsql as
$$
begin
if old.parent is not null then
update test set nchildren = nchildren - 1 where id = old.parent;
end if;
return old;
end;
$$;
create trigger test_del_trig before delete on test
for each row execute procedure test_del_func();

insert into test values (1, null, 'root');
insert into test values (2, 1, 'root child A');
insert into test values (3, 1, 'root child B');
insert into test values (4, 2, 'grandchild 1');
insert into test values (5, 3, 'grandchild 2');

update test set data = 'root!' where id = 1;

select * from test;

delete from test;

select * from test;

drop table test;
drop function test_ins_func();
drop function test_del_func();

When you run this, with or without the current patch, you get:

id | parent | data | nchildren
----+--------+--------------+-----------
2 | 1 | root child A | 1
4 | 2 | grandchild 1 | 0
3 | 1 | root child B | 1
5 | 3 | grandchild 2 | 0
1 | | root! | 2
(5 rows)

DELETE 4
id | parent | data | nchildren
----+--------+-------+-----------
1 | | root! | 0
(1 row)

the reason being that when the outer delete arrives at the last (root!)
row, GetTupleForTrigger fails because the tuple is already self-updated,
and we treat that as canceling the outer delete action.

I'm not sure what to do about this. If we throw an error there, there
will be no way that the trigger can override the error because it will
never get to run. Possibly we could plow ahead with the expectation
of throwing an error later if the trigger doesn't cancel the
update/delete, but is it safe to do so if we don't hold lock on the
tuple? In any case that idea doesn't help with the remaining caller,
ExecLockRows.

regards, tom lane