Обсуждение: patch for distinguishing PG instances in event log

Поиск
Список
Период
Сортировка

patch for distinguishing PG instances in event log

От
"MauMau"
Дата:
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

Вложения

Re: patch for distinguishing PG instances in event log

От
Josh Berkus
Дата:
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


Re: patch for distinguishing PG instances in event log

От
Magnus Hagander
Дата:
2011/5/26 MauMau <maumau307@gmail.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/

Вложения

Re: patch for distinguishing PG instances in event log

От
"MauMau"
Дата:
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@hagander.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





Re: patch for distinguishing PG instances in event log

От
Magnus Hagander
Дата:
On Fri, Jul 15, 2011 at 15:03, MauMau <maumau307@gmail.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@hagander.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/