Subject: Does 'instead of delete' trigger support modification of OLD

From: Eugen Konkov <[email-masked]>
Date: Tue, 29 Oct 2019 17:54:36 +0200

Hi.

This is not clear from doc, so I have asked on IRC too.

from the DOC: https://www.postgresql.org/docs/current/trigger-definition.html
In the case of  INSTEAD OF triggers, the possibly-modified row returned by each trigger becomes the input to the next trigger

I modify OLD row, thus I expect to get modified version when run next query: 

    WITH t1 AS( delete from abc returning *)
    select * from t1;

fiddle: https://dbfiddle.uk/?rdbms=postgres_12&fiddle=637730305f66bf531794edb09a462c95

> https://www.postgresql.org/docs/current/trigger-definition.html
A row-level INSTEAD OF trigger should either return NULL to indicate that it did not modify any data from the view's underlying base tables,
or it should return the view row that was passed in (the NEW row for INSERT and UPDATE operations, or the OLD row for DELETE operations).
A nonnull return value is used to signal that the trigger performed the necessary data modifications in the view.
This will cause the count of the number of rows affected by the command to be incremented. For INSERT and UPDATE operations, the trigger may
modify the NEW row before returning it. This will change the data returned by INSERT RETURNING or UPDATE RETURNING,
and is useful when the view will not show exactly the same data that was provided.

But I still does not understand. Doc explicitly do not prohibit modification of OLD and has no examples for DELETE RETURNING case

So I want to ask clarify doc a bit.
If this prohibited, why this is prohibited? have any discussion on this?
If not prohibited, does this is not implemented for DELETE RETURNING queries? if so, is it left for later?

I have next use case.
I am implementing Bi-Temporal tables. The table have columns: id, app_period, value
for example I have next data: 7, '[2019-01-01, 2020-01-01)', 130
You can imagine this as having value 7 for each day of the year.
Now I want to delete this value for May month. I setup special variable to period: '[2019-05-01,2019-06-01)' and then delete:

    select app_period( '[2019-05-01,2019-06-01)' );
    WITH t1 AS( delete from abc returning *)
    select * from t1;

Algorithm of deletion is next:
1. Deactivate target row
   7, '[2019-01-01, 2020-01-01)', 130
2. If target row has wider app_period then we insert record that data back:
      NOT '[2019-05-01,2019-06-01)' @> '[2019-01-01, 2020-01-01)'
    INSERT INTO abc ( id, app_period, value ) values 
        ( 7, '[2019-01-01,2019-05-01)', 130 ),
        ( 7, '[2019-06-01,2020-01-01)', 130 ),
3. OLD.app_period = OLD.app_period * app_period(); 
    '[2019-01-01, 2020-01-01)' * '[2019-05-01,2019-06-01)' --> '[2019-05-01,2019-06-01)'

Because only 130 value is deleted from specified period I expect next result for the query above:
     ( 7, '[2019-05-01,2019-06-01)', 130 )

But despite on OLD was modified, actual result is:
     ( 7, '[2019-01-01,2020-01-01)', 130 )
You can see that this is original data.

So, does INSTEAD OF DELETE support modification of row?

-- 
Best regards,
Eugen Konkov

---

From: Bruce Momjian <[email-masked]>
Date: Wed, 6 Nov 2019 13:59:35 -0500

> fiddle: https://dbfiddle.uk/?rdbms=postgres_12&fiddle=637730305f66bf531794edb09a462c95

Wow, that is a very nice way to present the queries.

> But I still does not understand. Doc explicitly do not prohibit modification of OLD and has no examples for DELETE RETURNING case

I looked in the CREATE TRIGGER manual page and found this:

	https://www.postgresql.org/docs/12/sql-createtrigger.html
	If the trigger fires before or instead of the event, the trigger
	can skip the operation for the current row, or change the row
	being inserted (for INSERT and UPDATE operations only).

I don't see the "(for INSERT and UPDATE operations only)" language in
the main trigger documentation,
https://www.postgresql.org/docs/current/trigger-definition.html.  I have
written the attached patch to fix that.  Does that help?

As far as allowing DELETE to modify the trigger row for RETURNING, I am
not sure how much work it would take to allow that, but it seems like it
is a valid requite, and if so, I can add it to the TODO list.

-- 
  Bruce Momjian  <[email-masked]>        http://momjian.us
  EnterpriseDB                             http://enterprisedb.com

+ As you are, so once was I.  As I am, so you will be. +
+                      Ancient Roman grave inscription +
diff --git a/doc/src/sgml/trigger.sgml b/doc/src/sgml/trigger.sgml
new file mode 100644
index 67e1861..f62f420
*** a/doc/src/sgml/trigger.sgml
--- b/doc/src/sgml/trigger.sgml
***************
*** 230,236 ****
      used to signal that the trigger performed the necessary data
      modifications in the view.  This will cause the count of the number
      of rows affected by the command to be incremented. For
!     <command>INSERT</command> and <command>UPDATE</command> operations, the trigger
      may modify the <varname>NEW</varname> row before returning it.  This will
      change the data returned by
      <command>INSERT RETURNING</command> or <command>UPDATE RETURNING</command>,
--- 230,236 ----
      used to signal that the trigger performed the necessary data
      modifications in the view.  This will cause the count of the number
      of rows affected by the command to be incremented. For
!     <command>INSERT</command> and <command>UPDATE</command> operations only, the trigger
      may modify the <varname>NEW</varname> row before returning it.  This will
      change the data returned by
      <command>INSERT RETURNING</command> or <command>UPDATE RETURNING</command>,

---

From: Eugen Konkov <[email-masked]>
Date: Thu, 7 Nov 2019 11:20:32 +0200

> written the attached patch to fix that.  Does that help?

No.   If   we document that PG does not allow to modify OLD at instead
of  trigger,  the  we can not implement that. Probably we can put note
that  "currently  modification of the trigger row for RETURNING is not
implemented"

> is a valid requite, and if so, I can add it to the TODO list.

Yes,  Add please into TODO the feature to "allowing DELETE to modify the trigger row
for  RETURNING".  Becuase, as I have described at first letter, without
this the RETURNING rows **does not correspond actually deleted data**

Thank you.

-- 
Best regards,
Eugen Konkov

---

From: Eugen Konkov <[email-masked]>
Date: Thu, 7 Nov 2019 11:24:29 +0200

Hello Eugen,

> that  "currently  modification of the trigger row for RETURNING is not

sorry, typo. Please read:
"currently  modification of the trigger row for DELETE RETURNING is notimplemented"

-- 
Best regards,
Eugen Konkov

---

From: Bruce Momjian <[email-masked]>
Date: Thu, 7 Nov 2019 16:26:55 -0500

> "currently  modification of the trigger row for DELETE RETURNING is notimplemented"

In looking at the existing docs, the bullet above the quoted text says:

	For row-level INSERT and UPDATE triggers only, the returned row becomes
	                                         ----
	the row that will be inserted or will replace the row being updated.
	This allows the trigger function to modify the row being inserted or
	updated.

First, notice "only", which was missing from the later sentence:

	For <command>INSERT</command> and <command>UPDATE</command>
	operations [only], the trigger may modify the
	<varname>NEW</varname> row before returning it.

which I have now added with my applied patch to all supported releases. 

The major use of modifying NEW is to modify the data that goes into the
database, and its use to modify data seen by later executed triggers, or
by RETURNING, is only a side-effect of its primary purpose.  Therefore,
it is not surprising that, since DELETE does not modify any data, just
removes it, that the modification of OLD to appear in later triggers or
RETURNING is not supported.

> > this the RETURNING rows **does not correspond actually deleted data**

I have added a TODO item:

	Allow DELETE triggers to modify rows, for use by RETURNING

---

From: Bruce Momjian <[email-masked]>
Date: Thu, 7 Nov 2019 17:28:18 -0500

> 	Allow DELETE triggers to modify rows, for use by RETURNING 

Thinking some more on this, I now don't think a TODO makes sense, so I
have removed it.

Triggers are designed to check and modify input data, and since DELETE
has no input data, it makes no sense.  In the attached SQL script, you
can see that only the BEFORE INSERT trigger fires, so there is no way
even with INSERT to change what is passed after the write to RETURNING. 
What you can do is to modify the returning expression, which is what I
have done for the last query --- hopefully that will help you.

---

From: Eugen Konkov <[email-masked]>
Date: Sat, 9 Nov 2019 14:05:02 +0200

Hello Bruce,

> have done for the last query --- hopefully that will help you.

You  lost  my  idea.  First  of  all  I am talking about views and an
INSTEAD OF triggers.

INSERT/UPDATE operation present which data is added into DB
DELETE operation present which data is deleted from DB
(in  my  case I am not deleted exact that data which matched by where.
See example below)

Thus INSTEAD OF INSERT/UPDATE triggers are designed to check and modify input data
eg.  we can insert/update something different then incoming data (here
we are modifying NEW)

Thus INSTEAD OF DELETE triggers are designed to check and delete **output** data
eg. we can delete something different then underlaid data (here we are
modifying OLD)

for example, we have next data: 1 2 3 4 5 6 7 8
it  is not presented by eight rows, but instead it is presented as one
row with range data type: [1..8]

When we insert data we will not get new row, we change current:
insert into table values ( 9 ) will result
[1..9]
instead of
[1..8]
9

So  lets  look  into INSTEAD OF DELETE trigger when we deleting
data:
delete from table where x in ( 5, 6, 7 );
after deleting this we should get:
[1..4]
[8..9]

thus
with t1 as ( delete from table where x in ( 5, 6, 7 ) returning * )
select * from t1
should return:
[5..7]
instead of
[1..9]
because we does not delete ALL [1..9], we just delete ONLY [5..7]

Thus I need to change matched row OLD.x from [1..9] to [5..7]

Please  reread  my  first  letter. There I describe more real life example
when I am manipulating bi-temporal data.

where some value exist at given period:
id | app_period | value
7     [2019-01-01, 2019-04-05)    207

And I am deleting third month: [ 2019-03-01, 2019-04-01 )
with  t1  as  (  delete  from  table where app_period && [ 2019-03-01,
2019-04-01 ) returning * )
select * from t1;
7   [ 2019-03-01, 2019-04-01 )   207

select * from table;
7   [ 2019-01-01, 2019-03-01 )   207
7   [ 2019-04-01, 2019-04-05 )   207

-- 
Best regards,
Eugen Konkov

---

From: Eugen Konkov <[email-masked]>
Date: Sat, 9 Nov 2019 14:10:13 +0200

Hello Eugen,

> select * from table;

Here when data is deleted the next row is matched:
   7     [2019-01-01, 2019-04-05)    207
and assigned to OLD.
Because I am deleting data ONLY from [ 2019-03-01, 2019-04-01 ) period
I am required to change OLD:

OLD.app_period = [ 2019-03-01, 2019-04-01 )

instead of

-- 
Best regards,
Eugen Konkov

---

From: Liudmila Mantrova <[email-masked]>
Date: Mon, 11 Nov 2019 19:00:22 +0300

> 8 нояб. 2019 г., в 0:26, Bruce Momjian <[email-masked]> написал(а):
> 

Hi Bruce, 

I happened to browse recent documentation-related commits and I didn’t see this patch in REL_12_STABLE. Judging by the commit message, it should be applied there too.

---

From: Bruce Momjian <[email-masked]>
Date: Tue, 12 Nov 2019 22:04:59 -0500

> I happened to browse recent documentation-related commits and I didn’t see this patch in REL_12_STABLE. Judging by the commit message, it should be applied there too.

Wow, not sure how that happened, fixed.

---

From: Eugen Konkov <[email-masked]>
Date: Wed, 4 Dec 2019 13:37:57 +0200

Hi again.

> Thinking some more on this, I now don't think a TODO makes sense, so I

Please look into this example: https://dbfiddle.uk/?rdbms=postgres_12&fiddle=95ed9fab6870d7c4b6266ea4d93def13
This is real life code from our production.

You  can  see that this is important to get correct info about deleted
data

    -- EXPECTED app_period: ["2018-08-20", "2018-08-25")
    -- ACTUAL   app_period: ["2018-08-14", )

> has no input data, it makes no sense.

Please   put   back   into   TODO  list this feature  request to allow
triggers to modify output data.

INPUT     -- receives data                         OK (behavior is expected)
UPDATE  -- receives and returns data     OK (behavior is expected)
DELETE  -- returns data                           FAIL (behavior is not expected)

This  is  inconsistent  to  allow  modify  output  data for UPDATE and
restrict to do this for DELETE

Thank you

-- 
Best regards,
Eugen Konkov

---

From: Eugen Konkov <[email-masked]>
Date: Wed, 4 Dec 2019 13:39:43 +0200

Hello Eugen,

> https://dbfiddle.uk/?rdbms=postgres_12&fiddle=95ed9fab6870d7c4b6266ea4d93def13

sorry, forget to update link to the latest example:
https://dbfiddle.uk/?rdbms=postgres_12&fiddle=8e114ccc9f15a30ca3115cdc6c70d247

-- 
Best regards,
Eugen Konkov

---

From: Eugen Konkov <[email-masked]>
Date: Fri, 17 Jan 2020 12:14:03 +0200

Hello Bruce,

> has no input data, it makes no sense.

Sorry,  I  am  still ambiguous. You say that DELETE has no input data,
but doc says that it has:

https://www.postgresql.org/docs/current/trigger-definition.html
For  a row-level trigger, the input data also includes ... the OLD row for ... DELETE triggers

Also  restricting  DELETE  to  change  the  returned  data  by  DELETE
RETURNING seems as incomplete.

For example if triggers implement some compression.
 -- insert into field ZZZZZ value
 -- compress and actually store Zx5 into field
 -- Delete this insert row
 -- So user should get back that the value ZZZZZ was deleted and not Zx5.
 Correct?

 but currently user will see Zx5, because next code:

 OLD.value =  uncompress( OLD.value );

 does not effect RETURNING =(

-- 
Best regards,
Eugen Konkov