On 10/20/2025 3:10 AM, Jakub Wartak wrote:
> On Thu, Oct 9, 2025 at 5:50 PM Bryan Green <dbryan(dot)green(at)gmail(dot)com> wrote:
>
> Hi Bryan!
>
>> 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.
>
> 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