Lists: | pgsql-bugspgsql-hackers |
---|
From: | "Oleksiy Shchukin" <Oleksiy(dot)Shchukin(at)globallogic(dot)com> |
---|---|
To: | pgsql-bugs(at)postgresql(dot)org |
Subject: | BUG #4599: bugfix for contrib/dblink module |
Date: | 2008-12-31 10:14:58 |
Message-ID: | 200812311014.mBVAEwia032307@wwwmaster.postgresql.org |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Lists: | pgsql-bugs pgsql-hackers |
The following bug has been logged online:
Bug reference: 4599
Logged by: Oleksiy Shchukin
Email address: Oleksiy(dot)Shchukin(at)globallogic(dot)com
PostgreSQL version: 8.3.5
Operating system: all
Description: bugfix for contrib/dblink module
Details:
Description
-----------
Function dblink_get_result(text connname [, bool fail_on_error]) from
contrib/dblink module ignores 2nd argument.
Reproduce how-to
----------------
I send bad SQL statement via dblink_send_query(text,text), and than try to
grab result with dblink_get_result(connname, /*fail_on_error=*/ false), the
bad sql statement error is issued on client side inspite of
'/*fail_on_error=*/ false' parameter is passed to dblink_get_result()
dblink.c fix
------------
In dblink.c:785 2nd argument for dblink_get_result(text,bool) is referenced
as 'PG_GETARG_BOOL(2)', must be 'PG_GETARG_BOOL(1)'.
I have already compiled and tested it, works fine for me.
Annotated dblink.c from HEAD
(http://anoncvs.postgresql.org/cvsweb.cgi/pgsql/contrib/dblink/dblink.c?anno
tate=1.76) states that commiter is Joe, please, pass him the bug fix.
Great thanks for your excellect work! :)
Oleksiy
From: | Joe Conway <mail(at)joeconway(dot)com> |
---|---|
To: | Oleksiy Shchukin <Oleksiy(dot)Shchukin(at)globallogic(dot)com> |
Cc: | pgsql-bugs(at)postgresql(dot)org |
Subject: | Re: BUG #4599: bugfix for contrib/dblink module |
Date: | 2009-01-03 01:43:42 |
Message-ID: | 495EC2CE.4050808@joeconway.com |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Lists: | pgsql-bugs pgsql-hackers |
Oleksiy Shchukin wrote:
> The following bug has been logged online:
>
> Bug reference: 4599
> Logged by: Oleksiy Shchukin
> Email address: Oleksiy(dot)Shchukin(at)globallogic(dot)com
> PostgreSQL version: 8.3.5
> Operating system: all
> Description: bugfix for contrib/dblink module
[snip]
> dblink.c fix
> ------------
> In dblink.c:785 2nd argument for dblink_get_result(text,bool) is referenced
> as 'PG_GETARG_BOOL(2)', must be 'PG_GETARG_BOOL(1)'.
This looks like a correct assessment. Will fix...
Thanks!
Joe
From: | Joe Conway <mail(at)joeconway(dot)com> |
---|---|
To: | Oleksiy Shchukin <Oleksiy(dot)Shchukin(at)globallogic(dot)com> |
Cc: | "Hackers (PostgreSQL)" <pgsql-hackers(at)postgresql(dot)org> |
Subject: | Re: [BUGS] BUG #4599: bugfix for contrib/dblink module |
Date: | 2009-01-03 03:55:37 |
Message-ID: | 495EE1B9.9010004@joeconway.com |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Lists: | pgsql-bugs pgsql-hackers |
Oleksiy Shchukin wrote:
> Reproduce how-to
> ----------------
> I send bad SQL statement via dblink_send_query(text,text), and than try to
> grab result with dblink_get_result(connname, /*fail_on_error=*/ false), the
> bad sql statement error is issued on client side inspite of
> '/*fail_on_error=*/ false' parameter is passed to dblink_get_result()
On line 795 below, fail should get set to PG_GETARG_BOOL(1). However, as
line 842 is about to be executed, fail is still set to true, even though
PG_GETARG_BOOL(1) is clearly false. Any ideas?
8<-----------------------------------------------------------
788 else if (is_async && do_get)
(gdb)
791 if (PG_NARGS() == 2)
(gdb)
794 DBLINK_GET_CONN;
(gdb)
795 fail = PG_GETARG_BOOL(1);
(gdb)
819 if (!conn)
(gdb)
822 if (!is_async || (is_async && do_get))
(gdb)
825 if (!is_async)
(gdb)
829 res = PQgetResult(conn);
(gdb)
831 if (!res)
(gdb)
838 if (!res ||
(gdb)
842 dblink_res_error(conname, res,
"could not execute query", fail);
(gdb) p fail
$8 = 1 '\001'
(gdb) print PG_GETARG_BOOL(1)
$9 = 0 '\0'
8<-----------------------------------------------------------
Joe
From: | Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> |
---|---|
To: | Joe Conway <mail(at)joeconway(dot)com> |
Cc: | Oleksiy Shchukin <Oleksiy(dot)Shchukin(at)globallogic(dot)com>, "Hackers (PostgreSQL)" <pgsql-hackers(at)postgresql(dot)org> |
Subject: | Re: [BUGS] BUG #4599: bugfix for contrib/dblink module |
Date: | 2009-01-03 15:26:18 |
Message-ID: | 28719.1230996378@sss.pgh.pa.us |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Lists: | pgsql-bugs pgsql-hackers |
Joe Conway <mail(at)joeconway(dot)com> writes:
> On line 795 below, fail should get set to PG_GETARG_BOOL(1). However, as
> line 842 is about to be executed, fail is still set to true, even though
> PG_GETARG_BOOL(1) is clearly false. Any ideas?
I can't duplicate that here, but my first reaction on studying this code
is "ick!". Having a non-set-returning function calling the SRF
infrastructure (and not bothering to clean it up on exit, either) is
just horrid --- I have no idea what side-effects that might have, but at
the very least there's going to be a memory leak. Trying to implement
three significantly different functions as one function with a maze of
if's is not good style in any case.
I think you should break those three functions apart. There is no value
in having send_query share any code with the others. It might be
feasible to have the other two share a subroutine that collects the
result data.
regards, tom lane
From: | Joe Conway <mail(at)joeconway(dot)com> |
---|---|
To: | Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> |
Cc: | Oleksiy Shchukin <Oleksiy(dot)Shchukin(at)globallogic(dot)com>, "Hackers (PostgreSQL)" <pgsql-hackers(at)postgresql(dot)org> |
Subject: | Re: [BUGS] BUG #4599: bugfix for contrib/dblink module |
Date: | 2009-01-03 20:00:22 |
Message-ID: | 495FC3D6.5030801@joeconway.com |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Lists: | pgsql-bugs pgsql-hackers |
Tom Lane wrote:
> I think you should break those three functions apart. There is no value
> in having send_query share any code with the others. It might be
> feasible to have the other two share a subroutine that collects the
> result data.
OK, will do. I applied the simple fix to the 8.2 and 8.3 branches, but
will do refactoring per your suggestion on cvs tip.
Thanks,
Joe