Re: Should we add a compiler warning for large stack frames?

Hi,

On 2024-04-11 15:16:57 -0400, Andrew Dunstan wrote:
> On 2024-04-11 Th 15:01, Andres Freund wrote:
> > [1345/1940 42 69%] Compiling C object src/bin/pg_verifybackup/pg_verifybackup.p/pg_verifybackup.c.o
> > ../../../../../home/andres/src/postgresql/src/bin/pg_verifybackup/pg_verifybackup.c: In function 'verify_file_checksum':
> > ../../../../../home/andres/src/postgresql/src/bin/pg_verifybackup/pg_verifybackup.c:842:1: warning: stack usage is 131232 bytes [-Wstack-usage=]
> > 842 | verify_file_checksum(verifier_context *context, manifest_file *m,
> > | ^~~~~~~~~~~~~~~~~~~~
> >
> This one's down to me. I asked Robert some time back why we were using a
> very conservative buffer size, and he agreed we could probably make it
> larger, but the number chosen is mine, not his. It was a completely
> arbitrary choice.
>
> I'm happy to reduce it, but it's not clear to me why we care that much for a
> client binary. There's no stack depth checking going on here.

There isn't - but it that doesn't necessarily mean it's great to just use a
lot of stack. It can even mean that you ought to be more careful, because
you'll just crash, rather than get an error about having exceeded the stack
size. When the stack size is increased by more than a few pages, the OS /
compiler defenses for detecting that don't work as well, if you're unlucky.

128k is probably not going to be an issue in practice. However, it also seems
not great from a performance POV to use this much stack in a function that's
called fairly often. I'd allocate the buffer in verify_backup_checksums() and
reuse it across all to-be-checked files.

Greetings,

Andres Freund