Re: embedded list v2 - Mailing list pgsql-hackers

Alvaro Herrera <alvherre@2ndquadrant.com> writes:
> Here's an updated version of both patches, as well as a third patch that
> converts the cc_node list link in catcache.c into an slist.

There's a lot of stuff here that seems rather unfortunate and/or sloppy.

Does it even compile?  The 0002 patch refers to a typedef ilist_d_head
that I don't see defined anywhere.  (It would be good to shorten that
name by a couple of characters anyway, for tab-stop alignment reasons.)

The documentation (such as it is) claims that the lists are circularly
linked, which doesn't seem to be the case from the code; slist_foreach
for instance certainly won't work if that's true.  (As far as the
docs go, I'd get rid of the README file and put the how-to-use-it
comments into the header file, where they have some chance of being
(a) seen and (b) maintained.  But first they need to be rewritten.)

The distinction between head and node structs seems rather useless,
and it's certainly notationally tedious.  Do we need it?

I find the need for this change quite unfortunate:

@@ -256,7 +258,7 @@ typedef structstatic AutoVacuumShmemStruct *AutoVacuumShmem;/* the database list in the launcher,
andthe context that contains it */
 
-static Dllist *DatabaseList = NULL;
+static ilist_d_head DatabaseList;static MemoryContext DatabaseListCxt = NULL;/* Pointer to my own WorkerInfo, valid on
eachworker */
 
@@ -403,6 +405,9 @@ AutoVacLauncherMain(int argc, char *argv[])    /* Identify myself via ps */
init_ps_display("autovacuumlauncher process", "", "", "");
 
+    /* initialize to be empty */
+    ilist_d_init(&DatabaseList);
+    ereport(LOG,            (errmsg("autovacuum launcher started")));

Instead let's provide a macro for an empty list value, so that you can
do something like

static ilist_d_head DatabaseList = EMPTY_DLIST;

and not require a new assumption that there will be a convenient place
to initialize static list variables.  In general I think it'd be a lot
better if the lists were defined such that all-zeroes is a valid empty
list header, anyway.

The apparently random decisions to make some things inline functions
and other things macros (even though they evaluate their args multiple
times) will come back to bite us.  I am much more interested in
unsurprising behavior of these constructs than I am in squeezing out
an instruction or two, so I'm very much against the unsafe macros.

I think we could do without such useless distinctions as slist_get_head
vs slist_get_head_unchecked, too.  We've already got Assert and
ILIST_DEBUG, we do not need even more layers of check-or-not
distinctions.

Meanwhile, obvious checks that *should* be made, like slist_pop_head
asserting that there be something to pop, don't seem to be made.

Not a full review, just some things that struck me in a quick scan...
        regards, tom lane