From 8dc04f3999de477509a375264de50a7c43bd034c Mon Sep 17 00:00:00 2001 From: Bharath Rupireddy Date: Mon, 26 Sep 2022 12:45:42 +0000 Subject: [PATCH v1] Avoid memory leaks during base backups Postgres currently can leak memory if a failure occurs during base backup in do_pg_backup_start() or do_pg_backup_stop() or perform_base_backup(). The palloc'd memory such as backup_state or tablespace_map in xlogfuncs.c or basebackup.c or tablespaceinfo in perform_base_backup() or any other, is left-out which may cause memory bloat on the server eventually. To experience this issue, run pg_basebackup with --label name longer than 1024 characters and observe memory with watch command, the memory usage goes up. It looks like the memory leak issue has been there for quite some time. To solve the above problem, leverage the error callback mechanism and memory context. The design of the patch is as follows: 1) pg_backup_start() and pg_backup_stop() - the error callback frees up the backup_state and tablespace_map variables allocated in TopMemoryContext. We don't need a separate memory context here because do_pg_backup_start() and do_pg_backup_stop() don't return any dynamically created memory for now. We can choose to create a separate memory context for the future changes that may come, but now it is not required. 2) perform_base_backup() - a new memory context has been created that gets deleted by the callback upon error. The error callbacks are typically called for all the elevels, but we need to free up the memory only when elevel is >= ERROR or == COMMERROR. The COMMERROR is a common scenario because the server can close the connection to the client or vice versa in which case the base backup fails. For all other elevels like WARNING, NOTICE, DEBUGX, INFO etc. we don't free up the memory. --- src/backend/access/transam/xlogfuncs.c | 49 ++++++++++++++++++++++++++ src/backend/backup/basebackup.c | 49 ++++++++++++++++++++++++++ src/backend/utils/error/elog.c | 17 +++++++++ src/include/utils/elog.h | 1 + 4 files changed, 116 insertions(+) diff --git a/src/backend/access/transam/xlogfuncs.c b/src/backend/access/transam/xlogfuncs.c index f724b18733..3467a6ae22 100644 --- a/src/backend/access/transam/xlogfuncs.c +++ b/src/backend/access/transam/xlogfuncs.c @@ -45,6 +45,38 @@ static BackupState *backup_state = NULL; static StringInfo tablespace_map = NULL; +/* + * Error callback for deallocating and resetting backup-related variables. + */ +static void +deallocate_backup_vars_err_cb(void *arg) +{ + int elevel; + + elevel = geterrlevel(); + + /* + * Error callbacks are typically called for all elevels, but we only care + * about ERROR, FATAL and so on (elevel >= ERROR), and client communication + * problems (elevel == COMMERROR). + */ + if (elevel >= ERROR || elevel == COMMERROR) + { + if (backup_state != NULL) + { + pfree(backup_state); + backup_state = NULL; + } + + if (tablespace_map == NULL) + { + pfree(tablespace_map->data); + pfree(tablespace_map); + tablespace_map = NULL; + } + } +} + /* * pg_backup_start: set up for taking an on-line backup dump * @@ -62,6 +94,7 @@ pg_backup_start(PG_FUNCTION_ARGS) char *backupidstr; SessionBackupState status = get_backup_status(); MemoryContext oldcontext; + ErrorContextCallback errcallback; backupidstr = text_to_cstring(backupid); @@ -96,9 +129,16 @@ pg_backup_start(PG_FUNCTION_ARGS) tablespace_map = makeStringInfo(); MemoryContextSwitchTo(oldcontext); + /* Set up callback to deallocate backup-related variables */ + errcallback.callback = deallocate_backup_vars_err_cb; + errcallback.previous = error_context_stack; + error_context_stack = &errcallback; + register_persistent_abort_backup_handler(); do_pg_backup_start(backupidstr, fast, NULL, backup_state, tablespace_map); + /* Pop the error context stack */ + error_context_stack = errcallback.previous; PG_RETURN_LSN(backup_state->startpoint); } @@ -132,6 +172,7 @@ pg_backup_stop(PG_FUNCTION_ARGS) bool waitforarchive = PG_GETARG_BOOL(0); StringInfo backup_label; SessionBackupState status = get_backup_status(); + ErrorContextCallback errcallback; /* Initialize attributes information in the tuple descriptor */ if (get_call_result_type(fcinfo, NULL, &tupdesc) != TYPEFUNC_COMPOSITE) @@ -146,9 +187,17 @@ pg_backup_stop(PG_FUNCTION_ARGS) Assert(backup_state != NULL); Assert(tablespace_map != NULL); + /* Set up callback to deallocate backup-related variables */ + errcallback.callback = deallocate_backup_vars_err_cb; + errcallback.previous = error_context_stack; + error_context_stack = &errcallback; + /* Stop the backup */ do_pg_backup_stop(backup_state, waitforarchive); + /* Pop the error context stack */ + error_context_stack = errcallback.previous; + /* Build the contents of backup_label */ backup_label = build_backup_content(backup_state, false); diff --git a/src/backend/backup/basebackup.c b/src/backend/backup/basebackup.c index 495bbb506a..f063106c89 100644 --- a/src/backend/backup/basebackup.c +++ b/src/backend/backup/basebackup.c @@ -42,6 +42,7 @@ #include "storage/reinit.h" #include "utils/builtins.h" #include "utils/guc.h" +#include "utils/memutils.h" #include "utils/ps_status.h" #include "utils/relcache.h" #include "utils/resowner.h" @@ -220,6 +221,29 @@ static const struct exclude_list_item noChecksumFiles[] = { {NULL, false} }; +/* + * Error callback for deleting base backup memory context. + */ +static void +delete_base_backup_mcxt_err_cb(void *arg) +{ + MemoryContext backupcontext = (MemoryContext) arg; + int elevel; + + if (backupcontext == NULL) + return; + + elevel = geterrlevel(); + + /* + * Error callbacks are typically called for all elevels, but we only care + * about ERROR, FATAL and so on (elevel >= ERROR), and client communication + * problems (elevel == COMMERROR). + */ + if (elevel >= ERROR || elevel == COMMERROR) + MemoryContextDelete(backupcontext); +} + /* * Actually do a base backup for the specified tablespaces. * @@ -235,6 +259,9 @@ perform_base_backup(basebackup_options *opt, bbsink *sink) backup_manifest_info manifest; BackupState *backup_state; StringInfo tablespace_map; + MemoryContext oldcontext; + MemoryContext backupcontext; + ErrorContextCallback errcallback; /* Initial backup state, insofar as we know it now. */ state.tablespaces = NIL; @@ -252,6 +279,22 @@ perform_base_backup(basebackup_options *opt, bbsink *sink) InitializeBackupManifest(&manifest, opt->manifest, opt->manifest_checksum_type); + /* + * Perform base backup in a special memory context to not leak any memory + * in case of failures. + */ + backupcontext = AllocSetContextCreate(CurrentMemoryContext, + "base backup context", + ALLOCSET_DEFAULT_SIZES); + + oldcontext = MemoryContextSwitchTo(backupcontext); + + /* Set up callback to delete base backup memory context */ + errcallback.callback = delete_base_backup_mcxt_err_cb; + errcallback.arg = (void *) backupcontext; + errcallback.previous = error_context_stack; + error_context_stack = &errcallback; + total_checksum_failures = 0; /* Allocate backup related varilables. */ @@ -661,6 +704,12 @@ perform_base_backup(basebackup_options *opt, bbsink *sink) */ FreeBackupManifest(&manifest); + /* Pop the error context stack */ + error_context_stack = errcallback.previous; + + MemoryContextSwitchTo(oldcontext); + MemoryContextDelete(backupcontext); + /* clean up the resource owner we created */ WalSndResourceCleanup(true); diff --git a/src/backend/utils/error/elog.c b/src/backend/utils/error/elog.c index eb724a9d7f..6519fb310c 100644 --- a/src/backend/utils/error/elog.c +++ b/src/backend/utils/error/elog.c @@ -1404,6 +1404,23 @@ geterrcode(void) return edata->sqlerrcode; } +/* + * geterrlevel --- return the currently error level + * + * This is only intended for use in error callback subroutines, since there + * is no other place outside elog.c where the concept is meaningful. + */ +int +geterrlevel(void) +{ + ErrorData *edata = &errordata[errordata_stack_depth]; + + /* we don't bother incrementing recursion_depth */ + CHECK_STACK_DEPTH(); + + return edata->elevel; +} + /* * geterrposition --- return the currently set error position (0 if none) * diff --git a/src/include/utils/elog.h b/src/include/utils/elog.h index 4dd9658a3c..a8955f9572 100644 --- a/src/include/utils/elog.h +++ b/src/include/utils/elog.h @@ -222,6 +222,7 @@ extern int internalerrquery(const char *query); extern int err_generic_string(int field, const char *str); extern int geterrcode(void); +extern int geterrlevel(void); extern int geterrposition(void); extern int getinternalerrposition(void); -- 2.34.1