Re: [HACKERS] Badly designed error reporting code in controldata_utils.c

[Michael Paquier]

On Tue, Mar 8, 2016 at 1:51 PM, Andres Freund  wrote:
> On 2016-03-08 13:45:25 +0900, Michael Paquier wrote:
>> On Mon, Mar 7, 2016 at 10:26 AM, Andres Freund  wrote:
>> > FWIW I'm considering implementing elog/ereport properly for the
>> > frontend.  We've grown several hacks around that not being present, and
>> > I think those by now have a higher aggregate complexity than a proper
>> > implementation would have.
>>
>> That would be handy. And this is are going to need something like
>> callbacks to allow frontend applications how to apply elevel. Take for
>> example pg_rewind, it has an interface with DEBUG and PROGRESS level
>> directly embedded with FATAL controlled by user-defined options.
>
> What does "directly embedded with FATAL" mean?

Incorrect words. I just mean that there is a central code path used by
DEBUG and FATAL in this case, and DEBUG is controlled by a user-side
switch, meaning that if we want to get into something aimed at being
used by any in-core or community frontend clients, we are going to
need something carefully designed.
-- 
Michael


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Badly designed error reporting code in controldata_utils.c

[Andres Freund]

On 2016-03-08 13:45:25 +0900, Michael Paquier wrote:
> On Mon, Mar 7, 2016 at 10:26 AM, Andres Freund  wrote:
> > FWIW I'm considering implementing elog/ereport properly for the
> > frontend.  We've grown several hacks around that not being present, and
> > I think those by now have a higher aggregate complexity than a proper
> > implementation would have.
> 
> That would be handy. And this is are going to need something like
> callbacks to allow frontend applications how to apply elevel. Take for
> example pg_rewind, it has an interface with DEBUG and PROGRESS level
> directly embedded with FATAL controlled by user-defined options.

What does "directly embedded with FATAL" mean?

I don't really want to go further than what our system already is
capable of; once we have that we can look for the next steps.


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Badly designed error reporting code in controldata_utils.c

[Michael Paquier]

On Mon, Mar 7, 2016 at 10:26 AM, Andres Freund  wrote:
> FWIW I'm considering implementing elog/ereport properly for the
> frontend.  We've grown several hacks around that not being present, and
> I think those by now have a higher aggregate complexity than a proper
> implementation would have.

That would be handy. And this is are going to need something like
callbacks to allow frontend applications how to apply elevel. Take for
example pg_rewind, it has an interface with DEBUG and PROGRESS level
directly embedded with FATAL controlled by user-defined options.
-- 
Michael


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Badly designed error reporting code in controldata_utils.c

[Tom Lane]

Joe Conway  writes:
> Committed/pushed with exit(EXIT_FAILURE)

Thanks!  I lit off a new run on gaur/pademelon to confirm.  Should
have results in six hours or so...

regards, tom lane


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Badly designed error reporting code in controldata_utils.c

[Joe Conway]

On 03/07/2016 08:02 AM, Joe Conway wrote:
> On 03/06/2016 07:34 PM, Tom Lane wrote:
>> Joe Conway  writes:
>>> On 03/06/2016 05:47 PM, Tom Lane wrote:
 That's much better, but is there a reason you're using exit(2)
 and not exit(EXIT_FAILURE) ?
>>
>>> Only because I was trying to stick with what was originally in
>>> src/bin/pg_controldata/pg_controldata.c
>>
>> Meh.  It looks to me like the standard way to handle this
>> for code in src/common/ is exit(EXIT_FAILURE).
> 
> I have no issue with using EXIT_FAILURE, but note:

Committed/pushed with exit(EXIT_FAILURE)

Joe

-- 
Crunchy Data - http://crunchydata.com
PostgreSQL Support for Secure Enterprises
Consulting, Training, & Open Source Development



signature.asc
Description: OpenPGP digital signature

Re: [HACKERS] Badly designed error reporting code in controldata_utils.c

[Joe Conway]

On 03/06/2016 07:34 PM, Tom Lane wrote:
> Joe Conway  writes:
>> On 03/06/2016 05:47 PM, Tom Lane wrote:
>>> That's much better, but is there a reason you're using exit(2)
>>> and not exit(EXIT_FAILURE) ?
> 
>> Only because I was trying to stick with what was originally in
>> src/bin/pg_controldata/pg_controldata.c
> 
> Meh.  It looks to me like the standard way to handle this
> for code in src/common/ is exit(EXIT_FAILURE).

I have no issue with using EXIT_FAILURE, but note:

1) That will change the exit error from the previous value of 2 to 1 for
   pg_controldata

2) There are many examples in other parts of the source that do not use
   EXIT_FAILURE, and even in src/common:

8<-
grep -rnE "exit\(EXIT_FAILURE\)" src/common/* --include=*.c
src/common/fe_memutils.c:36:exit(EXIT_FAILURE);
src/common/fe_memutils.c:76:exit(EXIT_FAILURE);
src/common/fe_memutils.c:93:exit(EXIT_FAILURE);
src/common/fe_memutils.c:99:exit(EXIT_FAILURE);
src/common/psprintf.c:135:  exit(EXIT_FAILURE);
src/common/psprintf.c:182:  exit(EXIT_FAILURE);

grep -rnE "exit\((1|2)\)" src/common/* --include=*.c
src/common/restricted_token.c:187:  exit(1);
src/common/username.c:86:   exit(1);
8<-

Joe

-- 
Crunchy Data - http://crunchydata.com
PostgreSQL Support for Secure Enterprises
Consulting, Training, & Open Source Development



signature.asc
Description: OpenPGP digital signature

Re: [HACKERS] Badly designed error reporting code in controldata_utils.c

[Tom Lane]

Joe Conway  writes:
> On 03/06/2016 05:47 PM, Tom Lane wrote:
>> That's much better, but is there a reason you're using exit(2)
>> and not exit(EXIT_FAILURE) ?

> Only because I was trying to stick with what was originally in
> src/bin/pg_controldata/pg_controldata.c

Meh.  It looks to me like the standard way to handle this
for code in src/common/ is exit(EXIT_FAILURE).

regards, tom lane


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Badly designed error reporting code in controldata_utils.c

[Joe Conway]

On 03/06/2016 05:47 PM, Tom Lane wrote:
> Joe Conway  writes:
>> Something like the attached what you had in mind?
> 
> That's much better, but is there a reason you're using exit(2)
> and not exit(EXIT_FAILURE) ?

Only because I was trying to stick with what was originally in
src/bin/pg_controldata/pg_controldata.c

Joe

-- 
Crunchy Data - http://crunchydata.com
PostgreSQL Support for Secure Enterprises
Consulting, Training, & Open Source Development



signature.asc
Description: OpenPGP digital signature

Re: [HACKERS] Badly designed error reporting code in controldata_utils.c

[Tom Lane]

Joe Conway  writes:
> Something like the attached what you had in mind?

That's much better, but is there a reason you're using exit(2)
and not exit(EXIT_FAILURE) ?

regards, tom lane


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Badly designed error reporting code in controldata_utils.c

[Andres Freund]

On 2016-03-06 17:16:42 -0800, Joe Conway wrote:
> - #ifndef FRONTEND
> - /* NOTE: caller must provide gettext call around the format string */
> - #define log_error(...)  \
> - elog(ERROR, __VA_ARGS__)
> - #else
> - #define log_error(...)  \
> - do { \
> - char *buf = psprintf(__VA_ARGS__); \
> - fprintf(stderr, "%s: %s\n", progname, buf); \
> - exit(2); \
> - } while (0)
> - #endif
> -

FWIW I'm considering implementing elog/ereport properly for the
frontend.  We've grown several hacks around that not being present, and
I think those by now have a higher aggregate complexity than a proper
implementation would have.

Greetings,

Andres Freund


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Badly designed error reporting code in controldata_utils.c

[Joe Conway]

On 03/06/2016 01:26 PM, Joe Conway wrote:
> On 03/06/2016 01:24 PM, Tom Lane wrote:
>> * It's randomly unlike every single other place we've addressed the
>> same problem.  Everywhere else in src/common does it like this:

[...snip...]

>> and I think that's what this needs to do too, especially in view of the
>> fact that there are only two places that would have to be fixed anyway.
> 
> Ok, will fix.

Something like the attached what you had in mind?

Thanks,

Joe

-- 
Crunchy Data - http://crunchydata.com
PostgreSQL Support for Secure Enterprises
Consulting, Training, & Open Source Development
diff --git a/src/common/controldata_utils.c b/src/common/controldata_utils.c
index b6d0a12..65f2cb9 100644
*** a/src/common/controldata_utils.c
--- b/src/common/controldata_utils.c
***
*** 28,46 
  #include "common/controldata_utils.h"
  #include "port/pg_crc32c.h"
  
- #ifndef FRONTEND
- /* NOTE: caller must provide gettext call around the format string */
- #define log_error(...)	\
- 	elog(ERROR, __VA_ARGS__)
- #else
- #define log_error(...)	\
- 	do { \
- 			char *buf = psprintf(__VA_ARGS__); \
- 			fprintf(stderr, "%s: %s\n", progname, buf); \
- 			exit(2); \
- 	} while (0)
- #endif
- 
  /*
   * get_controlfile(char *DataDir, const char *progname)
   *
--- 28,33 
*** get_controlfile(char *DataDir, const cha
*** 59,70 
  	snprintf(ControlFilePath, MAXPGPATH, "%s/global/pg_control", DataDir);
  
  	if ((fd = open(ControlFilePath, O_RDONLY | PG_BINARY, 0)) == -1)
! 		log_error(_("could not open file \"%s\" for reading: %s"),
!   ControlFilePath, strerror(errno));
  
  	if (read(fd, ControlFile, sizeof(ControlFileData)) != sizeof(ControlFileData))
! 		log_error(_("could not read file \"%s\": %s"),
!   ControlFilePath, strerror(errno));
  
  	close(fd);
  
--- 46,76 
  	snprintf(ControlFilePath, MAXPGPATH, "%s/global/pg_control", DataDir);
  
  	if ((fd = open(ControlFilePath, O_RDONLY | PG_BINARY, 0)) == -1)
! #ifndef FRONTEND
! 		ereport(ERROR,
! (errcode_for_file_access(),
! errmsg("could not open file \"%s\" for reading: %m",
! 	   ControlFilePath)));
! #else
! 	{
! 		fprintf(stderr, _("%s: could not open file \"%s\" for reading: %s\n"),
! progname, ControlFilePath, strerror(errno));
! 		exit(2);
! 	}
! #endif
  
  	if (read(fd, ControlFile, sizeof(ControlFileData)) != sizeof(ControlFileData))
! #ifndef FRONTEND
! 		ereport(ERROR,
! (errcode_for_file_access(),
! errmsg("could not read file \"%s\": %m", ControlFilePath)));
! #else
! 	{
! 		fprintf(stderr, _("%s: could not read file \"%s\": %s\n"),
! progname, ControlFilePath, strerror(errno));
! 		exit(2);
! 	}
! #endif
  
  	close(fd);
  


signature.asc
Description: OpenPGP digital signature

Re: [HACKERS] Badly designed error reporting code in controldata_utils.c

[Joe Conway]

On 03/06/2016 01:24 PM, Tom Lane wrote:
> * It's randomly unlike every single other place we've addressed the
> same problem.  Everywhere else in src/common does it like this:
> 
> #ifndef FRONTEND
> ereport(ERROR,
> (errcode(ERRCODE_PROGRAM_LIMIT_EXCEEDED),
>  errmsg("out of memory")));
> #else
> fprintf(stderr, _("out of memory\n"));
> exit(EXIT_FAILURE);
> #endif
> 
> and I think that's what this needs to do too, especially in view of the
> fact that there are only two places that would have to be fixed anyway.

Ok, will fix.

Joe

-- 
Crunchy Data - http://crunchydata.com
PostgreSQL Support for Secure Enterprises
Consulting, Training, & Open Source Development



signature.asc
Description: OpenPGP digital signature

[HACKERS] Badly designed error reporting code in controldata_utils.c

[Tom Lane]

My attention was drawn to the log_error() stuff in controldata_utils.c by
the fact that buildfarm member pademelon spit up on it.  The reason for
that compile failure is that pademelon's dinosaur of a compiler doesn't
support __VA_ARGS__.  I do not feel a need to get into a discussion about
whether we should move our portability goalposts for the convenience of
this commit, because there are other reasons why this is a crummy solution
for error reporting:

* It uses elog() not ereport() for what seems a not-particularly-internal
error, which among other things means that an entirely inappropriate
errcode() will be reported.

* It relies on strerror(errno), not %m, which may not work reliably even
in elog() and certainly won't in ereport() (because of order-of-evaluation
uncertainties).

* Translatability of the error message in the frontend context seems
a bit dubious; generally we let translators work with the whole string
to be printed, not just part of it.

* It's randomly unlike every single other place we've addressed the
same problem.  Everywhere else in src/common does it like this:

#ifndef FRONTEND
ereport(ERROR,
(errcode(ERRCODE_PROGRAM_LIMIT_EXCEEDED),
 errmsg("out of memory")));
#else
fprintf(stderr, _("out of memory\n"));
exit(EXIT_FAILURE);
#endif

and I think that's what this needs to do too, especially in view of the
fact that there are only two places that would have to be fixed anyway.

regards, tom lane


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers