Re: patch for distinguishing PG instances in event log

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