Horton

Thread

Commits

  1. Clear base backup progress on backup failure

  1. Clear base backup progress reporting on error

    Chao Li <li.evan.chao@gmail.com> — 2026-06-26T08:58:50Z

    Hi,

While testing “[deb674454] Add backup_type column to pg_stat_progress_basebackup”, I didn’t find anything wrong with the feature itself, but I noticed a suspicious existing behavior of pg_stat_progress_basebackup. After a BASE_BACKUPcommand fails, pg_stat_progress_basebackup can still show the failed session.

See this simple repro:

1. Start a server

2. Under the data directory, create a unreadable file, that will cause base backup to fail:
```
% cd <data-dir>
% mkdir data
% touch data/zzz_unreadable
% chmod 000 data/zzz_unreadable
```

3. In psql, check pg_stat_progress_basebackup. It shows nothing:
```
evantest=# SELECT pid, phase, backup_type, backup_streamed > 0 AS streamed FROM pg_stat_progress_basebackup;
 pid | phase | backup_type | streamed
-----+-------+-------------+----------
(0 rows)
```

4. Start a walsender, issue a BASE_BACKUP command, and the command should fail:
```
% psql 'dbname=postgres replication=database’
postgres=# BASE_BACKUP (checkpoint 'fast’);
<… omit some output …>
ERROR:  could not open file "./data/zzz_unreadable": Permission denied
```

5. In psql, check pg_stat_progress_basebackup again:
```
evantest=# SELECT pid, phase, backup_type, backup_streamed > 0 AS streamed FROM pg_stat_progress_basebackup;
  pid  |          phase           | backup_type | streamed
-------+--------------------------+-------------+----------
 50156 | streaming database files | full        | t
(1 row)
```

It still shows the failed base backup session with phase “streaming database files”, which seems wrong.

After debugging, I think the problem is:

* bbsink_progress_new calls pgstat_progress_start_command.
* pgstat_progress_end_command is called by basebackup_progress_done, and basebackup_progress_done is only called by perform_base_backup after a backup succeeds.
* If a backup fails, the FINALLY clause calls bbsink_cleanup, which calls sink->bbs_ops->cleanup(sink). This eventually calls bbsink_forward_cleanup, so in the failure path, pgstat_progress_end_command is not called.

This patch makes the following changes:

* Add a new cleanup helper that calls pgstat_progress_end_command and bbsink_forward_cleanup.
* Change bbsink_progress_ops’s cleanup callback to point to the new helper.
* Remove the call to basebackup_progress_done from perform_base_backup.
* Since basebackup_progress_done has no other caller, remove it.

With this change, pgstat_progress_start_command and pgstat_progress_end_command are called in paired new/cleanup paths, which looks more consistent.

I don’t think this is a serious bug, because with the usual pg_basebackup command path, the client normally disconnects after the error, so the stale progress entry is not observable. It is mainly visible when the same replication connection stays open after the failed BASE_BACKUP command. So I feel it might not be worth adding a TAP test. But if others think differently, I will be happy to add one.

Best regards,
--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/

  2. Re: Clear base backup progress reporting on error

    Fujii Masao <masao.fujii@gmail.com> — 2026-06-26T10:46:45Z

    On Fri, Jun 26, 2026 at 5:59 PM Chao Li <li.evan.chao@gmail.com> wrote:
> This patch makes the following changes:

Thanks for the report and patch!

> I don’t think this is a serious bug, because with the usual pg_basebackup command path, the client normally disconnects after the error, so the stale progress entry is not observable. It is mainly visible when the same replication connection stays open after the failed BASE_BACKUP command. So I feel it might not be worth adding a TAP test.

+1

You may think this isn't worth backpatching to the stable branches
for the same reason?

Regards,

-- 
Fujii Masao

  3. Re: Clear base backup progress reporting on error

    Chao Li <li.evan.chao@gmail.com> — 2026-06-26T11:08:17Z

    
> On Jun 26, 2026, at 18:46, Fujii Masao <masao.fujii@gmail.com> wrote:
> 
> On Fri, Jun 26, 2026 at 5:59 PM Chao Li <li.evan.chao@gmail.com> wrote:
>> This patch makes the following changes:
> 
> Thanks for the report and patch!
> 
>> I don’t think this is a serious bug, because with the usual pg_basebackup command path, the client normally disconnects after the error, so the stale progress entry is not observable. It is mainly visible when the same replication connection stays open after the failed BASE_BACKUP command. So I feel it might not be worth adding a TAP test.
> 
> +1
> 
> You may think this isn't worth backpatching to the stable branches
> for the same reason?
> 

I personally don’t feel back-patch is needed. 

Best regards,
--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/

  4. Re: Clear base backup progress reporting on error

    Fujii Masao <masao.fujii@gmail.com> — 2026-07-01T14:17:04Z

    On Fri, Jun 26, 2026 at 8:08 PM Chao Li <li.evan.chao@gmail.com> wrote:
>
>
>
> > On Jun 26, 2026, at 18:46, Fujii Masao <masao.fujii@gmail.com> wrote:
> >
> > On Fri, Jun 26, 2026 at 5:59 PM Chao Li <li.evan.chao@gmail.com> wrote:
> >> This patch makes the following changes:
> >
> > Thanks for the report and patch!
> >
> >> I don’t think this is a serious bug, because with the usual pg_basebackup command path, the client normally disconnects after the error, so the stale progress entry is not observable. It is mainly visible when the same replication connection stays open after the failed BASE_BACKUP command. So I feel it might not be worth adding a TAP test.
> >
> > +1
> >
> > You may think this isn't worth backpatching to the stable branches
> > for the same reason?
> >
>
> I personally don’t feel back-patch is needed.

I've pushed the patch. Thanks!

I backpatched it to v15 but skipped v14 because v14 lacks the bbsink
infrastructure, so the fix does not apply cleanly there. Also, the bug
does not affect the backup itself and is normally not observable
when using pg_basebackup.

Regards,

-- 
Fujii Masao

  5. Re: Clear base backup progress reporting on error

    Chao Li <li.evan.chao@gmail.com> — 2026-07-01T23:45:34Z

    
> On Jul 1, 2026, at 22:17, Fujii Masao <masao.fujii@gmail.com> wrote:
> 
> On Fri, Jun 26, 2026 at 8:08 PM Chao Li <li.evan.chao@gmail.com> wrote:
>> 
>> 
>> 
>>> On Jun 26, 2026, at 18:46, Fujii Masao <masao.fujii@gmail.com> wrote:
>>> 
>>> On Fri, Jun 26, 2026 at 5:59 PM Chao Li <li.evan.chao@gmail.com> wrote:
>>>> This patch makes the following changes:
>>> 
>>> Thanks for the report and patch!
>>> 
>>>> I don’t think this is a serious bug, because with the usual pg_basebackup command path, the client normally disconnects after the error, so the stale progress entry is not observable. It is mainly visible when the same replication connection stays open after the failed BASE_BACKUP command. So I feel it might not be worth adding a TAP test.
>>> 
>>> +1
>>> 
>>> You may think this isn't worth backpatching to the stable branches
>>> for the same reason?
>>> 
>> 
>> I personally don’t feel back-patch is needed.
> 
> I've pushed the patch. Thanks!
> 
> I backpatched it to v15 but skipped v14 because v14 lacks the bbsink
> infrastructure, so the fix does not apply cleanly there. Also, the bug
> does not affect the backup itself and is normally not observable
> when using pg_basebackup.
> 
> Regards,
> 
> -- 
> Fujii Masao

Thanks for pushing.

Best regards,
--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/

  6. Re: Clear base backup progress reporting on error

    Andrew Dunstan <andrew@dunslane.net> — 2026-07-04T14:16:52Z

    On Wed, Jul 1, 2026 at 10:17 AM Fujii Masao <masao.fujii@gmail.com> wrote:

> On Fri, Jun 26, 2026 at 8:08 PM Chao Li <li.evan.chao@gmail.com> wrote:
> >
> >
> >
> > > On Jun 26, 2026, at 18:46, Fujii Masao <masao.fujii@gmail.com> wrote:
> > >
> > > On Fri, Jun 26, 2026 at 5:59 PM Chao Li <li.evan.chao@gmail.com>
> wrote:
> > >> This patch makes the following changes:
> > >
> > > Thanks for the report and patch!
> > >
> > >> I don’t think this is a serious bug, because with the usual
> pg_basebackup command path, the client normally disconnects after the
> error, so the stale progress entry is not observable. It is mainly visible
> when the same replication connection stays open after the failed
> BASE_BACKUP command. So I feel it might not be worth adding a TAP test.
> > >
> > > +1
> > >
> > > You may think this isn't worth backpatching to the stable branches
> > > for the same reason?
> > >
> >
> > I personally don’t feel back-patch is needed.
>
> I've pushed the patch. Thanks!
>
> I backpatched it to v15 but skipped v14 because v14 lacks the bbsink
> infrastructure, so the fix does not apply cleanly there. Also, the bug
> does not affect the backup itself and is normally not observable
> when using pg_basebackup.
>
>
>
This has upset the ABI compliance checker (see <
https://buildfarm.postgresql.org/cgi-bin/show_failures.pl?max_days=3&stage=abi-compliance-check&filter=Submit>
. Looks like you need to update the  .abi-compliance-history file on the
affected back branches.

cheers

andrew

  7. Re: Clear base backup progress reporting on error

    Fujii Masao <masao.fujii@gmail.com> — 2026-07-05T23:37:19Z

    On Sat, Jul 4, 2026 at 11:17 PM Andrew Dunstan <andrew@dunslane.net> wrote:
> This has upset the ABI compliance checker (see <https://buildfarm.postgresql.org/cgi-bin/show_failures.pl?max_days=3&stage=abi-compliance-check&filter=Submit> . Looks like you need to update the  .abi-compliance-history file on the affected back branches.

Thanks for pointing out this! That's my mistake...

The ABI break was caused by removing the external function
basebackup_progress_done(). I think the better fix for the stable branches
is to restore that function, even though PostgreSQL core itself no longer
uses it, rather than updating the .abi-compliance-history file. Because
some external backup tools may still be calling basebackup_progress_done().
Thoughts?

Regards,

-- 
Fujii Masao

  8. Re: Clear base backup progress reporting on error

    Tom Lane <tgl@sss.pgh.pa.us> — 2026-07-05T23:40:50Z

    Fujii Masao <masao.fujii@gmail.com> writes:
> The ABI break was caused by removing the external function
> basebackup_progress_done(). I think the better fix for the stable branches
> is to restore that function, even though PostgreSQL core itself no longer
> uses it, rather than updating the .abi-compliance-history file. Because
> some external backup tools may still be calling basebackup_progress_done().

+1

			regards, tom lane

  9. Re: Clear base backup progress reporting on error

    Fujii Masao <masao.fujii@gmail.com> — 2026-07-06T00:52:42Z

    On Mon, Jul 6, 2026 at 8:40 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>
> Fujii Masao <masao.fujii@gmail.com> writes:
> > The ABI break was caused by removing the external function
> > basebackup_progress_done(). I think the better fix for the stable branches
> > is to restore that function, even though PostgreSQL core itself no longer
> > uses it, rather than updating the .abi-compliance-history file. Because
> > some external backup tools may still be calling basebackup_progress_done().
>
> +1

I've restored the function in v15 - v18. Thanks!

Regards,

-- 
Fujii Masao