Track unowned relations in doubly-linked list

Relations dropped in a single transaction are tracked in a list of
unowned relations.  With large number of dropped relations this resulted
in poor performance at the end of a transaction, when the relations are
removed from the singly linked list one by one.

Commit b4166911 attempted to address this issue (particularly when it
happens during recovery) by removing the relations in a reverse order,
resulting in O(1) lookups in the list of unowned relations.  This did
not work reliably, though, and it was possible to trigger the O(N^2)
behavior in various ways.

Instead of trying to remove the relations in a specific order with
respect to the linked list, which seems rather fragile, switch to a
regular doubly linked.  That allows us to remove relations cheaply no
matter where in the list they are.

As b4166911 was a bugfix, backpatched to all supported versions, do the
same thing here.

Reviewed-by: Alvaro Herrera
Discussion: https://www.postgresql.org/message-id/flat/80c27103-99e4-1d0c-642c-d9f3b94aaa0a%402ndquadrant.com
Backpatch-through: 9.4

diff --git a/src/backend/storage/smgr/md.c b/src/backend/storage/smgr/md.c
index d08e837c9311cb4289f10625f40d85f9eaa4fe0c..d2a9dfd8b28845ca383ff55e737888f606951150 100644 (file)
--- a/src/backend/storage/smgr/md.c
+++ b/src/backend/storage/smgr/md.c
@@ -1744,13 +1744,7 @@ DropRelationFiles(RelFileNode *delrels, int ndelrels, bool isRedo)
 
        smgrdounlinkall(srels, ndelrels, isRedo);
 
-       /*
-        * Call smgrclose() in reverse order as when smgropen() is called.
-        * This trick enables remove_from_unowned_list() in smgrclose()
-        * to search the SMgrRelation from the unowned list,
-        * with O(1) performance.
-        */
-       for (i = ndelrels - 1; i >= 0; i--)
+       for (i = 0; i < ndelrels; i++)
                smgrclose(srels[i]);
        pfree(srels);
 }

diff --git a/src/backend/storage/smgr/smgr.c b/src/backend/storage/smgr/smgr.c
index 0ca095c4d60f99e7356ba3e25138fabffc4f2650..00e1969411e618be6fde65ee8e962aa921192448 100644 (file)
--- a/src/backend/storage/smgr/smgr.c
+++ b/src/backend/storage/smgr/smgr.c
@@ -18,6 +18,7 @@
 #include "postgres.h"
 
 #include "commands/tablespace.h"
+#include "lib/ilist.h"
 #include "storage/bufmgr.h"
 #include "storage/ipc.h"
 #include "storage/smgr.h"
@@ -82,12 +83,10 @@ static const int NSmgr = lengthof(smgrsw);
  */
 static HTAB *SMgrRelationHash = NULL;
 
-static SMgrRelation first_unowned_reln = NULL;
+static dlist_head      unowned_relns;
 
 /* local function prototypes */
 static void smgrshutdown(int code, Datum arg);
-static void add_to_unowned_list(SMgrRelation reln);
-static void remove_from_unowned_list(SMgrRelation reln);
 
 
 /*
@@ -150,7 +149,7 @@ smgropen(RelFileNode rnode, BackendId backend)
                ctl.entrysize = sizeof(SMgrRelationData);
                SMgrRelationHash = hash_create("smgr relation table", 400,
                                                                           &ctl, HASH_ELEM | HASH_BLOBS);
-               first_unowned_reln = NULL;
+               dlist_init(&unowned_relns);
        }
 
        /* Look up or create an entry */
@@ -177,7 +176,7 @@ smgropen(RelFileNode rnode, BackendId backend)
                        reln->md_num_open_segs[forknum] = 0;
 
                /* it has no owner yet */
-               add_to_unowned_list(reln);
+               dlist_push_tail(&unowned_relns, &reln->node);
        }
 
        return reln;
@@ -207,7 +206,7 @@ smgrsetowner(SMgrRelation *owner, SMgrRelation reln)
        if (reln->smgr_owner)
                *(reln->smgr_owner) = NULL;
        else
-               remove_from_unowned_list(reln);
+               dlist_delete(&reln->node);
 
        /* Now establish the ownership relationship. */
        reln->smgr_owner = owner;
@@ -231,53 +230,8 @@ smgrclearowner(SMgrRelation *owner, SMgrRelation reln)
        /* unset our reference to the owner */
        reln->smgr_owner = NULL;
 
-       add_to_unowned_list(reln);
-}
-
-/*
- * add_to_unowned_list -- link an SMgrRelation onto the unowned list
- *
- * Check remove_from_unowned_list()'s comments for performance
- * considerations.
- */
-static void
-add_to_unowned_list(SMgrRelation reln)
-{
-       /* place it at head of the list (to make smgrsetowner cheap) */
-       reln->next_unowned_reln = first_unowned_reln;
-       first_unowned_reln = reln;
-}
-
-/*
- * remove_from_unowned_list -- unlink an SMgrRelation from the unowned list
- *
- * If the reln is not present in the list, nothing happens.  Typically this
- * would be caller error, but there seems no reason to throw an error.
- *
- * In the worst case this could be rather slow; but in all the cases that seem
- * likely to be performance-critical, the reln being sought will actually be
- * first in the list.  Furthermore, the number of unowned relns touched in any
- * one transaction shouldn't be all that high typically.  So it doesn't seem
- * worth expending the additional space and management logic needed for a
- * doubly-linked list.
- */
-static void
-remove_from_unowned_list(SMgrRelation reln)
-{
-       SMgrRelation *link;
-       SMgrRelation cur;
-
-       for (link = &first_unowned_reln, cur = *link;
-                cur != NULL;
-                link = &cur->next_unowned_reln, cur = *link)
-       {
-               if (cur == reln)
-               {
-                       *link = cur->next_unowned_reln;
-                       cur->next_unowned_reln = NULL;
-                       break;
-               }
-       }
+       /* add to list of unowned relations */
+       dlist_push_tail(&unowned_relns, &reln->node);
 }
 
 /*
@@ -304,7 +258,7 @@ smgrclose(SMgrRelation reln)
        owner = reln->smgr_owner;
 
        if (!owner)
-               remove_from_unowned_list(reln);
+               dlist_delete(&reln->node);
 
        if (hash_search(SMgrRelationHash,
                                        (void *) &(reln->smgr_rnode),
@@ -797,13 +751,19 @@ smgrpostckpt(void)
 void
 AtEOXact_SMgr(void)
 {
+       dlist_mutable_iter      iter;
+
        /*
         * Zap all unowned SMgrRelations.  We rely on smgrclose() to remove each
         * one from the list.
         */
-       while (first_unowned_reln != NULL)
+       dlist_foreach_modify(iter, &unowned_relns)
        {
-               Assert(first_unowned_reln->smgr_owner == NULL);
-               smgrclose(first_unowned_reln);
+               SMgrRelation    rel = dlist_container(SMgrRelationData, node,
+                                                                                         iter.cur);
+
+               Assert(rel->smgr_owner == NULL);
+
+               smgrclose(rel);
        }
 }

diff --git a/src/include/storage/smgr.h b/src/include/storage/smgr.h
index 03eabd3c06bf836da21a97af7899c5f9f0a51876..b3e9e4056c3d8e7afd3c833cae46c0eff7f2b002 100644 (file)
--- a/src/include/storage/smgr.h
+++ b/src/include/storage/smgr.h
@@ -15,6 +15,7 @@
 #define SMGR_H
 
 #include "fmgr.h"
+#include "lib/ilist.h"
 #include "storage/block.h"
 #include "storage/relfilenode.h"
 
@@ -72,7 +73,7 @@ typedef struct SMgrRelationData
        struct _MdfdVec *md_seg_fds[MAX_FORKNUM + 1];
 
        /* if unowned, list link in list of all unowned SMgrRelations */
-       struct SMgrRelationData *next_unowned_reln;
+       dlist_node      node;
 } SMgrRelationData;
 
 typedef SMgrRelationData *SMgrRelation;