Bogus sizing parameters in some AllocSetContextCreate calls - Mailing list pgsql-hackers

The standard calling pattern for AllocSetContextCreate is
   cxt = AllocSetContextCreate(parent, name,                               ALLOCSET_DEFAULT_MINSIZE,
          ALLOCSET_DEFAULT_INITSIZE,                               ALLOCSET_DEFAULT_MAXSIZE);
 

or for some contexts you might s/DEFAULT/SMALL/g if you expect the context
to never contain very much data.  I happened to notice that there are a
few calls that get this pattern wrong.  After a bit of grepping, I found:

hba.c lines 389, 2196:                                  ALLOCSET_DEFAULT_MINSIZE,
ALLOCSET_DEFAULT_MINSIZE,                                 ALLOCSET_DEFAULT_MAXSIZE);
 

guc-file.l line 146:                                      ALLOCSET_DEFAULT_MINSIZE,
ALLOCSET_DEFAULT_MINSIZE,                                      ALLOCSET_DEFAULT_MAXSIZE);
 

brin.c line 857:
                               ALLOCSET_SMALL_INITSIZE,                               ALLOCSET_SMALL_MINSIZE,
                   ALLOCSET_SMALL_MAXSIZE);
 

autovacuum.c line 2184:                                         ALLOCSET_DEFAULT_INITSIZE,
          ALLOCSET_DEFAULT_MINSIZE,                                         ALLOCSET_DEFAULT_MAXSIZE);
 

typcache.c lines 757, 842:
                                           ALLOCSET_SMALL_INITSIZE,
ALLOCSET_SMALL_MINSIZE,                                          ALLOCSET_SMALL_MAXSIZE);
 

In all of these cases, we're passing zero as the initBlockSize, which is
invalid, but but AllocSetContextCreate silently forces it up to 1K.  So
there's no failure but there may be some inefficiency due to small block
sizes of early allocation blocks.  I seriously doubt that's intentional;
in some of these cases it might be sane to use the SMALL parameters
instead, but this isn't a good way to get that effect.  The last four
cases are also passing a nonzero value as the minContextSize, forcing
the context to allocate at least that much instantly and hold onto it
over resets.  That's inefficient as well, though probably only minorly so.

I can state confidently that the typcache.c cases are just typos, because
I wrote them :-(.  It seems unlikely that the others are intentional
either; certainly none of these cases have any comments suggesting that
any special behavior is meant.

While we could just fix these cases, the fact that as many as seven of
the only-140-or-so calls in our code are wrong suggests that we ought
to think about a less typo-prone solution.  My low-tech proposal is to
define two new macros along the lines of

#define ALLOCSET_DEFAULT_SIZES  ALLOCSET_DEFAULT_MINSIZE, ALLOCSET_DEFAULT_INITSIZE, ALLOCSET_DEFAULT_MAXSIZE

#define ALLOCSET_SMALL_SIZES  ALLOCSET_SMALL_MINSIZE, ALLOCSET_SMALL_INITSIZE, ALLOCSET_SMALL_MAXSIZE

so that a typical call now looks like
   cxt = AllocSetContextCreate(parent, name,                               ALLOCSET_DEFAULT_SIZES);

This approach doesn't break any third-party code that doesn't get
converted right away, and it also doesn't create a problem for the small
number of places that are intentionally trying to obtain nonstandard
effects.

There are three or four places that use the pattern
             ALLOCSET_SMALL_MINSIZE,             ALLOCSET_SMALL_INITSIZE,             ALLOCSET_DEFAULT_MAXSIZE

with the intention of starting small but allowing the context to grow big
efficiently if it needs to.  It might be worth defining a third macro
for this pattern.  The cases that fit into none of these patterns are
few enough and special enough (eg, ErrorContext) that I don't think they
need adjustment.

Barring objection, I propose to make these changes in HEAD and 9.6.
I don't feel a great need to adjust the back branches --- although there
might be some argument for adding these new calling-pattern macros to the
back branches so as not to create a back-patching hazard for patches
that add new AllocSetContextCreate calls.

Comments?
        regards, tom lane