Thread: pgsql-server/src backend/main/main.c backend/p ...

CVSROOT:    /cvsroot
Module name:    pgsql-server
Changes by:    momjian@svr1.postgresql.org    04/05/18 17:18:59

Modified files:
    src/backend/main: main.c
    src/backend/postmaster: postmaster.c
    src/backend/tcop: postgres.c
    src/bin/initdb : Makefile
    src/bin/pg_dump: Makefile
    src/bin/psql   : Makefile print.c
    src/interfaces/ecpg/preproc: Makefile
    src/port       : exec.c pipe.c

Log message:
    Move find_my_exec() way up into main.c so it is available to the
    timezone code and other places.

    Remove elog() calls from find_my_exec;  do fprintf(stderr) instead.  We
    can then remove the exec.c handling in the makefile because it doesn't
    have to be built to suppress elog calls.

momjian@svr1.postgresql.org (Bruce Momjian) writes:
>     Remove elog() calls from find_my_exec;  do fprintf(stderr) instead.

Isn't that a seriously bad idea?

I thought that output to stderr generally goes into the bit bucket in
Windows.

            regards, tom lane

Tom Lane wrote:
> momjian@svr1.postgresql.org (Bruce Momjian) writes:
> >     Remove elog() calls from find_my_exec;  do fprintf(stderr) instead.
>
> Isn't that a seriously bad idea?
>
> I thought that output to stderr generally goes into the bit bucket in
> Windows.

Yes, it probably is bad, but I am not sure how frequently this will
happen, _and_ I need to set my_exec_path very far up in the tree to the
timezone stuff knows the location, hence the fact that elog() calls
wouldn't work at all anyway, I think.

--
  Bruce Momjian                        |  http://candle.pha.pa.us
  pgman@candle.pha.pa.us               |  (610) 359-1001
  +  If your life is a hard drive,     |  13 Roberts Road
  +  Christ can be your backup.        |  Newtown Square, Pennsylvania 19073

Tom Lane wrote:
> momjian@svr1.postgresql.org (Bruce Momjian) writes:
> >     Remove elog() calls from find_my_exec;  do fprintf(stderr) instead.
>
> Isn't that a seriously bad idea?
>
> I thought that output to stderr generally goes into the bit bucket in
> Windows.

I have never heard stderr is ignored in Win32.  There was an issue that
buffering of stderr on Win32 wasn't line-buffered like on Unix, but that
is the only issue I know of.

--
  Bruce Momjian                        |  http://candle.pha.pa.us
  pgman@candle.pha.pa.us               |  (610) 359-1001
  +  If your life is a hard drive,     |  13 Roberts Road
  +  Christ can be your backup.        |  Newtown Square, Pennsylvania 19073

Bruce Momjian <pgman@candle.pha.pa.us> writes:
> Yes, it probably is bad, but I am not sure how frequently this will
> happen, _and_ I need to set my_exec_path very far up in the tree to the
> timezone stuff knows the location, hence the fact that elog() calls
> wouldn't work at all anyway, I think.

Perhaps the timezone stuff is what needs to be moved.  I'd be surprised
if there aren't eventually elog calls in src/timezone, so thinking that
timezone must work before elog does seems backwards.

            regards, tom lane

Tom Lane wrote:
> Bruce Momjian <pgman@candle.pha.pa.us> writes:
> > Yes, it probably is bad, but I am not sure how frequently this will
> > happen, _and_ I need to set my_exec_path very far up in the tree to the
> > timezone stuff knows the location, hence the fact that elog() calls
> > wouldn't work at all anyway, I think.
>
> Perhaps the timezone stuff is what needs to be moved.  I'd be surprised
> if there aren't eventually elog calls in src/timezone, so thinking that
> timezone must work before elog does seems backwards.

Magnus says the timezone stuff is being called somewhere he can't find,
perhaps in some underlying library.  Anyway, we can easily switch back
to elog.  It does have to get this info before processing GUC for sure.

--
  Bruce Momjian                        |  http://candle.pha.pa.us
  pgman@candle.pha.pa.us               |  (610) 359-1001
  +  If your life is a hard drive,     |  13 Roberts Road
  +  Christ can be your backup.        |  Newtown Square, Pennsylvania 19073

> > > Yes, it probably is bad, but I am not sure how frequently
> this will
> > > happen, _and_ I need to set my_exec_path very far up in
> the tree to
> > > the timezone stuff knows the location, hence the fact that elog()
> > > calls wouldn't work at all anyway, I think.
> >
> > Perhaps the timezone stuff is what needs to be moved.  I'd be
> > surprised if there aren't eventually elog calls in src/timezone, so
> > thinking that timezone must work before elog does seems backwards.
>
> Magnus says the timezone stuff is being called somewhere he
> can't find, perhaps in some underlying library.  Anyway, we
> can easily switch back to elog.  It does have to get this
> info before processing GUC for sure.

Um. Not quite.
It certainly cannot be called from a *library* function. All our
functions are (in my new patch coming up) prefixed with pg_, so they'er
not called from a library.
But they are called very early at some point. In postmaster.c it worked
fine as it was, but in postgres.c it got called before it had
my_exec_path, and thus failed to find it's own libarry directory.

AFAIK the only ereport() or elog() in the timezone code is in
pg_timezone_initialize() (new function), and that one is called at a
fixed location after we have loaded the config file and thus have elog.

The main issue being that before we load the config file, we don't know
where to put our output, so it's not much point in doing it.

//Magnus

> > >     Remove elog() calls from find_my_exec;  do
> fprintf(stderr) instead.
> >
> > Isn't that a seriously bad idea?
> >
> > I thought that output to stderr generally goes into the bit
> bucket in
> > Windows.
>
> I have never heard stderr is ignored in Win32.  There was an
> issue that buffering of stderr on Win32 wasn't line-buffered
> like on Unix, but that is the only issue I know of.

It is never ignored. WHen running as a service, you will only see it if
ou check the box "allow service to interact with desktop", and this can
only be done on Local System services (thus not postgresql) So in this
case, the end effect will be that you can't see it. But you will most
definitly see it when running it from the commandline (say, when you are
debugging why it won't start..)

//Magnus

"Magnus Hagander" <mha@sollentuna.net> writes:
>>> I thought that output to stderr generally goes into the bit
>>> bucket in Windows.

> It is never ignored. WHen running as a service, you will only see it if
> ou check the box "allow service to interact with desktop", and this can
> only be done on Local System services (thus not postgresql) So in this
> case, the end effect will be that you can't see it. But you will most
> definitly see it when running it from the commandline (say, when you are
> debugging why it won't start..)

I guess the important question is whether it is possible to redirect
stderr into a file on Windows.  If not, I'd say the output-to-stderr
option is pretty useless on that platform.  I certainly don't want
critical messages sent to stderr with no possibility of capture.

            regards, tom lane

> >>> I thought that output to stderr generally goes into the
> bit bucket
> >>> in Windows.
>
> > It is never ignored. WHen running as a service, you will
> only see it
> > if ou check the box "allow service to interact with
> desktop", and this
> > can only be done on Local System services (thus not
> postgresql) So in
> > this case, the end effect will be that you can't see it.
> But you will
> > most definitly see it when running it from the commandline
> (say, when
> > you are debugging why it won't start..)
>
> I guess the important question is whether it is possible to
> redirect stderr into a file on Windows.  If not, I'd say the
> output-to-stderr option is pretty useless on that platform.
> I certainly don't want critical messages sent to stderr with
> no possibility of capture.

It is. From the commandline, not from a service. (You could do it from a
service wrapper, I think, but somebody reported problems with that
earlier)

//Magnus

Bruce Momjian <pgman@candle.pha.pa.us> writes:
> Tom Lane wrote:
>> momjian@svr1.postgresql.org (Bruce Momjian) writes:
>>> Remove elog() calls from find_my_exec;  do fprintf(stderr) instead.
>>
>> Isn't that a seriously bad idea?

> Yes, it probably is bad, but I am not sure how frequently this will
> happen, _and_ I need to set my_exec_path very far up in the tree to the
> timezone stuff knows the location, hence the fact that elog() calls
> wouldn't work at all anyway, I think.

The more I think about this, the less I like it.

find_my_exec and friends are *not* more critical to the backend than
elog is, and should not be done sooner.  The argument that "we have to
find postgresql.conf before elog works" is specious --- elog will work
fine before we have read the config file, indeed had better do so since
guc.c uses elog to report problems.  What will happen is that errors
will get reported to the compiled-in-default location, which happens to
be stderr at the moment.  All you are accomplishing with this change is
to hard-wire that default and make it impossible to change in the
future.

Other concerns: I do not like changing random error reports from elog to
printf, firstly because it will encourage people to code other error
reports as printfs, which is something that took us a huge amount of
work to stamp out, and secondly because the requirement will propagate.
Are you going to avoid using palloc in find_my_exec, too?  What about in
any subroutines that these things call?

I recommend reverting both this change and the ones to do find_my_exec
etc in main.c.  You're better off doing these things where they were
done in PostmasterMain, PostgresMain, etc.  palloc and elog are simply
fundamental parts of the backend programming environment; it does not
make sense to push complex functionality into a part of the code where
they aren't available.

            regards, tom lane