Re: patch for distinguishing PG instances in event log

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/