postgres.git / summary / log / commit / refs

commit    6cc66197ff9aa34cc47486d7bf719f376f6f5ff8
Author:   Michael Paquier <michael@paquier.xyz>
Date:     Sat Oct 22 02:54:02 2022 +0000
First released in: 16.0

    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)));


[parent: 1f0c4fa25525]