Re: patch for distinguishing PG instances in event log - Mailing list pgsql-hackers

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