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


Reply via email to