Re: patch for distinguishing PG instances in event log

Hello,

I wrote and attached a patch for the TODO item below (which I proposed).

Allow multiple Postgres clusters running on the same machine to distinguish
themselves in the event log
http://archives.postgresql.org/pgsql-hackers/2011-03/msg01297.php
http://archives.postgresql.org/pgsql-hackers/2011-05/msg00574.php

I changed two things from the original proposal.

1. regsvr32.exe needs /n when you specify event source
I described the reason in src/bin/pgevent/pgevent.c.

2. I moved the article for event log registration to more suitable place
The traditional place and what I originally proposed were not best, because
those who don't build from source won't read those places.

I successfully tested event log registration/unregistration, event logging
with/without event_source parameter, and SHOWing event_source parameter with
psql on Windows Vista (32-bit). I would appreciate if someone could test it
on 64-bit Windows who has the 64-bit environment.

I'll add this patch to the first CommitFest of 9.2. Thank you in advance for
reviewing it.

Regards
MauMau

All,

Merlin volunteered to review this patch and has not turned in a review.

Can someone who is Windows-saavy pitch in and review it ASAP?

> I wrote and attached a patch for the TODO item below (which I proposed).
>
> Allow multiple Postgres clusters running on the same machine to
> distinguish themselves in the event log
> http://archives.postgresql.org/pgsql-hackers/2011-03/msg01297.php
> http://archives.postgresql.org/pgsql-hackers/2011-05/msg00574.php

--
Josh Berkus
PostgreSQL Experts Inc.
http://pgexperts.com

2011/5/26 MauMau <maumau307(at)gmail(dot)com>:
> Hello,
>
> I wrote and attached a patch for the TODO item below (which I proposed).
>
> Allow multiple Postgres clusters running on the same machine to distinguish
> themselves in the event log
> http://archives.postgresql.org/pgsql-hackers/2011-03/msg01297.php
> http://archives.postgresql.org/pgsql-hackers/2011-05/msg00574.php
>
> I changed two things from the original proposal.
>
> 1. regsvr32.exe needs /n when you specify event source
> I described the reason in src/bin/pgevent/pgevent.c.
>
> 2. I moved the article for event log registration to more suitable place
> The traditional place and what I originally proposed were not best, because
> those who don't build from source won't read those places.
>
> I successfully tested event log registration/unregistration, event logging
> with/without event_source parameter, and SHOWing event_source parameter with
> psql on Windows Vista (32-bit). I would appreciate if someone could test it
> on 64-bit Windows who has the 64-bit environment.
>
> I'll add this patch to the first CommitFest of 9.2. Thank you in advance for
> reviewing it.

+ <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?

* Also, what is the use for set_eventlog_parameters()? It's just a
string variable, it shuold work without it.

* 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.

* The guc also needs to go in postgresql.conf.sample

* We never build in unicode mode, so all those checks are unnecessary.

* Are we really allowed to call MessageBox in DlLRegisterService?
Won't that break badly in cases like silent installs?

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.

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

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.

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.
--------------------------------------------------

> * 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.

> * 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.

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 ...

> * 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.

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.

Regards
MauMau

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/