View Issue Details
| ID | Project | Category | View Status | Date Submitted | Last Update |
|---|---|---|---|---|---|
| 0000551 | Pgpool-II | Bug | public | 2019-10-08 15:22 | 2020-03-06 14:53 |
| Reporter | nagata | Assigned To | pengbo | ||
| Priority | normal | Severity | minor | Reproducibility | always |
| Status | closed | Resolution | open | ||
| Target Version | 4.1.1 | Fixed in Version | 4.1.1 | ||
| Summary | 0000551: Some cases where now() is not rewritten in native replication mode | ||||
| Description | I 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. | ||||
| Tags | No tags attached. | ||||
|
|
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 |
|
|
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? |
|
|
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. |
|
|
|
|
|
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. |
|
|
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()); |
|
|
> 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.... |
|
|
Thank you. I will fix CTEs rewriting. |
|
|
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; ------------- |
|
|
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. |
|
|
Patch is committed. I will add more regression tests. Close issue. |
|
|
Thank you. Patch is committed. I will add more regression tests. Close issue. |
| Date Modified | Username | Field | Change |
|---|---|---|---|
| 2019-10-08 15:22 | nagata | New Issue | |
| 2019-10-08 15:27 | nagata | Description Updated | |
| 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 | |
| 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 | |
| 2019-10-29 18:46 | nagata | Note Added: 0002949 | |
| 2019-10-29 18:53 | nagata | Note Edited: 0002949 | |
| 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 | |
| 2019-11-27 15:50 | nagata | Note Edited: 0002978 | |
| 2019-11-27 15:54 | nagata | Note Edited: 0002978 | |
| 2020-03-06 14:52 | administrator | Note Added: 0003249 | |
| 2020-03-06 14:52 | pengbo | Note Added: 0003250 | |
| 2020-03-06 14:53 | pengbo | Status | assigned => closed |
| 2020-03-06 14:53 | pengbo | Fixed in Version | => 4.1.1 |
| 2020-03-06 14:53 | pengbo | Target Version | => 4.1.1 |
