Horton

Thread

Commits

  1. Remove ACLDEBUG #define and associated code.

  2. Remove useless (and broken) logging logic in memory context functions.

  3. Remove HEAPDEBUGALL

  1. HEAPDEBUGALL is broken

    Peter Eisentraut <peter.eisentraut@2ndquadrant.com> — 2020-04-19T12:50:27Z

    The HEAPDEBUGALL define has been broken since PG12 due to tableam 
changes.  Should we just remove this?  It doesn't look very useful. 
It's been around since Postgres95.

If we opt for removing: PG12 added an analogous HEAPAMSLOTDEBUGALL 
(which still compiles correctly).  Would we want to keep that?

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

  2. Re: HEAPDEBUGALL is broken

    Tom Lane <tgl@sss.pgh.pa.us> — 2020-04-19T13:37:08Z

    Peter Eisentraut <peter.eisentraut@2ndquadrant.com> writes:
> The HEAPDEBUGALL define has been broken since PG12 due to tableam 
> changes.  Should we just remove this?  It doesn't look very useful. 
> It's been around since Postgres95.
> If we opt for removing: PG12 added an analogous HEAPAMSLOTDEBUGALL 
> (which still compiles correctly).  Would we want to keep that?

+1 for removing both.  There are a lot of such debug "features"
in the code, and few of them are worth anything IME.

			regards, tom lane

  3. Re: HEAPDEBUGALL is broken

    Alexander Law <exclusion@gmail.com> — 2020-04-19T20:00:00Z

    Hello hackers,
19.04.2020 13:37, Tom Lane wrote:
>
> Peter Eisentraut <peter.eisentraut@2ndquadrant.com> writes:
>> The HEAPDEBUGALL define has been broken since PG12 due to tableam
>> changes.  Should we just remove this?  It doesn't look very useful.
>> It's been around since Postgres95.
>> If we opt for removing: PG12 added an analogous HEAPAMSLOTDEBUGALL
>> (which still compiles correctly).  Would we want to keep that?
>
> +1 for removing both.  There are a lot of such debug "features"
> in the code, and few of them are worth anything IME.
To the point, I've tried to use HAVE_ALLOCINFO on master today and it
failed too:
$ CPPFLAGS="-DHAVE_ALLOCINFO" ./configure --enable-tap-tests
--enable-debug --enable-cassert  >/dev/null && make -j16 >/dev/null
generation.c: In function ‘GenerationAlloc’:
generation.c:191:11: error: ‘GenerationContext {aka struct
GenerationContext}’ has no member named ‘name’
     (_cxt)->name, (_chunk), (_chunk)->size)
           ^
generation.c:386:3: note: in expansion of macro ‘GenerationAllocInfo’
   GenerationAllocInfo(set, chunk);
   ^~~~~~~~~~~~~~~~~~~
generation.c:191:11: error: ‘GenerationContext {aka struct
GenerationContext}’ has no member named ‘name’
     (_cxt)->name, (_chunk), (_chunk)->size)
           ^
generation.c:463:2: note: in expansion of macro ‘GenerationAllocInfo’
  GenerationAllocInfo(set, chunk);
  ^~~~~~~~~~~~~~~~~~~

Best regards,
Alexander

  4. Re: HEAPDEBUGALL is broken

    Peter Eisentraut <peter.eisentraut@2ndquadrant.com> — 2020-04-21T18:01:53Z

    On 2020-04-19 22:00, Alexander Lakhin wrote:
> To the point, I've tried to use HAVE_ALLOCINFO on master today and it 
> failed too:
> $ CPPFLAGS="-DHAVE_ALLOCINFO" ./configure --enable-tap-tests 
> --enable-debug --enable-cassert  >/dev/null && make -j16 >/dev/null
> generation.c: In function ‘GenerationAlloc’:
> generation.c:191:11: error: ‘GenerationContext {aka struct 
> GenerationContext}’ has no member named ‘name’
>       (_cxt)->name, (_chunk), (_chunk)->size)
>             ^
> generation.c:386:3: note: in expansion of macro ‘GenerationAllocInfo’
>     GenerationAllocInfo(set, chunk);
>     ^~~~~~~~~~~~~~~~~~~
> generation.c:191:11: error: ‘GenerationContext {aka struct 
> GenerationContext}’ has no member named ‘name’
>       (_cxt)->name, (_chunk), (_chunk)->size)
>             ^
> generation.c:463:2: note: in expansion of macro ‘GenerationAllocInfo’
>    GenerationAllocInfo(set, chunk);
>    ^~~~~~~~~~~~~~~~~~~

Do you have a proposed patch?

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

  5. Re: HEAPDEBUGALL is broken

    Peter Eisentraut <peter.eisentraut@2ndquadrant.com> — 2020-04-21T18:11:26Z

    On 2020-04-19 15:37, Tom Lane wrote:
> Peter Eisentraut <peter.eisentraut@2ndquadrant.com> writes:
>> The HEAPDEBUGALL define has been broken since PG12 due to tableam
>> changes.  Should we just remove this?  It doesn't look very useful.
>> It's been around since Postgres95.
>> If we opt for removing: PG12 added an analogous HEAPAMSLOTDEBUGALL
>> (which still compiles correctly).  Would we want to keep that?
> 
> +1 for removing both.  There are a lot of such debug "features"
> in the code, and few of them are worth anything IME.

removed

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

  6. Re: HEAPDEBUGALL is broken

    Tom Lane <tgl@sss.pgh.pa.us> — 2020-04-21T18:27:44Z

    Peter Eisentraut <peter.eisentraut@2ndquadrant.com> writes:
> On 2020-04-19 15:37, Tom Lane wrote:
>> +1 for removing both.  There are a lot of such debug "features"
>> in the code, and few of them are worth anything IME.

> removed

I don't see a commit?

			regards, tom lane

  7. Re: HEAPDEBUGALL is broken

    Alexander Law <exclusion@gmail.com> — 2020-04-22T04:00:00Z

    21.04.2020 21:01, Peter Eisentraut wrote:
> On 2020-04-19 22:00, Alexander Lakhin wrote:
>> To the point, I've tried to use HAVE_ALLOCINFO on master today and it
>> failed too:
>
> Do you have a proposed patch?
>
As this is broken at least since the invention of the generational
allocator (2017-11-23, a4ccc1ce), I believe than no one uses this (and
slab is broken too). Nonetheless, HAVE_ALLOCINFO in aset.c is still
working, so it could be leaved alone, though the output too chatty for
general use (`make check` produces postmaster log of size 3.8GB). I
think someone would still need to insert some extra conditions to use
that or find another way to debug memory allocations.

So I would just remove this debug macro. The proposed patch is attached.

Best regards,
Alexander

  8. Re: HEAPDEBUGALL is broken

    Peter Eisentraut <peter.eisentraut@2ndquadrant.com> — 2020-04-22T11:29:47Z

    On 2020-04-21 20:27, Tom Lane wrote:
> Peter Eisentraut <peter.eisentraut@2ndquadrant.com> writes:
>> On 2020-04-19 15:37, Tom Lane wrote:
>>> +1 for removing both.  There are a lot of such debug "features"
>>> in the code, and few of them are worth anything IME.
> 
>> removed
> 
> I don't see a commit?

pushed now

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

  9. Re: HEAPDEBUGALL is broken

    Tom Lane <tgl@sss.pgh.pa.us> — 2020-04-22T14:17:26Z

    Peter Eisentraut <peter.eisentraut@2ndquadrant.com> writes:
> On 2020-04-21 20:27, Tom Lane wrote:
>> I don't see a commit?

> pushed now

Looking at this, I'm tempted to nuke ACLDEBUG as well, which
is the only remaining undocumented symbol in pg_config_manual.h.
The code it controls looks equally forlorn and not-useful-as-is.

			regards, tom lane

  10. Re: HEAPDEBUGALL is broken

    Tom Lane <tgl@sss.pgh.pa.us> — 2020-04-22T14:19:51Z

    Alexander Lakhin <exclusion@gmail.com> writes:
> 21.04.2020 21:01, Peter Eisentraut wrote:
>> Do you have a proposed patch?

> As this is broken at least since the invention of the generational
> allocator (2017-11-23, a4ccc1ce), I believe than no one uses this (and
> slab is broken too). Nonetheless, HAVE_ALLOCINFO in aset.c is still
> working, so it could be leaved alone, though the output too chatty for
> general use (`make check` produces postmaster log of size 3.8GB). I
> think someone would still need to insert some extra conditions to use
> that or find another way to debug memory allocations.

> So I would just remove this debug macro. The proposed patch is attached.

I didn't review this in close detail, but I think it's a good idea.
We have better memory-use-analysis tools these days, such as valgrind,
so it's no surprise that nobody is using this old code.

			regards, tom lane

  11. Re: HEAPDEBUGALL is broken

    Andres Freund <andres@anarazel.de> — 2020-04-23T03:44:18Z

    On 2020-04-19 09:37:08 -0400, Tom Lane wrote:
> Peter Eisentraut <peter.eisentraut@2ndquadrant.com> writes:
> > The HEAPDEBUGALL define has been broken since PG12 due to tableam 
> > changes.  Should we just remove this?  It doesn't look very useful. 
> > It's been around since Postgres95.
> > If we opt for removing: PG12 added an analogous HEAPAMSLOTDEBUGALL 
> > (which still compiles correctly).  Would we want to keep that?
> 
> +1 for removing both.  There are a lot of such debug "features"
> in the code, and few of them are worth anything IME.

Belatedly: +many

  12. Re: HEAPDEBUGALL is broken

    Tom Lane <tgl@sss.pgh.pa.us> — 2020-04-23T19:28:40Z

    I wrote:
> Alexander Lakhin <exclusion@gmail.com> writes:
>> So I would just remove this debug macro. The proposed patch is attached.

> I didn't review this in close detail, but I think it's a good idea.

I checked this more closely and pushed it.

			regards, tom lane

  13. Re: HEAPDEBUGALL is broken

    Tom Lane <tgl@sss.pgh.pa.us> — 2020-04-23T19:38:56Z

    I wrote:
> Looking at this, I'm tempted to nuke ACLDEBUG as well, which
> is the only remaining undocumented symbol in pg_config_manual.h.
> The code it controls looks equally forlorn and not-useful-as-is.

Did that, too.

			regards, tom lane

  14. Re: HEAPDEBUGALL is broken

    Michael Paquier <michael@paquier.xyz> — 2020-04-24T07:19:24Z

    On Wed, Apr 22, 2020 at 08:44:18PM -0700, Andres Freund wrote:
> On 2020-04-19 09:37:08 -0400, Tom Lane wrote:
>> +1 for removing both.  There are a lot of such debug "features"
>> in the code, and few of them are worth anything IME.
> 
> Belatedly: +many

+1.
--
Michael