From ea5ae4cae6a230e048f0ff4587b54d441712c6fd Mon Sep 17 00:00:00 2001 From: Tom Lane Date: Fri, 16 Dec 2022 13:07:42 -0500 Subject: [PATCH] Fix inability to reference CYCLE column from inside its CTE. Such references failed with "cache lookup failed for type 0" because we didn't resolve the type of the CYCLE column until after analyzing the CTE's query. We can just move that processing to before the recursive parse_sub_analyze call, though. While here, invent a couple of local variables to make this code less egregiously wider-than-80-columns. Per bug #17723 from Vik Fearing. Back-patch to v14 where the CYCLE feature was added. Discussion: https://postgr.es/m/17723-2c4985ff111e7bba@postgresql.org --- src/backend/parser/parse_cte.c | 170 ++++++++++++++++------------- src/test/regress/expected/with.out | 23 ++++ src/test/regress/sql/with.sql | 9 ++ 3 files changed, 129 insertions(+), 73 deletions(-) diff --git a/src/backend/parser/parse_cte.c b/src/backend/parser/parse_cte.c index f6ae96333a..ac263af07e 100644 --- a/src/backend/parser/parse_cte.c +++ b/src/backend/parser/parse_cte.c @@ -241,10 +241,76 @@ static void analyzeCTE(ParseState *pstate, CommonTableExpr *cte) { Query *query; + CTESearchClause *search_clause = cte->search_clause; + CTECycleClause *cycle_clause = cte->cycle_clause; /* Analysis not done already */ Assert(!IsA(cte->ctequery, Query)); + /* + * Before analyzing the CTE's query, we'd better identify the data type of + * the cycle mark column if any, since the query could refer to that. + * Other validity checks on the cycle clause will be done afterwards. + */ + if (cycle_clause) + { + TypeCacheEntry *typentry; + Oid op; + + cycle_clause->cycle_mark_value = + transformExpr(pstate, cycle_clause->cycle_mark_value, + EXPR_KIND_CYCLE_MARK); + cycle_clause->cycle_mark_default = + transformExpr(pstate, cycle_clause->cycle_mark_default, + EXPR_KIND_CYCLE_MARK); + + cycle_clause->cycle_mark_type = + select_common_type(pstate, + list_make2(cycle_clause->cycle_mark_value, + cycle_clause->cycle_mark_default), + "CYCLE", NULL); + cycle_clause->cycle_mark_value = + coerce_to_common_type(pstate, + cycle_clause->cycle_mark_value, + cycle_clause->cycle_mark_type, + "CYCLE/SET/TO"); + cycle_clause->cycle_mark_default = + coerce_to_common_type(pstate, + cycle_clause->cycle_mark_default, + cycle_clause->cycle_mark_type, + "CYCLE/SET/DEFAULT"); + + cycle_clause->cycle_mark_typmod = + select_common_typmod(pstate, + list_make2(cycle_clause->cycle_mark_value, + cycle_clause->cycle_mark_default), + cycle_clause->cycle_mark_type); + + cycle_clause->cycle_mark_collation = + select_common_collation(pstate, + list_make2(cycle_clause->cycle_mark_value, + cycle_clause->cycle_mark_default), + true); + + /* Might as well look up the relevant <> operator while we are at it */ + typentry = lookup_type_cache(cycle_clause->cycle_mark_type, + TYPECACHE_EQ_OPR); + if (!OidIsValid(typentry->eq_opr)) + ereport(ERROR, + errcode(ERRCODE_UNDEFINED_FUNCTION), + errmsg("could not identify an equality operator for type %s", + format_type_be(cycle_clause->cycle_mark_type))); + op = get_negator(typentry->eq_opr); + if (!OidIsValid(op)) + ereport(ERROR, + errcode(ERRCODE_UNDEFINED_FUNCTION), + errmsg("could not identify an inequality operator for type %s", + format_type_be(cycle_clause->cycle_mark_type))); + + cycle_clause->cycle_mark_neop = op; + } + + /* Now we can get on with analyzing the CTE's query */ query = parse_sub_analyze(cte->ctequery, pstate, cte, false, true); cte->ctequery = (Node *) query; @@ -339,7 +405,10 @@ analyzeCTE(ParseState *pstate, CommonTableExpr *cte) elog(ERROR, "wrong number of output columns in WITH"); } - if (cte->search_clause || cte->cycle_clause) + /* + * Now make validity checks on the SEARCH and CYCLE clauses, if present. + */ + if (search_clause || cycle_clause) { Query *ctequery; SetOperationStmt *sos; @@ -386,12 +455,12 @@ analyzeCTE(ParseState *pstate, CommonTableExpr *cte) errmsg("with a SEARCH or CYCLE clause, the right side of the UNION must be a SELECT"))); } - if (cte->search_clause) + if (search_clause) { ListCell *lc; List *seen = NIL; - foreach(lc, cte->search_clause->search_col_list) + foreach(lc, search_clause->search_col_list) { Value *colname = lfirst(lc); @@ -400,33 +469,31 @@ analyzeCTE(ParseState *pstate, CommonTableExpr *cte) (errcode(ERRCODE_SYNTAX_ERROR), errmsg("search column \"%s\" not in WITH query column list", strVal(colname)), - parser_errposition(pstate, cte->search_clause->location))); + parser_errposition(pstate, search_clause->location))); if (list_member(seen, colname)) ereport(ERROR, (errcode(ERRCODE_DUPLICATE_COLUMN), errmsg("search column \"%s\" specified more than once", strVal(colname)), - parser_errposition(pstate, cte->search_clause->location))); + parser_errposition(pstate, search_clause->location))); seen = lappend(seen, colname); } - if (list_member(cte->ctecolnames, makeString(cte->search_clause->search_seq_column))) + if (list_member(cte->ctecolnames, makeString(search_clause->search_seq_column))) ereport(ERROR, errcode(ERRCODE_SYNTAX_ERROR), errmsg("search sequence column name \"%s\" already used in WITH query column list", - cte->search_clause->search_seq_column), - parser_errposition(pstate, cte->search_clause->location)); + search_clause->search_seq_column), + parser_errposition(pstate, search_clause->location)); } - if (cte->cycle_clause) + if (cycle_clause) { ListCell *lc; List *seen = NIL; - TypeCacheEntry *typentry; - Oid op; - foreach(lc, cte->cycle_clause->cycle_col_list) + foreach(lc, cycle_clause->cycle_col_list) { Value *colname = lfirst(lc); @@ -435,97 +502,54 @@ analyzeCTE(ParseState *pstate, CommonTableExpr *cte) (errcode(ERRCODE_SYNTAX_ERROR), errmsg("cycle column \"%s\" not in WITH query column list", strVal(colname)), - parser_errposition(pstate, cte->cycle_clause->location))); + parser_errposition(pstate, cycle_clause->location))); if (list_member(seen, colname)) ereport(ERROR, (errcode(ERRCODE_DUPLICATE_COLUMN), errmsg("cycle column \"%s\" specified more than once", strVal(colname)), - parser_errposition(pstate, cte->cycle_clause->location))); + parser_errposition(pstate, cycle_clause->location))); seen = lappend(seen, colname); } - if (list_member(cte->ctecolnames, makeString(cte->cycle_clause->cycle_mark_column))) + if (list_member(cte->ctecolnames, makeString(cycle_clause->cycle_mark_column))) ereport(ERROR, errcode(ERRCODE_SYNTAX_ERROR), errmsg("cycle mark column name \"%s\" already used in WITH query column list", - cte->cycle_clause->cycle_mark_column), - parser_errposition(pstate, cte->cycle_clause->location)); + cycle_clause->cycle_mark_column), + parser_errposition(pstate, cycle_clause->location)); - cte->cycle_clause->cycle_mark_value = transformExpr(pstate, cte->cycle_clause->cycle_mark_value, - EXPR_KIND_CYCLE_MARK); - cte->cycle_clause->cycle_mark_default = transformExpr(pstate, cte->cycle_clause->cycle_mark_default, - EXPR_KIND_CYCLE_MARK); - - if (list_member(cte->ctecolnames, makeString(cte->cycle_clause->cycle_path_column))) + if (list_member(cte->ctecolnames, makeString(cycle_clause->cycle_path_column))) ereport(ERROR, errcode(ERRCODE_SYNTAX_ERROR), errmsg("cycle path column name \"%s\" already used in WITH query column list", - cte->cycle_clause->cycle_path_column), - parser_errposition(pstate, cte->cycle_clause->location)); + cycle_clause->cycle_path_column), + parser_errposition(pstate, cycle_clause->location)); - if (strcmp(cte->cycle_clause->cycle_mark_column, - cte->cycle_clause->cycle_path_column) == 0) + if (strcmp(cycle_clause->cycle_mark_column, + cycle_clause->cycle_path_column) == 0) ereport(ERROR, errcode(ERRCODE_SYNTAX_ERROR), errmsg("cycle mark column name and cycle path column name are the same"), - parser_errposition(pstate, cte->cycle_clause->location)); - - cte->cycle_clause->cycle_mark_type = select_common_type(pstate, - list_make2(cte->cycle_clause->cycle_mark_value, - cte->cycle_clause->cycle_mark_default), - "CYCLE", NULL); - cte->cycle_clause->cycle_mark_value = coerce_to_common_type(pstate, - cte->cycle_clause->cycle_mark_value, - cte->cycle_clause->cycle_mark_type, - "CYCLE/SET/TO"); - cte->cycle_clause->cycle_mark_default = coerce_to_common_type(pstate, - cte->cycle_clause->cycle_mark_default, - cte->cycle_clause->cycle_mark_type, - "CYCLE/SET/DEFAULT"); - - cte->cycle_clause->cycle_mark_typmod = select_common_typmod(pstate, - list_make2(cte->cycle_clause->cycle_mark_value, - cte->cycle_clause->cycle_mark_default), - cte->cycle_clause->cycle_mark_type); - - cte->cycle_clause->cycle_mark_collation = select_common_collation(pstate, - list_make2(cte->cycle_clause->cycle_mark_value, - cte->cycle_clause->cycle_mark_default), - true); - - typentry = lookup_type_cache(cte->cycle_clause->cycle_mark_type, TYPECACHE_EQ_OPR); - if (!typentry->eq_opr) - ereport(ERROR, - errcode(ERRCODE_UNDEFINED_FUNCTION), - errmsg("could not identify an equality operator for type %s", - format_type_be(cte->cycle_clause->cycle_mark_type))); - op = get_negator(typentry->eq_opr); - if (!op) - ereport(ERROR, - errcode(ERRCODE_UNDEFINED_FUNCTION), - errmsg("could not identify an inequality operator for type %s", - format_type_be(cte->cycle_clause->cycle_mark_type))); - - cte->cycle_clause->cycle_mark_neop = op; + parser_errposition(pstate, cycle_clause->location)); } - if (cte->search_clause && cte->cycle_clause) + if (search_clause && cycle_clause) { - if (strcmp(cte->search_clause->search_seq_column, - cte->cycle_clause->cycle_mark_column) == 0) + if (strcmp(search_clause->search_seq_column, + cycle_clause->cycle_mark_column) == 0) ereport(ERROR, errcode(ERRCODE_SYNTAX_ERROR), errmsg("search sequence column name and cycle mark column name are the same"), - parser_errposition(pstate, cte->search_clause->location)); + parser_errposition(pstate, search_clause->location)); - if (strcmp(cte->search_clause->search_seq_column, - cte->cycle_clause->cycle_path_column) == 0) + if (strcmp(search_clause->search_seq_column, + cycle_clause->cycle_path_column) == 0) ereport(ERROR, errcode(ERRCODE_SYNTAX_ERROR), errmsg("search sequence column name and cycle path column name are the same"), - parser_errposition(pstate, cte->search_clause->location)); + parser_errposition(pstate, search_clause->location)); } } diff --git a/src/test/regress/expected/with.out b/src/test/regress/expected/with.out index 8e5e31a7be..5ccadb5042 100644 --- a/src/test/regress/expected/with.out +++ b/src/test/regress/expected/with.out @@ -1221,6 +1221,29 @@ select * from test; 0 | t | {(0),(1),(2),(3),(4),(5),(6),(7),(8),(9),(0)} (11 rows) +with recursive test as ( + select 0 as x + union all + select (x + 1) % 10 + from test + where not is_cycle -- redundant, but legal +) cycle x set is_cycle using path +select * from test; + x | is_cycle | path +---+----------+----------------------------------------------- + 0 | f | {(0)} + 1 | f | {(0),(1)} + 2 | f | {(0),(1),(2)} + 3 | f | {(0),(1),(2),(3)} + 4 | f | {(0),(1),(2),(3),(4)} + 5 | f | {(0),(1),(2),(3),(4),(5)} + 6 | f | {(0),(1),(2),(3),(4),(5),(6)} + 7 | f | {(0),(1),(2),(3),(4),(5),(6),(7)} + 8 | f | {(0),(1),(2),(3),(4),(5),(6),(7),(8)} + 9 | f | {(0),(1),(2),(3),(4),(5),(6),(7),(8),(9)} + 0 | t | {(0),(1),(2),(3),(4),(5),(6),(7),(8),(9),(0)} +(11 rows) + -- multiple CTEs with recursive graph(f, t, label) as ( diff --git a/src/test/regress/sql/with.sql b/src/test/regress/sql/with.sql index def3587d34..dc36b7ccb0 100644 --- a/src/test/regress/sql/with.sql +++ b/src/test/regress/sql/with.sql @@ -605,6 +605,15 @@ with recursive test as ( ) cycle x set is_cycle using path select * from test; +with recursive test as ( + select 0 as x + union all + select (x + 1) % 10 + from test + where not is_cycle -- redundant, but legal +) cycle x set is_cycle using path +select * from test; + -- multiple CTEs with recursive graph(f, t, label) as (