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

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

Tom Lane wrote:

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.

Sure, I didn't know elog() sent to stderr before it was initialized.

The following patch uses elog() when linked to the backend, and stderr
directly when used by client code. The old code was really DEBUG2-level
stuff (it showed how it was searching the PATH mostly), but I have
remove those because for clients that is going to go to stderr, so now
it only does an elog(LOG) when something significant happens.

I did have to re-add the separate compile of exec.c to every place that
used it because clients don't have elog, but that's OK.

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.

In the old code, the exec path was only used for dynamic linking for
platforms that needed a full path for dynamic linking. Now the timezone
code and even the $libdir path (used by GUC) is set from my_exec_path,
so it has to be earlier. I can put it much earlier in both postgres and
postmaster, but by having it in main.c, I have it in only one place. It
doesn't do any palloc or anything fancy, because of course it is also
used by client apps.

Patch attached and applied.

-- 
  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:

so it has to be earlier. I can put it much earlier in both postgres and
postmaster, but by having it in main.c, I have it in only one place. It
doesn't do any palloc or anything fancy, because of course it is also
used by client apps.

Patch attached and applied.

And tested? I didn't say that I thought elog would work in main.c.
In fact I'm pretty certain it won't.

regards, tom lane

Tom Lane wrote:

Bruce Momjian <pgman@candle.pha.pa.us> writes:

so it has to be earlier. I can put it much earlier in both postgres and
postmaster, but by having it in main.c, I have it in only one place. It
doesn't do any palloc or anything fancy, because of course it is also
used by client apps.

Patch attached and applied.

And tested? I didn't say that I thought elog would work in main.c.
In fact I'm pretty certain it won't.

Compiles/regression tests pass.

This is why I removed elog in the first place, because I thought it
wouldn't work up higher before the guc load, but you said it would use
the default. Are you saying it only works after guc is loaded? Seems
we have a chicken and egg thing here. I think the guc load needs the
default timezone, and for that we need the exec path so we can find
share.

So, what do you want with the code?

-- 
  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:

Tom Lane wrote:

And tested? I didn't say that I thought elog would work in main.c.
In fact I'm pretty certain it won't.

Compiles/regression tests pass.

You need to test the cases that will produce elog output.

This is why I removed elog in the first place, because I thought it
wouldn't work up higher before the guc load, but you said it would use
the default. Are you saying it only works after guc is loaded?

No, I specifically said it didn't need guc. But it does need palloc,
which means MemoryContextInit() has to run first. I could see running
find_my_exec immediately before InitializeGUCOptions() in PostmasterMain.

regards, tom lane

Tom Lane wrote:

Bruce Momjian <pgman@candle.pha.pa.us> writes:

Tom Lane wrote:

And tested? I didn't say that I thought elog would work in main.c.
In fact I'm pretty certain it won't.

Compiles/regression tests pass.

You need to test the cases that will produce elog output.

This is why I removed elog in the first place, because I thought it
wouldn't work up higher before the guc load, but you said it would use
the default. Are you saying it only works after guc is loaded?

No, I specifically said it didn't need guc. But it does need palloc,
which means MemoryContextInit() has to run first. I could see running
find_my_exec immediately before InitializeGUCOptions() in PostmasterMain.

OK, I moved the find_my_exec calls to the locations you mentioned in
both postgres.c and postmaster.c and remove it from main.c. I tested
the elog and it worked.

-- 
  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:

OK, I moved the find_my_exec calls to the locations you mentioned in
both postgres.c and postmaster.c and remove it from main.c. I tested
the elog and it worked.

You omitted get_pkglib_path() from the postmaster case; was that
intentional?

Also, in the non-exec case it seems unnecessary and possibly unsafe to
repeat this computation in a backend. It should inherit the settings
from the postmaster via fork(), I'd think. So possibly

#ifndef EXEC_BACKEND
if (!IsUnderPostmaster)
#endif
{
find_my_exec(...);
get_pkglib_path(...);
}

regards, tom lane

Tom Lane wrote:

Bruce Momjian <pgman@candle.pha.pa.us> writes:

OK, I moved the find_my_exec calls to the locations you mentioned in
both postgres.c and postmaster.c and remove it from main.c. I tested
the elog and it worked.

You omitted get_pkglib_path() from the postmaster case; was that
intentional?

Got it later. Thanks.

Also, in the non-exec case it seems unnecessary and possibly unsafe to
repeat this computation in a backend. It should inherit the settings
from the postmaster via fork(), I'd think. So possibly

#ifndef EXEC_BACKEND
if (!IsUnderPostmaster)
#endif
{
find_my_exec(...);
get_pkglib_path(...);
}

Thanks. It originally was as below and I forgot about that. It now is:

if (my_exec_path[0] == '\0' && find_my_exec(argv[0], my_exec_path) < 0)

The issue is that fork/exec will need to set it too.

-- 
  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:

Tom Lane wrote:

Also, in the non-exec case it seems unnecessary and possibly unsafe to
repeat this computation in a backend.

Thanks. It originally was as below and I forgot about that. It now is:
if (my_exec_path[0] == '\0' && find_my_exec(argv[0], my_exec_path) < 0)
The issue is that fork/exec will need to set it too.

That fixes it for my_exec_path, but what of the pkglib_path stuff?

regards, tom lane

Tom Lane wrote:

Bruce Momjian <pgman@candle.pha.pa.us> writes:

Tom Lane wrote:

Also, in the non-exec case it seems unnecessary and possibly unsafe to
repeat this computation in a backend.

Thanks. It originally was as below and I forgot about that. It now is:
if (my_exec_path[0] == '\0' && find_my_exec(argv[0], my_exec_path) < 0)
The issue is that fork/exec will need to set it too.

That fixes it for my_exec_path, but what of the pkglib_path stuff?

OK, I will fix that one, though it is a cheap call.

-- 
  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