Re: [PATCHES] [HACKERS] Patching dblink.c to avoid warning about
Bruce Momjian wrote: Joe Conway wrote: Thanks for the review Tom -- as usual, great suggestions. The attached (simpler) patch makes use of your advice. If there are no objections, I'll apply this tomorrow evening. Looks good. Thanks. Committed. Joe ---(end of broadcast)--- TIP 6: explain analyze is your friend
Re: [PATCHES] [HACKERS] Patching dblink.c to avoid warning about
Joe Conway wrote: > Tom Lane wrote: > > 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. > > Thanks for the review Tom -- as usual, great suggestions. The attached > (simpler) patch makes use of your advice. If there are no objections, > I'll apply this tomorrow evening. Looks good. Thanks. -- Bruce Momjian| http://candle.pha.pa.us pgman@candle.pha.pa.us | (610) 359-1001 + If your life is a hard drive, | 13 Roberts Road + Christ can be your backup.| Newtown Square, Pennsylvania 19073 ---(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
Tom Lane wrote:
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.
Thanks for the review Tom -- as usual, great suggestions. The attached
(simpler) patch makes use of your advice. If there are no objections,
I'll apply this tomorrow evening.
Joe
Index: dblink.c
===
RCS file: /opt/src/cvs/pgsql/contrib/dblink/dblink.c,v
retrieving revision 1.47
diff -c -r1.47 dblink.c
*** dblink.c 15 Oct 2005 02:49:04 - 1.47
--- dblink.c 17 Oct 2005 02:11:59 -
***
*** 60,68
typedef struct remoteConn
{
! PGconn *conn; /* Hold the remote connection */
! int autoXactCursors;/* Indicates the number of open cursors,
! * non-zero means we opened the xact ourselves */
} remoteConn;
/*
--- 60,68
typedef struct remoteConn
{
! PGconn *conn;/* Hold the remote connection */
! int openCursorCount; /* The number of open cursors */
! bool newXactForCursor; /* Opened a transaction for a cursor */
} remoteConn;
/*
***
*** 84,93
static char *generate_relation_name(Oid relid);
/* Global */
! List *res_id = NIL;
! int res_id_index = 0;
! PGconn *persistent_conn = NULL;
! static HTAB *remoteConnHash = NULL;
/*
* Following is list that holds multiple remote connections.
--- 84,91
static char *generate_relation_name(Oid relid);
/* Global */
! static remoteConn *pconn = NULL;
! static HTAB *remoteConnHash = NULL;
/*
* Following is list that holds multiple remote connections.
***
*** 184,189
--- 182,197
} \
} while (0)
+ #define DBLINK_INIT \
+ do { \
+ if (!pconn) \
+ { \
+ pconn = (remoteConn *) MemoryContextAlloc(TopMemoryContext, sizeof(remoteConn)); \
+ pconn->conn = NULL; \
+ pconn->openCursorCount = 0; \
+ pconn->newXactForCursor = FALSE; \
+ } \
+ } while (0)
/*
* Create a persistent connection to another database
***
*** 199,204
--- 207,214
PGconn *conn = NULL;
remoteConn *rconn = NULL;
+ DBLINK_INIT;
+
if (PG_NARGS() == 2)
{
connstr = GET_STR(PG_GETARG_TEXT_P(1));
***
*** 234,240
createNewConnection(connname, rconn);
}
else
! persistent_conn = conn;
PG_RETURN_TEXT_P(GET_TEXT("OK"));
}
--- 244,250
createNewConnection(connname, rconn);
}
else
! pconn->conn = conn;
PG_RETURN_TEXT_P(GET_TEXT("OK"));
}
***
*** 250,255
--- 260,267
remoteConn *rconn = NULL;
PGconn *conn = NULL;
+ DBLINK_INIT;
+
if (PG_NARGS() == 1)
{
conname = GET_STR(PG_GETARG_TEXT_P(0));
***
*** 258,264
conn = rconn->conn;
}
else
! conn = persistent_conn;
if (!conn)
DBLINK_CONN_NOT_AVAIL;
--- 270,276
conn = rconn->conn;
}
else
! conn = pconn->conn;
if (!conn)
DBLINK_CONN_NOT_AVAIL;
***
*** 270,276
pfree(rconn);
}
else
! persistent_conn = NULL;
PG_RETURN_TEXT_P(GET_TEXT("OK"));
}
--- 282,288
pfree(rconn);
}
else
! pconn->conn = NULL;
PG_RETURN_TEXT_P(GET_TEXT("OK"));
}
***
*** 292,303
remoteConn *rconn = NULL;
bool fail = true; /* default to backward compatible behavior */
if (PG_NARGS() == 2)
{
/* text,text */
curname = GET_STR(PG_GETARG_TEXT_P(0));
sql = GET_STR(PG_GETARG_TEXT_P(1));
! conn = persistent_conn;
}
else if (PG_NARGS() == 3)
{
--- 304,317
remoteConn *rconn = NULL;
bool fail = true; /* default to backward compatible behavior */
+ DBLINK_INIT;
+
if (PG_NARGS() == 2)
{
/* text,text */
curname = GET_STR(PG_GETARG_TEXT_P(0));
sql = GET_STR(PG_GETARG_TEXT_P(1));
! rconn = pconn;
}
else if (PG_NARGS() == 3)
{
***
*** 307,313
curname = GET_STR(PG_GETARG_TEXT_P(0));
sql = GET_STR(PG_GETARG_TEXT_P(1));
fail = PG_GETARG_BOOL(2);
! conn = persistent_conn;
}
else
{
--- 321,327
curname = GET_STR(PG_GETARG_TEXT_P(0));
sql = GET_STR(PG_GETARG_TEXT_P(1));
fail = PG_GETARG_BOOL(2);
! rconn = pconn;
}
else
{
***
*** 315,322
curname = GET_STR(PG_GETARG_TEXT_P(1));
sql = GET_STR(PG_GETARG_TEXT_P(2));
rconn = getConnectionByName(conname);
- if (rconn)
- conn = rconn->conn;
}
}
else if (PG_NARGS() == 4)
--- 329,334
***
*** 327,344
sql = GET_STR(PG_GETARG_TEXT_P(2));
fail = PG_GETARG_BOOL(3);
rconn = getConnectionByName(conname);
- if (rconn)
- conn = rconn->conn;
}
! if (!conn)
DBLINK_CONN_
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
Bruce Momjian wrote:
No problem -- thanks. I have slimmed down the patch by applying the
cosmetic parts to CVS. Use the URL above to get the newest versions of
the dblink.c and regression changes.
Here is my counter-proposal to Bruce's dblink patch. Any comments?
Is it too late to apply this for 8.1? I tend to agree with calling this
a bugfix.
Thanks,
Joe
Index: dblink.c
===
RCS file: /opt/src/cvs/pgsql/contrib/dblink/dblink.c,v
retrieving revision 1.47
diff -c -r1.47 dblink.c
*** dblink.c 15 Oct 2005 02:49:04 - 1.47
--- dblink.c 16 Oct 2005 02:04:13 -
***
*** 60,68
typedef struct remoteConn
{
! PGconn *conn; /* Hold the remote connection */
! int autoXactCursors;/* Indicates the number of open cursors,
! * non-zero means we opened the xact ourselves */
} remoteConn;
/*
--- 60,68
typedef struct remoteConn
{
! PGconn *conn;/* Hold the remote connection */
! int openCursorCount; /* The number of open cursors */
! bool newXactForCursor; /* Opened a transaction for a cursor */
} remoteConn;
/*
***
*** 84,93
static char *generate_relation_name(Oid relid);
/* Global */
! List *res_id = NIL;
! int res_id_index = 0;
! PGconn *persistent_conn = NULL;
! static HTAB *remoteConnHash = NULL;
/*
* Following is list that holds multiple remote connections.
--- 84,91
static char *generate_relation_name(Oid relid);
/* Global */
! static remoteConn *pconn = NULL;
! static HTAB *remoteConnHash = NULL;
/*
* Following is list that holds multiple remote connections.
***
*** 184,189
--- 182,197
} \
} while (0)
+ #define DBLINK_INIT \
+ do { \
+ if (!pconn) \
+ { \
+ pconn = (remoteConn *) MemoryContextAlloc(TopMemoryContext, sizeof(remoteConn)); \
+ pconn->conn = NULL; \
+ pconn->openCursorCount = 0; \
+ pconn->newXactForCursor = FALSE; \
+ } \
+ } while (0)
/*
* Create a persistent connection to another database
***
*** 199,204
--- 207,214
PGconn *conn = NULL;
remoteConn *rconn = NULL;
+ DBLINK_INIT;
+
if (PG_NARGS() == 2)
{
connstr = GET_STR(PG_GETARG_TEXT_P(1));
***
*** 234,240
createNewConnection(connname, rconn);
}
else
! persistent_conn = conn;
PG_RETURN_TEXT_P(GET_TEXT("OK"));
}
--- 244,250
createNewConnection(connname, rconn);
}
else
! pconn->conn = conn;
PG_RETURN_TEXT_P(GET_TEXT("OK"));
}
***
*** 250,255
--- 260,267
remoteConn *rconn = NULL;
PGconn *conn = NULL;
+ DBLINK_INIT;
+
if (PG_NARGS() == 1)
{
conname = GET_STR(PG_GETARG_TEXT_P(0));
***
*** 258,264
conn = rconn->conn;
}
else
! conn = persistent_conn;
if (!conn)
DBLINK_CONN_NOT_AVAIL;
--- 270,276
conn = rconn->conn;
}
else
! conn = pconn->conn;
if (!conn)
DBLINK_CONN_NOT_AVAIL;
***
*** 270,276
pfree(rconn);
}
else
! persistent_conn = NULL;
PG_RETURN_TEXT_P(GET_TEXT("OK"));
}
--- 282,288
pfree(rconn);
}
else
! pconn->conn = NULL;
PG_RETURN_TEXT_P(GET_TEXT("OK"));
}
***
*** 285,290
--- 297,304
char *msg;
PGresult *res = NULL;
PGconn *conn = NULL;
+ int *openCursorCount = NULL;
+ bool *newXactForCursor = NULL;
char *curname = NULL;
char *sql = NULL;
char *conname = NULL;
***
*** 292,303
remoteConn *rconn = NULL;
bool fail = true; /* default to backward compatible behavior */
if (PG_NARGS() == 2)
{
/* text,text */
curname = GET_STR(PG_GETARG_TEXT_P(0));
sql = GET_STR(PG_GETARG_TEXT_P(1));
! conn = persistent_conn;
}
else if (PG_NARGS() == 3)
{
--- 306,321
remoteConn *rconn = NULL;
bool fail = true; /* default to backward compatible behavior */
+ DBLINK_INIT;
+
if (PG_NARGS() == 2)
{
/* text,text */
curname = GET_STR(PG_GETARG_TEXT_P(0));
sql = GET_STR(PG_GETARG_TEXT_P(1));
! conn = pconn->conn;
! openCursorCount = &pconn->openCursorCount;
! newXactForCursor = &pconn->newXactForCursor;
}
else if (PG_NARGS() == 3)
{
***
*** 307,313
curname = GET_STR(PG_GETARG_TEXT_P(0));
sql = GET_STR(PG_GETARG_TEXT_P(1));
fail = PG_GETARG_BOOL(2);
! conn = persistent_conn;
}
else
{
--- 325,333
curname = GET_STR(PG_GETARG_TEXT_P(0));
sql = GET_STR(PG_GETARG_TEXT_P(1));
fail = PG_GETARG_BOOL(2);
! conn = pconn->conn;
! openCursorCount = &pconn->openCursorCount;
! newXactForCursor = &pconn->newXactForCursor;
}
else
{
***
*** 316,322
--- 336,346
sql = GET_STR(PG_GETARG_TEXT_P(2));
rconn = getConnectionByName(conname);
if (r
Re: [PATCHES] [HACKERS] Patching dblink.c to avoid warning about open
[ reposted]
Tom Lane wrote:
> Bruce Momjian 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.
I have developed a dblink regression patch that tests for the bug. It
opens a transaction in the client code, opens/closes a cursor, then
tries a DECLARE. Without my patch, this fails because the
dblink_close() closes the transaction, but with my patch it succeeds.
Here is the test:
-- test opening cursor in a transaction
SELECT dblink_exec('myconn','BEGIN');
-- an open transaction will prevent dblink_open() from opening its own
SELECT dblink_open('myconn','rmt_foo_cursor','SELECT * FROM foo');
-- this should not close the cursor because the client opened it
SELECT dblink_close('myconn','rmt_foo_cursor');
-- this should succeed because we have an open transaction
SELECT dblink_exec('myconn','DECLARE xact_test CURSOR FOR SELECT * FROM
foo');
-- commit remote transaction
SELECT dblink_exec('myconn','COMMIT');
Here is the failure reported by the current code:
-- this should succeed because we have an open transaction
SELECT dblink_exec('myconn','DECLARE xact_test CURSOR FOR SELECT * FROM
foo');
ERROR: sql error
DETAIL: ERROR: DECLARE CURSOR may only be used in transaction blocks
There was also a problem in that if two cursors were opened, the first
close would close the transaction. I have fixed that code by changing
the xact variable in to a counter that keeps track of the number of
opened cursors and commits only when they are all closed.
Both the dblink.c patch and the regression patch are at:
ftp://candle.pha.pa.us/pub/postgresql/mypatches
--
Bruce Momjian| http://candle.pha.pa.us
pgman@candle.pha.pa.us | (610) 359-1001
+ If your life is a hard drive, | 13 Roberts Road
+ Christ can be your backup.| Newtown Square, Pennsylvania 19073
---(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
Joe Conway wrote: > Bruce Momjian wrote: > > There was also a problem in that if two cursors were opened, the first > > close would close the transaction. I have fixed that code by changing > > the xact variable in to a counter that keeps track of the number of > > opened cursors and commits only when they are all closed. > > > > Both the dblink.c patch and the regression patch are at: > > > > ftp://candle.pha.pa.us/pub/postgresql/mypatches > > > > OK, I'll take a look, but I won't have time for a couple of days (I'm > not at home -- visiting my dad for his 80th birthday -- and have no > broadband access). No problem -- thanks. I have slimmed down the patch by applying the cosmetic parts to CVS. Use the URL above to get the newest versions of the dblink.c and regression changes. -- Bruce Momjian| http://candle.pha.pa.us pgman@candle.pha.pa.us | (610) 359-1001 + If your life is a hard drive, | 13 Roberts Road + Christ can be your backup.| Newtown Square, Pennsylvania 19073 ---(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
Bruce Momjian wrote: There was also a problem in that if two cursors were opened, the first close would close the transaction. I have fixed that code by changing the xact variable in to a counter that keeps track of the number of opened cursors and commits only when they are all closed. Both the dblink.c patch and the regression patch are at: ftp://candle.pha.pa.us/pub/postgresql/mypatches OK, I'll take a look, but I won't have time for a couple of days (I'm not at home -- visiting my dad for his 80th birthday -- and have no broadband access). Joe ---(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
Tom Lane wrote: > Bruce Momjian writes: > > The problem with not using rconn is that we are not saving the > > transaction status at the _start_ of the cursor open. If we don't do > > that, we have no way of knowing on close if _we_ opened the transaction > > or whether it was opened by the user. > > Oh, I see. In that case the flag is horribly misnamed and miscommented. > Something like "newXactForCursor" would be clearer. OK, renamed to isClientXact. -- Bruce Momjian| http://candle.pha.pa.us pgman@candle.pha.pa.us | (610) 359-1001 + If your life is a hard drive, | 13 Roberts Road + Christ can be your backup.| Newtown Square, Pennsylvania 19073 ---(end of broadcast)--- TIP 6: explain analyze is your friend
Re: [PATCHES] [HACKERS] Patching dblink.c to avoid warning about open transaction
Bruce Momjian writes: > The problem with not using rconn is that we are not saving the > transaction status at the _start_ of the cursor open. If we don't do > that, we have no way of knowing on close if _we_ opened the transaction > or whether it was opened by the user. Oh, I see. In that case the flag is horribly misnamed and miscommented. Something like "newXactForCursor" would be clearer. 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
Tom Lane wrote: > Bruce Momjian 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. The problem with not using rconn is that we are not saving the transaction status at the _start_ of the cursor open. If we don't do that, we have no way of knowing on close if _we_ opened the transaction or whether it was opened by the user. -- Bruce Momjian| http://candle.pha.pa.us pgman@candle.pha.pa.us | (610) 359-1001 + If your life is a hard drive, | 13 Roberts Road + Christ can be your backup.| Newtown Square, Pennsylvania 19073 ---(end of broadcast)--- TIP 3: Have you checked our extensive FAQ? http://www.postgresql.org/docs/faq
Re: [PATCHES] [HACKERS] Patching dblink.c to avoid warning about open transaction
Bruce Momjian 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
Joe Conway wrote: > 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 ... > > > > I had asked on the original simple patch, and I'll ask again now -- why > is this needed? I don't have a clear understanding of the problem that > this (or the earlier) patch is trying to solve. Without that, it is > impossible to say whether it is a bug fix or feature. 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. If a remote transactions has already been started by the user, dblink_open is going to call BEGIN, which is going to fail because there is already an open transaction, right? Here is an crude example: test=> BEGIN; BEGIN test=> begin; WARNING: there is already a transaction in progress BEGIN test=> SELECT 1; ?column? -- 1 (1 row) test=> COMMIT; COMMIT test=> COMMIT; WARNING: there is no transaction in progress COMMIT The BEGIN and COMMIT are only a warning though, but the early COMMIT by the dblink_close() is a serious issue. -- Bruce Momjian| http://candle.pha.pa.us pgman@candle.pha.pa.us | (610) 359-1001 + If your life is a hard drive, | 13 Roberts Road + Christ can be your backup.| Newtown Square, Pennsylvania 19073 ---(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
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 ... I had asked on the original simple patch, and I'll ask again now -- why is this needed? I don't have a clear understanding of the problem that this (or the earlier) patch is trying to solve. Without that, it is impossible to say whether it is a bug fix or feature. Joe ---(end of broadcast)--- TIP 2: Don't 'kill -9' the postmaster
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
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 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
> 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.
--
Bruce Momjian| http://candle.pha.pa.us
pgman@candle.pha.pa.us | (610) 359-1001
+ If your life is a hard drive, | 13 Roberts Road
+ Christ can be your backup.| Newtown Square, Pennsylvania 19073
Index: contrib/dblink/dblink.c
===
RCS file: /cvsroot/pgsql/contrib/dblink/dblink.c,v
retrieving revision 1.43
diff -c -c -r1.43 dblink.c
*** contrib/dblink/dblink.c 30 May 2005 23:09:06 - 1.43
--- contrib/dblink/dblink.c 6 Oct 2005 04:18:34 -
***
*** 60,67
typedef struct remoteConn
{
! PGconn *con;/* Hold the remote connection */
! boolremoteTrFlag; /* Indicates whether or not a
transaction
* on remote
database is in progress */
} remoteConn;
--- 60,67
typedef struct remoteConn
{
! PGconn *conn; /* Hold the remote connection */
! boolremoteXactOpen; /* Indicates whether or not a
transaction
* on remote
database is in progress */
} remoteConn;
***
*** 86,110
/* Global */
List *res_id = NIL;
int res_id_index = 0;
- PGconn *persistent_conn = NULL;
static HTAB *remoteConnHash = NULL;
/*
! Following is list that holds multiple remote connections.
! Calling convention of each dblink function changes to accept
! connection name as the first parameter. The connection list is
! much like ecpg e.g. a mapping between a name and a PGconn object.
*/
typedef struct remoteConnHashEnt
{
charname[NAMEDATALEN];
! remoteConn *rcon;
} remoteConnHashEnt;
/* initial number of connection hashes */
#define NUMCONN 16
/* general utility */
#define GET_TEXT(cstrp) DatumGetTextP(DirectFunctionCall1(textin,
CStringGetDatum(cstrp)))
#define GET_STR(textp) DatumGetCString(DirectFunctionCall1(textout,
PointerGetDatum(textp)))
--- 86,115
/* Global */
List *res_id = NIL;
int res_id_index = 0;
static HTAB *remoteConnHash = NULL;
/*
! *Following is list that holds multiple remote connections.
! *Calling convention of each dblink function changes to accept
! *connection name as the first parameter. The connection list is
! *much like ecpg e.g. a mapping between a name and a PGconn object.
*/
typedef struct remoteConnHashEnt
{
charname[NAMEDATALEN];
! remoteConn *rconn; /* EMPTY_CONNECTION_NAME also possible */
} remoteConnHashEnt;
/* initial number of connection hashes */
#define NUMCONN 16
+ /*
+ *Because the argument protocol is V1, no connection name behaves
+ *the same as a NULL-passed connection name
+ */
+ #define EMPTY_CONNECTION_NAME ""
+
/* general utility */
#define GET_TEXT(cstrp) DatumGetTextP(DirectFunctionCall1(textin,
CStringGetDatum(cstrp)))
#define GET_STR(textp) DatumGetCString(DirectFunctionCall1(textout,
PointerGetDatum(textp)))
***
*** 116,131
var_ = NULL; \
} \
} while (0)
#define DBLINK_RES_INTERNALERROR(p2) \
do { \
! msg = pstrdup(PQerrorMessage(conn)); \
if (res) \
PQclear(res); \
elog(ERROR, "%s: %s", p2, msg); \
} while (0)
#define DBLINK_RES_ERROR(p2) \
do { \
! msg = pstrdup(PQerrorMessage(conn)); \
if (res) \
PQclear(res); \
ereport(ERROR, \
--- 121,138
var_ = NULL;
