Fix postgres_fdw's issues with inconsistent interpretation of data values.

tglsfdc · ashcmd · commit d651c47681cd · 2013-05-30T13:53:03.000-07:00
For datatypes whose output formatting depends on one or more GUC settings, we have to worry about whether the other server will interpret the value the same way it was meant. pg_dump has been aware of this hazard for a long time, but postgres_fdw needs to deal with it too. To fix data retrieval from the remote server, set the necessary remote GUC settings at connection startup. (We were already assuming that settings made then would persist throughout the remote session.) To fix data transmission to the remote server, temporarily force the relevant GUCs to the right values when we're about to convert any data values to text for transmission. This is all pretty grotty, and not very cheap either. It's tempting to think of defining one uber-GUC that would override any settings that might render printed data values unportable. But of course, older remote servers wouldn't know any such thing and would still need this logic. While at it, revert commit f7951ee, since this provides a real fix. (The timestamptz given in the error message returned from the "remote" server will now reliably be shown in UTC.)
diff --git a/contrib/postgres_fdw/connection.c b/contrib/postgres_fdw/connection.c
@@ -64,6 +64,7 @@ static bool xact_got_connection = false;
 static PGconn *connect_pg_server(ForeignServer *server, UserMapping *user);
 static void check_conn_params(const char **keywords, const char **values);
 static void configure_remote_session(PGconn *conn);
+static void do_sql_command(PGconn *conn, const char *sql);
 static void begin_remote_xact(ConnCacheEntry *entry);
 static void pgfdw_xact_callback(XactEvent event, void *arg);
 static void pgfdw_subxact_callback(SubXactEvent event,
@@ -299,11 +300,43 @@ check_conn_params(const char **keywords, const char **values)
 static void
 configure_remote_session(PGconn *conn)
 {
-	const char *sql;
-	PGresult   *res;
+	int			remoteversion = PQserverVersion(conn);
 
 	/* Force the search path to contain only pg_catalog (see deparse.c) */
-	sql = "SET search_path = pg_catalog";
+	do_sql_command(conn, "SET search_path = pg_catalog");
+
+	/*
+	 * Set remote timezone; this is basically just cosmetic, since all
+	 * transmitted and returned timestamptzs should specify a zone explicitly
+	 * anyway.	However it makes the regression test outputs more predictable.
+	 *
+	 * We don't risk setting remote zone equal to ours, since the remote
+	 * server might use a different timezone database.
+	 */
+	do_sql_command(conn, "SET timezone = UTC");
+
+	/*
+	 * Set values needed to ensure unambiguous data output from remote.  (This
+	 * logic should match what pg_dump does.  See also set_transmission_modes
+	 * in postgres_fdw.c.)
+	 */
+	do_sql_command(conn, "SET datestyle = ISO");
+	if (remoteversion >= 80400)
+		do_sql_command(conn, "SET intervalstyle = postgres");
+	if (remoteversion >= 90000)
+		do_sql_command(conn, "SET extra_float_digits = 3");
+	else
+		do_sql_command(conn, "SET extra_float_digits = 2");
+}
+
+/*
+ * Convenience subroutine to issue a non-data-returning SQL command to remote
+ */
+static void
+do_sql_command(PGconn *conn, const char *sql)
+{
+	PGresult   *res;
+
 	res = PQexec(conn, sql);
 	if (PQresultStatus(res) != PGRES_COMMAND_OK)
 		pgfdw_report_error(ERROR, res, true, sql);
@@ -324,7 +357,6 @@ static void
 begin_remote_xact(ConnCacheEntry *entry)
 {
 	int			curlevel = GetCurrentTransactionNestLevel();
-	PGresult   *res;
 
 	/* Start main transaction if we haven't yet */
 	if (entry->xact_depth <= 0)
@@ -338,10 +370,7 @@ begin_remote_xact(ConnCacheEntry *entry)
 			sql = "START TRANSACTION ISOLATION LEVEL SERIALIZABLE";
 		else
 			sql = "START TRANSACTION ISOLATION LEVEL REPEATABLE READ";
-		res = PQexec(entry->conn, sql);
-		if (PQresultStatus(res) != PGRES_COMMAND_OK)
-			pgfdw_report_error(ERROR, res, true, sql);
-		PQclear(res);
+		do_sql_command(entry->conn, sql);
 		entry->xact_depth = 1;
 	}
 
@@ -355,10 +384,7 @@ begin_remote_xact(ConnCacheEntry *entry)
 		char		sql[64];
 
 		snprintf(sql, sizeof(sql), "SAVEPOINT s%d", entry->xact_depth + 1);
-		res = PQexec(entry->conn, sql);
-		if (PQresultStatus(res) != PGRES_COMMAND_OK)
-			pgfdw_report_error(ERROR, res, true, sql);
-		PQclear(res);
+		do_sql_command(entry->conn, sql);
 		entry->xact_depth++;
 	}
 }
@@ -475,11 +501,8 @@ pgfdw_xact_callback(XactEvent event, void *arg)
 		switch (event)
 		{
 			case XACT_EVENT_COMMIT:
-				/* Commit all remote transactions during pre-commit */
-				res = PQexec(entry->conn, "COMMIT TRANSACTION");
-				if (PQresultStatus(res) != PGRES_COMMAND_OK)
-					pgfdw_report_error(ERROR, res, true, "COMMIT TRANSACTION");
-				PQclear(res);
+				/* Commit all remote transactions */
+				do_sql_command(entry->conn, "COMMIT TRANSACTION");
 				break;
 			case XACT_EVENT_PREPARE:
 				/* Should not get here -- pre-commit should have handled it */
@@ -564,15 +587,15 @@ pgfdw_subxact_callback(SubXactEvent event, SubTransactionId mySubid,
 			elog(ERROR, "missed cleaning up remote subtransaction at level %d",
 				 entry->xact_depth);
 
-                /* Rollback all remote subtransactions during abort */
-                snprintf(sql, sizeof(sql),
-                         "ROLLBACK TO SAVEPOINT s%d; RELEASE SAVEPOINT s%d",
-                         curlevel, curlevel);
-                res = PQexec(entry->conn, sql);
-                if (PQresultStatus(res) != PGRES_COMMAND_OK)
-                    pgfdw_report_error(WARNING, res, true, sql);
-                else
-                    PQclear(res);
+		/* Rollback all remote subtransactions during abort */
+		snprintf(sql, sizeof(sql),
+			 "ROLLBACK TO SAVEPOINT s%d; RELEASE SAVEPOINT s%d",
+			 curlevel, curlevel);
+		res = PQexec(entry->conn, sql);
+		if (PQresultStatus(res) != PGRES_COMMAND_OK)
+			pgfdw_report_error(WARNING, res, true, sql);
+		else
+			PQclear(res);
 
 		/* OK, we're outta that level of subtransaction */
 		entry->xact_depth--;
diff --git a/contrib/postgres_fdw/deparse.c b/contrib/postgres_fdw/deparse.c
@@ -436,8 +436,12 @@ appendWhereClause(StringInfo buf,
 				  List *exprs,
 				  PlannerInfo *root)
 {
+	int			nestlevel;
 	ListCell   *lc;
 
+	/* Make sure any constants in the exprs are printed portably */
+	nestlevel = set_transmission_modes();
+
 	foreach(lc, exprs)
 	{
 		RestrictInfo *ri = (RestrictInfo *) lfirst(lc);
@@ -454,6 +458,8 @@ appendWhereClause(StringInfo buf,
 
 		is_first = false;
 	}
+
+	reset_transmission_modes(nestlevel);
 }
 
 /*
diff --git a/contrib/postgres_fdw/postgres_fdw.c b/contrib/postgres_fdw/postgres_fdw.c
@@ -25,6 +25,8 @@
 #include "optimizer/pathnode.h"
 #include "optimizer/planmain.h"
 #include "parser/parsetree.h"
+#include "utils/builtins.h"
+#include "utils/guc.h"
 #include "utils/lsyscache.h"
 #include "utils/memutils.h"
 
@@ -907,9 +909,12 @@ create_cursor(ForeignScanState *node)
 	if (numParams > 0 && !festate->extparams_done)
 	{
 		ParamListInfo params = node->ss.ps.state->es_param_list_info;
+		int			nestlevel;
 		List	   *param_numbers;
 		ListCell   *lc;
 
+		nestlevel = set_transmission_modes();
+
 		param_numbers = (List *)
 			list_nth(festate->fdw_private, FdwPrivateExternParamIds);
 		foreach(lc, param_numbers)
@@ -951,6 +956,8 @@ create_cursor(ForeignScanState *node)
 															prm->value);
 			}
 		}
+		reset_transmission_modes(nestlevel);
+
 		festate->extparams_done = true;
 	}
 
@@ -1058,6 +1065,56 @@ fetch_more_data(ForeignScanState *node)
 	MemoryContextSwitchTo(oldcontext);
 }
 
+/*
+ * Force assorted GUC parameters to settings that ensure that we'll output
+ * data values in a form that is unambiguous to the remote server.
+ *
+ * This is rather expensive and annoying to do once per row, but there's
+ * little choice if we want to be sure values are transmitted accurately;
+ * we can't leave the settings in place between rows for fear of affecting
+ * user-visible computations.
+ *
+ * We use the equivalent of a function SET option to allow the settings to
+ * persist only until the caller calls reset_transmission_modes().	If an
+ * error is thrown in between, guc.c will take care of undoing the settings.
+ *
+ * The return value is the nestlevel that must be passed to
+ * reset_transmission_modes() to undo things.
+ */
+int
+set_transmission_modes(void)
+{
+	int			nestlevel = NewGUCNestLevel();
+
+	/*
+	 * The values set here should match what pg_dump does.	See also
+	 * configure_remote_session in connection.c.
+	 */
+	if (DateStyle != USE_ISO_DATES)
+		(void) set_config_option("datestyle", "ISO",
+								 PGC_USERSET, PGC_S_SESSION,
+								 GUC_ACTION_SAVE, true, 0);
+	if (IntervalStyle != INTSTYLE_POSTGRES)
+		(void) set_config_option("intervalstyle", "postgres",
+								 PGC_USERSET, PGC_S_SESSION,
+								 GUC_ACTION_SAVE, true, 0);
+	if (extra_float_digits < 3)
+		(void) set_config_option("extra_float_digits", "3",
+								 PGC_USERSET, PGC_S_SESSION,
+								 GUC_ACTION_SAVE, true, 0);
+
+	return nestlevel;
+}
+
+/*
+ * Undo the effects of set_transmission_modes().
+ */
+void
+reset_transmission_modes(int nestlevel)
+{
+	AtEOXact_GUC(true, nestlevel);
+}
+
 /*
  * Utility routine to close a cursor.
  */
diff --git a/contrib/postgres_fdw/postgres_fdw.h b/contrib/postgres_fdw/postgres_fdw.h
@@ -20,6 +20,10 @@
 
 #include "libpq-fe.h"
 
+/* in postgres_fdw.c */
+extern int	set_transmission_modes(void);
+extern void reset_transmission_modes(int nestlevel);
+
 /* in connection.c */
 extern PGconn *GetConnection(ForeignServer *server, UserMapping *user);
 extern void ReleaseConnection(PGconn *conn);

Original file line number	Diff line number	Diff line change
`@@ -436,8 +436,12 @@ appendWhereClause(StringInfo buf,`
`436`	`436`	`List *exprs,`
`437`	`437`	`PlannerInfo *root)`
`438`	`438`	`{`
	`439`	`+ int nestlevel;`
`439`	`440`	`ListCell *lc;`
`440`	`441`
	`442`	`+ /* Make sure any constants in the exprs are printed portably */`
	`443`	`+ nestlevel = set_transmission_modes();`
	`444`	`+`
`441`	`445`	`foreach(lc, exprs)`
`442`	`446`	`{`
`443`	`447`	`RestrictInfo ri = (RestrictInfo ) lfirst(lc);`
`@@ -454,6 +458,8 @@ appendWhereClause(StringInfo buf,`
`454`	`458`
`455`	`459`	`is_first = false;`
`456`	`460`	`}`
	`461`	`+`
	`462`	`+ reset_transmission_modes(nestlevel);`
`457`	`463`	`}`
`458`	`464`
`459`	`465`	`/*`