View Issue Details

IDProjectCategoryView StatusLast Update
0000551Pgpool-IIBugpublic2019-11-27 15:54
ReporternagataAssigned Topengbo 
PrioritynormalSeverityminorReproducibilityalways
Status assignedResolutionopen 
Product Version 
Target VersionFixed in Version 
Summary0000551: Some cases where now() is not rewritten in native replication mode
DescriptionI found some cases where now() function in a query is not rewritten to a timestamp text in native replication mode.

1. CREATE TABLE tbl1 AS SELECT now();
2. SELECT now() INTO tbl2;
3.
WITH x AS (SELECT now()),
       ins AS ( INSERT INTO tbl3 SELECT * FROM x RETURNING 1)
SELECT;

These might be corner cases but users could issues such queries and I couldn't find any description about these restrictions in the documentation.
TagsNo tags attached.

Activities

pengbo

2019-10-25 17:35

developer   ~0002943

Thank you for you report.

I have fixed and committed it:
https://git.postgresql.org/gitweb?p=pgpool2.git;a=commitdiff;h=48caca8bcb91dd052f289595f26cd594e9dc2739

-------
test
-------

(1)
test=# CREATE TABLE tbl1 AS SELECT now();

LOG:
statement: CREATE TABLE "tbl1" AS SELECT "pg_catalog"."timestamptz"('2019-10-25 17:33:30.400873+09'::text)
statement: CREATE TABLE "tbl1" AS SELECT "pg_catalog"."timestamptz"('2019-10-25 17:33:30.400873+09'::text)

(2)
test=# SELECT now() INTO tbl2;

LOG:
statement: CREATE TABLE "tbl2" AS SELECT "pg_catalog"."timestamptz"('2019-10-25 17:34:08.830724+09'::text)
statement: CREATE TABLE "tbl2" AS SELECT "pg_catalog"."timestamptz"('2019-10-25 17:34:08.830724+09'::text)

(3)
test=# WITH x AS (SELECT now()),
test-# ins AS ( INSERT INTO tbl3 SELECT * FROM x RETURNING 1)
test-# SELECT;

LOG:
statement: WITH "x" AS ( SELECT "pg_catalog"."timestamptz"('2019-10-25 17:32:13.666084+09'::text)),"ins" AS (INSERT INTO "tbl3" SELECT * FROM "x" RETURNING 1) SELECT
statement: WITH "x" AS ( SELECT "pg_catalog"."timestamptz"('2019-10-25 17:32:13.666084+09'::text)),"ins" AS (INSERT INTO "tbl3" SELECT * FROM "x" RETURNING 1) SELECT

nagata

2019-10-28 10:55

developer   ~0002944

Last edited: 2019-10-28 10:57

View 2 revisions

well ... I am not sure that it is acceptable to rewrite all queries including now() even if this doesn't any update.

test=# select now();
-> SELECT "pg_catalog"."timestamptz"('2019-10-28 10:29:42.008142+09'::text)

Also, I found that CREATE MATERIALIZED VIEW including now() is rewritten to CREATE TABLE statements! (Note this is CreateTableAsStmt internally.)

test=# create materialized view mv2 as select now();
 -> CREATE TABLE "mv2" AS SELECT "pg_catalog"."timestamptz"('2019-10-28 10:34:47.71469+09'::text)

I disagree with rewriting CREATE MATERIALIZED VIEW statement in the first place. Even if now() in matviews definition is correctly rewritten, this would not be updated by REFRESH properly. Also, I think it would be unacceptable that view definition queries specified by users are rewritten silently. So, I think it should be a restriction that users must not issue CREATE MATERIALIZED VIEW via Pgpool-II in native replication mode if users want to avoid inconsistency. Any thought?

pengbo

2019-10-29 17:34

developer   ~0002946

Thank you for reviewing.

> well ... I am not sure that it is acceptable to rewrite all queries including now() even if this doesn't any update.

You are right. I fix that.

> I disagree with rewriting CREATE MATERIALIZED VIEW statement in the first place. Even if now() in matviews definition is correctly rewritten, this would not be updated by REFRESH properly. Also, I think it would be unacceptable that view definition queries specified by users are rewritten silently. So, I think it should be a restriction that users must not issue CREATE MATERIALIZED VIEW via Pgpool-II in native replication mode if users want to avoid inconsistency. Any thought?

Yes. We should not rewrite "CREATE MATERIALIZED VIEW" to "CREATE TABLE".
I made a patch to rewrite "CREATE TABLE" only. And I will add this restriction to documentation.

Patch is attached.

pengbo

2019-10-29 17:35

developer  

rewrite_query_v2.patch (1,271 bytes)
diff --git a/src/rewrite/pool_timestamp.c b/src/rewrite/pool_timestamp.c
index f36dd80..4698da5 100644
--- a/src/rewrite/pool_timestamp.c
+++ b/src/rewrite/pool_timestamp.c
@@ -908,7 +908,7 @@ rewrite_timestamp(POOL_CONNECTION_POOL * backend, Node *node,
 		/*
 		 * CREATE TABLE t1 AS SELECT now();
 		 */
-		if (IsA(c_stmt->query, SelectStmt))
+		if (IsA(c_stmt->query, SelectStmt) && c_stmt->relkind == OBJECT_TABLE)
 		{
 			/* rewrite params */
 			raw_expression_tree_walker(
@@ -925,16 +925,22 @@ rewrite_timestamp(POOL_CONNECTION_POOL * backend, Node *node,
 		/*
 		 * SELECT now() INTO t1;
 		 */
-		raw_expression_tree_walker(
-								   (Node *) s_stmt,
-								   rewrite_timestamp_walker, (void *) &ctx);
+		if (s_stmt->intoClause)
+		{
+			raw_expression_tree_walker(
+									   (Node *) s_stmt,
+									   rewrite_timestamp_walker, (void *) &ctx);
+		}
 
 		/*
 		 * WITH ins AS ( INSERT INTO t1 SELECT now()) SELECT;
 		 */
-		raw_expression_tree_walker(
-								   (Node *) s_stmt->withClause,
-								   rewrite_timestamp_walker, (void *) &ctx);
+		if (s_stmt->withClause)
+		{
+			raw_expression_tree_walker(
+									   (Node *) s_stmt->withClause,
+									   rewrite_timestamp_walker, (void *) &ctx);
+		}
 
 		rewrite = ctx.rewrite;
 	}
rewrite_query_v2.patch (1,271 bytes)

nagata

2019-10-29 18:10

developer   ~0002947

This also rewrite the following query which is not modifying.

WITH x AS (SELECT now()) SELECT;

So, you have to check if CTEs has non-SELECT query before rewriting as is_select_query() does.

nagata

2019-10-29 18:14

developer   ~0002948

Last edited: 2019-10-29 18:16

View 2 revisions

In addition, I found another case where now() is not rewritten when I was looking into is_select_query().

explain analyze insert into x values (now());

nagata

2019-10-29 18:46

developer   ~0002949

Last edited: 2019-10-29 18:53

View 2 revisions

> So, you have to check if CTEs has non-SELECT query before rewriting as is_select_query() does.

Or, maybe, you can call is_select_query() in rewrite_timestamp() (in this case you need to add sql string to args of rewrite_timestamp()), or befor calling rewrite_timestamp(). This is just an idea....

pengbo

2019-10-31 08:42

developer   ~0002952

Thank you.
I will fix CTEs rewriting.

pengbo

2019-11-15 16:14

developer   ~0002962

I fixed CTEs and "explain analyze ..." rewriting.
Patch is attached.


test=# explain analyze INSERT INTO t1 values (now());
-------------
LOG: DB node id: 0 backend pid: 12363 statement: EXPLAIN (analyze )INSERT INTO "t1" VALUES ("pg_catalog"."timestamptz"('2019-11-15 16:10:52.5316 61+09'::text))
LOG: DB node id: 1 backend pid: 12364 statement: EXPLAIN (analyze )INSERT INTO "t1" VALUES ("pg_catalog"."timestamptz"('2019-11-15 16:10:52.5316 61+09'::text))
-------------

test=# WITH x AS (SELECT now()) SELECT;
-------------
LOG: DB node id: 0 backend pid: 12363 statement: WITH x AS (SELECT now()) SELECT;
-------------

rewrite_query_v3.patch (2,035 bytes)
diff --git a/src/protocol/pool_proto_modules.c b/src/protocol/pool_proto_modules.c
index e1aa52c..a778570 100644
--- a/src/protocol/pool_proto_modules.c
+++ b/src/protocol/pool_proto_modules.c
@@ -619,7 +619,7 @@ SimpleQuery(POOL_CONNECTION * frontend,
 		 */
 		if (!commit)
 		{
-			char	   *rewrite_query;
+			char	   *rewrite_query = NULL;
 
 			if (node)
 			{
@@ -637,7 +637,9 @@ SimpleQuery(POOL_CONNECTION * frontend,
 				}
 
 				/* rewrite `now()' to timestamp literal */
-				rewrite_query = rewrite_timestamp(backend, query_context->parse_tree, false, msg);
+				if (!is_select_query(node, query_context->original_query) ||
+					pool_has_function_call(node) || pool_config->replicate_select)
+					rewrite_query = rewrite_timestamp(backend, query_context->parse_tree, false, msg);
 
 				/*
 				 * If the query is BEGIN READ WRITE or BEGIN ... SERIALIZABLE
diff --git a/src/rewrite/pool_timestamp.c b/src/rewrite/pool_timestamp.c
index 45e7ea4..6d693ce 100644
--- a/src/rewrite/pool_timestamp.c
+++ b/src/rewrite/pool_timestamp.c
@@ -816,6 +816,27 @@ rewrite_timestamp(POOL_CONNECTION_POOL * backend, Node *node,
 	}
 	else if (IsA(node, CopyStmt) &&((CopyStmt *) node)->query != NULL)
 		stmt = ((CopyStmt *) node)->query;
+	else if (IsA(node, ExplainStmt))
+	{
+		ListCell   *lc;
+		bool        analyze = false;
+
+		/* Check to see if this is EXPLAIN ANALYZE */
+		foreach(lc, ((ExplainStmt *) node)->options)
+		{
+			DefElem    *opt = (DefElem *) lfirst(lc);
+
+			if (strcmp(opt->defname, "analyze") == 0)
+			{
+				stmt = ((ExplainStmt *) node)->query;
+				analyze = true;
+				break;
+			}
+		}
+
+		if (!analyze)
+			return NULL;
+	}
 	else
 		stmt = node;
 
@@ -932,6 +953,13 @@ rewrite_timestamp(POOL_CONNECTION_POOL * backend, Node *node,
 									   rewrite_timestamp_walker, (void *) &ctx);
 		}
 
+		if (s_stmt->withClause)
+		{
+			raw_expression_tree_walker(
+									   (Node *) s_stmt->withClause,
+									   rewrite_timestamp_walker, (void *) &ctx);
+		}
+
 		rewrite = ctx.rewrite;
 	}
 	else
rewrite_query_v3.patch (2,035 bytes)

nagata

2019-11-27 15:43

developer   ~0002978

Last edited: 2019-11-27 15:54

View 4 revisions

Although I didn't test, this seems good to me. Maybe it is better to add the comment for pool_timestamp.c. Currently, there is only the following and this doesn't give any explanation about CopyStmt or ExplainStmt.

 809 /*
 810 * Prepare?
 811 */

Also, I recommend to add test cases confirmed in this thread for 010.rewrite_timestamp.

Issue History

Date Modified Username Field Change
2019-10-08 15:22 nagata New Issue
2019-10-08 15:27 nagata Description Updated View Revisions
2019-10-09 08:43 pengbo Assigned To => pengbo
2019-10-09 08:43 pengbo Status new => assigned
2019-10-25 17:35 pengbo Note Added: 0002943
2019-10-25 17:37 pengbo Status assigned => feedback
2019-10-28 10:55 nagata Note Added: 0002944
2019-10-28 10:55 nagata Status feedback => assigned
2019-10-28 10:57 nagata Note Edited: 0002944 View Revisions
2019-10-29 17:34 pengbo Note Added: 0002946
2019-10-29 17:35 pengbo File Added: rewrite_query_v2.patch
2019-10-29 18:10 nagata Note Added: 0002947
2019-10-29 18:14 nagata Note Added: 0002948
2019-10-29 18:16 nagata Note Edited: 0002948 View Revisions
2019-10-29 18:46 nagata Note Added: 0002949
2019-10-29 18:53 nagata Note Edited: 0002949 View Revisions
2019-10-31 08:42 pengbo Note Added: 0002952
2019-11-15 16:14 pengbo File Added: rewrite_query_v3.patch
2019-11-15 16:14 pengbo Note Added: 0002962
2019-11-27 15:43 nagata Note Added: 0002978
2019-11-27 15:44 nagata Note Edited: 0002978 View Revisions
2019-11-27 15:50 nagata Note Edited: 0002978 View Revisions
2019-11-27 15:54 nagata Note Edited: 0002978 View Revisions