Re: [HACKERS] Bogus sizing parameters in some AllocSetContextCreate calls

[Tomas Vondra]

On 08/28/2016 04:59 PM, Tom Lane wrote:

Tomas Vondra  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


--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Bogus sizing parameters in some AllocSetContextCreate calls

[Tom Lane]

Tomas Vondra  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


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Bogus sizing parameters in some AllocSetContextCreate calls

[Tomas Vondra]

On 08/28/2016 04:41 PM, Tom Lane wrote:

Robert Haas  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


--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Bogus sizing parameters in some AllocSetContextCreate calls

[Tomas Vondra]

On 08/28/2016 04:41 PM, Tom Lane wrote:

Robert Haas  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


--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Bogus sizing parameters in some AllocSetContextCreate calls

[Tom Lane]

Robert Haas  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


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Bogus sizing parameters in some AllocSetContextCreate calls

[Robert Haas]

On Sat, Aug 27, 2016 at 2:08 PM, Tom Lane  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


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Bogus sizing parameters in some AllocSetContextCreate calls

[Tom Lane]

Andres Freund  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


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Bogus sizing parameters in some AllocSetContextCreate calls

[Andres Freund]

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*...


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Bogus sizing parameters in some AllocSetContextCreate calls

[Tom Lane]

Andres Freund  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


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Bogus sizing parameters in some AllocSetContextCreate calls

[Andres Freund]

On 2016-08-27 15:36:28 -0400, Tom Lane wrote:
> Andres Freund  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


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Bogus sizing parameters in some AllocSetContextCreate calls

[Tom Lane]

Andres Freund  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


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Bogus sizing parameters in some AllocSetContextCreate calls

[Andres Freund]

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


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers