Horton

Thread

Commits

  1. Factor out system call names from error messages

  2. Convert elog(LOG) calls to ereport() where appropriate

  1. convert elog(LOG) calls to ereport

    Peter Eisentraut <peter.eisentraut@enterprisedb.com> — 2020-12-02T13:26:24Z

    There are a number of elog(LOG) calls that appear to be user-facing, so 
they should be ereport()s.  This patch changes them.  There are more 
elog(LOG) calls remaining, but they all appear to be some kind of 
debugging support.  Also, I changed a few elog(FATAL)s that were nearby, 
but I didn't specifically look for them.

  2. Re: convert elog(LOG) calls to ereport

    Alvaro Herrera <alvherre@alvh.no-ip.org> — 2020-12-02T14:04:45Z

    On 2020-Dec-02, Peter Eisentraut wrote:

> There are a number of elog(LOG) calls that appear to be user-facing, so they
> should be ereport()s.  This patch changes them.  There are more elog(LOG)
> calls remaining, but they all appear to be some kind of debugging support.
> Also, I changed a few elog(FATAL)s that were nearby, but I didn't
> specifically look for them.

> -		elog(LOG, "WSAIoctl(SIO_KEEPALIVE_VALS) failed: %ui",
> -			 WSAGetLastError());
> +		ereport(LOG,
> +				(errmsg("WSAIoctl(SIO_KEEPALIVE_VALS) failed: %ui",
> +						WSAGetLastError())));

Please take the opportunity to move the flag name out of the message in
this one, also.  I do wonder if it'd be a good idea to move the syscall
name itself out of the message, too; that would reduce the number of
messages to translate 50x to just "%s(%s) failed: %m" instead of one
message per distinct syscall.

Should fd.c messages do errcode_for_file_access() like elsewhere?

Overall, it looks good to me.

Thanks

  3. Re: convert elog(LOG) calls to ereport

    Michael Paquier <michael@paquier.xyz> — 2020-12-03T07:02:55Z

    On Wed, Dec 02, 2020 at 11:04:45AM -0300, Alvaro Herrera wrote:
> Please take the opportunity to move the flag name out of the message in
> this one, also.  I do wonder if it'd be a good idea to move the syscall
> name itself out of the message, too; that would reduce the number of
> messages to translate 50x to just "%s(%s) failed: %m" instead of one
> message per distinct syscall.

+1.

+   else
+       ereport(LOG,
+               (errmsg("checkpoint starting:%s%s%s%s%s%s%s%s",

Would it be better to add a note for translators here, in short that
all those %s are options related to checkpoint/restartpoints?

The ones in geqo_erx.c look like debugging information, and the ones
in win32_shmem.c for segment creation are code paths only used by the
postmaster.
--
Michael

  4. Re: convert elog(LOG) calls to ereport

    Noah Misch <noah@leadboat.com> — 2020-12-03T07:55:30Z

    On Wed, Dec 02, 2020 at 02:26:24PM +0100, Peter Eisentraut wrote:
> There are a number of elog(LOG) calls that appear to be user-facing, so they
> should be ereport()s.

> @@ -8591,25 +8604,46 @@ LogCheckpointEnd(bool restartpoint)
>  			CheckpointStats.ckpt_sync_rels;
>  	average_msecs = (long) ((average_sync_time + 999) / 1000);
>  
> -	elog(LOG, "%s complete: wrote %d buffers (%.1f%%); "
> -		 "%d WAL file(s) added, %d removed, %d recycled; "
> -		 "write=%ld.%03d s, sync=%ld.%03d s, total=%ld.%03d s; "
> -		 "sync files=%d, longest=%ld.%03d s, average=%ld.%03d s; "
> -		 "distance=%d kB, estimate=%d kB",

+1 to this change.

> @@ -1763,7 +1771,8 @@ pq_setkeepalivesidle(int idle, Port *port)
>  #else
>  	if (idle != 0)
>  	{
> -		elog(LOG, "setting the keepalive idle time is not supported");
> +		ereport(LOG,
> +				(errmsg("setting the keepalive idle time is not supported")));

+1

> --- a/src/backend/optimizer/geqo/geqo_erx.c
> +++ b/src/backend/optimizer/geqo/geqo_erx.c
> @@ -420,7 +420,8 @@ edge_failure(PlannerInfo *root, Gene *gene, int index, Edge *edge_table, int num
>  			}
>  		}
>  
> -		elog(LOG, "no edge found via random decision and total_edges == 4");
> +		ereport(LOG,
> +				(errmsg("no edge found via random decision and total_edges == 4")));

The user can't act upon this without reading the source code.  This and the
other messages proposed in this file are better as elog().

> @@ -343,7 +346,8 @@ PGSharedMemoryCreate(Size size,
>  	 * care.
>  	 */
>  	if (!CloseHandle(hmap))
> -		elog(LOG, "could not close handle to shared memory: error code %lu", GetLastError());
> +		ereport(LOG,
> +				(errmsg("could not close handle to shared memory: error code %lu", GetLastError())));

The numerous messages proposed in src/backend/port/ files are can't-happen
events, and there's little a DBA can do without reading the source code.
They're better as elog().

> @@ -6108,8 +6111,9 @@ backend_read_statsfile(void)
>  				/* Copy because timestamptz_to_str returns a static buffer */
>  				filetime = pstrdup(timestamptz_to_str(file_ts));
>  				mytime = pstrdup(timestamptz_to_str(cur_ts));
> -				elog(LOG, "stats collector's time %s is later than backend local time %s",
> -					 filetime, mytime);
> +				ereport(LOG,
> +						(errmsg("statistics collector's time %s is later than backend local time %s",
> +								filetime, mytime)));

+1

> @@ -769,10 +769,11 @@ StartupReplicationOrigin(void)
>  		replication_states[last_state].remote_lsn = disk_state.remote_lsn;
>  		last_state++;
>  
> -		elog(LOG, "recovered replication state of node %u to %X/%X",
> -			 disk_state.roident,
> -			 (uint32) (disk_state.remote_lsn >> 32),
> -			 (uint32) disk_state.remote_lsn);
> +		ereport(LOG,
> +				(errmsg("recovered replication state of node %u to %X/%X",
> +						disk_state.roident,
> +						(uint32) (disk_state.remote_lsn >> 32),
> +						(uint32) disk_state.remote_lsn)));

+1

> @@ -1914,7 +1914,8 @@ FileClose(File file)
>  
>  		/* in any case do the unlink */
>  		if (unlink(vfdP->fileName))
> -			elog(LOG, "could not unlink file \"%s\": %m", vfdP->fileName);
> +			ereport(LOG,
> +					(errmsg("could not unlink file \"%s\": %m", vfdP->fileName)));

+1

>  
>  		/* and last report the stat results */
>  		if (stat_errno == 0)
> @@ -1922,7 +1923,8 @@ FileClose(File file)
>  		else
>  		{
>  			errno = stat_errno;
> -			elog(LOG, "could not stat file \"%s\": %m", vfdP->fileName);
> +			ereport(LOG,
> +					(errmsg("could not stat file \"%s\": %m", vfdP->fileName)));

+1


For the changes I didn't mention explicitly (most of them), I'm -0.5.  Many of
them "can't happen", use source code terms of art, and/or breach guideline
"Avoid mentioning called function names, either; instead say what the code was
trying to do" (https://www.postgresql.org/docs/devel/error-style-guide.html).

  5. Re: convert elog(LOG) calls to ereport

    Peter Eisentraut <peter.eisentraut@enterprisedb.com> — 2020-12-04T13:34:26Z

    On 2020-12-02 15:04, Alvaro Herrera wrote:
>> -		elog(LOG, "WSAIoctl(SIO_KEEPALIVE_VALS) failed: %ui",
>> -			 WSAGetLastError());
>> +		ereport(LOG,
>> +				(errmsg("WSAIoctl(SIO_KEEPALIVE_VALS) failed: %ui",
>> +						WSAGetLastError())));
> 
> Please take the opportunity to move the flag name out of the message in
> this one, also.

done

> I do wonder if it'd be a good idea to move the syscall
> name itself out of the message, too; that would reduce the number of
> messages to translate 50x to just "%s(%s) failed: %m" instead of one
> message per distinct syscall.

Seems useful, but perhaps as a separate project.

> Should fd.c messages do errcode_for_file_access() like elsewhere?

yes, done

  6. Re: convert elog(LOG) calls to ereport

    Peter Eisentraut <peter.eisentraut@enterprisedb.com> — 2020-12-04T13:34:57Z

    On 2020-12-03 08:02, Michael Paquier wrote:
> +   else
> +       ereport(LOG,
> +               (errmsg("checkpoint starting:%s%s%s%s%s%s%s%s",
> 
> Would it be better to add a note for translators here, in short that
> all those %s are options related to checkpoint/restartpoints?

done

> The ones in geqo_erx.c look like debugging information, and the ones
> in win32_shmem.c for segment creation are code paths only used by the
> postmaster.

I dialed those back a bit.

  7. Re: convert elog(LOG) calls to ereport

    Peter Eisentraut <peter.eisentraut@enterprisedb.com> — 2020-12-04T13:35:48Z

    On 2020-12-03 08:55, Noah Misch wrote:
> For the changes I didn't mention explicitly (most of them), I'm -0.5.  Many of
> them "can't happen", use source code terms of art, and/or breach guideline
> "Avoid mentioning called function names, either; instead say what the code was
> trying to do" (https://www.postgresql.org/docs/devel/error-style-guide.html).

Thanks for the detailed feedback.  I dialed back some of the changes 
based on your feedback.

  8. Re: convert elog(LOG) calls to ereport

    Michael Paquier <michael@paquier.xyz> — 2020-12-05T02:22:12Z

    On Fri, Dec 04, 2020 at 02:34:26PM +0100, Peter Eisentraut wrote:
> On 2020-12-02 15:04, Alvaro Herrera wrote:
>> I do wonder if it'd be a good idea to move the syscall
>> name itself out of the message, too; that would reduce the number of
>> messages to translate 50x to just "%s(%s) failed: %m" instead of one
>> message per distinct syscall.
> 
> Seems useful, but perhaps as a separate project.

-       elog(LOG, "getsockname() failed: %m");
+       ereport(LOG,
+               (errmsg("getsockname() failed: %m")));
FWIW, I disagree with the approach taken by eb93f3a.  As of HEAD, it
is now required to translate all those strings.  I think that it would
have been better to remove the function names from all those error
messages and not require the same pattern to be translated N times.
--
Michael

  9. Re: convert elog(LOG) calls to ereport

    Peter Eisentraut <peter.eisentraut@enterprisedb.com> — 2021-04-23T12:42:06Z

    On 05.12.20 03:22, Michael Paquier wrote:
> On Fri, Dec 04, 2020 at 02:34:26PM +0100, Peter Eisentraut wrote:
>> On 2020-12-02 15:04, Alvaro Herrera wrote:
>>> I do wonder if it'd be a good idea to move the syscall
>>> name itself out of the message, too; that would reduce the number of
>>> messages to translate 50x to just "%s(%s) failed: %m" instead of one
>>> message per distinct syscall.
>>
>> Seems useful, but perhaps as a separate project.
> 
> -       elog(LOG, "getsockname() failed: %m");
> +       ereport(LOG,
> +               (errmsg("getsockname() failed: %m")));
> FWIW, I disagree with the approach taken by eb93f3a.  As of HEAD, it
> is now required to translate all those strings.  I think that it would
> have been better to remove the function names from all those error
> messages and not require the same pattern to be translated N times.

I made another pass across this and implemented the requested change.