Обсуждение: Should we work around msvc failing to compile tab-complete.c?
Hi,
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."
I've reported this to the visual studio folks at [1] and supposedly a fix is
waiting to be released. I don't know if that's just for the internal compiler
error in 2022 or lifting the limitation.
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.
Besides just breaking the if-else-if chain at some arbitrary point, we could
alternatively make a more general efficiency improvement, and add a bit of
nesting at a few places. E.g. instead of having ~33 checks for COMMENT being
the first word, we could use "outer" else-if for COMMENT and check the
sub-variants inside that branch:
else if (HeadMatches("COMMENT"))
{
if (Matches("COMMENT"))
COMPLETE_WITH("ON");
else if (Matches("COMMENT", "ON"))
...
}
if we do that to one or two common keywords (COMMENT and DROP seems easiest)
we'd be out from that limitation, and would probably reduce the number of
cycles for completions noticeably.
OTOH, that'd be a more noisy change and it'd be less defensible to apply it to
17 - which IMO would be nice to do.
Greetings,
Andres Freund
[1] https://developercommunity.visualstudio.com/t/tab-completec4023:-fatal-error-C1001:/10685868
[2] https://postgr.es/m/CACLU5mRRLAW2kca3k2gVDM8kow6wgvT_BCaeg37jz%3DKyj1afvw%40mail.gmail.com
Andres Freund <andres@anarazel.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?
As for the specific thing to do, that long if-chain seems horrid
from an efficiency standpoint as well as stressing compiler
implementations. I realize that this pretty much only needs to run
at human-reaction-time speed, but it still offends my inner nerd.
I also wonder when we are going to hit problems with some earlier
test unexpectedly pre-empting some later one.
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?
regards, tom lane
Hi,
On 2024-07-08 14:18:03 -0400, Tom Lane wrote:
> Andres Freund <andres@anarazel.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@postgresql.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
Andres Freund <andres@anarazel.de> writes:
> 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.
If it doesn't even compile in existing branches, that seems to me to
be plenty enough reason to call it a new HEAD-only feature. That
gives us some leeway for experimentation and a more invasive patch
than we might want to risk in stable branches.
> On 2024-07-08 14:18:03 -0400, Tom Lane wrote:
>> 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?
What I'm thinking is that (as you say) any fix that's not as ugly
as Kirk's hack is going to involve massive reindentation, with
corresponding breakage of any pending patches. It would be a
shame to do that twice, so I'd rather look for a long-term fix
instead of putting in a stop-gap.
Let me play with the "table" idea a little bit and see what
I get.
regards, tom lane
On Mon, 8 Jul 2024 at 21:08, Andres Freund <andres@anarazel.de> wrote:
Hi,
On 2024-07-08 14:18:03 -0400, Tom Lane wrote:
> Andres Freund <andres@anarazel.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@postgresql.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.
Packagers aren't likely to be using readline, as it's GPL and it would have to be shipped with packages on Windows. They are more likely to be using libedit if anything. Not sure if that has any bearing on the compilation failure.
Dave Page
pgAdmin: https://www.pgadmin.org
Hi, On 2024-07-09 09:14:33 +0100, Dave Page wrote: > On Mon, 8 Jul 2024 at 21:08, Andres Freund <andres@anarazel.de> wrote: > > I think we'd need to backpatch more for older branches. At least > > > > commit 3f28bd7337d > > Author: Thomas Munro <tmunro@postgresql.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. > > > Packagers aren't likely to be using readline, as it's GPL and it would have > to be shipped with packages on Windows. I'm not sure (I mean that literally, not as a way to state that I think it's not as you say) it'd actually be *that* big a problem to use readline - sure it'd make psql GPL, but that's the only thing using readline. Since psql doesn't link to extensible / user provided code, it might actually be ok. With openssl < 3.0 the mix between openssl and readline would be problematic, IIRC the licenses aren't really compatible. But luckily openssl relicensed to apache v2. But that is pretty orthogonal to the issue at hand: > They are more likely to be using libedit if anything. Not sure if that has > any bearing on the compilation failure. The compilation issue is entirely independent of readline vs libedit, the too-long if-else-if-else-if chain is purely in our code. While the error message (particularly the compiler crash) is suboptimal, I can't entirely fault the compiler for not dealing with 700 else-ifs. In fact the else-if chain is a problem for other compilers too, it seems to be hitting something in the vicinity of O(n^2) in gcc. Greetings, Andres Freund
Andres Freund <andres@anarazel.de> writes:
> On 2024-07-09 09:14:33 +0100, Dave Page wrote:
>> Packagers aren't likely to be using readline, as it's GPL and it would have
>> to be shipped with packages on Windows.
> I'm not sure (I mean that literally, not as a way to state that I think it's
> not as you say) it'd actually be *that* big a problem to use readline - sure
> it'd make psql GPL, but that's the only thing using readline. Since psql
> doesn't link to extensible / user provided code, it might actually be ok.
> With openssl < 3.0 the mix between openssl and readline would be problematic,
> IIRC the licenses aren't really compatible. But luckily openssl relicensed to
> apache v2.
One thing that struck me while looking at tab-complete.c just now is
that there are aspects of the readline API that require strings to be
malloc'd by the client (tab-complete.c) and later free'd within
libreadline. I wonder how that will play with Windows' weird rules
about when one DLL's malloc pool will interoperate with another's
(cf PQfreemem). Worst case: the reason no one uses readline under
Windows is that it flat out doesn't work.
regards, tom lane
Hi, On 2024-07-09 17:44:27 -0400, Tom Lane wrote: > Worst case: the reason no one uses readline under Windows is that it flat > out doesn't work. I've just tried it again, it works after splitting the else-if chain. > One thing that struck me while looking at tab-complete.c just now is > that there are aspects of the readline API that require strings to be > malloc'd by the client (tab-complete.c) and later free'd within > libreadline. I wonder how that will play with Windows' weird rules > about when one DLL's malloc pool will interoperate with another's > (cf PQfreemem). It seems to work fine as long as a debug-readline is paired with a debug-psql or a release-readline is paired with a release-psql. Intentionally cross-matching the two does indeed quickly crash, with stack trace that looks like exactly the issue you describe. This is a release readline in a debug psql, but it shouldn't matter which way round. Just doing "tab": # Child-SP RetAddr Call Site 00 00000017`db5ff430 00007ff9`1fa18182 ntdll!RtlReportCriticalFailure+0x56 01 00000017`db5ff520 00007ff9`1fa1846a ntdll!RtlpHeapHandleError+0x12 02 00000017`db5ff550 00007ff9`1fa1e0f1 ntdll!RtlpHpHeapHandleError+0x7a 03 00000017`db5ff580 00007ff9`1f9b79d2 ntdll!RtlpLogHeapFailure+0x45 04 00000017`db5ff5b0 00007ff9`1f9347b1 ntdll!RtlpFreeHeapInternal+0x822c2 05 00000017`db5ff670 00007ff9`1d7df05b ntdll!RtlFreeHeap+0x51 *** WARNING: Unable to verify checksum for C:\dev\postgres-meson\build-ninja-2022\tmp_install\usr\local\pgsql\bin\readline.dll 06 00000017`db5ff6b0 00007ff8`f7ab637c ucrtbase!_free_base+0x1b 07 (Inline Function) --------`-------- readline!_rl_free_match_list+0x19 [C:\dev\vcpkg\buildtrees\readline-win32\src\e6f798e014-dba5d3560f.clean\complete.c@ 1627] 08 00000017`db5ff6e0 00007ff8`f7ab12fc readline!rl_complete_internal+0x4dc [C:\dev\vcpkg\buildtrees\readline-win32\src\e6f798e014-dba5d3560f.clean\complete.c@ 1755] 09 00000017`db5ff750 00007ff8`f7ab1725 readline!_rl_dispatch_subseq+0x2dc [C:\dev\vcpkg\buildtrees\readline-win32\src\e6f798e014-dba5d3560f.clean\readline.c@ 582] 0a (Inline Function) --------`-------- readline!_rl_dispatch+0x18 [C:\dev\vcpkg\buildtrees\readline-win32\src\e6f798e014-dba5d3560f.clean\readline.c@ 530] 0b 00000017`db5ff7a0 00007ff8`f7ab1625 readline!readline_internal_char+0xd5 [C:\dev\vcpkg\buildtrees\readline-win32\src\e6f798e014-dba5d3560f.clean\readline.c@ 449] 0c (Inline Function) --------`-------- readline!readline_internal_charloop+0x13 [C:\dev\vcpkg\buildtrees\readline-win32\src\e6f798e014-dba5d3560f.clean\readline.c@ 490] 0d (Inline Function) --------`-------- readline!readline_internal+0x18 [C:\dev\vcpkg\buildtrees\readline-win32\src\e6f798e014-dba5d3560f.clean\readline.c@ 504] 0e 00000017`db5ff7e0 00007ff7`6f1e9dba readline!readline+0xb5 [C:\dev\vcpkg\buildtrees\readline-win32\src\e6f798e014-dba5d3560f.clean\readline.c@ 300] 0f 00000017`db5ff810 00007ff7`6f1eb211 psql!gets_interactive+0x3a [C:\dev\postgres-meson\src\bin\psql\input.c @ 91] 10 00000017`db5ff850 00007ff7`6f1ee7ec psql!MainLoop+0x3a1 [C:\dev\postgres-meson\src\bin\psql\mainloop.c @ 166] 11 00000017`db5ff980 00007ff7`6f287a99 psql!main+0xcfc [C:\dev\postgres-meson\src\bin\psql\startup.c @ 462] 12 00000017`db5ffaa0 00007ff7`6f2879e2 psql!invoke_main+0x39 [D:\a\_work\1\s\src\vctools\crt\vcstartup\src\startup\exe_common.inl@ 79] 13 00000017`db5ffaf0 00007ff7`6f28789e psql!__scrt_common_main_seh+0x132 [D:\a\_work\1\s\src\vctools\crt\vcstartup\src\startup\exe_common.inl@ 288] 14 00000017`db5ffb60 00007ff7`6f287b0e psql!__scrt_common_main+0xe [D:\a\_work\1\s\src\vctools\crt\vcstartup\src\startup\exe_common.inl@ 331] 15 00000017`db5ffb90 00007ff9`1e9a7374 psql!mainCRTStartup+0xe [D:\a\_work\1\s\src\vctools\crt\vcstartup\src\startup\exe_main.cpp@ 17] 16 00000017`db5ffbc0 00007ff9`1f95cc91 KERNEL32!BaseThreadInitThunk+0x14 17 00000017`db5ffbf0 00000000`00000000 ntdll!RtlUserThreadStart+0x21 executing a statement (crashes after execution): # Child-SP RetAddr Call Site 00 000000db`ae3ff898 00007ff9`1f9cd088 ntdll!RtlpBreakPointHeap+0x16 01 000000db`ae3ff8a0 00007ff9`1f96f6f5 ntdll!RtlpValidateHeapEntry+0x5d858 02 000000db`ae3ff8e0 00007ff9`1d2b6edb ntdll!RtlValidateHeap+0x95 03 000000db`ae3ff930 00007ff8`ba38dc52 KERNELBASE!HeapValidate+0xb 04 000000db`ae3ff960 00007ff8`ba390a76 ucrtbased!_CrtIsValidHeapPointer+0x42 [minkernel\crts\ucrt\src\appcrt\heap\debug_heap.cpp@ 1407] 05 000000db`ae3ff9a0 00007ff8`ba38f565 ucrtbased!free_dbg_nolock+0x136 [minkernel\crts\ucrt\src\appcrt\heap\debug_heap.cpp@ 904] 06 000000db`ae3ffaa0 00007ff8`ba392118 ucrtbased!_free_dbg+0x55 [minkernel\crts\ucrt\src\appcrt\heap\debug_heap.cpp @1030] 07 000000db`ae3ffae0 00007ff7`6f1ebd00 ucrtbased!free+0x28 [minkernel\crts\ucrt\src\appcrt\heap\free.cpp @ 39] 08 000000db`ae3ffb20 00007ff7`6f1ee7ec psql!MainLoop+0xe90 [C:\dev\postgres-meson\src\bin\psql\mainloop.c @ 579] 09 000000db`ae3ffc50 00007ff7`6f287a99 psql!main+0xcfc [C:\dev\postgres-meson\src\bin\psql\startup.c @ 462] 0a 000000db`ae3ffd70 00007ff7`6f2879e2 psql!invoke_main+0x39 [D:\a\_work\1\s\src\vctools\crt\vcstartup\src\startup\exe_common.inl@ 79] 0b 000000db`ae3ffdc0 00007ff7`6f28789e psql!__scrt_common_main_seh+0x132 [D:\a\_work\1\s\src\vctools\crt\vcstartup\src\startup\exe_common.inl@ 288] 0c 000000db`ae3ffe30 00007ff7`6f287b0e psql!__scrt_common_main+0xe [D:\a\_work\1\s\src\vctools\crt\vcstartup\src\startup\exe_common.inl@ 331] 0d 000000db`ae3ffe60 00007ff9`1e9a7374 psql!mainCRTStartup+0xe [D:\a\_work\1\s\src\vctools\crt\vcstartup\src\startup\exe_main.cpp@ 17] 0e 000000db`ae3ffe90 00007ff9`1f95cc91 KERNEL32!BaseThreadInitThunk+0x14 0f 000000db`ae3ffec0 00000000`00000000 ntdll!RtlUserThreadStart+0x21 Note that the line numbers seem to commonly point to where the next frame would return to (i.e. mainloop.c:579 is the call to free, but on return the if (slashCmdStatus == PSQL_CMD_TERMINATE) would be reached, so that's displayed - why I don't know). Greetings, Andres Freund
Andres Freund <andres@anarazel.de> writes:
> On 2024-07-09 17:44:27 -0400, Tom Lane wrote:
>> Worst case: the reason no one uses readline under Windows is that it flat
>> out doesn't work.
> It seems to work fine as long as a debug-readline is paired with a debug-psql
> or a release-readline is paired with a release-psql.
OK, cool. I was concerned about the number of options that have to
match according to our PQfreemem docs. But I guess in practice
Windows packagers would likely compile libreadline as part of their
PG build, so that the build options would always match.
regards, tom lane
On Tue, 9 Jul 2024 at 17:23, Andres Freund <andres@anarazel.de> wrote:
Hi,
On 2024-07-09 09:14:33 +0100, Dave Page wrote:
> On Mon, 8 Jul 2024 at 21:08, Andres Freund <andres@anarazel.de> wrote:
> > I think we'd need to backpatch more for older branches. At least
> >
> > commit 3f28bd7337d
> > Author: Thomas Munro <tmunro@postgresql.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.
>
>
> Packagers aren't likely to be using readline, as it's GPL and it would have
> to be shipped with packages on Windows.
I'm not sure (I mean that literally, not as a way to state that I think it's
not as you say) it'd actually be *that* big a problem to use readline - sure
it'd make psql GPL, but that's the only thing using readline. Since psql
doesn't link to extensible / user provided code, it might actually be ok.
With openssl < 3.0 the mix between openssl and readline would be problematic,
IIRC the licenses aren't really compatible. But luckily openssl relicensed to
apache v2.
Certainly in the case of the packages produced at EDB, we didn't want any potential surprises for users/redistributors/embedders, so *any* GPL was a problem.
But that is pretty orthogonal to the issue at hand:
Right.
What is more relevant is that as far as I can see, we've never actually supported either libedit or readline in MSVC++ builds - which kinda makes sense because Windows terminals are very different from traditional *nix ones. Windows isn't supported by either libedit or readline as far as I can see, except through a couple of forks.
I imagine from mingw/cygwin builds, readline would only actually work properly in their respective terminals.
Dave Page
pgAdmin: https://www.pgadmin.org
On 2024-07-10 We 5:55 AM, Dave Page wrote:
What is more relevant is that as far as I can see, we've never actually supported either libedit or readline in MSVC++ builds - which kinda makes sense because Windows terminals are very different from traditional *nix ones. Windows isn't supported by either libedit or readline as far as I can see, except through a couple of forks.I imagine from mingw/cygwin builds, readline would only actually work properly in their respective terminals.
configure.ac explicitly forces --without-readline on win32, so no for mingw. configure.ac claims there are issues especially with non-US code pages.
One of the reasons I've continued to support building with Cygwin is that its readline does seem to work, making its psql the best I know of on Windows.
cheers
andrew
-- Andrew Dunstan EDB: https://www.enterprisedb.com
Hi, On 2024-07-10 10:55:50 +0100, Dave Page wrote: > What is more relevant is that as far as I can see, we've never actually > supported either libedit or readline in MSVC++ builds - which kinda makes > sense because Windows terminals are very different from traditional *nix > ones. Windows isn't supported by either libedit or readline as far as I can > see, except through a couple of forks. > > I imagine from mingw/cygwin builds, readline would only actually work > properly in their respective terminals. FWIW, if you can get readline to build (using something like [1] or one of the forks that add the support) these days it does work even in the plain windows "command prompt". The only obvious thing that doesn't seem to work is ctrl-c cancelling queries rather than terminating psql. But I think that's an issue independent of readline. Greetings, Andres Freund [1] https://github.com/msys2/MINGW-packages/tree/master/mingw-w64-readline
