Re: Fix handling of copy_file_range() return value

On 22.06.26 10:46, Yingying Chen wrote:
>
>
> On Mon, Jun 22, 2026 at 3:20 PM Peter Eisentraut <peter(at)eisentraut(dot)org
> <mailto:peter(at)eisentraut(dot)org>> wrote:
>
> While checking return/error handling of file system calls, I found that
> the copy_file_range() call in pg_combinebackup has a potential problem.
> If copy_file_range() returns 0, which is a documented condition, then
> the loop never makes progress and could spin forever.
>
> The other uses of copy_file_range() in the tree are surrounded by
> different logic and don't appear to have this problem.
>
> My suggested fix is to make a return value of 0 an error.  It most
> likely indicates that the source file has an unexpected size.
>
> Hi, Peter,
>
> Thanks for the patch. I agree wb==0 should be treated as an error, so
> the fix direction looks good to me.

Thanks, I have committed the patch and backpatched it.

> I have a small comment:
> ====
>         wb = copy_file_range(s->fd, &off, wfd, NULL, BLCKSZ - nwritten, 0);
>
>         if (wb < 0)
>                 pg_fatal("error while copying file range from \"%s\" to
> \"%s\": %m",
>                                  s->filename, output_filename);
>         else if (wb == 0)
>                 pg_fatal("unexpected end of file while copying file
> range from \"%s\" to \"%s\"",
>                                  s->filename, output_filename);
>
>
> As copy_file_range copies from s->fd, should pg_fatal just s->filename
> in the error message? In that case, input_filename is no longer used in
> write_reconstructed_file(), then it might be removed from the argument list.

Yes, interesting. I notice that the input_filename function argument
wasn't used in the first commit dc212340058. Only later when the
copy_file_range() support was added (commit ac811015513), it became
used. Maybe input_filename is fully redundant with s->filename, but
this isn't very thoroughly documented, so I'm not sure.