Re: patch for distinguishing PG instances in event log

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/