Fix postgres_fdw's issues with inconsistent interpretation of data values.
authorTom Lane
Tue, 12 Mar 2013 01:31:28 +0000 (21:31 -0400)
committerTom Lane
Tue, 12 Mar 2013 01:31:28 +0000 (21:31 -0400)
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 f7951eef89be78c50ea2241f593d76dfefe176c9, 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.)

contrib/postgres_fdw/connection.c
contrib/postgres_fdw/deparse.c
contrib/postgres_fdw/expected/postgres_fdw.out
contrib/postgres_fdw/postgres_fdw.c
contrib/postgres_fdw/postgres_fdw.h
contrib/postgres_fdw/sql/postgres_fdw.sql

index 22ac50e6f9f1ad6cf8fe2c62ed7c9fded52e9002..0a723f0dfa1b4b01fc189a0a09ef3661bb137b2c 100644 (file)
@@ -67,6 +67,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,
@@ -314,11 +315,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);
@@ -339,7 +372,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)
@@ -353,10 +385,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;
    }
 
@@ -370,10 +399,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++;
    }
 }
@@ -509,10 +535,7 @@ pgfdw_xact_callback(XactEvent event, void *arg)
        {
            case XACT_EVENT_PRE_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);
+               do_sql_command(entry->conn, "COMMIT TRANSACTION");
 
                /*
                 * If there were any errors in subtransactions, and we made
@@ -647,10 +670,7 @@ pgfdw_subxact_callback(SubXactEvent event, SubTransactionId mySubid,
        {
            /* Commit all remote subtransactions during pre-commit */
            snprintf(sql, sizeof(sql), "RELEASE SAVEPOINT s%d", curlevel);
-           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);
        }
        else
        {
index f5d723cc38aacf4502f61d2d16b73d2a98685216..8ed915744af1a11a73062d224e70b4f34863f815 100644 (file)
@@ -469,8 +469,12 @@ appendWhereClause(StringInfo buf,
                  List *exprs,
                  bool is_first)
 {
+   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);
@@ -487,6 +491,8 @@ appendWhereClause(StringInfo buf,
 
        is_first = false;
    }
+
+   reset_transmission_modes(nestlevel);
 }
 
 /*
index 50b1f239209c8a3ee6d9b98a5d5a59524051b7ff..9b7ca3136051eb64eefb710d8404576d81f5ed3b 100644 (file)
@@ -1983,10 +1983,10 @@ INSERT INTO ft1(c1, c2) VALUES(1111, -2);  -- c2positive
 ERROR:  new row for relation "T 1" violates check constraint "c2positive"
 DETAIL:  Failing row contains (1111, -2, null, null, null, (^-^;), null, null).
 CONTEXT:  Remote SQL command: INSERT INTO "S 1"."T 1"("C 1", c2) VALUES ($1, $2)
-UPDATE ft1 SET c2 = -c2, c4 = null WHERE c1 = 1;  -- c2positive
+UPDATE ft1 SET c2 = -c2 WHERE c1 = 1;  -- c2positive
 ERROR:  new row for relation "T 1" violates check constraint "c2positive"
-DETAIL:  Failing row contains (1, -1, 00001_trig_update, null, 1970-01-02 00:00:00, 1, 1         , foo).
-CONTEXT:  Remote SQL command: UPDATE "S 1"."T 1" SET c2 = $2, c4 = $3 WHERE ctid = $1
+DETAIL:  Failing row contains (1, -1, 00001_trig_update, 1970-01-02 08:00:00+00, 1970-01-02 00:00:00, 1, 1         , foo).
+CONTEXT:  Remote SQL command: UPDATE "S 1"."T 1" SET c2 = $2 WHERE ctid = $1
 -- Test savepoint/rollback behavior
 select c2, count(*) from ft2 where c2 < 500 group by 1 order by 1;
  c2  | count 
@@ -2142,10 +2142,10 @@ select c2, count(*) from ft2 where c2 < 500 group by 1 order by 1;
 (13 rows)
 
 savepoint s3;
-update ft2 set c2 = -2, c4 = null where c2 = 42; -- fail on remote side
+update ft2 set c2 = -2 where c2 = 42; -- fail on remote side
 ERROR:  new row for relation "T 1" violates check constraint "c2positive"
-DETAIL:  Failing row contains (10, -2, 00010_trig_update_trig_update, null, 1970-01-11 00:00:00, 0, 0         , foo).
-CONTEXT:  Remote SQL command: UPDATE "S 1"."T 1" SET c2 = $2, c4 = $3 WHERE ctid = $1
+DETAIL:  Failing row contains (10, -2, 00010_trig_update_trig_update, 1970-01-11 08:00:00+00, 1970-01-11 00:00:00, 0, 0         , foo).
+CONTEXT:  Remote SQL command: UPDATE "S 1"."T 1" SET c2 = $2 WHERE ctid = $1
 rollback to savepoint s3;
 select c2, count(*) from ft2 where c2 < 500 group by 1 order by 1;
  c2  | count 
index a6db061d603194de1477fa0b07e8d8a13996ae01..95505c8a1c76b1dae1186c5fbf2eb537a0f01a22 100644 (file)
@@ -30,6 +30,7 @@
 #include "optimizer/var.h"
 #include "parser/parsetree.h"
 #include "utils/builtins.h"
+#include "utils/guc.h"
 #include "utils/lsyscache.h"
 #include "utils/memutils.h"
 
@@ -1554,9 +1555,12 @@ create_cursor(ForeignScanState *node)
    if (numParams > 0 && !fsstate->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(fsstate->fdw_private, FdwScanPrivateExternParamIds);
        foreach(lc, param_numbers)
@@ -1598,6 +1602,9 @@ create_cursor(ForeignScanState *node)
                                                            prm->value);
            }
        }
+
+       reset_transmission_modes(nestlevel);
+
        fsstate->extparams_done = true;
    }
 
@@ -1705,6 +1712,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.
  */
@@ -1791,16 +1848,20 @@ convert_prep_stmt_params(PgFdwModifyState *fmstate,
    /* 1st parameter should be ctid, if it's in use */
    if (tupleid != NULL)
    {
+       /* don't need set_transmission_modes for TID output */
        p_values[pindex] = OutputFunctionCall(&fmstate->p_flinfo[pindex],
                                              PointerGetDatum(tupleid));
        pindex++;
    }
 
    /* get following parameters from slot */
-   if (slot != NULL)
+   if (slot != NULL && fmstate->target_attrs != NIL)
    {
+       int         nestlevel;
        ListCell   *lc;
 
+       nestlevel = set_transmission_modes();
+
        foreach(lc, fmstate->target_attrs)
        {
            int         attnum = lfirst_int(lc);
@@ -1815,6 +1876,8 @@ convert_prep_stmt_params(PgFdwModifyState *fmstate,
                                                      value);
            pindex++;
        }
+
+       reset_transmission_modes(nestlevel);
    }
 
    Assert(pindex == fmstate->p_nums);
index 236a60b4898090dc50920c434870209f07905045..9149aa186f766beb0b61fe063f219f0833812c12 100644 (file)
 
 #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,
              bool will_prep_stmt);
index 2e9e17f5f93cc467ba330355d85fa44d00099700..007109c7c768020ff09eb3569119325a3e883767 100644 (file)
@@ -316,7 +316,7 @@ ALTER TABLE "S 1"."T 1" ADD CONSTRAINT c2positive CHECK (c2 >= 0);
 
 INSERT INTO ft1(c1, c2) VALUES(11, 12);  -- duplicate key
 INSERT INTO ft1(c1, c2) VALUES(1111, -2);  -- c2positive
-UPDATE ft1 SET c2 = -c2, c4 = null WHERE c1 = 1;  -- c2positive
+UPDATE ft1 SET c2 = -c2 WHERE c1 = 1;  -- c2positive
 
 -- Test savepoint/rollback behavior
 select c2, count(*) from ft2 where c2 < 500 group by 1 order by 1;
@@ -337,7 +337,7 @@ select c2, count(*) from ft2 where c2 < 500 group by 1 order by 1;
 release savepoint s2;
 select c2, count(*) from ft2 where c2 < 500 group by 1 order by 1;
 savepoint s3;
-update ft2 set c2 = -2, c4 = null where c2 = 42; -- fail on remote side
+update ft2 set c2 = -2 where c2 = 42; -- fail on remote side
 rollback to savepoint s3;
 select c2, count(*) from ft2 where c2 < 500 group by 1 order by 1;
 release savepoint s3;