diff --git a/src/backend/libpq/be-fsstubs.c b/src/backend/libpq/be-fsstubs.c index 97add3f257..d352032df1 100644 --- a/src/backend/libpq/be-fsstubs.c +++ b/src/backend/libpq/be-fsstubs.c @@ -42,6 +42,7 @@ #include #include +#include "access/xact.h" #include "libpq/be-fsstubs.h" #include "libpq/libpq-fs.h" #include "miscadmin.h" @@ -50,6 +51,7 @@ #include "utils/acl.h" #include "utils/builtins.h" #include "utils/memutils.h" +#include "utils/snapmgr.h" /* define this to enable debug logging */ /* #define FSDB 1 */ @@ -67,19 +69,11 @@ static LargeObjectDesc **cookies = NULL; static int cookies_size = 0; +static bool lo_cleanup_needed = false; static MemoryContext fscxt = NULL; -#define CreateFSContext() \ - do { \ - if (fscxt == NULL) \ - fscxt = AllocSetContextCreate(TopMemoryContext, \ - "Filesystem", \ - ALLOCSET_DEFAULT_SIZES); \ - } while (0) - - -static int newLOfd(LargeObjectDesc *lobjCookie); -static void deleteLOfd(int fd); +static int newLOfd(void); +static void closeLOfd(int fd); static Oid lo_import_internal(text *filename, Oid lobjOid); @@ -99,11 +93,26 @@ be_lo_open(PG_FUNCTION_ARGS) elog(DEBUG4, "lo_open(%u,%d)", lobjId, mode); #endif - CreateFSContext(); + /* + * Allocate a large object descriptor first. This will also create + * 'fscxt' if this is the first LO opened in this transaction. + */ + fd = newLOfd(); lobjDesc = inv_open(lobjId, mode, fscxt); + lobjDesc->subid = GetCurrentSubTransactionId(); - fd = newLOfd(lobjDesc); + /* + * We must register the snapshot in TopTransaction's resowner so that it + * stays alive until the LO is closed rather than until the current portal + * shuts down. + */ + if (lobjDesc->snapshot) + lobjDesc->snapshot = RegisterSnapshotOnOwner(lobjDesc->snapshot, + TopTransactionResourceOwner); + + Assert(cookies[fd] == NULL); + cookies[fd] = lobjDesc; PG_RETURN_INT32(fd); } @@ -122,9 +131,7 @@ be_lo_close(PG_FUNCTION_ARGS) elog(DEBUG4, "lo_close(%d)", fd); #endif - inv_close(cookies[fd]); - - deleteLOfd(fd); + closeLOfd(fd); PG_RETURN_INT32(0); } @@ -238,12 +245,7 @@ be_lo_creat(PG_FUNCTION_ARGS) { Oid lobjId; - /* - * 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(); - + lo_cleanup_needed = true; lobjId = inv_create(InvalidOid); PG_RETURN_OID(lobjId); @@ -254,12 +256,7 @@ be_lo_create(PG_FUNCTION_ARGS) { Oid lobjId = PG_GETARG_OID(0); - /* - * 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(); - + lo_cleanup_needed = true; lobjId = inv_create(lobjId); PG_RETURN_OID(lobjId); @@ -330,16 +327,13 @@ be_lo_unlink(PG_FUNCTION_ARGS) for (i = 0; i < cookies_size; i++) { if (cookies[i] != NULL && cookies[i]->id == lobjId) - { - inv_close(cookies[i]); - deleteLOfd(i); - } + closeLOfd(i); } } /* * inv_drop does not create a need for end-of-transaction cleanup and - * hence we don't need to have created fscxt. + * hence we don't need to set lo_cleanup_needed. */ PG_RETURN_INT32(inv_drop(lobjId)); } @@ -419,8 +413,6 @@ lo_import_internal(text *filename, Oid lobjOid) LargeObjectDesc *lobj; Oid oid; - CreateFSContext(); - /* * open the file to be read in */ @@ -435,12 +427,13 @@ lo_import_internal(text *filename, Oid lobjOid) /* * create an inversion object */ + lo_cleanup_needed = true; oid = inv_create(lobjOid); /* * read in from the filesystem and write to the inversion object */ - lobj = inv_open(oid, INV_WRITE, fscxt); + lobj = inv_open(oid, INV_WRITE, CurrentMemoryContext); while ((nbytes = read(fd, buf, BUFSIZE)) > 0) { @@ -482,12 +475,11 @@ be_lo_export(PG_FUNCTION_ARGS) LargeObjectDesc *lobj; mode_t oumask; - CreateFSContext(); - /* * open the inversion object (no need to test for failure) */ - lobj = inv_open(lobjId, INV_READ, fscxt); + lo_cleanup_needed = true; + lobj = inv_open(lobjId, INV_READ, CurrentMemoryContext); /* * open the file to be written to @@ -594,20 +586,22 @@ AtEOXact_LargeObject(bool isCommit) { int i; - if (fscxt == NULL) + if (!lo_cleanup_needed) return; /* no LO operations in this xact */ /* * Close LO fds and clear cookies array so that LO fds are no longer good. - * On abort we skip the close step. + * The memory context and resource owner holding them are going away at + * the end-of-transaction anyway, but on commit, we need to close them to + * avoid warnings about leaked resources at commit. On abort we can skip + * this step. */ - for (i = 0; i < cookies_size; i++) + if (isCommit) { - if (cookies[i] != NULL) + for (i = 0; i < cookies_size; i++) { - if (isCommit) - inv_close(cookies[i]); - deleteLOfd(i); + if (cookies[i] != NULL) + closeLOfd(i); } } @@ -616,11 +610,14 @@ AtEOXact_LargeObject(bool isCommit) cookies_size = 0; /* Release the LO memory context to prevent permanent memory leaks. */ - MemoryContextDelete(fscxt); + if (fscxt) + MemoryContextDelete(fscxt); fscxt = NULL; /* Give inv_api.c a chance to clean up, too */ close_lo_relation(isCommit); + + lo_cleanup_needed = false; } /* @@ -648,14 +645,7 @@ AtEOSubXact_LargeObject(bool isCommit, SubTransactionId mySubid, if (isCommit) lo->subid = parentSubid; else - { - /* - * Make sure we do not call inv_close twice if it errors out - * for some reason. Better a leak than a crash. - */ - deleteLOfd(i); - inv_close(lo); - } + closeLOfd(i); } } } @@ -665,19 +655,22 @@ AtEOSubXact_LargeObject(bool isCommit, SubTransactionId mySubid, *****************************************************************************/ static int -newLOfd(LargeObjectDesc *lobjCookie) +newLOfd(void) { int i, newsize; + lo_cleanup_needed = true; + if (fscxt == NULL) + fscxt = AllocSetContextCreate(TopMemoryContext, + "Filesystem", + ALLOCSET_DEFAULT_SIZES); + /* Try to find a free slot */ for (i = 0; i < cookies_size; i++) { if (cookies[i] == NULL) - { - cookies[i] = lobjCookie; return i; - } } /* No free slot, so make the array bigger */ @@ -702,15 +695,25 @@ newLOfd(LargeObjectDesc *lobjCookie) cookies_size = newsize; } - Assert(cookies[i] == NULL); - cookies[i] = lobjCookie; return i; } static void -deleteLOfd(int fd) +closeLOfd(int fd) { + LargeObjectDesc *lobj; + + /* + * Make sure we do not try to free twice if this errors out for some + * reason. Better a leak than a crash. + */ + lobj = cookies[fd]; cookies[fd] = NULL; + + if (lobj->snapshot) + UnregisterSnapshotFromOwner(lobj->snapshot, + TopTransactionResourceOwner); + inv_close(lobj); } /***************************************************************************** @@ -729,13 +732,8 @@ lo_get_fragment_internal(Oid loOid, int64 offset, int32 nbytes) int total_read PG_USED_FOR_ASSERTS_ONLY; bytea *result = NULL; - /* - * 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(); - - loDesc = inv_open(loOid, INV_READ, fscxt); + lo_cleanup_needed = true; + loDesc = inv_open(loOid, INV_READ, CurrentMemoryContext); /* * Compute number of bytes we'll actually read, accommodating nbytes == -1 @@ -819,10 +817,9 @@ be_lo_from_bytea(PG_FUNCTION_ARGS) LargeObjectDesc *loDesc; int written PG_USED_FOR_ASSERTS_ONLY; - CreateFSContext(); - + lo_cleanup_needed = true; loOid = inv_create(loOid); - loDesc = inv_open(loOid, INV_WRITE, fscxt); + loDesc = inv_open(loOid, INV_WRITE, CurrentMemoryContext); written = inv_write(loDesc, VARDATA_ANY(str), VARSIZE_ANY_EXHDR(str)); Assert(written == VARSIZE_ANY_EXHDR(str)); inv_close(loDesc); @@ -842,9 +839,8 @@ be_lo_put(PG_FUNCTION_ARGS) LargeObjectDesc *loDesc; int written PG_USED_FOR_ASSERTS_ONLY; - CreateFSContext(); - - loDesc = inv_open(loOid, INV_WRITE, fscxt); + lo_cleanup_needed = true; + loDesc = inv_open(loOid, INV_WRITE, CurrentMemoryContext); /* Permission check */ if (!lo_compat_privileges && diff --git a/src/backend/storage/large_object/inv_api.c b/src/backend/storage/large_object/inv_api.c index a477cb9200..fd8df8d1ef 100644 --- a/src/backend/storage/large_object/inv_api.c +++ b/src/backend/storage/large_object/inv_api.c @@ -242,10 +242,12 @@ inv_create(Oid lobjId) /* * inv_open -- access an existing large object. * - * Returns: - * 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. + * Returns a 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. If the returned descriptor has a snapshot associated + * with it, the caller must ensure that it also lives long enough, + * e.g. by calling RegisterSnapshotOnOwner */ LargeObjectDesc * inv_open(Oid lobjId, int flags, MemoryContext mcxt) @@ -312,19 +314,16 @@ inv_open(Oid lobjId, int flags, MemoryContext mcxt) retval = (LargeObjectDesc *) MemoryContextAlloc(mcxt, sizeof(LargeObjectDesc)); retval->id = lobjId; - retval->subid = GetCurrentSubTransactionId(); retval->offset = 0; retval->flags = descflags; + /* caller sets if needed, not used by the functions in this file */ + retval->subid = InvalidSubTransactionId; + /* - * We must register the snapshot in TopTransaction's resowner, because it - * must stay alive until the LO is closed rather than until the current - * portal shuts down. Do this last to avoid uselessly leaking the - * snapshot if an error is thrown above. + * The snapshot (if any) is just the currently active snapshot. The + * caller will replace it with a longer-lived copy if needed. */ - if (snapshot) - snapshot = RegisterSnapshotOnOwner(snapshot, - TopTransactionResourceOwner); retval->snapshot = snapshot; return retval; @@ -338,10 +337,6 @@ void inv_close(LargeObjectDesc *obj_desc) { Assert(PointerIsValid(obj_desc)); - - UnregisterSnapshotFromOwner(obj_desc->snapshot, - TopTransactionResourceOwner); - pfree(obj_desc); } diff --git a/src/test/regress/input/largeobject.source b/src/test/regress/input/largeobject.source index 974f36b21f..aa03a51ddf 100644 --- a/src/test/regress/input/largeobject.source +++ b/src/test/regress/input/largeobject.source @@ -124,6 +124,17 @@ BEGIN; SELECT lo_open(loid, x'40000'::int) from lotest_stash_values; ABORT; +DO $$ +DECLARE + loid oid; +BEGIN + SELECT tbl.loid INTO loid FROM lotest_stash_values tbl; + PERFORM lo_export(loid, '@abs_builddir@/results/invalid/path'); +EXCEPTION + WHEN UNDEFINED_FILE THEN RAISE NOTICE 'could not open file, as expected'; +END; +$$; + -- Test truncation. BEGIN; UPDATE lotest_stash_values SET fd=lo_open(loid, CAST(x'20000' | x'40000' AS integer)); diff --git a/src/test/regress/output/largeobject.source b/src/test/regress/output/largeobject.source index 1b7b3daee2..94470625ee 100644 --- a/src/test/regress/output/largeobject.source +++ b/src/test/regress/output/largeobject.source @@ -161,6 +161,17 @@ SELECT lo_open(loid, x'40000'::int) from lotest_stash_values; (1 row) ABORT; +DO $$ +DECLARE + loid oid; +BEGIN + SELECT tbl.loid INTO loid FROM lotest_stash_values tbl; + PERFORM lo_export(loid, '@abs_builddir@/results/invalid/path'); +EXCEPTION + WHEN UNDEFINED_FILE THEN RAISE NOTICE 'could not open file, as expected'; +END; +$$; +NOTICE: could not open file, as expected -- Test truncation. BEGIN; UPDATE lotest_stash_values SET fd=lo_open(loid, CAST(x'20000' | x'40000' AS integer));