Commit 486f994b authored by Tom Lane's avatar Tom Lane

Revise large-object access routines to avoid running with CurrentMemoryContext

set to the large object context ("fscxt"), as this is inevitably a source of
transaction-duration memory leaks.  Not sure why we'd not noticed it before;
maybe people weren't touching a whole lot of LOs in the same transaction
before the 8.1 pg_dump changes.  Per report from Wayne Conrad.

Backpatched as far as 8.1, but the problem doubtless goes all the way back.
I'm disinclined to spend the time to try to verify that the older branches
would still work if patched, seeing that this code was significantly modified
for 8.0 and again for 8.1, and that we don't have any trouble reports before
8.1.  (Maybe the leaks were smaller before?)
parent d2896a9e
...@@ -8,15 +8,15 @@ ...@@ -8,15 +8,15 @@
* *
* *
* IDENTIFICATION * IDENTIFICATION
* $PostgreSQL: pgsql/src/backend/libpq/be-fsstubs.c,v 1.81 2006/03/05 15:58:27 momjian Exp $ * $PostgreSQL: pgsql/src/backend/libpq/be-fsstubs.c,v 1.82 2006/04/26 00:34:57 tgl Exp $
* *
* NOTES * NOTES
* This should be moved to a more appropriate place. It is here * This should be moved to a more appropriate place. It is here
* for lack of a better place. * for lack of a better place.
* *
* These functions operate in a private MemoryContext, which means * These functions store LargeObjectDesc structs in a private MemoryContext,
* that large object descriptors hang around until we destroy the context * which means that large object descriptors hang around until we destroy
* at transaction end. It'd be possible to prolong the lifetime * the context at transaction end. It'd be possible to prolong the lifetime
* of the context so that LO FDs are good across transactions (for example, * of the context so that LO FDs are good across transactions (for example,
* we could release the context only if we see that no FDs remain open). * we could release the context only if we see that no FDs remain open).
* But we'd need additional state in order to do the right thing at the * But we'd need additional state in order to do the right thing at the
...@@ -29,8 +29,9 @@ ...@@ -29,8 +29,9 @@
* *
* As of PostgreSQL 8.0, much of the angst expressed above is no longer * As of PostgreSQL 8.0, much of the angst expressed above is no longer
* relevant, and in fact it'd be pretty easy to allow LO FDs to stay * relevant, and in fact it'd be pretty easy to allow LO FDs to stay
* open across transactions. However backwards compatibility suggests * open across transactions. (Snapshot relevancy would still be an issue.)
* that we should stick to the status quo. * However backwards compatibility suggests that we should stick to the
* status quo.
* *
*------------------------------------------------------------------------- *-------------------------------------------------------------------------
*/ */
...@@ -56,9 +57,9 @@ ...@@ -56,9 +57,9 @@
* LO "FD"s are indexes into the cookies array. * LO "FD"s are indexes into the cookies array.
* *
* A non-null entry is a pointer to a LargeObjectDesc allocated in the * A non-null entry is a pointer to a LargeObjectDesc allocated in the
* LO private memory context. The cookies array itself is also dynamically * LO private memory context "fscxt". The cookies array itself is also
* allocated in that context. Its current allocated size is cookies_len * dynamically allocated in that context. Its current allocated size is
* entries, of which any unused entries will be NULL. * cookies_len entries, of which any unused entries will be NULL.
*/ */
static LargeObjectDesc **cookies = NULL; static LargeObjectDesc **cookies = NULL;
static int cookies_size = 0; static int cookies_size = 0;
...@@ -91,7 +92,6 @@ lo_open(PG_FUNCTION_ARGS) ...@@ -91,7 +92,6 @@ lo_open(PG_FUNCTION_ARGS)
int32 mode = PG_GETARG_INT32(1); int32 mode = PG_GETARG_INT32(1);
LargeObjectDesc *lobjDesc; LargeObjectDesc *lobjDesc;
int fd; int fd;
MemoryContext currentContext;
#if FSDB #if FSDB
elog(DEBUG4, "lo_open(%u,%d)", lobjId, mode); elog(DEBUG4, "lo_open(%u,%d)", lobjId, mode);
...@@ -99,13 +99,10 @@ lo_open(PG_FUNCTION_ARGS) ...@@ -99,13 +99,10 @@ lo_open(PG_FUNCTION_ARGS)
CreateFSContext(); CreateFSContext();
currentContext = MemoryContextSwitchTo(fscxt); lobjDesc = inv_open(lobjId, mode, fscxt);
lobjDesc = inv_open(lobjId, mode);
if (lobjDesc == NULL) if (lobjDesc == NULL)
{ /* lookup failed */ { /* lookup failed */
MemoryContextSwitchTo(currentContext);
#if FSDB #if FSDB
elog(DEBUG4, "could not open large object %u", lobjId); elog(DEBUG4, "could not open large object %u", lobjId);
#endif #endif
...@@ -114,8 +111,6 @@ lo_open(PG_FUNCTION_ARGS) ...@@ -114,8 +111,6 @@ lo_open(PG_FUNCTION_ARGS)
fd = newLOfd(lobjDesc); fd = newLOfd(lobjDesc);
MemoryContextSwitchTo(currentContext);
PG_RETURN_INT32(fd); PG_RETURN_INT32(fd);
} }
...@@ -123,7 +118,6 @@ Datum ...@@ -123,7 +118,6 @@ Datum
lo_close(PG_FUNCTION_ARGS) lo_close(PG_FUNCTION_ARGS)
{ {
int32 fd = PG_GETARG_INT32(0); int32 fd = PG_GETARG_INT32(0);
MemoryContext currentContext;
if (fd < 0 || fd >= cookies_size || cookies[fd] == NULL) if (fd < 0 || fd >= cookies_size || cookies[fd] == NULL)
{ {
...@@ -136,15 +130,10 @@ lo_close(PG_FUNCTION_ARGS) ...@@ -136,15 +130,10 @@ lo_close(PG_FUNCTION_ARGS)
elog(DEBUG4, "lo_close(%d)", fd); elog(DEBUG4, "lo_close(%d)", fd);
#endif #endif
Assert(fscxt != NULL);
currentContext = MemoryContextSwitchTo(fscxt);
inv_close(cookies[fd]); inv_close(cookies[fd]);
deleteLOfd(fd); deleteLOfd(fd);
MemoryContextSwitchTo(currentContext);
PG_RETURN_INT32(0); PG_RETURN_INT32(0);
} }
...@@ -160,7 +149,6 @@ lo_close(PG_FUNCTION_ARGS) ...@@ -160,7 +149,6 @@ lo_close(PG_FUNCTION_ARGS)
int int
lo_read(int fd, char *buf, int len) lo_read(int fd, char *buf, int len)
{ {
MemoryContext currentContext;
int status; int status;
if (fd < 0 || fd >= cookies_size || cookies[fd] == NULL) if (fd < 0 || fd >= cookies_size || cookies[fd] == NULL)
...@@ -171,20 +159,14 @@ lo_read(int fd, char *buf, int len) ...@@ -171,20 +159,14 @@ lo_read(int fd, char *buf, int len)
return -1; return -1;
} }
Assert(fscxt != NULL);
currentContext = MemoryContextSwitchTo(fscxt);
status = inv_read(cookies[fd], buf, len); status = inv_read(cookies[fd], buf, len);
MemoryContextSwitchTo(currentContext);
return status; return status;
} }
int int
lo_write(int fd, char *buf, int len) lo_write(int fd, char *buf, int len)
{ {
MemoryContext currentContext;
int status; int status;
if (fd < 0 || fd >= cookies_size || cookies[fd] == NULL) if (fd < 0 || fd >= cookies_size || cookies[fd] == NULL)
...@@ -201,13 +183,8 @@ lo_write(int fd, char *buf, int len) ...@@ -201,13 +183,8 @@ lo_write(int fd, char *buf, int len)
errmsg("large object descriptor %d was not opened for writing", errmsg("large object descriptor %d was not opened for writing",
fd))); fd)));
Assert(fscxt != NULL);
currentContext = MemoryContextSwitchTo(fscxt);
status = inv_write(cookies[fd], buf, len); status = inv_write(cookies[fd], buf, len);
MemoryContextSwitchTo(currentContext);
return status; return status;
} }
...@@ -218,7 +195,6 @@ lo_lseek(PG_FUNCTION_ARGS) ...@@ -218,7 +195,6 @@ lo_lseek(PG_FUNCTION_ARGS)
int32 fd = PG_GETARG_INT32(0); int32 fd = PG_GETARG_INT32(0);
int32 offset = PG_GETARG_INT32(1); int32 offset = PG_GETARG_INT32(1);
int32 whence = PG_GETARG_INT32(2); int32 whence = PG_GETARG_INT32(2);
MemoryContext currentContext;
int status; int status;
if (fd < 0 || fd >= cookies_size || cookies[fd] == NULL) if (fd < 0 || fd >= cookies_size || cookies[fd] == NULL)
...@@ -229,13 +205,8 @@ lo_lseek(PG_FUNCTION_ARGS) ...@@ -229,13 +205,8 @@ lo_lseek(PG_FUNCTION_ARGS)
PG_RETURN_INT32(-1); PG_RETURN_INT32(-1);
} }
Assert(fscxt != NULL);
currentContext = MemoryContextSwitchTo(fscxt);
status = inv_seek(cookies[fd], offset, whence); status = inv_seek(cookies[fd], offset, whence);
MemoryContextSwitchTo(currentContext);
PG_RETURN_INT32(status); PG_RETURN_INT32(status);
} }
...@@ -243,17 +214,15 @@ Datum ...@@ -243,17 +214,15 @@ Datum
lo_creat(PG_FUNCTION_ARGS) lo_creat(PG_FUNCTION_ARGS)
{ {
Oid lobjId; Oid lobjId;
MemoryContext currentContext;
/* do we actually need fscxt for this? */ /*
* We don't actually need to store into fscxt, but create it anyway to
* ensure that AtEOXact_LargeObject knows there is state to clean up
*/
CreateFSContext(); CreateFSContext();
currentContext = MemoryContextSwitchTo(fscxt);
lobjId = inv_create(InvalidOid); lobjId = inv_create(InvalidOid);
MemoryContextSwitchTo(currentContext);
PG_RETURN_OID(lobjId); PG_RETURN_OID(lobjId);
} }
...@@ -261,17 +230,15 @@ Datum ...@@ -261,17 +230,15 @@ Datum
lo_create(PG_FUNCTION_ARGS) lo_create(PG_FUNCTION_ARGS)
{ {
Oid lobjId = PG_GETARG_OID(0); Oid lobjId = PG_GETARG_OID(0);
MemoryContext currentContext;
/* do we actually need fscxt for this? */ /*
* We don't actually need to store into fscxt, but create it anyway to
* ensure that AtEOXact_LargeObject knows there is state to clean up
*/
CreateFSContext(); CreateFSContext();
currentContext = MemoryContextSwitchTo(fscxt);
lobjId = inv_create(lobjId); lobjId = inv_create(lobjId);
MemoryContextSwitchTo(currentContext);
PG_RETURN_OID(lobjId); PG_RETURN_OID(lobjId);
} }
...@@ -288,10 +255,6 @@ lo_tell(PG_FUNCTION_ARGS) ...@@ -288,10 +255,6 @@ lo_tell(PG_FUNCTION_ARGS)
PG_RETURN_INT32(-1); PG_RETURN_INT32(-1);
} }
/*
* We assume we do not need to switch contexts for inv_tell. That is true
* for now, but is probably more than this module ought to assume...
*/
PG_RETURN_INT32(inv_tell(cookies[fd])); PG_RETURN_INT32(inv_tell(cookies[fd]));
} }
...@@ -305,10 +268,8 @@ lo_unlink(PG_FUNCTION_ARGS) ...@@ -305,10 +268,8 @@ lo_unlink(PG_FUNCTION_ARGS)
*/ */
if (fscxt != NULL) if (fscxt != NULL)
{ {
MemoryContext currentContext;
int i; int i;
currentContext = MemoryContextSwitchTo(fscxt);
for (i = 0; i < cookies_size; i++) for (i = 0; i < cookies_size; i++)
{ {
if (cookies[i] != NULL && cookies[i]->id == lobjId) if (cookies[i] != NULL && cookies[i]->id == lobjId)
...@@ -317,13 +278,11 @@ lo_unlink(PG_FUNCTION_ARGS) ...@@ -317,13 +278,11 @@ lo_unlink(PG_FUNCTION_ARGS)
deleteLOfd(i); deleteLOfd(i);
} }
} }
MemoryContextSwitchTo(currentContext);
} }
/* /*
* inv_drop does not need a context switch, indeed it doesn't touch any * inv_drop does not create a need for end-of-transaction cleanup and
* LO-specific data structures at all. (Again, that's probably more than * hence we don't need to have created fscxt.
* this module ought to be assuming.)
*/ */
PG_RETURN_INT32(inv_drop(lobjId)); PG_RETURN_INT32(inv_drop(lobjId));
} }
...@@ -391,10 +350,6 @@ lo_import(PG_FUNCTION_ARGS) ...@@ -391,10 +350,6 @@ lo_import(PG_FUNCTION_ARGS)
errhint("Anyone can use the client-side lo_import() provided by libpq."))); errhint("Anyone can use the client-side lo_import() provided by libpq.")));
#endif #endif
/*
* We don't actually need to switch into fscxt, but create it anyway to
* ensure that AtEOXact_LargeObject knows there is state to clean up
*/
CreateFSContext(); CreateFSContext();
/* /*
...@@ -420,7 +375,7 @@ lo_import(PG_FUNCTION_ARGS) ...@@ -420,7 +375,7 @@ lo_import(PG_FUNCTION_ARGS)
/* /*
* read in from the filesystem and write to the inversion object * read in from the filesystem and write to the inversion object
*/ */
lobj = inv_open(lobjOid, INV_WRITE); lobj = inv_open(lobjOid, INV_WRITE, fscxt);
while ((nbytes = FileRead(fd, buf, BUFSIZE)) > 0) while ((nbytes = FileRead(fd, buf, BUFSIZE)) > 0)
{ {
...@@ -465,16 +420,12 @@ lo_export(PG_FUNCTION_ARGS) ...@@ -465,16 +420,12 @@ lo_export(PG_FUNCTION_ARGS)
errhint("Anyone can use the client-side lo_export() provided by libpq."))); errhint("Anyone can use the client-side lo_export() provided by libpq.")));
#endif #endif
/*
* We don't actually need to switch into fscxt, but create it anyway to
* ensure that AtEOXact_LargeObject knows there is state to clean up
*/
CreateFSContext(); CreateFSContext();
/* /*
* open the inversion object (no need to test for failure) * open the inversion object (no need to test for failure)
*/ */
lobj = inv_open(lobjId, INV_READ); lobj = inv_open(lobjId, INV_READ, fscxt);
/* /*
* open the file to be written to * open the file to be written to
...@@ -524,13 +475,10 @@ void ...@@ -524,13 +475,10 @@ void
AtEOXact_LargeObject(bool isCommit) AtEOXact_LargeObject(bool isCommit)
{ {
int i; int i;
MemoryContext currentContext;
if (fscxt == NULL) if (fscxt == NULL)
return; /* no LO operations in this xact */ return; /* no LO operations in this xact */
currentContext = MemoryContextSwitchTo(fscxt);
/* /*
* Close LO fds and clear cookies array so that LO fds are no longer good. * Close LO fds and clear cookies array so that LO fds are no longer good.
* On abort we skip the close step. * On abort we skip the close step.
...@@ -549,8 +497,6 @@ AtEOXact_LargeObject(bool isCommit) ...@@ -549,8 +497,6 @@ AtEOXact_LargeObject(bool isCommit)
cookies = NULL; cookies = NULL;
cookies_size = 0; cookies_size = 0;
MemoryContextSwitchTo(currentContext);
/* Release the LO memory context to prevent permanent memory leaks. */ /* Release the LO memory context to prevent permanent memory leaks. */
MemoryContextDelete(fscxt); MemoryContextDelete(fscxt);
fscxt = NULL; fscxt = NULL;
...@@ -623,8 +569,7 @@ newLOfd(LargeObjectDesc *lobjCookie) ...@@ -623,8 +569,7 @@ newLOfd(LargeObjectDesc *lobjCookie)
i = 0; i = 0;
newsize = 64; newsize = 64;
cookies = (LargeObjectDesc **) cookies = (LargeObjectDesc **)
palloc(newsize * sizeof(LargeObjectDesc *)); MemoryContextAllocZero(fscxt, newsize * sizeof(LargeObjectDesc *));
MemSet(cookies, 0, newsize * sizeof(LargeObjectDesc *));
cookies_size = newsize; cookies_size = newsize;
} }
else else
......
...@@ -4,12 +4,20 @@ ...@@ -4,12 +4,20 @@
* routines for manipulating inversion fs large objects. This file * routines for manipulating inversion fs large objects. This file
* contains the user-level large object application interface routines. * contains the user-level large object application interface routines.
* *
*
* Note: many of these routines leak memory in CurrentMemoryContext, as indeed
* does most of the backend code. We expect that CurrentMemoryContext will
* be a short-lived context. Data that must persist across function calls
* is kept either in CacheMemoryContext (the Relation structs) or in the
* memory context given to inv_open (for LargeObjectDesc structs).
*
*
* Portions Copyright (c) 1996-2006, PostgreSQL Global Development Group * Portions Copyright (c) 1996-2006, PostgreSQL Global Development Group
* Portions Copyright (c) 1994, Regents of the University of California * Portions Copyright (c) 1994, Regents of the University of California
* *
* *
* IDENTIFICATION * IDENTIFICATION
* $PostgreSQL: pgsql/src/backend/storage/large_object/inv_api.c,v 1.115 2006/03/05 15:58:38 momjian Exp $ * $PostgreSQL: pgsql/src/backend/storage/large_object/inv_api.c,v 1.116 2006/04/26 00:34:57 tgl Exp $
* *
*------------------------------------------------------------------------- *-------------------------------------------------------------------------
*/ */
...@@ -205,14 +213,17 @@ inv_create(Oid lobjId) ...@@ -205,14 +213,17 @@ inv_create(Oid lobjId)
* inv_open -- access an existing large object. * inv_open -- access an existing large object.
* *
* Returns: * Returns:
* large object descriptor, appropriately filled in. * Large object descriptor, appropriately filled in. The descriptor
* and subsidiary data are allocated in the specified memory context,
* which must be suitably long-lived for the caller's purposes.
*/ */
LargeObjectDesc * LargeObjectDesc *
inv_open(Oid lobjId, int flags) inv_open(Oid lobjId, int flags, MemoryContext mcxt)
{ {
LargeObjectDesc *retval; LargeObjectDesc *retval;
retval = (LargeObjectDesc *) palloc(sizeof(LargeObjectDesc)); retval = (LargeObjectDesc *) MemoryContextAlloc(mcxt,
sizeof(LargeObjectDesc));
retval->id = lobjId; retval->id = lobjId;
retval->subid = GetCurrentSubTransactionId(); retval->subid = GetCurrentSubTransactionId();
...@@ -225,9 +236,12 @@ inv_open(Oid lobjId, int flags) ...@@ -225,9 +236,12 @@ inv_open(Oid lobjId, int flags)
} }
else if (flags & INV_READ) else if (flags & INV_READ)
{ {
/* be sure to copy snap into fscxt */ /* be sure to copy snap into mcxt */
MemoryContext oldContext = MemoryContextSwitchTo(mcxt);
retval->snapshot = CopySnapshot(ActiveSnapshot); retval->snapshot = CopySnapshot(ActiveSnapshot);
retval->flags = IFS_RDLOCK; retval->flags = IFS_RDLOCK;
MemoryContextSwitchTo(oldContext);
} }
else else
elog(ERROR, "invalid flags: %d", flags); elog(ERROR, "invalid flags: %d", flags);
...@@ -242,7 +256,8 @@ inv_open(Oid lobjId, int flags) ...@@ -242,7 +256,8 @@ inv_open(Oid lobjId, int flags)
} }
/* /*
* Closes an existing large object descriptor. * Closes a large object descriptor previously made by inv_open(), and
* releases the long-term memory used by it.
*/ */
void void
inv_close(LargeObjectDesc *obj_desc) inv_close(LargeObjectDesc *obj_desc)
......
...@@ -8,7 +8,7 @@ ...@@ -8,7 +8,7 @@
* Portions Copyright (c) 1996-2006, PostgreSQL Global Development Group * Portions Copyright (c) 1996-2006, PostgreSQL Global Development Group
* Portions Copyright (c) 1994, Regents of the University of California * Portions Copyright (c) 1994, Regents of the University of California
* *
* $PostgreSQL: pgsql/src/include/storage/large_object.h,v 1.33 2006/03/05 15:58:59 momjian Exp $ * $PostgreSQL: pgsql/src/include/storage/large_object.h,v 1.34 2006/04/26 00:34:57 tgl Exp $
* *
*------------------------------------------------------------------------- *-------------------------------------------------------------------------
*/ */
...@@ -71,7 +71,7 @@ typedef struct LargeObjectDesc ...@@ -71,7 +71,7 @@ typedef struct LargeObjectDesc
/* inversion stuff in inv_api.c */ /* inversion stuff in inv_api.c */
extern void close_lo_relation(bool isCommit); extern void close_lo_relation(bool isCommit);
extern Oid inv_create(Oid lobjId); extern Oid inv_create(Oid lobjId);
extern LargeObjectDesc *inv_open(Oid lobjId, int flags); extern LargeObjectDesc *inv_open(Oid lobjId, int flags, MemoryContext mcxt);
extern void inv_close(LargeObjectDesc *obj_desc); extern void inv_close(LargeObjectDesc *obj_desc);
extern int inv_drop(Oid lobjId); extern int inv_drop(Oid lobjId);
extern int inv_seek(LargeObjectDesc *obj_desc, int offset, int whence); extern int inv_seek(LargeObjectDesc *obj_desc, int offset, int whence);
......
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment