[BUG] Cache invalidation for queries that contains const of temporary composite type

Started by Maksim Milyutinover 8 years ago8 messages

milyutinma@gmail.com

over 8 years ago

1 attachment(s)

Hello everyone!

I have found out the problem when try to sequentially call the function
that casts constant to composite type of temporary table that is deleted
ateach transaction termination (i.e. at each function call completion).

For example, we have the following function 'test':

CREATE OR REPLACE FUNCTION test()
RETURNS void
LANGUAGE plpgsql
AS $function$
begin
create temp table tbl (id int) on commit drop;
perform json_populate_record(null::tbl, '{ "id": 11 }'::json) as tt;
end;
$function$

Оn the second and subsequent calls we'll observe the following error:

ERROR: cache lookup failed for type 16392

I investigated the problem and realized that result type of function
*json_populate_record* (/funcresulttype/ field of /FuncExpr/ struct) as
well as type of the first null argument (/consttype/ field of /Const/
struct) refer to the invalid composite type related with temporary
table'tbl'. Namely they take a value of oid gotten from the first 'tbl'
initialization. The plan of query *'perform
json_populate_record(null::tbl, '{ "id": 11 }'::json) as tt'*is cached
and is not invalidated at each function call. This is because** the
statement of this query doesn't have any dependency from the 'tbl'
relation (/relationOids/ list of /CachedPlanSource/ struct).

Attached patch resolves this problem by adding dependency from relation
upon detection Const expression of composite type of that relation.

--
Regards,
Maksim Milyutin

Attachments:

composite_type_cache_inval.patchtext/x-patch; name=composite_type_cache_inval.patchDownload

diff --git a/src/backend/optimizer/plan/setrefs.c b/src/backend/optimizer/plan/setrefs.c
index b0c9e94..bf5e4f4 100644
--- a/src/backend/optimizer/plan/setrefs.c
+++ b/src/backend/optimizer/plan/setrefs.c
@@ -78,6 +78,12 @@ typedef struct
 	(((con)->consttype == REGCLASSOID || (con)->consttype == OIDOID) && \
 	 !(con)->constisnull)
 
+/*
+ * Check if a Const node has composite type
+ */
+#define ISTABLETYPECONST(con) \
+	(OidIsValid(get_typ_typrelid((con)->consttype)))
+
 #define fix_scan_list(root, lst, rtoffset) \
 	((List *) fix_scan_expr(root, (Node *) (lst), rtoffset))
 
@@ -1410,6 +1416,12 @@ fix_expr_common(PlannerInfo *root, Node *node)
 			root->glob->relationOids =
 				lappend_oid(root->glob->relationOids,
 							DatumGetObjectId(con->constvalue));
+
+		/* Check whether const has composite type */
+		if (ISTABLETYPECONST(con))
+			root->glob->relationOids =
+				lappend_oid(root->glob->relationOids,
+							get_typ_typrelid(con->consttype));
 	}
 	else if (IsA(node, GroupingFunc))
 	{
diff --git a/src/test/regress/sql/plancache.sql b/src/test/regress/sql/plancache.sql
index cb2a551..41be0d6 100644
--- a/src/test/regress/sql/plancache.sql
+++ b/src/test/regress/sql/plancache.sql
@@ -140,6 +140,21 @@ create temp sequence seq;
 
 execute p2;
 
+-- Check that invalidation deals with casting const value to temporary
+-- composite type reinitialized on each new transaction
+
+create function cache_query_with_composite_const() returns void as $$
+begin
+    create temp table tbl(id int) on commit drop;
+
+    -- Plan of the next query has to be rebuilt on each new call of function
+    -- due to casting first argument 'null' to recreated temprary table 'tbl'
+    perform json_populate_record(null::tbl, '{"id": 0}'::json);
+end$$ language plpgsql;
+
+select cache_query_with_composite_const();
+select cache_query_with_composite_const();
+
 -- Check DDL via SPI, immediately followed by SPI plan re-use
 -- (bug in original coding)

Maksim Milyutin

milyutinma@gmail.com

over 8 years ago

In reply to: Maksim Milyutin (#1)

3 attachment(s)

Re: [BUG] Cache invalidation for queries that contains const of temporary composite type

25.09.17 20:50, Maksim Milyutin wrote:

I have found out the problem when try to sequentially call the
function that casts constant to composite type of temporary table that
is deleted ateach transaction termination (i.e. at each function call
completion).

For example, we have the following function 'test':

CREATE OR REPLACE FUNCTION test()
RETURNS void
LANGUAGE plpgsql
AS $function$
begin
create temp table tbl (id int) on commit drop;
perform json_populate_record(null::tbl, '{ "id": 11 }'::json) as tt;
end;
$function$

Оn the second and subsequent calls we'll observe the following error:

ERROR: cache lookup failed for type 16392

I investigated the problem and realized that result type of function
*json_populate_record* (/funcresulttype/ field of /FuncExpr/ struct)
as well as type of the first null argument (/consttype/ field of
/Const/ struct) refer to the invalid composite type related with
temporary table'tbl'. Namely they take a value of oid gotten from the
first 'tbl' initialization. The plan of query *'perform
json_populate_record(null::tbl, '{ "id": 11 }'::json) as tt'*is cached
and is not invalidated at each function call. This is because** the
statement of this query doesn't have any dependency from the 'tbl'
relation (/relationOids/ list of /CachedPlanSource/ struct).

Attached patch resolves this problem by adding dependency from
relation upon detection Const expression of composite type of that
relation

Updated patchset contains more transparent definition of composite type
for constant node and regression test for described above buggy case.

--
Regards,
Maksim Milyutin

Attachments:

composite_type_cache_inval.patchtext/plain; charset=UTF-8; name=composite_type_cache_inval.patch; x-mac-creator=0; x-mac-type=0Download

diff --git a/src/backend/optimizer/plan/setrefs.c b/src/backend/optimizer/plan/setrefs.c
index b0c9e94459..482b0d227c 100644
--- a/src/backend/optimizer/plan/setrefs.c
+++ b/src/backend/optimizer/plan/setrefs.c
@@ -78,6 +78,12 @@ typedef struct
 	(((con)->consttype == REGCLASSOID || (con)->consttype == OIDOID) && \
 	 !(con)->constisnull)
 
+/*
+ * Check if a Const node has composite type
+ */
+#define ISTABLETYPECONST(con) \
+	(get_typtype((con)->consttype) == TYPTYPE_COMPOSITE)
+
 #define fix_scan_list(root, lst, rtoffset) \
 	((List *) fix_scan_expr(root, (Node *) (lst), rtoffset))
 
@@ -1410,6 +1416,12 @@ fix_expr_common(PlannerInfo *root, Node *node)
 			root->glob->relationOids =
 				lappend_oid(root->glob->relationOids,
 							DatumGetObjectId(con->constvalue));
+
+		/* Check whether const has composite type */
+		if (ISTABLETYPECONST(con))
+			root->glob->relationOids =
+				lappend_oid(root->glob->relationOids,
+							get_typ_typrelid(con->consttype));
 	}
 	else if (IsA(node, GroupingFunc))
 	{

plancache.out.difftext/plain; charset=UTF-8; name=plancache.out.diff; x-mac-creator=0; x-mac-type=0Download

diff --git a/src/test/regress/expected/plancache.out b/src/test/regress/expected/plancache.out
index c2eeff1614..5c0be47778 100644
--- a/src/test/regress/expected/plancache.out
+++ b/src/test/regress/expected/plancache.out
@@ -220,6 +220,28 @@ execute p2;
        1
 (1 row)
 
+-- Check that invalidation deals with casting const value to temporary
+-- composite type reinitialized on each new transaction
+create function cache_query_with_composite_const() returns void as $$
+begin
+    create temp table tbl(id int) on commit drop;
+
+    -- Plan of the next query has to be rebuilt on each new call of function
+    -- due to casting first argument 'null' to recreated temprary table 'tbl'
+    perform json_populate_record(null::tbl, '{"id": 0}'::json);
+end$$ language plpgsql;
+select cache_query_with_composite_const();
+ cache_query_with_composite_const 
+----------------------------------
+ 
+(1 row)
+
+select cache_query_with_composite_const();
+ cache_query_with_composite_const 
+----------------------------------
+ 
+(1 row)
+
 -- Check DDL via SPI, immediately followed by SPI plan re-use
 -- (bug in original coding)
 create function cachebug() returns void as $$

plancache.sql.difftext/plain; charset=UTF-8; name=plancache.sql.diff; x-mac-creator=0; x-mac-type=0Download

diff --git a/src/test/regress/sql/plancache.sql b/src/test/regress/sql/plancache.sql
index cb2a551487..41be0d6bf4 100644
--- a/src/test/regress/sql/plancache.sql
+++ b/src/test/regress/sql/plancache.sql
@@ -140,6 +140,21 @@ create temp sequence seq;
 
 execute p2;
 
+-- Check that invalidation deals with casting const value to temporary
+-- composite type reinitialized on each new transaction
+
+create function cache_query_with_composite_const() returns void as $$
+begin
+    create temp table tbl(id int) on commit drop;
+
+    -- Plan of the next query has to be rebuilt on each new call of function
+    -- due to casting first argument 'null' to recreated temprary table 'tbl'
+    perform json_populate_record(null::tbl, '{"id": 0}'::json);
+end$$ language plpgsql;
+
+select cache_query_with_composite_const();
+select cache_query_with_composite_const();
+
 -- Check DDL via SPI, immediately followed by SPI plan re-use
 -- (bug in original coding)

Maksim Milyutin

milyutinma@gmail.com

over 8 years ago

In reply to: Maksim Milyutin (#2)

Re: [BUG] Cache invalidation for queries that contains const of temporary composite type

On 26.09.2017 23:25, Maksim Milyutin wrote:

25.09.17 20:50, Maksim Milyutin wrote:

I have found out the problem when try to sequentially call the
function that casts constant to composite type of temporary table
that is deleted ateach transaction termination (i.e. at each function
call completion).

For example, we have the following function 'test':

CREATE OR REPLACE FUNCTION test()
RETURNS void
LANGUAGE plpgsql
AS $function$
begin
create temp table tbl (id int) on commit drop;
perform json_populate_record(null::tbl, '{ "id": 11 }'::json) as tt;
end;
$function$

Оn the second and subsequent calls we'll observe the following error:

ERROR: cache lookup failed for type 16392

I investigated the problem and realized that result type of function
*json_populate_record* (/funcresulttype/ field of /FuncExpr/ struct)
as well as type of the first null argument (/consttype/ field of
/Const/ struct) refer to the invalid composite type related with
temporary table'tbl'. Namely they take a value of oid gotten from the
first 'tbl' initialization. The plan of query *'perform
json_populate_record(null::tbl, '{ "id": 11 }'::json) as tt'*is
cached and is not invalidated at each function call. This is
because** the statement of this query doesn't have any dependency
from the 'tbl' relation (/relationOids/ list of /CachedPlanSource/
struct).

Attached patch resolves this problem by adding dependency from
relation upon detection Const expression of composite type of that
relation

Updated patchset contains more transparent definition of composite
type for constant node and regression test for described above buggy case.

Is there any interest on the problem in this thread?

--
Regards,
Maksim Milyutin

Alexander Korotkov

a.korotkov@postgrespro.ru

over 8 years ago

In reply to: Maksim Milyutin (#3)

Re: [BUG] Cache invalidation for queries that contains const of temporary composite type

On Mon, Oct 2, 2017 at 3:43 PM, Maksim Milyutin <milyutinma@gmail.com>
wrote:

On 26.09.2017 23:25, Maksim Milyutin wrote:

Updated patchset contains more transparent definition of composite type
for constant node and regression test for described above buggy case.

Is there any interest on the problem in this thread?

Probably everybody are too busy with upcoming release and other patches.
Since this patch is a bug fix, it definitely should be considered. Please,
register this patch at upcoming commitfest to ensure it wouldn't be
forgotten.
Regression test changes (both .sql and .out) are essential parts of the
patch. I see no point in posting them separately. Please, incorporate
them into your patch.
Did you check this patch with manually created composite type (made by
CREATE TYPE typname AS ...)?

------
Alexander Korotkov
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company

Maksim Milyutin

milyutinma@gmail.com

over 8 years ago

In reply to: Alexander Korotkov (#4)

Re: [BUG] Cache invalidation for queries that contains const of temporary composite type

Hi, Alexander!

Thanks for the comments.

02.10.17 20:02, Alexander Korotkov wrote:

Please, register this patch at upcoming commitfest to ensure it
wouldn't be forgotten.
Regression test changes (both .sql and .out) are essential parts of
the patch. I see no point in posting them separately. Please,
incorporate them into your patch.

OK, I'll take your advice.

Did you check this patch with manually created composite type (made by
CREATE TYPE typname AS ...)?

I have tested the following case:

create type pair as (x int, y int);
prepare test as select json_populate_record(null::pair, '{"x": 1, "y":
2}'::json);
drop type pair cascade;

execute test;

-- The following output is obtained before patch
ERROR: cache lookup failed for type 16419

-- After applying patch
ERROR: type "pair" does not exist

But after recreating 'pair' type I'll get the following message:
ERROR: cached plan must not change result type

I don't know whether it's right behavior. Anyhow your point is a good
motivation to experiment and investigate different scenarios of work
with cached plan that depends on non-stable type. Thanks for that.

--
Regards,
Maksim Milyutin

Alexander Korotkov

a.korotkov@postgrespro.ru

over 8 years ago

In reply to: Maksim Milyutin (#5)

Re: [BUG] Cache invalidation for queries that contains const of temporary composite type

On Tue, Oct 3, 2017 at 12:20 AM, Maksim Milyutin <milyutinma@gmail.com>
wrote:

I have tested the following case:

create type pair as (x int, y int);
prepare test as select json_populate_record(null::pair, '{"x": 1, "y":
2}'::json);
drop type pair cascade;

execute test;

-- The following output is obtained before patch
ERROR: cache lookup failed for type 16419

-- After applying patch
ERROR: type "pair" does not exist

But after recreating 'pair' type I'll get the following message:
ERROR: cached plan must not change result type

I don't know whether it's right behavior. Anyhow your point is a good
motivation to experiment and investigate different scenarios of work with
cached plan that depends on non-stable type. Thanks for that.

I think ideally, cached plan should be automatically invalidated and stored
procedure should work without error.
Not really sure if it's feasible...

------
Alexander Korotkov
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company

Kyotaro HORIGUCHI

horiguchi.kyotaro@lab.ntt.co.jp

over 8 years ago

In reply to: Alexander Korotkov (#6)

Re: [BUG] Cache invalidation for queries that contains const of temporary composite type

Hi,

Sorry, I saw this once but somehow my attension was blown away on
the way.

At Tue, 3 Oct 2017 02:41:34 +0300, Alexander Korotkov <a.korotkov@postgrespro.ru> wrote in <CAPpHfdtCuh5UVezbjA2W+a-tPqXQ6xjAjeH6yyZ1DzxHOSwfsQ@mail.gmail.com>

On Tue, Oct 3, 2017 at 12:20 AM, Maksim Milyutin <milyutinma@gmail.com>
wrote:

I have tested the following case:

create type pair as (x int, y int);
prepare test as select json_populate_record(null::pair, '{"x": 1, "y":
2}'::json);
drop type pair cascade;

execute test;

-- The following output is obtained before patch
ERROR: cache lookup failed for type 16419

-- After applying patch
ERROR: type "pair" does not exist

But after recreating 'pair' type I'll get the following message:
ERROR: cached plan must not change result type

I don't know whether it's right behavior. Anyhow your point is a good
motivation to experiment and investigate different scenarios of work with
cached plan that depends on non-stable type. Thanks for that.

I think ideally, cached plan should be automatically invalidated and stored
procedure should work without error.
Not really sure if it's feasible...

Without the patch dropping a table used in a prepared query
results in the similar error. So I suppose it's the desired
behavior in the case.

execute test;
| ERROR: relation "t3" does not exist

The first thought that patch gave me is that the problem is not
limited to constants. Actually the following sequence also
reproduces similar failure even with this patch.

create table t2 (x int , y int);
create type pair as (x int, y int);
prepare test as select row(x, y)::pair from t2;
drop type pair;
execute test;
| ERROR: cache lookup failed for type 16410

In this case the causal expression is in the following form.

TargetEntry (
expr = (
RowExpr:
typeid = 16410,
row_format = COERCE_EXPLICIT_CAST,
args = List (Var(t2.x), Var(t2.y))
)
)

regards,

--
Kyotaro Horiguchi
NTT Open Source Software Center

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Maksim Milyutin

milyutinma@gmail.com

over 8 years ago

In reply to: Kyotaro HORIGUCHI (#7)

Re: [BUG] Cache invalidation for queries that contains const of temporary composite type

Hi HORIGUCHI-san!

Thanks for the valuable comments.

03.10.17 4:30, Kyotaro HORIGUCHI wrote:

The first thought that patch gave me is that the problem is not
limited to constants. Actually the following sequence also
reproduces similar failure even with this patch.

create table t2 (x int , y int);
create type pair as (x int, y int);
prepare test as select row(x, y)::pair from t2;
drop type pair;
execute test;
| ERROR: cache lookup failed for type 16410

In this case the causal expression is in the following form.

TargetEntry (
expr = (
RowExpr:
typeid = 16410,
row_format = COERCE_EXPLICIT_CAST,
args = List (Var(t2.x), Var(t2.y))
)
)

Yeah, RowExpr has no dependency from type relation with oid=16410. I
think the routine 'fix_expr_common' needs to be reworked to take into
account any possible dependencies of expression from composite type.

On November commitfest I'll lay out patch that covers your case.

--
Regards,
Maksim Milyutin

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers