Remove race condition in pg_get_expr().

Since its introduction, pg_get_expr() has intended to silently
return NULL if called with an invalid relation OID, as can happen
when scanning the catalogs concurrently with relation drops.
However, there is a race condition: we check validity of the OID
at the start, but it could get dropped just afterward, leading to
failures.  This is the cause of some intermittent instability we're
seeing in a proposed new test case, and presumably it's a hazard in
the field as well.

We can fix this by AccessShareLock-ing the target relation for the
duration of pg_get_expr().  Since we don't require any permissions
on the target relation, this is semantically a bit undesirable.  But
it turns out that the set_relation_column_names() subroutine already
takes a transient AccessShareLock on that relation, and has done since
commit 2ffa740be in 2012.  Given the lack of complaints about that, it
seems like there should be no harm in holding the lock a bit longer.

Back-patch to all supported branches.

Discussion: https://postgr.es/m/31ddcc01-a71b-4e8c-9948-01d1c47293ca@eisentraut.org

diff --git a/src/backend/utils/adt/ruleutils.c b/src/backend/utils/adt/ruleutils.c
index b625f471a8475c68c077f8040c45def11da999ca..a928a8c55df774b0fecf5157a99b0413d337f5c7 100644 (file)
--- a/src/backend/utils/adt/ruleutils.c
+++ b/src/backend/utils/adt/ruleutils.c
@@ -352,8 +352,7 @@ static char *pg_get_partkeydef_worker(Oid relid, int prettyFlags,
                                      bool attrsOnly, bool missing_ok);
 static char *pg_get_constraintdef_worker(Oid constraintId, bool fullCommand,
                                         int prettyFlags, bool missing_ok);
-static text *pg_get_expr_worker(text *expr, Oid relid, const char *relname,
-                               int prettyFlags);
+static text *pg_get_expr_worker(text *expr, Oid relid, int prettyFlags);
 static int print_function_arguments(StringInfo buf, HeapTuple proctup,
                                     bool print_table_args, bool print_defaults);
 static void print_function_rettype(StringInfo buf, HeapTuple proctup);
@@ -2615,6 +2614,11 @@ decompile_column_index_array(Datum column_index_array, Oid relId,
  * partial indexes, column default expressions, etc.  We also support
  * Var-free expressions, for which the OID can be InvalidOid.
  *
+ * If the OID is nonzero but not actually valid, don't throw an error,
+ * just return NULL.  This is a bit questionable, but it's what we've
+ * done historically, and it can help avoid unwanted failures when
+ * examining catalog entries for just-deleted relations.
+ *
  * We expect this function to work, or throw a reasonably clean error,
  * for any node tree that can appear in a catalog pg_node_tree column.
  * Query trees, such as those appearing in pg_rewrite.ev_action, are
@@ -2627,29 +2631,16 @@ pg_get_expr(PG_FUNCTION_ARGS)
 {
    text       *expr = PG_GETARG_TEXT_PP(0);
    Oid         relid = PG_GETARG_OID(1);
+   text       *result;
    int         prettyFlags;
-   char       *relname;
 
    prettyFlags = PRETTYFLAG_INDENT;
 
-   if (OidIsValid(relid))
-   {
-       /* Get the name for the relation */
-       relname = get_rel_name(relid);
-
-       /*
-        * If the OID isn't actually valid, don't throw an error, just return
-        * NULL.  This is a bit questionable, but it's what we've done
-        * historically, and it can help avoid unwanted failures when
-        * examining catalog entries for just-deleted relations.
-        */
-       if (relname == NULL)
-           PG_RETURN_NULL();
-   }
+   result = pg_get_expr_worker(expr, relid, prettyFlags);
+   if (result)
+       PG_RETURN_TEXT_P(result);
    else
-       relname = NULL;
-
-   PG_RETURN_TEXT_P(pg_get_expr_worker(expr, relid, relname, prettyFlags));
+       PG_RETURN_NULL();
 }
 
 Datum
@@ -2658,33 +2649,27 @@ pg_get_expr_ext(PG_FUNCTION_ARGS)
    text       *expr = PG_GETARG_TEXT_PP(0);
    Oid         relid = PG_GETARG_OID(1);
    bool        pretty = PG_GETARG_BOOL(2);
+   text       *result;
    int         prettyFlags;
-   char       *relname;
 
    prettyFlags = GET_PRETTY_FLAGS(pretty);
 
-   if (OidIsValid(relid))
-   {
-       /* Get the name for the relation */
-       relname = get_rel_name(relid);
-       /* See notes above */
-       if (relname == NULL)
-           PG_RETURN_NULL();
-   }
+   result = pg_get_expr_worker(expr, relid, prettyFlags);
+   if (result)
+       PG_RETURN_TEXT_P(result);
    else
-       relname = NULL;
-
-   PG_RETURN_TEXT_P(pg_get_expr_worker(expr, relid, relname, prettyFlags));
+       PG_RETURN_NULL();
 }
 
 static text *
-pg_get_expr_worker(text *expr, Oid relid, const char *relname, int prettyFlags)
+pg_get_expr_worker(text *expr, Oid relid, int prettyFlags)
 {
    Node       *node;
    Node       *tst;
    Relids      relids;
    List       *context;
    char       *exprstr;
+   Relation    rel = NULL;
    char       *str;
 
    /* Convert input pg_node_tree (really TEXT) object to C string */
@@ -2729,9 +2714,19 @@ pg_get_expr_worker(text *expr, Oid relid, const char *relname, int prettyFlags)
                     errmsg("expression contains variables")));
    }
 
-   /* Prepare deparse context if needed */
+   /*
+    * Prepare deparse context if needed.  If we are deparsing with a relid,
+    * we need to transiently open and lock the rel, to make sure it won't go
+    * away underneath us.  (set_relation_column_names would lock it anyway,
+    * so this isn't really introducing any new behavior.)
+    */
    if (OidIsValid(relid))
-       context = deparse_context_for(relname, relid);
+   {
+       rel = try_relation_open(relid, AccessShareLock);
+       if (rel == NULL)
+           return NULL;
+       context = deparse_context_for(RelationGetRelationName(rel), relid);
+   }
    else
        context = NIL;
 
@@ -2739,6 +2734,9 @@ pg_get_expr_worker(text *expr, Oid relid, const char *relname, int prettyFlags)
    str = deparse_expression_pretty(node, context, false, false,
                                    prettyFlags, 0);
 
+   if (rel != NULL)
+       relation_close(rel, AccessShareLock);
+
    return string_to_text(str);
 }