Thread: Re: Unexpected behavior with transition tables in update statement trigger
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
Attachment
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
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
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], 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 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
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 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. > >[1] https://www.postgresql.org/developer/roadmap/ Thanks, that's what I am going to do. 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. > >[1] https://www.postgresql.org/developer/roadmap/ Thanks, that's what I am going to do. Cheers, Tom