Re: inefficient loop in StandbyReleaseLockList()

"Bossart, Nathan" <bossartn@amazon.com> writes:
> Ah, I see it now.  The patch looks good to me, then.

Thanks for looking!  Here's an expanded patch that also takes care
of the other two easy-to-fix cases, nodeAgg.c and llvmjit.c.
AFAICS, llvm_release_context is like StandbyReleaseLockList
in that we don't need to worry about whether the data structure
is valid after an error partway through.  (Maybe we should be
worrying, but I think the callers would need work as well if
that's to be the standard.)

            regards, tom lane

diff --git a/contrib/pg_trgm/trgm_regexp.c b/contrib/pg_trgm/trgm_regexp.c
index 64816dd370..1887a39161 100644
--- a/contrib/pg_trgm/trgm_regexp.c
+++ b/contrib/pg_trgm/trgm_regexp.c
@@ -907,6 +907,7 @@ transformGraph(TrgmNFA *trgmNFA)
     HASHCTL        hashCtl;
     TrgmStateKey initkey;
     TrgmState  *initstate;
+    ListCell   *lc;

     /* Initialize this stage's workspace in trgmNFA struct */
     trgmNFA->queue = NIL;
@@ -937,12 +938,13 @@ transformGraph(TrgmNFA *trgmNFA)
     /*
      * Recursively build the expanded graph by processing queue of states
      * (breadth-first search).  getState already put initstate in the queue.
+     * Note that getState will append new states to the queue within the loop,
+     * too; this works as long as we don't do repeat fetches using the "lc"
+     * pointer.
      */
-    while (trgmNFA->queue != NIL)
+    foreach(lc, trgmNFA->queue)
     {
-        TrgmState  *state = (TrgmState *) linitial(trgmNFA->queue);
-
-        trgmNFA->queue = list_delete_first(trgmNFA->queue);
+        TrgmState  *state = (TrgmState *) lfirst(lc);

         /*
          * If we overflowed then just mark state as final.  Otherwise do
@@ -966,22 +968,29 @@ transformGraph(TrgmNFA *trgmNFA)
 static void
 processState(TrgmNFA *trgmNFA, TrgmState *state)
 {
+    ListCell   *lc;
+
     /* keysQueue should be NIL already, but make sure */
     trgmNFA->keysQueue = NIL;

     /*
      * Add state's own key, and then process all keys added to keysQueue until
-     * queue is empty.  But we can quit if the state gets marked final.
+     * queue is finished.  But we can quit if the state gets marked final.
      */
     addKey(trgmNFA, state, &state->stateKey);
-    while (trgmNFA->keysQueue != NIL && !(state->flags & TSTATE_FIN))
+    foreach(lc, trgmNFA->keysQueue)
     {
-        TrgmStateKey *key = (TrgmStateKey *) linitial(trgmNFA->keysQueue);
+        TrgmStateKey *key = (TrgmStateKey *) lfirst(lc);

-        trgmNFA->keysQueue = list_delete_first(trgmNFA->keysQueue);
+        if (state->flags & TSTATE_FIN)
+            break;
         addKey(trgmNFA, state, key);
     }

+    /* Release keysQueue to clean up for next cycle */
+    list_free(trgmNFA->keysQueue);
+    trgmNFA->keysQueue = NIL;
+
     /*
      * Add outgoing arcs only if state isn't final (we have no interest in
      * outgoing arcs if we already match)
diff --git a/src/backend/executor/nodeAgg.c b/src/backend/executor/nodeAgg.c
index c99a0de4dd..f5a187cae3 100644
--- a/src/backend/executor/nodeAgg.c
+++ b/src/backend/executor/nodeAgg.c
@@ -2584,8 +2584,9 @@ agg_refill_hash_table(AggState *aggstate)
     if (aggstate->hash_batches == NIL)
         return false;

-    batch = linitial(aggstate->hash_batches);
-    aggstate->hash_batches = list_delete_first(aggstate->hash_batches);
+    /* hash_batches is a stack, with the top item at the end of the list */
+    batch = llast(aggstate->hash_batches);
+    aggstate->hash_batches = list_delete_last(aggstate->hash_batches);

     hash_agg_set_limits(aggstate->hashentrysize, batch->input_card,
                         batch->used_bits, &aggstate->hash_mem_limit,
@@ -3098,7 +3099,7 @@ hashagg_spill_finish(AggState *aggstate, HashAggSpill *spill, int setno)
         new_batch = hashagg_batch_new(tape, setno,
                                       spill->ntuples[i], cardinality,
                                       used_bits);
-        aggstate->hash_batches = lcons(new_batch, aggstate->hash_batches);
+        aggstate->hash_batches = lappend(aggstate->hash_batches, new_batch);
         aggstate->hash_batches_used++;
     }

@@ -3113,8 +3114,6 @@ hashagg_spill_finish(AggState *aggstate, HashAggSpill *spill, int setno)
 static void
 hashagg_reset_spill_state(AggState *aggstate)
 {
-    ListCell   *lc;
-
     /* free spills from initial pass */
     if (aggstate->hash_spills != NULL)
     {
@@ -3132,13 +3131,7 @@ hashagg_reset_spill_state(AggState *aggstate)
     }

     /* free batches */
-    foreach(lc, aggstate->hash_batches)
-    {
-        HashAggBatch *batch = (HashAggBatch *) lfirst(lc);
-
-        pfree(batch);
-    }
-    list_free(aggstate->hash_batches);
+    list_free_deep(aggstate->hash_batches);
     aggstate->hash_batches = NIL;

     /* close tape set */
diff --git a/src/backend/jit/llvm/llvmjit.c b/src/backend/jit/llvm/llvmjit.c
index 169dad96d7..fb29449573 100644
--- a/src/backend/jit/llvm/llvmjit.c
+++ b/src/backend/jit/llvm/llvmjit.c
@@ -171,6 +171,7 @@ static void
 llvm_release_context(JitContext *context)
 {
     LLVMJitContext *llvm_context = (LLVMJitContext *) context;
+    ListCell   *lc;

     /*
      * When this backend is exiting, don't clean up LLVM. As an error might
@@ -188,12 +189,9 @@ llvm_release_context(JitContext *context)
         llvm_context->module = NULL;
     }

-    while (llvm_context->handles != NIL)
+    foreach(lc, llvm_context->handles)
     {
-        LLVMJitHandle *jit_handle;
-
-        jit_handle = (LLVMJitHandle *) linitial(llvm_context->handles);
-        llvm_context->handles = list_delete_first(llvm_context->handles);
+        LLVMJitHandle *jit_handle = (LLVMJitHandle *) lfirst(lc);

 #if LLVM_VERSION_MAJOR > 11
         {
@@ -221,6 +219,8 @@ llvm_release_context(JitContext *context)

         pfree(jit_handle);
     }
+    list_free(llvm_context->handles);
+    llvm_context->handles = NIL;
 }

 /*