Re: fix "Success" error messages - Mailing list pgsql-hackers

On Mon, Aug 26, 2019 at 09:40:23PM +0200, Peter Eisentraut wrote:
> Here is an updated patch set that rearranges this a bit according to
> your suggestions, and also fixes some similar issues in pg_checksums.

Thanks for the new patch, and you are right that pg_checksums has been
slacking here.  There is the same issue with pg_verify_checksums in
11.  Not sure that's worth a back-patch though.  Those parts could
find their way to v12 easily.

> -            ereport(ERROR,
> -                    (errcode_for_file_access(),
> -                     errmsg("could not access status of transaction %u", xid),
> -                     errdetail("Could not read from file \"%s\" at offset %u: %m.",
> -                               path, offset)));
> +            if (errno)
> +                ereport(ERROR,
> +                        (errcode_for_file_access(),
> +                         errmsg("could not access status of transaction %u", xid),
> +                         errdetail("Could not read from file \"%s\" at offset %u: %m.",
> +                                   path, offset)));
> +            else
> +                ereport(ERROR,
> +                        (errmsg("could not access status of transaction %u", xid),
> +                         errdetail("Could not read from file \"%s\" at offset %u: read too few bytes.", path,
offset)));

Last time I worked on that, the following suggestion was made for
error messages with shorter reads or writes:
could not read file \"%s\": read %d of %zu
Still this is clearly an improvement and I that's not worth the extra
complication, so +1 for this way of doing things.

>          if (r == 0)
>              break;
> -        if (r != BLCKSZ)
> +        else if (r < 0)
> +        {
> +            pg_log_error("could not read block %u in file \"%s\": %m",
> +                         blockno, fn);
> +            exit(1);
> +        }
> +        else if (r != BLCKSZ)
>          {
>              pg_log_error("could not read block %u in file \"%s\": read %d of %d",
>                           blockno, fn, r, BLCKSZ);

Other code areas (xlog.c, pg_waldump.c, etc.) prefer doing it this
way, after checking the size read:
if (r != BLCKSZ)
{
    if (r < 0)
        pg_log_error("could not read blah: %m");
    else
        pg_log_error("could not read blah: read %d of %d")
}

>              /* Write block with checksum */
> -            if (write(f, buf.data, BLCKSZ) != BLCKSZ)
> +            w = write(f, buf.data, BLCKSZ);
> +            if (w != BLCKSZ)
>              {
> -                pg_log_error("could not write block %u in file \"%s\": %m",
> -                             blockno, fn);
> +                if (w < 0)
> +                    pg_log_error("could not write block %u in file \"%s\": %m",
> +                                 blockno, fn);
> +                else
> +                    pg_log_error("could not write block %u in file \"%s\": wrote %d of %d",
> +                                 blockno, fn, w, BLCKSZ);
>                  exit(1);
>              }
>          }

This is consistent.
--
Michael