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
From ba3bf14d523e4c656be1cf3d8bdb80afaa3355ed Mon Sep 17 00:00:00 2001
From: Bryan Green <[email protected]>
Date: Fri, 31 Oct 2025 20:27:38 -0600
Subject: [PATCH v5] Add backtrace support for Windows using DbgHelp API
Previously, backtrace generation on Windows would return an "unsupported"
message. This patch implements Windows backtrace support using
CaptureStackBackTrace() for capturing the call stack and the DbgHelp API
(SymFromAddrW, SymGetLineFromAddr64) for symbol resolution.
The implementation provides symbol names, offsets, addresses, and when PDB
files are available, source file names and line numbers. Symbol names and
file paths are converted from UTF-16 to the database encoding using
wchar2char(), which properly handles both UTF-8 and non-UTF-8 databases on
Windows. When symbol information is unavailable or encoding conversion
fails, it falls back to displaying raw addresses.
The implementation uses the explicit Unicode versions of the DbgHelp
functions (SYMBOL_INFOW, SymFromAddrW) rather than the generic versions.
This is necessary because the generic SYMBOL_INFO becomes SYMBOL_INFOA on
PostgreSQL's Windows builds (which don't define UNICODE), providing strings
in the Windows ANSI codepage rather than a predictable encoding that can be
converted to the database encoding.
Symbol handler initialization (SymInitialize) is performed once per process
and cached. If initialization fails, a warning is logged and no backtrace
is generated.
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.
Author: Bryan Green <[email protected]>
Reviewed-by: Euler Taveira <[email protected]>
Reviewed-by: Jakub Wartak <[email protected]>
---
src/backend/meson.build | 5 ++
src/backend/utils/error/elog.c | 151 ++++++++++++++++++++++++++++++++-
2 files changed, 155 insertions(+), 1 deletion(-)
diff --git a/src/backend/meson.build b/src/backend/meson.build
index b831a54165..eeb69c4079 100644
--- a/src/backend/meson.build
+++ b/src/backend/meson.build
@@ -1,6 +1,11 @@
# Copyright (c) 2022-2025, PostgreSQL Global Development Group
backend_build_deps = [backend_code]
+
+if host_system == 'windows' and cc.get_id() == 'msvc'
+ backend_build_deps += cc.find_library('dbghelp')
+endif
+
backend_sources = []
backend_link_with = [pgport_srv, common_srv]
diff --git a/src/backend/utils/error/elog.c b/src/backend/utils/error/elog.c
index 29643c5143..17a3ba256a 100644
--- a/src/backend/utils/error/elog.c
+++ b/src/backend/utils/error/elog.c
@@ -66,11 +66,15 @@
#include <execinfo.h>
#endif
+#ifdef _MSC_VER
+#include <dbghelp.h>
+#include <windows.h>
+#endif
+
#include "access/xact.h"
#include "common/ip.h"
#include "libpq/libpq.h"
#include "libpq/pqformat.h"
-#include "mb/pg_wchar.h"
#include "miscadmin.h"
#include "nodes/miscnodes.h"
#include "pgstat.h"
@@ -140,6 +144,11 @@ static void write_syslog(int level, const char *line);
static void write_eventlog(int level, const char *line, int len);
#endif
+#ifdef _MSC_VER
+static bool backtrace_symbols_initialized = false;
+static HANDLE backtrace_process = NULL;
+#endif
+
/* We provide a small stack of ErrorData records for re-entrant cases */
#define ERRORDATA_STACK_SIZE 5
@@ -1121,6 +1130,13 @@ errbacktrace(void)
* specifies how many inner frames to skip. Use this to avoid showing the
* internal backtrace support functions in the backtrace. This requires that
* this and related functions are not inlined.
+ *
+ * Platform-specific implementations:
+ * - Unix/Linux: Uses backtrace() and backtrace_symbols()
+ * - Windows: Uses CaptureStackBackTrace() with DbgHelp for symbol resolution
+ * (requires PDB files; falls back to exported functions/raw addresses if
+ * unavailable)
+ * - Other: Returns unsupported message
*/
static void
set_backtrace(ErrorData *edata, int num_skip)
@@ -1136,6 +1152,14 @@ set_backtrace(ErrorData *edata, int num_skip)
char **strfrms;
nframes = backtrace(buf, lengthof(buf));
+
+ if (nframes == 0)
+ {
+ appendStringInfoString(&errtrace, "\nNo stack frames
captured");
+ edata->backtrace = errtrace.data;
+ return;
+ }
+
strfrms = backtrace_symbols(buf, nframes);
if (strfrms != NULL)
{
@@ -1147,6 +1171,131 @@ set_backtrace(ErrorData *edata, int num_skip)
appendStringInfoString(&errtrace,
"insufficient memory for backtrace generation");
}
+#elif defined(_MSC_VER)
+ {
+ void *buf[100];
+ int nframes;
+ char buffer[sizeof(SYMBOL_INFOW) +
MAX_SYM_NAME * sizeof(wchar_t)];
+ PSYMBOL_INFOW symbol;
+
+ if (!backtrace_symbols_initialized)
+ {
+ backtrace_process = GetCurrentProcess();
+
+ SymSetOptions(SYMOPT_UNDNAME |
+ SYMOPT_DEFERRED_LOADS |
+ SYMOPT_LOAD_LINES |
+ SYMOPT_FAIL_CRITICAL_ERRORS);
+
+ if (!SymInitialize(backtrace_process, NULL, TRUE))
+ {
+ elog(WARNING, "could not initialize the symbol
handler: error code %lu",
+ GetLastError());
+ edata->backtrace = errtrace.data;
+ return;
+ }
+ backtrace_symbols_initialized = true;
+ }
+
+ nframes = CaptureStackBackTrace(num_skip, lengthof(buf), buf,
NULL);
+
+ if (nframes == 0)
+ {
+ appendStringInfoString(&errtrace, "\nNo stack frames
captured");
+ edata->backtrace = errtrace.data;
+ return;
+ }
+
+ symbol = (PSYMBOL_INFOW) buffer;
+ symbol->MaxNameLen = MAX_SYM_NAME;
+ symbol->SizeOfStruct = sizeof(SYMBOL_INFOW);
+
+ for (int i = 0; i < nframes; i++)
+ {
+ DWORD64 address = (DWORD64)buf[i];
+ DWORD64 displacement = 0;
+ BOOL sym_result;
+
+ sym_result = SymFromAddrW(backtrace_process,
+
address,
+
&displacement,
+
symbol);
+
+ if (sym_result)
+ {
+ IMAGEHLP_LINEW64 line;
+ DWORD line_displacement = 0;
+ char symbol_name[MAX_SYM_NAME];
+ size_t result;
+
+ line.SizeOfStruct = sizeof(IMAGEHLP_LINE64);
+
+ /*
+ * Convert symbol name from UTF-16 to database
encoding using
+ * wchar2char(), which handles both UTF-8 and
non-UTF-8 databases
+ * correctly on Windows.
+ */
+ result = wchar2char(symbol_name, (const wchar_t
*) symbol->Name,
+
sizeof(symbol_name), NULL);
+
+ if (result == (size_t) -1)
+ {
+ /* Conversion failed, use address only
*/
+ appendStringInfo(&errtrace,
+
"\n[0x%llx]",
+
(unsigned long long) address);
+ continue;
+ }
+
+ if (SymGetLineFromAddrW64(backtrace_process,
+
address,
+
&line_displacement,
+
&line))
+ {
+ char filename[MAX_PATH];
+
+ /* Convert filename from UTF-16 to
database encoding */
+ result = wchar2char(filename, (const
wchar_t *) line.FileName,
+
sizeof(filename), NULL);
+
+ 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);
+ }
+ }
+ else
+ {
+ elog(WARNING, "symbol lookup failed: error code
%lu",
+ GetLastError());
+ edata->backtrace = errtrace.data;
+ }
+ }
+ }
#else
appendStringInfoString(&errtrace,
"backtrace generation is not
supported by this installation");
--
2.46.0.windows.1
