Re: embedded list v2 - Mailing list pgsql-hackers

Hi,

On Friday, September 14, 2012 10:48:35 PM Tom Lane wrote:
> 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.
Most of that unfortunately my fault not Alvaro's...

> 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.)
True, only dlist's are circular. The docs were in the header at first, then 
somebody voted for moving them ;)


> The distinction between head and node structs seems rather useless,
> and it's certainly notationally tedious.  Do we need it?
I think its useful. It makes the usage in structs its embedded to way much 
clearer. The head struct actually has a different meaning than normal node 
structs because its there even if the list is empty...

> I find the need for this change quite unfortunate:
> 
> @@ -256,7 +258,7 @@ typedef struct
>  static AutoVacuumShmemStruct *AutoVacuumShmem;
> 
>  /* the database list in the launcher, and the context that contains it */
> -static Dllist *DatabaseList = NULL;
> +static ilist_d_head DatabaseList;
>  static MemoryContext DatabaseListCxt = NULL;
> 
>  /* Pointer to my own WorkerInfo, valid on each worker */
> @@ -403,6 +405,9 @@ AutoVacLauncherMain(int argc, char *argv[])
>      /* Identify myself via ps */
>      init_ps_display("autovacuum launcher 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;
I don't think that can work with dlists because they are circular and need 
their pointers to be adjusted. From my POV it seems to be better to keep those 
in sync.


> 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.
For the dlists thats a major efficiency difference in some use cases. 
Unfortunately those are the ones I care about... Due to not needing to check 
for NULLs several branches can be removed from the whole thing.

> 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.
At the moment the only thing that are macros are things that cannot be 
expressed as inline functions because they return the actual contained type 
and/or because they contain a for() loop. Do you have a trick in mind to 
handle such cases?

Earlier on when talking with Alvaro I mentioned that I would like to add some 
more functions that return the [sd]list_node's instead of the contained 
elements. Those should be inline functions.

> 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.
The _unchecked variants remove the check whether the list is already empty and 
thus give up some safety for speed. Quite often that check is made before 
calling dlist_get_head() or such anyway...
I wondered whether the solution for that would be to remove the variants that 
check for an empty list (except an Assert).


> Not a full review, just some things that struck me in a quick scan...
Thanks!

Andres
-- Andres Freund                       http://www.2ndQuadrant.com/PostgreSQL Development, 24x7 Support, Training &
Services