Horton

Thread

Commits

  1. Fix handling of copy_file_range() return value

  2. Allow using copy_file_range in write_reconstructed_file

  3. Add support for incremental backup.

  1. Fix handling of copy_file_range() return value

    Peter Eisentraut <peter@eisentraut.org> — 2026-06-22T07:19:59Z

    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.

  2. Re: Fix handling of copy_file_range() return value

    Nazir Bilal Yavuz <byavuz81@gmail.com> — 2026-06-22T08:17:44Z

    Hi,

On Mon, 22 Jun 2026 at 10:20, Peter Eisentraut <peter@eisentraut.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.

You are right, that is a problem only with this use of
copy_file_range(), and your patch fixes it; LGTM.

-- 
Regards,
Nazir Bilal Yavuz
Microsoft

  3. Re: Fix handling of copy_file_range() return value

    Kyotaro Horiguchi <horikyota.ntt@gmail.com> — 2026-06-22T08:34:53Z

    At Mon, 22 Jun 2026 09:19:59 +0200, Peter Eisentraut <peter@eisentraut.org> wrote in 
> 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.

Good catch. I agree with the analysis, and the proposed fix looks
reasonable to me.

I also checked the other four uses of copy_file_range() in the
tree. All but one handle a zero return value as EOF, and the remaining
case (check_copy_file_range()) appears safe because it is only testing
whether copy_file_range() is usable.

Regards,

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

  4. Re: Fix handling of copy_file_range() return value

    Yingying Chen <cyy9255@gmail.com> — 2026-06-22T08:46:45Z

    On Mon, Jun 22, 2026 at 3:20 PM Peter Eisentraut <peter@eisentraut.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.

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.

Regards,
Yingying Chen

  5. Re: Fix handling of copy_file_range() return value

    Peter Eisentraut <peter@eisentraut.org> — 2026-06-29T11:12:51Z

    On 22.06.26 10:46, Yingying Chen wrote:
> 
> 
> On Mon, Jun 22, 2026 at 3:20 PM Peter Eisentraut <peter@eisentraut.org 
> <mailto:peter@eisentraut.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.