From 4efb4f0d48787fbdf42c0b760f19c997664f0d56 Mon Sep 17 00:00:00 2001 From: Tom Lane Date: Fri, 17 Feb 2023 16:40:34 -0500 Subject: [PATCH] Print the correct aliases for DML target tables in ruleutils. ruleutils.c blindly printed the user-given alias (or nothing if there hadn't been one) for the target table of INSERT/UPDATE/DELETE queries. That works a large percentage of the time, but not always: for queries appearing in WITH, it's possible that we chose a different alias to avoid conflict with outer-scope names. Since the chosen alias would be used in any Var references to the target table, this'd lead to an inconsistent printout with consequences such as dump/restore failures. The correct logic for printing (or not) a relation alias was embedded in get_from_clause_item. Factor it out to a separate function so that we don't need a jointree node to use it. (Only a limited part of that function can be reached from these new call sites, but this seems like the cleanest non-duplicative factorization.) In passing, I got rid of a redundant "\d+ rules_src" step in rules.sql. Initial report from Jonathan Katz; thanks to Vignesh C for analysis. This has been broken for a long time, so back-patch to all supported branches. Discussion: https://postgr.es/m/e947fa21-24b2-f922-375a-d4f763ef3e4b@postgresql.org Discussion: https://postgr.es/m/CALDaNm1MMntjmT_NJGp-Z=xbF02qHGAyuSHfYHias3TqQbPF2w@mail.gmail.com --- src/backend/utils/adt/ruleutils.c | 145 ++++++++++++++++------------ src/test/regress/expected/rules.out | 47 +++++---- src/test/regress/sql/rules.sql | 13 ++- 3 files changed, 126 insertions(+), 79 deletions(-) diff --git a/src/backend/utils/adt/ruleutils.c b/src/backend/utils/adt/ruleutils.c index 4aa2747c62..2388f76070 100644 --- a/src/backend/utils/adt/ruleutils.c +++ b/src/backend/utils/adt/ruleutils.c @@ -463,6 +463,8 @@ static void get_from_clause(Query *query, const char *prefix, deparse_context *context); static void get_from_clause_item(Node *jtnode, Query *query, deparse_context *context); +static void get_rte_alias(RangeTblEntry *rte, int varno, bool use_as, + deparse_context *context); static void get_column_alias_list(deparse_columns *colinfo, deparse_context *context); static void get_from_clause_coldeflist(RangeTblFunction *rtfunc, @@ -6206,12 +6208,14 @@ get_insert_query_def(Query *query, deparse_context *context, context->indentLevel += PRETTYINDENT_STD; appendStringInfoChar(buf, ' '); } - appendStringInfo(buf, "INSERT INTO %s ", + appendStringInfo(buf, "INSERT INTO %s", generate_relation_name(rte->relid, NIL)); - /* INSERT requires AS keyword for target alias */ - if (rte->alias != NULL) - appendStringInfo(buf, "AS %s ", - quote_identifier(rte->alias->aliasname)); + + /* Print the relation alias, if needed; INSERT requires explicit AS */ + get_rte_alias(rte, query->resultRelation, true, context); + + /* always want a space here */ + appendStringInfoChar(buf, ' '); /* * Add the insert-column-names list. Any indirection decoration needed on @@ -6393,9 +6397,10 @@ get_update_query_def(Query *query, deparse_context *context, appendStringInfo(buf, "UPDATE %s%s", only_marker(rte), generate_relation_name(rte->relid, NIL)); - if (rte->alias != NULL) - appendStringInfo(buf, " %s", - quote_identifier(rte->alias->aliasname)); + + /* Print the relation alias, if needed */ + get_rte_alias(rte, query->resultRelation, false, context); + appendStringInfoString(buf, " SET "); /* Deparse targetlist */ @@ -6601,9 +6606,9 @@ get_delete_query_def(Query *query, deparse_context *context, appendStringInfo(buf, "DELETE FROM %s%s", only_marker(rte), generate_relation_name(rte->relid, NIL)); - if (rte->alias != NULL) - appendStringInfo(buf, " %s", - quote_identifier(rte->alias->aliasname)); + + /* Print the relation alias, if needed */ + get_rte_alias(rte, query->resultRelation, false, context); /* Add the USING clause if given */ get_from_clause(query, " USING ", context); @@ -10179,10 +10184,8 @@ get_from_clause_item(Node *jtnode, Query *query, deparse_context *context) { int varno = ((RangeTblRef *) jtnode)->rtindex; RangeTblEntry *rte = rt_fetch(varno, query->rtable); - char *refname = get_rtable_name(varno, context); deparse_columns *colinfo = deparse_columns_fetch(varno, dpns); RangeTblFunction *rtfunc1 = NULL; - bool printalias; if (rte->lateral) appendStringInfoString(buf, "LATERAL "); @@ -10319,54 +10322,7 @@ get_from_clause_item(Node *jtnode, Query *query, deparse_context *context) } /* Print the relation alias, if needed */ - printalias = false; - if (rte->alias != NULL) - { - /* Always print alias if user provided one */ - printalias = true; - } - else if (colinfo->printaliases) - { - /* Always print alias if we need to print column aliases */ - printalias = true; - } - else if (rte->rtekind == RTE_RELATION) - { - /* - * No need to print alias if it's same as relation name (this - * would normally be the case, but not if set_rtable_names had to - * resolve a conflict). - */ - if (strcmp(refname, get_relation_name(rte->relid)) != 0) - printalias = true; - } - else if (rte->rtekind == RTE_FUNCTION) - { - /* - * For a function RTE, always print alias. This covers possible - * renaming of the function and/or instability of the - * FigureColname rules for things that aren't simple functions. - * Note we'd need to force it anyway for the columndef list case. - */ - printalias = true; - } - else if (rte->rtekind == RTE_VALUES) - { - /* Alias is syntactically required for VALUES */ - printalias = true; - } - else if (rte->rtekind == RTE_CTE) - { - /* - * No need to print alias if it's same as CTE name (this would - * normally be the case, but not if set_rtable_names had to - * resolve a conflict). - */ - if (strcmp(refname, rte->ctename) != 0) - printalias = true; - } - if (printalias) - appendStringInfo(buf, " %s", quote_identifier(refname)); + get_rte_alias(rte, varno, false, context); /* Print the column definitions or aliases, if needed */ if (rtfunc1 && rtfunc1->funccolnames != NIL) @@ -10500,6 +10456,73 @@ get_from_clause_item(Node *jtnode, Query *query, deparse_context *context) (int) nodeTag(jtnode)); } +/* + * get_rte_alias - print the relation's alias, if needed + * + * If printed, the alias is preceded by a space, or by " AS " if use_as is true. + */ +static void +get_rte_alias(RangeTblEntry *rte, int varno, bool use_as, + deparse_context *context) +{ + deparse_namespace *dpns = (deparse_namespace *) linitial(context->namespaces); + char *refname = get_rtable_name(varno, context); + deparse_columns *colinfo = deparse_columns_fetch(varno, dpns); + bool printalias = false; + + if (rte->alias != NULL) + { + /* Always print alias if user provided one */ + printalias = true; + } + else if (colinfo->printaliases) + { + /* Always print alias if we need to print column aliases */ + printalias = true; + } + else if (rte->rtekind == RTE_RELATION) + { + /* + * No need to print alias if it's same as relation name (this would + * normally be the case, but not if set_rtable_names had to resolve a + * conflict). + */ + if (strcmp(refname, get_relation_name(rte->relid)) != 0) + printalias = true; + } + else if (rte->rtekind == RTE_FUNCTION) + { + /* + * For a function RTE, always print alias. This covers possible + * renaming of the function and/or instability of the FigureColname + * rules for things that aren't simple functions. Note we'd need to + * force it anyway for the columndef list case. + */ + printalias = true; + } + else if (rte->rtekind == RTE_SUBQUERY || + rte->rtekind == RTE_VALUES) + { + /* Alias is syntactically required for SUBQUERY and VALUES */ + printalias = true; + } + else if (rte->rtekind == RTE_CTE) + { + /* + * No need to print alias if it's same as CTE name (this would + * normally be the case, but not if set_rtable_names had to resolve a + * conflict). + */ + if (strcmp(refname, rte->ctename) != 0) + printalias = true; + } + + if (printalias) + appendStringInfo(context->buf, "%s%s", + use_as ? " AS " : " ", + quote_identifier(refname)); +} + /* * get_column_alias_list - print column alias list for an RTE * diff --git a/src/test/regress/expected/rules.out b/src/test/regress/expected/rules.out index 48ec02c0e5..011eb56932 100644 --- a/src/test/regress/expected/rules.out +++ b/src/test/regress/expected/rules.out @@ -3036,28 +3036,21 @@ select * from rules_log; (16 rows) create rule r4 as on delete to rules_src do notify rules_src_deletion; -\d+ rules_src - Table "public.rules_src" - Column | Type | Collation | Nullable | Default | Storage | Stats target | Description ---------+---------+-----------+----------+---------+---------+--------------+------------- - f1 | integer | | | | plain | | - f2 | integer | | | 0 | plain | | -Rules: - r1 AS - ON UPDATE TO rules_src DO INSERT INTO rules_log (f1, f2, tag, id) VALUES (old.f1,old.f2,'old'::text,DEFAULT), (new.f1,new.f2,'new'::text,DEFAULT) - r2 AS - ON UPDATE TO rules_src DO VALUES (old.f1,old.f2,'old'::text), (new.f1,new.f2,'new'::text) - r3 AS - ON INSERT TO rules_src DO INSERT INTO rules_log (f1, f2, tag, id) VALUES (NULL::integer,NULL::integer,'-'::text,DEFAULT), (new.f1,new.f2,'new'::text,DEFAULT) - r4 AS - ON DELETE TO rules_src DO - NOTIFY rules_src_deletion - -- -- Ensure an aliased target relation for insert is correctly deparsed. -- create rule r5 as on insert to rules_src do instead insert into rules_log AS trgt SELECT NEW.* RETURNING trgt.f1, trgt.f2; create rule r6 as on update to rules_src do instead UPDATE rules_log AS trgt SET tag = 'updated' WHERE trgt.f1 = new.f1; +-- +-- Check deparse disambiguation of INSERT/UPDATE/DELETE targets. +-- +create rule r7 as on delete to rules_src do instead + with wins as (insert into int4_tbl as trgt values (0) returning *), + wupd as (update int4_tbl trgt set f1 = f1+1 returning *), + wdel as (delete from int4_tbl trgt where f1 = 0 returning *) + insert into rules_log AS trgt select old.* from wins, wupd, wdel + returning trgt.f1, trgt.f2; +-- check display of all rules added above \d+ rules_src Table "public.rules_src" Column | Type | Collation | Nullable | Default | Storage | Stats target | Description @@ -3082,6 +3075,26 @@ Rules: r6 AS ON UPDATE TO rules_src DO INSTEAD UPDATE rules_log trgt SET tag = 'updated'::text WHERE trgt.f1 = new.f1 + r7 AS + ON DELETE TO rules_src DO INSTEAD WITH wins AS ( + INSERT INTO int4_tbl AS trgt_1 (f1) + VALUES (0) + RETURNING trgt_1.f1 + ), wupd AS ( + UPDATE int4_tbl trgt_1 SET f1 = trgt_1.f1 + 1 + RETURNING trgt_1.f1 + ), wdel AS ( + DELETE FROM int4_tbl trgt_1 + WHERE trgt_1.f1 = 0 + RETURNING trgt_1.f1 + ) + INSERT INTO rules_log AS trgt (f1, f2) SELECT old.f1, + old.f2 + FROM wins, + wupd, + wdel + RETURNING trgt.f1, + trgt.f2 -- -- Also check multiassignment deparsing. diff --git a/src/test/regress/sql/rules.sql b/src/test/regress/sql/rules.sql index 236217e979..288a5e86af 100644 --- a/src/test/regress/sql/rules.sql +++ b/src/test/regress/sql/rules.sql @@ -1034,13 +1034,24 @@ insert into rules_src values(22,23), (33,default); select * from rules_src; select * from rules_log; create rule r4 as on delete to rules_src do notify rules_src_deletion; -\d+ rules_src -- -- Ensure an aliased target relation for insert is correctly deparsed. -- create rule r5 as on insert to rules_src do instead insert into rules_log AS trgt SELECT NEW.* RETURNING trgt.f1, trgt.f2; create rule r6 as on update to rules_src do instead UPDATE rules_log AS trgt SET tag = 'updated' WHERE trgt.f1 = new.f1; + +-- +-- Check deparse disambiguation of INSERT/UPDATE/DELETE targets. +-- +create rule r7 as on delete to rules_src do instead + with wins as (insert into int4_tbl as trgt values (0) returning *), + wupd as (update int4_tbl trgt set f1 = f1+1 returning *), + wdel as (delete from int4_tbl trgt where f1 = 0 returning *) + insert into rules_log AS trgt select old.* from wins, wupd, wdel + returning trgt.f1, trgt.f2; + +-- check display of all rules added above \d+ rules_src --