public inbox for [email protected]
help / color / mirror / Atom feedFrom: Jelte Fennema <[email protected]>
To: PostgreSQL-development <[email protected]>
Subject: Add new for_each macros for iterating over a List that do not require ListCell pointer
Date: Tue, 24 Oct 2023 18:03:48 +0200
Message-ID: <CAGECzQSwXKnxGwW1_Q5JE+8Ja20kyAbhBHO04vVrQsLcDciwXA@mail.gmail.com> (raw)
Many usages of the foreach macro in the Postgres codebase only use the
ListCell variable to then get its value. This adds macros that
simplify iteration code for that very common use case. Instead of
passing a ListCell you can pass a variable of the type of its
contents. This IMHO improves readability of the code by reducing the
total amount of code while also essentially forcing the use of useful
variable names.
While this might seem like a small quality of life improvement, in
practice it turns out to be very nice to use. At Microsoft we have
been using macros very similar to these ones in the Citus codebase for
a long time now and we pretty much never use plain foreach anymore for
new code.
Finally, I guess there needs to be some bikeshedding on the naming. In
the Citus codebase we call them foreach_xyz instead of the
for_each_xyz naming pattern that is used in this patchset. I'm not
sure what the current stance is on if foreach should be written with
or without an underscore between for and each. Currently pg_list.h
uses both.
P.S. Similar macros for forboth/forthree are also possible, but
require an exponential macro count handle all different possibilities,
which might not be worth the effort since forboth/forthree are used
much less often than foreach. In Citus we do have 3 forboth macros
that don't require ListCell for the most common cases (foreach_ptr,
foreach_ptr_oid, foreach_int_oid). But I did not want to clutter this
patchset with that discussion.
Attachments:
[application/octet-stream] v1-0001-Add-macros-for-looping-through-a-list-without-nee.patch (4.6K, ../CAGECzQSwXKnxGwW1_Q5JE+8Ja20kyAbhBHO04vVrQsLcDciwXA@mail.gmail.com/2-v1-0001-Add-macros-for-looping-through-a-list-without-nee.patch)
download | inline diff:
From 0effd91ab9ee3b3c678f1d128e487b9a728c8588 Mon Sep 17 00:00:00 2001
From: Jelte Fennema-Nio <[email protected]>
Date: Tue, 24 Oct 2023 16:46:08 +0200
Subject: [PATCH v1 1/2] Add macros for looping through a list without needing
a ListCell
Many usages of the foreach macro only use the ListCell variable to get
its contents. This adds macros that simplify iteration code for that
common use case.
---
src/include/nodes/pg_list.h | 92 ++++++++++++++++++++++++++++++++++---
1 file changed, 86 insertions(+), 6 deletions(-)
diff --git a/src/include/nodes/pg_list.h b/src/include/nodes/pg_list.h
index 529a382d284..af1ecc5e394 100644
--- a/src/include/nodes/pg_list.h
+++ b/src/include/nodes/pg_list.h
@@ -383,13 +383,15 @@ lnext(const List *l, const ListCell *c)
* delete the current list element from the List associated with a
* surrounding foreach() loop, returning the new List pointer.
*
- * This is equivalent to list_delete_cell(), but it also adjusts the foreach
- * loop's state so that no list elements will be missed. Do not delete
- * elements from an active foreach loop's list in any other way!
+ * This is similar to list_delete_cell(), but it also works when using
+ * for_each_xyz macros that don't require passing in a "ListCell *".
+ * Furthermore it adjusts the foreach loop's state so that no list elements
+ * will be missed. Do not delete elements from an active foreach loop's list in
+ * any other way!
*/
-#define foreach_delete_current(lst, cell) \
- (cell##__state.i--, \
- (List *) (cell##__state.l = list_delete_cell(lst, cell)))
+#define foreach_delete_current(lst, var) \
+ (var##__state.i--, \
+ (List *) (var##__state.l = list_delete_cell(lst, &var##__state.l->elements[var##__state.i])))
/*
* foreach_current_index -
@@ -452,6 +454,84 @@ for_each_cell_setup(const List *lst, const ListCell *initcell)
return r;
}
+/*
+ * for_each_ptr -
+ * a convenience macro which loops through a list of pointers without
+ * needing a "ListCell *", just a declared pointer variable to store the
+ * current pointer int;
+ *
+ * The caveats for foreach() apply equally here.
+ */
+#define for_each_ptr(var, lst) \
+ for (ForEachState var##__state = {(lst), 0}; \
+ (var##__state.l != NIL && \
+ var##__state.i < var##__state.l->length) ? \
+ (var = lfirst(&var##__state.l->elements[var##__state.i]), true) : \
+ (var = NULL, false); \
+ var##__state.i++)
+
+/*
+ * for_each_int -
+ * a convenience macro which loops through a list of ints without needing a
+ * "ListCell *", just a declared int variable to store the current int in.
+ *
+ * The caveats for foreach() apply equally here.
+ */
+#define for_each_int(var, lst) \
+ for (ForEachState var##__state = {(lst), 0}; \
+ (var##__state.l != NIL && \
+ var##__state.i < var##__state.l->length) ? \
+ (var = lfirst_int(&var##__state.l->elements[var##__state.i]), true) : \
+ (var = 0, false); \
+ var##__state.i++)
+
+/*
+ * foreach_oid -
+ * a convenience macro which loops through an oid list without needing a
+ * "ListCell *", just a declared Oid variable to store the current oid in.
+ *
+ * The caveats for foreach() apply equally here.
+ */
+#define for_each_oid(var, lst) \
+ for (ForEachState var##__state = {(lst), 0}; \
+ (var##__state.l != NIL && \
+ var##__state.i < var##__state.l->length) ? \
+ (var = lfirst_oid(&var##__state.l->elements[var##__state.i]), true) : \
+ (var = 0, false); \
+ var##__state.i++)
+
+/*
+ * foreach_xid -
+ * a convenience macro which loops through an xid list without needing a
+ * "ListCell *", just a declared TransactionId variable to store the current
+ * xid in.
+ *
+ * The caveats for foreach() apply equally here.
+ */
+#define for_each_xid(var, lst) \
+ for (ForEachState var##__state = {(lst), 0}; \
+ (var##__state.l != NIL && \
+ var##__state.i < var##__state.l->length) ? \
+ (var = lfirst_xid(&var##__state.l->elements[var##__state.i]), true) : \
+ (var = 0, false); \
+ var##__state.i++)
+
+/*
+ * for_each_node -
+ * a convenience macro which loops through a node list without needing a
+ * "ListCell *", just a declared pointer variable to store the pointer of
+ * the current node in.
+ *
+ * The caveats for foreach() apply equally here.
+ */
+#define for_each_node(type, var, lst) \
+ for (ForEachState var##__state = {(lst), 0}; \
+ (var##__state.l != NIL && \
+ var##__state.i < var##__state.l->length) ? \
+ (var = lfirst_node(type, &var##__state.l->elements[var##__state.i]), true) : \
+ (var = 0, false); \
+ var##__state.i++)
+
/*
* forboth -
* a convenience macro for advancing through two linked lists
base-commit: 00d7fb5e2e39198387ae00af8dd18b787b6a4d63
--
2.34.1
[application/octet-stream] v1-0002-Use-new-for_each_xyz-macros-in-a-few-places.patch (7.3K, ../CAGECzQSwXKnxGwW1_Q5JE+8Ja20kyAbhBHO04vVrQsLcDciwXA@mail.gmail.com/3-v1-0002-Use-new-for_each_xyz-macros-in-a-few-places.patch)
download | inline diff:
From 12bce0f7d0e999f048b1847d3d8513ea2daaa1ed Mon Sep 17 00:00:00 2001
From: Jelte Fennema-Nio <[email protected]>
Date: Tue, 24 Oct 2023 17:13:20 +0200
Subject: [PATCH v1 2/2] Use new for_each_xyz macros in a few places
This starts using each of the newly added for_each_xyz macros in at
least one place. This shows how they improve readability by reducing the
amount of boilerplate code.
---
src/backend/executor/execExpr.c | 11 ++++----
src/backend/executor/nodeSubplan.c | 28 +++++++++------------
src/backend/replication/logical/relation.c | 5 ++--
src/backend/replication/logical/tablesync.c | 6 ++---
src/backend/replication/pgoutput/pgoutput.c | 8 +++---
5 files changed, 25 insertions(+), 33 deletions(-)
diff --git a/src/backend/executor/execExpr.c b/src/backend/executor/execExpr.c
index 2c62b0c9c84..e73261ec445 100644
--- a/src/backend/executor/execExpr.c
+++ b/src/backend/executor/execExpr.c
@@ -216,7 +216,8 @@ ExecInitQual(List *qual, PlanState *parent)
ExprState *state;
ExprEvalStep scratch = {0};
List *adjust_jumps = NIL;
- ListCell *lc;
+ Expr *node;
+ int jump;
/* short-circuit (here and in ExecQual) for empty restriction list */
if (qual == NIL)
@@ -250,10 +251,8 @@ ExecInitQual(List *qual, PlanState *parent)
scratch.resvalue = &state->resvalue;
scratch.resnull = &state->resnull;
- foreach(lc, qual)
+ for_each_ptr(node, qual)
{
- Expr *node = (Expr *) lfirst(lc);
-
/* first evaluate expression */
ExecInitExprRec(node, state, &state->resvalue, &state->resnull);
@@ -265,9 +264,9 @@ ExecInitQual(List *qual, PlanState *parent)
}
/* adjust jump targets */
- foreach(lc, adjust_jumps)
+ for_each_int(jump, adjust_jumps)
{
- ExprEvalStep *as = &state->steps[lfirst_int(lc)];
+ ExprEvalStep *as = &state->steps[jump];
Assert(as->opcode == EEOP_QUAL);
Assert(as->d.qualexpr.jumpdone == -1);
diff --git a/src/backend/executor/nodeSubplan.c b/src/backend/executor/nodeSubplan.c
index c136f75ac24..8f88f289269 100644
--- a/src/backend/executor/nodeSubplan.c
+++ b/src/backend/executor/nodeSubplan.c
@@ -307,7 +307,7 @@ ExecScanSubPlan(SubPlanState *node,
Datum rowresult;
bool rownull;
int col;
- ListCell *plst;
+ int paramid;
if (subLinkType == EXISTS_SUBLINK)
{
@@ -367,9 +367,8 @@ ExecScanSubPlan(SubPlanState *node,
* Now set all the setParam params from the columns of the tuple
*/
col = 1;
- foreach(plst, subplan->setParam)
+ for_each_int(paramid, subplan->setParam)
{
- int paramid = lfirst_int(plst);
ParamExecData *prmdata;
prmdata = &(econtext->ecxt_param_exec_vals[paramid]);
@@ -412,9 +411,8 @@ ExecScanSubPlan(SubPlanState *node,
* combining expression.
*/
col = 1;
- foreach(plst, subplan->paramIds)
+ for_each_int(paramid, subplan->paramIds)
{
- int paramid = lfirst_int(plst);
ParamExecData *prmdata;
prmdata = &(econtext->ecxt_param_exec_vals[paramid]);
@@ -480,10 +478,11 @@ ExecScanSubPlan(SubPlanState *node,
}
else if (subLinkType == MULTIEXPR_SUBLINK)
{
+ int paramid;
+
/* We don't care about function result, but set the setParams */
- foreach(l, subplan->setParam)
+ for_each_int(paramid, subplan->setParam)
{
- int paramid = lfirst_int(l);
ParamExecData *prmdata;
prmdata = &(econtext->ecxt_param_exec_vals[paramid]);
@@ -604,16 +603,15 @@ buildSubPlanHash(SubPlanState *node, ExprContext *econtext)
slot = ExecProcNode(planstate))
{
int col = 1;
- ListCell *plst;
bool isnew;
+ int paramid;
/*
* Load up the Params representing the raw sub-select outputs, then
* form the projection tuple to store in the hashtable.
*/
- foreach(plst, subplan->paramIds)
+ for_each_int(paramid, subplan->paramIds)
{
- int paramid = lfirst_int(plst);
ParamExecData *prmdata;
prmdata = &(innerecontext->ecxt_param_exec_vals[paramid]);
@@ -880,11 +878,10 @@ ExecInitSubPlan(SubPlan *subplan, PlanState *parent)
if (subplan->setParam != NIL && subplan->parParam == NIL &&
subplan->subLinkType != CTE_SUBLINK)
{
- ListCell *lst;
+ int paramid;
- foreach(lst, subplan->setParam)
+ for_each_int(paramid, subplan->setParam)
{
- int paramid = lfirst_int(lst);
ParamExecData *prm = &(estate->es_param_exec_vals[paramid]);
prm->execPlan = sstate;
@@ -906,7 +903,7 @@ ExecInitSubPlan(SubPlan *subplan, PlanState *parent)
List *oplist,
*lefttlist,
*righttlist;
- ListCell *l;
+ OpExpr *opexpr;
/* We need a memory context to hold the hash table(s) */
sstate->hashtablecxt =
@@ -966,9 +963,8 @@ ExecInitSubPlan(SubPlan *subplan, PlanState *parent)
cross_eq_funcoids = (Oid *) palloc(ncols * sizeof(Oid));
i = 1;
- foreach(l, oplist)
+ for_each_node(OpExpr, opexpr, oplist)
{
- OpExpr *opexpr = lfirst_node(OpExpr, l);
Expr *expr;
TargetEntry *tle;
Oid rhs_eq_oper;
diff --git a/src/backend/replication/logical/relation.c b/src/backend/replication/logical/relation.c
index d62eefed138..f8faceb57ce 100644
--- a/src/backend/replication/logical/relation.c
+++ b/src/backend/replication/logical/relation.c
@@ -747,11 +747,10 @@ static Oid
FindUsableIndexForReplicaIdentityFull(Relation localrel, AttrMap *attrmap)
{
List *idxlist = RelationGetIndexList(localrel);
- ListCell *lc;
+ Oid idxoid;
- foreach(lc, idxlist)
+ for_each_oid(idxoid, idxlist)
{
- Oid idxoid = lfirst_oid(lc);
bool isUsableIdx;
Relation idxRel;
IndexInfo *idxInfo;
diff --git a/src/backend/replication/logical/tablesync.c b/src/backend/replication/logical/tablesync.c
index 37a0abe2f4d..c82d05a642e 100644
--- a/src/backend/replication/logical/tablesync.c
+++ b/src/backend/replication/logical/tablesync.c
@@ -416,7 +416,7 @@ process_syncing_tables_for_apply(XLogRecPtr current_lsn)
TimestampTz last_start_time;
};
static HTAB *last_start_times = NULL;
- ListCell *lc;
+ SubscriptionRelState *rstate;
bool started_tx = false;
bool should_exit = false;
@@ -453,10 +453,8 @@ process_syncing_tables_for_apply(XLogRecPtr current_lsn)
/*
* Process all tables that are being synchronized.
*/
- foreach(lc, table_states_not_ready)
+ for_each_ptr(rstate, table_states_not_ready)
{
- SubscriptionRelState *rstate = (SubscriptionRelState *) lfirst(lc);
-
if (rstate->state == SUBREL_STATE_SYNCDONE)
{
/*
diff --git a/src/backend/replication/pgoutput/pgoutput.c b/src/backend/replication/pgoutput/pgoutput.c
index c1c66848f36..ec6df59bedc 100644
--- a/src/backend/replication/pgoutput/pgoutput.c
+++ b/src/backend/replication/pgoutput/pgoutput.c
@@ -2229,7 +2229,7 @@ cleanup_rel_sync_cache(TransactionId xid, bool is_commit)
{
HASH_SEQ_STATUS hash_seq;
RelationSyncEntry *entry;
- ListCell *lc;
+ TransactionId streamed_txn;
Assert(RelationSyncCache != NULL);
@@ -2242,15 +2242,15 @@ cleanup_rel_sync_cache(TransactionId xid, bool is_commit)
* corresponding schema and we don't need to send it unless there is
* any invalidation for that relation.
*/
- foreach(lc, entry->streamed_txns)
+ for_each_xid(streamed_txn, entry->streamed_txns)
{
- if (xid == lfirst_xid(lc))
+ if (xid == streamed_txn)
{
if (is_commit)
entry->schema_sent = true;
entry->streamed_txns =
- foreach_delete_current(entry->streamed_txns, lc);
+ foreach_delete_current(entry->streamed_txns, streamed_txn);
break;
}
}
--
2.34.1
view thread (5+ messages) latest in thread
reply
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: [email protected]
Cc: [email protected]
Subject: Re: Add new for_each macros for iterating over a List that do not require ListCell pointer
In-Reply-To: <CAGECzQSwXKnxGwW1_Q5JE+8Ja20kyAbhBHO04vVrQsLcDciwXA@mail.gmail.com>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox