Ensure that we don't read rule definition with portable input on

Rules are converted in their string representation and stored in the catalog.
While building relation descriptor, this information is read back and converted
into a Node representation. Since relation descriptors could be built when we
are reading plan information sent by the remote server in a stringified
representation, trying to read the rules with portable input on may lead to
unpleasant behaviour. So we must first reset portable input and restore it back
after reading the rules. The same applies to RLS policies (even though we don't
have a test showing the impact, but it looks like a sane thing to fix anyways)

diff --git a/src/backend/nodes/readfuncs.c b/src/backend/nodes/readfuncs.c
index 99dfce1cb9ac1e8b222b7631af25122d6d82021f..dd47b2698cb405b07aafe6526db200b8f574759d 100644 (file)
--- a/src/backend/nodes/readfuncs.c
+++ b/src/backend/nodes/readfuncs.c
@@ -59,10 +59,12 @@
  * Later we may want to add extra parameter in stringToNode() function
  */
 static bool portable_input = false;
-void
+bool
 set_portable_input(bool value)
 {
+       bool old_portable_input = portable_input;
        portable_input = value;
+       return old_portable_input;
 }
 #endif /* XCP */
 

diff --git a/src/backend/pgxc/pool/execRemote.c b/src/backend/pgxc/pool/execRemote.c
index 0286337639ddc5749c7ff7568bef8ad1e37411ba..2fd23687fc9d15a93bf82cf0a059ba2e0076fcc2 100644 (file)
--- a/src/backend/pgxc/pool/execRemote.c
+++ b/src/backend/pgxc/pool/execRemote.c
@@ -5600,8 +5600,26 @@ ExecInitRemoteSubplan(RemoteSubplan *node, EState *estate, int eflags)
                rstmt.distributionNodes = node->distributionNodes;
                rstmt.distributionRestrict = node->distributionRestrict;
 
-               set_portable_output(true);
-               remotestate->subplanstr = nodeToString(&rstmt);
+               /*
+                * A try-catch block to ensure that we don't leave behind a stale state
+                * if nodeToString fails for whatever reason.
+                *
+                * XXX We should probably rewrite it someday by either passing a
+                * context to nodeToString() or remembering this information somewhere
+                * else which gets reset in case of errors. But for now, this seems
+                * enough.
+                */
+               PG_TRY();
+               {
+                       set_portable_output(true);
+                       remotestate->subplanstr = nodeToString(&rstmt);
+               }
+               PG_CATCH();
+               {
+                       set_portable_output(false);
+                       PG_RE_THROW();
+               }
+               PG_END_TRY();
                set_portable_output(false);
 
                /*

diff --git a/src/backend/utils/cache/plancache.c b/src/backend/utils/cache/plancache.c
index 524c62d7a0e31b8662c1a17be2d094265bd3cea2..501cd54b9b52fd2bb051b26c807e47a5a5a0432f 100644 (file)
--- a/src/backend/utils/cache/plancache.c
+++ b/src/backend/utils/cache/plancache.c
@@ -2017,9 +2017,26 @@ SetRemoteSubplan(CachedPlanSource *plansource, const char *plan_string)
 
        /*
         * Restore query plan.
+        *
+        * A try-catch block to ensure that we don't leave behind a stale state
+        * if nodeToString fails for whatever reason.
+        *
+        * XXX We should probably rewrite it someday by either passing a
+        * context to nodeToString() or remembering this information somewhere
+        * else which gets reset in case of errors. But for now, this seems
+        * enough.
         */
-       set_portable_input(true);
-       rstmt = (RemoteStmt *) stringToNode((char *) plan_string);
+       PG_TRY();
+       {
+               set_portable_input(true);
+               rstmt = (RemoteStmt *) stringToNode((char *) plan_string);
+       }
+       PG_CATCH();
+       {
+               set_portable_input(false);
+               PG_RE_THROW();
+       }
+       PG_END_TRY();
        set_portable_input(false);
 
        stmt = makeNode(PlannedStmt);

diff --git a/src/backend/utils/cache/relcache.c b/src/backend/utils/cache/relcache.c
index c6542134456ec5218d3f8f588320cf8c5866fdc6..7a586f95cfac7111d238d7a9fe6ff4757d8a85e9 100644 (file)
--- a/src/backend/utils/cache/relcache.c
+++ b/src/backend/utils/cache/relcache.c
@@ -1340,7 +1340,20 @@ RelationBuildDesc(Oid targetRelId, bool insertIt)
         * Fetch rules and triggers that affect this relation
         */
        if (relation->rd_rel->relhasrules)
+       {
+               /*
+                * We could be in the middle of reading a portable input string and get
+                * called to build the relation descriptor. This might require
+                * recursive calls to stringToNode (e.g. while reading rules) and those
+                * must not be done with portable input turned on (because the
+                * stringified versions are not created with portable output turned
+                * on). So temporarily reset portable input to false and restore once
+                * we are done with reading rules.
+                */
+               bool saved_portable_input = set_portable_input(false);
                RelationBuildRuleLock(relation);
+               set_portable_input(saved_portable_input);
+       }
        else
        {
                relation->rd_rules = NULL;
@@ -1358,7 +1371,12 @@ RelationBuildDesc(Oid targetRelId, bool insertIt)
                RelationBuildLocator(relation);
 #endif
        if (relation->rd_rel->relrowsecurity)
+       {
+               /* See comments near RelationBuildRuleLocok for details */
+               bool saved_portable_input = set_portable_input(false);
                RelationBuildRowSecurity(relation);
+               set_portable_input(saved_portable_input);
+       }
        else
                relation->rd_rsdesc = NULL;
 

diff --git a/src/include/nodes/nodes.h b/src/include/nodes/nodes.h
index 47e55dbb5f3710bad045b159c6f053e217118057..a620f29d0c3f0ee9deded4e6b074579d916acb4e 100644 (file)
--- a/src/include/nodes/nodes.h
+++ b/src/include/nodes/nodes.h
@@ -651,7 +651,7 @@ extern char *bmsToString(const struct Bitmapset *bms);
  * nodes/{readfuncs.c,read.c}
  */
 #ifdef XCP
-extern void set_portable_input(bool value);
+extern bool set_portable_input(bool value);
 #endif
 extern void *stringToNode(char *str);
 extern struct Bitmapset *readBitmapset(void);