Обсуждение: [PATCH] Add Windows support for backtrace_functions (MSVC only)
Hi,
I've implemented Windows support for the backtrace_functions configuration
parameter using the DbgHelp API. This brings Windows debugging capabilities
closer to parity with Unix/Linux platforms.
Currently, backtrace_functions only works on Unix-like systems. This patch
adds equivalent functionality for Windows MSVC builds using:
- CaptureStackBackTrace() for capturing the call stack
- SymFromAddrW() and SymGetLineFromAddrW64() for symbol resolution
- UTF-8 conversion to handle international file paths correctly
When PDB symbol files are available, backtraces show function names,
offsets,
file paths, and line numbers. Without PDB files, raw memory addresses
are shown.
Testing performed:
- MSVC build with PDB files: Backtraces include function names, offsets,
source files, and line numbers
- MSVC build without PDB files: Backtraces show memory addresses only
- Verified backtraces appear in server logs when backtrace_functions is set
- Confirmed no crashes or memory leaks
- Passed all tests with Cirrus CI for all defined platforms.
The implementation is MSVC-specific. MinGW builds will use the existing
fallback message ("backtrace generation is not supported by this
installation").
The patch is attached. I'm happy to make revisions based on feedback.
Best regards,
Bryan Green
Вложения
On Thu, Oct 9, 2025 at 5:50 PM Bryan Green <dbryan.green@gmail.com> wrote:
Hi Bryan!
> I've implemented Windows support for the backtrace_functions configuration parameter using the DbgHelp API. This
bringsWindows debugging capabilities closer to parity with Unix/Linux platforms.
Cool, thanks for working on this. Win32 is a bit alien to me, but I've
got access to win32 so I've played with the patch a little bit. With
simple: SET backtrace_functions = 'typenameType'; CREATE TABLE tab (id
invalidtype);
I've got
2025-10-20 08:47:25.440 CEST [25700] ERROR: type "invalidtype"
does not exist at character 22
2025-10-20 08:47:25.440 CEST [25700] BACKTRACE:
typenameType+0x86
[C:\git\postgres-master\src\backend\parser\parse_type.c:270]
transformColumnDefinition+0x1df
[C:\git\postgres-master\src\backend\parser\parse_utilcmd.c:649]
transformCreateStmt+0x306
[C:\git\postgres-master\src\backend\parser\parse_utilcmd.c:279]
[..]
main+0x4f8 [C:\git\postgres-master\src\backend\main\main.c:218]
__scrt_common_main_seh+0x10c
[D:\a\_work\1\s\src\vctools\crt\vcstartup\src\startup\exe_common.inl:288]
BaseThreadInitThunk+0x17 [0x7ffc5f06e8d7]
RtlUserThreadStart+0x2c [0x7ffc5ff08d9c]
2025-10-20 08:47:25.440 CEST [25700] STATEMENT: CREATE TABLE tab
(id invalidtype);
> Without PDB files, raw memory addresses are shown.
and after removing PDB files, so it's how the real distribution of
Win32 are shipped, it's still showing function names (but MSVC
shouldn't be putting symbols into binaries, right? I mean it's better
than I expected):
2025-10-20 09:49:03.061 CEST [22832] ERROR: type "invalidtype"
does not exist at character 22
2025-10-20 09:49:03.061 CEST [22832] BACKTRACE:
typenameType+0x86 [0x7ff71e6e3426]
transformAlterTableStmt+0xb7f [0x7ff71e6e5eef]
transformCreateStmt+0x306 [0x7ff71e6e78a6]
[..]
meanwhile on Linux:
2025-10-20 08:49:11.758 CEST [4310] ERROR: type "invalidtype"
does not exist at character 22
2025-10-20 08:49:11.758 CEST [4310] BACKTRACE:
postgres: postgres postgres [local] CREATE
TABLE(typenameType+0x86) [0x560f082eeb06]
postgres: postgres postgres [local] CREATE
TABLE(+0x36ae37) [0x560f082efe37]
postgres: postgres postgres [local] CREATE
TABLE(transformCreateStmt+0x386) [0x560f082ef676]
[..]
Clearly the MSVC version by default seems to be more reliable in terms
of symbols resolution. Anyway:
1. Should outputs be aligned in terms of process title, so should we
aim with this $patch to also include the full process name (like
`postgres: postgres postgres [local]` above and often the operation
`CREATE TABLE` too) which seems to be coming from setproctitle(3bsd)
and does depend on update_process_title GUC. However on win32 this is
off (so nobody will touch it), because docs say 'This setting defaults
to on on most platforms, but it defaults to off on Windows due to that
platform's larger overhead for updating the process title'. IMHO it is
good as it is, which is to have something rather than nothing.
Personally for me it is pretty strange that original
backtrace_symbols(3) includes './progname' in the output on Linux, but
it is what we have today.
2. Code review itself:
a. nitpicking nearby:
+ * - Unix/Linux/: Uses backtrace() and backtrace_symbols() <--
git diff shows there's unnecessary empty space at the end
> Confirmed no crashes or memory leaks
b. Shouldn't we call SymCleanup(process) at some point to free
resources anyway? (I'm wondering about the scenario in case a very
long-lived process hits $backtrace_functions every couple of seconds -
wouldn't that cause a leak over a very long term without SymCleanup()
?)
-J.
On Mon, Oct 20, 2025 at 10:10:25AM +0200, Jakub Wartak wrote: > Cool, thanks for working on this. Win32 is a bit alien to me, but I've > got access to win32 so I've played with the patch a little bit. With > simple: SET backtrace_functions = 'typenameType'; CREATE TABLE tab (id > invalidtype); Perhaps it would be possible to add some WIN32-specific tests that check some log patterns based on the backtraces printed? (I have not read the patch in details, just an idea while passing by.) -- Michael
Вложения
On Mon, Oct 20, 2025 at 10:40 AM Michael Paquier <michael@paquier.xyz> wrote: > > On Mon, Oct 20, 2025 at 10:10:25AM +0200, Jakub Wartak wrote: > > Cool, thanks for working on this. Win32 is a bit alien to me, but I've > > got access to win32 so I've played with the patch a little bit. With > > simple: SET backtrace_functions = 'typenameType'; CREATE TABLE tab (id > > invalidtype); > > Perhaps it would be possible to add some WIN32-specific tests that > check some log patterns based on the backtraces printed? (I have not > read the patch in details, just an idea while passing by.) Hi Michael, thanks for stepping in; It looks like the original 71a8a4f6e3654 had no tests and sadly the simpler idea - even lowest client_min_messages settings do not make it possible to send the backtrace log to the frontend (send_message_to_server_log() logs edata->backtrace, but send_message_to_frontend() does not). Also is it worth it to test that setting backtrace_funciton=FOO really emits .*FOO.* in log message cross-platform way? Because we would have to accept at least 3 valid results 'not supported' (e.g. mingw) + 'FOO' + what if just the address is returned without function name due to use of some wild compiler options? Just an idea, maybe just setting backtrace_funcitons and *ignoring* output in log, but proving the server did not crash afterwards would be also considered as acceptable? (and have a better testing surface that we have today), but from a different angle it might look like bloatware... -J.
On 10/20/2025 3:10 AM, Jakub Wartak wrote: > On Thu, Oct 9, 2025 at 5:50 PM Bryan Green <dbryan.green@gmail.com> wrote: > > Hi Bryan! > >> I've implemented Windows support for the backtrace_functions configuration parameter using the DbgHelp API. This bringsWindows debugging capabilities closer to parity with Unix/Linux platforms. > > Cool, thanks for working on this. Win32 is a bit alien to me, but I've > got access to win32 so I've played with the patch a little bit. With > simple: SET backtrace_functions = 'typenameType'; CREATE TABLE tab (id > invalidtype); > > I've got > 2025-10-20 08:47:25.440 CEST [25700] ERROR: type "invalidtype" > does not exist at character 22 > 2025-10-20 08:47:25.440 CEST [25700] BACKTRACE: > typenameType+0x86 > [C:\git\postgres-master\src\backend\parser\parse_type.c:270] > transformColumnDefinition+0x1df > [C:\git\postgres-master\src\backend\parser\parse_utilcmd.c:649] > transformCreateStmt+0x306 > [C:\git\postgres-master\src\backend\parser\parse_utilcmd.c:279] > [..] > main+0x4f8 [C:\git\postgres-master\src\backend\main\main.c:218] > __scrt_common_main_seh+0x10c > [D:\a\_work\1\s\src\vctools\crt\vcstartup\src\startup\exe_common.inl:288] > BaseThreadInitThunk+0x17 [0x7ffc5f06e8d7] > RtlUserThreadStart+0x2c [0x7ffc5ff08d9c] > 2025-10-20 08:47:25.440 CEST [25700] STATEMENT: CREATE TABLE tab > (id invalidtype); > >> Without PDB files, raw memory addresses are shown. > > and after removing PDB files, so it's how the real distribution of > Win32 are shipped, it's still showing function names (but MSVC > shouldn't be putting symbols into binaries, right? I mean it's better > than I expected): MSVC does put function names into release builds. You should see function names, raw addresses, NO file paths or line numbers in a release build. PDB files would give you the file paths and line numbers.> 2025-10-20 09:49:03.061 CEST [22832] ERROR: type "invalidtype" > does not exist at character 22 > 2025-10-20 09:49:03.061 CEST [22832] BACKTRACE: > typenameType+0x86 [0x7ff71e6e3426] > transformAlterTableStmt+0xb7f [0x7ff71e6e5eef] > transformCreateStmt+0x306 [0x7ff71e6e78a6] > [..] > > meanwhile on Linux: > 2025-10-20 08:49:11.758 CEST [4310] ERROR: type "invalidtype" > does not exist at character 22 > 2025-10-20 08:49:11.758 CEST [4310] BACKTRACE: > postgres: postgres postgres [local] CREATE > TABLE(typenameType+0x86) [0x560f082eeb06] > postgres: postgres postgres [local] CREATE > TABLE(+0x36ae37) [0x560f082efe37] > postgres: postgres postgres [local] CREATE > TABLE(transformCreateStmt+0x386) [0x560f082ef676] > [..] > > Clearly the MSVC version by default seems to be more reliable in terms > of symbols resolution. Anyway: > > 1. Should outputs be aligned in terms of process title, so should we > aim with this $patch to also include the full process name (like > `postgres: postgres postgres [local]` above and often the operation > `CREATE TABLE` too) which seems to be coming from setproctitle(3bsd) > and does depend on update_process_title GUC. However on win32 this is > off (so nobody will touch it), because docs say 'This setting defaults > to on on most platforms, but it defaults to off on Windows due to that > platform's larger overhead for updating the process title'. IMHO it is > good as it is, which is to have something rather than nothing. > Personally for me it is pretty strange that original > backtrace_symbols(3) includes './progname' in the output on Linux, but > it is what we have today. > I think it is good as is.> 2. Code review itself: > > a. nitpicking nearby: > + * - Unix/Linux/: Uses backtrace() and backtrace_symbols() <-- > git diff shows there's unnecessary empty space at the end > Thanks for catching this.>> Confirmed no crashes or memory leaks > > b. Shouldn't we call SymCleanup(process) at some point to free > resources anyway? (I'm wondering about the scenario in case a very > long-lived process hits $backtrace_functions every couple of seconds - > wouldn't that cause a leak over a very long term without SymCleanup() > ?) > > -J. Yes. We should call cleanup at the backend shutdown, as initialize is called once. I have put together a new patch (for better patch naming) and added the cleanup code. BG
Вложения
On Wed, Oct 22, 2025 at 3:09 AM Bryan Green <dbryan.green@gmail.com> wrote: Hi Bryan, [..] > Yes. We should call cleanup at the backend shutdown, as initialize is > called once. I have put together a new patch (for better patch naming) > and added the cleanup code. I've played a little time with this and this looks good to me , including 5-min pgbench runs with backtrace_functions set (it behaves stable even for pgbench -C which stresses it much). Cfbot is also green. One thing i've I think I've noticed (but I've double-checked that's not related to backtrace_functions set) - so it's that apparently backends on windows leak(?) a tiny bit of memory - it's like 5 backends leak like 5 * <1kB / second @ ~3k TPS total as seen by Resource Manager, yet i have no time to investigate that), anyway it's does not seem to be connected to $topic. Maybe one outstanding question is the answer to Michael's earlier question. -J.
On 10/20/2025 3:40 AM, Michael Paquier wrote:
> On Mon, Oct 20, 2025 at 10:10:25AM +0200, Jakub Wartak wrote:
>> Cool, thanks for working on this. Win32 is a bit alien to me, but I've
>> got access to win32 so I've played with the patch a little bit. With
>> simple: SET backtrace_functions = 'typenameType'; CREATE TABLE tab (id
>> invalidtype);
>
> Perhaps it would be possible to add some WIN32-specific tests that
> check some log patterns based on the backtraces printed? (I have not
> read the patch in details, just an idea while passing by.)
> --
> Michael
Michael,
Thanks for even glancing at this. I did not add any regression
tests because the output goes to the server log and not the client.
BG
On Mon, Oct 27, 2025, at 2:58 PM, Bryan Green wrote: > Thanks for even glancing at this. I did not add any regression > tests because the output goes to the server log and not the client. > Since Michael said WIN32-specific tests and mentioned log pattern, he is referring to TAP tests. You can add src/test/modules/test_backtrace that exercises this code path. I didn't test your patch but I'm wondering if we could add an abstraction here. I mean invent pg_backtrace() and pg_backtrace_symbols() that maps to the current functions (Unix-like). On Windows these functions are mapped to win32_backtrace() (that encapsulates CaptureStackBackTrace()) and a new win32_backtrace_symbols() (that encapsulates the rest of the code). There Windows-specific functions should be added to src/port -- my suggestion is win32backtrace.c. Besides that you have to adjust backtrace_symbols in both configure.ac and meson.build so it runs the code path inside HAVE_BACKTRACE_SYMBOLS. -- Euler Taveira EDB https://www.enterprisedb.com/
On 2025-Oct-27, Euler Taveira wrote: > On Mon, Oct 27, 2025, at 2:58 PM, Bryan Green wrote: > > Thanks for even glancing at this. I did not add any regression > > tests because the output goes to the server log and not the client. > > Since Michael said WIN32-specific tests and mentioned log pattern, he is > referring to TAP tests. You can add src/test/modules/test_backtrace that > exercises this code path. Hmm, are we really sure this is necessary? > I didn't test your patch but I'm wondering if we could add an > abstraction here. I mean invent pg_backtrace() and > pg_backtrace_symbols() that maps to the current functions (Unix-like). Do we really need this? I don't think we're going to add support for backtraces anywhere else any time soon, so this looks premature. What other programs do you think we have where this would be useful? I have a really hard time imagining that things like psql and pg_dump would be improved by having backtrace-reporting support. And if we have a single place in the code using a facility, then ISTM the platform-specific code can live there with no damage. If somebody is interested in adding backtracing other programs in the future, they can introduce the abstraction then -- we will probably now exactly what sort of API would be useful when we have more than one user than now that we have just the backend, with very specific needs. -- Álvaro Herrera Breisgau, Deutschland — https://www.EnterpriseDB.com/ "La primera ley de las demostraciones en vivo es: no trate de usar el sistema. Escriba un guión que no toque nada para no causar daños." (Jakob Nielsen)
On Tue, Oct 28, 2025, at 1:51 PM, Álvaro Herrera wrote: > On 2025-Oct-27, Euler Taveira wrote: > >> On Mon, Oct 27, 2025, at 2:58 PM, Bryan Green wrote: >> > Thanks for even glancing at this. I did not add any regression >> > tests because the output goes to the server log and not the client. >> >> Since Michael said WIN32-specific tests and mentioned log pattern, he is >> referring to TAP tests. You can add src/test/modules/test_backtrace that >> exercises this code path. > > Hmm, are we really sure this is necessary? > Good question. We are testing an external API. Maybe a test in this thread is enough to convince someone that the code is correct. >> I didn't test your patch but I'm wondering if we could add an >> abstraction here. I mean invent pg_backtrace() and >> pg_backtrace_symbols() that maps to the current functions (Unix-like). > > Do we really need this? I don't think we're going to add support for > backtraces anywhere else any time soon, so this looks premature. What > other programs do you think we have where this would be useful? I have > a really hard time imagining that things like psql and pg_dump would be > improved by having backtrace-reporting support. And if we have a single > place in the code using a facility, then ISTM the platform-specific code > can live there with no damage. > It was just a suggestion; not a requirement. As you said it would avoid rework in the future if or when someone decides to use this code in other parts of the software. I wouldn't propose this change if I knew it was complicated to have a simple and clean abstraction. -- Euler Taveira EDB https://www.enterprisedb.com/
On 10/29/2025 6:53 AM, Euler Taveira wrote: > On Tue, Oct 28, 2025, at 1:51 PM, Álvaro Herrera wrote: >> On 2025-Oct-27, Euler Taveira wrote: >> >>> On Mon, Oct 27, 2025, at 2:58 PM, Bryan Green wrote: >>>> Thanks for even glancing at this. I did not add any regression >>>> tests because the output goes to the server log and not the client. >>> >>> Since Michael said WIN32-specific tests and mentioned log pattern, he is >>> referring to TAP tests. You can add src/test/modules/test_backtrace that >>> exercises this code path. >> >> Hmm, are we really sure this is necessary? >> > > Good question. We are testing an external API. Maybe a test in this thread is > enough to convince someone that the code is correct. > Right, attached is v2 with TAP tests added. The test checks whether postgres.pdb exists on disk to figure out what kind of output to expect, then verifies the actual backtrace format matches. This ought to catch the case where the PDB is there but DbgHelp can't load it for some reason, which seems like the most likely failure mode. It also runs a bunch of errors in quick succession to make sure we don't crash or anything. (Can't really detect memory leaks without external tools, so didn't try.)>>> I didn't test your patch but I'm wondering if we could add an >>> abstraction here. I mean invent pg_backtrace() and >>> pg_backtrace_symbols() that maps to the current functions (Unix-like). >> >> Do we really need this? I don't think we're going to add support for >> backtraces anywhere else any time soon, so this looks premature. What >> other programs do you think we have where this would be useful? I have >> a really hard time imagining that things like psql and pg_dump would be >> improved by having backtrace-reporting support. And if we have a single >> place in the code using a facility, then ISTM the platform-specific code >> can live there with no damage. >> > > It was just a suggestion; not a requirement. As you said it would avoid rework > in the future if or when someone decides to use this code in other parts of the > software. I wouldn't propose this change if I knew it was complicated to have a > simple and clean abstraction. > > I am not convinced that we need an abstraction considering our backtrace logic for both platforms is in one place at the moment and not spread throughout the source. I have a hard time imagining this being used anywhere but where it currently is... I am more than happy to do as the community wishes though. The latest patch has tap tests now and I look forward to any guidance the community wishes to provide. Bryan Green
Вложения
On Thu, Oct 30, 2025 at 4:06 AM Bryan Green <dbryan.green@gmail.com> wrote: [..] Hi Bryan, cfbot is red. I'm was fan of having those tests for this (bring complexity and we didn't have tests for Linux backtrace anyway), but now MINGW win32 is failing on those tests where the feature is not present: [03:57:44.552] ------------------------------------- 8< ------------------------------------- [03:57:44.552] stderr: [03:57:44.552] # Failed test 'backtrace has valid format' [03:57:44.552] # at C:/cirrus/src/test/modules/test_backtrace/t/t_windows_backtrace.pl line 106. [03:57:44.552] # Failed test 'Unable to determine scenario - PostgreSQL should always have export symbols' [03:57:44.552] # at C:/cirrus/src/test/modules/test_backtrace/t/t_windows_backtrace.pl line 180. [03:57:44.552] # Failed test 'Scenario mismatch: expected 2, got 0' [03:57:44.552] # at C:/cirrus/src/test/modules/test_backtrace/t/t_windows_backtrace.pl line 224. [03:57:44.552] # Failed test 'PL/pgSQL error has deeper stack (found 0 frames)' [03:57:44.552] # at C:/cirrus/src/test/modules/test_backtrace/t/t_windows_backtrace.pl line 318. [03:57:44.552] # Failed test 'multiple rapid errors produced backtraces (0 addresses found)' [03:57:44.552] # at C:/cirrus/src/test/modules/test_backtrace/t/t_windows_backtrace.pl line 366. [03:57:44.552] # Looks like you failed 5 tests of 19. Anyway, as expected this was thrown: 2025-10-30 03:57:37.973 GMT client backend[244] t_windows_backtrace.pl BACKTRACE: backtrace generation is not supported by this installation Please see attached files for convenience as I already had them on disk. They are from https://commitfest.postgresql.org/patch/6116/ -> https://cirrus-ci.com/task/5155398535086080 -> artifacts. Instead of +# Skip if not Windows +if ($^O ne 'MSWin32') +{ + plan skip_all => 'Windows-specific backtrace tests'; +} Maybe we could also bypass MINGW too like below ? (but I have no access, so i havent tried) use Config; # Skip if not Windows or MINGW/MSYS is detected if ($^O ne 'MSWin32' || $Config{'ccname'} =~ /gcc|mingw/i || defined($ENV{'MSYSTEM'})) { plan skip_all => 'Windows-specific backtrace tests'; } -J.
Вложения
On 10/30/2025 2:01 AM, Jakub Wartak wrote: > On Thu, Oct 30, 2025 at 4:06 AM Bryan Green <dbryan.green@gmail.com> wrote: > > [..] > > Hi Bryan, cfbot is red. I'm was fan of having those tests for this > (bring complexity and we didn't have tests for Linux backtrace > anyway), but now MINGW win32 is failing on those tests where the > feature is not present: > > [03:57:44.552] ------------------------------------- 8< > ------------------------------------- > [03:57:44.552] stderr: > [03:57:44.552] # Failed test 'backtrace has valid format' > [03:57:44.552] # at > C:/cirrus/src/test/modules/test_backtrace/t/t_windows_backtrace.pl > line 106. > [03:57:44.552] # Failed test 'Unable to determine scenario - > PostgreSQL should always have export symbols' > [03:57:44.552] # at > C:/cirrus/src/test/modules/test_backtrace/t/t_windows_backtrace.pl > line 180. > [03:57:44.552] # Failed test 'Scenario mismatch: expected 2, got 0' > [03:57:44.552] # at > C:/cirrus/src/test/modules/test_backtrace/t/t_windows_backtrace.pl > line 224. > [03:57:44.552] # Failed test 'PL/pgSQL error has deeper stack (found > 0 frames)' > [03:57:44.552] # at > C:/cirrus/src/test/modules/test_backtrace/t/t_windows_backtrace.pl > line 318. > [03:57:44.552] # Failed test 'multiple rapid errors produced > backtraces (0 addresses found)' > [03:57:44.552] # at > C:/cirrus/src/test/modules/test_backtrace/t/t_windows_backtrace.pl > line 366. > [03:57:44.552] # Looks like you failed 5 tests of 19. > > Anyway, as expected this was thrown: > 2025-10-30 03:57:37.973 GMT client backend[244] t_windows_backtrace.pl > BACKTRACE: backtrace generation is not supported by this installation > > Please see attached files for convenience as I already had them on > disk. They are from https://commitfest.postgresql.org/patch/6116/ -> > https://cirrus-ci.com/task/5155398535086080 -> artifacts. > > Instead of > +# Skip if not Windows > +if ($^O ne 'MSWin32') > +{ > + plan skip_all => 'Windows-specific backtrace tests'; > +} > > Maybe we could also bypass MINGW too like below ? (but I have no > access, so i havent tried) > use Config; > # Skip if not Windows or MINGW/MSYS is detected > if ($^O ne 'MSWin32' || $Config{'ccname'} =~ /gcc|mingw/i || > defined($ENV{'MSYSTEM'})) > { > plan skip_all => 'Windows-specific backtrace tests'; > } > > -J. Thanks for the feedback. Apologies for forgetting about filtering mingw out of the tests. I have made changes to the test as well as meson.build in the test_backtrace directory to keep it from running on mingw. My cirrus build is all green across all the platforms. Find v3 attached. Bryan Green
Вложения
On 2025-Oct-30, Jakub Wartak wrote: > Hi Bryan, cfbot is red. I'm was fan of having those tests for this > (bring complexity and we didn't have tests for Linux backtrace > anyway), but now MINGW win32 is failing on those tests where the > feature is not present: I hate to say this after the code is written, but I think we should not put any tests in the first step. I predict that these are going to be enormously brittle and that we'll waste a lot of time making them stable. I think we should commit the Windows support for backtraces first, then consider whether we actually want TAP tests for the overall feature. We've gone several years with glibc backtrace support without any tests -- why do we think the Windows implementation thereof _must_ necessarily have them? -- Álvaro Herrera PostgreSQL Developer — https://www.EnterpriseDB.com/ <Schwern> It does it in a really, really complicated way <crab> why does it need to be complicated? <Schwern> Because it's MakeMaker.
On 10/30/2025 3:37 AM, Álvaro Herrera wrote: > On 2025-Oct-30, Jakub Wartak wrote: > >> Hi Bryan, cfbot is red. I'm was fan of having those tests for this >> (bring complexity and we didn't have tests for Linux backtrace >> anyway), but now MINGW win32 is failing on those tests where the >> feature is not present: > > I hate to say this after the code is written, but I think we should not > put any tests in the first step. I predict that these are going to be > enormously brittle and that we'll waste a lot of time making them > stable. I think we should commit the Windows support for backtraces > first, then consider whether we actually want TAP tests for the overall > feature. We've gone several years with glibc backtrace support without > any tests -- why do we think the Windows implementation thereof _must_ > necessarily have them? > It will not bother me to remove them. It was my first effort at writing TAP tests, so it was a nice learning experience.
On Thu, Oct 30, 2025 at 10:40 AM Bryan Green <dbryan.green@gmail.com> wrote: > > On 10/30/2025 3:37 AM, Álvaro Herrera wrote: > > On 2025-Oct-30, Jakub Wartak wrote: > > > >> Hi Bryan, cfbot is red. I'm was fan of having those tests for this > >> (bring complexity and we didn't have tests for Linux backtrace > >> anyway), but now MINGW win32 is failing on those tests where the > >> feature is not present: > > > > I hate to say this after the code is written, but I think we should not > > put any tests in the first step. I predict that these are going to be > > enormously brittle and that we'll waste a lot of time making them > > stable. I think we should commit the Windows support for backtraces > > first, then consider whether we actually want TAP tests for the overall > > feature. We've gone several years with glibc backtrace support without > > any tests -- why do we think the Windows implementation thereof _must_ > > necessarily have them? > > > It will not bother me to remove them. It was my first effort at writing > TAP tests, so it was a nice learning experience. Well, that was a typo on my part (stupid me), I wanted to write: I was NOT a fan of having those tests for this (in first place) - sorry for confusion! Anyway we have test because I think Michael and Euler triggered this but earlier i've tried to persuade NOT to do this (see: `Also is it worth it to test that setting backtrace_funciton=FOO really emits .*FOO.* in log message cross-platform way?`), anyway Bryan implemented this and it looks like v3 has just turned [gG]reen ;) (https://cirrus-ci.com/build/6001832838823936) -J.
On 10/30/2025 3:52 AM, Jakub Wartak wrote: > On Thu, Oct 30, 2025 at 10:40 AM Bryan Green <dbryan.green@gmail.com> wrote: >> >> On 10/30/2025 3:37 AM, Álvaro Herrera wrote: >>> On 2025-Oct-30, Jakub Wartak wrote: >>> >>>> Hi Bryan, cfbot is red. I'm was fan of having those tests for this >>>> (bring complexity and we didn't have tests for Linux backtrace >>>> anyway), but now MINGW win32 is failing on those tests where the >>>> feature is not present: >>> >>> I hate to say this after the code is written, but I think we should not >>> put any tests in the first step. I predict that these are going to be >>> enormously brittle and that we'll waste a lot of time making them >>> stable. I think we should commit the Windows support for backtraces >>> first, then consider whether we actually want TAP tests for the overall >>> feature. We've gone several years with glibc backtrace support without >>> any tests -- why do we think the Windows implementation thereof _must_ >>> necessarily have them? >>> >> It will not bother me to remove them. It was my first effort at writing >> TAP tests, so it was a nice learning experience. > > Well, that was a typo on my part (stupid me), I wanted to write: I was > NOT a fan of having those tests for this (in first place) - sorry for > confusion! > > Anyway we have test because I think Michael and Euler triggered this > but earlier i've tried to persuade NOT to do this (see: `Also is it > worth it to test that setting backtrace_funciton=FOO really emits > .*FOO.* in log message cross-platform way?`), anyway Bryan implemented > this and it looks like v3 has just turned [gG]reen ;) > (https://cirrus-ci.com/build/6001832838823936) > > -J. The tests are easy enough to get rid of. I think Alvaro has a good idea of committing the windows support for backtraces and then consider whether we want TAP tests or not. I will make a v4 patch without the TAP tests unless someone strongly objects. The tests don't really test whether this code would be the cause of cause of a problem, they mainly test whether you are getting the correct output in your backtrace. If you have a pdb file, you should get filenames and linenumbers in addition to addresses and symbols. If you don't have a pdb file you will only get export function symbols and addresses. So, even if you should have a pdb and don't...you still get something useful. Bryan Green
On 10/30/2025 3:52 AM, Jakub Wartak wrote: > On Thu, Oct 30, 2025 at 10:40 AM Bryan Green <dbryan.green@gmail.com> wrote: >> >> On 10/30/2025 3:37 AM, Álvaro Herrera wrote: >>> On 2025-Oct-30, Jakub Wartak wrote: >>> >>>> Hi Bryan, cfbot is red. I'm was fan of having those tests for this >>>> (bring complexity and we didn't have tests for Linux backtrace >>>> anyway), but now MINGW win32 is failing on those tests where the >>>> feature is not present: >>> >>> I hate to say this after the code is written, but I think we should not >>> put any tests in the first step. I predict that these are going to be >>> enormously brittle and that we'll waste a lot of time making them >>> stable. I think we should commit the Windows support for backtraces >>> first, then consider whether we actually want TAP tests for the overall >>> feature. We've gone several years with glibc backtrace support without >>> any tests -- why do we think the Windows implementation thereof _must_ >>> necessarily have them? >>> >> It will not bother me to remove them. It was my first effort at writing >> TAP tests, so it was a nice learning experience. > > Well, that was a typo on my part (stupid me), I wanted to write: I was > NOT a fan of having those tests for this (in first place) - sorry for > confusion! > > Anyway we have test because I think Michael and Euler triggered this > but earlier i've tried to persuade NOT to do this (see: `Also is it > worth it to test that setting backtrace_funciton=FOO really emits > .*FOO.* in log message cross-platform way?`), anyway Bryan implemented > this and it looks like v3 has just turned [gG]reen ;) > (https://cirrus-ci.com/build/6001832838823936) > > -J. I had reservations about the value the tests were adding, and considering I am getting more concern around having the tests than not having them for this initial release I have decided to remove them. v4 patch is attached. It is the same as the initial 0001-* patch. Thanks, Bryan Green
Вложения
On Thu, Oct 30, 2025, at 11:51 AM, Bryan Green wrote:
> I had reservations about the value the tests were adding, and
> considering I am getting more concern around having the tests than not
> having them for this initial release I have decided to remove them. v4
> patch is attached. It is the same as the initial 0001-* patch.
>
I spent some time on this patch. Here are some comments and suggestions.
+#ifdef _MSC_VER
+#include <windows.h>
+#include <dbghelp.h>
+static bool win32_backtrace_symbols_initialized = false;
+static HANDLE win32_backtrace_process = NULL;
+#endif
We usually a different style. Headers go to the top on the same section as
system headers and below postgres.h. It is generally kept sorted. The same
applies to variables. Add them near the other static variables. BTW does it need
the win32_ prefix for a Window-only variable?
+ wchar_t buffer[sizeof(SYMBOL_INFOW) + MAX_SYM_NAME * sizeof(wchar_t)];
+ PSYMBOL_INFOW symbol;
According to [1], SYMBOL_INFO is an alias that automatically selects ANSI vs
UNICODE. Shouldn't we use it instead of SYMBOL_INFOW?
+ elog(WARNING, "SymInitialize failed with error %lu", error);
Is there a reason to continue if SymInitialize failed? It should return after
printing the message. Per the error message style guide [2], my suggestion is
"could not initialize the symbol handler: error code %lu". You can also use
GetLastError() directly in the elog call.
+ symbol = (PSYMBOL_INFOW) buffer;
+ symbol ->MaxNameLen = MAX_SYM_NAME;
+ symbol ->SizeOfStruct = sizeof(SYMBOL_INFOW);
We generally don't add spaces between variable and a member.
+ DWORD i;
I'm curious why did you declare this variable as DWORD? Shouldn't int be
sufficient? The CaptureStackBackTrace function returns an unsigned short
(UShort). You can also declare it in the for loop.
+ DWORD64 address = (DWORD64) (stack[i]);
The parenthesis around stack is superfluous. The code usually doesn't contain
additional parenthesis (unless it improves readability).
+ if (frames == 0)
+ {
+ appendStringInfoString(&errtrace, "\nNo stack frames captured");
+ edata->backtrace = errtrace.data;
+ return;
+ }
It seems CaptureStackBackTrace function may return zero frames under certain
conditions. It is a good point having this additional message. I noticed that
the current code path (HAVE_BACKTRACE_SYMBOLS) doesn't have this block. IIUC,
in certain circumstances (ARM vs unwind-tables flag), the backtrace() also
returns zero frames. Should we add this block for the backtrace() code path?
+ sym_result = SymFromAddrW(win32_backtrace_process,
+ address,
+ &displacement,
+ symbol);
You should use SymFromAddr, no? [3] I saw that you used the Unicode functions
instead of the generic functions [4].
+ /* Convert symbol name to UTF-8 */
+ utf8_len = WideCharToMultiByte(CP_UTF8, 0, symbol->Name, -1,
+ NULL, 0, NULL, NULL);
+ if (utf8_len > 0)
+ {
+ char *filename_utf8;
+ int filename_len;
+
+ utf8_buffer = palloc(utf8_len);
+ WideCharToMultiByte(CP_UTF8, 0, symbol->Name, -1,
+ utf8_buffer, utf8_len, NULL, NULL);
+ /* Convert symbol name to UTF-8 */
+ utf8_len = WideCharToMultiByte(CP_UTF8, 0, symbol->Name, -1,
+ NULL, 0, NULL, NULL);
You are calling WideCharToMultiByte twice. The reason is to allocate the exact
memory size. However, you can adopt another logic to avoid the first call.
maxlen = symbol->NameLen * pg_database_encoding_max_length();
symbol_name = palloc(maxlen + 1);
(I suggest symbol_name instead of ut8_buffer.)
You are considering only the UTF-8 case. Shouldn't it use wcstombs or
wcstombs_l? Maybe (re)use wchar2char -- see pg_locale_libc.c.
+ if (filename_len > 0)
+ {
+ filename_utf8 = palloc(filename_len);
+ WideCharToMultiByte(CP_UTF8, 0, line.FileName, -1,
+ filename_utf8, filename_len,
+ NULL, NULL);
+
+ appendStringInfo(&errtrace,
+ "\n%s+0x%llx [%s:%lu]",
+ utf8_buffer,
+ (unsigned long long) displacement,
+ filename_utf8,
+ (unsigned long) line.LineNumber);
+
+ pfree(filename_utf8);
+ }
+ else
+ {
+ appendStringInfo(&errtrace,
+ "\n%s+0x%llx [0x%llx]",
+ utf8_buffer,
+ (unsigned long long) displacement,
+ (unsigned long long) address);
+ }
Maybe I missed something but is there a reason for not adding the address in the
first condition?
[1] https://learn.microsoft.com/en-us/windows/win32/api/dbghelp/ns-dbghelp-symbol_infow
[2] https://www.postgresql.org/docs/current/error-style-guide.html
[3] https://learn.microsoft.com/en-us/windows/win32/api/dbghelp/nf-dbghelp-symfromaddrw
[4] https://learn.microsoft.com/en-us/windows/win32/intl/conventions-for-function-prototypes
--
Euler Taveira
EDB https://www.enterprisedb.com/
On 10/31/2025 1:46 PM, Euler Taveira wrote:
> On Thu, Oct 30, 2025, at 11:51 AM, Bryan Green wrote:
>> I had reservations about the value the tests were adding, and
>> considering I am getting more concern around having the tests than not
>> having them for this initial release I have decided to remove them. v4
>> patch is attached. It is the same as the initial 0001-* patch.
>>
>
> I spent some time on this patch. Here are some comments and suggestions.
>
Thanks for the review.
> +#ifdef _MSC_VER
> +#include <windows.h>
> +#include <dbghelp.h>
> +static bool win32_backtrace_symbols_initialized = false;
> +static HANDLE win32_backtrace_process = NULL;
> +#endif
>
> We usually a different style. Headers go to the top on the same section as
> system headers and below postgres.h. It is generally kept sorted. The same
Will fix. I will rework the style (error and otherwise) to follow the
project tradition.
> applies to variables. Add them near the other static variables. BTW does it need
> the win32_ prefix for a Window-only variable?
>
> + wchar_t buffer[sizeof(SYMBOL_INFOW) + MAX_SYM_NAME * sizeof(wchar_t)];
> + PSYMBOL_INFOW symbol;
>
> According to [1], SYMBOL_INFO is an alias that automatically selects ANSI vs
> UNICODE. Shouldn't we use it instead of SYMBOL_INFOW?
>
Good point. I was being overly explicit about wanting wide chars, but
you're right that the generic versions are the way to go.
> + elog(WARNING, "SymInitialize failed with error %lu", error);
>
> Is there a reason to continue if SymInitialize failed? It should return after
None at all. Will return immediately.
> printing the message. Per the error message style guide [2], my suggestion is
> "could not initialize the symbol handler: error code %lu". You can also use
> GetLastError() directly in the elog call.
>
> + symbol = (PSYMBOL_INFOW) buffer;
> + symbol ->MaxNameLen = MAX_SYM_NAME;
> + symbol ->SizeOfStruct = sizeof(SYMBOL_INFOW);
>
> We generally don't add spaces between variable and a member.
>
> + DWORD i;
>
> I'm curious why did you declare this variable as DWORD? Shouldn't int be
> sufficient? The CaptureStackBackTrace function returns an unsigned short
> (UShort). You can also declare it in the for loop.
>
Out of habit. I will change it to int.
> + DWORD64 address = (DWORD64) (stack[i]);
>
> The parenthesis around stack is superfluous. The code usually doesn't contain
> additional parenthesis (unless it improves readability).
>
I will remove the parenthesis.
> + if (frames == 0)
> + {
> + appendStringInfoString(&errtrace, "\nNo stack frames captured");
> + edata->backtrace = errtrace.data;
> + return;
> + }
>
> It seems CaptureStackBackTrace function may return zero frames under certain
> conditions. It is a good point having this additional message. I noticed that
> the current code path (HAVE_BACKTRACE_SYMBOLS) doesn't have this block. IIUC,
> in certain circumstances (ARM vs unwind-tables flag), the backtrace() also
> returns zero frames. Should we add this block for the backtrace() code path?
>
Probably, though that seems like separate cleanup. Want me to include it
here or handle separately (in another patch)?
> + sym_result = SymFromAddrW(win32_backtrace_process,
> + address,
> + &displacement,
> + symbol);
>
> You should use SymFromAddr, no? [3] I saw that you used the Unicode functions
> instead of the generic functions [4].
>
> + /* Convert symbol name to UTF-8 */
> + utf8_len = WideCharToMultiByte(CP_UTF8, 0, symbol->Name, -1,
> + NULL, 0, NULL, NULL);
> + if (utf8_len > 0)
> + {
> + char *filename_utf8;
> + int filename_len;
> +
> + utf8_buffer = palloc(utf8_len);
> + WideCharToMultiByte(CP_UTF8, 0, symbol->Name, -1,
> + utf8_buffer, utf8_len, NULL, NULL);
>
> + /* Convert symbol name to UTF-8 */
> + utf8_len = WideCharToMultiByte(CP_UTF8, 0, symbol->Name, -1,
> + NULL, 0, NULL, NULL);
>
> You are calling WideCharToMultiByte twice. The reason is to allocate the exact
> memory size. However, you can adopt another logic to avoid the first call.
>
> maxlen = symbol->NameLen * pg_database_encoding_max_length();
> symbol_name = palloc(maxlen + 1);
>
> (I suggest symbol_name instead of ut8_buffer.)
>
> You are considering only the UTF-8 case. Shouldn't it use wcstombs or
> wcstombs_l? Maybe (re)use wchar2char -- see pg_locale_libc.c.
>
Hmm, you're probably right. I was thinking these Windows API strings
needed special handling, but wchar2char should handle the conversion to
database encoding correctly. Let me test that approach.
> + if (filename_len > 0)
> + {
> + filename_utf8 = palloc(filename_len);
> + WideCharToMultiByte(CP_UTF8, 0, line.FileName, -1,
> + filename_utf8, filename_len,
> + NULL, NULL);
> +
> + appendStringInfo(&errtrace,
> + "\n%s+0x%llx [%s:%lu]",
> + utf8_buffer,
> + (unsigned long long) displacement,
> + filename_utf8,
> + (unsigned long) line.LineNumber);
> +
> + pfree(filename_utf8);
> + }
> + else
> + {
> + appendStringInfo(&errtrace,
> + "\n%s+0x%llx [0x%llx]",
> + utf8_buffer,
> + (unsigned long long) displacement,
> + (unsigned long long) address);
> + }
>
> Maybe I missed something but is there a reason for not adding the address in the
> first condition?
>
No particular reason. I was trying to keep it concise when we have file/
line info, but for consistency it probably should be there.
Will send v5 with these fixes.
>
> [1] https://learn.microsoft.com/en-us/windows/win32/api/dbghelp/ns-dbghelp-symbol_infow
> [2] https://www.postgresql.org/docs/current/error-style-guide.html
> [3] https://learn.microsoft.com/en-us/windows/win32/api/dbghelp/nf-dbghelp-symfromaddrw
> [4] https://learn.microsoft.com/en-us/windows/win32/intl/conventions-for-function-prototypes
>
>
Thanks again for the review,
Bryan
On 10/31/2025 2:07 PM, Bryan Green wrote:
> On 10/31/2025 1:46 PM, Euler Taveira wrote:
>> On Thu, Oct 30, 2025, at 11:51 AM, Bryan Green wrote:
>>> I had reservations about the value the tests were adding, and
>>> considering I am getting more concern around having the tests than not
>>> having them for this initial release I have decided to remove them. v4
>>> patch is attached. It is the same as the initial 0001-* patch.
>>>
>>
>> I spent some time on this patch. Here are some comments and suggestions.
>>
>
> Thanks for the review.
>
>> +#ifdef _MSC_VER
>> +#include <windows.h>
>> +#include <dbghelp.h>
>> +static bool win32_backtrace_symbols_initialized = false;
>> +static HANDLE win32_backtrace_process = NULL;
>> +#endif
>>
>> We usually a different style. Headers go to the top on the same
>> section as
>> system headers and below postgres.h. It is generally kept sorted. The
>> same
>
> Will fix. I will rework the style (error and otherwise) to follow the
> project tradition.
>
>> applies to variables. Add them near the other static variables. BTW
>> does it need
>> the win32_ prefix for a Window-only variable?
>>
>> + wchar_t buffer[sizeof(SYMBOL_INFOW) + MAX_SYM_NAME *
>> sizeof(wchar_t)];
>> + PSYMBOL_INFOW symbol;
>>
>> According to [1], SYMBOL_INFO is an alias that automatically selects
>> ANSI vs
>> UNICODE. Shouldn't we use it instead of SYMBOL_INFOW?
>>
>
> Good point. I was being overly explicit about wanting wide chars, but
> you're right that the generic versions are the way to go.
>
>> + elog(WARNING, "SymInitialize failed with error %lu",
>> error);
>>
>> Is there a reason to continue if SymInitialize failed? It should
>> return after
>
> None at all. Will return immediately.
>
>> printing the message. Per the error message style guide [2], my
>> suggestion is
>> "could not initialize the symbol handler: error code %lu". You can
>> also use
>> GetLastError() directly in the elog call.
>>
>> + symbol = (PSYMBOL_INFOW) buffer;
>> + symbol ->MaxNameLen = MAX_SYM_NAME;
>> + symbol ->SizeOfStruct = sizeof(SYMBOL_INFOW);
>>
>> We generally don't add spaces between variable and a member.
>>
>> + DWORD i;
>>
>> I'm curious why did you declare this variable as DWORD? Shouldn't int be
>> sufficient? The CaptureStackBackTrace function returns an unsigned short
>> (UShort). You can also declare it in the for loop.
>>
>
> Out of habit. I will change it to int.
>
>> + DWORD64 address = (DWORD64) (stack[i]);
>>
>> The parenthesis around stack is superfluous. The code usually doesn't
>> contain
>> additional parenthesis (unless it improves readability).
>>
>
> I will remove the parenthesis.
>
>> + if (frames == 0)
>> + {
>> + appendStringInfoString(&errtrace, "\nNo stack frames
>> captured");
>> + edata->backtrace = errtrace.data;
>> + return;
>> + }
>>
>> It seems CaptureStackBackTrace function may return zero frames under
>> certain
>> conditions. It is a good point having this additional message. I
>> noticed that
>> the current code path (HAVE_BACKTRACE_SYMBOLS) doesn't have this
>> block. IIUC,
>> in certain circumstances (ARM vs unwind-tables flag), the backtrace()
>> also
>> returns zero frames. Should we add this block for the backtrace() code
>> path?
>>
>
> Probably, though that seems like separate cleanup. Want me to include it
> here or handle separately (in another patch)?
>
>> + sym_result = SymFromAddrW(win32_backtrace_process,
>> + address,
>> + &displacement,
>> + symbol);
>>
>> You should use SymFromAddr, no? [3] I saw that you used the Unicode
>> functions
>> instead of the generic functions [4].
>>
>> + /* Convert symbol name to UTF-8 */
>> + utf8_len = WideCharToMultiByte(CP_UTF8, 0,
>> symbol->Name, -1,
>> + NULL, 0, NULL, NULL);
>> + if (utf8_len > 0)
>> + {
>> + char *filename_utf8;
>> + int filename_len;
>> +
>> + utf8_buffer = palloc(utf8_len);
>> + WideCharToMultiByte(CP_UTF8, 0, symbol->Name,
>> -1,
>> + utf8_buffer, utf8_len,
>> NULL, NULL);
>>
>> + /* Convert symbol name to UTF-8 */
>> + utf8_len = WideCharToMultiByte(CP_UTF8, 0,
>> symbol->Name, -1,
>> + NULL, 0, NULL, NULL);
>>
>> You are calling WideCharToMultiByte twice. The reason is to allocate
>> the exact
>> memory size. However, you can adopt another logic to avoid the first
>> call.
>>
>> maxlen = symbol->NameLen * pg_database_encoding_max_length();
>> symbol_name = palloc(maxlen + 1);
>>
>> (I suggest symbol_name instead of ut8_buffer.)
>>
>> You are considering only the UTF-8 case. Shouldn't it use wcstombs or
>> wcstombs_l? Maybe (re)use wchar2char -- see pg_locale_libc.c.
>>
>
> Hmm, you're probably right. I was thinking these Windows API strings
> needed special handling, but wchar2char should handle the conversion to
> database encoding correctly. Let me test that approach.
>
>> + if (filename_len > 0)
>> + {
>> + filename_utf8 = palloc(filename_len);
>> + WideCharToMultiByte(CP_UTF8, 0,
>> line.FileName, -1,
>> + filename_utf8,
>> filename_len,
>> + NULL, NULL);
>> +
>> + appendStringInfo(&errtrace,
>> + "\n%s+0x%llx [%s:%lu]",
>> + utf8_buffer,
>> + (unsigned long long)
>> displacement,
>> + filename_utf8,
>> + (unsigned long)
>> line.LineNumber);
>> +
>> + pfree(filename_utf8);
>> + }
>> + else
>> + {
>> + appendStringInfo(&errtrace,
>> + "\n%s+0x%llx [0x%llx]",
>> + utf8_buffer,
>> + (unsigned long long)
>> displacement,
>> + (unsigned long long)
>> address);
>> + }
>>
>> Maybe I missed something but is there a reason for not adding the
>> address in the
>> first condition?
>>
>
> No particular reason. I was trying to keep it concise when we have file/
> line info, but for consistency it probably should be there.
>
> Will send v5 with these fixes.
>
>>
>> [1] https://learn.microsoft.com/en-us/windows/win32/api/dbghelp/ns-
>> dbghelp-symbol_infow
>> [2] https://www.postgresql.org/docs/current/error-style-guide.html
>> [3] https://learn.microsoft.com/en-us/windows/win32/api/dbghelp/nf-
>> dbghelp-symfromaddrw
>> [4] https://learn.microsoft.com/en-us/windows/win32/intl/conventions-
>> for-function-prototypes
>>
>>
>
> Thanks again for the review,
> Bryan
Hi all,
v5 patch attached, incorporating all of Euler's feedback with a caveat
around unicode.
The most interesting aspect turned out to be the encoding conversion for
symbol names and file paths. Initially I tried using the generic
SYMBOL_INFO and SymFromAddr functions as Euler suggested, but ran into a
subtle issue: on PostgreSQL's Windows builds, these become SYMBOL_INFOA
and SymFromAddrA (the ANSI versions), which return strings in whatever
Windows ANSI codepage happens to be active (CP1252, etc). This doesn't
necessarily match the database encoding.
When I tried converting these with wchar2char(), it failed because the
input wasn't actually wide characters - leading to backtraces showing
only raw addresses even though symbols were present.
The solution was to use the explicit Unicode versions (SYMBOL_INFOW and
SymFromAddrW), which reliably return UTF-16 strings that wchar2char()
can properly convert to the database encoding. This handles both UTF-8
and non-UTF-8 databases correctly, and wchar2char() gracefully returns
-1 on conversion failure rather than throwing errors during error
handling. Of course this also necessitated using IMAGEHLP_LINEW64 and
SymGetLineFromAddrW64.
Tested with both UTF-8 and WIN1252 databases - backtraces now show
proper symbol names in both cases.
This patch also adds a check for zero frames returned by backtrace() on
Unix/Linux platforms, which can occur in certain circumstances such as
ARM builds without unwind tables.
Bryan
Вложения
On Sat, Nov 1, 2025, at 1:40 AM, Bryan Green wrote:
> v5 patch attached, incorporating all of Euler's feedback with a caveat
> around unicode.
>
Thanks for sharing another patch.
> The most interesting aspect turned out to be the encoding conversion for
> symbol names and file paths. Initially I tried using the generic
> SYMBOL_INFO and SymFromAddr functions as Euler suggested, but ran into a
> subtle issue: on PostgreSQL's Windows builds, these become SYMBOL_INFOA
> and SymFromAddrA (the ANSI versions), which return strings in whatever
> Windows ANSI codepage happens to be active (CP1252, etc). This doesn't
> necessarily match the database encoding.
>
Odd. Does the build define UNICODE? (Don't have a Windows machine right now to
explore this case.)
> The solution was to use the explicit Unicode versions (SYMBOL_INFOW and
> SymFromAddrW), which reliably return UTF-16 strings that wchar2char()
> can properly convert to the database encoding. This handles both UTF-8
> and non-UTF-8 databases correctly, and wchar2char() gracefully returns
> -1 on conversion failure rather than throwing errors during error
> handling. Of course this also necessitated using IMAGEHLP_LINEW64 and
> SymGetLineFromAddrW64.
>
Works for me.
> This patch also adds a check for zero frames returned by backtrace() on
> Unix/Linux platforms, which can occur in certain circumstances such as
> ARM builds without unwind tables.
>
Please create a separate patch.
+ if (result != (size_t) -1)
+ {
+ appendStringInfo(&errtrace,
+ "\n%s+0x%llx [0x%llx] [%s:%lu]",
+ symbol_name,
+ (unsigned long long) displacement,
+ (unsigned long long) address,
+ filename,
+ (unsigned long) line.LineNumber);
+ }
+ else
+ {
+ /* Filename conversion failed, omit it */
+ appendStringInfo(&errtrace,
+ "\n%s+0x%llx [0x%llx]",
+ symbol_name,
+ (unsigned long long) displacement,
+ (unsigned long long) address);
+ }
+ }
+ else
+ {
+ /* No line info available */
+ appendStringInfo(&errtrace,
+ "\n%s+0x%llx [0x%llx]",
+ symbol_name,
+ (unsigned long long) displacement,
+ (unsigned long long) address);
+ }
One last suggestion is to have a single code path for these last two conditions
to avoid duplication. Move this if to outside the "if SymGetLineFromAddrW64".
Merge the comment to reflect both conditions (file conversion failed and no
line info).
--
Euler Taveira
EDB https://www.enterprisedb.com/
On 11/1/2025 9:31 AM, Euler Taveira wrote:
> On Sat, Nov 1, 2025, at 1:40 AM, Bryan Green wrote:
>> v5 patch attached, incorporating all of Euler's feedback with a caveat
>> around unicode.
>>
>
> Thanks for sharing another patch.
>
Thanks for the continued review. v6 patch attached with your latest
suggestions incorporated.
>> The most interesting aspect turned out to be the encoding conversion for
>> symbol names and file paths. Initially I tried using the generic
>> SYMBOL_INFO and SymFromAddr functions as Euler suggested, but ran into a
>> subtle issue: on PostgreSQL's Windows builds, these become SYMBOL_INFOA
>> and SymFromAddrA (the ANSI versions), which return strings in whatever
>> Windows ANSI codepage happens to be active (CP1252, etc). This doesn't
>> necessarily match the database encoding.
>>
>
> Odd. Does the build define UNICODE? (Don't have a Windows machine right now to
> explore this case.)
>
PostgreSQL's Windows builds don't define UNICODE globally. I verified
this by checking that SYMBOL_INFO resolves to the ANSI version - when I
tried using it with wchar2char(), the conversion failed because the
input wasn't actually wide characters.
>> The solution was to use the explicit Unicode versions (SYMBOL_INFOW and
>> SymFromAddrW), which reliably return UTF-16 strings that wchar2char()
>> can properly convert to the database encoding. This handles both UTF-8
>> and non-UTF-8 databases correctly, and wchar2char() gracefully returns
>> -1 on conversion failure rather than throwing errors during error
>> handling. Of course this also necessitated using IMAGEHLP_LINEW64 and
>> SymGetLineFromAddrW64.
>>
>
> Works for me.
>
>> This patch also adds a check for zero frames returned by backtrace() on
>> Unix/Linux platforms, which can occur in certain circumstances such as
>> ARM builds without unwind tables.
>>
>
> Please create a separate patch.
Removed the check - I'll submit that separately.
>
> + if (result != (size_t) -1)
> + {
> + appendStringInfo(&errtrace,
> + "\n%s+0x%llx [0x%llx] [%s:%lu]",
> + symbol_name,
> + (unsigned long long) displacement,
> + (unsigned long long) address,
> + filename,
> + (unsigned long) line.LineNumber);
> + }
> + else
> + {
> + /* Filename conversion failed, omit it */
> + appendStringInfo(&errtrace,
> + "\n%s+0x%llx [0x%llx]",
> + symbol_name,
> + (unsigned long long) displacement,
> + (unsigned long long) address);
> + }
> + }
> + else
> + {
> + /* No line info available */
> + appendStringInfo(&errtrace,
> + "\n%s+0x%llx [0x%llx]",
> + symbol_name,
> + (unsigned long long) displacement,
> + (unsigned long long) address);
> + }
>
> One last suggestion is to have a single code path for these last two conditions
> to avoid duplication. Move this if to outside the "if SymGetLineFromAddrW64".
> Merge the comment to reflect both conditions (file conversion failed and no
> line info).
>
>
Done. Now the code prints symbol+offset+address first, then
conditionally appends line info if both SymGetLineFromAddrW64 succeeds
and the filename conversion succeeds. This eliminates the duplication.
Also added the backtrace_cleanup() function per Jakub's request to
properly release DbgHelp resources via SymCleanup() on process exit.
This inadvertently was removed when the TAP tests were removed.
Tested with both debug and release builds. With PDB files present,
backtraces now show symbol names, offsets, addresses, filenames, and
line numbers. Without PDB files (release build), exported function names
are still resolved.
Bryan