Re: Backup command and functions can cause assertion failure and segmentation fault

On Fri, Jul 08, 2022 at 08:56:14AM -0400, Robert Haas wrote:
> On Thu, Jul 7, 2022 at 10:58 AM Fujii Masao <masao.fujii@oss.nttdata.com> wrote:
>> But if many think that it's worth adding the test, I will give a
>> try. But even in that case, I think it's better to commit the
>> proposed patch at first to fix the bug, and then to write the patch
>> adding the test.

I have looked at that in details, and it is possible to rely on
pg_stat_activity.wait_event to be BaseBackupThrottle, which would make
sure that the checkpoint triggered at the beginning of the backup
finishes and that we are in the middle of the base backup.  The
command for the test should be a psql command with two -c switches
without ON_ERROR_STOP, so as the second pg_backup_stop() starts after
BASE_BACKUP is cancelled using the same connection, for something like
that:
psql -c "BASE_BACKUP (CHECKPOINT 'fast', MAX_RATE 32);" \
     -c "select pg_backup_stop()" "replication=database"

The last part of the test should do a pump_until() and capture "backup
is not in progress" from the stderr output of the command run.

This is leading me to the attached, that crashes quickly without the
fix and passes with the fix.

> It's true that we don't really have good test coverage of write-ahead
> logging and recovery, but this doesn't seem like the most important
> thing to be testing in that area, either, and developing stable tests
> for stuff like this can be a lot of work.

Well, stability does not seem like a problem to me here.

> I do kind of feel like the patch is fixing two separate bugs. The
> change to SendBaseBackup() is fixing the problem that, because there's
> SQL access on replication connections, we could try to start a backup
> in the middle of another backup by mixing and matching the two
> different methods of doing backups. The change to do_pg_abort_backup()
> is fixing the fact that, after aborting a base backup, we don't reset
> the session state properly so that another backup can be tried
> afterwards.
>
> I don't know if it's worth committing them separately - they are very
> small fixes. But it would probably at least be good to highlight in
> the commit message that there are two different issues.

Grouping both fixes in the same commit sounds fine by me.  No
objections from here.
--
Michael