Horton

Re: Allowing printf("%m") only where it actually works

Michael Paquier <michael@paquier.xyz>

View thread View on postgr.es
From: Michael Paquier <michael@paquier.xyz>
To: Tom Lane <tgl@sss.pgh.pa.us>
Cc: Thomas Munro <thomas.munro@enterprisedb.com>, Alvaro Herrera <alvherre@2ndquadrant.com>, PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
Date: 2018-09-25T07:16:36Z
Lists: pgsql-hackers 
On Mon, Sep 24, 2018 at 01:18:47PM -0400, Tom Lane wrote:
> Michael Paquier <michael@paquier.xyz> writes:
>> On Wed, Sep 12, 2018 at 01:40:01PM -0400, Tom Lane wrote:
>>> Rebase attached --- no substantive changes.
> 
>> -       if (handleDLL == NULL)
>> -           ereport(FATAL,
>> -                   (errmsg_internal("could not load netmsg.dll: error
>> -            code %lu", GetLastError())));
> 
>> In 0001, this is replaced by a non-FATAL error for the backend, which
>> does not seem like a good idea to me because the user loses visibility
>> with this DDL which canot be loaded.  I still have to see this error...
> 
> Well, we have to change the code somehow to make it usable in frontend
> as well as backend.  And we can *not* have it do exit(1) in libpq.
> So the solution I chose was to make it act the same as if FormatMessage
> were to fail.  I don't find this behavior unreasonable: what is really
> important is the original error code, not whether we were able to
> pretty-print it.  I think the ereport(FATAL) coding is a pretty darn
> bad idea even in the backend.

Ok.  I won't fight hard on that.  Why changing the error message from
"could not load netmsg.dll" to "unrecognized winsock error" then?  The
original error string is much more verbose to grab the context.

> Seems a bit make-worky, but here you go.  0001 is the same as before
> (but rebased up to today, so some line numbers change).  0002
> changes things so that we always use our snprintf, removing all the
> configure logic associated with that.  0003 implements %m in snprintf.c
> and adjusts our various printf-wrapper functions to ensure that they
> pass errno through reliably.  0004 changes elog.c to rely on %m being
> implemented below it.

Thanks for the new versions.  The only thing I could find to complain
about is the error message above, the rest looks in nice shape.
--
Michael

Commits

  1. In pg_log_generic(), be more paranoid about preserving errno.

  2. Make src/common/exec.c's error logging less ugly.

  3. Select appropriate PG_PRINTF_ATTRIBUTE for recent NetBSD.

  4. Fix detection of the result type of strerror_r().

  5. Try another way to detect the result type of strerror_r().

  6. Clean up *printf macros to avoid conflict with format archetypes.

  7. Fix link failures due to snprintf/strerror changes.

  8. Implement %m in src/port/snprintf.c, and teach elog.c to rely on that.

  9. Always use our own versions of *printf().

  10. Incorporate strerror_r() into src/port/snprintf.c, too.

  11. Convert elog.c's useful_strerror() into a globally-used strerror wrapper.

  12. Revert "Distinguish printf-like functions that support %m from those that don't."

  13. Produce compiler errors if errno is referenced inside elog/ereport calls.

  14. Distinguish printf-like functions that support %m from those that don't.

  15. Fix unportable usage of printf("%m").

  16. Be more robust when strerror() doesn't give a useful result.