Re: Bogus sizing parameters in some AllocSetContextCreate calls

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

Hi,

On 2016-08-27 14:08:44 -0400, Tom Lane wrote:
> The standard calling pattern for AllocSetContextCreate is
> 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.

I think we might also / instead want to think about replacing a lot of
those AllocSetContextCreate with a wrapper function. Currently is really
rather annoying to experiment with switching the default allocator
out. And if we're touching all that code anyway ...

Andres

Andres Freund <andres(at)anarazel(dot)de> writes:
> On 2016-08-27 14:08:44 -0400, Tom Lane wrote:
>> Barring objection, I propose to make these changes in HEAD and 9.6.

> I think we might also / instead want to think about replacing a lot of
> those AllocSetContextCreate with a wrapper function. Currently is really
> rather annoying to experiment with switching the default allocator
> out. And if we're touching all that code anyway ...

I do not see why you'd think that "switching the default allocator out"
requires anything except making AllocSetContextCreate do something else.

What is actually of interest, IMO, is making *some* contexts have a
different allocator, and that is going to require case-by-case decisions
anyway. A blanket switch seems completely useless to me.

regards, tom lane

On 2016-08-27 15:36:28 -0400, Tom Lane wrote:
> Andres Freund <andres(at)anarazel(dot)de> writes:
> > On 2016-08-27 14:08:44 -0400, Tom Lane wrote:
> >> Barring objection, I propose to make these changes in HEAD and 9.6.
>
> > I think we might also / instead want to think about replacing a lot of
> > those AllocSetContextCreate with a wrapper function. Currently is really
> > rather annoying to experiment with switching the default allocator
> > out. And if we're touching all that code anyway ...
>
> I do not see why you'd think that "switching the default allocator out"
> requires anything except making AllocSetContextCreate do something else.

Well yea. But given it's explicitly tied to aset.c that doesn't seem
like a sensible thing. DefaultContextCreate() or something (in mcxt.c),
which then calls AllocSetContextCreate, seems better to me.

> What is actually of interest, IMO, is making *some* contexts have a
> different allocator, and that is going to require case-by-case decisions
> anyway. A blanket switch seems completely useless to me.

Don't think aset.c is actually that good, so I'm not convinced of
that. And even if it's just to verify that a special case allocator
actually works with more coverage...

Andres

Andres Freund <andres(at)anarazel(dot)de> writes:
> On 2016-08-27 15:36:28 -0400, Tom Lane wrote:
>> What is actually of interest, IMO, is making *some* contexts have a
>> different allocator, and that is going to require case-by-case decisions
>> anyway. A blanket switch seems completely useless to me.

> Don't think aset.c is actually that good, so I'm not convinced of
> that. And even if it's just to verify that a special case allocator
> actually works with more coverage...

I'm not exactly following. If you have a new allocator that dominates
aset.c on all cases, why would we not simply replace aset.c with it?
Mass substitution implemented at the caller end seems like the hard way.

(Or in other words, the fact that "DefaultContextCreate" is spelled
"AllocSetContextCreate" is a bit of a historical artifact, but I do
not see why changing the spelling is a useful exercise.)

regards, tom lane

On 2016-08-27 15:46:26 -0400, Tom Lane wrote:
> (Or in other words, the fact that "DefaultContextCreate" is spelled
> "AllocSetContextCreate" is a bit of a historical artifact, but I do
> not see why changing the spelling is a useful exercise.)

Well, if you're going through nearly all of the instances where it's
referenced *anyway*...

Andres Freund <andres(at)anarazel(dot)de> writes:
> On 2016-08-27 15:46:26 -0400, Tom Lane wrote:
>> (Or in other words, the fact that "DefaultContextCreate" is spelled
>> "AllocSetContextCreate" is a bit of a historical artifact, but I do
>> not see why changing the spelling is a useful exercise.)

> Well, if you're going through nearly all of the instances where it's
> referenced *anyway*...

I am not touching, and cannot touch, all the instances in third-party
code.

regards, tom lane

On Sat, Aug 27, 2016 at 2:08 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> 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 noticed this a while back while playing with my alternate allocator,
but wasn't sure how much of it was intentional. Anyway, +1 for doing
something to clean this up. Your proposal sounds OK, but maybe
AllocSetContextCreateDefault() and AllocSetContextCreateSmall() would
be nice and easier to remember.

Also, I think we ought to replace this code in aset.c:

initBlockSize = MAXALIGN(initBlockSize);
if (initBlockSize < 1024)
initBlockSize = 1024;
maxBlockSize = MAXALIGN(maxBlockSize);

With this:

Assert(initBlockSize >= 1024 && initBlockSize == MAXALIGN(initBlockSize));
Assert(maxBlockSize == MAXALIGN(maxBlockSize));

This might save a few cycles which wouldn't be unwelcome given that
AllocSetContextCreate does show up in profiles, but more importantly I
think it's completely inappropriate for this function to paper over
whatever errors may be made by callers.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Robert Haas <robertmhaas(at)gmail(dot)com> writes:
> Also, I think we ought to replace this code in aset.c:

> initBlockSize = MAXALIGN(initBlockSize);
> if (initBlockSize < 1024)
> initBlockSize = 1024;
> maxBlockSize = MAXALIGN(maxBlockSize);

> With this:

> Assert(initBlockSize >= 1024 && initBlockSize == MAXALIGN(initBlockSize));
> Assert(maxBlockSize == MAXALIGN(maxBlockSize));

Good idea --- if we'd had it that way, these errors would never have
gotten committed in the first place. I'm for doing that only in HEAD
though.

regards, tom lane

On 08/28/2016 04:41 PM, Tom Lane wrote:
> Robert Haas <robertmhaas(at)gmail(dot)com> writes:
>> Also, I think we ought to replace this code in aset.c:
>
>> initBlockSize = MAXALIGN(initBlockSize);
>> if (initBlockSize < 1024)
>> initBlockSize = 1024;
>> maxBlockSize = MAXALIGN(maxBlockSize);
>
>> With this:
>
>> Assert(initBlockSize >= 1024 && initBlockSize == MAXALIGN(initBlockSize));
>> Assert(maxBlockSize == MAXALIGN(maxBlockSize));
>
> Good idea --- if we'd had it that way, these errors would never have
> gotten committed in the first place. I'm for doing that only in HEAD
> though.
>

Then maybe also add

Assert(initBlockSize <= maxBlockSize);

And perhaps also an assert making sure the minContextSize value makes
sens with respect to the min/max block sizes?

I'm however pretty sure this will have absolutely no impact on profiles.
It might save a few cycles, but this only runs when creating the memory
context and all profiles I've seen are about palloc/pfree.

It might matter when creating many memory contexts, but then you
probably have other things to worry about.

regards

--
Tomas Vondra http://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

On 08/28/2016 04:41 PM, Tom Lane wrote:
> Robert Haas <robertmhaas(at)gmail(dot)com> writes:
>> Also, I think we ought to replace this code in aset.c:
>
>> initBlockSize = MAXALIGN(initBlockSize);
>> if (initBlockSize < 1024)
>> initBlockSize = 1024;
>> maxBlockSize = MAXALIGN(maxBlockSize);
>
>> With this:
>
>> Assert(initBlockSize >= 1024 && initBlockSize == MAXALIGN(initBlockSize));
>> Assert(maxBlockSize == MAXALIGN(maxBlockSize));
>
> Good idea --- if we'd had it that way, these errors would never have
> gotten committed in the first place. I'm for doing that only in HEAD
> though.
>
> regards, tom lane
>

Then maybe also add

Assert(initBlockSize <= maxBlockSize);

And perhaps also an assert making sure the minContextSize value makes
sens with respect to the min/max block sizes?

I'm however pretty sure this will have absolutely no impact on profiles.
It might save a few cycles, but this only runs when creating the memory
context and all profiles I've seen are about palloc/pfree.

It might matter when creating many memory contexts, but then you
probably have other things to worry about.

regards

--
Tomas Vondra http://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Tomas Vondra <tomas(dot)vondra(at)2ndquadrant(dot)com> writes:
> I'm however pretty sure this will have absolutely no impact on profiles.

Yeah, I do not believe that either. Also, I think that the intent of the
existing code is to defend against bad parameters even in non-assert
builds. Which might argue for turning these into test-and-elog rather
than asserts.

regards, tom lane

On 08/28/2016 04:59 PM, Tom Lane wrote:
> Tomas Vondra <tomas(dot)vondra(at)2ndquadrant(dot)com> writes:
>> I'm however pretty sure this will have absolutely no impact on profiles.
>
> Yeah, I do not believe that either. Also, I think that the intent of the
> existing code is to defend against bad parameters even in non-assert
> builds. Which might argue for turning these into test-and-elog rather
> than asserts.
>

I agree. There's a fair number of contrib modules, and when they compile
just fine people are unlikely to test them with assert-enabled builds.
So they would not notice any issues, but it'd get broken as we've
removed the code that fixes the values for them.

--
Tomas Vondra http://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services