On Fri, Oct 21, 2022 at 11:58 AM Michael Paquier <mich...@paquier.xyz> wrote: > > To be exact, it seems to me that tablespace_map and backup_state > should be reset before deleting backupcontext, but the reset of > backupcontext should happen after the fact. > > + backup_state = NULL; > tablespace_map = NULL; > These two in pg_backup_start() don't matter, do they? They are > reallocated a couple of lines down.
After all, that is what is being discussed here; what if palloc down below fails and they're not reset to NULL after MemoryContextReset()? > + * across. We keep the memory allocated in this memory context less, > What does "We keep the memory allocated in this memory context less" > mean here? We try to keep it less because we don't want to allocate more memory and leak it unless pg_start_backup() is called again. Please read the description. I'll leave it to the committer's discretion whether to have that part or remove it. On Fri, Oct 21, 2022 at 12:11 PM Kyotaro Horiguchi <horikyota....@gmail.com> wrote: > > > + * 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. > > I think the "less" is somewhat obscure. I feel we should be more > explicitly. And we don't need to put emphasis on "leak". I recklessly > propose this as the draft. I tried to put it simple, please see the attached v10. I'll leave it to the committer's discretion for better wording. On Fri, Oct 21, 2022 at 7:47 PM Robert Haas <robertmh...@gmail.com> wrote: > > On Fri, Oct 21, 2022 at 2:18 AM Michael Paquier <mich...@paquier.xyz> wrote: > > AFAIK, one of the callbacks associated to a memory context could > > fail, see comments before MemoryContextCallResetCallbacks() in > > MemoryContextDelete(). I agree that it should not matter here, but I > > think that it is better to reset the pointers before attempting the > > deletion of the memory context in this case. > > I think this is nitpicking. There's no real danger here, and if there > were, the error handling would have to take it into account somehow, > which it doesn't. > > I'd probably do it before resetting the context as a matter of style, > to make it clear that there's no window in which the pointers are set > but referencing invalid memory. But I do not think it makes any > practical difference at all. Please see the attached v10. -- Bharath Rupireddy PostgreSQL Contributors Team RDS Open Source Databases Amazon Web Services: https://aws.amazon.com
From 3d8cf10d0bab48afee639359669b69c2901afd34 Mon Sep 17 00:00:00 2001 From: Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com> Date: Fri, 21 Oct 2022 15:04:36 +0000 Subject: [PATCH v10] Avoid memory leaks during backups using SQL-callable functions Any failure while taking backups using SQL-callable functions can leak the memory allocated for backup_state and tablespace_map, as they both need to be long-lived, they are being created in TopMemoryContext. To fix the memory leak problem, we create a special session-level memory context as a direct child of TopMemoryContext so that the memory allocated is carried across. We delete the memory context at the end of pg_backup_stop(). We keep the memory allocated in this memory context less, because any error before reaching pg_backup_stop() can still leak the memory until pg_backup_start() is called again. While this is not smart, it helps to keep things simple. Author: Bharath Rupireddy Reviewed-by: Robert Haas, Alvaro Herrera Reviewed-by: Cary Huang, Michael Paquier Discussion: https://www.postgresql.org/message-id/CALj2ACXqvfKF2B0beQ=aJMdWnpNohmBPsRg=EDQj_6y1t2O8mQ@mail.gmail.com --- src/backend/access/transam/xlogfuncs.c | 42 ++++++++++++++------------ 1 file changed, 23 insertions(+), 19 deletions(-) diff --git a/src/backend/access/transam/xlogfuncs.c b/src/backend/access/transam/xlogfuncs.c index a801a94fe8..e9ec86316b 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,27 @@ 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(). We create a special session-level memory context as + * a direct child of TopMemoryContext so that the memory allocated is + * carried across. We typically delete the memory context at the end of + * pg_backup_stop(), however, an error before it can leak the memory until + * pg_backup_start() is called again. Hence, we try to keep the memory + * allocated in this memory context less. 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); + backup_state = NULL; tablespace_map = NULL; + MemoryContextReset(backupcontext); } + oldcontext = MemoryContextSwitchTo(backupcontext); + backup_state = (BackupState *) palloc0(sizeof(BackupState)); tablespace_map = makeStringInfo(); MemoryContextSwitchTo(oldcontext); @@ -157,12 +160,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 backup 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.34.1
