Re: Report a potential memory leak in setup_config() - Mailing list pgsql-bugs

Hi,

On 2022-02-15 21:38:11 -0500, Tom Lane wrote:
> I wrote:
> > Yeah, I read that.  I think making the backend read these files directly
> > makes a ton of sense, so I'm no longer very excited about improving the
> > performance or memory consumption of the replace_token stuff.
>
> Actually, wait a second.  I grant your point about postgres.bki, but
> a whole lot of the replace_token calls in initdb are messing with
> the configuration files, which (a) is something I don't see changing,
> and (b) pretty much none of that could be pushed back to build time.
> So even though the config files are a lot smaller than postgres.bki,
> maybe there's still a point in polishing replace_token a bit.

Agreed. postgresql.conf.sample isn't that small either and I can't see that
moving to the backend.

When I'd looked at this last I apparently (looking at git stash, I ended up
discarding this) decided that the best way would be to change replace_token's
API to one that just processes one line at a time, with an outer loop that
processes all tokens in a line.  I'm not really sure why anymore.

Definitely not finished and contains a lot of timing cruft... But it does
markedly reduce the memory usage, despite only converting bootstrap.bki
replacement:

without REPLACE_FAST:
==2217045== HEAP SUMMARY:
==2217045==     in use at exit: 894,012 bytes in 119 blocks
==2217045==   total heap usage: 26,284 allocs, 26,165 frees, 3,127,341 bytes allocated
==2217045==
==2217045== LEAK SUMMARY:
==2217045==    definitely lost: 887,136 bytes in 46 blocks
==2217045==    indirectly lost: 4,453 bytes in 50 blocks
==2217045==      possibly lost: 0 bytes in 0 blocks
==2217045==    still reachable: 2,423 bytes in 23 blocks
==2217045==         suppressed: 0 bytes in 0 blocks
==2217045== Rerun with --leak-check=full to see details of leaked memory

with:
==2215712== HEAP SUMMARY:
==2215712==     in use at exit: 120,490 bytes in 90 blocks
==2215712==   total heap usage: 26,257 allocs, 26,167 frees, 2,393,822 bytes allocated
==2215712==
==2215712== LEAK SUMMARY:
==2215712==    definitely lost: 116,096 bytes in 38 blocks
==2215712==    indirectly lost: 1,971 bytes in 29 blocks
==2215712==      possibly lost: 0 bytes in 0 blocks
==2215712==    still reachable: 2,423 bytes in 23 blocks
==2215712==         suppressed: 0 bytes in 0 blocks
==2215712== Rerun with --leak-check=full to see details of leaked memory


I think I was a bit underwhelmed by the wins on the runtime side on linux. A
profile without children looks quite different before / after. But initdb's
own cpu time is just such a small part of the overall runtime...


profile before:

-   46.91%     0.00%  initdb   initdb                [.] main
   - main
      - 42.99% initialize_data_directory
         - 25.45% bootstrap_template1
            + 10.59% replace_token
            + 7.98% readfile
            + 3.47% __GI___libc_free (inlined)
            + 2.63% __GI__IO_fputs (inlined)
            + 0.14% pclose_check
            + 0.14% progress

profile after:
-   42.17%     0.00%  initdb   initdb                [.] main
   - main
      - 37.98% initialize_data_directory
         - 20.32% bootstrap_template1
            + 9.11% readfile
            + 6.37% replace_one_token
            + 3.16% __GI__IO_fputs (inlined)
            + 0.79% __GI___libc_free (inlined)
            + 0.17% check_ok
            + 0.14% progress
            + 0.10% pclose_check

after both the biggest remaining cpu user is the strstr().

Both profiles are with the change included in the patch to avoid fflush()ing
for each line. That's noticeable as it turns out.

Anyway, I think this angle is kind of moot if we move this stuff to the
backend, and likely to don't perform replacement on any of the big files.


Greetings,

Andres Freund