From b1f2aab7fe99f7578c17a820448ce7ee1e86d313 Mon Sep 17 00:00:00 2001 From: Michael Paquier Date: Fri, 20 Oct 2023 10:31:46 +0900 Subject: [PATCH] Remove pg_hint_plan.hints_anywhere This GUC setting was a wart, able to bypass a check to force hints to happen before and/or after a set of characters. A weird behavior that it caused is that it was possible to define a hint within what should be considered a value in a query. For example, a query like that would use a hint: SELECT * FROM t1 JOIN t2 ON t1.val = t2.val WHERE '/*+MergeJoin(t1 t2)*/' <> ''; The reason why this parameter existed to begin with was to be more liberal with the position of a hint, because pg_hint_plan has always used a custom parser for query lookups. An upcoming patch will be able to solve these problems by introducing a proper query parser in this extension, based on what upstream uses for psql with yyac. Note that this GUC was not documented, as well, remaining hidden to the end-user. --- Makefile | 2 +- expected/hints_anywhere.out | 83 ------------------------------------- pg_hint_plan.c | 64 +++++++++++----------------- sql/hints_anywhere.sql | 24 ----------- 4 files changed, 26 insertions(+), 147 deletions(-) delete mode 100644 expected/hints_anywhere.out delete mode 100644 sql/hints_anywhere.sql diff --git a/Makefile b/Makefile index 3ccccfd4..7b068de7 100644 --- a/Makefile +++ b/Makefile @@ -8,7 +8,7 @@ MODULES = pg_hint_plan HINTPLANVER = 1.7.0 REGRESS = init base_plan pg_hint_plan ut-init ut-A ut-S ut-J ut-L ut-G ut-R \ - ut-fdw ut-W ut-T ut-fini hints_anywhere plpgsql oldextversions + ut-fdw ut-W ut-T ut-fini plpgsql oldextversions REGRESS_OPTS = --encoding=UTF8 EXTENSION = pg_hint_plan diff --git a/expected/hints_anywhere.out b/expected/hints_anywhere.out deleted file mode 100644 index e126e759..00000000 --- a/expected/hints_anywhere.out +++ /dev/null @@ -1,83 +0,0 @@ -LOAD 'pg_hint_plan'; -SET client_min_messages TO log; -\set SHOW_CONTEXT always -SET pg_hint_plan.debug_print TO on; -explain (costs false) -select * from t1 join t2 on t1.id = t2.id where '/*+HashJoin(t1 t2)*/' <> ''; - QUERY PLAN --------------------------------------- - Merge Join - Merge Cond: (t1.id = t2.id) - -> Index Scan using t1_pkey on t1 - -> Index Scan using t2_pkey on t2 -(4 rows) - -set pg_hint_plan.hints_anywhere = on; -explain (costs false) -select * from t1 join t2 on t1.id = t2.id where '/*+HashJoin(t1 t2)*/' <> ''; -LOG: pg_hint_plan: -used hint: -HashJoin(t1 t2) -not used hint: -duplication hint: -error hint: - - QUERY PLAN ------------------------------- - Hash Join - Hash Cond: (t1.id = t2.id) - -> Seq Scan on t1 - -> Hash - -> Seq Scan on t2 -(5 rows) - -set pg_hint_plan.hints_anywhere = off; -explain (costs false) -select * from t1 join t2 on t1.id = t2.id where '/*+HashJoin(t1 t2)*/' <> ''; - QUERY PLAN --------------------------------------- - Merge Join - Merge Cond: (t1.id = t2.id) - -> Index Scan using t1_pkey on t1 - -> Index Scan using t2_pkey on t2 -(4 rows) - -set pg_hint_plan.hints_anywhere = on; -/*+ MergeJoin(t1 t2) */ -explain (costs false) -select * from t1 join t2 on t1.val = t2.val where '/*+HashJoin(t1 t2)*/' <> ''; -LOG: pg_hint_plan: -used hint: -MergeJoin(t1 t2) -not used hint: -duplication hint: -error hint: - - QUERY PLAN -------------------------------------------- - Merge Join - Merge Cond: (t2.val = t1.val) - -> Index Scan using t2_val on t2 - -> Materialize - -> Index Scan using t1_val on t1 -(5 rows) - -/*+ HashJoin(t1 t2) */ -explain (costs false) -select * from t1 join t2 on t1.val = t2.val where '/*+MergeJoin(t1 t2)*/' <> ''; -LOG: pg_hint_plan: -used hint: -HashJoin(t1 t2) -not used hint: -duplication hint: -error hint: - - QUERY PLAN --------------------------------- - Hash Join - Hash Cond: (t2.val = t1.val) - -> Seq Scan on t2 - -> Hash - -> Seq Scan on t1 -(5 rows) - diff --git a/pg_hint_plan.c b/pg_hint_plan.c index 69a17292..fe35cd09 100644 --- a/pg_hint_plan.c +++ b/pg_hint_plan.c @@ -522,7 +522,6 @@ static int pg_hint_plan_parse_message_level = INFO; static int pg_hint_plan_debug_message_level = LOG; /* Default is off, to keep backward compatibility. */ static bool pg_hint_plan_enable_hint_table = false; -static bool pg_hint_plan_hints_anywhere = false; static int plpgsql_recurse_level = 0; /* PLpgSQL recursion level */ static int recurse_level = 0; /* recursion level incl. direct SPI calls */ @@ -773,17 +772,6 @@ _PG_init(void) assign_enable_hint_table, NULL); - DefineCustomBoolVariable("pg_hint_plan.hints_anywhere", - "Read hints from anywhere in a query.", - "This option lets pg_hint_plan ignore syntax so be cautious for false reads.", - &pg_hint_plan_hints_anywhere, - false, - PGC_USERSET, - 0, - NULL, - NULL, - NULL); - EmitWarningsOnPlaceholders("pg_hint_plan"); /* Install hooks. */ @@ -1973,34 +1961,32 @@ get_hints_from_comment(const char *p) hint_head = strstr(p, HINT_START); if (hint_head == NULL) return NULL; - if (!pg_hint_plan_hints_anywhere) + + for (;p < hint_head; p++) { - for (;p < hint_head; p++) - { - /* - * Allow these characters precedes hint comment: - * - digits - * - alphabets which are in ASCII range - * - space, tabs and new-lines - * - underscores, for identifier - * - commas, for SELECT clause, EXPLAIN and PREPARE - * - parentheses, for EXPLAIN and PREPARE - * - squared brackets, for arrays (like arguments of PREPARE - * statements). - * - * Note that we don't use isalpha() nor isalnum() in ctype.h here to - * avoid behavior which depends on locale setting. - */ - if (!(*p >= '0' && *p <= '9') && - !(*p >= 'A' && *p <= 'Z') && - !(*p >= 'a' && *p <= 'z') && - !isspace(*p) && - *p != '_' && - *p != ',' && - *p != '(' && *p != ')' && - *p != '[' && *p != ']') - return NULL; - } + /* + * Allow these characters precedes hint comment: + * - digits + * - alphabets which are in ASCII range + * - space, tabs and new-lines + * - underscores, for identifier + * - commas, for SELECT clause, EXPLAIN and PREPARE + * - parentheses, for EXPLAIN and PREPARE + * - squared brackets, for arrays (like arguments of PREPARE + * statements). + * + * Note that we don't use isalpha() nor isalnum() in ctype.h here to + * avoid behavior which depends on locale setting. + */ + if (!(*p >= '0' && *p <= '9') && + !(*p >= 'A' && *p <= 'Z') && + !(*p >= 'a' && *p <= 'z') && + !isspace(*p) && + *p != '_' && + *p != ',' && + *p != '(' && *p != ')' && + *p != '[' && *p != ']') + return NULL; } head = (char *)hint_head; diff --git a/sql/hints_anywhere.sql b/sql/hints_anywhere.sql deleted file mode 100644 index b567df36..00000000 --- a/sql/hints_anywhere.sql +++ /dev/null @@ -1,24 +0,0 @@ -LOAD 'pg_hint_plan'; -SET client_min_messages TO log; -\set SHOW_CONTEXT always -SET pg_hint_plan.debug_print TO on; - -explain (costs false) -select * from t1 join t2 on t1.id = t2.id where '/*+HashJoin(t1 t2)*/' <> ''; - -set pg_hint_plan.hints_anywhere = on; -explain (costs false) -select * from t1 join t2 on t1.id = t2.id where '/*+HashJoin(t1 t2)*/' <> ''; - -set pg_hint_plan.hints_anywhere = off; -explain (costs false) -select * from t1 join t2 on t1.id = t2.id where '/*+HashJoin(t1 t2)*/' <> ''; - -set pg_hint_plan.hints_anywhere = on; -/*+ MergeJoin(t1 t2) */ -explain (costs false) -select * from t1 join t2 on t1.val = t2.val where '/*+HashJoin(t1 t2)*/' <> ''; - -/*+ HashJoin(t1 t2) */ -explain (costs false) -select * from t1 join t2 on t1.val = t2.val where '/*+MergeJoin(t1 t2)*/' <> '';