On Wed, Oct 19, 2022 at 9:23 PM Bharath Rupireddy
<bharath.rupireddyforpostg...@gmail.com> wrote:
>
> On Wed, Oct 19, 2022 at 8:10 PM Robert Haas <robertmh...@gmail.com> wrote:
>
> > One option is to just have do_pg_start_backup() blow
> > away any old memory context before it allocates any new memory, and
> > forget about releasing anything in PostgresMain(). That means memory
> > could remain allocated after a failure until you next retry the
> > operation, but I don't think that really matters. It's not a lot of
> > memory; we just don't want it to accumulate across many repetitions.
>
> This seems reasonable to me.
I tried implementing this, please see the attached v7 patch.
Currently, memory allocated in the new memory context is around 4KB
[1]. In the extreme and rarest of the rare cases where somebody
executes select pg_backup_start(repeat('foo', 1024)); or a failure
occurs before reaching pg_backup_stop() on all of the sessions
(max_connections) at once, the maximum/peak memory bloat/leak is
around max_connections*4KB, which will still be way less than the
total amount of RAM. Hence, I think this approach seems very
reasonable and non-invasive yet can solve the memory leak problem.
Thoughts?
[1]
(gdb) p *backupcontext
$4 = {type = T_AllocSetContext, isReset = false, allowInCritSection =
false, mem_allocated = 4232,
methods = 0x55c925b81f90 <mcxt_methods+240>, parent =
0x55c92766d2a0, firstchild = 0x0, prevchild = 0x0,
nextchild = 0x55c92773f1f0, name = 0x55c9258be05c "on-line backup
context", ident = 0x0, reset_cbs = 0x0}
--
Bharath Rupireddy
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com
From 657933ab5cc6001ad13d56b89fbf220af541f216 Mon Sep 17 00:00:00 2001
From: Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com>
Date: Thu, 20 Oct 2022 10:09:59 +0000
Subject: [PATCH v7] Avoid memory leaks during backups using SQL-callable
functions
---
src/backend/access/transam/xlogfuncs.c | 43 ++++++++++++--------------
1 file changed, 20 insertions(+), 23 deletions(-)
diff --git a/src/backend/access/transam/xlogfuncs.c b/src/backend/access/transam/xlogfuncs.c
index a801a94fe8..198a99a1ed 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;
+/* A long-lived workspace for SQL-callable backup functions. */
+static MemoryContext backupcontext = NULL;
+
/*
* pg_backup_start: set up for taking an on-line backup dump
*
@@ -72,27 +75,22 @@ 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(). Create a special session-level memory context as a
+ * direct child of TopMemoryContext so that the memory allocated is carried
+ * across. We keep the memory allocated in this memory context less,
+ * because any error before reaching pg_backup_stop() can leak the memory
+ * until pg_backup_start() is called again. While this is not smart, it
+ * helps to keep things simple.
*/
- 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);
- tablespace_map = NULL;
- }
+ MemoryContextReset(backupcontext);
+ oldcontext = MemoryContextSwitchTo(backupcontext);
+ backup_state = (BackupState *) palloc0(sizeof(BackupState));
tablespace_map = makeStringInfo();
MemoryContextSwitchTo(oldcontext);
@@ -157,13 +155,12 @@ pg_backup_stop(PG_FUNCTION_ARGS)
values[2] = CStringGetTextDatum(tablespace_map->data);
/* Deallocate backup-related variables */
- pfree(backup_state);
- backup_state = NULL;
- pfree(tablespace_map->data);
- pfree(tablespace_map);
- tablespace_map = NULL;
pfree(backup_label);
+ /* Clean up the session-level backup memory context */
+ MemoryContextDelete(backupcontext);
+ backupcontext = NULL;
+
/* Returns the record as Datum */
PG_RETURN_DATUM(HeapTupleGetDatum(heap_form_tuple(tupdesc, values, nulls)));
}
--
2.34.1
