Re: [PATCH] Add Windows support for backtrace_functions (MSVC only) - Mailing list pgsql-hackers

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