Re: Small improvements to pg_list.h's linitial(), lsecond(), lthird() etc macros - Mailing list pgsql-hackers

I wrote:
> list_second_cell() does have uses, although I observe that they
> are almost exclusively in locutions such as
>     for_each_cell(lc, rollups, list_second_cell(rollups))
> to iterate over all-but-the-first elements of a list.  I wonder if
> we ought to come up with a better notation for that.  I'm imagining
> something like
>     for_each_from(lc, rollups, 1)
> to start from list index 1.  It looks like this would be marginally
> more efficient, and perhaps more readable.

Concretely, I'm thinking about the attached.  This does seem simpler.
It reduces the size of the executable by about 560 bytes on
my machine, or 46 bytes per usage, which isn't bad.  (Note: this
is in an assert-enabled build, might be different otherwise.)
I didn't try to measure performance changes, but it should be for
the better.

Looking at the remaining instances of for_each_cell, I see several
where it seems like it'd be simpler and clearer to use for_each_from.
But for the moment I confined myself to changing just the instances
following the pattern above.

I noticed while messing with this that I'd neglected to const-ify
the support functions for for_each_cell() and for_both_cell(),
so this fixes that too.

I'm somewhat inclined to back-patch this into v13.  The missing
const decoration seems arguably a bug, which we've missed noticing
only because of our generally lamentable under-usage of const.
And I think it'll be helpful for future back-patching if
for_each_from is available in all versions with the new List API.

            regards, tom lane

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..eefe705c39 100644
--- a/src/include/nodes/pg_list.h
+++ b/src/include/nodes/pg_list.h
@@ -389,6 +389,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 +431,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 +482,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),