[ 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 &#45;0500 (EST)(envelope&#45;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=0 HEIGHT=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>
Sat, 20 Jan 2001 22:51:23 -0500 (EST)
Bruce Momjian <pgman@candle.pha.pa.us>
Re: Re: [PATCHES] Patch to support transactions with BLOBs
	for current CVS
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);
Sat, 20 Jan 2001 22:58:40 -0500 (EST)
Bruce Momjian <pgman@candle.pha.pa.us>
Re: Re: [PATCHES] Patch to support transactions with BLOBs
	for current CVS
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

Sat, 20 Jan 2001 23:39:52 -0500
Tom Lane <tgl@sss.pgh.pa.us>
Re: Re: [PATCHES] Patch to support transactions with BLOBs for
	current CVS
> > 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
----------------------------------

Sun, 21 Jan 2001 12:48:17 +0600
Denis Perchine <dyp@perchine.com>
Re: Re: [PATCHES] Patch to support transactions with BLOBs for
	current CVS
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

Sun, 21 Jan 2001 02:08:07 -0500
Tom Lane <tgl@sss.pgh.pa.us>
Re: Re: [PATCHES] Patch to support transactions with BLOBs for
	current CVS
> > 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
----------------------------------

Sun, 21 Jan 2001 13:09:46 +0600
Denis Perchine <dyp@perchine.com>
Re: Re: [PATCHES] Patch to support transactions with BLOBs for
	current CVS
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

Sun, 21 Jan 2001 13:34:50 -0500
Tom Lane <tgl@sss.pgh.pa.us>
Re: Re: [PATCHES] Patch to support transactions with BLOBs for
	current CVS