Re: Re: Proposed Windows-specific change: Enable crash dumps (like core files)

On Fri, Dec 17, 2010 at 12:08, Craig Ringer <craig(at)postnewspapers(dot)com(dot)au> wrote:
> On 17/12/2010 3:46 PM, Magnus Hagander wrote:
>>
>> On Dec 17, 2010 8:02 AM, "Craig Ringer" <craig(at)postnewspapers(dot)com(dot)au
>> <mailto:craig(at)postnewspapers(dot)com(dot)au>> wrote:
>>  >
>>  > On 16/12/10 21:01, Magnus Hagander wrote:
>>  >
>>  > > Found another problem in it: when running with an older version of
>>  > > dbghelp.dll (which I was), it simply didn't work. We need to grab the
>>  > > version of dbghelp.dll at runtime and pick which things we're going
>> to
>>  > > dump based on that.
>>  >
>>  > I was about to suggest dropping the fallback loading of the system
>>  > dbghelp.dll, and just bundle a suitable version with Pg, then I saw how
>>  > big the DLL is. It's 800kb (!!) of code that'll hopefully never get
>> used
>>  > on any given system. With a footprint like that, bundling it seems
>>  > rather less attractive.
>>  >
>>  > I think Magnus is right: test the dbghelp.dll version and fall back to
>>  > supported features - as far back as XP, anyway; who cares about
>> anything
>>  > earlier than that. An updated version can always be put in place by the
>>  > user if the built-in one can't produce enough detail.
>>
>> Did you get a chance to test that it still produced a full dump on your
>> system?
>
> I've just tested that and it looks ok so far. The dumps produced are smaller
> - 1.3MB instead of ~4MB for a dump produced by calling a simple "crashme()"
> function from SQL, the source for which follows at the end of this post.
> However, I'm getting reasonable stack traces, as shown by the windb.exe
> output below.
>
> That said, the dump appears to exclude the backend's private memory. I can't
> resolve *fcinfo from PG_FUNCTION_ARGS; it looks like the direct values of
> arguments are available as they're on the stack, as are function local
> variables, but the process heap isn't dumped so you can't see the contents
> of any pointers-to-struct. That'll make it harder to track down many kinds
> of error - but, on the other hand, means that dumps of processes with huge
> work_mem etc won't be massively bloated.

What version of dbghelp do you have? And what does the API report for
it? The code is supposed to add the private memory if it finds version
6 or higher, perhaps that check is incorrect?

> That might be a safer starting point than including the private process
> memory, really. I'd like to offer a flag to turn on dumping of private
> process memory but I'm not sure how best to go about it, given that we'd
> want to avoid accessing GUCs etc if possible. "later", I think; this is
> better for now.

I'm not too happy with having a bunch of switches for it. People
likely to tweak those can just install the debugger tools and use
those, I think.

So I'd be happy to have it enabled - given that we want a default.
However, what I'm most interested in right now is finding out why it's
not being included anymore in the new patch, because it's supposed to
be..

> I'm happy with the patch as it stands, with the one exception that
> consideration of mingw is required before it can be committed.

What do you mean? That it has to be tested on mingw specifically, or
something else?

--
 Magnus Hagander
 Me: http://www.hagander.net/
 Work: http://www.redpill-linpro.com/