From 6cc66197ff9aa34cc47486d7bf719f376f6f5ff8 Mon Sep 17 00:00:00 2001 From: Michael Paquier Date: Sat, 22 Oct 2022 11:54:02 +0900 Subject: [PATCH] Improve memory handling across SQL-callable backup functions Since pg_backup_start() and pg_backup_stop() exist, the tablespace map data and the backup state data (backup_label string until 7d70809) have been allocated in the TopMemoryContext. This approach would cause memory leaks in the session calling these functions if failures happen before pg_backup_stop() ends, leaking more memory on repeated failures. Both things need little memory so that would not be really noticeable for most users, except perhaps connection poolers with long-lived connections able to trigger backup failures with these functions. This commit improves the logic in this area by not allocating anymore the backup-related data that needs to travel across the SQL-callable backup functions in TopMemoryContext, by using instead a dedicated memory context child of TopMemoryContext. The memory context is created in pg_backup_start() and deleted when finishing pg_backup_stop(). In the event of an in-flight failure, this memory context gets reset in the follow-up pg_backup_start() call, so as we are sure that only one run worth of data is leaked at any time. Some cleanup was already done for the backup data on a follow-up call of pg_backup_start(), but using a memory context makes the whole simpler. BASE_BACKUP commands are executed in isolation, relying on the memory context created for replication commands, hence these do not need such an extra logic. Author: Bharath Rupireddy Reviewed-by: Robert Haas, Alvaro Herrera, Cary Huang, Michael Paquier Discussion: https://postgr.es/m/CALj2ACXqvfKF2B0beQ=aJMdWnpNohmBPsRg=EDQj_6y1t2O8mQ@mail.gmail.com --- src/backend/access/transam/xlogfuncs.c | 41 ++++++++++++++------------ 1 file changed, 22 insertions(+), 19 deletions(-) diff --git a/src/backend/access/transam/xlogfuncs.c b/src/backend/access/transam/xlogfuncs.c index a801a94fe85..487d5d9cac7 100644 --- a/src/backend/access/transam/xlogfuncs.c +++ b/src/backend/access/transam/xlogfuncs.c @@ -45,6 +45,9 @@ static BackupState *backup_state = NULL; static StringInfo tablespace_map = NULL; +/* Session-level context for the SQL-callable backup functions */ +static MemoryContext backupcontext = NULL; + /* * pg_backup_start: set up for taking an on-line backup dump * @@ -72,27 +75,26 @@ pg_backup_start(PG_FUNCTION_ARGS) /* * backup_state and tablespace_map need to be long-lived as they are used - * in pg_backup_stop(). + * in pg_backup_stop(). These are allocated in a dedicated memory context + * child of TopMemoryContext, deleted at the end of pg_backup_stop(). If + * an error happens before ending the backup, memory would be leaked in + * this context until pg_backup_start() is called again. */ - oldcontext = MemoryContextSwitchTo(TopMemoryContext); - - /* Allocate backup state or reset it, if it comes from a previous run */ - if (backup_state == NULL) - backup_state = (BackupState *) palloc0(sizeof(BackupState)); + if (backupcontext == NULL) + { + backupcontext = AllocSetContextCreate(TopMemoryContext, + "on-line backup context", + ALLOCSET_START_SMALL_SIZES); + } else - MemSet(backup_state, 0, sizeof(BackupState)); - - /* - * tablespace_map may have been created in a previous backup, so take this - * occasion to clean it. - */ - if (tablespace_map != NULL) { - pfree(tablespace_map->data); - pfree(tablespace_map); + backup_state = NULL; tablespace_map = NULL; + MemoryContextReset(backupcontext); } + oldcontext = MemoryContextSwitchTo(backupcontext); + backup_state = (BackupState *) palloc0(sizeof(BackupState)); tablespace_map = makeStringInfo(); MemoryContextSwitchTo(oldcontext); @@ -157,12 +159,13 @@ pg_backup_stop(PG_FUNCTION_ARGS) values[2] = CStringGetTextDatum(tablespace_map->data); /* Deallocate backup-related variables */ - pfree(backup_state); + pfree(backup_label); + + /* Clean up the session-level state and its memory context */ backup_state = NULL; - pfree(tablespace_map->data); - pfree(tablespace_map); tablespace_map = NULL; - pfree(backup_label); + MemoryContextDelete(backupcontext); + backupcontext = NULL; /* Returns the record as Datum */ PG_RETURN_DATUM(HeapTupleGetDatum(heap_form_tuple(tupdesc, values, nulls))); -- 2.39.5