Horton

Thread

Commits

  1. Prevent dangling-pointer access when update trigger returns old tuple.

  2. Improve regression test coverage of regress.c.

  3. Remove unused functions in regress.c.

  4. Make plpgsql use its DTYPE_REC code paths for composite-type variables.

  5. This is part #1 for of the DEFERRED CONSTRAINT TRIGGER support.

  1. invalid memory alloc request size error with commit 4b93f579

    Rushabh Lathia <rushabh.lathia@gmail.com> — 2018-02-26T07:14:32Z

    Hi,

With commit 4b93f57999a2ca9b9c9e573ea32ab1aeaa8bf496, which plpgsql
use its DTYPE_REC code paths for composite-type variables - below
test started failing with "invalid memory alloc request size 2139062167
<%28213%29%20906-2167>"
error.

Testcase:

create table foo ( name varchar(20), type varchar(20));

insert into foo values ( 'Ford', 'Car');

CREATE OR REPLACE FUNCTION Trigger_Function() returns trigger as
$$
BEGIN
  RAISE NOTICE 'OLD: %, NEW: %', OLD, NEW;
  IF NEW.name = 'Ford' THEN
    return OLD; -- return old tuple
  END IF;
  return NEW; -- return original tuple
END;
$$ language plpgsql;

CREATE TRIGGER Before_Update_Trigger BEFORE UPDATE ON foo FOR EACH ROW
EXECUTE PROCEDURE Trigger_Function();

UPDATE foo SET type = 'New Car' where name = 'Ford';

Error coming while trying to copy the invalid tuple from
(heap_copytuple() <- ExecCopySlotTuple() <- ExecMaterializeSlot() <-
ExecUpdate() <- ExecModifyTable())

Here ExecBRUpdateTriggers() returns the invalid tuple when trigger
return old tuple.  Looking further I found that tuple is getting free
at ExecBRUpdateTriggers(), below code:

    if (trigtuple != fdw_trigtuple)
        heap_freetuple(trigtuple);


It seems like before commit 4b93f57999a2ca9b9c9e573ea32ab1aeaa8bf496,
plpgsql_exec_trigger() always used to copy the old and new tuple but
after that commit it doen't copy the "old" and "new" tuple if
if user just did "return new" or "return old" without changing anything.

With commit 4b93f57999a2ca9b9c9e573ea32ab1aeaa8bf496, which plpgsql
use its DTYPE_REC code paths for composite-type variables - below
test started failing with "invalid memory alloc request size 2139062167"
error.

Testcase:

create table foo ( name varchar(20), type varchar(20));

insert into foo values ( 'Ford', 'Car');

CREATE OR REPLACE FUNCTION Trigger_Function() returns trigger as
$$
BEGIN
  RAISE NOTICE 'OLD: %, NEW: %', OLD, NEW;
  IF NEW.name = 'Ford' THEN
    return OLD; -- return old tuple
  END IF;
  return NEW; -- return original tuple
END;
$$ language plpgsql;

CREATE TRIGGER Before_Update_Trigger BEFORE UPDATE ON foo FOR EACH ROW
EXECUTE PROCEDURE Trigger_Function();

UPDATE foo SET type = 'New Car' where name = 'Ford';

Error coming while trying to copy the invalid tuple from
(heap_copytuple() <- ExecCopySlotTuple() <- ExecMaterializeSlot() <-
ExecUpdate() <- ExecModifyTable())

Here ExecBRUpdateTriggers() returns the invalid tuple when trigger
return old tuple.  Looking further I found that tuple is getting free
at ExecBRUpdateTriggers(), below code:

    if (trigtuple != fdw_trigtuple)
        heap_freetuple(trigtuple);


It seems like before commit 4b93f57999a2ca9b9c9e573ea32ab1aeaa8bf496,
plpgsql_exec_trigger() always used to copy the old and new tuple but
after that commit it doen't copy the "old" and "new" tuple if
if user just did "return new" or "return old" without changing anything.

+           /*
+            * Copy tuple to upper executor memory.  But if user just did
+            * "return new" or "return old" without changing anything,
there's
+            * no need to copy; we can return the original tuple (which will
+            * save a few cycles in trigger.c as well as here).
+            */
+           if (rettup != trigdata->tg_newtuple &&
+               rettup != trigdata->tg_trigtuple)
+               rettup = SPI_copytuple(rettup);

In ExecBRUpdateTriggers(), we need to add a check that if trigtuple is same
as newtuple, then we don't require to free the trigtuple.

ExecBRDeleteTriggers() also does the similar things, but their we don't
need a check because it doesn't care about the return tuple.

PFA patch which add a check to not free the trigtuple if newtuple is same
as trigtuple and also added the related testcase.


Thanks,
Rushabh Lathia
www.EnterpriseDB.com

  2. Re: invalid memory alloc request size error with commit 4b93f579

    Tom Lane <tgl@sss.pgh.pa.us> — 2018-02-26T19:52:06Z

    Rushabh Lathia <rushabh.lathia@gmail.com> writes:
> In ExecBRUpdateTriggers(), we need to add a check that if trigtuple is same
> as newtuple, then we don't require to free the trigtuple.

Hm.  Seems like this is a very old bug: it's always been legal for a
trigger to return the "old" tuple if it felt like it, even if plpgsql
didn't happen to exercise that case.

Because of that angle, I'm not really happy with using plpgsql as
part of the test case.  The bug ought to be repaired in the back
branches too, but this test will prove little in the back branches.
Moreover, if somebody were to rejigger plpgsql again, the test might
stop proving anything at all.

I wonder whether it is worth creating a C trigger function
(probably in regress.c) specifically to exercise this situation.
If not, I'm inclined not to bother with adding a test case.

			regards, tom lane

  3. Re: invalid memory alloc request size error with commit 4b93f579

    Tom Lane <tgl@sss.pgh.pa.us> — 2018-02-26T21:25:25Z

    I wrote:
> I wonder whether it is worth creating a C trigger function
> (probably in regress.c) specifically to exercise this situation.

Actually, that doesn't seem too bad at all.  I propose applying
and back-patching the attached.

BTW, I noticed while doing this that the adjacent "funny_dup17"
trigger is dead code, and has been since commit 1547ee01 of
1999-09-29.  I'm inclined to rip it out, because anyone looking
at regress.c would naturally assume that anything in there is
being exercised.

			regards, tom lane

  4. Re: invalid memory alloc request size error with commit 4b93f579

    Daniel Gustafsson <daniel@yesql.se> — 2018-02-27T01:25:12Z

    > On 27 Feb 2018, at 05:25, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> 
> I wrote:
>> I wonder whether it is worth creating a C trigger function
>> (probably in regress.c) specifically to exercise this situation.
> 
> Actually, that doesn't seem too bad at all.  I propose applying
> and back-patching the attached.

LGTM

> BTW, I noticed while doing this that the adjacent "funny_dup17"
> trigger is dead code, and has been since commit 1547ee01 of
> 1999-09-29.  I'm inclined to rip it out, because anyone looking
> at regress.c would naturally assume that anything in there is
> being exercised.

+1, yes please.  regress_dist_ptpath() and regress_path_dist() in regress.c
also seem to be dead, and have been so for..  quite some time.

cheers ./daniel

  5. Re: invalid memory alloc request size error with commit 4b93f579

    Rushabh Lathia <rushabh.lathia@gmail.com> — 2018-02-27T01:57:45Z

    Thanks Tom.

On Tue, Feb 27, 2018 at 2:55 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

> I wrote:
> > I wonder whether it is worth creating a C trigger function
> > (probably in regress.c) specifically to exercise this situation.
>
> Actually, that doesn't seem too bad at all.  I propose applying
> and back-patching the attached.
>
>
Patch looks good to me.


> BTW, I noticed while doing this that the adjacent "funny_dup17"
> trigger is dead code, and has been since commit 1547ee01 of
> 1999-09-29.  I'm inclined to rip it out, because anyone looking
> at regress.c would naturally assume that anything in there is
> being exercised.
>
>                         regards, tom lane
>
>


-- 
Rushabh Lathia

  6. Re: invalid memory alloc request size error with commit 4b93f579

    Tom Lane <tgl@sss.pgh.pa.us> — 2018-02-27T03:10:06Z

    Daniel Gustafsson <daniel@yesql.se> writes:
>> On 27 Feb 2018, at 05:25, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> BTW, I noticed while doing this that the adjacent "funny_dup17"
>> trigger is dead code, and has been since commit 1547ee01 of
>> 1999-09-29.  I'm inclined to rip it out, because anyone looking
>> at regress.c would naturally assume that anything in there is
>> being exercised.

> +1, yes please.  regress_dist_ptpath() and regress_path_dist() in regress.c
> also seem to be dead, and have been so for..  quite some time.

Yeah.  Looking at
https://coverage.postgresql.org/src/test/regress/regress.c.gcov.html
it's evident that none of these functions are actually exercised
in the regression tests:

regress_dist_ptpath	unreferenced anywhere
regress_path_dist	unreferenced anywhere
poly2path		unreferenced anywhere
widget_in		used in type definition, but no input ever happens
widget_out		used in type definition, but no output ever happens
pt_in_widget		used for operator that evidently isn't called
boxarea			SQL function is created, but used nowhere
funny_dup17		SQL function is created, but used nowhere
int44in			used in type definition, but no input ever happens
int44out		used in type definition, but no output ever happens
test_fdw_handler	used by dummy FDW tests

I'm inclined to just remove regress_dist_ptpath, regress_path_dist,
poly2path, boxarea, and funny_dup17.  The others might better be dealt
with by making some actual use of them, since those type and operator
creation commands seem to have some test value of their own.

I notice BTW that int44in and int44out are not inverses, ie int44out
produces a string that int44in can't read :-(.  We'd definitely have to
fix that if we wanted to make any real use of the type.

			regards, tom lane

  7. Re: invalid memory alloc request size error with commit 4b93f579

    Daniel Gustafsson <daniel@yesql.se> — 2018-02-27T05:44:55Z

    > On 27 Feb 2018, at 11:10, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> 
> Daniel Gustafsson <daniel@yesql.se> writes:
>>> On 27 Feb 2018, at 05:25, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>>> BTW, I noticed while doing this that the adjacent "funny_dup17"
>>> trigger is dead code, and has been since commit 1547ee01 of
>>> 1999-09-29.  I'm inclined to rip it out, because anyone looking
>>> at regress.c would naturally assume that anything in there is
>>> being exercised.
> 
>> +1, yes please.  regress_dist_ptpath() and regress_path_dist() in regress.c
>> also seem to be dead, and have been so for..  quite some time.
> 
> Yeah.  Looking at
> https://coverage.postgresql.org/src/test/regress/regress.c.gcov.html
> it's evident that none of these functions are actually exercised
> in the regression tests:

Aha, that was a more clever way of figuring it out than what I did.

> I'm inclined to just remove regress_dist_ptpath, regress_path_dist,
> poly2path, boxarea, and funny_dup17.  The others might better be dealt
> with by making some actual use of them, since those type and operator
> creation commands seem to have some test value of their own.

Agreed.

> I notice BTW that int44in and int44out are not inverses, ie int44out
> produces a string that int44in can't read :-(.  We'd definitely have to
> fix that if we wanted to make any real use of the type.

Thats not nice given that the names imply that, but I agree that it’s not
something that needs to be changed given its current usecase.  That probably
warrants a comment in regress.c and/or the create_type test suite though.

cheers ./daniel