From: Tom Lane <tgl@sss.pgh.pa.us>
To: Bossart, Nathan <bossartn@amazon.com>
Cc: Kyotaro Horiguchi <horikyota.ntt@gmail.com>
Cc: michael@paquier.xyz <michael@paquier.xyz>
Cc: andres@anarazel.de <andres@anarazel.de>
Cc: sulamul@gmail.com <sulamul@gmail.com>
Cc: pgsql-hackers@postgresql.org <pgsql-hackers@postgresql.org>
Subject: Re: inefficient loop in StandbyReleaseLockList()
Date: Mon, 01 Nov 2021 13:32:41 -0400
Message-ID: <1192215.1635787961@sss.pgh.pa.us> (raw)
In-Reply-To: <696759A4-C422-4BB7-B99A-E3DDB9B3A970@amazon.com>
References: <0108A03D-FF2B-4CBC-933B-E047CD975158@amazon.com>
<1013797.1635709095@sss.pgh.pa.us>
<1043766.1635713701@sss.pgh.pa.us>
<20211101.114019.1470121542110379535.horikyota.ntt@gmail.com>
<1138901.1635782315@sss.pgh.pa.us>
<C84534A6-2E66-47FF-B193-EBB566544D49@amazon.com>
<1142780.1635785847@sss.pgh.pa.us>
<696759A4-C422-4BB7-B99A-E3DDB9B3A970@amazon.com>
"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
Attachments:fix-easy-list-delete-first-usages.patch (text/x-diff, 4.9K) downloaddiff --git a/contrib/pg_trgm/trgm_regexp.c b/contrib/pg_trgm/trgm_regexp.cindex 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.cindex 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.cindex 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;
}
/*
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: pgsql-hackers@postgresql.org
Cc: tgl@sss.pgh.pa.us, bossartn@amazon.com, horikyota.ntt@gmail.com, michael@paquier.xyz, andres@anarazel.de, sulamul@gmail.com
Subject: Re: inefficient loop in StandbyReleaseLockList()
In-Reply-To: <1192215.1635787961@sss.pgh.pa.us>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox