Re: pg_get_expr locking

I wrote:
> Peter Eisentraut <peter@eisentraut.org> writes:
>> I think the situation is that one test (domain) runs pg_get_expr as part 
>> of an information_schema view, while at the same time another test 
>> (alter_table) drops a table that the pg_get_expr is just processing.

> The test case that's failing is, IIUC,

> +SELECT * FROM information_schema.domain_constraints
> +  WHERE domain_name IN ('con', 'dom', 'pos_int', 'things')
> +  ORDER BY constraint_name;

Oh, scratch that: there are two confusingly lookalike queries
in the patch.  The one that is failing is

SELECT * FROM information_schema.check_constraints
  WHERE (constraint_schema, constraint_name)
        IN (SELECT constraint_schema, constraint_name
            FROM information_schema.domain_constraints
            WHERE domain_name IN ('con', 'dom', 'pos_int', 'things'))
  ORDER BY constraint_name;

and we have trouble because the evaluation of pg_get_expr in
check_constraints is done before the semijoin that would restrict
it to just the desired objects.

After looking at the code I'm less worried about the
permissions-checking angle than I was before, because I see
that pg_get_expr already takes a transient AccessShareLock
on the rel, down inside set_relation_column_names.  This is
not ideal from a permissions standpoint perhaps, but it's
probably OK considering we've done that for a long time.
We just need to hold that lock a little while longer.

I propose the attached as a reasonably localized fix.
We could imagine a more aggressive refactoring that would
allow passing down the Relation instead of re-opening it
in set_relation_column_names, but I doubt it's worth the
trouble.

            regards, tom lane

diff --git a/src/backend/utils/adt/ruleutils.c b/src/backend/utils/adt/ruleutils.c
index b625f471a8..644966721f 100644
--- 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 +2622,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 +2639,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 +2657,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 +2722,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 +2742,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);
 }