Re: inefficient loop in StandbyReleaseLockList()

Поиск
Список
Период
Сортировка
От Tom Lane
Тема Re: inefficient loop in StandbyReleaseLockList()
Дата
Msg-id 1043766.1635713701@sss.pgh.pa.us
обсуждение исходный текст
Ответ на Re: inefficient loop in StandbyReleaseLockList()  (Tom Lane <tgl@sss.pgh.pa.us>)
Ответы Re: inefficient loop in StandbyReleaseLockList()  (Kyotaro Horiguchi <horikyota.ntt@gmail.com>)
Список pgsql-hackers
I wrote:
> Pushed the patch as given.  I've not yet reviewed other list_delete_first
> callers, but I'll take a look.  (I seem to remember that I did survey
> them while writing 1cff1b95a, but I evidently missed that this code
> could be dealing with a list long enough to be problematic.)

I looked at the remaining list_delete_first callers.

1. Attached is a proposed patch to get rid of the calls in trgm_regexp.c.
I'm not certain that those lists could get long enough to be a problem,
given the existing complexity limits in that file (MAX_EXPANDED_STATES
etc).  But I'm not certain they can't, either, and it's easy enough to
fix along the same lines as in StandbyReleaseLockList.

2. I think we almost certainly have a problem in SyncPostCheckpoint.

3. Is agg_refill_hash_table a problem?  Probably; we've seen cases
with lots of batches.

4. I'm a bit worried about the uses in access/gist/, but I don't know
that code well enough to want to mess with it.  It's possible the
list lengths are bounded by the index tree height, in which case it
likely doesn't matter.  The logic in gistFindPath looks like a mess
anyway since it's appending to both ends of the "fifo" list in different
places (is that really necessary?).

5. Not sure about CopyMultiInsertInfoFlush ... how many buffers
could we have there?

6. llvm_release_context may not have a long enough list to be a
problem, but on the other hand, it looks easy to fix.

7. The list lengths in the parser and dependency.c, ruleutils.c,
explain.c are bounded by subquery nesting depth or plan tree depth,
so I doubt it's worth worrying about.

8. The uses in namespace.c don't seem like an issue either -- for
instance, GetOverrideSearchPath can't iterate more than twice,
and the overrideStack list shouldn't get very deep.

            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)

В списке pgsql-hackers по дате отправления:

Предыдущее
От: Zhihong Yu
Дата:
Сообщение: small change to comment for ATExecDetachPartition
Следующее
От: Tomas Vondra
Дата:
Сообщение: Re: should we enable log_checkpoints out of the box?