Re: non-exclusive backup cleanup is mildly broken

On Thu, Dec 12, 2019 at 01:52:31PM +0100, Magnus Hagander wrote:
> On Thu, Dec 12, 2019 at 5:58 AM Kyotaro Horiguchi <horikyota(dot)ntt(at)gmail(dot)com>
> wrote:
>> The direction seems reasonable, but the patch doesn't free up the
>> before_shmem_exec slot nor avoid duplicate registration of the
>> callback. Actually before_shmem_exit_list gets bloat with multiple
>> do_pg_abort_backup entries through repeated rounds of non-exclusive
>> backups.
>>
>> As the result, if one ends a session while a non-exclusive backup is
>> active after closing the previous non-exclusive backup,
>> do_pg_abort_backup aborts for assertion failure.

Agreed, that's an issue and do_pg_abort_abort should not touch
sessionBackupState, so you should keep cancel_before_shmem_exit after
pg_stop_backup(). Other than that, I have looked in details at how
safe it is to move before_shmem_exit(do_pg_abort_backup) before
do_pg_start_backup() and the cleanups of nonExclusiveBackups happen
safely and consistently in the event of an error during
pg_start_backup().

> +1, I also think the direction seems perfectly reasonable, but we should
> avoid re-adding the callback since we're not removing it. Leaving it around
> seems cheap enough as long as there is only one.

+ (errmsg("aborting backup due to backend exiting before
pg_stop_back up was called")));
Not sure that pg_stop_back exists ;p

> My first reaction would be to just disallow the combination of prepared
> transactions and start/stop backups. But looking at it it seems like an
> unnecessary restriction and an approach like this one seems better.

I think that's a bad idea to put a restriction of this kind. There
are large consumers of 2PC, and everybody needs backups.
--
Michael