Re: embedded list v2 - Mailing list pgsql-hackers

On 28 June 2012 19:20, Andres Freund <andres@2ndquadrant.com> wrote:
> <0001-Add-embedded-list-interface.patch>
>
> Looks good now?

I have a few gripes.

+      * there isn't much we can test in a single linked list except that its

There are numerous references to "single linked lists", where, I
believe, "singly linked list" is intended (the same can be said for
"double" and "doubly" respectively).

/* Functions we want to be inlined if possible */
+ #ifndef USE_INLINE
...
+ #endif /* USE_INLINE */

A minor quibble, but that last line probably be:

#endif /* ! defined USE_INLINE */

Another minor quibble. We usually try and keep these in alphabetical order:

*** a/src/backend/lib/Makefile
--- b/src/backend/lib/Makefile
*************** subdir = src/backend/lib
*** 12,17 **** top_builddir = ../../.. include $(top_builddir)/src/Makefile.global

! OBJS = dllist.o stringinfo.o
 include $(top_srcdir)/src/backend/common.mk
--- 12,17 ---- top_builddir = ../../.. include $(top_builddir)/src/Makefile.global

! OBJS = dllist.o stringinfo.o ilist.o
 include $(top_srcdir)/src/backend/common.mk

New files generally don't require this:

+  * Portions Copyright (c) 1994, Regents of the University of California

This needs to be altered:

+ /*
+  * enable for extra debugging. This is rather expensive so its not enabled by
+  * default even with --enable-cassert
+ */
+ /* #define ILIST_DEBUG */

I'm not sure that this extra error-detection warrants its own macro.
syncrep.c similarly has its own rather expensive error-detection, with
entire function bodies only compiled when USE_ASSERT_CHECKING is
defined. Any of the other dedicated "debugging macros", like
LOCK_DEBUG or WAL_DEBUG seem to exist to dump LOG messages when
binaries were built with the macros defined (this can happen via
pg_config_manual.h. I note that what you have here lacks a
corresponding entry). I think that it would be more idiomatic to just
use USE_ASSERT_CHECKING, and rewrite the debugging functions such that
they are used directly within an Assert, in the style of syncrep.c .
Now, I know Tom wasn't too enthusiastic about having this sort of
thing within --enable-cassert builds, but I'm inclined to think that
there is little point in having this additional error checking if
they're not at least available when that configuration is used. Maybe
we should consider taking the sophisticated asserts out of
--enable-cassert builds, or removing them entirely, if and when they
prove to be annoying?

There is slight misalignment within the comments at the top of ilist.h.

Within ilist.c, the following block exists:

+ #ifndef USE_INLINE
+ #define ILIST_USE_DECLARATION
+ #endif
+
+ #include "lib/ilist.h"
+
+ #ifndef USE_INLINE
+ #undef ILIST_USE_DECLARATION
+ #endif

I see little reason for the latter "#undef" block within ilist.c. It
isn't that exactly the same code already exists at the end of ilist.h
(though there is that too) - it's mostly that it's unnecessary,
because there is no need or logical reason to #undef within ilist.c.

+ /*
+  * The following function declarations are only used if inlining is supported
+  * or when included from a file that explicitly declares USE_DECLARATION
+  */
+ #ifdef ILIST_USE_DECLARATION

Shouldn't that be "The following function definitions..." and
ILIST_USE_DEFINITIONS?

I think the fact that it's possible in principle for
ILIST_USE_DECLARATION to not be exactly equivalent to ! USE_INLINE is
strictly speaking dangerous, since USE_INLINE needs to be baked into
the ABI targeted by third-party module developers. What if a module
was built that called a function that didn't have an entry in the
procedure linkage table, due to ad-hoc usage of ILIST_USE_DECLARATION?
That'd result in a libdl error, if you're lucky. Now, that probably
sounds stupid - I'm pretty sure that you didn't intend that
ILIST_USE_DECLARATION be set by just any old client of this
infrastructure. Rather, you intended that ILIST_USE_DECLARATION be set
either when ilist.h was included while USE_INLINE, so that macro
expansion would make those functions inline, or within ilist.c, when
!USE_INLINE, so that the functions would not be inlined. This should
be much more explicit though. You simply describe the mechanism rather
than the intent at present.

I rather suspect that INLINE_IF_POSSIBLE should be a general purpose
utility, perhaps defined next to NON_EXEC_STATIC within c.h, with a
brief explanation there (rather than in any new header that needs to
do this). I think you'd be able to reasonably split singly and doubly
linked lists into their own headers without much repetition between
the two then. You could formalise the idea that where USE_INLINE,
another macro, defined alongside INLINE_IF_POSSIBLE (functionally
equivalent to ILIST_USE_DECLARATION in the extant code - say,
USE_INLINING_DEFINITIONS) is going to be generally defined everywhere
USE_INLINE is defined. You're then going to have to deal with the hack
whereby USE_INLINING_DEFINITIONS is set just "to suck the definitions
out of the header" to make !USE_INLINE work within .c files only (or
pretty close). That's kins of logical though - !USE_INLINE is hardly
ever used, so it deserves to get the slightly grottier code. The only
direct macro usage that your header files now need is "#ifdef
USE_INLINING_DEFINITIONS" (plus INLINE_IF_POSSIBLE rather than plain
"static inline", of course), as well as having extern declarations for
the !USE_INLINE case. The idea is that each header file has slightly
fewer eyebrow-raising macro hacks, and there is at least no obvious
redundancy between the two. In particular, this only has to happen
once, within c.h:

#ifdef USE_INLINE
#define INLINE_IF_POSSIBLE static inline
#define USE_INLINING_DEFINITIONS // actually spelt
"ILIST_USE_DECLARATION" in patch
#else
#define INLINE_IF_POSSIBLE
#endif

What's more, under this scheme, the following code does not need to
(and shouldn't) appear at the end of headers like ilist.h:

+ /*
+  * if we defined ILIST_USE_DECLARATION undef it again, its not interesting
+  * outside this file
+  */
+ #ifdef USE_INLINE
+ #undef USE_INLINING_DEFINITIONS // actually spelt
"ILIST_USE_DECLARATION" in patch
+ #endif

because only when considering the !USE_INLINE case do we have to
consider that we might need to manually set USE_INLINING_DEFINITIONS
to "suck in" definitions within C files (as I've said, unsetting it
very probably doesn't matter within a C file, so no need to do it
there either).

If that isn't quite clear, the simple version is:

We assume the USE_INLINE case until we learn otherwise, rather than
assuming neither USE_INLINE nor !USE_INLINE. It's cleaner to treat
!USE_INLINE as a sort of edge case with special handling, rather than
having ilist.h and friends worry about cleaning up after themselves
all the time, and worrying about initialising things for themselves
alone according to the present build configuration.

In any case, if we're going to manage the !USE_INLINE case like this
at all, we should probably formalise it along some lines.

I still don't think this is going to fly:

+ #ifdef __GNUC__
+ #define unused_attr __attribute__((unused))
+ #else
+ #define unused_attr
+ #endif

There is no reasonable way to prevent MSVC from giving a warning as a
result of this. The rationale for doing things this way is:

+ /*
+  * gcc warns about unused parameters if -Wunused-parameter is specified (part
+  * of -Wextra ). In the cases below its perfectly fine not to use those
+  * parameters because they are just passed to make the interface
consistent and
+  * to allow debugging in case of ILIST_DEBUG.
+  *
+  */

Seems to me that you'd be able to do a better job with a thin macro
shim on the existing (usually) inline functions. That'd also take care
of your concerns about multiple evaluation.

The comments could probably use some wordsmithing in various places.
That's no big deal though.

I agree with Alvaro on the naming of these functions. I'd prefer it if
they didn't have the "i" prefix too, though that's hardly very
important.

That's all I have for now. I'll take a look at the other patch
(0002-Remove-usage-of-lib-dllist.h-and-replace-it-by-the-n.patch)
soon.

-- 
Peter Geoghegan       http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training and Services