Re: WIP fix proposal for bug #6123

On Aug1, 2011, at 20:02 , Kevin Grittner wrote:
> "Kevin Grittner" <Kevin(dot)Grittner(at)wicourts(dot)gov> wrote:
> I consider the trigger behavior addressed by this patch to be a bug,
> and serious enough to merit inclusion of a fix in the 9.1 release,
> even at this late stage.

I'm sorry but I disagree, on multiple grounds.

First, I'm not sure this is even a bug. To me, it seems that postgres
currently resolves an inherently ambiguous situation in one possible
way, while you expect it to pick another. It might be that the behaviour
that you suggest is more in line with people's expectations (More on
that below), but that still doesn't make the existing behaviour wrong.

Secondly, even if one considers the current behaviour to be wrong,
that still doesn't prove that your proposed behaviour is any better
(More on that below too). There might very well be situations in which
the current behaviour is preferable over the behaviour you suggest,
and maybe these situations turn out to be much more common than anyone
anticipates right now. But if we apply the patch now, the chance of that
being discovered before 9.1 is released is virtually zero. And after
the release there's no going back, so we might end up changing the
behaviour to the worse.

And lastly, I believe that your ultimate goal of guaranteeing that
a row is actually deleted (updated) after a BEFORE DELETE (BEFORE UPDATE)
trigger has fired is actually insurmountable anyway, at least for
isolation level READ COMMITTED. We don't seem to lock rows before firing
BEFORE DELETE or BEFORE UPDATE triggers, so a row may very well be
updated by a concurrent transaction between the firing of these triggers
and the actual DELETE or UPDATE. In READ COMMITTED mode, we'll then
re-check if the original WHERE condition still applies (using the
EvalPlanQual machinery), and only go forward with the modification
if it does.

Having said all that, I don't doubt that the current behaviour is
source of pain for you, and I do believe that we ought to improve things -
but not by replacing one unsatisfactory behaviour with another. The
root of the issue seems to be that you're trying to do things in a
BEFORE trigger that really belong into an AFTER trigger. If you could
explain why using an AFTER trigger doesn't work for you, maybe we could
improve them to be suitable for your purposes (and please forgive me
if you already did that).

> To recap: a trigger for UPDATE BEFORE EACH ROW or DELETE BEFORE EACH
> ROW, which directly or indirectly causes update of the OLD row in
> the trigger, can cause the triggering operation to be silently
> ignored even though the trigger returns a non-NULL record, and even
> though all triggered modifications are persisted. The only clue
> that an incomplete and incongruous set of operations has been
> performed is that the UPDATE or DELETE count from the statement is
> reduced by the number of rows on which this occurred: if an UPDATE
> would have affected 5 rows, but one of them just fired triggers *as
> though* it had been updated but was actually left untouched by the
> original UPDATE statement, you would get "UPDATE 4" rather than
> "UPDATE 5".
>
> This patch causes DELETE to behave as most people would expect, and
> throws and ERROR on the conflicting UPDATE case.

I'm prepared to argue that whether or not people expect the behaviour
that patch implements depends entirely on how you phrase the question.
If you ask "do you expect a row to be actually deleted after BEFORE
DELETE triggers have run", you'll undoubtedly hear "yes". But if you
ask "do you expect a row to be deleted even though it didn't match
the DELETE's WHERE condition" I bet you'll hear a very firm "no". And
equally firm do I expect to be the "no" if you ask "do you expect the
row that is actually deleted and the contents of the variable OLD in
the delete trigger to differ".

But the behaviour indicated in the second and third question *is* what
happens with your patch applied. Here's an example

create table t (id int);
create or replace function on_t_delete() returns trigger as $$
begin
raise notice 'deleting row %', old.id;
update t set id = -id where id = old.id;
return old;
end;
$$ language plpgsql volatile;
create trigger t_delete
before delete on t
for each row execute procedure on_t_delete();

insert into t (id) values (1);
insert into t (id) values (2);

delete from t where id > 0;

Without your patch, the DELETE doesn't remove any rows, because
they're updated in on_t_delete(). With your patch applied, the
rows are removed - even though, due to the UPDATE in on_t_delete(),
they *don't* match the DELETE's WHERE condition (id > 0) at the time
they're deleted.

best regards,
Florian Pflug