pg.ddx.io pgsql-hackers@postgresql.org mailing list archive
help / color / mirror / Atom feedinvalid memory alloc request size error with commit 4b93f579
7+ messages / 3 participants
[nested] [flat]
Commits from this thread:
2018-02-27 b45f821e22 5ccb775869 9bc33ef5ec 06f47297e2 2ee44e10d4 25b692568f Prevent dangling-pointer access when update trigger returns old tuple.
2018-02-27 be42eb9d62 Improve regression test coverage of regress.c.
2018-02-27 db3af9feb1 Remove unused functions in regress.c.
* invalid memory alloc request size error with commit 4b93f579
@ 2018-02-26 07:14 Rushabh Lathia <rushabh.lathia@gmail.com>
0 siblings, 1 reply; 7+ messages in thread
From: Rushabh Lathia @ 2018-02-26 07:14 UTC (permalink / raw)
To: pgsql-hackers
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
Attachments:
before_trigger_fix.patch (text/x-patch, 2.1K) download
diff --git a/src/backend/commands/trigger.c b/src/backend/commands/trigger.c
index fffc009..9315244 100644
--- a/src/backend/commands/trigger.c
+++ b/src/backend/commands/trigger.c
@@ -2815,7 +2815,8 @@ ExecBRUpdateTriggers(EState *estate, EPQState *epqstate,
return NULL; /* "do nothing" */
}
}
- if (trigtuple != fdw_trigtuple)
+ if (trigtuple != fdw_trigtuple &&
+ trigtuple != newtuple)
heap_freetuple(trigtuple);
if (newtuple != slottuple)
diff --git a/src/pl/plpgsql/src/expected/plpgsql_record.out b/src/pl/plpgsql/src/expected/plpgsql_record.out
index 29e42fd..0e898c2 100644
--- a/src/pl/plpgsql/src/expected/plpgsql_record.out
+++ b/src/pl/plpgsql/src/expected/plpgsql_record.out
@@ -479,6 +479,25 @@ table mutable;
bar baz
(2 rows)
+-- check returning old tuple
+create or replace function sillytrig() returns trigger language plpgsql as
+$$begin
+ raise notice 'old.ctid = %', old.ctid;
+ raise notice 'old.tableoid = %', old.tableoid::regclass;
+ return old;
+end$$;
+update mutable set f2 = f2 || ' baz';
+NOTICE: old.ctid = (0,3)
+NOTICE: old.tableoid = mutable
+NOTICE: old.ctid = (0,4)
+NOTICE: old.tableoid = mutable
+table mutable;
+ f2
+---------
+ foo baz
+ bar baz
+(2 rows)
+
-- check returning a composite datum from a trigger
create or replace function sillytrig() returns trigger language plpgsql as
$$begin
diff --git a/src/pl/plpgsql/src/sql/plpgsql_record.sql b/src/pl/plpgsql/src/sql/plpgsql_record.sql
index 781ccb0..0b5eaed 100644
--- a/src/pl/plpgsql/src/sql/plpgsql_record.sql
+++ b/src/pl/plpgsql/src/sql/plpgsql_record.sql
@@ -310,6 +310,16 @@ insert into mutable values ('foo'), ('bar');
update mutable set f2 = f2 || ' baz';
table mutable;
+-- check returning old tuple
+create or replace function sillytrig() returns trigger language plpgsql as
+$$begin
+ raise notice 'old.ctid = %', old.ctid;
+ raise notice 'old.tableoid = %', old.tableoid::regclass;
+ return old;
+end$$;
+update mutable set f2 = f2 || ' baz';
+table mutable;
+
-- check returning a composite datum from a trigger
create or replace function sillytrig() returns trigger language plpgsql as
* Re: invalid memory alloc request size error with commit 4b93f579
@ 2018-02-26 19:52 Tom Lane <tgl@sss.pgh.pa.us>
parent: Rushabh Lathia <rushabh.lathia@gmail.com>
0 siblings, 1 reply; 7+ messages in thread
From: Tom Lane @ 2018-02-26 19:52 UTC (permalink / raw)
To: Rushabh Lathia <rushabh.lathia@gmail.com>; +Cc: pgsql-hackers
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
^ permalink raw reply [nested|flat] 7+ messages in thread
* Re: invalid memory alloc request size error with commit 4b93f579
@ 2018-02-26 21:25 Tom Lane <tgl@sss.pgh.pa.us>
parent: Tom Lane <tgl@sss.pgh.pa.us>
0 siblings, 2 replies; 7+ messages in thread
From: Tom Lane @ 2018-02-26 21:25 UTC (permalink / raw)
To: Rushabh Lathia <rushabh.lathia@gmail.com>; +Cc: pgsql-hackers
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
Attachments:
before_trigger_fix-2.patch (text/x-diff, 4.4K) download
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 (
* Re: invalid memory alloc request size error with commit 4b93f579
@ 2018-02-27 01:25 Daniel Gustafsson <daniel@yesql.se>
parent: Tom Lane <tgl@sss.pgh.pa.us>
1 sibling, 1 reply; 7+ messages in thread
From: Daniel Gustafsson @ 2018-02-27 01:25 UTC (permalink / raw)
To: Tom Lane <tgl@sss.pgh.pa.us>; +Cc: Rushabh Lathia <rushabh.lathia@gmail.com>; pgsql-hackers
> 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
^ permalink raw reply [nested|flat] 7+ messages in thread
* Re: invalid memory alloc request size error with commit 4b93f579
@ 2018-02-27 01:57 Rushabh Lathia <rushabh.lathia@gmail.com>
parent: Tom Lane <tgl@sss.pgh.pa.us>
1 sibling, 0 replies; 7+ messages in thread
From: Rushabh Lathia @ 2018-02-27 01:57 UTC (permalink / raw)
To: Tom Lane <tgl@sss.pgh.pa.us>; +Cc: pgsql-hackers
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
^ permalink raw reply [nested|flat] 7+ messages in thread
* Re: invalid memory alloc request size error with commit 4b93f579
@ 2018-02-27 03:10 Tom Lane <tgl@sss.pgh.pa.us>
parent: Daniel Gustafsson <daniel@yesql.se>
0 siblings, 1 reply; 7+ messages in thread
From: Tom Lane @ 2018-02-27 03:10 UTC (permalink / raw)
To: Daniel Gustafsson <daniel@yesql.se>; +Cc: Rushabh Lathia <rushabh.lathia@gmail.com>; pgsql-hackers
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
^ permalink raw reply [nested|flat] 7+ messages in thread
* Re: invalid memory alloc request size error with commit 4b93f579
@ 2018-02-27 05:44 Daniel Gustafsson <daniel@yesql.se>
parent: Tom Lane <tgl@sss.pgh.pa.us>
0 siblings, 0 replies; 7+ messages in thread
From: Daniel Gustafsson @ 2018-02-27 05:44 UTC (permalink / raw)
To: Tom Lane <tgl@sss.pgh.pa.us>; +Cc: Rushabh Lathia <rushabh.lathia@gmail.com>; pgsql-hackers
> 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
^ permalink raw reply [nested|flat] 7+ messages in thread
end of thread, other threads:[~2018-02-27 05:44 UTC | newest]
Thread overview: 7+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2018-02-26 07:14 invalid memory alloc request size error with commit 4b93f579 Rushabh Lathia <rushabh.lathia@gmail.com>
2018-02-26 19:52 ` Tom Lane <tgl@sss.pgh.pa.us>
2018-02-26 21:25 ` Tom Lane <tgl@sss.pgh.pa.us>
2018-02-27 01:25 ` Daniel Gustafsson <daniel@yesql.se>
2018-02-27 03:10 ` Tom Lane <tgl@sss.pgh.pa.us>
2018-02-27 05:44 ` Daniel Gustafsson <daniel@yesql.se>
2018-02-27 01:57 ` Rushabh Lathia <rushabh.lathia@gmail.com>
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox