From: | Magnus Hagander <magnus(at)hagander(dot)net> |
---|---|
To: | MauMau <maumau307(at)gmail(dot)com> |
Cc: | pgsql-hackers(at)postgresql(dot)org |
Subject: | Re: patch for distinguishing PG instances in event log |
Date: | 2011-07-15 22:29:16 |
Message-ID: | CABUevEzLNsSo4PoHtZ27_+qWGnWbhWb7cagNVtB08cQ=cLYHng@mail.gmail.com |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
On Fri, Jul 15, 2011 at 15:03, MauMau <maumau307(at)gmail(dot)com> wrote:
> Magnus,
>
> Thank you for reviewing my patch. I'm going to modify the patch according to
> your comments and re-submit it. Before that, I'd like to discuss some points
> and get your agreement.
Ok, please do. If you want to, you can work off my git branch at
http://github.com/mhagander/postgres (branch multievent).
> From: "Magnus Hagander" <magnus(at)hagander(dot)net>
>>
>> + <para>
>> + On Windows, you need to register an event source
>> + and its library with the operating system in order
>> + to make use of the <systemitem>eventlog</systemitem> option for
>> + <varname>log_destination</>.
>> + See <xref linkend="event-log-registration"> for details.
>> + </para>
>>
>> * This part is not strictly correct - you don't *need* to do that, it
>> just makes things look nicer, no?
>
> How about the following statement? Is it better to say "correctly" instead
> of "cleanly"?
>
> --------------------------------------------------
> On Windows, when you use the eventlog option for log_destination, you need
> to register an event sourceand its library with the operating system so that
> the Windows Event Viewer can display event log messages cleanly.
> --------------------------------------------------
Replace "need" with "should" and I'm happy with that.
>> * Also, what is the use for set_eventlog_parameters()? It's just a
>> string variable, it shuold work without it.
>
> Yes, exactly. I'll follow your modification.
>
>> * We these days avoid #ifdef'ing gucs just because they are not on
>> that platform - so the list is consistent. The guc should be available
>> on non-windows platforms as well.
>
> I see. When I saw syslog_ident not being #ifndef WIN32'ed, I thought that
> was because syslog might be available on Cygwin or MinGW. Anyway, I'll take
> your modification.
Nope, it used to be #ifdefed on HAVE_SYSLOG, but we changed that a
while ago for consistency.
>> * The guc also needs to go in postgresql.conf.sample
>
> I missed this completely.
>
>> * Are we really allowed to call MessageBox in DlLRegisterService?
>> Won't that break badly in cases like silent installs?
>
> It seems that the only way to notify the error is MessageBox. Printing to
> stdout/stderr does not show any messages on the command prompt. I guess
> that's why the original author of pgevent.c used MessageBox.
oh, we're already using messagebox.. I must've been confused, I
thought it was new. Heck, it might even be me who wrote that :O
> We cannot know within the DLL if /s (silent) switch was specified to
> regsvr32.exe. If we want to suppress MessageBox during silent installs, it
> seems that we need to know the silent install by an environment variable or
> so. That is:
>
> C:\> set PGSILENT=true
> C:\> regsvr32.exe ...
I think if we're not Ok with messagebox, then we should just write it
to the eventlog. However, given that we have been using messagebox
before and had no complaints, I should withdraw my complaint for now -
keep using messagebox like the surrounding code. We can attack the
potential issue of that in a separate patch later.
>> * We never build in unicode mode, so all those checks are unnecessary.
>>
>> Attached is an updated patch, which doesn't work yet. I believe the
>> changes to the backend are correct, but probably some of the cleanups
>> and changes in the dll are incorrect, because I seem to be unable to
>> register either the default or a custom handler so far.
>
> The cause of the problem you are encountering is here:
>
> + if (pszCmdLine && *pszCmdLine != '\0')
> + strncpy(event_source, sizeof(event_source), pszCmdLine);
> + else
> + strcpy(event_source, "PostgreSQL");
>
> DllInstall() always receives the /i argument in UTF-16. str... functions
> like strncpy() cannot be used for handling UTF-16 strings. For example, when
> you run "regsvr32.exe /i:abc ...", DllInstall's pszCmdLine value becomes
> "a\0b\0c\0". So, strncpy() just copies "a". This is the reason why you
> cannot register the custom event source.
Oh, it gets it as UTF-16 even when not in unicode mode. I see - yeah,
I didn't have time to read up on the calling conventions properly.
> The reason why you cannot register the default event source (i.e. running
> regsvr32 without /i) is that you memset()ed event_source in DllMain() and
> set "PostgreSQL" to it in DllInstall(). DllInstall() is not called when /i
> is not specified. So, event_source remains empty.
>
> To solve the problem, we need to use wcstombs_s() instead of strncpy(), and
> initialize event_source to "PostgreSQL" when it is defined.
Ok, seems reasonable.
--
Magnus Hagander
Me: http://www.hagander.net/
Work: http://www.redpill-linpro.com/
From | Date | Subject | |
---|---|---|---|
Next Message | Josh Berkus | 2011-07-15 22:33:14 | Re: Is there a committer in the house? |
Previous Message | Kevin Grittner | 2011-07-15 22:23:10 | isolation tests broken for other than 'read committed' |