| Lists: | pgsql-hackerspgsql-patches |
|---|
| From: | Jonathan Beit-Aharon <jbeitaharon(at)intrusic(dot)com> |
|---|---|
| To: | pgsql-hackers(at)postgresql(dot)org |
| Cc: | Jonathan Beit-Aharon <jbeitaharon(at)intrusic(dot)com> |
| Subject: | Patching dblink.c to avoid warning about open transaction |
| Date: | 2005-09-23 20:26:56 |
| Message-ID: | 43346510.3050405@intrusic.com |
| Views: | Raw Message | Whole Thread | Download mbox | Resend email |
| Lists: | pgsql-hackers pgsql-patches |
<!DOCTYPE html PUBLIC "-//W3C//DTD HTML 4.01 Transitional//EN">
<html>
<head>
<meta content="text/html;charset=ISO-8859-1" http-equiv="Content-Type">
<title></title>
</head>
<body bgcolor="#ffffff" text="#3333ff">
<meta content="text/html;charset=ISO-8859-1" http-equiv="Content-Type">
<title></title>
<font size="+1">Hi,<br>
</font><font size="+1">I'm not a member of this list (yet), so please
CC me on responses and
discussion.<br>
</font><br>
<font size="+1">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.<br>
<br>
I hope someone will find it useful,<br>
Jonathan<br>
<br>
<br>
--- dblink.c Sat Jan 1 00:43:05 2005<br>
+++ /home/jbeitaharon/dev/third/postgreSQL/contrib/dblink/dblink.c
Thu Sep 22 16:10:20 2005<br>
@@ -329,12 +329,16 @@<br>
<br>
if (!conn)<br>
DBLINK_CONN_NOT_AVAIL;<br>
+<br>
+ if (rcon)<br>
+ rcon->remoteTrFlag = (PQtransactionStatus(conn) !=
PQTRANS_IDLE);<br>
<br>
- res = PQexec(conn, "BEGIN");<br>
- if (PQresultStatus(res) != PGRES_COMMAND_OK)<br>
- DBLINK_RES_INTERNALERROR("begin error");<br>
-<br>
- PQclear(res);<br>
+ if ((!rcon) || (!(rcon->remoteTrFlag))) {<br>
+ res = PQexec(conn, "BEGIN");<br>
+ if (PQresultStatus(res) != PGRES_COMMAND_OK)<br>
+ DBLINK_RES_INTERNALERROR("begin error");<br>
+ PQclear(res);<br>
+ }<br>
<br>
appendStringInfo(str, "DECLARE %s CURSOR FOR %s", curname, sql);<br>
res = PQexec(conn, str->data);<br>
@@ -424,12 +428,13 @@<br>
<br>
PQclear(res);<br>
<br>
- /* commit the transaction */<br>
- res = PQexec(conn, "COMMIT");<br>
- if (PQresultStatus(res) != PGRES_COMMAND_OK)<br>
- DBLINK_RES_INTERNALERROR("commit error");<br>
-<br>
- PQclear(res);<br>
+ if ((!rcon) || (!(rcon->remoteTrFlag))) {<br>
+ /* commit the transaction */<br>
+ res = PQexec(conn, "COMMIT");<br>
+ if (PQresultStatus(res) != PGRES_COMMAND_OK)<br>
+ DBLINK_RES_INTERNALERROR("commit error");<br>
+ PQclear(res);<br>
+ }<br>
<br>
PG_RETURN_TEXT_P(GET_TEXT("OK"));<br>
}</font>
</body>
</html>
| Attachment | Content-Type | Size |
|---|---|---|
| unknown_filename | text/html | 3.9 KB |
| From: | Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> |
|---|---|
| To: | Jonathan Beit-Aharon <jbeitaharon(at)intrusic(dot)com> |
| Cc: | pgsql-hackers(at)postgresql(dot)org |
| Subject: | Re: Patching dblink.c to avoid warning about open transaction |
| Date: | 2005-09-23 21:31:37 |
| Message-ID: | 6686.1127511097@sss.pgh.pa.us |
| Views: | Raw Message | Whole Thread | Download mbox | Resend email |
| Lists: | pgsql-hackers pgsql-patches |
Jonathan Beit-Aharon <jbeitaharon(at)intrusic(dot)com> writes:
> if (!conn)<br>
> DBLINK_CONN_NOT_AVAIL;<br>
> +<br>
> + if (rcon)<br>
> + rcon->remoteTrFlag = (PQtransactionStatus(conn) !=
> PQTRANS_IDLE);<br>
> <br>
> - res = PQexec(conn, "BEGIN");<br>
> - if (PQresultStatus(res) != PGRES_COMMAND_OK)<br>
>
> [etc]
Could we see this in a less broken format?
regards, tom lane
| From: | Bruce Momjian <pgman(at)candle(dot)pha(dot)pa(dot)us> |
|---|---|
| To: | Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> |
| Cc: | Jonathan Beit-Aharon <jbeitaharon(at)intrusic(dot)com>, pgsql-hackers(at)postgresql(dot)org |
| Subject: | Re: Patching dblink.c to avoid warning about open transaction |
| Date: | 2005-09-23 21:39:15 |
| Message-ID: | 200509232139.j8NLdFW21624@candle.pha.pa.us |
| Views: | Raw Message | Whole Thread | Download mbox | Resend email |
| Lists: | pgsql-hackers pgsql-patches |
Tom Lane wrote:
> Jonathan Beit-Aharon <jbeitaharon(at)intrusic(dot)com> writes:
> > if (!conn)<br>
> > DBLINK_CONN_NOT_AVAIL;<br>
>
> Could we see this in a less broken format?
Here is the patch in text format.
--
Bruce Momjian | http://candle.pha.pa.us
pgman(at)candle(dot)pha(dot)pa(dot)us | (610) 359-1001
+ If your life is a hard drive, | 13 Roberts Road
+ Christ can be your backup. | Newtown Square, Pennsylvania 19073
| Attachment | Content-Type | Size |
|---|---|---|
| unknown_filename | text/plain | 1.4 KB |
| From: | Bruce Momjian <pgman(at)candle(dot)pha(dot)pa(dot)us> |
|---|---|
| To: | mail(at)joeconway(dot)com |
| Cc: | Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>, Jonathan Beit-Aharon <jbeitaharon(at)intrusic(dot)com>, pgsql-hackers(at)postgresql(dot)org |
| Subject: | Re: Patching dblink.c to avoid warning about open transaction |
| Date: | 2005-09-26 16:58:04 |
| Message-ID: | 200509261658.j8QGw4416917@candle.pha.pa.us |
| Views: | Raw Message | Whole Thread | Download mbox | Resend email |
| Lists: | pgsql-hackers pgsql-patches |
[ Joe, would you review this? ]
Your patch has been added to the PostgreSQL unapplied patches list at:
http://momjian.postgresql.org/cgi-bin/pgpatches
It will be applied as soon as one of the PostgreSQL committers reviews
and approves it.
---------------------------------------------------------------------------
Bruce Momjian wrote:
> Tom Lane wrote:
> > Jonathan Beit-Aharon <jbeitaharon(at)intrusic(dot)com> writes:
> > > if (!conn)<br>
> > > DBLINK_CONN_NOT_AVAIL;<br>
> >
> > Could we see this in a less broken format?
>
> Here is the patch in text format.
>
> --
> Bruce Momjian | http://candle.pha.pa.us
> pgman(at)candle(dot)pha(dot)pa(dot)us | (610) 359-1001
> + If your life is a hard drive, | 13 Roberts Road
> + Christ can be your backup. | Newtown Square, Pennsylvania 19073
> --- dblink.c Sat Jan 1 00:43:05 2005
> +++
> /home/jbeitaharon/dev/third/postgreSQL/contrib/dblink/dblink.c
> Thu Sep 22 16:10:20 2005
> @@ -329,12 +329,16 @@
> if (!conn)
> DBLINK_CONN_NOT_AVAIL;
> +
> + if (rcon)
> + rcon->remoteTrFlag = (PQtransactionStatus(conn) !=
> PQTRANS_IDLE);
> - res = PQexec(conn, "BEGIN");
> - if (PQresultStatus(res) != PGRES_COMMAND_OK)
> - DBLINK_RES_INTERNALERROR("begin error");
> -
> - PQclear(res);
> + if ((!rcon) || (!(rcon->remoteTrFlag))) {
> + res = PQexec(conn, "BEGIN");
> + if (PQresultStatus(res) != PGRES_COMMAND_OK)
> + DBLINK_RES_INTERNALERROR("begin error");
> + PQclear(res);
> + }
> appendStringInfo(str, "DECLARE %s CURSOR FOR %s", curname,
> sql);
> res = PQexec(conn, str->data);
> @@ -424,12 +428,13 @@
> PQclear(res);
> - /* commit the transaction */
> - res = PQexec(conn, "COMMIT");
> - if (PQresultStatus(res) != PGRES_COMMAND_OK)
> - DBLINK_RES_INTERNALERROR("commit error");
> -
> - PQclear(res);
> + if ((!rcon) || (!(rcon->remoteTrFlag))) {
> + /* commit the transaction */
> + res = PQexec(conn, "COMMIT");
> + if (PQresultStatus(res) != PGRES_COMMAND_OK)
> + DBLINK_RES_INTERNALERROR("commit error");
> + PQclear(res);
> + }
> PG_RETURN_TEXT_P(GET_TEXT("OK"));
> }
>
> ---------------------------(end of broadcast)---------------------------
> TIP 5: don't forget to increase your free space map settings
--
Bruce Momjian | http://candle.pha.pa.us
pgman(at)candle(dot)pha(dot)pa(dot)us | (610) 359-1001
+ If your life is a hard drive, | 13 Roberts Road
+ Christ can be your backup. | Newtown Square, Pennsylvania 19073
| From: | Joe Conway <mail(at)joeconway(dot)com> |
|---|---|
| To: | Bruce Momjian <pgman(at)candle(dot)pha(dot)pa(dot)us> |
| Cc: | Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>, Jonathan Beit-Aharon <jbeitaharon(at)intrusic(dot)com>, pgsql-hackers(at)postgresql(dot)org |
| Subject: | Re: Patching dblink.c to avoid warning about open transaction |
| Date: | 2005-09-26 20:37:04 |
| Message-ID: | 43385BF0.1070703@joeconway.com |
| Views: | Raw Message | Whole Thread | Download mbox | Resend email |
| Lists: | pgsql-hackers pgsql-patches |
Bruce Momjian wrote:
> [ Joe, would you review this? ]
>
> Your patch has been added to the PostgreSQL unapplied patches list at:
>
> http://momjian.postgresql.org/cgi-bin/pgpatches
>
> It will be applied as soon as one of the PostgreSQL committers reviews
> and approves it.
The patch itself is pretty simple, but I'm unclear on the use case.
Jonathan, can you elaborate a bit?
Thanks,
Joe
p.s. I'm on a business trip in Asia again, so my responses may be
delayed a bit.
| From: | Jonathan Beit-Aharon <jbeitaharon(at)intrusic(dot)com> |
|---|---|
| To: | Joe Conway <mail(at)joeconway(dot)com> |
| Cc: | Bruce Momjian <pgman(at)candle(dot)pha(dot)pa(dot)us>, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>, pgsql-hackers(at)postgresql(dot)org |
| Subject: | Re: Patching dblink.c to avoid warning about open transaction |
| Date: | 2005-09-27 13:35:04 |
| Message-ID: | 43394A88.2020902@intrusic.com |
| Views: | Raw Message | Whole Thread | Download mbox | Resend email |
| Lists: | pgsql-hackers pgsql-patches |
Joe Conway wrote:
> Bruce Momjian wrote:
>
>> [ Joe, would you review this? ]
>>
>> Your patch has been added to the PostgreSQL unapplied patches list at:
>>
>> http://momjian.postgresql.org/cgi-bin/pgpatches
>>
>> It will be applied as soon as one of the PostgreSQL committers reviews
>> and approves it.
>
>
> The patch itself is pretty simple, but I'm unclear on the use case.
> Jonathan, can you elaborate a bit?
>
> Thanks,
>
> Joe
>
> p.s. I'm on a business trip in Asia again, so my responses may be
> delayed a bit.
Hi Joe,
We are using the dblink module on Sensor servers to provide
summarization services to a Central server. Sensors are in the business
of populating certain Postgres databases, and the Central is in the
business of populating a summary Postgres database. The issue in our
situation is that the Central server does an explicit BEGIN transaction
some time before it calls the dblink_open() routine. On the Sensors, we
were getting many "there is already a transaction in progress" warnings,
and overflowing our log storage. Is this patch the right way to go
about this?
Thanks,
Jonathan
| From: | Bruce Momjian <pgman(at)candle(dot)pha(dot)pa(dot)us> |
|---|---|
| To: | Jonathan Beit-Aharon <jbeitaharon(at)intrusic(dot)com> |
| Cc: | PostgreSQL-patches <pgsql-patches(at)postgresql(dot)org>, Joe Conway <mail(at)joeconway(dot)com> |
| Subject: | Re: [HACKERS] Patching dblink.c to avoid warning about open transaction |
| Date: | 2005-10-07 02:38:54 |
| Message-ID: | 200510070238.j972csp00705@candle.pha.pa.us |
| Views: | Raw Message | Whole Thread | Download mbox | Resend email |
| Lists: | pgsql-hackers pgsql-patches |
> 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(at)candle(dot)pha(dot)pa(dot)us | (610) 359-1001
+ If your life is a hard drive, | 13 Roberts Road
+ Christ can be your backup. | Newtown Square, Pennsylvania 19073
| Attachment | Content-Type | Size |
|---|---|---|
| unknown_filename | text/plain | 27.9 KB |
| From: | David Fetter <david(at)fetter(dot)org> |
|---|---|
| To: | Bruce Momjian <pgman(at)candle(dot)pha(dot)pa(dot)us> |
| Cc: | Jonathan Beit-Aharon <jbeitaharon(at)intrusic(dot)com>, PostgreSQL-patches <pgsql-patches(at)postgresql(dot)org>, Joe Conway <mail(at)joeconway(dot)com> |
| Subject: | Re: [HACKERS] Patching dblink.c to avoid warning about open transaction |
| Date: | 2005-10-07 03:22:04 |
| Message-ID: | 20051007032204.GH30487@fetter.org |
| Views: | Raw Message | Whole Thread | Download mbox | Resend email |
| Lists: | pgsql-hackers pgsql-patches |
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 david(at)fetter(dot)org http://fetter.org/
phone: +1 510 893 6100 mobile: +1 415 235 3778
Remember to vote!
| From: | Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> |
|---|---|
| To: | David Fetter <david(at)fetter(dot)org> |
| Cc: | Bruce Momjian <pgman(at)candle(dot)pha(dot)pa(dot)us>, Jonathan Beit-Aharon <jbeitaharon(at)intrusic(dot)com>, PostgreSQL-patches <pgsql-patches(at)postgresql(dot)org>, Joe Conway <mail(at)joeconway(dot)com> |
| Subject: | Re: [HACKERS] Patching dblink.c to avoid warning about open transaction |
| Date: | 2005-10-07 03:31:46 |
| Message-ID: | 28742.1128655906@sss.pgh.pa.us |
| Views: | Raw Message | Whole Thread | Download mbox | Resend email |
| Lists: | pgsql-hackers pgsql-patches |
David Fetter <david(at)fetter(dot)org> 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
| From: | David Fetter <david(at)fetter(dot)org> |
|---|---|
| To: | Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> |
| Cc: | Bruce Momjian <pgman(at)candle(dot)pha(dot)pa(dot)us>, Jonathan Beit-Aharon <jbeitaharon(at)intrusic(dot)com>, PostgreSQL-patches <pgsql-patches(at)postgresql(dot)org>, Joe Conway <mail(at)joeconway(dot)com> |
| Subject: | Re: [HACKERS] Patching dblink.c to avoid warning about open transaction |
| Date: | 2005-10-07 03:54:13 |
| Message-ID: | 20051007035413.GI30487@fetter.org |
| Views: | Raw Message | Whole Thread | Download mbox | Resend email |
| Lists: | pgsql-hackers pgsql-patches |
On Thu, Oct 06, 2005 at 11:31:46PM -0400, Tom Lane wrote:
> David Fetter <david(at)fetter(dot)org> 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 david(at)fetter(dot)org http://fetter.org/
phone: +1 510 893 6100 mobile: +1 415 235 3778
Remember to vote!
| From: | Joe Conway <mail(at)joeconway(dot)com> |
|---|---|
| To: | Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> |
| Cc: | David Fetter <david(at)fetter(dot)org>, Bruce Momjian <pgman(at)candle(dot)pha(dot)pa(dot)us>, Jonathan Beit-Aharon <jbeitaharon(at)intrusic(dot)com>, PostgreSQL-patches <pgsql-patches(at)postgresql(dot)org> |
| Subject: | Re: [HACKERS] Patching dblink.c to avoid warning about |
| Date: | 2005-10-07 14:38:21 |
| Message-ID: | 4346885D.8010303@joeconway.com |
| Views: | Raw Message | Whole Thread | Download mbox | Resend email |
| Lists: | pgsql-hackers pgsql-patches |
Tom Lane wrote:
> David Fetter <david(at)fetter(dot)org> 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
| From: | Bruce Momjian <pgman(at)candle(dot)pha(dot)pa(dot)us> |
|---|---|
| To: | Joe Conway <mail(at)joeconway(dot)com> |
| Cc: | Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>, David Fetter <david(at)fetter(dot)org>, Jonathan Beit-Aharon <jbeitaharon(at)intrusic(dot)com>, PostgreSQL-patches <pgsql-patches(at)postgresql(dot)org> |
| Subject: | Re: [HACKERS] Patching dblink.c to avoid warning about open |
| Date: | 2005-10-07 14:52:31 |
| Message-ID: | 200510071452.j97EqV223690@candle.pha.pa.us |
| Views: | Raw Message | Whole Thread | Download mbox | Resend email |
| Lists: | pgsql-hackers pgsql-patches |
Joe Conway wrote:
> Tom Lane wrote:
> > David Fetter <david(at)fetter(dot)org> 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(at)candle(dot)pha(dot)pa(dot)us | (610) 359-1001
+ If your life is a hard drive, | 13 Roberts Road
+ Christ can be your backup. | Newtown Square, Pennsylvania 19073
| From: | Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> |
|---|---|
| To: | Bruce Momjian <pgman(at)candle(dot)pha(dot)pa(dot)us> |
| Cc: | Joe Conway <mail(at)joeconway(dot)com>, David Fetter <david(at)fetter(dot)org>, Jonathan Beit-Aharon <jbeitaharon(at)intrusic(dot)com>, PostgreSQL-patches <pgsql-patches(at)postgresql(dot)org> |
| Subject: | Re: [HACKERS] Patching dblink.c to avoid warning about open transaction |
| Date: | 2005-10-07 15:47:28 |
| Message-ID: | 4889.1128700048@sss.pgh.pa.us |
| Views: | Raw Message | Whole Thread | Download mbox | Resend email |
| Lists: | pgsql-hackers pgsql-patches |
Bruce Momjian <pgman(at)candle(dot)pha(dot)pa(dot)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
| From: | Bruce Momjian <pgman(at)candle(dot)pha(dot)pa(dot)us> |
|---|---|
| To: | Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> |
| Cc: | Joe Conway <mail(at)joeconway(dot)com>, David Fetter <david(at)fetter(dot)org>, Jonathan Beit-Aharon <jbeitaharon(at)intrusic(dot)com>, PostgreSQL-patches <pgsql-patches(at)postgresql(dot)org> |
| Subject: | Re: [HACKERS] Patching dblink.c to avoid warning about open |
| Date: | 2005-10-07 15:50:27 |
| Message-ID: | 200510071550.j97FoRp03730@candle.pha.pa.us |
| Views: | Raw Message | Whole Thread | Download mbox | Resend email |
| Lists: | pgsql-hackers pgsql-patches |
Tom Lane wrote:
> Bruce Momjian <pgman(at)candle(dot)pha(dot)pa(dot)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.
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(at)candle(dot)pha(dot)pa(dot)us | (610) 359-1001
+ If your life is a hard drive, | 13 Roberts Road
+ Christ can be your backup. | Newtown Square, Pennsylvania 19073
| From: | Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> |
|---|---|
| To: | Bruce Momjian <pgman(at)candle(dot)pha(dot)pa(dot)us> |
| Cc: | Joe Conway <mail(at)joeconway(dot)com>, David Fetter <david(at)fetter(dot)org>, Jonathan Beit-Aharon <jbeitaharon(at)intrusic(dot)com>, PostgreSQL-patches <pgsql-patches(at)postgresql(dot)org> |
| Subject: | Re: [HACKERS] Patching dblink.c to avoid warning about open transaction |
| Date: | 2005-10-07 15:59:53 |
| Message-ID: | 5021.1128700793@sss.pgh.pa.us |
| Views: | Raw Message | Whole Thread | Download mbox | Resend email |
| Lists: | pgsql-hackers pgsql-patches |
Bruce Momjian <pgman(at)candle(dot)pha(dot)pa(dot)us> 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
| From: | Bruce Momjian <pgman(at)candle(dot)pha(dot)pa(dot)us> |
|---|---|
| To: | Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> |
| Cc: | Joe Conway <mail(at)joeconway(dot)com>, David Fetter <david(at)fetter(dot)org>, Jonathan Beit-Aharon <jbeitaharon(at)intrusic(dot)com>, PostgreSQL-patches <pgsql-patches(at)postgresql(dot)org> |
| Subject: | Re: [HACKERS] Patching dblink.c to avoid warning about open |
| Date: | 2005-10-07 16:05:16 |
| Message-ID: | 200510071605.j97G5Gj06255@candle.pha.pa.us |
| Views: | Raw Message | Whole Thread | Download mbox | Resend email |
| Lists: | pgsql-hackers pgsql-patches |
Tom Lane wrote:
> Bruce Momjian <pgman(at)candle(dot)pha(dot)pa(dot)us> 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(at)candle(dot)pha(dot)pa(dot)us | (610) 359-1001
+ If your life is a hard drive, | 13 Roberts Road
+ Christ can be your backup. | Newtown Square, Pennsylvania 19073