On 12.08.2013 21:08, Pavel Stehule wrote:
2013/8/10 Tom Lane<t...@sss.pgh.pa.us>:
Pavel Stehule<pavel.steh...@gmail.com> writes:
I found so there are no simple API for working with LO from PL without
access to file system.
What? See lo_open(), loread(), lowrite(), etc.
yes, so there are three problems with these functions:
a) probably (I didn't find) undocumented
It's there, although it's a bit difficult to find by searching. See:
http://www.postgresql.org/docs/devel/static/lo-funcs.html.
I don't actually agree with this phrase on that page:
The ones that are actually useful to call via SQL commands are
lo_creat, lo_create, lo_unlink, lo_import, and lo_export
Calling lo_open, loread and lowrite seems equally useful to me.
b) design with lo handler is little bit PL/pgSQL unfriendly.
It's a bit awkward, I agree.
c) probably there is a bug - it doesn't expect handling errors
postgres=# select fbuilder.attachment_to_xml(0);
WARNING: Snapshot reference leak: Snapshot 0x978f6f0 still referenced
attachment_to_xml
───────────────────
[null]
(1 row)
Yeah, that's a server-side bug. inv_open() registers the snapshot before
checking if the large object exists. If it doesn't, the
already-registered snapshot is not unregistered, hence the warning.
I've committed the attached fix for that bug.
- Heikki
>From 357f7521384df34c697b3544115622520a6a0e9f Mon Sep 17 00:00:00 2001
From: Heikki Linnakangas <heikki.linnakan...@iki.fi>
Date: Mon, 30 Sep 2013 11:29:09 +0300
Subject: [PATCH 1/1] Fix snapshot leak if lo_open called on non-existent
object.
lo_open registers the currently active snapshot, and checks if the
large object exists after that. Normally, snapshots registered by lo_open
are unregistered at end of transaction when the lo descriptor is closed, but
if we error out before the lo descriptor is added to the list of open
descriptors, it is leaked. Fix by moving the snapshot registration to after
checking if the large object exists.
Reported by Pavel Stehule. Backpatch to 8.4. The snapshot registration
system was introduced in 8.4, so prior versions are not affected (and not
supported, anyway).
---
src/backend/storage/large_object/inv_api.c | 44 ++++++++++++++++++------------
1 file changed, 26 insertions(+), 18 deletions(-)
diff --git a/src/backend/storage/large_object/inv_api.c b/src/backend/storage/large_object/inv_api.c
index fb91571..d248743 100644
--- a/src/backend/storage/large_object/inv_api.c
+++ b/src/backend/storage/large_object/inv_api.c
@@ -240,29 +240,18 @@ LargeObjectDesc *
inv_open(Oid lobjId, int flags, MemoryContext mcxt)
{
LargeObjectDesc *retval;
-
- retval = (LargeObjectDesc *) MemoryContextAlloc(mcxt,
- sizeof(LargeObjectDesc));
-
- retval->id = lobjId;
- retval->subid = GetCurrentSubTransactionId();
- retval->offset = 0;
+ Snapshot snapshot = NULL;
+ int descflags = 0;
if (flags & INV_WRITE)
{
- retval->snapshot = NULL; /* instantaneous MVCC snapshot */
- retval->flags = IFS_WRLOCK | IFS_RDLOCK;
+ snapshot = NULL; /* instantaneous MVCC snapshot */
+ descflags = IFS_WRLOCK | IFS_RDLOCK;
}
else if (flags & INV_READ)
{
- /*
- * 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.
- */
- retval->snapshot = RegisterSnapshotOnOwner(GetActiveSnapshot(),
- TopTransactionResourceOwner);
- retval->flags = IFS_RDLOCK;
+ snapshot = GetActiveSnapshot();
+ descflags = IFS_RDLOCK;
}
else
ereport(ERROR,
@@ -271,11 +260,30 @@ inv_open(Oid lobjId, int flags, MemoryContext mcxt)
flags)));
/* Can't use LargeObjectExists here because we need to specify snapshot */
- if (!myLargeObjectExists(lobjId, retval->snapshot))
+ if (!myLargeObjectExists(lobjId, snapshot))
ereport(ERROR,
(errcode(ERRCODE_UNDEFINED_OBJECT),
errmsg("large object %u does not exist", lobjId)));
+ /*
+ * 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 after checking that the LO exists,
+ * to avoid leaking the snapshot if an error is thrown.
+ */
+ if (snapshot)
+ snapshot = RegisterSnapshotOnOwner(snapshot,
+ TopTransactionResourceOwner);
+
+ /* All set, create a descriptor */
+ retval = (LargeObjectDesc *) MemoryContextAlloc(mcxt,
+ sizeof(LargeObjectDesc));
+ retval->id = lobjId;
+ retval->subid = GetCurrentSubTransactionId();
+ retval->offset = 0;
+ retval->snapshot = snapshot;
+ retval->flags = descflags;
+
return retval;
}
--
1.8.4.rc3
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
