Thread: Re: Re: [PATCHES] Patch to support transactions with BLOBs for current CVS
[ Charset KOI8-R unsupported, converting... ] > On Saturday 20 January 2001 10:05, you wrote: > > I just wanted to confirm that this patch was applied. > > Yes, it is. But the following patch is not applied. But I sure that it is > neccessary, otherwise we will get really strange errors (see discussion in > the thread). > > http://www.postgresql.org/mhonarc/pgsql-patches/2000-11/msg00013.html Can people comment on the following patch that Dennis says is needed? It prevents BLOB operations outside transactions. Dennis, can you explain why BLOB operations have to be done inside transactions? --------------------------------------------------------------------------- Hello, here is the patch attached which do check in each BLOB operation, if we are in transaction, and raise an error otherwise. This will prevent such mistakes. -- Sincerely Yours, Denis Perchine -- Bruce Momjian | http://candle.pha.pa.us pgman@candle.pha.pa.us | (610) 853-3000 + If your life is a hard drive, | 830 Blythe Avenue + Christ can be your backup. | Drexel Hill, Pennsylvania 19026 <!-- MHonArc v2.4.7 --> <!--X-Subject: Patch to check whether we are in TX when to lo_* --> <!--X-From-R13: Rravf Brepuvar <qlcNcrepuvar.pbz> --> <!--X-Date: Fri, 3 Nov 2000 11:59:39 -0500 (EST)(envelope-from dyp@perchine.com) --> <!--X-Message-Id: 0011032159470B.00541@dyp.perchine.com --> <!--X-Content-Type: multipart/mixed --> <!--X-Head-End--> <!DOCTYPE HTML PUBLIC "-//W3C//DTD HTML//EN"> <HTML> <HEAD> <META NAME="robots" CONTENT="all"> <TITLE>Patch to check whether we are in TX when to lo_*</TITLE> </HEAD> <BODY BGCOLOR="#FFFDEC"> <CENTER><A HREF="http://ads.pgsql.com/cgi-bin/redirect.cgi"><IMG SRC="http://ads.pgsql.com/cgi-bin/display_image.cgi" BORDER=0HEIGHT=60 WIDTH=468></A></CENTER> <P><HR WIDTH=40% SIZE=3 NOSHADE><P> <!--X-Body-Begin--> <!--X-User-Header--> <!--X-User-Header-End--> <!--X-TopPNI--> <p> <!--X-TopPNI-End--> <!--X-MsgBody--> <!--X-Subject-Header-Begin--> <h2>Patch to check whether we are in TX when to lo_*</h2> <HR SIZE=3 WIDTH=40% NOSHADE> <!--X-Subject-Header-End--> <!--X-Head-of-Message--> <ul> <li><strong>From</strong>: <strong>Denis Perchine <<A HREF="mailto:dyp@perchine.com">dyp@perchine.com</A>></strong></li> <li><strong>To</strong>: <strong><A HREF="mailto:pgsql-patches@postgresql.org">pgsql-patches@postgresql.org</A></strong></li> <li><strong>Subject</strong>: <strong>Patch to check whether we are in TX when to lo_*</strong></li> <li>Date: Fri, 3 Nov 2000 21:59:47 +0600</li> </ul> <!--X-Head-of-Message-End--> <!--X-Head-Body-Sep-Begin--> <hr> <!--X-Head-Body-Sep-End--> <!--X-Body-of-Message--> <PRE> Hello, here is the patch attached which do check in each BLOB operation, if we are in transaction, and raise an error otherwise. This will prevent such mistakes. -- Sincerely Yours, Denis Perchine ---------------------------------- E-Mail: dyp@perchine.com HomePage: <A HREF="http://www.perchine.com/dyp/">http://www.perchine.com/dyp/</A> FidoNet: 2:5000/120.5 ---------------------------------- </PRE> <PRE> Index: inv_api.c =================================================================== RCS file: /home/projects/pgsql/cvsroot/pgsql/src/backend/storage/large_object/inv_api.c,v retrieving revision 1.80 diff -u -r1.80 inv_api.c --- inv_api.c 2000/11/02 23:52:06 1.80 +++ inv_api.c 2000/11/03 16:57:57 @@ -64,6 +64,9 @@ Oid file_oid; LargeObjectDesc *retval; + if (!IsTransactionBlock()) + elog(ERROR, "inv_create: Not in transaction. BLOBs should be used inside transaction."); + /* * Allocate an OID to be the LO's identifier. */ @@ -117,6 +120,9 @@ { LargeObjectDesc *retval; + if (!IsTransactionBlock()) + elog(ERROR, "inv_open: Not in transaction. BLOBs should be used inside transaction."); + if (! LargeObjectExists(lobjId)) elog(ERROR, "inv_open: large object %u not found", lobjId); @@ -145,6 +151,9 @@ void inv_close(LargeObjectDesc *obj_desc) { + if (!IsTransactionBlock()) + elog(ERROR, "inv_close: Not in transaction. BLOBs should be used inside transaction."); + Assert(PointerIsValid(obj_desc)); if (obj_desc->flags & IFS_WRLOCK) @@ -164,6 +173,9 @@ int inv_drop(Oid lobjId) { + if (!IsTransactionBlock()) + elog(ERROR, "inv_drop: Not in transaction. BLOBs should be used inside transaction."); + LargeObjectDrop(lobjId); /* @@ -248,6 +260,9 @@ int inv_seek(LargeObjectDesc *obj_desc, int offset, int whence) { + if (!IsTransactionBlock()) + elog(ERROR, "inv_seek: Not in transaction. BLOBs should be used inside transaction."); + Assert(PointerIsValid(obj_desc)); switch (whence) @@ -280,6 +295,9 @@ int inv_tell(LargeObjectDesc *obj_desc) { + if (!IsTransactionBlock()) + elog(ERROR, "inv_tell: Not in transaction. BLOBs should be used inside transaction."); + Assert(PointerIsValid(obj_desc)); return obj_desc->offset; @@ -303,6 +321,9 @@ bytea *datafield; bool pfreeit; + if (!IsTransactionBlock()) + elog(ERROR, "inv_read: Not in transaction. BLOBs should be used inside transaction."); + Assert(PointerIsValid(obj_desc)); Assert(buf != NULL); @@ -414,6 +435,9 @@ char replace[Natts_pg_largeobject]; bool write_indices; Relation idescs[Num_pg_largeobject_indices]; + + if (!IsTransactionBlock()) + elog(ERROR, "inv_write: Not in transaction. BLOBs should be used inside transaction."); Assert(PointerIsValid(obj_desc)); Assert(buf != NULL); </PRE> <!--X-Body-of-Message-End--> <!--X-MsgBody-End--> <!--X-Follow-Ups--> <hr> <!--X-Follow-Ups-End--> <!--X-References--> <!--X-References-End--> <!--X-BotPNI--> <ul> <li>Prev by Date: <strong><a href="msg00012.html">Re: Small fix for inv_getsize</a></strong> </li> <li>Next by Date: <strong><a href="msg00014.html">Re: Patch to check whether we are in TX when to lo_*</a></strong> </li> <li>Prev by thread: <strong><a href="msg00018.html">Re: Inherited column patches</a></strong> </li> <li>Next by thread: <strong><a href="msg00014.html">Re: Patch to check whether we are in TX when to lo_*</a></strong> </li> <li>Index(es): <ul> <li><a href="index.html#00013"><strong>Date</strong></a></li> <li><a href="threads.html#00013"><strong>Thread</strong></a></li> </ul> </li> </ul> <!--X-BotPNI-End--> <!--X-User-Footer--> <strong> <a href="http://www.postgresql.org/mhonarc/">Home</a> | <a href="index.html">Main Index</a> | <a href="threads.html">Thread Index</a> </strong> <!--X-User-Footer-End--> </body> </html>
Sorry, here is a clean version of the patch. > > http://www.postgresql.org/mhonarc/pgsql-patches/2000-11/msg00013.html > > Can people comment on the following patch that Dennis says is needed? > It prevents BLOB operations outside transactions. Dennis, can you > explain why BLOB operations have to be done inside transactions? > > --------------------------------------------------------------------------- > Hello, > > here is the patch attached which do check in each BLOB operation, if we are > in transaction, and raise an error otherwise. This will prevent such > mistakes. > -- Bruce Momjian | http://candle.pha.pa.us pgman@candle.pha.pa.us | (610) 853-3000 + If your life is a hard drive, | 830 Blythe Avenue + Christ can be your backup. | Drexel Hill, Pennsylvania 19026 Index: inv_api.c =================================================================== RCS file: /home/projects/pgsql/cvsroot/pgsql/src/backend/storage/large_object/inv_api.c,v retrieving revision 1.80 diff -u -r1.80 inv_api.c --- inv_api.c 2000/11/02 23:52:06 1.80 +++ inv_api.c 2000/11/03 16:57:57 @@ -64,6 +64,9 @@ Oid file_oid; LargeObjectDesc *retval; + if (!IsTransactionBlock()) + elog(ERROR, "inv_create: Not in transaction. BLOBs should be used inside transaction."); + /* * Allocate an OID to be the LO's identifier. */ @@ -117,6 +120,9 @@ { LargeObjectDesc *retval; + if (!IsTransactionBlock()) + elog(ERROR, "inv_open: Not in transaction. BLOBs should be used inside transaction."); + if (! LargeObjectExists(lobjId)) elog(ERROR, "inv_open: large object %u not found", lobjId); @@ -145,6 +151,9 @@ void inv_close(LargeObjectDesc *obj_desc) { + if (!IsTransactionBlock()) + elog(ERROR, "inv_close: Not in transaction. BLOBs should be used inside transaction."); + Assert(PointerIsValid(obj_desc)); if (obj_desc->flags & IFS_WRLOCK) @@ -164,6 +173,9 @@ int inv_drop(Oid lobjId) { + if (!IsTransactionBlock()) + elog(ERROR, "inv_drop: Not in transaction. BLOBs should be used inside transaction."); + LargeObjectDrop(lobjId); /* @@ -248,6 +260,9 @@ int inv_seek(LargeObjectDesc *obj_desc, int offset, int whence) { + if (!IsTransactionBlock()) + elog(ERROR, "inv_seek: Not in transaction. BLOBs should be used inside transaction."); + Assert(PointerIsValid(obj_desc)); switch (whence) @@ -280,6 +295,9 @@ int inv_tell(LargeObjectDesc *obj_desc) { + if (!IsTransactionBlock()) + elog(ERROR, "inv_tell: Not in transaction. BLOBs should be used inside transaction."); + Assert(PointerIsValid(obj_desc)); return obj_desc->offset; @@ -303,6 +321,9 @@ bytea *datafield; bool pfreeit; + if (!IsTransactionBlock()) + elog(ERROR, "inv_read: Not in transaction. BLOBs should be used inside transaction."); + Assert(PointerIsValid(obj_desc)); Assert(buf != NULL); @@ -414,6 +435,9 @@ char replace[Natts_pg_largeobject]; bool write_indices; Relation idescs[Num_pg_largeobject_indices]; + + if (!IsTransactionBlock()) + elog(ERROR, "inv_write: Not in transaction. BLOBs should be used inside transaction."); Assert(PointerIsValid(obj_desc)); Assert(buf != NULL);
Bruce Momjian <pgman@candle.pha.pa.us> writes:
> Can people comment on the following patch that Dennis says is needed?
I object strongly. As given, this would break lo_creat, lo_unlink,
lo_import, and lo_export --- none of which need to be in a transaction
block --- not to mention possibly causing gratuitous failures during
lo_commit.
I'm not convinced that we need such a check at all; I don't see anything
especially wrong with the existing behavior. But if we do want it, this
is the wrong abstraction level. be-fsstubs.c is the place to do it,
and only in the routines that take or return an open-LO descriptor.
regards, tom lane
Re: Re: [PATCHES] Patch to support transactions with BLOBs for current CVS
From
Denis Perchine
Date:
> > On Saturday 20 January 2001 10:05, you wrote: > > > I just wanted to confirm that this patch was applied. > > > > Yes, it is. But the following patch is not applied. But I sure that it is > > neccessary, otherwise we will get really strange errors (see discussion > > in the thread). > > > > http://www.postgresql.org/mhonarc/pgsql-patches/2000-11/msg00013.html > > Can people comment on the following patch that Dennis says is needed? > It prevents BLOB operations outside transactions. Dennis, can you > explain why BLOB operations have to be done inside transactions? If you forget to put BLOB in TX, you will get errors like 'lo_read: invalid large obj descriptor (0)'. The problem is that in be-fsstubs.c in lo_commit all descriptors are removed. And if you did not opened TX, it will be commited after each function call. And for the next call there will be no such fd in the tables. Tom later wrote: > I object strongly. As given, this would break lo_creat, lo_unlink, > lo_import, and lo_export --- none of which need to be in a transaction > block --- not to mention possibly causing gratuitous failures during > lo_commit. First of all it will not break lo_creat, lo_unlink for sure. But we can remove checks from inv_create, and inv_drop. They are not important. At least there will be no strange errors issued. I do not know why do you think there will be any problems with lo_commit. I can not find such reasons. I can not say anything about lo_import/lo_export, as I do not know why they are not inside TX themselves. I am not sure, maybe Tom is right, and we should fix be-fsstubs.c instead. But I do not see any reasons why we not put lo_import, and lo_export in TX. At least this will prevent other backends from reading partially imported BLOBs... -- Sincerely Yours, Denis Perchine ---------------------------------- E-Mail: dyp@perchine.com HomePage: http://www.perchine.com/dyp/ FidoNet: 2:5000/120.5 ----------------------------------
Denis Perchine <dyp@perchine.com> writes:
> First of all it will not break lo_creat, lo_unlink for sure.
lo_creat depends on inv_create followed by inv_close; your patch
proposed to disable both of those outside transaction blocks.
lo_unlink depends on inv_drop, which ditto. Your patch therefore
restricts lo_creat and lo_unlink to be done inside transaction blocks,
which is a new and completely unnecessary restriction that will
doubtless break many existing applications.
> But I do not see any reasons why we not put lo_import, and lo_export in TX.
> At least this will prevent other backends from reading partially imported
> BLOBs...
lo_import and lo_export always execute in a transaction, just like any
other backend operation. There is no need to force them to be done in
a transaction block. If you're not clear about this, perhaps you need
to review the difference between transactions and transaction blocks.
regards, tom lane
Re: Re: [PATCHES] Patch to support transactions with BLOBs for current CVS
From
Denis Perchine
Date:
> > First of all it will not break lo_creat, lo_unlink for sure. > > lo_creat depends on inv_create followed by inv_close; your patch > proposed to disable both of those outside transaction blocks. > lo_unlink depends on inv_drop, which ditto. Your patch therefore > restricts lo_creat and lo_unlink to be done inside transaction blocks, > which is a new and completely unnecessary restriction that will > doubtless break many existing applications. OK.As I already said we can remove checks from inv_create/inv_drop. They are not needed there. > > But I do not see any reasons why we not put lo_import, and lo_export in > > TX. At least this will prevent other backends from reading partially > > imported BLOBs... > > lo_import and lo_export always execute in a transaction, just like any > other backend operation. There is no need to force them to be done in > a transaction block. If you're not clear about this, perhaps you need > to review the difference between transactions and transaction blocks. Hmmm... Where can I read about it? At least which source/header? -- Sincerely Yours, Denis Perchine ---------------------------------- E-Mail: dyp@perchine.com HomePage: http://www.perchine.com/dyp/ FidoNet: 2:5000/120.5 ----------------------------------
Denis Perchine <dyp@perchine.com> writes:
>> lo_import and lo_export always execute in a transaction, just like any
>> other backend operation. There is no need to force them to be done in
>> a transaction block. If you're not clear about this, perhaps you need
>> to review the difference between transactions and transaction blocks.
> Hmmm... Where can I read about it? At least which source/header?
Try src/backend/access/transam/xact.c. The point is that you need a
transaction block only if you need to combine multiple SQL commands
into a single transaction. A standalone command or function call is
still done inside a transaction.
regards, tom lane