Thread

1. patch for distinguishing PG instances in event log

   MauMau <maumau307@gmail.com> — 2011-05-26T15:24:59Z

   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
   

2. Re: patch for distinguishing PG instances in event log

   Josh Berkus <josh@agliodbs.com> — 2011-07-10T18:34:39Z

   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
   
   

3. Re: patch for distinguishing PG instances in event log

   Magnus Hagander <magnus@hagander.net> — 2011-07-14T15:00:51Z

   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/
   

4. Re: patch for distinguishing PG instances in event log

   MauMau <maumau307@gmail.com> — 2011-07-15T14:03:11Z

   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
   
   
   
   
   

5. Re: patch for distinguishing PG instances in event log

   Magnus Hagander <magnus@hagander.net> — 2011-07-15T22:29:16Z

   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/