Thread: Problem with trigger makes Detail record be invalid

I´m using Postgres 10 on ubuntu.

suppose a simple Master/Detail structure like this:

create table Master(ID integer primary key, name text);
create table Detail(ID integer primary key, Master_ID Integer, OtherInfo
text);
alter table Detail add constraint FKMasterDetail foreign key (Master_ID)
references Master(ID) on update cascade on delete cascade;

Then insert some records on it:
insert into Master(ID, Name) values(1,'First'), values(2,'Second');
insert into Detail(ID, Master_ID, OtherInfo) values(1,1,'Detail
Information'), (2,2,'Detail Information2');

Then, if I delete on Master will delete on detail too. Fine.
delete from Master where ID=1;

But now, suppose I have a complex trigger before update or delete that runs
on Detail table.
create function DoAComplexJobOnDetail() returns trigger as $$
begin
  -- Do lots of things then
  Return new; --This is the error, because I´m returning new even for
delete;
end;$$ language plpgsql;
create trigger DetailDoAComplexJob before update or delete on Detail for
each row execute procedure DoAComplexJobOnDetail();

Then try to delete the other Master record. It will be deleted on Master but
Detail record doesn´t and will obviously become invalid because the foreign
key.
delete from Master where ID=2;
select * from Master; --will show no records.
select * from Detail; --will show one record pointing to Master_ID=2, that
doesn´t exist anymore.

Is this a bug or it´s mine responsability to check that trigger result ?
If that trigger responds incorrectly I think that no information could be
executed.



--
Sent from: http://www.postgresql-archive.org/PostgreSQL-general-f1843780.html

On 04/19/2018 10:55 AM, PegoraroF10 wrote:
> I´m using Postgres 10 on ubuntu.
> 
> suppose a simple Master/Detail structure like this:
> 
> create table Master(ID integer primary key, name text);
> create table Detail(ID integer primary key, Master_ID Integer, OtherInfo
> text);
> alter table Detail add constraint FKMasterDetail foreign key (Master_ID)
> references Master(ID) on update cascade on delete cascade;
> 
> Then insert some records on it:
> insert into Master(ID, Name) values(1,'First'), values(2,'Second');
> insert into Detail(ID, Master_ID, OtherInfo) values(1,1,'Detail
> Information'), (2,2,'Detail Information2');
> 
> Then, if I delete on Master will delete on detail too. Fine.
> delete from Master where ID=1;
> 
> But now, suppose I have a complex trigger before update or delete that runs
> on Detail table.
> create function DoAComplexJobOnDetail() returns trigger as $$
> begin
>    -- Do lots of things then
>    Return new; --This is the error, because I´m returning new even for
> delete;

That can be dealt with using TG_OP value to conditionally change what is 
RETURNed:

https://www.postgresql.org/docs/10/static/plpgsql-trigger.html

"TG_OP

     Data type text; a string of INSERT, UPDATE, DELETE, or TRUNCATE 
telling for which operation the trigger was fired.
"

See Example 42.4. A PL/pgSQL Trigger Procedure For Auditing at bottom of 
page.

> end;$$ language plpgsql;
> create trigger DetailDoAComplexJob before update or delete on Detail for
> each row execute procedure DoAComplexJobOnDetail();
> 
> Then try to delete the other Master record. It will be deleted on Master but
> Detail record doesn´t and will obviously become invalid because the foreign
> key.
> delete from Master where ID=2;
> select * from Master; --will show no records.
> select * from Detail; --will show one record pointing to Master_ID=2, that
> doesn´t exist anymore.
> 
> Is this a bug or it´s mine responsability to check that trigger result ?

Without seeing exactly what the trigger function on Detail is doing that 
is not answerable.

> If that trigger responds incorrectly I think that no information could be
> executed.
> 
> 
> 
> --
> Sent from: http://www.postgresql-archive.org/PostgreSQL-general-f1843780.html
> 


-- 
Adrian Klaver
adrian.klaver@aklaver.com

I know my trigger is incorrect. I know that I can use TG_OP to know what
operation is being done.
My question is ...
> Is this a bug or it´s mine responsability to check that trigger result ?

I think it´s a bug because if something got wrong on detail deletion and it
was rolled back, how could be a parent record be deleted ?



--
Sent from: http://www.postgresql-archive.org/PostgreSQL-general-f1843780.html

On 04/19/2018 11:30 AM, PegoraroF10 wrote:
> I know my trigger is incorrect. I know that I can use TG_OP to know what
> operation is being done.
> My question is ...
>> Is this a bug or it´s mine responsability to check that trigger result ?
> 
> I think it´s a bug because if something got wrong on detail deletion and it
> was rolled back, how could be a parent record be deleted ?

In your example I saw no rollback or error message:

"delete from Master where ID=2;
select * from Master; --will show no records.
select * from Detail; --will show one record pointing to Master_ID=2, that
doesn´t exist anymore."

Was there an error message?

Then there is the fact that your trigger is doing something to the row 
BEFORE the delete or update and presumably modifying it. Without knowing 
what the function is doing or what it is actually returning then we are 
in full on guessing mode.

> 
> 
> 
> --
> Sent from: http://www.postgresql-archive.org/PostgreSQL-general-f1843780.html
> 


-- 
Adrian Klaver
adrian.klaver@aklaver.com

On 04/19/2018 11:30 AM, PegoraroF10 wrote:
> I know my trigger is incorrect. I know that I can use TG_OP to know what
> operation is being done.
> My question is ...
>> Is this a bug or it´s mine responsability to check that trigger result ?
> 
> I think it´s a bug because if something got wrong on detail deletion and it
> was rolled back, how could be a parent record be deleted ?

Another thought, are there are any other triggers on the Master and/or 
Detail tables?

> 
> 
> 
> --
> Sent from: http://www.postgresql-archive.org/PostgreSQL-general-f1843780.html
> 


-- 
Adrian Klaver
adrian.klaver@aklaver.com

My point of view that there was a partial rollback, just on detail table. If
I´ve done a delete from Master and I have a foreign key to it with cascade
option, or all records should be deleted or no one should, this is my point.

Did you see that Master table has no records and Detail table has one record
?
I think you agree with me that we have a a detail record with no master, so
it´s unusable, right ?

I´m not able to do a backup/restore of this database because that record
doesn´t match.



--
Sent from: http://www.postgresql-archive.org/PostgreSQL-general-f1843780.html

Adrian Klaver <adrian.klaver@aklaver.com> writes:
> On 04/19/2018 10:55 AM, PegoraroF10 wrote:
>> Is this a bug or it´s mine responsability to check that trigger result ?

> Without seeing exactly what the trigger function on Detail is doing that 
> is not answerable.

I think the OP is complaining because his misimplemented trigger can break
the consistency of the foreign key constraint.  That is not a bug, it's
an intentional design decision: triggers are lower-level than foreign key
enforcement queries, and fire during such queries.  It's easy to construct
examples where people would be very unhappy if this were not so, because
then FK-driven updates would not be seen by the table's triggers.  It does
mean that you have to be careful when writing a trigger.

(I'm not sure that this issue is adequately documented, though.
I'd have expected to find something about it in triggers.sgml and/or
create_trigger.sgml, but in a quick look neither of them mentions foreign
keys.)

            regards, tom lane

On 04/19/2018 11:52 AM, PegoraroF10 wrote:
> My point of view that there was a partial rollback, just on detail table. If
> I´ve done a delete from Master and I have a foreign key to it with cascade
> option, or all records should be deleted or no one should, this is my point.

Except you now have a trigger or possibly triggers that are altering the 
delete process and possibly counteracting the system trigger that is a 
FK. There have been enough instances of this show up on this list for me 
to know this is a distinct possibility.

> 
> Did you see that Master table has no records and Detail table has one record
> ?
> I think you agree with me that we have a a detail record with no master, so
> it´s unusable, right ?

We have not seen the actual records, so I cannot say or agree/disagree.

> 
> I´m not able to do a backup/restore of this database because that record
> doesn´t match.
> 
> 
> 
> --
> Sent from: http://www.postgresql-archive.org/PostgreSQL-general-f1843780.html
> 


-- 
Adrian Klaver
adrian.klaver@aklaver.com

=?UTF-8?Q?Fabr=C3=ADzio_de_Royes_Mello?= <fabrizio@timbira.com.br> writes:
> 2018-04-19 15:57 GMT-03:00 Tom Lane <tgl@sss.pgh.pa.us>:
>> (I'm not sure that this issue is adequately documented, though.
>> I'd have expected to find something about it in triggers.sgml and/or
>> create_trigger.sgml, but in a quick look neither of them mentions foreign
>> keys.)

> We don't have it properly documented... at the time I answered this
> question on pt-br stackoverflow I noticed the lack o documentation and
> unfortunately I completely forgot to propose a small patch for it.

It strikes me that there are basically two things a trigger could do to
break FK consistency:

1. Turn an FK-commanded update into a no-op by returning NULL.

2. Change the content of the FK-related columns during an FK-commanded
update.

Both of these apply only to BEFORE ROW triggers, of course.

It might not be unreasonable or unduly expensive to throw an error for
case #1.  I don't think I want to get into the expense of checking
for case #2, but covering case #1 would be enough to catch all of the
reports of this type of problem that I can remember.

IIRC, you can also break FK consistency with poorly-thought-out rules,
but given that rules are close-to-deprecated, I'm not very concerned
about sanding down rough edges in that case.

(But if you feel like writing a documentation patch, please do, because
we'd not be likely to back-patch a behavioral change like this even
if we get around to making it.)

            regards, tom lane

Correct, that delete done a partial commit. And this is absolutely
unacceptable.
I know I did that trigger incorrectly but referential integrity is
obligatory.
Imagine if I have a database crash and need to restore as soon as possible.
How much time I´ll spend removing those records from a backup to get entire
database restored properly.

Well, I´ll review all my triggers. And I have hundreds of them.



--
Sent from: http://www.postgresql-archive.org/PostgreSQL-general-f1843780.html

On 04/19/2018 06:49 PM, PegoraroF10 wrote:
> Correct, that delete done a partial commit. And this is absolutely
> unacceptable.

Yet a known possible outcome. See the section on Triggers towards bottom 
of page:

https://en.wikipedia.org/wiki/Foreign_key

> I know I did that trigger incorrectly but referential integrity is
> obligatory.

I would agree if the FK relationship was entirely driven by the system 
trigger e.g:

alter table Detail add constraint FKMasterDetail foreign key (Master_ID)
references Master(ID) on update cascade on delete cascade;

As soon as you added your UPDATE/DELETE trigger you took on 
responsibility for how the data was passed around. I understand that 
this was not communicated as well as it should be in the docs.

> Imagine if I have a database crash and need to restore as soon as possible.
> How much time I´ll spend removing those records from a backup to get entire
> database restored properly.

Myself, having written more then my fair share of poorly thought out 
trigger  functions, I test new ones extensively before I release them 
into the wild.

> 
> Well, I´ll review all my triggers. And I have hundreds of them.
> 
> 
> 



-- 
Adrian Klaver
adrian.klaver@aklaver.com

On 04/20/2018 07:21 AM, David G. Johnston wrote:
> On 04/19/2018 06:49 PM, PegoraroF10 wrote:
> On Fri, Apr 20, 2018 at 6:55 AM, Adrian Klaver 
> <adrian.klaver@aklaver.com <mailto:adrian.klaver@aklaver.com>>wrote:
> 
>         I know I did that trigger incorrectly but referential integrity is
>         obligatory.
> 
> 
>     I would agree if the FK relationship was entirely driven by the
>     system trigger e.g:
> 
>     alter table Detail add constraint FKMasterDetail foreign key (Master_ID)
>     references Master(ID) on update cascade on delete cascade;
> 
>     As soon as you added your UPDATE/DELETE trigger you took on
>     responsibility for how the data was passed around.
> 
> 
> Such responsibility is an artifact of our specific implementation and 
> not an inherent property of writing triggers in the presence of FK 
> constraints.

https://en.wikipedia.org/wiki/Foreign_key#Triggers

> 
> We've left a foot-gun laying around and should not be surprised when 
> less experienced users pick it up and shoot themselves in the foot.
> 
> IOW, I do agree with the OP - its just an unfortunate reality that this 
> isn't how things work today.  Whether one can accept and work within 
> this reality is a personal decision.
> 
> This does reinforce that testing the restoration of ones backups is 
> important.
> 
> David J.
> 


-- 
Adrian Klaver
adrian.klaver@aklaver.com

Well, talking about responsabilities, I think one of responsabilities of a
mature database is that it can only accept data it was configured for. If
you try to store a timestamp in a integer field or a huge numeric value in a
smallint field, Postgres will block you because that operation is not
acceptable. 
So, it's not acceptable to break referential integrity, is it ?



--
Sent from: http://www.postgresql-archive.org/PostgreSQL-general-f1843780.html

On 04/20/2018 01:30 PM, PegoraroF10 wrote:
> Well, talking about responsabilities, I think one of responsabilities of a
> mature database is that it can only accept data it was configured for. If
> you try to store a timestamp in a integer field or a huge numeric value in a

Actually there have been examples on this list where folks have stored a 
timestamp as seconds from an epoch in an integer field. Of course then 
someone has to know what that field really represents. This is not nit 
picking on my part so much as an example of end user inventiveness. To 
that end Postgres has many ways of coming to a solution for a problem. 
Unfortunately, there are paths to a solution can trip you up. This means 
there is often no simple answer to a problem. Basically, more choices 
means more pre-thinking, testing, re-thinking, repeat as needed.

> smallint field, Postgres will block you because that operation is not
> acceptable.
> So, it's not acceptable to break referential integrity, is it ?

That is a maybe:

https://www.postgresql.org/docs/10/static/sql-altertable.html

"
DISABLE TRIGGER [ trigger_name | ALL | USER ]

ALL

     Disable or enable all triggers belonging to the table. (This 
requires superuser privilege if any of the triggers are internally 
generated constraint triggers such as those that are used to implement 
foreign key constraints or deferrable uniqueness and exclusion constraints.)


ADD table_constraint [ NOT VALID ]

     This form adds a new constraint to a table using the same syntax as 
CREATE TABLE, plus the option NOT VALID, which is currently only allowed 
for foreign key and CHECK constraints. If the constraint is marked NOT 
VALID, the potentially-lengthy initial check to verify that all rows in 
the table satisfy the constraint is skipped. The constraint will still 
be enforced against subsequent inserts or updates (that is, they'll fail 
unless there is a matching row in the referenced table, in the case of 
foreign keys; and they'll fail unless the new row matches the specified 
check constraints). But the database will not assume that the constraint 
holds for all rows in the table, until it is validated by using the 
VALIDATE CONSTRAINT option.
"

https://www.postgresql.org/docs/10/static/sql-createtrigger.html

"SQL specifies that BEFORE DELETE triggers on cascaded deletes fire 
after the cascaded DELETE completes. The PostgreSQL behavior is for 
BEFORE DELETE to always fire before the delete action, even a cascading 
one. This is considered more consistent. There is also nonstandard 
behavior if BEFORE triggers modify rows or prevent updates during an 
update that is caused by a referential action. This can lead to 
constraint violations or stored data that does not honor the referential 
constraint."

https://www.postgresql.org/docs/10/static/plpgsql-trigger.html

"Row-level triggers fired BEFORE can return null to signal the trigger 
manager to skip the rest of the operation for this row (i.e., subsequent 
triggers are not fired, and the INSERT/UPDATE/DELETE does not occur for 
this row). "

A certain amount of this came about because folks are dealing with messy 
data and want to get it into the database first, do the clean up there 
and then apply the RI constraints. Other folks have different ways of 
doing it. It comes done to personal choice. That means though you have 
to know that dangerous paths exist and how to avoid them. A lot of this 
is ingrained in the code and in use in the wild, so I would not expect 
there would be major changes in how things work. Instead as has already 
been indicated there maybe better documentation on the way detailing all 
the above.

> 
> 
> 
> --
> Sent from: http://www.postgresql-archive.org/PostgreSQL-general-f1843780.html
> 
> 


-- 
Adrian Klaver
adrian.klaver@aklaver.com