Re: [PATCHES] [HACKERS] Patching dblink.c to avoid warning about open transaction
Joe Conway [EMAIL PROTECTED] writes: Here is my counter-proposal to Bruce's dblink patch. Any comments? Minor coding suggestion: to me it seems messy to do + int*openCursorCount = NULL; + bool *newXactForCursor = NULL; ! openCursorCount = pconn-openCursorCount; ! newXactForCursor = pconn-newXactForCursor; ! *newXactForCursor = TRUE; This looks a bit cluttered already, and would get more so if you need to add more fields to a remoteConn. Plus it confuses the reader (at least this reader) who is left wondering if you intend that those variables might sometimes point to something other than two fields of the same remoteConn. I think it would be shorter and clearer to write remoteConn *remconn = NULL; ... remconn = rconn; ... remconn-newXactForCursor = TRUE; Also, you might be able to combine this variable with the existing rconn local variable and thus simplify the code even more. Is it too late to apply this for 8.1? I tend to agree with calling this a bugfix. I think it's reasonable to fix now, yes. regards, tom lane ---(end of broadcast)--- TIP 1: if posting/reading through Usenet, please send an appropriate subscribe-nomail command to [EMAIL PROTECTED] so that your message can get through to the mailing list cleanly
Re: [PATCHES] [HACKERS] Patching dblink.c to avoid warning about open transaction
Bruce Momjian pgman@candle.pha.pa.us writes: Well, as I said in the patch email: The reported problem is that dblink_open/dblink_close() (for cursor reads) do a BEGIN/COMMIT regardless of the transaction state of the remote connection. There was code in dblink.c to track the remote transaction state (rconn), but it was not being maintained or used. You should lose the remoteXactOpen flag entirely, in favor of just testing PQtransactionStatus() on-the-fly when necessary. Simpler, more reliable, not notably slower. With that change, the separate remoteConn struct could be dropped altogether in favor of just using the PGconn pointer. This would make things notationally simpler, and in fact perhaps allow undoing the bulk of the edits in your patch. As-is I think the patch is pretty risky to apply during beta. regards, tom lane ---(end of broadcast)--- TIP 4: Have you searched our list archives? http://archives.postgresql.org
Re: [PATCHES] [HACKERS] Patching dblink.c to avoid warning about open transaction
On Thu, Oct 06, 2005 at 10:38:54PM -0400, Bruce Momjian wrote: I'm not a member of this list (yet), so please CC me on responses and discussion. The patch below seems to be completion of work already started, because the boolean remoteTrFlag was already defined, and all I had to add was its setting and two references. I hope someone will find it useful, Jonathan I have worked on this issue and have an extensive patch to dblink to fix it. The reported problem is that dblink_open/dblink_close() (for cursor reads) do a BEGIN/COMMIT regardless of the transaction state of the remote connection. There was code in dblink.c to track the remote transaction state (rconn), but it was not being maintained or used. This patch fixes that by routing all connections through an rconn structure and using the transaction status properly. I removed the global persistent connection and function-local 'conn' structures in favor of using rconn consistently. This cleans up a lot of error-prone code that tried to track what type of connection was being used. I don't know if people want this for 8.1 or 8.2. 8.1, IMHO. It's a bug fix. :) Cheers, D -- David Fetter [EMAIL PROTECTED] http://fetter.org/ phone: +1 510 893 6100 mobile: +1 415 235 3778 Remember to vote! ---(end of broadcast)--- TIP 9: In versions below 8.0, the planner will ignore your desire to choose an index scan if your joining column's datatypes do not match
Re: [PATCHES] [HACKERS] Patching dblink.c to avoid warning about open transaction
David Fetter [EMAIL PROTECTED] writes: On Thu, Oct 06, 2005 at 10:38:54PM -0400, Bruce Momjian wrote: I don't know if people want this for 8.1 or 8.2. 8.1, IMHO. It's a bug fix. :) Not unless Joe Conway signs off on it ... regards, tom lane ---(end of broadcast)--- TIP 4: Have you searched our list archives? http://archives.postgresql.org
Re: [PATCHES] [HACKERS] Patching dblink.c to avoid warning about open transaction
On Thu, Oct 06, 2005 at 11:31:46PM -0400, Tom Lane wrote: David Fetter [EMAIL PROTECTED] writes: On Thu, Oct 06, 2005 at 10:38:54PM -0400, Bruce Momjian wrote: I don't know if people want this for 8.1 or 8.2. 8.1, IMHO. It's a bug fix. :) Not unless Joe Conway signs off on it ... Of course it's his to sign or not :) Cheers, D -- David Fetter [EMAIL PROTECTED] http://fetter.org/ phone: +1 510 893 6100 mobile: +1 415 235 3778 Remember to vote! ---(end of broadcast)--- TIP 4: Have you searched our list archives? http://archives.postgresql.org
