Обсуждение: invalid memory alloc request size error with commit 4b93f579

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

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

diff --git a/src/backend/commands/trigger.c b/src/backend/commands/trigger.c
index fffc009..fbd176b 100644
*** a/src/backend/commands/trigger.c
--- b/src/backend/commands/trigger.c
*************** ExecBRUpdateTriggers(EState *estate, EPQ
*** 2815,2821 ****
              return NULL;        /* "do nothing" */
          }
      }
!     if (trigtuple != fdw_trigtuple)
          heap_freetuple(trigtuple);

      if (newtuple != slottuple)
--- 2815,2821 ----
              return NULL;        /* "do nothing" */
          }
      }
!     if (trigtuple != fdw_trigtuple && trigtuple != newtuple)
          heap_freetuple(trigtuple);

      if (newtuple != slottuple)
diff --git a/src/test/regress/expected/triggers.out b/src/test/regress/expected/triggers.out
index ce8fa21..4771069 100644
*** a/src/test/regress/expected/triggers.out
--- b/src/test/regress/expected/triggers.out
*************** DROP TABLE fkeys2;
*** 153,158 ****
--- 153,184 ----
  -- select count(*) from dup17 where x = 13;
  --
  -- DROP TABLE dup17;
+ -- Check behavior when trigger returns unmodified trigtuple
+ create table trigtest (f1 int, f2 text);
+ create trigger trigger_return_old
+     before insert or delete or update on trigtest
+     for each row execute procedure trigger_return_old();
+ insert into trigtest values(1, 'foo');
+ select * from trigtest;
+  f1 | f2
+ ----+-----
+   1 | foo
+ (1 row)
+
+ update trigtest set f2 = f2 || 'bar';
+ select * from trigtest;
+  f1 | f2
+ ----+-----
+   1 | foo
+ (1 row)
+
+ delete from trigtest;
+ select * from trigtest;
+  f1 | f2
+ ----+----
+ (0 rows)
+
+ drop table trigtest;
  create sequence ttdummy_seq increment 10 start 0 minvalue 0;
  create table tttest (
      price_id    int4,
diff --git a/src/test/regress/input/create_function_1.source b/src/test/regress/input/create_function_1.source
index cde78eb..6b6bcd8 100644
*** a/src/test/regress/input/create_function_1.source
--- b/src/test/regress/input/create_function_1.source
*************** CREATE FUNCTION funny_dup17 ()
*** 42,47 ****
--- 42,52 ----
          AS '@libdir@/regress@DLSUFFIX@'
          LANGUAGE C;

+ CREATE FUNCTION trigger_return_old ()
+         RETURNS trigger
+         AS '@libdir@/regress@DLSUFFIX@'
+         LANGUAGE C;
+
  CREATE FUNCTION ttdummy ()
          RETURNS trigger
          AS '@libdir@/regress@DLSUFFIX@'
diff --git a/src/test/regress/output/create_function_1.source b/src/test/regress/output/create_function_1.source
index ab601be..9154b4a 100644
*** a/src/test/regress/output/create_function_1.source
--- b/src/test/regress/output/create_function_1.source
*************** CREATE FUNCTION funny_dup17 ()
*** 39,44 ****
--- 39,48 ----
          RETURNS trigger
          AS '@libdir@/regress@DLSUFFIX@'
          LANGUAGE C;
+ CREATE FUNCTION trigger_return_old ()
+         RETURNS trigger
+         AS '@libdir@/regress@DLSUFFIX@'
+         LANGUAGE C;
  CREATE FUNCTION ttdummy ()
          RETURNS trigger
          AS '@libdir@/regress@DLSUFFIX@'
diff --git a/src/test/regress/regress.c b/src/test/regress/regress.c
index 13e7207..521b181 100644
*** a/src/test/regress/regress.c
--- b/src/test/regress/regress.c
*************** funny_dup17(PG_FUNCTION_ARGS)
*** 444,449 ****
--- 444,465 ----
      return PointerGetDatum(tuple);
  }

+ PG_FUNCTION_INFO_V1(trigger_return_old);
+
+ Datum
+ trigger_return_old(PG_FUNCTION_ARGS)
+ {
+     TriggerData *trigdata = (TriggerData *) fcinfo->context;
+     HeapTuple    tuple;
+
+     if (!CALLED_AS_TRIGGER(fcinfo))
+         elog(ERROR, "trigger_return_old: not fired by trigger manager");
+
+     tuple = trigdata->tg_trigtuple;
+
+     return PointerGetDatum(tuple);
+ }
+
  #define TTDUMMY_INFINITY    999999

  static SPIPlanPtr splan = NULL;
diff --git a/src/test/regress/sql/triggers.sql b/src/test/regress/sql/triggers.sql
index ae8349c..2bad469 100644
*** a/src/test/regress/sql/triggers.sql
--- b/src/test/regress/sql/triggers.sql
*************** DROP TABLE fkeys2;
*** 138,143 ****
--- 138,159 ----
  --
  -- DROP TABLE dup17;

+ -- Check behavior when trigger returns unmodified trigtuple
+ create table trigtest (f1 int, f2 text);
+
+ create trigger trigger_return_old
+     before insert or delete or update on trigtest
+     for each row execute procedure trigger_return_old();
+
+ insert into trigtest values(1, 'foo');
+ select * from trigtest;
+ update trigtest set f2 = f2 || 'bar';
+ select * from trigtest;
+ delete from trigtest;
+ select * from trigtest;
+
+ drop table trigtest;
+
  create sequence ttdummy_seq increment 10 start 0 minvalue 0;

  create table tttest (

> 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

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

> 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