Re: [HACKERS] Patching dblink.c to avoid warning about open

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

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(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

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

Joe Conway <mail(at)joeconway(dot)com> 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

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

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(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

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