Unexpected behavior with transition tables in update statement trigger
Hi all,
I am on Postgres 10.2 and try to get a statement level trigger to work
that is executed after UPDATE statements on a particular table. This
trigger references both the old and new transition table and for some
reason I am unable to reference each transition table multiple times in
a CTE or subquery. E.g. forming a UNION ALL with all rows of the new
transition table with itself, does only use the new table row once. I
don't understand why and would appreciate some insight.
My problem is probably better described with some SQL, so here is a
little test setup:
CREATE TABLE test (id serial, data int);
INSERT INTO test VALUES (0, 1);
CREATE OR REPLACE FUNCTION on_edit() RETURNS trigger
LANGUAGE plpgsql AS
$$
DECLARE
tmp text;
BEGIN
WITH test AS (
SELECT row_to_json(a)::text FROM new_test a
UNION ALL
SELECT '----'
UNION ALL
SELECT row_to_json(b)::text FROM new_test b
)
SELECT array_to_string(array(SELECT row_to_json(t)::text FROM test t), ', ')::text INTO tmp;
PERFORM pg_notify('update', tmp::text);
WITH test AS (
SELECT row_to_json(a)::text FROM new_test a
UNION ALL
SELECT '----'
UNION ALL
SELECT row_to_json(b)::text FROM old_test b
)
SELECT array_to_string(array(SELECT row_to_json(t)::text FROM test t), ', ')::text INTO tmp;
PERFORM pg_notify('update', tmp::text);
RETURN NEW;
END;
$$;
CREATE TRIGGER on_edit AFTER UPDATE ON test
REFERENCING NEW TABLE AS new_test OLD TABLE as old_test
FOR EACH STATEMENT EXECUTE PROCEDURE on_edit();
LISTEN update;
UPDATE test SET data = 2;
This will create a new table test with one entry, adds the statement
level trigger, registers a NOTIFY listener and updates the table. The
trigger will first NOTIFY the result of a UNION ALL with the new
transition table with itself. The second NOTIFY has the result of the
UNION ALL with the new and old transition tables as payload. This is the
output:
Asynchronous notification "update" with payload "{"id":0,"data":2}, ----" received from server process with PID 6695.
Asynchronous notification "update" with payload "{"id":0,"data":2}, ----, {"id":0,"data":1}" received from server process with PID 6695.
I would have expected the first line to be
Asynchronous notification "update" with payload "{"id":0,"data":2}, ----, {"id":0,"data":2}" received from server process with PID 6695.
Why isn't it? It's the same result with a subquery. What do I overlook
here?
Cheers,
Tom
On Sat, Feb 24, 2018 at 4:47 PM, Tom Kazimiers <tom@voodoo-arts.net> wrote:
I am on Postgres 10.2 and try to get a statement level trigger to work that
is executed after UPDATE statements on a particular table. This trigger
references both the old and new transition table and for some reason I am
unable to reference each transition table multiple times in a CTE or
subquery. E.g. forming a UNION ALL with all rows of the new transition table
with itself, does only use the new table row once. I don't understand why
and would appreciate some insight.
Thanks for the reproducer. Yeah, that seems to be a bug.
nodeNamedTuplestorescan.c allocates a new read pointer for each
separate scan of the named tuplestore, but it doesn't call
tuplestore_select_read_pointer() so that the two scans that appear in
your UNION ALL plan are sharing the same read pointer. At first
glance the attached seems to fix the problem, but I'll need to look
more carefully tomorrow.
--
Thomas Munro
http://www.enterprisedb.com
Attachments:
named-tuplestore-scan-select.patchapplication/octet-stream; name=named-tuplestore-scan-select.patchDownload+1-0
Hi Thomas,
On Mon, Feb 26, 2018 at 11:15:44PM +1300, Thomas Munro wrote:
On Sat, Feb 24, 2018 at 4:47 PM, Tom Kazimiers <tom@voodoo-arts.net> wrote:
Thanks for the reproducer. Yeah, that seems to be a bug.
nodeNamedTuplestorescan.c allocates a new read pointer for each
separate scan of the named tuplestore, but it doesn't call
tuplestore_select_read_pointer() so that the two scans that appear in
your UNION ALL plan are sharing the same read pointer. At first
glance the attached seems to fix the problem, but I'll need to look
more carefully tomorrow.
Thanks very much for investigating this. I can confirm that applying
your patch results in the tuples I expected in both my test trigger and
my actual trigger function.
It would be great if this or a similar fix would make it into the next
official release.
Cheers,
Tom
On Tue, Feb 27, 2018 at 4:18 AM, Tom Kazimiers <tom@voodoo-arts.net> wrote:
On Mon, Feb 26, 2018 at 11:15:44PM +1300, Thomas Munro wrote:
On Sat, Feb 24, 2018 at 4:47 PM, Tom Kazimiers <tom@voodoo-arts.net>
wrote:
Thanks for the reproducer. Yeah, that seems to be a bug.
nodeNamedTuplestorescan.c allocates a new read pointer for each
separate scan of the named tuplestore, but it doesn't call
tuplestore_select_read_pointer() so that the two scans that appear in
your UNION ALL plan are sharing the same read pointer. At first
glance the attached seems to fix the problem, but I'll need to look
more carefully tomorrow.Thanks very much for investigating this. I can confirm that applying your
patch results in the tuples I expected in both my test trigger and my actual
trigger function.
Thanks for testing.
It would be great if this or a similar fix would make it into the next
official release.
Here's a new version with tuplestore_select_read_pointer() added in
another place where it was lacking, and commit message. Moving to
-hackers, where patches go.
Here's a shorter repro. On master it prints:
NOTICE: count = 1
NOTICE: count union = 1
With the patch the second number is 2, as it should be.
CREATE TABLE test (i int);
INSERT INTO test VALUES (1);
CREATE OR REPLACE FUNCTION my_trigger_fun() RETURNS trigger
LANGUAGE plpgsql AS
$$
BEGIN
RAISE NOTICE 'count = %', (SELECT COUNT(*) FROM new_test);
RAISE NOTICE 'count union = %', (SELECT COUNT(*)
FROM (SELECT * FROM new_test UNION ALL SELECT * FROM new_test) ss);
RETURN NULL;
END;
$$;
CREATE TRIGGER my_trigger AFTER UPDATE ON test
REFERENCING NEW TABLE AS new_test OLD TABLE as old_test
FOR EACH STATEMENT EXECUTE PROCEDURE my_trigger_fun();
UPDATE test SET i = i;
--
Thomas Munro
http://www.enterprisedb.com
Attachments:
0001-Fix-tuplestore-read-pointer-confusion-in-nodeNamedtu.patchapplication/octet-stream; name=0001-Fix-tuplestore-read-pointer-confusion-in-nodeNamedtu.patchDownload+2-1
On Tue, Feb 27, 2018 at 02:52:02PM +1300, Thomas Munro wrote:
On Tue, Feb 27, 2018 at 4:18 AM, Tom Kazimiers <tom@voodoo-arts.net> wrote:
It would be great if this or a similar fix would make it into the
next official release.Here's a new version with tuplestore_select_read_pointer() added in
another place where it was lacking, and commit message. Moving to
-hackers, where patches go.
Thanks and just to confirm the obvious: the new patch still fixes this
bug in both my test trigger function and my real trigger function.
Here's a shorter repro. On master it prints:
NOTICE: count = 1
NOTICE: count union = 1With the patch the second number is 2, as it should be.
CREATE TABLE test (i int);
INSERT INTO test VALUES (1);CREATE OR REPLACE FUNCTION my_trigger_fun() RETURNS trigger
LANGUAGE plpgsql AS
$$
BEGIN
RAISE NOTICE 'count = %', (SELECT COUNT(*) FROM new_test);
RAISE NOTICE 'count union = %', (SELECT COUNT(*)
FROM (SELECT * FROM new_test UNION ALL SELECT * FROM new_test) ss);
RETURN NULL;
END;
$$;CREATE TRIGGER my_trigger AFTER UPDATE ON test
REFERENCING NEW TABLE AS new_test OLD TABLE as old_test
FOR EACH STATEMENT EXECUTE PROCEDURE my_trigger_fun();UPDATE test SET i = i;
That's a much cleaner repro (and test case I suppose), thanks.
Tom
Thomas Munro <thomas.munro@enterprisedb.com> writes:
Here's a new version with tuplestore_select_read_pointer() added in
another place where it was lacking, and commit message. Moving to
-hackers, where patches go.
Pushed, along with a regression test based on your example.
Unfortunately, this came in a bit too late for this week's releases :-(
regards, tom lane
On Wed, Feb 28, 2018 at 9:58 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
Thomas Munro <thomas.munro@enterprisedb.com> writes:
Here's a new version with tuplestore_select_read_pointer() added in
another place where it was lacking, and commit message. Moving to
-hackers, where patches go.Pushed, along with a regression test based on your example.
Unfortunately, this came in a bit too late for this week's releases :-(
Thanks!
Tom K, if you need a workaround before 10.4 comes out in May[1]https://www.postgresql.org/developer/roadmap/, you
could try selecting the whole transition table into a CTE up front.
Something like WITH my_copy AS (SELECT * FROM new_table) SELECT * FROM
my_copy UNION ALL SELECT * FROM my_copy should work.
[1]: https://www.postgresql.org/developer/roadmap/
--
Thomas Munro
http://www.enterprisedb.com
On Tue, Feb 27, 2018 at 03:58:14PM -0500, Tom Lane wrote:
Thomas Munro <thomas.munro@enterprisedb.com> writes:
Here's a new version with tuplestore_select_read_pointer() added in
another place where it was lacking, and commit message. Moving to
-hackers, where patches go.Pushed, along with a regression test based on your example.
Unfortunately, this came in a bit too late for this week's releases :-(
Ah, so close. :-) Regardless, thanks both of you for fixing this and
committing the fix to master. I am looking forward to the release
including this.
Cheers,
Tom
On Wed, Feb 28, 2018 at 10:27:23AM +1300, Thomas Munro wrote:
Tom K, if you need a workaround before 10.4 comes out in May[1], you
could try selecting the whole transition table into a CTE up front.
Something like WITH my_copy AS (SELECT * FROM new_table) SELECT * FROM
my_copy UNION ALL SELECT * FROM my_copy should work.
Thanks, that's what I am going to do.
Cheers,
Tom