Re: [PATCH] Add Windows support for backtrace_functions (MSVC only)

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