On Fri, Oct 21, 2022 at 7:06 PM Michael Paquier <mich...@paquier.xyz> wrote:
>
> On Fri, Oct 21, 2022 at 05:53:25PM +0800, Richard Guo wrote:
> > Yeah, the two conditions could be both false. How about we update the
> > comment a bit to emphasize this? Such as
> >
> > /* At most one of these conditions can be true */
> > or
> > /* These conditions can not be both true */
>
> If you do that, it would be a bit easier to read as of the following
> assertion instead? Like:
> Assert(!during_backup_start ||
> sessionBackupState == SESSION_BACKUP_NONE);
>
> Please note that I have not checked in details all the interactions
> behind register_persistent_abort_backup_handler() before entering in
> do_pg_backup_start() and the ERROR_CLEANUP block used in this
> routine (just a matter of some elog(ERROR)s put here and there, for
> example). Anyway, yes, both conditions can be false, and that's easy
> to reproduce:
> 1) Do pg_backup_start().
> 2) Do pg_backup_stop().
> 3) Stop the session to kick do_pg_abort_backup()
> 4) Assert()-boom.
I'm wondering if we need the assertion at all. We know that when the
arg is true or the sessionBackupState is SESSION_BACKUP_RUNNING, the
runningBackups would've been incremented and we can just go ahead and
decrement it, like the attached patch. This is a cleaner approach IMO
unless I'm missing something here.
--
Bharath Rupireddy
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com
From b147720ca68a120efdf8f20c58cb3499901e6d61 Mon Sep 17 00:00:00 2001
From: Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com>
Date: Fri, 21 Oct 2022 14:29:27 +0000
Subject: [PATCH v1] Fix assertion failure in do_pg_abort_backup()
---
src/backend/access/transam/xlog.c | 11 ++++++-----
1 file changed, 6 insertions(+), 5 deletions(-)
diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c
index dea978a962..104309c56f 100644
--- a/src/backend/access/transam/xlog.c
+++ b/src/backend/access/transam/xlog.c
@@ -8841,11 +8841,12 @@ do_pg_abort_backup(int code, Datum arg)
{
bool during_backup_start = DatumGetBool(arg);
- /* Only one of these conditions can be true */
- Assert(during_backup_start ^
- (sessionBackupState == SESSION_BACKUP_RUNNING));
-
- if (during_backup_start || sessionBackupState != SESSION_BACKUP_NONE)
+ /*
+ * When either of these were true, we know that the runningBackups has
+ * already been incremented, hence decrement it.
+ */
+ if (during_backup_start ||
+ sessionBackupState == SESSION_BACKUP_RUNNING)
{
WALInsertLockAcquireExclusive();
Assert(XLogCtl->Insert.runningBackups > 0);
--
2.34.1
