Location via proxy:   [ UP ]  
[Report a bug]   [Manage cookies]                
Skip to content

Commit df3737a

Browse files
alvherreBRupireddy2
andcommitted
Remove pg_backup_start_callback and reuse similar code
We had two copies of almost identical logic to revert shared memory state when a running backup aborts; we can remove pg_backup_start_callback if we adapt do_pg_abort_backup so that it can be used for this purpose too. However, in order for this to work, we have to repurpose the flag passed to do_pg_abort_backup. It used to indicate whether to throw a warning (and the only caller always passed true). It now indicates whether the callback is being called at start time (in which case the session backup state is known not to have been set to RUNNING yet, so action is always taken) or shmem time (in which case action is only taken if the session backup state is RUNNING). Thus the meaning of the flag is no longer superfluous, but it's actually quite critical to get right. I (Álvaro) chose to change the polarity and the code flow re. the flag from what Bharath submitted, for coding clarity. Co-authored-by: Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com> Discussion: https://www.postgresql.org/message-id/20221013111330.564fk5tkwe3ha77l%40alvherre.pgsql
1 parent 9668c4a commit df3737a

File tree

1 file changed

+35
-46
lines changed
  • src/backend/access/transam

1 file changed

+35
-46
lines changed

src/backend/access/transam/xlog.c

+35-46
Original file line numberDiff line numberDiff line change
@@ -679,8 +679,6 @@ static void ReadControlFile(void);
679679
static void UpdateControlFile(void);
680680
static char *str_time(pg_time_t tnow);
681681

682-
static void pg_backup_start_callback(int code, Datum arg);
683-
684682
static int get_sync_bit(int method);
685683

686684
static void CopyXLogRecordToWAL(int write_len, bool isLogSwitch,
@@ -8271,7 +8269,7 @@ void
82718269
do_pg_backup_start(const char *backupidstr, bool fast, List **tablespaces,
82728270
BackupState *state, StringInfo tblspcmapfile)
82738271
{
8274-
bool backup_started_in_recovery = false;
8272+
bool backup_started_in_recovery;
82758273

82768274
Assert(state != NULL);
82778275
backup_started_in_recovery = RecoveryInProgress();
@@ -8320,8 +8318,12 @@ do_pg_backup_start(const char *backupidstr, bool fast, List **tablespaces,
83208318
XLogCtl->Insert.forcePageWrites = true;
83218319
WALInsertLockRelease();
83228320

8323-
/* Ensure we release forcePageWrites if fail below */
8324-
PG_ENSURE_ERROR_CLEANUP(pg_backup_start_callback, (Datum) 0);
8321+
/*
8322+
* Ensure we release forcePageWrites if fail below. NB -- for this to work
8323+
* correctly, it is critical that sessionBackupState is only updated after
8324+
* this block is over.
8325+
*/
8326+
PG_ENSURE_ERROR_CLEANUP(do_pg_abort_backup, DatumGetBool(true));
83258327
{
83268328
bool gotUniqueStartpoint = false;
83278329
DIR *tblspcdir;
@@ -8531,7 +8533,7 @@ do_pg_backup_start(const char *backupidstr, bool fast, List **tablespaces,
85318533

85328534
state->starttime = (pg_time_t) time(NULL);
85338535
}
8534-
PG_END_ENSURE_ERROR_CLEANUP(pg_backup_start_callback, (Datum) 0);
8536+
PG_END_ENSURE_ERROR_CLEANUP(do_pg_abort_backup, DatumGetBool(true));
85358537

85368538
state->started_in_recovery = backup_started_in_recovery;
85378539

@@ -8541,23 +8543,6 @@ do_pg_backup_start(const char *backupidstr, bool fast, List **tablespaces,
85418543
sessionBackupState = SESSION_BACKUP_RUNNING;
85428544
}
85438545

8544-
/* Error cleanup callback for pg_backup_start */
8545-
static void
8546-
pg_backup_start_callback(int code, Datum arg)
8547-
{
8548-
/* Update backup counters and forcePageWrites on failure */
8549-
WALInsertLockAcquireExclusive();
8550-
8551-
Assert(XLogCtl->Insert.runningBackups > 0);
8552-
XLogCtl->Insert.runningBackups--;
8553-
8554-
if (XLogCtl->Insert.runningBackups == 0)
8555-
{
8556-
XLogCtl->Insert.forcePageWrites = false;
8557-
}
8558-
WALInsertLockRelease();
8559-
}
8560-
85618546
/*
85628547
* Utility routine to fetch the session-level status of a backup running.
85638548
*/
@@ -8850,38 +8835,42 @@ do_pg_backup_stop(BackupState *state, bool waitforarchive)
88508835
* system out of backup mode, thus making it a lot more safe to call from
88518836
* an error handler.
88528837
*
8853-
* The caller can pass 'arg' as 'true' or 'false' to control whether a warning
8854-
* is emitted.
8838+
* 'arg' indicates that it's being called during backup setup; so
8839+
* sessionBackupState has not been modified yet, but runningBackups has
8840+
* already been incremented. When it's false, then it's invoked as a
8841+
* before_shmem_exit handler, and therefore we must not change state
8842+
* unless sessionBackupState indicates that a backup is actually running.
88558843
*
8856-
* NB: This gets used as a before_shmem_exit handler, hence the odd-looking
8857-
* signature.
8844+
* NB: This gets used as a PG_ENSURE_ERROR_CLEANUP callback and
8845+
* before_shmem_exit handler, hence the odd-looking signature.
88588846
*/
88598847
void
88608848
do_pg_abort_backup(int code, Datum arg)
88618849
{
8862-
bool emit_warning = DatumGetBool(arg);
8850+
bool during_backup_start = DatumGetBool(arg);
88638851

8864-
/*
8865-
* Quick exit if session does not have a running backup.
8866-
*/
8867-
if (sessionBackupState != SESSION_BACKUP_RUNNING)
8868-
return;
8852+
/* Only one of these conditions can be true */
8853+
Assert(during_backup_start ^
8854+
(sessionBackupState == SESSION_BACKUP_RUNNING));
88698855

8870-
WALInsertLockAcquireExclusive();
8871-
Assert(XLogCtl->Insert.runningBackups > 0);
8872-
XLogCtl->Insert.runningBackups--;
8873-
8874-
if (XLogCtl->Insert.runningBackups == 0)
8856+
if (during_backup_start || sessionBackupState != SESSION_BACKUP_NONE)
88758857
{
8876-
XLogCtl->Insert.forcePageWrites = false;
8877-
}
8858+
WALInsertLockAcquireExclusive();
8859+
Assert(XLogCtl->Insert.runningBackups > 0);
8860+
XLogCtl->Insert.runningBackups--;
88788861

8879-
sessionBackupState = SESSION_BACKUP_NONE;
8880-
WALInsertLockRelease();
8862+
if (XLogCtl->Insert.runningBackups == 0)
8863+
{
8864+
XLogCtl->Insert.forcePageWrites = false;
8865+
}
8866+
8867+
sessionBackupState = SESSION_BACKUP_NONE;
8868+
WALInsertLockRelease();
88818869

8882-
if (emit_warning)
8883-
ereport(WARNING,
8884-
(errmsg("aborting backup due to backend exiting before pg_backup_stop was called")));
8870+
if (!during_backup_start)
8871+
ereport(WARNING,
8872+
errmsg("aborting backup due to backend exiting before pg_backup_stop was called"));
8873+
}
88858874
}
88868875

88878876
/*
@@ -8895,7 +8884,7 @@ register_persistent_abort_backup_handler(void)
88958884

88968885
if (already_done)
88978886
return;
8898-
before_shmem_exit(do_pg_abort_backup, DatumGetBool(true));
8887+
before_shmem_exit(do_pg_abort_backup, DatumGetBool(false));
88998888
already_done = true;
89008889
}
89018890

0 commit comments

Comments
 (0)