Received: from malur.postgresql.org ([217.196.149.56]) by arkaria.postgresql.org with esmtps (TLS1.3:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.92) (envelope-from ) id 1mhbAy-0007KI-5o for pgsql-hackers@arkaria.postgresql.org; Mon, 01 Nov 2021 17:32:52 +0000 Received: from localhost ([127.0.0.1] helo=malur.postgresql.org) by malur.postgresql.org with esmtp (Exim 4.92) (envelope-from ) id 1mhbAw-0002lp-Rb for pgsql-hackers@arkaria.postgresql.org; Mon, 01 Nov 2021 17:32:50 +0000 Received: from magus.postgresql.org ([2a02:c0:301:0:ffff::29]) by malur.postgresql.org with esmtps (TLS1.3:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.92) (envelope-from ) id 1mhbAw-0002lg-Is for pgsql-hackers@lists.postgresql.org; Mon, 01 Nov 2021 17:32:50 +0000 Received: from sss.pgh.pa.us ([66.207.139.130]) by magus.postgresql.org with esmtps (TLS1.3:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.92) (envelope-from ) id 1mhbAu-0004aa-4g for pgsql-hackers@postgresql.org; Mon, 01 Nov 2021 17:32:50 +0000 Received: from sss1.sss.pgh.pa.us (localhost [127.0.0.1]) by sss.pgh.pa.us (8.15.2/8.15.2) with ESMTP id 1A1HWfF31192216; Mon, 1 Nov 2021 13:32:41 -0400 From: Tom Lane To: "Bossart, Nathan" cc: Kyotaro Horiguchi , "michael@paquier.xyz" , "andres@anarazel.de" , "sulamul@gmail.com" , "pgsql-hackers@postgresql.org" Subject: Re: inefficient loop in StandbyReleaseLockList() 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> <1142780.1635785847@sss.pgh.pa.us> <696759A4-C422-4BB7-B99A-E3DDB9B3A970@amazon.com> Comments: In-reply-to "Bossart, Nathan" message dated "Mon, 01 Nov 2021 17:02:49 -0000" MIME-Version: 1.0 Content-Type: multipart/mixed; boundary="----- =_aaaaaaaaaa0" Content-ID: <1192159.1635787929.0@sss.pgh.pa.us> Date: Mon, 01 Nov 2021 13:32:41 -0400 Message-ID: <1192215.1635787961@sss.pgh.pa.us> List-Id: List-Help: List-Subscribe: List-Post: List-Owner: List-Archive: Archived-At: Precedence: bulk ------- =_aaaaaaaaaa0 Content-Type: text/plain; charset="us-ascii" Content-ID: <1192159.1635787929.1@sss.pgh.pa.us> "Bossart, Nathan" 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 ------- =_aaaaaaaaaa0 Content-Type: text/x-diff; name="fix-easy-list-delete-first-usages.patch"; charset="us-ascii" Content-ID: <1192159.1635787929.2@sss.pgh.pa.us> Content-Description: fix-easy-list-delete-first-usages.patch Content-Transfer-Encoding: quoted-printable 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 =3D 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 loo= p, + * too; this works as long as we don't do repeat fetches using the "lc" + * pointer. */ - while (trgmNFA->queue !=3D NIL) + foreach(lc, trgmNFA->queue) { - TrgmState *state =3D (TrgmState *) linitial(trgmNFA->queue); - - trgmNFA->queue =3D list_delete_first(trgmNFA->queue); + TrgmState *state =3D (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 =3D NIL; = /* * Add state's own key, and then process all keys added to keysQueue unt= il - * 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 !=3D NIL && !(state->flags & TSTATE_FIN)) + foreach(lc, trgmNFA->keysQueue) { - TrgmStateKey *key =3D (TrgmStateKey *) linitial(trgmNFA->keysQueue); + TrgmStateKey *key =3D (TrgmStateKey *) lfirst(lc); = - trgmNFA->keysQueue =3D 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 =3D 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 =3D=3D NIL) return false; = - batch =3D linitial(aggstate->hash_batches); - aggstate->hash_batches =3D list_delete_first(aggstate->hash_batches); + /* hash_batches is a stack, with the top item at the end of the list */ + batch =3D llast(aggstate->hash_batches); + aggstate->hash_batches =3D 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, HashAggSpil= l *spill, int setno) new_batch =3D hashagg_batch_new(tape, setno, spill->ntuples[i], cardinality, used_bits); - aggstate->hash_batches =3D lcons(new_batch, aggstate->hash_batches); + aggstate->hash_batches =3D lappend(aggstate->hash_batches, new_batch); aggstate->hash_batches_used++; } = @@ -3113,8 +3114,6 @@ hashagg_spill_finish(AggState *aggstate, HashAggSpil= l *spill, int setno) static void hashagg_reset_spill_state(AggState *aggstate) { - ListCell *lc; - /* free spills from initial pass */ if (aggstate->hash_spills !=3D NULL) { @@ -3132,13 +3131,7 @@ hashagg_reset_spill_state(AggState *aggstate) } = /* free batches */ - foreach(lc, aggstate->hash_batches) - { - HashAggBatch *batch =3D (HashAggBatch *) lfirst(lc); - - pfree(batch); - } - list_free(aggstate->hash_batches); + list_free_deep(aggstate->hash_batches); aggstate->hash_batches =3D 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 =3D (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 =3D NULL; } = - while (llvm_context->handles !=3D NIL) + foreach(lc, llvm_context->handles) { - LLVMJitHandle *jit_handle; - - jit_handle =3D (LLVMJitHandle *) linitial(llvm_context->handles); - llvm_context->handles =3D list_delete_first(llvm_context->handles); + LLVMJitHandle *jit_handle =3D (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 =3D NIL; } = /* ------- =_aaaaaaaaaa0--