Re: [PATCHES] [GENERAL] dblink: rollback transaction
Joe, I applied the patch to 7.4.1 on RH Linux 8.0 and it works great. Thanks. Oleg -Original Message- From: Joe Conway [mailto:[EMAIL PROTECTED] Sent: Thursday, March 04, 2004 11:12 PM To: [EMAIL PROTECTED] Cc: Tom Lane; Oleg Lebedev Subject: Re: [PATCHES] [GENERAL] dblink: rollback transaction Joe Conway wrote: Tom Lane wrote: Joe Conway [EMAIL PROTECTED] writes: I like the idea in general, but maybe instead there should be a new overloaded version of the existing function names that accepts an additional bool argument. Without the argument, behavior would be as it is now; with it, you could specify the old or new behavior. Um, maybe I'm confused about the context, but aren't we talking about C function names here? No overloading is possible in C ... I was thinking in terms of overloaded SQL function names. For example, in addition to dblink_exec(text) and dblink_exec(text,text) we create dblink_exec(text,bool) and dblink_exec(text,text,bool). Currently both SQL versions of dblink_exec are implemented by a single C level function. But yes, we'd need another C level function to support the new SQL functions because there would be no way to distinguish the 2 two-argument versions otherwise. (Actually, now I'm wondering if we could use a single C function for all four SQL versions -- between PG_NARGS() and get_fn_expr_argtype() we should be able to figure out how we were called, shouldn't we?) The attached implements the new overloaded SQL functions as discussed above (i.e. start with existing argument combinations, add a new bool argument to each). I ended up with a single C function (by making use of number and type of the arguments) for each overloaded SQL function name. I'll commit in a day or two if there are no objections. Thanks, Joe * This e-mail may contain privileged or confidential material intended for the named recipient only. If you are not the named recipient, delete this message and all attachments. Unauthorized reviewing, copying, printing, disclosing, or otherwise using information in this e-mail is prohibited. We reserve the right to monitor e-mail sent through our network. * ---(end of broadcast)--- TIP 8: explain analyze is your friend
Re: [PATCHES] [GENERAL] dblink: rollback transaction
Joe Conway wrote: Tom Lane wrote: Joe Conway [EMAIL PROTECTED] writes: I like the idea in general, but maybe instead there should be a new overloaded version of the existing function names that accepts an additional bool argument. Without the argument, behavior would be as it is now; with it, you could specify the old or new behavior. Um, maybe I'm confused about the context, but aren't we talking about C function names here? No overloading is possible in C ... I was thinking in terms of overloaded SQL function names. For example, in addition to dblink_exec(text) and dblink_exec(text,text) we create dblink_exec(text,bool) and dblink_exec(text,text,bool). Currently both SQL versions of dblink_exec are implemented by a single C level function. But yes, we'd need another C level function to support the new SQL functions because there would be no way to distinguish the 2 two-argument versions otherwise. (Actually, now I'm wondering if we could use a single C function for all four SQL versions -- between PG_NARGS() and get_fn_expr_argtype() we should be able to figure out how we were called, shouldn't we?) The attached implements the new overloaded SQL functions as discussed above (i.e. start with existing argument combinations, add a new bool argument to each). I ended up with a single C function (by making use of number and type of the arguments) for each overloaded SQL function name. I'll commit in a day or two if there are no objections. Thanks, Joe Index: contrib/dblink/README.dblink === RCS file: /cvsroot/pgsql-server/contrib/dblink/README.dblink,v retrieving revision 1.9 diff -c -r1.9 README.dblink *** contrib/dblink/README.dblink4 Aug 2003 23:59:37 - 1.9 --- contrib/dblink/README.dblink5 Mar 2004 05:55:45 - *** *** 30,42 * */ - Version 0.6 (14 June, 2003): - Completely removed previously deprecated functions. Added ability - to create named persistent connections in addition to the single global - unnamed persistent connection. - Tested under Linux (Red Hat 9) and PostgreSQL 7.4devel. - Release Notes: Version 0.6 - functions deprecated in 0.5 have been removed - added ability to create named persistent connections --- 30,40 * */ Release Notes: + Version 0.7 (as of 25 Feb, 2004) + - Added new version of dblink, dblink_exec, dblink_open, dblink_close, + and, dblink_fetch -- allows ERROR on remote side of connection to + throw NOTICE locally instead of ERROR Version 0.6 - functions deprecated in 0.5 have been removed - added ability to create named persistent connections *** *** 85,91 You can use dblink.sql to create the functions in your database of choice, e.g. ! psql -U postgres template1 dblink.sql installs following functions into database template1: --- 83,89 You can use dblink.sql to create the functions in your database of choice, e.g. ! psql template1 dblink.sql installs following functions into database template1: *** *** 104,143 cursor ! dblink_open(text,text) RETURNS text - opens a cursor using unnamed connection already opened with dblink_connect() that will persist for duration of current backend or until it is closed ! dblink_open(text,text,text) RETURNS text - opens a cursor using a named connection already opened with dblink_connect() that will persist for duration of current backend or until it is closed ! dblink_fetch(text, int) RETURNS setof record - fetches data from an already opened cursor on the unnamed connection ! dblink_fetch(text, text, int) RETURNS setof record - fetches data from an already opened cursor on a named connection ! dblink_close(text) RETURNS text - closes a cursor on the unnamed connection ! dblink_close(text,text) RETURNS text - closes a cursor on a named connection query ! dblink(text,text) RETURNS setof record - returns a set of results from remote SELECT query; the first argument is either a connection string, or the name of an already opened persistant connection ! dblink(text) RETURNS setof record - returns a set of results from remote SELECT query, using the unnamed connection already opened with dblink_connect() execute ! dblink_exec(text, text) RETURNS text - executes an INSERT/UPDATE/DELETE query remotely; the first argument is either a connection string, or the name of an already opened persistant connection ! dblink_exec(text) RETURNS text - executes an INSERT/UPDATE/DELETE query remotely, using connection already
Re: [PATCHES] [GENERAL] dblink: rollback transaction
Tom Lane wrote: Joe Conway [EMAIL PROTECTED] writes: One question that I'd like some feedback on is the following: should the same change be applied to other functions that might throw an ERROR based on the remote side of the connection? For example, currently if dblink() is used in an attempt to access a non-existent remote table, an ERROR is thrown locally in response, killing any currently open transaction. Thoughts? What seems like a good idea after a few moments' thought is to leave the behavior of the various dblink_foo() functions the same as now (ie, throw error on remote error) and add new API functions named something like dblink_foo_noerror() that don't throw error but return a recognizable failure code instead. My argument for this approach is that there is no situation in which the programmer shouldn't have to think when he writes a given call whether it will elog or return an error indicator, because if he wants an error indicator then he is going to have to check for it. I like the idea in general, but maybe instead there should be a new overloaded version of the existing function names that accepts an additional bool argument. Without the argument, behavior would be as it is now; with it, you could specify the old or new behavior. Joe ---(end of broadcast)--- TIP 7: don't forget to increase your free space map settings
Re: [PATCHES] [GENERAL] dblink: rollback transaction
Joe Conway [EMAIL PROTECTED] writes: I like the idea in general, but maybe instead there should be a new overloaded version of the existing function names that accepts an additional bool argument. Without the argument, behavior would be as it is now; with it, you could specify the old or new behavior. Um, maybe I'm confused about the context, but aren't we talking about C function names here? No overloading is possible in C ... regards, tom lane ---(end of broadcast)--- TIP 1: subscribe and unsubscribe commands go to [EMAIL PROTECTED]
Re: [PATCHES] [GENERAL] dblink: rollback transaction
Tom Lane wrote: Joe Conway [EMAIL PROTECTED] writes: I like the idea in general, but maybe instead there should be a new overloaded version of the existing function names that accepts an additional bool argument. Without the argument, behavior would be as it is now; with it, you could specify the old or new behavior. Um, maybe I'm confused about the context, but aren't we talking about C function names here? No overloading is possible in C ... I was thinking in terms of overloaded SQL function names. For example, in addition to dblink_exec(text) and dblink_exec(text,text) we create dblink_exec(text,bool) and dblink_exec(text,text,bool). Currently both SQL versions of dblink_exec are implemented by a single C level function. But yes, we'd need another C level function to support the new SQL functions because there would be no way to distinguish the 2 two-argument versions otherwise. (Actually, now I'm wondering if we could use a single C function for all four SQL versions -- between PG_NARGS() and get_fn_expr_argtype() we should be able to figure out how we were called, shouldn't we?) Joe ---(end of broadcast)--- TIP 7: don't forget to increase your free space map settings
Re: [PATCHES] [GENERAL] dblink: rollback transaction
Oleg Lebedev wrote:
Your fix is awesome! That's exactly what I need.
What version of postgres do I need to have installed to try this patch?
I am on 7.3 now.
I plan to apply the attached to cvs tip in 24 hours or so. I don't think
it qualifies as a bug fix, and it does represent a change in user facing
behavior, so I do not intend to apply for 7.3.6 or 7.4.2.
The patch changes dblink_exec() such that an ERROR on the remote side of
the connection will generate a NOTICE on the local side, with the
dblink_exec() return value set to 'ERROR'. This allows the remote side
ERROR to be trapped and handled locally.
One question that I'd like some feedback on is the following: should the
same change be applied to other functions that might throw an ERROR
based on the remote side of the connection? For example, currently if
dblink() is used in an attempt to access a non-existent remote table, an
ERROR is thrown locally in response, killing any currently open
transaction. Thoughts?
Thanks,
Joe
Index: contrib/dblink/dblink.c
===
RCS file: /opt/src/cvs/pgsql-server/contrib/dblink/dblink.c,v
retrieving revision 1.29
diff -c -r1.29 dblink.c
*** contrib/dblink/dblink.c 28 Nov 2003 05:03:01 - 1.29
--- contrib/dblink/dblink.c 5 Feb 2004 19:49:00 -
***
*** 135,140
--- 135,150
errmsg(%s, p2), \
errdetail(%s, msg))); \
} while (0)
+ #define DBLINK_RES_ERROR_AS_NOTICE(p2) \
+ do { \
+ msg = pstrdup(PQerrorMessage(conn)); \
+ if (res) \
+ PQclear(res); \
+ ereport(NOTICE, \
+ (errcode(ERRCODE_SYNTAX_ERROR), \
+errmsg(%s, p2), \
+errdetail(%s, msg))); \
+ } while (0)
#define DBLINK_CONN_NOT_AVAIL \
do { \
if(conname) \
***
*** 731,739
if (!res ||
(PQresultStatus(res) != PGRES_COMMAND_OK
PQresultStatus(res) != PGRES_TUPLES_OK))
! DBLINK_RES_ERROR(sql error);
! if (PQresultStatus(res) == PGRES_COMMAND_OK)
{
/* need a tuple descriptor representing one TEXT column */
tupdesc = CreateTemplateTupleDesc(1, false);
--- 741,762
if (!res ||
(PQresultStatus(res) != PGRES_COMMAND_OK
PQresultStatus(res) != PGRES_TUPLES_OK))
! {
! DBLINK_RES_ERROR_AS_NOTICE(sql error);
!
! /* need a tuple descriptor representing one TEXT column */
! tupdesc = CreateTemplateTupleDesc(1, false);
! TupleDescInitEntry(tupdesc, (AttrNumber) 1, status,
! TEXTOID, -1, 0, false);
! /*
!* and save a copy of the command status string to return as our
!* result tuple
!*/
! sql_cmd_status = GET_TEXT(ERROR);
!
! }
! else if (PQresultStatus(res) == PGRES_COMMAND_OK)
{
/* need a tuple descriptor representing one TEXT column */
tupdesc = CreateTemplateTupleDesc(1, false);
---(end of broadcast)---
TIP 1: subscribe and unsubscribe commands go to [EMAIL PROTECTED]
