Thread: fixing dllist?
Hi, While coding the autovacuum stuff I noticed that the dllist.c doubly linked list infrastructure is using malloc(). And the failure cases are handled in #ifdef FRONTEND exit(1) #else elog(ERROR) #endif. This seems a bit ugly, but more importantly, it doesn't let me free the whole list by simply resetting a context. Would anybody be too upset if I wholesaledly changed malloc() to palloc() and get rid of the #ifdef FRONTEND? AFAICS, no frontend code in our tree uses it. (Currently, the code to free the list walks it node by node). A less invasive alternative would be #ifdef FRONTEND #define DLLALLOC(x) malloc(x) #else #define DLLALLOC(c) palloc(x) #endif One problem with using palloc() at all is that it would use a little bit more memory, but we don't seem to be very worried about that. Currently, dllist is mostly used to keep track of the backend list in postmaster, and the tuple lists in the catalog cache. Opinions? -- Alvaro Herrera http://www.CommandPrompt.com/ PostgreSQL Replication, Consulting, Custom Development, 24x7 support
Alvaro Herrera <alvherre@commandprompt.com> writes:
> Hi,
> While coding the autovacuum stuff I noticed that the dllist.c doubly
> linked list infrastructure is using malloc(). And the failure cases are
> handled in #ifdef FRONTEND exit(1) #else elog(ERROR) #endif.
> This seems a bit ugly, but more importantly, it doesn't let me free the
> whole list by simply resetting a context. Would anybody be too upset if
> I wholesaledly changed malloc() to palloc() and get rid of the #ifdef
> FRONTEND? AFAICS, no frontend code in our tree uses it.
IIRC, libpq once used it to manage the pending-notify list, but that was
a long time ago. I have no objection to getting rid of the FRONTEND
support.
If you change to plain palloc you'll need to check that existing callers
call it in a suitable context --- they may be assuming that the lists
will be durable regardless of CurrentMemoryContext.
regards, tom lane
Another change that could be done to Dllist is removing the Dllist
pointer from the Dlelem struct:
Index: src/include/lib/dllist.h
===================================================================
RCS file: /home/alvherre/Code/cvs/pgsql/src/include/lib/dllist.h,v
retrieving revision 1.27
diff -c -p -r1.27 dllist.h
*** src/include/lib/dllist.h 5 Jan 2007 22:19:55 -0000 1.27
--- src/include/lib/dllist.h 18 Mar 2007 05:53:12 -0000
*************** typedef struct Dlelem
*** 50,56 **** struct Dlelem *dle_next; /* next element */ struct Dlelem *dle_prev; /*
previouselement */ void *dle_val; /* value of the element */
- struct Dllist *dle_list; /* what list this element is in */ } Dlelem; typedef struct Dllist
--- 49,54 ----
This means that to remove a element from a list or move it to the front of the
list, you need not only know the element pointer itself, but also the list
pointer. This capability is not used much however; the only patch of any
significance needed is below. The rest of the callers know the list pointer
already.
I tried to measure a performance difference with pgbench (using a test
small enough to fit in memory, fsync off and initialized with -s 10,
test runs with -c 5) but the differences seem to be way down in the
noise.
Are there objections to this change?
Index: src/backend/utils/cache/catcache.c
===================================================================
RCS file: /home/alvherre/Code/cvs/pgsql/src/backend/utils/cache/catcache.c,v
retrieving revision 1.136
diff -c -p -r1.136 catcache.c
*** src/backend/utils/cache/catcache.c 5 Jan 2007 22:19:42 -0000 1.136
--- src/backend/utils/cache/catcache.c 18 Mar 2007 06:38:23 -0000
*************** CatCachePrintStats(int code, Datum arg)
*** 327,332 ****
--- 327,335 ---- static void CatCacheRemoveCTup(CatCache *cache, CatCTup *ct) {
+ uint32 hashValue;
+ Index hashIndex;
+ Assert(ct->refcount == 0); Assert(ct->my_cache == cache);
*************** CatCacheRemoveCTup(CatCache *cache, CatC
*** 343,349 **** } /* delink from linked list */
! DLRemove(&ct->cache_elem); /* free associated tuple data */ if (ct->tuple.t_data != NULL)
--- 346,354 ---- } /* delink from linked list */
! hashValue = CatalogCacheComputeTupleHashValue(cache, &ct->tuple);
! hashIndex = HASH_INDEX(hashValue, cache->cc_nbuckets);
! DLRemove(&cache->cc_bucket[hashIndex], &ct->cache_elem); /* free associated tuple data */ if
(ct->tuple.t_data!= NULL)
--
Alvaro Herrera http://www.CommandPrompt.com/
PostgreSQL Replication, Consulting, Custom Development, 24x7 support
Alvaro Herrera <alvherre@commandprompt.com> writes:
> Another change that could be done to Dllist is removing the Dllist
> pointer from the Dlelem struct:
I think this is a bad idea. The patch you propose makes
CatCacheRemoveCTup significantly more expensive (extra hash
calculation). Moreover, the savings involved is entirely illusory:
palloc chunks are powers of 2, therefore 3 pointers take the same
space as 4.
regards, tom lane