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

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