From 56fe008996bc1a547ce60c8dddd2ca821cac163e Mon Sep 17 00:00:00 2001 From: Tom Lane Date: Mon, 28 Sep 2020 20:32:53 -0400 Subject: [PATCH] Add for_each_from, to simplify loops starting from non-first list cells. We have a dozen or so places that need to iterate over all but the first cell of a List. Prior to v13 this was typically written as for_each_cell(lc, lnext(list_head(list))) Commit 1cff1b95a changed these to for_each_cell(lc, list, list_second_cell(list)) This patch introduces a new macro for_each_from() which expresses the start point as a list index, allowing these to be written as for_each_from(lc, list, 1) This is marginally more efficient, since ForEachState.i can be initialized directly instead of backing into it from a ListCell address. It also seems clearer and less typo-prone. Some of the remaining uses of for_each_cell() look like they could profitably be changed to for_each_from(), but here I confined myself to changing uses of list_second_cell(). Also, fix for_each_cell_setup() and for_both_cell_setup() to const-ify their arguments; that's a simple oversight in 1cff1b95a. Back-patch into v13, on the grounds that (1) the const-ification is a minor bug fix, and (2) it's better for back-patching purposes if we only have two ways to write these loops rather than three. In HEAD, also remove list_third_cell() and list_fourth_cell(), which were also introduced in 1cff1b95a, and are unused as of cc99baa43. It seems unlikely that any third-party code would have started to use them already; anyone who has can be directed to list_nth_cell instead. Discussion: https://postgr.es/m/CAApHDvpo1zj9KhEpU2cCRZfSM3Q6XGdhzuAS2v79PH7WJBkYVA@mail.gmail.com --- src/backend/commands/tablecmds.c | 2 +- src/backend/nodes/nodeFuncs.c | 4 +- src/backend/optimizer/plan/createplan.c | 2 +- src/backend/optimizer/plan/planner.c | 4 +- src/backend/parser/parse_agg.c | 4 +- src/backend/utils/adt/jsonpath_gram.y | 2 +- src/backend/utils/adt/ruleutils.c | 8 ++-- src/backend/utils/adt/selfuncs.c | 2 +- src/include/nodes/pg_list.h | 52 ++++++++++++++----------- 9 files changed, 42 insertions(+), 38 deletions(-) diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c index 16285ad09f..e0ac4e05e5 100644 --- a/src/backend/commands/tablecmds.c +++ b/src/backend/commands/tablecmds.c @@ -5732,7 +5732,7 @@ ATCheckPartitionsNotInUse(Relation rel, LOCKMODE lockmode) inh = find_all_inheritors(RelationGetRelid(rel), lockmode, NULL); /* first element is the parent rel; must ignore it */ - for_each_cell(cell, inh, list_second_cell(inh)) + for_each_from(cell, inh, 1) { Relation childrel; diff --git a/src/backend/nodes/nodeFuncs.c b/src/backend/nodes/nodeFuncs.c index 9ce8f43385..1dc873ed25 100644 --- a/src/backend/nodes/nodeFuncs.c +++ b/src/backend/nodes/nodeFuncs.c @@ -441,7 +441,7 @@ exprTypmod(const Node *expr) typmod = exprTypmod((Node *) linitial(cexpr->args)); if (typmod < 0) return -1; /* no point in trying harder */ - for_each_cell(arg, cexpr->args, list_second_cell(cexpr->args)) + for_each_from(arg, cexpr->args, 1) { Node *e = (Node *) lfirst(arg); @@ -469,7 +469,7 @@ exprTypmod(const Node *expr) typmod = exprTypmod((Node *) linitial(mexpr->args)); if (typmod < 0) return -1; /* no point in trying harder */ - for_each_cell(arg, mexpr->args, list_second_cell(mexpr->args)) + for_each_from(arg, mexpr->args, 1) { Node *e = (Node *) lfirst(arg); diff --git a/src/backend/optimizer/plan/createplan.c b/src/backend/optimizer/plan/createplan.c index 99278eed93..3d7a4e373f 100644 --- a/src/backend/optimizer/plan/createplan.c +++ b/src/backend/optimizer/plan/createplan.c @@ -2261,7 +2261,7 @@ create_groupingsets_plan(PlannerInfo *root, GroupingSetsPath *best_path) { bool is_first_sort = ((RollupData *) linitial(rollups))->is_hashed; - for_each_cell(lc, rollups, list_second_cell(rollups)) + for_each_from(lc, rollups, 1) { RollupData *rollup = lfirst(lc); AttrNumber *new_grpColIdx; diff --git a/src/backend/optimizer/plan/planner.c b/src/backend/optimizer/plan/planner.c index 3e2b4965c4..f331f82a6c 100644 --- a/src/backend/optimizer/plan/planner.c +++ b/src/backend/optimizer/plan/planner.c @@ -4430,7 +4430,7 @@ consider_groupingsets_paths(PlannerInfo *root, * below, must use the same condition. */ i = 0; - for_each_cell(lc, gd->rollups, list_second_cell(gd->rollups)) + for_each_from(lc, gd->rollups, 1) { RollupData *rollup = lfirst_node(RollupData, lc); @@ -4464,7 +4464,7 @@ consider_groupingsets_paths(PlannerInfo *root, rollups = list_make1(linitial(gd->rollups)); i = 0; - for_each_cell(lc, gd->rollups, list_second_cell(gd->rollups)) + for_each_from(lc, gd->rollups, 1) { RollupData *rollup = lfirst_node(RollupData, lc); diff --git a/src/backend/parser/parse_agg.c b/src/backend/parser/parse_agg.c index f813b587f1..783f3fe8f2 100644 --- a/src/backend/parser/parse_agg.c +++ b/src/backend/parser/parse_agg.c @@ -1083,7 +1083,7 @@ parseCheckAggregates(ParseState *pstate, Query *qry) if (gset_common) { - for_each_cell(l, gsets, list_second_cell(gsets)) + for_each_from(l, gsets, 1) { gset_common = list_intersection_int(gset_common, lfirst(l)); if (!gset_common) @@ -1774,7 +1774,7 @@ expand_grouping_sets(List *groupingSets, int limit) result = lappend(result, list_union_int(NIL, (List *) lfirst(lc))); } - for_each_cell(lc, expanded_groups, list_second_cell(expanded_groups)) + for_each_from(lc, expanded_groups, 1) { List *p = lfirst(lc); List *new_result = NIL; diff --git a/src/backend/utils/adt/jsonpath_gram.y b/src/backend/utils/adt/jsonpath_gram.y index 88ef9550e9..53f422260c 100644 --- a/src/backend/utils/adt/jsonpath_gram.y +++ b/src/backend/utils/adt/jsonpath_gram.y @@ -441,7 +441,7 @@ makeItemList(List *list) while (end->next) end = end->next; - for_each_cell(cell, list, list_second_cell(list)) + for_each_from(cell, list, 1) { JsonPathParseItem *c = (JsonPathParseItem *) lfirst(cell); diff --git a/src/backend/utils/adt/ruleutils.c b/src/backend/utils/adt/ruleutils.c index 03cf241996..62023c20b2 100644 --- a/src/backend/utils/adt/ruleutils.c +++ b/src/backend/utils/adt/ruleutils.c @@ -8113,7 +8113,7 @@ get_rule_expr(Node *node, deparse_context *context, { BoolExpr *expr = (BoolExpr *) node; Node *first_arg = linitial(expr->args); - ListCell *arg = list_second_cell(expr->args); + ListCell *arg; switch (expr->boolop) { @@ -8122,12 +8122,11 @@ get_rule_expr(Node *node, deparse_context *context, appendStringInfoChar(buf, '('); get_rule_expr_paren(first_arg, context, false, node); - while (arg) + for_each_from(arg, expr->args, 1) { appendStringInfoString(buf, " AND "); get_rule_expr_paren((Node *) lfirst(arg), context, false, node); - arg = lnext(expr->args, arg); } if (!PRETTY_PAREN(context)) appendStringInfoChar(buf, ')'); @@ -8138,12 +8137,11 @@ get_rule_expr(Node *node, deparse_context *context, appendStringInfoChar(buf, '('); get_rule_expr_paren(first_arg, context, false, node); - while (arg) + for_each_from(arg, expr->args, 1) { appendStringInfoString(buf, " OR "); get_rule_expr_paren((Node *) lfirst(arg), context, false, node); - arg = lnext(expr->args, arg); } if (!PRETTY_PAREN(context)) appendStringInfoChar(buf, ')'); diff --git a/src/backend/utils/adt/selfuncs.c b/src/backend/utils/adt/selfuncs.c index 00c7afc66f..bec357fcef 100644 --- a/src/backend/utils/adt/selfuncs.c +++ b/src/backend/utils/adt/selfuncs.c @@ -3519,7 +3519,7 @@ estimate_num_groups(PlannerInfo *root, List *groupExprs, double input_rows, * for remaining Vars on other rels. */ relvarinfos = lappend(relvarinfos, varinfo1); - for_each_cell(l, varinfos, list_second_cell(varinfos)) + for_each_from(l, varinfos, 1) { GroupVarInfo *varinfo2 = (GroupVarInfo *) lfirst(l); diff --git a/src/include/nodes/pg_list.h b/src/include/nodes/pg_list.h index 104df4174a..ec231010ce 100644 --- a/src/include/nodes/pg_list.h +++ b/src/include/nodes/pg_list.h @@ -144,26 +144,6 @@ list_second_cell(const List *l) return NULL; } -/* Fetch address of list's third cell, if it has one, else NULL */ -static inline ListCell * -list_third_cell(const List *l) -{ - if (l && l->length >= 3) - return &l->elements[2]; - else - return NULL; -} - -/* Fetch address of list's fourth cell, if it has one, else NULL */ -static inline ListCell * -list_fourth_cell(const List *l) -{ - if (l && l->length >= 4) - return &l->elements[3]; - else - return NULL; -} - /* Fetch list's length */ static inline int list_length(const List *l) @@ -389,6 +369,32 @@ lnext(const List *l, const ListCell *c) */ #define foreach_current_index(cell) (cell##__state.i) +/* + * for_each_from - + * Like foreach(), but start from the N'th (zero-based) list element, + * not necessarily the first one. + * + * It's okay for N to exceed the list length, but not for it to be negative. + * + * The caveats for foreach() apply equally here. + */ +#define for_each_from(cell, lst, N) \ + for (ForEachState cell##__state = for_each_from_setup(lst, N); \ + (cell##__state.l != NIL && \ + cell##__state.i < cell##__state.l->length) ? \ + (cell = &cell##__state.l->elements[cell##__state.i], true) : \ + (cell = NULL, false); \ + cell##__state.i++) + +static inline ForEachState +for_each_from_setup(const List *lst, int N) +{ + ForEachState r = {lst, N}; + + Assert(N >= 0); + return r; +} + /* * for_each_cell - * a convenience macro which loops through a list starting from a @@ -405,7 +411,7 @@ lnext(const List *l, const ListCell *c) cell##__state.i++) static inline ForEachState -for_each_cell_setup(List *lst, ListCell *initcell) +for_each_cell_setup(const List *lst, const ListCell *initcell) { ForEachState r = {lst, initcell ? list_cell_number(lst, initcell) : list_length(lst)}; @@ -456,8 +462,8 @@ for_each_cell_setup(List *lst, ListCell *initcell) cell1##__state.i1++, cell1##__state.i2++) static inline ForBothCellState -for_both_cell_setup(List *list1, ListCell *initcell1, - List *list2, ListCell *initcell2) +for_both_cell_setup(const List *list1, const ListCell *initcell1, + const List *list2, const ListCell *initcell2) { ForBothCellState r = {list1, list2, initcell1 ? list_cell_number(list1, initcell1) : list_length(list1),