Re: Should we work around msvc failing to compile tab-complete.c?

Hi,

On 2024-07-08 14:18:03 -0400, Tom Lane wrote:
> Andres Freund <andres(at)anarazel(dot)de> writes:
> > Compiling postgres on windows with tab-completion support fails either with
> > "fatal error C1026: parser stack overflow, program too complex”.
> > or (in recent versions) with
> > "…/src/bin/psql/tab-complete.c(4023): fatal error C1001: Internal compiler error."
>
> > It's pretty easy to work around the error [2]. I wonder if we should just do
> > that, then we don't have to insist on a very new msvc version being used and
> > it'll work even if they just decide to fix the internal compiler error.
>
> I'm on board with doing something here, but wouldn't we want to
> back-patch at least a minimal fix to all supported branches?

I think we'd need to backpatch more for older branches. At least

commit 3f28bd7337d
Author: Thomas Munro <tmunro(at)postgresql(dot)org>
Date: 2022-12-22 17:14:23 +1300

Add work-around for VA_ARGS_NARGS() on MSVC.

Given that - afaict - tab completion never used to work with msvc, I think
it'd be ok to just do it in 17 or 16+17 or such. Obviously nobody is currently
building with readline support for windows - not sure if any packager is going
to go back and add support for it in older branches.

> As for the specific thing to do, that long if-chain seems horrid
> from an efficiency standpoint as well as stressing compiler
> implementations.

Indeed.

Even with gcc it's one of the slowest files to compile in our codebase. At -O2
tab-complete.c takes 7.3s with gcc 14. Just adding an
else if (HeadMatches("ALTER"))
{
}
around all the ALTER branches reduces that to 4.5s to me. Doing that for
COMMENT and CREATE gets down to 3.2s.

> I realize that this pretty much only needs to run
> at human-reaction-time speed, but it still offends my inner nerd.

Same.

> Could we perhaps have a table of first words of each interesting
> match, and do a lookup in that before dispatching to code segments
> that are individually similar to what's there now?

Eventually it seems yet, it ought to be table driven in some form. But I
wonder if that's setting the bar too high for now. Even just doing some manual
re-nesting seems like it could get us quite far?

I'm thinking of something like an outer if-else-if chain for CREATE, ALTER,
DROP, COMMENT and everything that doesn't fit within those
(e.g. various TailMatches(), backslash command etc) we'd have reduced the
number of redundant checks a lot.

The amount of whitespace changes that'd imply isn't great, but I don't really
see a way around that.

Greetings,

Andres Freund