Re: [bug fix] pg_ctl always uses the same event source

Hello,

I've removed a limitation regarding event log on Windows with the attached
patch. I hesitate to admit this is a bug fix and want to regard this an
improvement, but maybe it's a bug fix from users' perspective. Actually, I
received problem reports from some users.

[Problem]
pg_ctl always uses event source "PostgreSQL" to output messages to the event
log. Some example messages are:

server starting
server started

This causes the problems below:

1. Cannot distinguish PostgreSQL instances because they use the same event
source.

2. If you use multiple installations of PostgreSQL on the same machine,
whether they are the same or different versions, Windows event viewer
reports an error "event source not found" in the following sequence. Other
system administration software which deal with event log may show other
anomalies.
2-1 Install the first PostgreSQL (e.g. 9.3) into <dir_1>, register
<dir_1>\lib\pgevent.dll for event source "PostgreSQL", then creates
<instance_1>.
2-2 Install the second PostgreSQL (e.g. 9.2) into <dir_2> as part of some
packaged application, register <dir_2>\lib\pgevent.dll for event source
"PostgreSQL", then creates <instance_2>. The application uses PostgreSQL as
its internal data repository. It sets event_source = 'appname_db' in
<instance_2>'s postgresql.conf.
2-3 At this point, there's no problem.
2-4 Uninstall the second PostgreSQL. <instance_2> is deleted. The packaged
application installer may or may not unregister pgevent.dll with
regsvr32.exe /u. ANyway, <dir_2>\bin\pgevent.dll is deleted.
2-5 <instance_1> continues to run and output messages to event log.
2-6 When you view the messages with event viewer, it reports an error
because the event source "PostgreSQL" and/or pgevent.dll was deleted in 2-4.

[Fix]
Make pg_ctl use event_source setting in postgresql.conf. This eliminates
the need for every instance to use the event source "PostgreSQL" for its
pg_ctl.

Regards
MauMau

From: "MauMau" <maumau307(at)gmail(dot)com>
> I've removed a limitation regarding event log on Windows with the attached
> patch. I hesitate to admit this is a bug fix and want to regard this an
> improvement, but maybe it's a bug fix from users' perspective. Actually,
> I
> received problem reports from some users.

I've done a small correction. I removed redundant default value from the
pg_ctl.c.

Regards
MauMau

On Sun, Dec 8, 2013 at 3:41 AM, MauMau <maumau307(at)gmail(dot)com> wrote:

> From: "MauMau" <maumau307(at)gmail(dot)com>
>
> I've removed a limitation regarding event log on Windows with the attached
>> patch. I hesitate to admit this is a bug fix and want to regard this an
>> improvement, but maybe it's a bug fix from users' perspective. Actually,
>> I
>> received problem reports from some users.
>>
>
> I've done a small correction. I removed redundant default value from the
> pg_ctl.c.
>

Not having looked at it in detail yet, but this seems to completely remove
the default value. What happens if the error that needs to be logged is the
one saying that it couldn't exec postgres to find out the value in the
config file? AFAICT it's going to try to register an eventsource with
whatever random garbage happens to be in the variable.

--
Magnus Hagander
Me: http://www.hagander.net/
Work: http://www.redpill-linpro.com/

From: "Magnus Hagander" <magnus(at)hagander(dot)net>
> Not having looked at it in detail yet, but this seems to completely remove
> the default value. What happens if the error that needs to be logged is
> the
> one saying that it couldn't exec postgres to find out the value in the
> config file? AFAICT it's going to try to register an eventsource with
> whatever random garbage happens to be in the variable.

Thank you for commenting, Magnus san.
The variable is global and contains an empty string, so even in the unlikely
situation where postgres -C fails, the event source simply becomes blank.

Regards
MauMau

On Mon, Dec 9, 2013 at 2:25 AM, MauMau <maumau307(at)gmail(dot)com> wrote:
> From: "Magnus Hagander" <magnus(at)hagander(dot)net>
>
>> Not having looked at it in detail yet, but this seems to completely remove
>> the default value. What happens if the error that needs to be logged is
>> the
>> one saying that it couldn't exec postgres to find out the value in the
>> config file? AFAICT it's going to try to register an eventsource with
>> whatever random garbage happens to be in the variable.
>
>
> Thank you for commenting, Magnus san.
> The variable is global and contains an empty string, so even in the unlikely
> situation where postgres -C fails, the event source simply becomes blank.

1. isn't it better to handle as it is done in write_eventlog() which
means if string is empty then
use PostgreSQL.
"evtHandle = RegisterEventSource(NULL, event_source ? event_source :
"PostgreSQL");"

2. What will happen if user doesn't change the name in "event_source"
or kept the same name,
won't it hit the same problem again? So shouldn't it try to
generate different name by appending
version string to it?

With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com

From: "Amit Kapila" <amit(dot)kapila16(at)gmail(dot)com>
> 1. isn't it better to handle as it is done in write_eventlog() which
> means if string is empty then
> use PostgreSQL.
> "evtHandle = RegisterEventSource(NULL, event_source ? event_source :
> "PostgreSQL");"

Thank you for reviewing. Yes, I did so with the first revision of this
patch (see the first mail of this thread.) I wanted to avoid duplicating
the default value in both the server and pg_ctl code. If user does not set
event_source, postgres -C returns the default value "PostgreSQL" in the
normal case, so I wanted to rely on it. I thought the second revision would
be appreciated by PostgreSQL-based products like EnterpriseDB, because there
are fewer source files to modify. But I don't mind which revision will be
adopted.

> 2. What will happen if user doesn't change the name in "event_source"
> or kept the same name,
> won't it hit the same problem again? So shouldn't it try to
> generate different name by appending
> version string to it?

Yes, but I assume that the user has to set his own name to identify his
instance uniquely. Even if version string is added, the same issue can
happen --- in the likely case where the user explicitly installs, for
example, PostgreSQL 9.3 as a standalone database, as well as some packaged
application that embeds PostgreSQL 9.3 which the user is unaware of.
If the user installs multiple versions of PostgreSQL explicitly with the
community installer, the installer can set event_source = 'PostgreSQL 9.3'
and 'PostgreSQL 9.4' for each instance.

Regards
MauMau

On Mon, Dec 9, 2013 at 5:52 PM, MauMau <maumau307(at)gmail(dot)com> wrote:
> From: "Amit Kapila" <amit(dot)kapila16(at)gmail(dot)com>
>
>> 1. isn't it better to handle as it is done in write_eventlog() which
>> means if string is empty then
>> use PostgreSQL.
>> "evtHandle = RegisterEventSource(NULL, event_source ? event_source :
>> "PostgreSQL");"
>
>
> Thank you for reviewing. Yes, I did so with the first revision of this
> patch (see the first mail of this thread.) I wanted to avoid duplicating
> the default value in both the server and pg_ctl code. If user does not set
> event_source, postgres -C returns the default value "PostgreSQL" in the
> normal case, so I wanted to rely on it.

I think it is better to keep it like what I suggested above,
because in that case
it will assign default name even if postgres -C fails due to some reason.

>
>
>> 2. What will happen if user doesn't change the name in "event_source"
>> or kept the same name,
>> won't it hit the same problem again? So shouldn't it try to
>> generate different name by appending
>> version string to it?
>
>
> Yes, but I assume that the user has to set his own name to identify his
> instance uniquely. Even if version string is added, the same issue can
> happen --- in the likely case where the user explicitly installs, for
> example, PostgreSQL 9.3 as a standalone database, as well as some packaged
> application that embeds PostgreSQL 9.3 which the user is unaware of.

I mentioned it just to make sure that if such things can happen, it
is good to
either avoid it or document it in some way, so that user can
understand better
how to configure his system.

With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com

>> From: "Amit Kapila" <amit(dot)kapila16(at)gmail(dot)com>
> I think it is better to keep it like what I suggested above,
> because in that case
> it will assign default name even if postgres -C fails due to some
> reason.
>
>>> 2. What will happen if user doesn't change the name in "event_source"
>>> or kept the same name,
>>> won't it hit the same problem again? So shouldn't it try to
>>> generate different name by appending
>>> version string to it?
>>

I re-considered that. As you suggested, I think I'll do as follows. Would
this be OK?

[pg_ctl.c]
evtHandle = RegisterEventSource(NULL, *event_source ? event_source :
"PostgreSQL " PG_MAJORVERSION);

[guc.c]
{"event_source", PGC_POSTMASTER, LOGGING_WHERE,
...
"PostgreSQL " PG_MAJORVERSION,
NULL, NULL, NULL

[elog.c]
Writing the default value in this file was redundant, because event_source
cannot be NULL. So change

evtHandle = RegisterEventSource(NULL, event_source ? event_source :
"PostgreSQL");

to

evtHandle = RegisterEventSource(NULL, event_source);

Regards
MauMau

On Wed, Dec 11, 2013 at 8:31 PM, MauMau <maumau307(at)gmail(dot)com> wrote:
>>> From: "Amit Kapila" <amit(dot)kapila16(at)gmail(dot)com>
>
> I re-considered that. As you suggested, I think I'll do as follows. Would
> this be OK?
>
> [pg_ctl.c]
> evtHandle = RegisterEventSource(NULL, *event_source ? event_source :
> "PostgreSQL " PG_MAJORVERSION);
>
>
> [guc.c]
> {"event_source", PGC_POSTMASTER, LOGGING_WHERE,
> ...
> "PostgreSQL " PG_MAJORVERSION,
> NULL, NULL, NULL

This is similar to what I had in mind.

>
> [elog.c]
> Writing the default value in this file was redundant, because event_source
> cannot be NULL. So change
>
>
> evtHandle = RegisterEventSource(NULL, event_source ? event_source :
> "PostgreSQL");
>
> to
>
> evtHandle = RegisterEventSource(NULL, event_source);

I think this change might not be safe as write_eventlog() gets called
from write_stderr() which might get called
before reading postgresql.conf, so the code as exists in PG might be
to handle that case or some other similar
situation where event_source is still not set. Have you confirmed in
all ways that it is never the case that
event_source is not set when the control reaches this function.

With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com

Hi, Amit san,

From: "Amit Kapila" <amit(dot)kapila16(at)gmail(dot)com>
>> [elog.c]
>> Writing the default value in this file was redundant, because
>> event_source
>> cannot be NULL. So change
>>
> I think this change might not be safe as write_eventlog() gets called
> from write_stderr() which might get called
> before reading postgresql.conf, so the code as exists in PG might be
> to handle that case or some other similar
> situation where event_source is still not set. Have you confirmed in
> all ways that it is never the case that
> event_source is not set when the control reaches this function.

Yes, you are right. I considered this again after replying to your previous
mail, and found out write_stderr() calls write_eventlog(). In that case,
event_source is still NULL before GUC initialization.

Please find attached the patch.
I put the default event source name in one place -- pg_config_manual.h,
which seems the best place. This could eliminate duplicate default values
in four places: pgevent.c, pg_ctl.c, elog.c, and guc.c. Thanks to your
review and comment, the code got cleaner and correct. Thank you very much.

Regards
MauMau

On Thu, Dec 12, 2013 at 4:43 PM, MauMau <maumau307(at)gmail(dot)com> wrote:
> Hi, Amit san,
>
> From: "Amit Kapila" <amit(dot)kapila16(at)gmail(dot)com>
>>>
>>> [elog.c]
>>> Writing the default value in this file was redundant, because
>>> event_source
>>> cannot be NULL. So change
>>>
>> I think this change might not be safe as write_eventlog() gets called
>> from write_stderr() which might get called
>> before reading postgresql.conf, so the code as exists in PG might be
>> to handle that case or some other similar
>> situation where event_source is still not set. Have you confirmed in
>> all ways that it is never the case that
>> event_source is not set when the control reaches this function.
>
>
> Yes, you are right. I considered this again after replying to your previous
> mail, and found out write_stderr() calls write_eventlog(). In that case,
> event_source is still NULL before GUC initialization.

Few minor things:
1.
evtHandle = RegisterEventSource(NULL,
*event_source? event_source: DEFAULT_EVENT_SOURCE);

In this code, you are trying to access the value (*event_source) and
incase it is not initialised,
it will not contain the value and could cause problem, why not
directly check 'event_source'?

2. minor coding style issue
pg_ctl.c
evtHandle = RegisterEventSource(NULL,
*event_source? event_source: DEFAULT_EVENT_SOURCE);

elog.c
! evtHandle = RegisterEventSource(NULL,
! event_source ? event_source : DEFAULT_EVENT_SOURCE);

In both above usages, it is better that arguments in second line should start
inline with previous lines first argument. You can refer other places,
for ex. refer call to ReportEvent in pg_ctl.c just below
RegisterEventSource call.

With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com

From: "Amit Kapila" <amit(dot)kapila16(at)gmail(dot)com>
> Few minor things:
> 1.
> evtHandle = RegisterEventSource(NULL,
> *event_source? event_source: DEFAULT_EVENT_SOURCE);
>
> In this code, you are trying to access the value (*event_source) and
> incase it is not initialised,
> it will not contain the value and could cause problem, why not
> directly check 'event_source'?

event_source here is a global static char array, so it's automatically
initialized with zeros and safe to access.

> 2. minor coding style issue
> pg_ctl.c
> evtHandle = RegisterEventSource(NULL,
> *event_source? event_source: DEFAULT_EVENT_SOURCE);
>
> elog.c
> ! evtHandle = RegisterEventSource(NULL,
> ! event_source ? event_source : DEFAULT_EVENT_SOURCE);
>
> In both above usages, it is better that arguments in second line should
> start
> inline with previous lines first argument. You can refer other places,
> for ex. refer call to ReportEvent in pg_ctl.c just below
> RegisterEventSource call.

Thanks. I passed the source files through pgindent and attached the revised
patch. Although the arguments in the second line are not in line with the
first line's arguments, that's what pgindent found good.

Regards
MauMau

On Tue, Dec 17, 2013 at 5:33 PM, MauMau <maumau307(at)gmail(dot)com> wrote:
> From: "Amit Kapila" <amit(dot)kapila16(at)gmail(dot)com>
>
>> Few minor things:
>
> event_source here is a global static char array, so it's automatically
> initialized with zeros and safe to access.

Right, I had missed that point.
>
>
>> 2. minor coding style issue
>
>
> Thanks. I passed the source files through pgindent and attached the revised
> patch. Although the arguments in the second line are not in line with the
> first line's arguments, that's what pgindent found good.

Okay, no problem.

Few other points:
-------------------------
1.
#ifdef WIN32
/* Get event source from postgresql.conf for eventlog output */
get_config_value("event_source", event_source, sizeof(event_source));
#endif

event logging is done for both win32 and cygwin env.
under hash define (Win32 || cygwin),
so event source name should also be retrieved for both
environments. Refer below in code:

#if defined(WIN32) || defined(__CYGWIN__)
static void
write_eventlog(int level, const char *line)

2.
Docs needs to be updated for default value:
http://www.postgresql.org/docs/devel/static/event-log-registration.html
http://www.postgresql.org/docs/devel/static/runtime-config-logging.html#GUC-EVENT-SOURCE

In this patch, we are planing to change default value of event_source
from PostgreSQL to PostgreSQL 9.4 (PostgreSQL PG_MAJORVERSION)
as part of fixing the issue reported in this thread.

If anyone has objection to that, please let us know now to avoid re-work
at later stage.

With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com

From: "Amit Kapila" <amit(dot)kapila16(at)gmail(dot)com>
> Few other points:
> -------------------------
> 1.
> #ifdef WIN32
> /* Get event source from postgresql.conf for eventlog output */
> get_config_value("event_source", event_source, sizeof(event_source));
> #endif
>
> event logging is done for both win32 and cygwin env.
> under hash define (Win32 || cygwin),
> so event source name should also be retrieved for both
> environments. Refer below in code:
>
> #if defined(WIN32) || defined(__CYGWIN__)
> static void
> write_eventlog(int level, const char *line)
>
> 2.
> Docs needs to be updated for default value:
> http://www.postgresql.org/docs/devel/static/event-log-registration.html
> http://www.postgresql.org/docs/devel/static/runtime-config-logging.html#GUC-EVENT-SOURCE

Okay, done. Thanks. I'll update the commitfest entry this weekend.

Regards
MauMau

On Fri, Dec 20, 2013 at 5:26 PM, MauMau <maumau307(at)gmail(dot)com> wrote:
> From: "Amit Kapila" <amit(dot)kapila16(at)gmail(dot)com>
>>
>> Few other points:
>>
>> -------------------------
>> 1.
>> #ifdef WIN32
>> /* Get event source from postgresql.conf for eventlog output */
>> get_config_value("event_source", event_source, sizeof(event_source));
>> #endif
>>
>> event logging is done for both win32 and cygwin env.
>> under hash define (Win32 || cygwin),
>> so event source name should also be retrieved for both
>> environments. Refer below in code:
>>
>> #if defined(WIN32) || defined(__CYGWIN__)
>> static void
>> write_eventlog(int level, const char *line)
>>
>> 2.
>> Docs needs to be updated for default value:
>> http://www.postgresql.org/docs/devel/static/event-log-registration.html
>>
>> http://www.postgresql.org/docs/devel/static/runtime-config-logging.html#GUC-EVENT-SOURCE
>
>
> Okay, done. Thanks. I'll update the commitfest entry this weekend.

Your changes are fine. The main part left from myside is test of this patch.
I will do that in next CF or If I get time before that, I will try to
complete it.

With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com

On Thu, Dec 5, 2013 at 7:54 PM, MauMau <maumau307(at)gmail(dot)com> wrote:
> Hello,
>
> I've removed a limitation regarding event log on Windows with the attached
> patch. I hesitate to admit this is a bug fix and want to regard this an
> improvement, but maybe it's a bug fix from users' perspective. Actually, I
> received problem reports from some users.
>
>
> [Problem]
> pg_ctl always uses event source "PostgreSQL" to output messages to the event
> log. Some example messages are:
>
> server starting
> server started
>
> This causes the problems below:
>
> 1. Cannot distinguish PostgreSQL instances because they use the same event
> source.
>
> 2. If you use multiple installations of PostgreSQL on the same machine,
> whether they are the same or different versions, Windows event viewer
> reports an error "event source not found" in the following sequence. Other
> system administration software which deal with event log may show other
> anomalies.
> 2-1 Install the first PostgreSQL (e.g. 9.3) into <dir_1>, register
> <dir_1>\lib\pgevent.dll for event source "PostgreSQL", then creates
> <instance_1>.
> 2-2 Install the second PostgreSQL (e.g. 9.2) into <dir_2> as part of some
> packaged application, register <dir_2>\lib\pgevent.dll for event source
> "PostgreSQL",

Today, I was trying to reproduce this issue and found that if user tries
to register event source second time with same name, we just replace
the previous event source's path in registry.
Shouldn't we try to stop user at this step only, means if he tries to
register with same event source name more than once return error,
saying event source with same name already exists?

Another thing is after register of pgevent.dll, if I just try to start
PostgreSQL
it shows below messages in EventLog. Although the message has information
about server startup, but the start of Description is something similar to
what you were reporting "event source not found".
Am I missing something?

Steps
1. installation of PostgreSQL from source code using Install.bat in
mdvc directory
2. initdb -D <data_dir>
3. regsvr32 /n /i:PostgreSQL <install_dir_path>\lib\pgevent32.dll
4. Modify postgresql.conf to set log_destination= 'eventlog'
5. pg_ctl.exe start -D ..\..\Data

Log Name: Application
Source: PostgreSQL
Date: 1/19/2014 7:49:54 PM
Event ID: 0
Task Category: None
Level: Information
Keywords: Classic
User: N/A
Computer: WIN-NGNN8PKIMD7
Description:
The description for Event ID 0 from source PostgreSQL cannot be found.
Either the component that raises this event is not installed on your
local computer or the installation is corrupted. You can install or
repair the component on the local computer.

If the event originated on another computer, the display information
had to be saved with the event.

The following information was included with the event:

LOG: ending log output to stderr
HINT: Future log output will go to log destination "eventlog".

the message resource is present but the message is not found in the
string/message table

With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com

From: "Amit Kapila" <amit(dot)kapila16(at)gmail(dot)com>
> Today, I was trying to reproduce this issue and found that if user tries
> to register event source second time with same name, we just replace
> the previous event source's path in registry.
> Shouldn't we try to stop user at this step only, means if he tries to
> register with same event source name more than once return error,
> saying event source with same name already exists?

I'm OK with either. If we add the check, I think that would be another
patch. However, I'm afraid the check won't be much effective, because the
packaged application then unregister and register (i.e. regsvr32 /u and then
regsvr32 /i) blindly.

> Another thing is after register of pgevent.dll, if I just try to start
> PostgreSQL
> it shows below messages in EventLog. Although the message has information
> about server startup, but the start of Description is something similar to
> what you were reporting "event source not found".
> Am I missing something?
>
> Steps
> 1. installation of PostgreSQL from source code using Install.bat in
> mdvc directory
> 2. initdb -D <data_dir>
> 3. regsvr32 /n /i:PostgreSQL <install_dir_path>\lib\pgevent32.dll

Please specify pgevent.dll, not pgevent32.dll.

Regards
MauMau

On Mon, Jan 20, 2014 at 4:05 AM, MauMau <maumau307(at)gmail(dot)com> wrote:
> From: "Amit Kapila" <amit(dot)kapila16(at)gmail(dot)com>
>
>> Today, I was trying to reproduce this issue and found that if user tries
>> to register event source second time with same name, we just replace
>> the previous event source's path in registry.
>> Shouldn't we try to stop user at this step only, means if he tries to
>> register with same event source name more than once return error,
>> saying event source with same name already exists?
>
>
> I'm OK with either. If we add the check, I think that would be another
> patch.

Do you think without this the problem reported by you is resolved completely.
User can hit same problem, if he tries to follow similar steps mentioned in
your first mail. I had tried below steps based on description in your
first mail:

Steps
1. installation of PostgreSQL from source code (master) using Install.bat in
msvc directory
2. initdb -D <data_dir>
3. regsvr32 /n /i:PostgreSQL <install_dir_path>\lib\pgevent.dll
4. Modify postgresql.conf to set log_destination= 'eventlog'
5. event_source = 'PostgreSQL'
6. pg_ctl.exe start -D ..\..\Data
7. psql -d postgres
8. Drop table t1; --try dropping some non-existant table
9. Check in Event viewer, you can find proper error.
10. NO Problem till above step.
11. Installation of PostgreSQL from source code (9.2) using Install.bat in
msvc directory
12. initdb -D <9.2_data_dir>
13. regsvr32 /n /i:PostgreSQL <install_9.2_dir_path>\lib\pgevent.dll
14. Remove 9.2 installation (i have just renamed the install folder)
15. now perform step 8 again on master (with or without restart of server)
16. Open Event Viewer, you can see the message
"event source not found"

Now this could have been avoided, if in step-13 we use different
event source name, but I think that will happen even without
this patch.

> However, I'm afraid the check won't be much effective, because the
> packaged application then unregister and register (i.e. regsvr32 /u and then
> regsvr32 /i) blindly.

If user register/unregister different versions of pgevent.dll blindly,
then I think
any fix in this area could not prevent the error "event source not found"

>
>> Another thing is after register of pgevent.dll, if I just try to start
>> PostgreSQL
>> it shows below messages in EventLog. Although the message has information
>> about server startup, but the start of Description is something similar to
>> what you were reporting "event source not found".
>> Am I missing something?
>>
>> Steps
>> 1. installation of PostgreSQL from source code using Install.bat in
>> mdvc directory
>> 2. initdb -D <data_dir>
>> 3. regsvr32 /n /i:PostgreSQL <install_dir_path>\lib\pgevent32.dll
>
>
> Please specify pgevent.dll, not pgevent32.dll.

Typo, I was using prevent.dll only, I think the reason is I need to restart
Event Viewer after register command.

With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com

From: "Amit Kapila" <amit(dot)kapila16(at)gmail(dot)com>
> Do you think without this the problem reported by you is resolved
> completely.
> User can hit same problem, if he tries to follow similar steps mentioned
> in
> your first mail. I had tried below steps based on description in your
> first mail:
>
> If user register/unregister different versions of pgevent.dll blindly,
> then I think
> any fix in this area could not prevent the error "event source not found"

OK, I'll add a check to prevent duplicate registration of the same event
source with the message:

"The specified event source is already registered."

Please correct me if there's better expression in English.

Are there any other suggestions to make this patch ready for committer? I'd
like to make all changes in one submission.

Regards
MauMau

On Mon, Jan 20, 2014 at 5:38 PM, MauMau <maumau307(at)gmail(dot)com> wrote:
> From: "Amit Kapila" <amit(dot)kapila16(at)gmail(dot)com>
>>
>> Do you think without this the problem reported by you is resolved
>> completely.
>> User can hit same problem, if he tries to follow similar steps mentioned
>> in
>> your first mail. I had tried below steps based on description in your
>> first mail:
>>
>> If user register/unregister different versions of pgevent.dll blindly,
>> then I think
>> any fix in this area could not prevent the error "event source not found"
>
>
> OK, I'll add a check to prevent duplicate registration of the same event
> source with the message:
>
> "The specified event source is already registered."
>
> Please correct me if there's better expression in English.

How about below message:

event source "<event_source_name>" is already registered.

This can make it more consistent with any other message in PG,
example create table.

> Are there any other suggestions to make this patch ready for committer?

Today, I reviewed the patch again and found it okay, except a small
inconsistency which is about default event source name in
postgresql.conf, all other places it's changed except in .conf file.
Do you think it makes sense to change there as well?

With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com

From: "Amit Kapila" <amit(dot)kapila16(at)gmail(dot)com>
> How about below message:
>
> event source "<event_source_name>" is already registered.

Thanks. I'll use this, making the initial letter the capital "E" like other
messages in the same source file. I'm going to submit the final patch and
update the CommitFest entry tomorrow at the earliest.

> Today, I reviewed the patch again and found it okay, except a small
> inconsistency which is about default event source name in
> postgresql.conf, all other places it's changed except in .conf file.
> Do you think it makes sense to change there as well?

Oh, I missed it. postgresql.conf.sample says:

# The commented-out settings shown in this file represent the default
values.

To follow this, we have the line as:

#event_source = 'PostgreSQL 9.4'

But this requires us to change this line for each major release. That's a
maintenance headache. Another idea is:

#event_source = 'PostgreSQL <major_release>'

But this is not the exact default value.

So I want to leave the line as now. Anyway, some other lines in
postgresql.conf.sample do not represent the default value, either,:

#data_directory = 'ConfigDir' # use data in another directory
#max_stack_depth = 2MB # min 100kB
#include_if_exists = 'exists.conf' # include file only if it exists
#include = 'special.conf' # include file

Regards
MauMau

On Tue, Jan 21, 2014 at 6:57 PM, MauMau <maumau307(at)gmail(dot)com> wrote:
> From: "Amit Kapila" <amit(dot)kapila16(at)gmail(dot)com>
>> Today, I reviewed the patch again and found it okay, except a small
>> inconsistency which is about default event source name in
>> postgresql.conf, all other places it's changed except in .conf file.
>> Do you think it makes sense to change there as well?
>
>
> Oh, I missed it. postgresql.conf.sample says:
>
> # The commented-out settings shown in this file represent the default
> values.
>
> To follow this, we have the line as:
>
> #event_source = 'PostgreSQL 9.4'
>
> But this requires us to change this line for each major release. That's a
> maintenance headache.

What I had in mind was to change it during initdb, we are already doing it
for some other parameter (unix_socket_directories), please refer below
code in initdb.c

#ifdef HAVE_UNIX_SOCKETS
snprintf(repltok, sizeof(repltok), "#unix_socket_directories = '%s'",
DEFAULT_PGSOCKET_DIR);
#else
snprintf(repltok, sizeof(repltok), "#unix_socket_directories = ''");
#endif
conflines = replace_token(conflines, "#unix_socket_directories = '/tmp'",
repltok);

Could you once check if it is possible in above way to change
event_source?

With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com

Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> writes:
> On Tue, Jan 21, 2014 at 6:57 PM, MauMau <maumau307(at)gmail(dot)com> wrote:
>> To follow this, we have the line as:
>>
>> #event_source = 'PostgreSQL 9.4'
>>
>> But this requires us to change this line for each major release. That's a
>> maintenance headache.

> What I had in mind was to change it during initdb, we are already doing it
> for some other parameter (unix_socket_directories),

What happens when somebody copies their postgresql.conf from an older
version? That's hardly uncommon, even though it might be considered bad
practice. I don't think it's a good idea to try to insert a version
identifier this way.

But ... what's the point of including the PG version in this string
anyhow? If you're trying to make the string unique across different
installations on the same machine, it's surely insufficient, and if
that's not the point then what is?

regards, tom lane

On Wed, Jan 22, 2014 at 9:19 AM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> writes:
>> On Tue, Jan 21, 2014 at 6:57 PM, MauMau <maumau307(at)gmail(dot)com> wrote:
>>> To follow this, we have the line as:
>>>
>>> #event_source = 'PostgreSQL 9.4'
>>>
>>> But this requires us to change this line for each major release. That's a
>>> maintenance headache.
>
>> What I had in mind was to change it during initdb, we are already doing it
>> for some other parameter (unix_socket_directories),
>
> What happens when somebody copies their postgresql.conf from an older
> version? That's hardly uncommon, even though it might be considered bad
> practice. I don't think it's a good idea to try to insert a version
> identifier this way.
>
> But ... what's the point of including the PG version in this string
> anyhow? If you're trying to make the string unique across different
> installations on the same machine, it's surely insufficient, and if
> that's not the point then what is?

Well, certainly it cannot handle all different scenario's (particularly
same version installations), but the original report for this case was
for different versions of server. I think chances of having different
versions of server are much more, which will be handled by this
case. I felt even service name should include and we already have
Fixme in code for it, but thats separate patch altogether.
pg_ctl.c
..
(static char *register_servicename = "PostgreSQL";
/* FIXME: + version ID? */

Also, I referred other s/w's which are registered for event source on
my m/c and I found it is common to include version number in some
form to distinguish different versions. For example, some of the
registered ones are:

Microsoft.Transactions.Bridge 3.0.0.0
Microsoft.Transactions.Bridge 4.0.0.0

ServiceModel Audit 3.0.0.0
ServiceModel Audit 4.0.0.0

I have also seen such a way to append versions to service names as
well.

With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com

On Tue, Jan 21, 2014 at 11:20 PM, Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
> On Wed, Jan 22, 2014 at 9:19 AM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>> Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> writes:
>>> On Tue, Jan 21, 2014 at 6:57 PM, MauMau <maumau307(at)gmail(dot)com> wrote:
>>>> To follow this, we have the line as:
>>>>
>>>> #event_source = 'PostgreSQL 9.4'
>>>>
>>>> But this requires us to change this line for each major release. That's a
>>>> maintenance headache.
>>
>>> What I had in mind was to change it during initdb, we are already doing it
>>> for some other parameter (unix_socket_directories),
>>
>> What happens when somebody copies their postgresql.conf from an older
>> version? That's hardly uncommon, even though it might be considered bad
>> practice. I don't think it's a good idea to try to insert a version
>> identifier this way.
>>
>> But ... what's the point of including the PG version in this string
>> anyhow? If you're trying to make the string unique across different
>> installations on the same machine, it's surely insufficient, and if
>> that's not the point then what is?
>
> Well, certainly it cannot handle all different scenario's (particularly
> same version installations), but the original report for this case was
> for different versions of server. I think chances of having different
> versions of server are much more, which will be handled by this
> case.

I wonder if the port number wouldn't be a better choice. And that
could even be done without adding a parameter.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On Wed, Jan 22, 2014 at 6:54 PM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
> On Tue, Jan 21, 2014 at 11:20 PM, Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
>> On Wed, Jan 22, 2014 at 9:19 AM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>>> Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> writes:
>>>> On Tue, Jan 21, 2014 at 6:57 PM, MauMau <maumau307(at)gmail(dot)com> wrote:
>>>>> To follow this, we have the line as:
>>>>>
>>>>> #event_source = 'PostgreSQL 9.4'
>>>>>
>>>>> But this requires us to change this line for each major release. That's a
>>>>> maintenance headache.
>>>
>>>> What I had in mind was to change it during initdb, we are already doing it
>>>> for some other parameter (unix_socket_directories),
>>>
>>> What happens when somebody copies their postgresql.conf from an older
>>> version? That's hardly uncommon, even though it might be considered bad
>>> practice. I don't think it's a good idea to try to insert a version
>>> identifier this way.
>>>
>>> But ... what's the point of including the PG version in this string
>>> anyhow? If you're trying to make the string unique across different
>>> installations on the same machine, it's surely insufficient, and if
>>> that's not the point then what is?
>>
>> Well, certainly it cannot handle all different scenario's (particularly
>> same version installations), but the original report for this case was
>> for different versions of server. I think chances of having different
>> versions of server are much more, which will be handled by this
>> case.
>
> I wonder if the port number wouldn't be a better choice. And that
> could even be done without adding a parameter.

We need this for register of event source which might be done before
start of server.

With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com

Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> writes:
> On Wed, Jan 22, 2014 at 6:54 PM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
>> I wonder if the port number wouldn't be a better choice. And that
>> could even be done without adding a parameter.

> We need this for register of event source which might be done before
> start of server.

So? Anything which can know the value of a GUC parameter can certainly
know the selected port number.

regards, tom lane

On Wed, Jan 22, 2014 at 9:03 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> writes:
>> On Wed, Jan 22, 2014 at 6:54 PM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
>>> I wonder if the port number wouldn't be a better choice. And that
>>> could even be done without adding a parameter.
>
>> We need this for register of event source which might be done before
>> start of server.
>
> So? Anything which can know the value of a GUC parameter can certainly
> know the selected port number.

1. In case of registration of event source, either user has to pass the name
or it uses hard coded default value, so if we use version number along with
'PostgreSQL', it can be consistent.
I don't see any way pgevent.c can know port number to append it to default
value, am I missing something here?
2. In pg_ctl if we use port number, then if user changes it across multiple
server restarts, then it can also create a problem, as the event source
name used will be different than what the name used during registration
of event source.

With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com

Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> writes:
> On Wed, Jan 22, 2014 at 9:03 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>> So? Anything which can know the value of a GUC parameter can certainly
>> know the selected port number.

> 1. In case of registration of event source, either user has to pass the name
> or it uses hard coded default value, so if we use version number along with
> 'PostgreSQL', it can be consistent.
> I don't see any way pgevent.c can know port number to append it to default
> value, am I missing something here?

[ shrug... ] But the same problem applies just as much to any attempt by
pg_ctl to know *any* postmaster parameter. In particular, this entire
patch is bogus, because the value it extracts from the postgresql.conf
file does not necessarily have anything to do with the setting the live
postmaster is actually using (which might be determined by a command-line
parameter or environment variable instead). If the technique could be
relied on, then it could be relied on just as much to extract the
postmaster's port setting.

I don't necessarily object to teaching pg_ctl to make a best-effort
estimate of a postmaster parameter in this way. But it's complete folly
to suppose that you can get an accurate value of event_source but not
the port number.

I think what we might want to do is redefine the server's behavior
as creating an event named after the concatenation of event_source
and port number, or maybe even get rid of event_source entirely and
just say it's "PostgreSQL" followed by the port number. If we did
the latter then the problem would reduce to whether pg_ctl knows
the port number, which I think we're already assuming for other
reasons.

regards, tom lane

On Thu, Jan 23, 2014 at 10:26 AM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> writes:
>> On Wed, Jan 22, 2014 at 9:03 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>>> So? Anything which can know the value of a GUC parameter can certainly
>>> know the selected port number.
>
>> 1. In case of registration of event source, either user has to pass the name
>> or it uses hard coded default value, so if we use version number along with
>> 'PostgreSQL', it can be consistent.
>> I don't see any way pgevent.c can know port number to append it to default
>> value, am I missing something here?
>
>
> I think what we might want to do is redefine the server's behavior
> as creating an event named after the concatenation of event_source
> and port number, or maybe even get rid of event_source entirely and
> just say it's "PostgreSQL" followed by the port number.

To accomplish this behaviour, each time server starts and stops,
we need to register and unregister event log using mechanism
described at below link to ensure that there is no mismatch between
what server uses and what OS knows.
http://www.postgresql.org/docs/devel/static/event-log-registration.html

IIUC, this will be a much larger behaviour change.
What is the problem you are envisioning if we use version number,
yesterday I have given some examples about other softwares which
are registered in event log and uses version number, so I don't
understand why it is big deal if we also use version number along with
PostgreSQL as a default value and if user specifies any particular name
then use the same.

With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com

On Thu, Jan 23, 2014 at 9:23 AM, Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
> On Thu, Jan 23, 2014 at 10:26 AM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>> Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> writes:
>>> On Wed, Jan 22, 2014 at 9:03 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>>>> So? Anything which can know the value of a GUC parameter can certainly
>>>> know the selected port number.
>>
>>> 1. In case of registration of event source, either user has to pass the name
>>> or it uses hard coded default value, so if we use version number along with
>>> 'PostgreSQL', it can be consistent.
>>> I don't see any way pgevent.c can know port number to append it to default
>>> value, am I missing something here?
>>
>>
>> I think what we might want to do is redefine the server's behavior
>> as creating an event named after the concatenation of event_source
>> and port number, or maybe even get rid of event_source entirely and
>> just say it's "PostgreSQL" followed by the port number.
>
> To accomplish this behaviour, each time server starts and stops,
> we need to register and unregister event log using mechanism
> described at below link to ensure that there is no mismatch between
> what server uses and what OS knows.
> http://www.postgresql.org/docs/devel/static/event-log-registration.html

Why wouldn't that be necessary with your approach, too? I mean, if
there's a GUC that controls the event source name, then it can be
changed between restarts, regardless of what you call it.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

From: "Amit Kapila" <amit(dot)kapila16(at)gmail(dot)com>
> How about below message:
>
> event source "<event_source_name>" is already registered.

OK, I added several lines for this. Please check the attached patch.

> What I had in mind was to change it during initdb, we are already doing it
> for some other parameter (unix_socket_directories), please refer below
> code in initdb.c

Yes, It seems we can do this. However, could you forgive me for leaving
this untouched? I'm afraid postgresql.conf.sample's issue is causing
unnecessary war among people here. That doesn't affect the point of this
patch --- make pg_ctl use the event_source setting. Anyway, not all
parameters in postgresql.conf cannot be used just by uncommenting them.
That's another issue.

I'll update the CommitFest entry soon.

Regards
MauMau

On 1/23/14, 4:08 PM, Robert Haas wrote:
> Why wouldn't that be necessary with your approach, too? I mean, if
> there's a GUC that controls the event source name, then it can be
> changed between restarts, regardless of what you call it.

I don't know if it's practical, but the logical conclusion here would be
to use an identifier that you cannot change, such as the system identifier.

Peter Eisentraut <peter_e(at)gmx(dot)net> writes:
> On 1/23/14, 4:08 PM, Robert Haas wrote:
>> Why wouldn't that be necessary with your approach, too? I mean, if
>> there's a GUC that controls the event source name, then it can be
>> changed between restarts, regardless of what you call it.

> I don't know if it's practical, but the logical conclusion here would be
> to use an identifier that you cannot change, such as the system identifier.

That particular ID would be a horrid choice, because we don't try very
hard to ensure it's unique. In particular, a standby server on the same
machine as the master (not an uncommon case, at least for testing
purposes) would be a guaranteed fail with that approach.

I'm still not clear on why we can't just use the port number.

regards, tom lane

Tom Lane escribió:
> Peter Eisentraut <peter_e(at)gmx(dot)net> writes:
> > On 1/23/14, 4:08 PM, Robert Haas wrote:
> >> Why wouldn't that be necessary with your approach, too? I mean, if
> >> there's a GUC that controls the event source name, then it can be
> >> changed between restarts, regardless of what you call it.
>
> > I don't know if it's practical, but the logical conclusion here would be
> > to use an identifier that you cannot change, such as the system identifier.
>
> That particular ID would be a horrid choice, because we don't try very
> hard to ensure it's unique. In particular, a standby server on the same
> machine as the master (not an uncommon case, at least for testing
> purposes) would be a guaranteed fail with that approach.
>
> I'm still not clear on why we can't just use the port number.

I wonder if it would make sense to generate a unique name at some
initial point in the history of the service (perhaps at initdb time, or
at the first postmaster start) and store this name in a special,
separate file in PGDATA. On subsequent starts we read the name from
there and always use it consistently.

--
Álvaro Herrera http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

Alvaro Herrera <alvherre(at)2ndquadrant(dot)com> writes:
> Tom Lane escribi:
>> That particular ID would be a horrid choice, because we don't try very
>> hard to ensure it's unique. In particular, a standby server on the same
>> machine as the master (not an uncommon case, at least for testing
>> purposes) would be a guaranteed fail with that approach.

> I wonder if it would make sense to generate a unique name at some
> initial point in the history of the service (perhaps at initdb time, or
> at the first postmaster start) and store this name in a special,
> separate file in PGDATA. On subsequent starts we read the name from
> there and always use it consistently.

Meh --- that would have the same behavior as the system identifier,
ie it would propagate to slave servers without extraordinary (and
easily bypassed) measures to prevent it.

regards, tom lane

From: "Tom Lane" <tgl(at)sss(dot)pgh(dot)pa(dot)us>
> I'm still not clear on why we can't just use the port number.

It will be possible to use port to set the default value of event_source GUC
when starting postmaster. But using port during event source registration
will involve much more.
To use port, we have to tell the location of $PGDATA to regsvr32.exe.
However, regsvr32.exe can only take an argument from /i, and we are using /i
for event source name specification. If we want to pass data directory, we
have to change the usage. Instead, we could probably have regsvr32.exe
check PGDATA env variable and invoke "postgres -C event_source", but that
would require much more complicated code (e.g. for locating postgres.exe,
because regsvr32.exe is in Windows directory)

Anyway, the point of my patch is to just make pg_ctl use event_source GUC
for outputing to event log. I want to rely on postgres -C, because pg_ctl
already uses it for retrieving data_directory GUC. I'd like to avoid
further complication in code and discussion. If you request, I can revert
the default value of event_source and regsvr32.exe /i to "PostgreSQL". I'm
okay with that, because syslog_ident also has the default value "postgres",
which doesn't contain the major release.

Any (not complicated) suggestions?

Regards
MauMau

"MauMau" <maumau307(at)gmail(dot)com> writes:
> From: "Tom Lane" <tgl(at)sss(dot)pgh(dot)pa(dot)us>
>> I'm still not clear on why we can't just use the port number.

> To use port, we have to tell the location of $PGDATA to regsvr32.exe.

[ scratches head... ] Exactly which of the other proposals *doesn't*
require that? Certainly anything that involves parsing settings out
of postgresql.conf will.

A more significant problem, probably, is that even knowing $PGDATA doesn't
tell you the port number with certainty, since the postmaster might end
up taking the port number from some other source than postgresql.conf
(command line, PGPORT environment, ...). We used to require that pg_ctl
know the port accurately, and it was a big step forward in reliability
when we got rid of that; so maybe going back to that is not such a good
idea.

I note though that pg_ctl does still need to know accurately where $PGDATA
is. Is there any particular length limit on event source names? Maybe
the full path to $PGDATA could be used instead of the port number.

regards, tom lane

On Fri, Jan 24, 2014 at 2:38 AM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
> On Thu, Jan 23, 2014 at 9:23 AM, Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
>> On Thu, Jan 23, 2014 at 10:26 AM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>>> Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> writes:
>>>> On Wed, Jan 22, 2014 at 9:03 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>>>>> So? Anything which can know the value of a GUC parameter can certainly
>>>>> know the selected port number.
>>>
>>>> 1. In case of registration of event source, either user has to pass the name
>>>> or it uses hard coded default value, so if we use version number along with
>>>> 'PostgreSQL', it can be consistent.
>>>> I don't see any way pgevent.c can know port number to append it to default
>>>> value, am I missing something here?
>>>
>>>
>>> I think what we might want to do is redefine the server's behavior
>>> as creating an event named after the concatenation of event_source
>>> and port number, or maybe even get rid of event_source entirely and
>>> just say it's "PostgreSQL" followed by the port number.
>>
>> To accomplish this behaviour, each time server starts and stops,
>> we need to register and unregister event log using mechanism
>> described at below link to ensure that there is no mismatch between
>> what server uses and what OS knows.
>> http://www.postgresql.org/docs/devel/static/event-log-registration.html
>
> Why wouldn't that be necessary with your approach, too?

Because in my approach we are using compile time constant
+ #define DEFAULT_EVENT_SOURCE
"PostgreSQL " PG_MAJORVERSION

> I mean, if
> there's a GUC that controls the event source name, then it can be
> changed between restarts, regardless of what you call it.

Yes, but not default values (when user don't provide any value
for event_soource). Here the question is about default value of
event_source.

I will try to explain by example, focus of example is for case when
user doesn't provide any value for event_source. The steps he needs
to follow are as below:

1. initdb -D <data_dir>
2. regsvr32 <install_dir_path>\lib\pgevent32.dll
3. Modify postgresql.conf to set log_destination= 'eventlog'
4. pg_ctl.exe start -D <data_dir>

Now for above, currently we use 'PostgreSQL' as default name
for event source both during registration (step-2) and server start
by pg_ctl (step-4). This will work fine, user will be able to see proper
messages in event log (Windows EventViewer), however if user uses
different versions on same m/c and follows above steps for both
versions, then there is a chance (incase user unistall one of version),
that EventViewer will not display PostgreSQL messages properly,
it will show something like "event source not found".

To resolve this case, we thought of appending version number to
'PostgreSQL' as a default name of event source. It might not work
in all cases (for ex. 2 instances of same postgres version), but
will work in many cases where currently it doesn't work.

Now the problem for using port number is in step-2 of above case,
currently pgevent.c has no way of knowing that port number value,
let us say we teach in some way like MauMau said by passing
parameter using /i option of regsvr32, but it might become confusing
for user to use that option, because currently it is used for passing
event source name (non-default case).

If appending some compile time constant (if you have anything other
than version number which is compile time const in your mind,
that could also work equally easy) to default name doesn't
sound to be viable fix for above problem, I think it is better to take
that out of patch and may be it can be taken up as a separate patch.

With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com

On Fri, Jan 24, 2014 at 4:22 AM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> "MauMau" <maumau307(at)gmail(dot)com> writes:
>> From: "Tom Lane" <tgl(at)sss(dot)pgh(dot)pa(dot)us>
>>> I'm still not clear on why we can't just use the port number.
>
>> To use port, we have to tell the location of $PGDATA to regsvr32.exe.
>
> [ scratches head... ] Exactly which of the other proposals *doesn't*
> require that?

Appending it with version number which is compile time constant.

> Certainly anything that involves parsing settings out
> of postgresql.conf will.

We don't need to parse for default value of event source, it is only
for case when user gives some specific name to event_source in
postgresql.conf and it that case, we expect that user provides the
same name during register command of event source, some thing
like:
regsvr32 /n /i: PostgreSQL_HEAD <install_dir_path>\lib\pgevent32.dll

Here point to note is that pgevent.dll never does any parsing or lookup
for event source name, either user provides it or we use default value.

With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com

From: "MauMau" <maumau307(at)gmail(dot)com>
> OK, I added several lines for this. Please check the attached patch.

I'm sorry, I attached the old patch as v5 in my previous mail. Attached on
this mail is the correct one.

I'll update the CommitFest entry soon.

Regards
MauMau

On Fri, Jan 24, 2014 at 9:10 AM, Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
> On Fri, Jan 24, 2014 at 2:38 AM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
>> On Thu, Jan 23, 2014 at 9:23 AM, Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
>>> On Thu, Jan 23, 2014 at 10:26 AM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>>>>
>>>> I think what we might want to do is redefine the server's behavior
>>>> as creating an event named after the concatenation of event_source
>>>> and port number, or maybe even get rid of event_source entirely and
>>>> just say it's "PostgreSQL" followed by the port number.
>>>
>>> To accomplish this behaviour, each time server starts and stops,
>>> we need to register and unregister event log using mechanism
>>> described at below link to ensure that there is no mismatch between
>>> what server uses and what OS knows.
>>> http://www.postgresql.org/docs/devel/static/event-log-registration.html
>>
>> Why wouldn't that be necessary with your approach, too?
>
> Because in my approach we are using compile time constant
> + #define DEFAULT_EVENT_SOURCE
> "PostgreSQL " PG_MAJORVERSION
>
>> I mean, if
>> there's a GUC that controls the event source name, then it can be
>> changed between restarts, regardless of what you call it.
>
> Yes, but not default values (when user don't provide any value
> for event_soource). Here the question is about default value of
> event_source.

To proceed with the review of this patch, I need to know about
whether appending version number or any other constant to
Default Event Source name is acceptable or not, else for now
we can remove this part of code from patch and handle non-default
case where the change will be that pg_ctl will enquire non-default
event_source value from server.

Could you please let me know your views about same?

With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com

Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> writes:
> On Fri, Jan 24, 2014 at 9:10 AM, Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
>> On Fri, Jan 24, 2014 at 2:38 AM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
>>> I mean, if
>>> there's a GUC that controls the event source name, then it can be
>>> changed between restarts, regardless of what you call it.

>> Yes, but not default values (when user don't provide any value
>> for event_soource). Here the question is about default value of
>> event_source.

> To proceed with the review of this patch, I need to know about
> whether appending version number or any other constant togli

> Default Event Source name is acceptable or not, else for now
> we can remove this part of code from patch and handle non-default
> case where the change will be that pg_ctl will enquire non-default
> event_source value from server.

> Could you please let me know your views about same?

Unless I'm missing something, this entire thread is a tempest in a teapot,
because the default event_source value does not matter, because *by
default we don't log to eventlog*. The presumption is that if the user
turns on logging to eventlog, it's his responsibility to first make sure
that event_source is set to something appropriate. And who's to say that
plain "PostgreSQL" isn't what he wanted, anyway? Even if he's got
multiple servers on one machine, maybe directing all their logs to the
same place is okay by him.

Also, those who don't run multiple servers are probably not going to
thank us for moving their logs around unnecessarily.

In short, I think we should just reject this idea as introducing more
problems than it solves, and not fully solving even the problem it
purports to solve.

Possibly there's room for a documentation patch reminding users to
make sure that event_source is set appropriately before they turn
on eventlog.

regards, tom lane

On Mon, Jan 27, 2014 at 9:01 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> writes:
>> On Fri, Jan 24, 2014 at 9:10 AM, Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
>
>> To proceed with the review of this patch, I need to know about
>> whether appending version number or any other constant togli
>
>> Default Event Source name is acceptable or not, else for now
>> we can remove this part of code from patch and handle non-default
>> case where the change will be that pg_ctl will enquire non-default
>> event_source value from server.
>
>> Could you please let me know your views about same?
>
> Unless I'm missing something, this entire thread is a tempest in a teapot,
> because the default event_source value does not matter, because *by
> default we don't log to eventlog*. The presumption is that if the user
> turns on logging to eventlog, it's his responsibility to first make sure
> that event_source is set to something appropriate. And who's to say that
> plain "PostgreSQL" isn't what he wanted, anyway? Even if he's got
> multiple servers on one machine, maybe directing all their logs to the
> same place is okay by him.

I think it's matter of user preference, how exactly he wants the setup
and as currently we don't have any strong reason to change default, so
lets keep it intact.

> Also, those who don't run multiple servers are probably not going to
> thank us for moving their logs around unnecessarily.
>
> In short, I think we should just reject this idea as introducing more
> problems than it solves, and not fully solving even the problem it
> purports to solve.
>
> Possibly there's room for a documentation patch reminding users to
> make sure that event_source is set appropriately before they turn
> on eventlog.

Okay, but in that case also right now pg_ctl doesn't know the value
of event source, so I think thats a clear bug and we should go ahead
and fix it.

As you said, I think we can improve documentation in this regard so
that user will be able to setup event log without any such problems.

As part of this patch we can fix the issue (make pg_ctl aware for event
source name) and improve documentation.

With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com

On Fri, Jan 24, 2014 at 4:38 PM, MauMau <maumau307(at)gmail(dot)com> wrote:
>

>> How about below message:
>>
>> event source "<event_source_name>" is already registered.

>> OK, I added several lines for this. Please check the attached patch.

It gives the proper message, but even after error, the second message
box it shows "DLLInstall ... succeeded." I think the reason is that caller
of function DllRegisterServer() doesn't check the return value.

+ char message[1024];

why you have kept message as a global buffer, can't we just declare locally
inside the function?

>> What I had in mind was to change it during initdb, we are already doing it
>> for some other parameter (unix_socket_directories), please refer below
>> code in initdb.c
> Yes, It seems we can do this. However, could you forgive me for leaving this untouched? I'm afraid postgresql.conf.sample's issue is causing
> unnecessary war among people here. That doesn't affect the point of this patch --- make pg_ctl use the event_source setting. Anyway, not all
> parameters in postgresql.conf cannot be used just by uncommenting them. That's another issue.

Okay, I think we can leave it and also remove it from other parts of patch.
Although I found it is the right way, but Tom is not convinced with the idea,
so lets keep the Default event source name handling as it is.

As suggested by Tom, please update documentation.
"> Possibly there's room for a documentation patch reminding users to
> make sure that event_source is set appropriately before they turn
> on eventlog."
I think right place to update this information is where we are explaining
about setting of event log i.e at below link or may be if you find some other
better place:
http://www.postgresql.org/docs/devel/static/runtime-config-logging.html#GUC-LOG-DESTINATION

With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com

Hi, Amit san,

I'm replying to your previous email. I wanted to reply to your latest mail
below, but I removed it from my mailer by mistake.

http://www.postgresql.org/message-id/CAA4eK1LAg6ndZdWLb5e=Ep5DzcE8KZU=JbmO+tFwySYHm2ja=Q@mail.gmail.com

Do you know how I can reply to an email which was deleted locally? I
thought I could download an old mail by clicking "raw" link and import it to
the mailer. However, it requires username/password input, and it seems to
be different from the one for editing CommitFest. I couldn't find how to
authenticate myself.

Anyway, the revised patch is attached.

From: "Amit Kapila" <amit(dot)kapila16(at)gmail(dot)com>
> It gives the proper message, but even after error, the second message
> box it shows "DLLInstall ... succeeded." I think the reason is that caller
> of function DllRegisterServer() doesn't check the return value.

I see. Corrected by checking the return value of DllRegisterServer().

> + char message[1024];

> why you have kept message as a global buffer, can't we just declare
> locally
> inside the function?

I made it a local variable. At first, I thought we might use it in other
functions in the future.

> Okay, I think we can leave it and also remove it from other parts of
> patch.
> Although I found it is the right way, but Tom is not convinced with the
> idea,
> so lets keep the Default event source name handling as it is.

OK, I changed the value of DEFAULT_EVENT_SOURCE to "PostgreSQL".

> As suggested by Tom, please update documentation.
> "> Possibly there's room for a documentation patch reminding users to
> > make sure that event_source is set appropriately before they turn
> > on eventlog."
> I think right place to update this information is where we are explaining
> about setting of event log i.e at below link or may be if you find some
> other
> better place:
> http://www.postgresql.org/docs/devel/static/runtime-config-logging.html#GUC-LOG-DESTINATION

Please let us make this a separate patch. I agree with you about the place
in the manual.

Regards
MauMau

On Fri, Jan 31, 2014 at 8:20 PM, MauMau <maumau307(at)gmail(dot)com> wrote:
> Hi, Amit san,
>
> I'm replying to your previous email. I wanted to reply to your latest mail
> below, but I removed it from my mailer by mistake.
>
> http://www.postgresql.org/message-id/CAA4eK1LAg6ndZdWLb5e=Ep5DzcE8KZU=JbmO+tFwySYHm2ja=Q@mail.gmail.com
>
> Do you know how I can reply to an email which was deleted locally? I
> thought I could download an old mail by clicking "raw" link and import it to
> the mailer. However, it requires username/password input, and it seems to
> be different from the one for editing CommitFest. I couldn't find how to
> authenticate myself.

Not sure, I have not done it before.

> Anyway, the revised patch is attached.
>
> From: "Amit Kapila" <amit(dot)kapila16(at)gmail(dot)com>
>
>
>> As suggested by Tom, please update documentation.
>> "> Possibly there's room for a documentation patch reminding users to
>> > make sure that event_source is set appropriately before they turn
>> > on eventlog."
>> I think right place to update this information is where we are explaining
>> about setting of event log i.e at below link or may be if you find some
>> other
>> better place:
>>
>> http://www.postgresql.org/docs/devel/static/runtime-config-logging.html#GUC-LOG-DESTINATION
>
>
> Please let us make this a separate patch. I agree with you about the place
> in the manual.
Okay, no problem.

As per my review your patch is fine now except for one minor indentation issue.

if (RegCreateKeyEx(HKEY_LOCAL_MACHINE, key_name, 0, NULL, 0, KEY_WRITE,
NULL, &key, &data))

Here it is better to start the parameters in second line from where
the params in
first line, make then inline.

I think it's just a very minor coding style thing, so I am marking this patch as
Ready For Committer.

With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com

On Sat, Feb 1, 2014 at 12:31 PM, Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
> I think it's just a very minor coding style thing, so I am marking this patch as
> Ready For Committer.

I could see that this patch has been marked as Needs Review in CF app.
suggesting that it should be rejected based on Tom's rejection in below mail:
http://www.postgresql.org/message-id/3315.1390836667@sss.pgh.pa.us

If I understand correctly that objection was on changing Default Event
Source name, and the patch now doesn't contain that change, it's
just a bug fix for letting pg_ctl know the non-default event source
set by user.

Please clarify if I misunderstood something, else this should be changed
to Ready For Committer.

With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com

On Mon, Feb 17, 2014 at 9:17 AM, Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
> On Sat, Feb 1, 2014 at 12:31 PM, Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
>> I think it's just a very minor coding style thing, so I am marking this patch as
>> Ready For Committer.
>
> I could see that this patch has been marked as Needs Review in CF app.
> suggesting that it should be rejected based on Tom's rejection in below mail:
> http://www.postgresql.org/message-id/3315.1390836667@sss.pgh.pa.us
>
> If I understand correctly that objection was on changing Default Event
> Source name, and the patch now doesn't contain that change, it's
> just a bug fix for letting pg_ctl know the non-default event source
> set by user.
>
> Please clarify if I misunderstood something, else this should be changed
> to Ready For Committer.

Tom/Andres, please let me know if you have objection for this patch, because
as per my understanding all the objectionable part of patch is removed
from final
patch and it's a defect fix to make pg_ctl aware of Event Source name set in
postgresql.conf.

If there is no objection, I will again change it to Ready For Committer.

With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com

From: "Amit Kapila" <amit(dot)kapila16(at)gmail(dot)com>
>> If I understand correctly that objection was on changing Default Event
>> Source name, and the patch now doesn't contain that change, it's
>> just a bug fix for letting pg_ctl know the non-default event source
>> set by user.
>>
>> Please clarify if I misunderstood something, else this should be changed
>> to Ready For Committer.
>
> Tom/Andres, please let me know if you have objection for this patch,
> because
> as per my understanding all the objectionable part of patch is removed
> from final
> patch and it's a defect fix to make pg_ctl aware of Event Source name set
> in
> postgresql.conf.
>
> If there is no objection, I will again change it to Ready For Committer.

Hi, Amit-san, I really appreciate your cooperation. Yes, I removed the
default value change that caused objection, so the patch can be marked ready
for committer. I understand the patch was marked needs for review by
misunderstanding Tom-san's opinion.

I remember that I read "silence means no objection, or implicit agreement"
somewhere in the community site or ML. So I think it would be no problem to
set the status to ready for committer again.

Regards
MauMau

On Mon, Mar 10, 2014 at 2:39 PM, MauMau <maumau307(at)gmail(dot)com> wrote:
> From: "Amit Kapila" <amit(dot)kapila16(at)gmail(dot)com>
>
>>> If I understand correctly that objection was on changing Default Event
>>> Source name, and the patch now doesn't contain that change, it's
>>> just a bug fix for letting pg_ctl know the non-default event source
>>> set by user.
>>>
>>> Please clarify if I misunderstood something, else this should be changed
>>> to Ready For Committer.
>>
>>
>> Tom/Andres, please let me know if you have objection for this patch,
>> because
>> as per my understanding all the objectionable part of patch is removed
>> from final
>> patch and it's a defect fix to make pg_ctl aware of Event Source name set
>> in
>> postgresql.conf.
>>
>> If there is no objection, I will again change it to Ready For Committer.
>
>
> Hi, Amit-san, I really appreciate your cooperation.

Thanks.

> Yes, I removed the
> default value change that caused objection, so the patch can be marked ready
> for committer. I understand the patch was marked needs for review by
> misunderstanding Tom-san's opinion.
>
> I remember that I read "silence means no objection, or implicit agreement"
> somewhere in the community site or ML. So I think it would be no problem to
> set the status to ready for committer again.

Okay, Done.

With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com

MauMau escribió:
> Hi, Amit san,
>
> I'm replying to your previous email. I wanted to reply to your
> latest mail below, but I removed it from my mailer by mistake.
>
> http://www.postgresql.org/message-id/CAA4eK1LAg6ndZdWLb5e=Ep5DzcE8KZU=JbmO+tFwySYHm2ja=Q@mail.gmail.com
>
> Do you know how I can reply to an email which was deleted locally?
> I thought I could download an old mail by clicking "raw" link and
> import it to the mailer. However, it requires username/password
> input, and it seems to be different from the one for editing
> CommitFest. I couldn't find how to authenticate myself.

The box that asks for password tells you what the user/password is. I
think it's something like archives/archives or similar. The password is
there only to keep spammers out, not to have any real auth.

--
Álvaro Herrera http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

From: "Alvaro Herrera" <alvherre(at)2ndquadrant(dot)com>
> MauMau escribió:
>> Do you know how I can reply to an email which was deleted locally?
>> I thought I could download an old mail by clicking "raw" link and
>> import it to the mailer. However, it requires username/password
>> input, and it seems to be different from the one for editing
>> CommitFest. I couldn't find how to authenticate myself.
>
> The box that asks for password tells you what the user/password is. I
> think it's something like archives/archives or similar. The password is
> there only to keep spammers out, not to have any real auth.

Thank you, the user/password was certainly displayed in the box --
archives/antispam. The "raw" link only gave the mail in text format. I
hoped to import the mail into Windows Mail on Windows Vista, but I couldn't.

Regards
MauMau

MauMau escribió:

> The "raw" link only gave the mail in text format. I hoped to import
> the mail into Windows Mail on Windows Vista, but I couldn't.

You might need to run a conversion process by which you transform the
raw file (in mbox format) into EML format or whatever it is that Windows
Mail uses. I vaguely recall there are tools for this.

--
Álvaro Herrera http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

From: "Alvaro Herrera" <alvherre(at)2ndquadrant(dot)com>
> MauMau escribió:
>
>> The "raw" link only gave the mail in text format. I hoped to import
>> the mail into Windows Mail on Windows Vista, but I couldn't.
>
> You might need to run a conversion process by which you transform the
> raw file (in mbox format) into EML format or whatever it is that Windows
> Mail uses. I vaguely recall there are tools for this.

Thanks. I could open the file without any conversion as follows:

1. Click the "raw" link on the Web browser (I'm using Internet Explorer).

2. The Web browser displays the mail file in text format. Save the file as
a text file (e.g. mail.txt).

3. Just change the extension from .txt to .eml (e.g. mail.eml).

4. Double-click the .eml file on the Windows Explorer. Windows Mail opens
and displayes the mail. I can reply to it.

Regards
MauMau

"MauMau" <maumau307(at)gmail(dot)com> writes:
> [ pg_ctl_eventsrc_v6.patch ]

I looked at this patch a bit. As a non-Windows person I have no intention
of being the committer, since I can't test the Windows-specific changes.
However, I do want to object to the business about having pg_ctl use
"postgres -C" to try to determine the event source name to use. The code
that you repurposed was okay for its original use, namely finding out
where a config directory would point the data directory to, but I don't
think it's up to snuff for identifying the value of event_source that a
running server is using. The actual value might have been set from a
command line option for instance. Moreover, the whole thing seems rather
circular since what pg_ctl wants the event source name for is to report
errors. What if it fails to get the event source name, or needs to
report an error before the (rather late) point at which it even tries
to get it?

regards, tom lane

On Sat, Apr 5, 2014 at 4:58 AM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> "MauMau" <maumau307(at)gmail(dot)com> writes:
>> [ pg_ctl_eventsrc_v6.patch ]
>
> I looked at this patch a bit. As a non-Windows person I have no intention
> of being the committer, since I can't test the Windows-specific changes.
> However, I do want to object to the business about having pg_ctl use
> "postgres -C" to try to determine the event source name to use. The code
> that you repurposed was okay for its original use, namely finding out
> where a config directory would point the data directory to, but I don't
> think it's up to snuff for identifying the value of event_source that a
> running server is using. The actual value might have been set from a
> command line option for instance.

Are you concerned about the case when user passes event_source name
in command line at the time of start:
pg_ctl start -o "-c event_source=PG9.4" -D <data_dir>

If my understanding is right about your concern, then I think it will consider
the above case even when passed in command line. Example
postgres.exe -C event_source -c event_source=PG9.4 -D <data_dir>
PG9.4

> Moreover, the whole thing seems rather
> circular since what pg_ctl wants the event source name for is to report
> errors. What if it fails to get the event source name, or needs to
> report an error before the (rather late) point at which it even tries
> to get it?

In that case, it will use Default Event Source name PostgreSQL which
is same what server also does in case it gets error before processing
of event source config. For Example if we get any error in check_root()
function, it will use Default Event Source name.

With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com

Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> writes:
> Are you concerned about the case when user passes event_source name
> in command line at the time of start:
> pg_ctl start -o "-c event_source=PG9.4" -D <data_dir>

Right.

> If my understanding is right about your concern, then I think it will consider
> the above case even when passed in command line. Example
> postgres.exe -C event_source -c event_source=PG9.4 -D <data_dir>
> PG9.4

How's that going to work during pg_ctl stop? There's no -o switch
provided.

It's conceivable that you could reverse-engineer it by looking at
postmaster.opts as well as what -C mode has to say, and that'd likely work
for the parameters pg_ctl cares about; but my goodness that's ugly and
fragile. You'd basically be reimplementing a lot of GUC logic in pg_ctl.

In any case, the real problem is that even if you trust the -C result to
be right, by the time you've got this information there have already been
a whole lot of opportunities for failures. It doesn't seem to me that
having pg_ctl switch its error reporting target halfway through is really
such a great idea.

> In that case, it will use Default Event Source name PostgreSQL which
> is same what server also does in case it gets error before processing
> of event source config.

That analogy doesn't hold because (1) a server spends most of its time
already up, so only a tiny fraction of its error traffic might go to the
default event source, and (2) actually, pretty much *none* of a server's
log traffic will go to the default event source if something else has been
configured, because the default value of log_destination isn't "eventlog".
There might be a small window where GUC has assigned log_destination but
not yet event_source from postgresql.conf, but it's just about negligible.

In contrast, a large fraction of pg_ctl's possible error traffic is
concerned with early failures like can't-find-the-postgres-executable
or can't-find-the-data-directory, either of which would foreclose any
possibility of selecting an event source that matches the server.

So basically, I think having pg_ctl try to do what this patch proposes
is a bad idea. If there's anyone who actually cares about where pg_ctl
logs to on Windows, what would make sense is to add a pg_ctl switch to
specify what event_source name to use. That's still not perfect of
course, but at least only command-line-parsing errors would come out
before the setting could be adopted.

regards, tom lane

On Sat, Apr 5, 2014 at 8:24 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> writes:
>> Are you concerned about the case when user passes event_source name
>> in command line at the time of start:
>> pg_ctl start -o "-c event_source=PG9.4" -D <data_dir>
>
> Right.
>
>> If my understanding is right about your concern, then I think it will consider
>> the above case even when passed in command line. Example
>> postgres.exe -C event_source -c event_source=PG9.4 -D <data_dir>
>> PG9.4
>
> How's that going to work during pg_ctl stop? There's no -o switch
> provided.

As there's no -o switch, so there won't be problem of getting wrong event
source name from server due to command line options which you mentioned
upthread or is there something which I am missing about it?

> It's conceivable that you could reverse-engineer it by looking at
> postmaster.opts as well as what -C mode has to say, and that'd likely work
> for the parameters pg_ctl cares about; but my goodness that's ugly and
> fragile. You'd basically be reimplementing a lot of GUC logic in pg_ctl.
>
> In any case, the real problem is that even if you trust the -C result to
> be right, by the time you've got this information there have already been
> a whole lot of opportunities for failures. It doesn't seem to me that
> having pg_ctl switch its error reporting target halfway through is really
> such a great idea.

You are right that with the current patch approach, we will miss many
opportunities for failures and the way suggested by you below (new switch)
is more appropriate to fix this issue. Another thought that occurred to me
is why not change the failures which are before set of appropriate
event_source to report on console. The main reason for using event log
to report error's in pg_ctl is because it can be used for service
(register/unregister/..) in Windows and all the work we do before setting
event_source is not related to it's usage as a service.

With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com

Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> writes:
> On Sat, Apr 5, 2014 at 8:24 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>> How's that going to work during pg_ctl stop? There's no -o switch
>> provided.

> As there's no -o switch, so there won't be problem of getting wrong event
> source name from server due to command line options which you mentioned
> upthread or is there something which I am missing about it?

AFAICS, the sole argument for trying to do things this way is to log to
the same event_source the running postmaster is using. But -C will not
provide that; it will only tell you what the postmaster *would* use if
it were freshly started right now. If -o had been used at postmaster
start time, an inquiry done by pg_ctl stop (or reload, etc) won't match.
Another, possibly more plausible, failure mode is if the entry in
postgresql.conf has been edited since the running postmaster looked at it.

Admittedly, all of these cases are kind of unusual. But that's all the
more reason, IMO, to be wary of sending our error messages to an
unexpected place in such cases. Somebody who's trying to debug a
configuration problem doesn't need the added complication of trying to
figure out where pg_ctl's error messages might have gone.

> You are right that with the current patch approach, we will miss many
> opportunities for failures and the way suggested by you below (new switch)
> is more appropriate to fix this issue. Another thought that occurred to me
> is why not change the failures which are before set of appropriate
> event_source to report on console. The main reason for using event log
> to report error's in pg_ctl is because it can be used for service
> (register/unregister/..) in Windows and all the work we do before setting
> event_source is not related to it's usage as a service.

AFAICS, pg_ctl already reports to stderr if stderr is a tty. This whole
issue only comes up when pg_ctl itself is running as a service (which
I guess primarily means starting/stopping the postgresql service?).
So that puts extra weight on trying to be sure that error messages go
to a predictable place; the user's going to be hard-pressed to debug
background failures without that.

regards, tom lane

On Sat, Apr 5, 2014 at 10:54 AM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> So basically, I think having pg_ctl try to do what this patch proposes
> is a bad idea.

I'm not a Windows person either, but I tend to agree. I can't think
that this is going to be very robust ... and if it's not going to be
robust, what's the point?

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On Mon, Apr 7, 2014 at 7:10 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> writes:
>> On Sat, Apr 5, 2014 at 8:24 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> AFAICS, pg_ctl already reports to stderr if stderr is a tty. This whole
> issue only comes up when pg_ctl itself is running as a service (which
> I guess primarily means starting/stopping the postgresql service?).
> So that puts extra weight on trying to be sure that error messages go
> to a predictable place; the user's going to be hard-pressed to debug
> background failures without that.

Agreed. I think if this needs to fixed, then it will be better to do
as per your suggestion of adding a new switch. So I have marked this
patch as "Returned with feedback".

With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com

Hello, Amit san, Tom san,

I'm sorry for my late response. I've just caught up with the discussion.
I'm almost convinced.

Please find attached the revised patch. I'd like to follow the idea of
adding a switch to pg_ctl. The newly added ""-e event_source" sets the
event source name for pg_ctl to use. When -e is used with pg_ctl register,
it will be added to the command line for Windows service (pg_ctl
runservice).

Regards
MauMau

On Sat, Apr 12, 2014 at 1:21 PM, MauMau <maumau307(at)gmail(dot)com> wrote:
> Hello, Amit san, Tom san,
>
> I'm sorry for my late response. I've just caught up with the discussion.
> I'm almost convinced.
>
> Please find attached the revised patch. I'd like to follow the idea of
> adding a switch to pg_ctl. The newly added ""-e event_source" sets the
> event source name for pg_ctl to use. When -e is used with pg_ctl register,
> it will be added to the command line for Windows service (pg_ctl
> runservice).

Currently -e option is accepted with all the options that can be provided
in pg_ctl. Shouldn't we accept it only with options related to service,
because that is only when it will be used. Basically write_stderr() will
write to event log only incase of service.

Another minor point is you have forgotten to remove below declaration:
+ static void get_config_value(const char *name, char *buf, int buf_size);

Sorry for delayed response and I am not sure that I will be able to
complete the review of patch in next few days as I will be on vacation.

With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com

From: "Amit Kapila" <amit(dot)kapila16(at)gmail(dot)com>
> Currently -e option is accepted with all the options that can be provided
> in pg_ctl. Shouldn't we accept it only with options related to service,
> because that is only when it will be used. Basically write_stderr() will
> write to event log only incase of service.

Thank you for your kind and patient review. I hope this will bear fruit.

I don't find it so strange that -e is accepted by all operation modes of
pg_ctl --- pg_ctl seems to handle switches that way. For example, -c is
only relevant to start and restart, but it is accepted by all modes. -D is
not relevant to pg_ctl kill, but it is not rejected. Plus, I prepared for
the future possibility that other modes of pg_ctl will use event log.

> Another minor point is you have forgotten to remove below declaration:
> + static void get_config_value(const char *name, char *buf, int buf_size);

Oh, removed.

> Sorry for delayed response and I am not sure that I will be able to
> complete the review of patch in next few days as I will be on vacation.

OK, I'm waiting for you. Please have a nice vacation.

Regards
MauMau

On Fri, Apr 18, 2014 at 5:21 PM, MauMau <maumau307(at)gmail(dot)com> wrote:
> From: "Amit Kapila" <amit(dot)kapila16(at)gmail(dot)com>
>
>> Currently -e option is accepted with all the options that can be provided
>> in pg_ctl. Shouldn't we accept it only with options related to service,
>> because that is only when it will be used. Basically write_stderr() will
>> write to event log only incase of service.
>
>
> Thank you for your kind and patient review. I hope this will bear fruit.
>
> I don't find it so strange that -e is accepted by all operation modes of
> pg_ctl --- pg_ctl seems to handle switches that way. For example, -c is
> only relevant to start and restart, but it is accepted by all modes. -D is
> not relevant to pg_ctl kill, but it is not rejected. Plus, I prepared for
> the future possibility that other modes of pg_ctl will use event log.

Okay, as there is no check for not-required switches with other options,
we can ignore this as well.

I have verified the patch and it works fine on Windows, as per my
verification event-source in pg_ctl will be only used once the user has
registered the service and then when it tries to preform start/stop on
service. Although current usecase seems to be quite narrow for a new
switch, however in future we can extend it to use for other messages
in pg_ctl as well.

Only one minor suggestion:
+ Name of the event source for pg_ctl to use for event log. The
+ default is PostgreSQL.

From this description, it is not clear when the event log will be used
in pg_ctl. For example, if user uses -e option with register, then he
might expect any failure during register command will be logged into
eventlog, but that is not true. So I think we should improve the docs
to reflect the usage of -e.

On Linux, I am getting below build failure for pg_ctl

pg_ctl.c: In function ‘main’:
pg_ctl.c:2173: error: ‘event_source’ undeclared (first use in this function)
pg_ctl.c:2173: error: (Each undeclared identifier is reported only once
pg_ctl.c:2173: error: for each function it appears in.)
make[3]: *** [pg_ctl.o] Error 1
make[2]: *** [all-pg_ctl-recurse] Error 2
make[1]: *** [all-bin-recurse] Error 2
make: *** [all-src-recurse] Error 2

With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com

From: "Amit Kapila" <amit(dot)kapila16(at)gmail(dot)com>
Only one minor suggestion:
+ Name of the event source for pg_ctl to use for event log. The
+ default is PostgreSQL.

From this description, it is not clear when the event log will be used
in pg_ctl. For example, if user uses -e option with register, then he
might expect any failure during register command will be logged into
eventlog, but that is not true. So I think we should improve the docs
to reflect the usage of -e.

On Linux, I am getting below build failure for pg_ctl

pg_ctl.c: In function ‘main’:
pg_ctl.c:2173: error: ‘event_source’ undeclared (first use in this
function)
pg_ctl.c:2173: error: (Each undeclared identifier is reported only once
pg_ctl.c:2173: error: for each function it appears in.)
make[3]: *** [pg_ctl.o] Error 1
make[2]: *** [all-pg_ctl-recurse] Error 2
make[1]: *** [all-bin-recurse] Error 2
make: *** [all-src-recurse] Error 2

Thank you for reviewing and testing. I changed the doc, but I'd appreciate
it if you could improve my poor English and update the CommitFest if it can
be better.

I rebased the patch to HEAD and removed the compilation error on Linux. I
made event_source variable on all platforms like register_servicename,
although they are not necessary on non-Windows platforms.

Regards
MauMau

On Thu, May 8, 2014 at 4:47 PM, MauMau <maumau307(at)gmail(dot)com> wrote:
> I rebased the patch to HEAD and removed the compilation error on Linux. I
> made event_source variable on all platforms like register_servicename,
> although they are not necessary on non-Windows platforms.

I have verified that current patch is fine and I have marked it as
Ready For Committer.

I think it's bit late for this patch for 9.4, you might want to move it to
next CF.

With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com

From: "Amit Kapila" <amit(dot)kapila16(at)gmail(dot)com>
> I think it's bit late for this patch for 9.4, you might want to move it to
> next CF.

Thanks, I've moved it. It's a regret that this very small patch wasn't put
in 9.4.

Regards
MauMau

On Sat, May 10, 2014 at 4:10 PM, MauMau <maumau307(at)gmail(dot)com> wrote:
> From: "Amit Kapila" <amit(dot)kapila16(at)gmail(dot)com>
>
>> I think it's bit late for this patch for 9.4, you might want to move it to
>> next CF.
>
>
> Thanks, I've moved it. It's a regret that this very small patch wasn't put
> in 9.4.

i took a look at the latest version of this patch, since it's marked
as ready for committer. A few comments:

Is there a reason for there still being changes in guc.c, pgevent.c
etc? Shouldn't it all be confined to pg_ctl now? That's my
understanding from the thread that that's the only part we care about.

More importantly, isn't it wrong to claim it will only be used for
register and unregister? If we get an early failure in start, for
example, there are numerous codepaths that will end up calling
write_stderr(), which will use the eventlog when running as a service.
Shouldn't the "-e" parameter be moved under "common options"?

I also think we should have the documentation specifically note that
regular postgres output still goes to whatever the backend is
configured to do. (and of course, any failure within the backend
*before* we have parsed the config file for example will still go to
the default source, and not the pg_ctl source - so we need to make it
really clear that this is *only* for output from pg_ctl).

--
Magnus Hagander
Me: http://www.hagander.net/
Work: http://www.redpill-linpro.com/

From: "Magnus Hagander" <magnus(at)hagander(dot)net>
> Is there a reason for there still being changes in guc.c, pgevent.c
> etc? Shouldn't it all be confined to pg_ctl now? That's my
> understanding from the thread that that's the only part we care about.

Yes, strictly speaking, those are useful for the original proposal.
However, they are still useful as refactoring, since the current code has
the default event source "PostgreSQL" in many places. Defining the default
name only at one location would make it easier for vendors like EnterpriseDB
to change the default name for their products. So I hope this will also be
included if you don't want to reject it by all means.

> More importantly, isn't it wrong to claim it will only be used for
> register and unregister? If we get an early failure in start, for
> example, there are numerous codepaths that will end up calling
> write_stderr(), which will use the eventlog when running as a service.
> Shouldn't the "-e" parameter be moved under "common options"?

Yes, you are right. -e is effective only if pg_ctl is invoked as a Windows
service. So it is written at register mode. That is, -e specifies the
event source used by the Windows service which is registered by "pg_ctl
register".

> I also think we should have the documentation specifically note that
> regular postgres output still goes to whatever the backend is
> configured to do. (and of course, any failure within the backend
> *before* we have parsed the config file for example will still go to
> the default source, and not the pg_ctl source - so we need to make it
> really clear that this is *only* for output from pg_ctl).

I see. I added this clarification to the description of -e. I would
appreciate it if you could correct my poor English when committing, if it
needs improvement.

Regards
MauMau

On Tue, Jul 15, 2014 at 4:01 PM, MauMau <maumau307(at)gmail(dot)com> wrote:
> From: "Magnus Hagander" <magnus(at)hagander(dot)net>
>
>> Is there a reason for there still being changes in guc.c, pgevent.c
>> etc? Shouldn't it all be confined to pg_ctl now? That's my
>> understanding from the thread that that's the only part we care about.
>
>
> Yes, strictly speaking, those are useful for the original proposal. However,
> they are still useful as refactoring, since the current code has the default
> event source "PostgreSQL" in many places. Defining the default name only at
> one location would make it easier for vendors like EnterpriseDB to change
> the default name for their products. So I hope this will also be included
> if you don't want to reject it by all means.

Well, it does in a couple of places. I'm nto sure it's that important
(as nobody has AFAIK ever requested that change from for example EDB),
but it's not a bad thing. However, with a hardcoded service name, I
think the changes to pg_event.c are probably both not necessary and
actually bad - as you'll now start getting errors in more harmless
scenarios.

>> More importantly, isn't it wrong to claim it will only be used for
>> register and unregister? If we get an early failure in start, for
>> example, there are numerous codepaths that will end up calling
>> write_stderr(), which will use the eventlog when running as a service.
>> Shouldn't the "-e" parameter be moved under "common options"?
>
>
> Yes, you are right. -e is effective only if pg_ctl is invoked as a Windows
> service. So it is written at register mode. That is, -e specifies the
> event source used by the Windows service which is registered by "pg_ctl
> register".

Oh, right. I see what you mean now. That goes for all parameters
though, including -D, and we don't specify those as register mode
only, so I still think it's wrong to place it there. It is now grouped
with all other parameters that we specifically *don't* write to the
commandline of the service.

>> I also think we should have the documentation specifically note that
>> regular postgres output still goes to whatever the backend is
>> configured to do. (and of course, any failure within the backend
>> *before* we have parsed the config file for example will still go to
>> the default source, and not the pg_ctl source - so we need to make it
>> really clear that this is *only* for output from pg_ctl).
>
>
> I see. I added this clarification to the description of -e. I would
> appreciate it if you could correct my poor English when committing, if it
> needs improvement.

Sure, no problem.

--
Magnus Hagander
Me: http://www.hagander.net/
Work: http://www.redpill-linpro.com/

From: "Magnus Hagander" <magnus(at)hagander(dot)net>
> Well, it does in a couple of places. I'm nto sure it's that important
> (as nobody has AFAIK ever requested that change from for example EDB),
> but it's not a bad thing. However, with a hardcoded service name, I
> think the changes to pg_event.c are probably both not necessary and
> actually bad - as you'll now start getting errors in more harmless
> scenarios.

Thank you. OK, in fact, all I want in pgevent.c is this:

! char event_source[256] = "PostgreSQL";
...
! char event_source[256] = DEFAULT_EVENT_SOURCE;

But what kind of errors do we get with other changes in pgevent.c? I made
these changes according to Amit-san's notice (please look at his comment
upthread), and I think he is right.

> Oh, right. I see what you mean now. That goes for all parameters
> though, including -D, and we don't specify those as register mode
> only, so I still think it's wrong to place it there. It is now grouped
> with all other parameters that we specifically *don't* write to the
> commandline of the service.

OK, let me reconsider this tomorrow. It's already after midnight here in
Japan, and I have to go to bed so that I can wake up tomorrow.

Regards
MauMau

On Tue, Jul 15, 2014 at 8:57 PM, MauMau <maumau307(at)gmail(dot)com> wrote:
>
> From: "Magnus Hagander" <magnus(at)hagander(dot)net>
>
>> Well, it does in a couple of places. I'm nto sure it's that important
>> (as nobody has AFAIK ever requested that change from for example EDB),
>> but it's not a bad thing.

I think this is nothing specific to any vendor rather it's good to have
a define when it is used at multiple places.

With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com

On Wed, Jul 16, 2014 at 6:37 AM, Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
> On Tue, Jul 15, 2014 at 8:57 PM, MauMau <maumau307(at)gmail(dot)com> wrote:
>>
>> From: "Magnus Hagander" <magnus(at)hagander(dot)net>
>>
>>> Well, it does in a couple of places. I'm nto sure it's that important
>>> (as nobody has AFAIK ever requested that change from for example EDB),
>>> but it's not a bad thing.
>
> I think this is nothing specific to any vendor rather it's good to have
> a define when it is used at multiple places.

Sure, I don't object to the change itself, but the motivation was strange.

There's also the change to throw an error if the source is already
registered, which is potentially a bigger problem. Since the default
will be the same everywhere, do we really want to throw an error when
you install a second version, now that this is the normal state?

There's also definitely a problem in that that codepath fires up a
MessageBox, but it's just a function called in a DLL. It might just as
well be called from a background service or from within an installer
with no visible desktop, at which point the process will appear to be
hung... I'm pretty sure you're not allowed to do that.

--
Magnus Hagander
Me: http://www.hagander.net/
Work: http://www.redpill-linpro.com/

On Wed, Jul 16, 2014 at 2:11 PM, Magnus Hagander <magnus(at)hagander(dot)net>
wrote:
> On Wed, Jul 16, 2014 at 6:37 AM, Amit Kapila <amit(dot)kapila16(at)gmail(dot)com>
wrote:
>
> There's also the change to throw an error if the source is already
> registered, which is potentially a bigger problem.

I think generally if the s/w is installed/registered and we try to install
it
second time without un-installing previous one, it gives some notice
indicating the same.

> Since the default
> will be the same everywhere, do we really want to throw an error when
> you install a second version, now that this is the normal state?

Allowing second version to overwrite the first can also cause problems
similar to what is reported in this thread, basically what if the second
installation/registration is removed, won't it cause the first one to loose
messages?

> There's also definitely a problem in that that codepath fires up a
> MessageBox, but it's just a function called in a DLL.

There are other paths in the same code which already fires up
MessageBox.

> It might just as
> well be called from a background service or from within an installer
> with no visible desktop, at which point the process will appear to be
> hung... I'm pretty sure you're not allowed to do that.

So in that case shouldn't we get rid of MessageBox() or provide
alternate way of logging from pgevent module.

With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com

On Wed, Jul 16, 2014 at 12:31 PM, Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
> On Wed, Jul 16, 2014 at 2:11 PM, Magnus Hagander <magnus(at)hagander(dot)net>
> wrote:
>> On Wed, Jul 16, 2014 at 6:37 AM, Amit Kapila <amit(dot)kapila16(at)gmail(dot)com>
>> wrote:
>>
>> There's also the change to throw an error if the source is already
>> registered, which is potentially a bigger problem.
>
> I think generally if the s/w is installed/registered and we try to install
> it
> second time without un-installing previous one, it gives some notice
> indicating the same.
>
>> Since the default
>> will be the same everywhere, do we really want to throw an error when
>> you install a second version, now that this is the normal state?
>
> Allowing second version to overwrite the first can also cause problems
> similar to what is reported in this thread, basically what if the second
> installation/registration is removed, won't it cause the first one to loose
> messages?

It will, this is true. I'm not sure there's a good way around that
given now Windows Event Logs work though, unless we restrict usage a
lot (as in only one installation of postgres at a time for example). I
thnk it's better to document what you need to do in a case like this
(re-register the existing DLL).

>> There's also definitely a problem in that that codepath fires up a
>> MessageBox, but it's just a function called in a DLL.
>
> There are other paths in the same code which already fires up
> MessageBox.

Ouch, I didn't realize that - I just looked at the patch. What's worse
is it's probably written by me :)

However, I'd say the one being added here is much more likely to fire
under such circumstances. Of the existing ones, the only really likely
case for them to fire is a permission denied case, and that will most
likely only happen from an interactive session. That might be why we
haven't seen it...

>> It might just as
>> well be called from a background service or from within an installer
>> with no visible desktop, at which point the process will appear to be
>> hung... I'm pretty sure you're not allowed to do that.
>
> So in that case shouldn't we get rid of MessageBox() or provide
> alternate way of logging from pgevent module.

It's something we might want to consider, but let's consider that a
separate patch.

Actually, it surprises me that we haven't had an issue before. But I
guess maybe the installers don't actually use regsvr32, but instead
just registers it manually by sticking it in the registry?

--
Magnus Hagander
Me: http://www.hagander.net/
Work: http://www.redpill-linpro.com/

On Wed, Jul 16, 2014 at 4:06 PM, Magnus Hagander <magnus(at)hagander(dot)net>
wrote:
>
> On Wed, Jul 16, 2014 at 12:31 PM, Amit Kapila <amit(dot)kapila16(at)gmail(dot)com>
wrote:
> > On Wed, Jul 16, 2014 at 2:11 PM, Magnus Hagander <magnus(at)hagander(dot)net>
> > wrote:
> >> On Wed, Jul 16, 2014 at 6:37 AM, Amit Kapila <amit(dot)kapila16(at)gmail(dot)com>
> >> wrote:
> >>
> >> There's also the change to throw an error if the source is already
> >> registered, which is potentially a bigger problem.
> >
> > I think generally if the s/w is installed/registered and we try to
install
> > it
> > second time without un-installing previous one, it gives some notice
> > indicating the same.
> >
> >> Since the default
> >> will be the same everywhere, do we really want to throw an error when
> >> you install a second version, now that this is the normal state?
> >
> > Allowing second version to overwrite the first can also cause problems
> > similar to what is reported in this thread, basically what if the second
> > installation/registration is removed, won't it cause the first one to
loose
> > messages?
>
> It will, this is true. I'm not sure there's a good way around that
> given now Windows Event Logs work though, unless we restrict usage a
> lot (as in only one installation of postgres at a time for example). I
> thnk it's better to document what you need to do in a case like this
> (re-register the existing DLL).

Given that now user has a way to use separate names for event log
messages, I think it is better not to change default behaviour and rather
just document the same.

> >> There's also definitely a problem in that that codepath fires up a
> >> MessageBox, but it's just a function called in a DLL.
> >
> > There are other paths in the same code which already fires up
> > MessageBox.
>
> Ouch, I didn't realize that - I just looked at the patch. What's worse
> is it's probably written by me :)
>
> However, I'd say the one being added here is much more likely to fire
> under such circumstances. Of the existing ones, the only really likely
> case for them to fire is a permission denied case, and that will most
> likely only happen from an interactive session. That might be why we
> haven't seen it...
>
> >> It might just as
> >> well be called from a background service or from within an installer
> >> with no visible desktop, at which point the process will appear to be
> >> hung... I'm pretty sure you're not allowed to do that.
> >
> > So in that case shouldn't we get rid of MessageBox() or provide
> > alternate way of logging from pgevent module.
>
> It's something we might want to consider, but let's consider that a
> separate patch.

Sure, that make sense.

So as a conclusion, the left over items to be handled for patch are:
1. Remove the new usage related to use of same event source name
for registration from pgevent.
2. Document the information to prevent loss of messages in some
scenarios as explained above.

With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com

From: "Magnus Hagander" <magnus(at)hagander(dot)net>
> There's also the change to throw an error if the source is already
> registered, which is potentially a bigger problem. Since the default
> will be the same everywhere, do we really want to throw an error when
> you install a second version, now that this is the normal state?
>
> There's also definitely a problem in that that codepath fires up a
> MessageBox, but it's just a function called in a DLL. It might just as
> well be called from a background service or from within an installer
> with no visible desktop, at which point the process will appear to be
> hung... I'm pretty sure you're not allowed to do that.

I got what you mean. I removed changes in pgevent.c except for the default
name. I attached the revised patch.

>>> More importantly, isn't it wrong to claim it will only be used for
>>> register and unregister? If we get an early failure in start, for
>>> example, there are numerous codepaths that will end up calling
>>> write_stderr(), which will use the eventlog when running as a service.
>>> Shouldn't the "-e" parameter be moved under "common options"?
>>
>>
>> Yes, you are right. -e is effective only if pg_ctl is invoked as a
>> Windows
>> service. So it is written at register mode. That is, -e specifies the
>> event source used by the Windows service which is registered by "pg_ctl
>> register".
>
> Oh, right. I see what you mean now. That goes for all parameters
> though, including -D, and we don't specify those as register mode
> only, so I still think it's wrong to place it there. It is now grouped
> with all other parameters that we specifically *don't* write to the
> commandline of the service.

Sorry, but I'm probably not understanding your comment. This may be due to
my English capability. -e is effective only on Windows, so it is placed in
section "Options for Windows". And I could not find a section named "Common
options". -e is currently meangful only with register mode, so it is placed
at register mode in Synopsis section. For example, -D is not attached to
kill mode.

Do you suggest that -e should be attached to all modes in Synopsis section,
or -e should be placed in the section "Options" instead of "Options for
Windows"?

Regards
MauMau

From: "Amit Kapila" <amit(dot)kapila16(at)gmail(dot)com>
So as a conclusion, the left over items to be handled for patch are:
> 1. Remove the new usage related to use of same event source name
> for registration from pgevent.
> 2. Document the information to prevent loss of messages in some
> scenarios as explained above.

I noticed the continued discussion after I had prepared and submitted the
revised patch. OK, how about the patch in the previous mail, Magnus-san?

Regards
MauMau

On Wed, Jul 16, 2014 at 2:01 PM, MauMau <maumau307(at)gmail(dot)com> wrote:
> From: "Amit Kapila" <amit(dot)kapila16(at)gmail(dot)com>
>
> So as a conclusion, the left over items to be handled for patch are:
>>
>> 1. Remove the new usage related to use of same event source name
>> for registration from pgevent.
>> 2. Document the information to prevent loss of messages in some
>> scenarios as explained above.
>
>
> I noticed the continued discussion after I had prepared and submitted the
> revised patch. OK, how about the patch in the previous mail, Magnus-san?

I mean that the -e option is valid for *all* commands, not just
register/unregister. If you include it in register, pg_ctl will write
it to the commandline used for start -- so clearly it is valid for the
start command as well. And it is certainly possible for a completely
different service to run pg_ctl start, in which case it will also be
used.

I have updated the patch with this change, so please verify that it
still works. I also rewrote the documentation slightly.

With that, applied. Thanks!

--
Magnus Hagander
Me: http://www.hagander.net/
Work: http://www.redpill-linpro.com/

On Thu, Jul 17, 2014 at 12:45 PM, Magnus Hagander <magnus(at)hagander(dot)net> wrote:
> On Wed, Jul 16, 2014 at 2:01 PM, MauMau <maumau307(at)gmail(dot)com> wrote:
>> From: "Amit Kapila" <amit(dot)kapila16(at)gmail(dot)com>
>>
>> So as a conclusion, the left over items to be handled for patch are:
>>>
>>> 1. Remove the new usage related to use of same event source name
>>> for registration from pgevent.
>>> 2. Document the information to prevent loss of messages in some
>>> scenarios as explained above.
>>
>>
>> I noticed the continued discussion after I had prepared and submitted the
>> revised patch. OK, how about the patch in the previous mail, Magnus-san?
>
> I mean that the -e option is valid for *all* commands, not just
> register/unregister. If you include it in register, pg_ctl will write
> it to the commandline used for start -- so clearly it is valid for the
> start command as well. And it is certainly possible for a completely
> different service to run pg_ctl start, in which case it will also be
> used.
>
> I have updated the patch with this change, so please verify that it
> still works. I also rewrote the documentation slightly.
>
> With that, applied. Thanks!

Did anyone actually test this patch? :)

I admit I did not build it on Windows specifically because I assumed
that was done as part of the development and review. And the changes
to pg_event.c can never have built, since the file does not include
the required header.

I have reverted that part of the patch for now, hopefully that'll
unbreak the buildfarm.

For the time being at least I left the other changes to DEFAULT_EVENT_SOURCE in.

--
Magnus Hagander
Me: http://www.hagander.net/
Work: http://www.redpill-linpro.com/

On Thu, Jul 17, 2014 at 4:51 PM, Magnus Hagander <magnus(at)hagander(dot)net>
wrote:
>
> Did anyone actually test this patch? :)
>
> I admit I did not build it on Windows specifically because I assumed
> that was done as part of the development and review. And the changes
> to pg_event.c can never have built, since the file does not include
> the required header.

I have tested it on Windows and infact on Linux as well to see if there
is any side impact before marking it as Ready For Committer.

It seems to me that the required header is removed in last version
(pg_ctl_eventsrc_v11) where MessageBox() related changes have been
removed from patch as per recent discussion. Sorry for not being able
to check last version posted.

> I have reverted that part of the patch for now, hopefully that'll
> unbreak the buildfarm.

Do you want me to write a patch to use DEFAULT_EVENT_SOURCE in
pgevent?

With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com

On Fri, Jul 18, 2014 at 5:33 AM, Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
> On Thu, Jul 17, 2014 at 4:51 PM, Magnus Hagander <magnus(at)hagander(dot)net>
> wrote:
>>
>> Did anyone actually test this patch? :)
>>
>> I admit I did not build it on Windows specifically because I assumed
>> that was done as part of the development and review. And the changes
>> to pg_event.c can never have built, since the file does not include
>> the required header.
>
> I have tested it on Windows and infact on Linux as well to see if there
> is any side impact before marking it as Ready For Committer.
>
> It seems to me that the required header is removed in last version
> (pg_ctl_eventsrc_v11) where MessageBox() related changes have been
> removed from patch as per recent discussion. Sorry for not being able
> to check last version posted.

Gotcha. Thanks for clarifying, and I apologize if I came across a bit
harsh even with the smiley.

>> I have reverted that part of the patch for now, hopefully that'll
>> unbreak the buildfarm.
>
> Do you want me to write a patch to use DEFAULT_EVENT_SOURCE in
> pgevent?

I'm not sure it's worth it. I do like the property that pg_event
doesn't have to pull in the full set of headers. But I guess it's
quite confusing to have *one* place that doesn't use the #define. So I
guess if it does work to just pull int he required header then yes, we
should do it - but if it turns out that header causes any conflicts
with anything else we're doing in the file, then let's just skipp it.

--
Magnus Hagander
Me: http://www.hagander.net/
Work: http://www.redpill-linpro.com/

From: "Magnus Hagander" <magnus(at)hagander(dot)net>
> On Fri, Jul 18, 2014 at 5:33 AM, Amit Kapila <amit(dot)kapila16(at)gmail(dot)com>
> wrote:
>> On Thu, Jul 17, 2014 at 4:51 PM, Magnus Hagander <magnus(at)hagander(dot)net>
>> wrote:
>>>
>>> Did anyone actually test this patch? :)
>>>
>>> I admit I did not build it on Windows specifically because I assumed
>>> that was done as part of the development and review. And the changes
>>> to pg_event.c can never have built, since the file does not include
>>> the required header.
>>
>> I have tested it on Windows and infact on Linux as well to see if there
>> is any side impact before marking it as Ready For Committer.
>>
>> It seems to me that the required header is removed in last version
>> (pg_ctl_eventsrc_v11) where MessageBox() related changes have been
>> removed from patch as per recent discussion. Sorry for not being able
>> to check last version posted.
>
> Gotcha. Thanks for clarifying, and I apologize if I came across a bit
> harsh even with the smiley.

I'm sorry to have caused both of you trouble. I have to admit that I didn't
compile the source when I removed the MessageBox()-related changes. The
attached patch fixes that. I confirmed successful build this time.

Thank you for committing, Magnus-san, and thank you very much for kind and
repeated review and help, Amit-san.

Regards
MauMau

On Fri, Jul 18, 2014 at 7:08 PM, MauMau <maumau307(at)gmail(dot)com> wrote:
>
> From: "Magnus Hagander" <magnus(at)hagander(dot)net>
>
>> On Fri, Jul 18, 2014 at 5:33 AM, Amit Kapila <amit(dot)kapila16(at)gmail(dot)com>
wrote:
>>>
>>> On Thu, Jul 17, 2014 at 4:51 PM, Magnus Hagander <magnus(at)hagander(dot)net>
>>> wrote:
>>>>
>>>>
>>>> Did anyone actually test this patch? :)
>>>>
>>>> I admit I did not build it on Windows specifically because I assumed
>>>> that was done as part of the development and review. And the changes
>>>> to pg_event.c can never have built, since the file does not include
>>>> the required header.
>>>
>>>
>>> I have tested it on Windows and infact on Linux as well to see if there
>>> is any side impact before marking it as Ready For Committer.
>>>
>>> It seems to me that the required header is removed in last version
>>> (pg_ctl_eventsrc_v11) where MessageBox() related changes have been
>>> removed from patch as per recent discussion. Sorry for not being able
>>> to check last version posted.
>>
>>
>> Gotcha. Thanks for clarifying, and I apologize if I came across a bit
>> harsh even with the smiley.

The statement was not at all harsh. I think you are right in asking that
question and I believe that is the minimum expectation once the patch
reaches Ready To Committer stage.

>
> I'm sorry to have caused both of you trouble. I have to admit that I
didn't compile the source when I removed the MessageBox()-related changes.
The attached patch fixes that. I confirmed successful build this time.

I have tested the attached patch on windows, it works fine both for
default and non-default cases. It passes other regression tests as well
and build went fine on Linux.

One more thing about inclusion of new header file in pgevent.c, I think
that is okay because we include it in other modules (client side) present
in bin directory like reindex.

With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com

On Sun, Jul 20, 2014 at 7:44 AM, Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
> On Fri, Jul 18, 2014 at 7:08 PM, MauMau <maumau307(at)gmail(dot)com> wrote:
>>
>> From: "Magnus Hagander" <magnus(at)hagander(dot)net>
>>
>>> On Fri, Jul 18, 2014 at 5:33 AM, Amit Kapila <amit(dot)kapila16(at)gmail(dot)com>
>>> wrote:
>>>>
>>>> On Thu, Jul 17, 2014 at 4:51 PM, Magnus Hagander <magnus(at)hagander(dot)net>
>>>> wrote:
>>>>>
>>>>>
>>>>> Did anyone actually test this patch? :)
>>>>>
>>>>> I admit I did not build it on Windows specifically because I assumed
>>>>> that was done as part of the development and review. And the changes
>>>>> to pg_event.c can never have built, since the file does not include
>>>>> the required header.
>>>>
>>>>
>>>> I have tested it on Windows and infact on Linux as well to see if there
>>>> is any side impact before marking it as Ready For Committer.
>>>>
>>>> It seems to me that the required header is removed in last version
>>>> (pg_ctl_eventsrc_v11) where MessageBox() related changes have been
>>>> removed from patch as per recent discussion. Sorry for not being able
>>>> to check last version posted.
>>>
>>>
>>> Gotcha. Thanks for clarifying, and I apologize if I came across a bit
>>> harsh even with the smiley.
>
> The statement was not at all harsh. I think you are right in asking that
> question and I believe that is the minimum expectation once the patch
> reaches Ready To Committer stage.
>
>
>>
>> I'm sorry to have caused both of you trouble. I have to admit that I
>> didn't compile the source when I removed the MessageBox()-related changes.
>> The attached patch fixes that. I confirmed successful build this time.
>
> I have tested the attached patch on windows, it works fine both for
> default and non-default cases. It passes other regression tests as well
> and build went fine on Linux.
>
> One more thing about inclusion of new header file in pgevent.c, I think
> that is okay because we include it in other modules (client side) present
> in bin directory like reindex.

Thanks to you both for confirming it works, applied in the current state.

--
Magnus Hagander
Me: http://www.hagander.net/
Work: http://www.redpill-linpro.com/