Re: gettext calls in pgport

As has previously been pointed out, the strings marked up for gettext
translation in the pgport library don't work and need to be moved back
to where they once came from, unless someone wants to add gettext and
locale setup in pgport. (That might be silly, because in theory locale
and gettext functions could one day become part of pgport.) I claim
that pgport is a low-layer library that works on the operating system
level and should communicate with its callers via error codes.

--
Peter Eisentraut
http://developer.postgresql.org/~petere/

Peter Eisentraut wrote:
> As has previously been pointed out, the strings marked up for gettext
> translation in the pgport library don't work and need to be moved back
> to where they once came from, unless someone wants to add gettext and
> locale setup in pgport. (That might be silly, because in theory locale
> and gettext functions could one day become part of pgport.) I claim
> that pgport is a low-layer library that works on the operating system
> level and should communicate with its callers via error codes.

Error codes seem like a lot more work than it is worth. I vote for
adding gettext support to /port. Also adding error codes duplicates all
the error strings in the call sites.

Added to open items list:

* Add gettext support to src/port

--
Bruce Momjian | http://candle.pha.pa.us
pgman(at)candle(dot)pha(dot)pa(dot)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(at)candle(dot)pha(dot)pa(dot)us> writes:
> Error codes seem like a lot more work than it is worth. I vote for
> adding gettext support to /port. Also adding error codes duplicates all
> the error strings in the call sites.

> Added to open items list:
> * Add gettext support to src/port

He who controls the TODO list dictates the solutions, eh?

I tend to agree with Peter, actually: it would be better to pull error
reporting issues out of pgport. Somebody just yesterday stuck an
"fprintf(stderr,...); exit(1)" into one of the pgport routines. This
sucks, but there is not a lot else that can be done if the code needs
to exist in both backend and clients. It'd be better to propagate the
error condition back to the caller.

An alternative possibility is to stop pretending that pgport is agnostic
about whether it is in backend or frontend. This might mean some
duplication of code between src/port/ and src/backend/port/, but if
that's what it takes to have sane error handling, that's what we should do.

regards, tom lane

Tom Lane wrote:
> Bruce Momjian <pgman(at)candle(dot)pha(dot)pa(dot)us> writes:
> > Error codes seem like a lot more work than it is worth. I vote for
> > adding gettext support to /port. Also adding error codes duplicates all
> > the error strings in the call sites.
>
> > Added to open items list:
> > * Add gettext support to src/port
>
> He who controls the TODO list dictates the solutions, eh?

And I can update it too. New text is:

* fix gettext support to src/port

We need to "fix" it somehow.

> I tend to agree with Peter, actually: it would be better to pull error
> reporting issues out of pgport. Somebody just yesterday stuck an
> "fprintf(stderr,...); exit(1)" into one of the pgport routines. This
> sucks, but there is not a lot else that can be done if the code needs
> to exist in both backend and clients. It'd be better to propagate the
> error condition back to the caller.

I have no problem propogating the error condition back to the caller,
but to propogate the error code/reason back means you would need -1 ==
"no file", -2 == "permission violation", etc and you then have those
strings back in the client. That was Peter's goal, I thought, but it
goes against the idea of centralizing those strings. I suppose you
could #define all the needed strings in an include file and use those
defines when checking for the error return code, but you have to make
sure you get them all right and adjust for new strings, and it doesn't
seem worth it.

In fact one interesting idea would be to pass a constant error string
back to the caller to fprintf/elog if they want, but that doesn't get
the strings out of /port, and it doesn't allow easy use of passing
fprintf strings with arguments back to the caller.

> An alternative possibility is to stop pretending that pgport is agnostic
> about whether it is in backend or frontend. This might mean some
> duplication of code between src/port/ and src/backend/port/, but if
> that's what it takes to have sane error handling, that's what we should do.

I tried that but the fix caused more uglyness than it prevented.

--
Bruce Momjian | http://candle.pha.pa.us
pgman(at)candle(dot)pha(dot)pa(dot)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:

>Somebody just yesterday stuck an
>"fprintf(stderr,...); exit(1)" into one of the pgport routines. This
>sucks, but there is not a lot else that can be done if the code needs
>to exist in both backend and clients. It'd be better to propagate the
>error condition back to the caller.
>
>An alternative possibility is to stop pretending that pgport is agnostic
>about whether it is in backend or frontend. This might mean some
>duplication of code between src/port/ and src/backend/port/, but if
>that's what it takes to have sane error handling, that's what we should do.
>
>
>
>

Maybe you're referring to the patch I sent in to strip the .exe suffix
in get_progname? ;-)

I wondered about that. The choices on strdup() error seemed to be:

. ignore the error and return the unstripped path, knowing the program
would fail in a minute on the next malloc call anyway
. return NULL and patch the code in about 20 places (of which one is the
backend) where get_progname() is called
. print a message and exit

I can see arguments for all of these ;-)

cheers

andrew

Andrew Dunstan <andrew(at)dunslane(dot)net> writes:
> Tom Lane wrote:
>> Somebody just yesterday stuck an
>> "fprintf(stderr,...); exit(1)" into one of the pgport routines. This
>> sucks, but there is not a lot else that can be done if the code needs
>> to exist in both backend and clients. It'd be better to propagate the
>> error condition back to the caller.

> Maybe you're referring to the patch I sent in to strip the .exe suffix
> in get_progname? ;-)

Yeah, that was it.

> I wondered about that. The choices on strdup() error seemed to be:

> . ignore the error and return the unstripped path, knowing the program
> would fail in a minute on the next malloc call anyway
> . return NULL and patch the code in about 20 places (of which one is the
> backend) where get_progname() is called
> . print a message and exit

Given the limited uses of get_progname, I think that "print a message
and exit" is fine; the problem is that the correct implementation of
that differs between backend and clients. The only really correct way
to log a message in the backend is elog/ereport --- there's no guarantee
that stderr connects to anything but /dev/null. Going directly to
exit() instead of proc_exit() is simply broken (although perhaps the
distinction does not matter, since the postmaster will treat exit(1) as
a backend crash and force a database-wide reset). If I thought that
this code path was ever likely to actually be taken in the field, I'd be
hollering much more loudly about it.

regards, tom lane

Am Montag, 18. Oktober 2004 19:43 schrieb Tom Lane:
> An alternative possibility is to stop pretending that pgport is agnostic
> about whether it is in backend or frontend. This might mean some
> duplication of code between src/port/ and src/backend/port/, but if
> that's what it takes to have sane error handling, that's what we should do.

The original plan for libpgport was to be a repository of functions that
replace missing operating system functionality, like libiberty. I would have
have liked to be able to lift these functions into other projects without
complications. That implies that these functions should certainly not care
about anything that by definition goes on above the operating system level.

Now the directory has grown into a sort of general repository of code that is
shared between more than one part of the PostgreSQL source tree, without any
regard for well-defined interfaces. If you need to do that, please put it
elsewhere, where only the involved parts see it. Not now, but in the future.

--
Peter Eisentraut
http://developer.postgresql.org/~petere/

Peter Eisentraut wrote:
> Am Montag, 18. Oktober 2004 19:43 schrieb Tom Lane:
> > An alternative possibility is to stop pretending that pgport is agnostic
> > about whether it is in backend or frontend. This might mean some
> > duplication of code between src/port/ and src/backend/port/, but if
> > that's what it takes to have sane error handling, that's what we should do.
>
> The original plan for libpgport was to be a repository of functions that
> replace missing operating system functionality, like libiberty. I would have
> have liked to be able to lift these functions into other projects without
> complications. That implies that these functions should certainly not care
> about anything that by definition goes on above the operating system level.
>
> Now the directory has grown into a sort of general repository of code that is
> shared between more than one part of the PostgreSQL source tree, without any
> regard for well-defined interfaces. If you need to do that, please put it
> elsewhere, where only the involved parts see it. Not now, but in the future.

So we will have a pgport and a pgshare? I don't see a huge value in
that. Ideally, yea, they are different concepts but practially it seems
like a waste to make the distinction.

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

Andrew Dunstan wrote:
>
>
> Tom Lane wrote:
>
> >Somebody just yesterday stuck an
> >"fprintf(stderr,...); exit(1)" into one of the pgport routines. This
> >sucks, but there is not a lot else that can be done if the code needs
> >to exist in both backend and clients. It'd be better to propagate the
> >error condition back to the caller.
> >
> >An alternative possibility is to stop pretending that pgport is agnostic
> >about whether it is in backend or frontend. This might mean some
> >duplication of code between src/port/ and src/backend/port/, but if
> >that's what it takes to have sane error handling, that's what we should do.
> >
> >
> >
> >
>
> Maybe you're referring to the patch I sent in to strip the .exe suffix
> in get_progname? ;-)
>
> I wondered about that. The choices on strdup() error seemed to be:
>
> . ignore the error and return the unstripped path, knowing the program
> would fail in a minute on the next malloc call anyway
> . return NULL and patch the code in about 20 places (of which one is the
> backend) where get_progname() is called
> . print a message and exit
>
> I can see arguments for all of these ;-)

I added a comment to the exit() call:

exit(1); /* This could exit the postmaster */

This clearly marks that this could be a postmaster issue someday.

--
Bruce Momjian | http://candle.pha.pa.us
pgman(at)candle(dot)pha(dot)pa(dot)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:
> Andrew Dunstan <andrew(at)dunslane(dot)net> writes:
> > Tom Lane wrote:
> >> Somebody just yesterday stuck an
> >> "fprintf(stderr,...); exit(1)" into one of the pgport routines. This
> >> sucks, but there is not a lot else that can be done if the code needs
> >> to exist in both backend and clients. It'd be better to propagate the
> >> error condition back to the caller.
>
> > Maybe you're referring to the patch I sent in to strip the .exe suffix
> > in get_progname? ;-)
>
> Yeah, that was it.
>
> > I wondered about that. The choices on strdup() error seemed to be:
>
> > . ignore the error and return the unstripped path, knowing the program
> > would fail in a minute on the next malloc call anyway
> > . return NULL and patch the code in about 20 places (of which one is the
> > backend) where get_progname() is called
> > . print a message and exit
>
> Given the limited uses of get_progname, I think that "print a message
> and exit" is fine; the problem is that the correct implementation of
> that differs between backend and clients. The only really correct way
> to log a message in the backend is elog/ereport --- there's no guarantee
> that stderr connects to anything but /dev/null. Going directly to
> exit() instead of proc_exit() is simply broken (although perhaps the
> distinction does not matter, since the postmaster will treat exit(1) as
> a backend crash and force a database-wide reset). If I thought that
> this code path was ever likely to actually be taken in the field, I'd be
> hollering much more loudly about it.

Actually the backend never calls get_progname(), only the postmaster
does. I added a comment in path.c and postmaster.c about the possible
call to exit. I think this cleans it up as well as possible.

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