Horton

Thread

Commits

  1. Use the correct tuplestore read pointer in a NamedTuplestoreScan.

  1. Unexpected behavior with transition tables in update statement trigger

    Tom Kazimiers <tom@voodoo-arts.net> — 2018-02-24T03:47:48Z

    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

  2. Re: Unexpected behavior with transition tables in update statement trigger

    Thomas Munro <thomas.munro@enterprisedb.com> — 2018-02-26T10:15:44Z

    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

  3. Re: Unexpected behavior with transition tables in update statement trigger

    Tom Kazimiers <tom@voodoo-arts.net> — 2018-02-26T15:18:07Z

    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

  4. Re: Unexpected behavior with transition tables in update statement trigger

    Thomas Munro <thomas.munro@enterprisedb.com> — 2018-02-27T01:52:02Z

    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

  5. Re: Unexpected behavior with transition tables in update statement trigger

    Tom Kazimiers <tom@voodoo-arts.net> — 2018-02-27T20:08:30Z

    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 = 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;

That's a much cleaner repro (and test case I suppose), thanks.

Tom

  6. Re: Unexpected behavior with transition tables in update statement trigger

    Tom Lane <tgl@sss.pgh.pa.us> — 2018-02-27T20:58:14Z

    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

  7. Re: Unexpected behavior with transition tables in update statement trigger

    Thomas Munro <thomas.munro@enterprisedb.com> — 2018-02-27T21:27:23Z

    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], 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

  8. Re: Unexpected behavior with transition tables in update statement trigger

    Tom Kazimiers <tom@voodoo-arts.net> — 2018-02-27T21:40:32Z

    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

  9. Re: Unexpected behavior with transition tables in update statement trigger

    Tom Kazimiers <tom@voodoo-arts.net> — 2018-02-27T22:54:44Z

    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.
>
>[1] https://www.postgresql.org/developer/roadmap/

Thanks, that's what I am going to do.

Cheers,
Tom