Fix incorrect permissions-checking code for extended statistics.

Commit a4d75c86b improved the extended-stats logic to allow extended
stats to be collected on expressions not just bare Vars.  To apply
such stats, we first verify that the user has permissions to read all
columns used in the stats.  (If not, the query will likely fail at
runtime, but the planner ought not do so.)  That had to get extended
to check permissions of columns appearing within such expressions,
but the code for that was completely wrong: it applied pull_varattnos
to the wrong pointer, leading to "unrecognized node type" failures.
Furthermore, although you couldn't get to this because of that bug,
it failed to account for the attnum offset applied by pull_varattnos.

This escaped recognition so far because the code in question is not
reached when the user has whole-table SELECT privilege (which is the
common case), and because only subexpressions not specially handled
by statext_is_compatible_clause_internal() are at risk.

I think a large part of the reason for this bug is under-documentation
of what statext_is_compatible_clause() is doing and what its arguments
are, so do some work on the comments to try to improve that.

Per bug #17570 from Alexander Kozhemyakin.  Patch by Richard Guo;
comments and other cosmetic improvements by me.  (Thanks also to
Japin Li for diagnosis.)  Back-patch to v14 where the bug came in.

Discussion: https://postgr.es/m/17570-f2f2e0f4bccf0965@postgresql.org

diff --git a/src/backend/statistics/extended_stats.c b/src/backend/statistics/extended_stats.c
index ee8f2efb3a6c4d3278d2cc88defcfde6adcce387..5922909fe6056dba415e04358d7526fa96f9a4b7 100644 (file)
--- a/src/backend/statistics/extended_stats.c
+++ b/src/backend/statistics/extended_stats.c
@@ -1316,10 +1316,38 @@ choose_best_statistics(List *stats, char requiredkind,
  * statext_is_compatible_clause_internal
  *     Determines if the clause is compatible with MCV lists.
  *
- * Does the heavy lifting of actually inspecting the clauses for
- * statext_is_compatible_clause. It needs to be split like this because
- * of recursion.  The attnums bitmap is an input/output parameter collecting
- * attribute numbers from all compatible clauses (recursively).
+ * To be compatible, the given clause must be a combination of supported
+ * clauses built from Vars or sub-expressions (where a sub-expression is
+ * something that exactly matches an expression found in statistics objects).
+ * This function recursively examines the clause and extracts any
+ * sub-expressions that will need to be matched against statistics.
+ *
+ * Currently, we only support the following types of clauses:
+ *
+ * (a) OpExprs of the form (Var/Expr op Const), or (Const op Var/Expr), where
+ * the op is one of ("=", "<", ">", ">=", "<=")
+ *
+ * (b) (Var/Expr IS [NOT] NULL)
+ *
+ * (c) combinations using AND/OR/NOT
+ *
+ * (d) ScalarArrayOpExprs of the form (Var/Expr op ANY (array)) or (Var/Expr
+ * op ALL (array))
+ *
+ * In the future, the range of supported clauses may be expanded to more
+ * complex cases, for example (Var op Var).
+ *
+ * Arguments:
+ * clause: (sub)clause to be inspected (bare clause, not a RestrictInfo)
+ * relid: rel that all Vars in clause must belong to
+ * *attnums: input/output parameter collecting attribute numbers of all
+ *     mentioned Vars.  Note that we do not offset the attribute numbers,
+ *     so we can't cope with system columns.
+ * *exprs: input/output parameter collecting primitive subclauses within
+ *     the clause tree
+ *
+ * Returns false if there is something we definitively can't handle.
+ * On true return, we can proceed to match the *exprs against statistics.
  */
 static bool
 statext_is_compatible_clause_internal(PlannerInfo *root, Node *clause,
@@ -1343,10 +1371,14 @@ statext_is_compatible_clause_internal(PlannerInfo *root, Node *clause,
        if (var->varlevelsup > 0)
            return false;
 
-       /* Also skip system attributes (we don't allow stats on those). */
+       /*
+        * Also reject system attributes and whole-row Vars (we don't allow
+        * stats on those).
+        */
        if (!AttrNumberIsForUserDefinedAttr(var->varattno))
            return false;
 
+       /* OK, record the attnum for later permissions checks. */
        *attnums = bms_add_member(*attnums, var->varattno);
 
        return true;
@@ -1501,7 +1533,7 @@ statext_is_compatible_clause_internal(PlannerInfo *root, Node *clause,
        foreach(lc, expr->args)
        {
            /*
-            * Had we found incompatible clause in the arguments, treat the
+            * If we find an incompatible clause in the arguments, treat the
             * whole clause as incompatible.
             */
            if (!statext_is_compatible_clause_internal(root,
@@ -1540,27 +1572,28 @@ statext_is_compatible_clause_internal(PlannerInfo *root, Node *clause,
  * statext_is_compatible_clause
  *     Determines if the clause is compatible with MCV lists.
  *
- * Currently, we only support the following types of clauses:
+ * See statext_is_compatible_clause_internal, above, for the basic rules.
+ * This layer deals with RestrictInfo superstructure and applies permissions
+ * checks to verify that it's okay to examine all mentioned Vars.
  *
- * (a) OpExprs of the form (Var/Expr op Const), or (Const op Var/Expr), where
- * the op is one of ("=", "<", ">", ">=", "<=")
+ * Arguments:
+ * clause: clause to be inspected (in RestrictInfo form)
+ * relid: rel that all Vars in clause must belong to
+ * *attnums: input/output parameter collecting attribute numbers of all
+ *     mentioned Vars.  Note that we do not offset the attribute numbers,
+ *     so we can't cope with system columns.
+ * *exprs: input/output parameter collecting primitive subclauses within
+ *     the clause tree
  *
- * (b) (Var/Expr IS [NOT] NULL)
- *
- * (c) combinations using AND/OR/NOT
- *
- * (d) ScalarArrayOpExprs of the form (Var/Expr op ANY (array)) or (Var/Expr
- * op ALL (array))
- *
- * In the future, the range of supported clauses may be expanded to more
- * complex cases, for example (Var op Var).
+ * Returns false if there is something we definitively can't handle.
+ * On true return, we can proceed to match the *exprs against statistics.
  */
 static bool
 statext_is_compatible_clause(PlannerInfo *root, Node *clause, Index relid,
                             Bitmapset **attnums, List **exprs)
 {
    RangeTblEntry *rte = root->simple_rte_array[relid];
-   RestrictInfo *rinfo = (RestrictInfo *) clause;
+   RestrictInfo *rinfo;
    int         clause_relid;
    Oid         userid;
 
@@ -1589,8 +1622,9 @@ statext_is_compatible_clause(PlannerInfo *root, Node *clause, Index relid,
    }
 
    /* Otherwise it must be a RestrictInfo. */
-   if (!IsA(rinfo, RestrictInfo))
+   if (!IsA(clause, RestrictInfo))
        return false;
+   rinfo = (RestrictInfo *) clause;
 
    /* Pseudoconstants are not really interesting here. */
    if (rinfo->pseudoconstant)
@@ -1612,34 +1646,48 @@ statext_is_compatible_clause(PlannerInfo *root, Node *clause, Index relid,
     */
    userid = rte->checkAsUser ? rte->checkAsUser : GetUserId();
 
+   /* Table-level SELECT privilege is sufficient for all columns */
    if (pg_class_aclcheck(rte->relid, userid, ACL_SELECT) != ACLCHECK_OK)
    {
        Bitmapset  *clause_attnums = NULL;
+       int         attnum = -1;
 
-       /* Don't have table privilege, must check individual columns */
-       if (*exprs != NIL)
+       /*
+        * We have to check per-column privileges.  *attnums has the attnums
+        * for individual Vars we saw, but there may also be Vars within
+        * subexpressions in *exprs.  We can use pull_varattnos() to extract
+        * those, but there's an impedance mismatch: attnums returned by
+        * pull_varattnos() are offset by FirstLowInvalidHeapAttributeNumber,
+        * while attnums within *attnums aren't.  Convert *attnums to the
+        * offset style so we can combine the results.
+        */
+       while ((attnum = bms_next_member(*attnums, attnum)) >= 0)
        {
-           pull_varattnos((Node *) exprs, relid, &clause_attnums);
-           clause_attnums = bms_add_members(clause_attnums, *attnums);
+           clause_attnums =
+               bms_add_member(clause_attnums,
+                              attnum - FirstLowInvalidHeapAttributeNumber);
        }
-       else
-           clause_attnums = *attnums;
 
-       if (bms_is_member(InvalidAttrNumber, clause_attnums))
-       {
-           /* Have a whole-row reference, must have access to all columns */
-           if (pg_attribute_aclcheck_all(rte->relid, userid, ACL_SELECT,
-                                         ACLMASK_ALL) != ACLCHECK_OK)
-               return false;
-       }
-       else
+       /* Now merge attnums from *exprs into clause_attnums */
+       if (*exprs != NIL)
+           pull_varattnos((Node *) *exprs, relid, &clause_attnums);
+
+       attnum = -1;
+       while ((attnum = bms_next_member(clause_attnums, attnum)) >= 0)
        {
-           /* Check the columns referenced by the clause */
-           int         attnum = -1;
+           /* Undo the offset */
+           AttrNumber  attno = attnum + FirstLowInvalidHeapAttributeNumber;
 
-           while ((attnum = bms_next_member(clause_attnums, attnum)) >= 0)
+           if (attno == InvalidAttrNumber)
+           {
+               /* Whole-row reference, so must have access to all columns */
+               if (pg_attribute_aclcheck_all(rte->relid, userid, ACL_SELECT,
+                                             ACLMASK_ALL) != ACLCHECK_OK)
+                   return false;
+           }
+           else
            {
-               if (pg_attribute_aclcheck(rte->relid, attnum, userid,
+               if (pg_attribute_aclcheck(rte->relid, attno, userid,
                                          ACL_SELECT) != ACLCHECK_OK)
                    return false;
            }

diff --git a/src/test/regress/expected/stats_ext.out b/src/test/regress/expected/stats_ext.out
index b0548108f01d3ea21622cfd201c4886e9f7b668a..b494411074499680bc4749e114dc088760e55d2f 100644 (file)
--- a/src/test/regress/expected/stats_ext.out
+++ b/src/test/regress/expected/stats_ext.out
@@ -3183,6 +3183,10 @@ GRANT USAGE ON SCHEMA tststats TO regress_stats_user1;
 SET SESSION AUTHORIZATION regress_stats_user1;
 SELECT * FROM tststats.priv_test_tbl; -- Permission denied
 ERROR:  permission denied for table priv_test_tbl
+-- Check individual columns if we don't have table privilege
+SELECT * FROM tststats.priv_test_tbl
+  WHERE a = 1 and tststats.priv_test_tbl.* > (1, 1) is not null;
+ERROR:  permission denied for table priv_test_tbl
 -- Attempt to gain access using a leaky operator
 CREATE FUNCTION op_leak(int, int) RETURNS bool
     AS 'BEGIN RAISE NOTICE ''op_leak => %, %'', $1, $2; RETURN $1 < $2; END'

diff --git a/src/test/regress/sql/stats_ext.sql b/src/test/regress/sql/stats_ext.sql
index d2c59b0a8ad29693f2d720dec1a9e09ff17060f7..953c7acfc8847562f5421b165d1e265debef20bf 100644 (file)
--- a/src/test/regress/sql/stats_ext.sql
+++ b/src/test/regress/sql/stats_ext.sql
@@ -1600,6 +1600,10 @@ GRANT USAGE ON SCHEMA tststats TO regress_stats_user1;
 SET SESSION AUTHORIZATION regress_stats_user1;
 SELECT * FROM tststats.priv_test_tbl; -- Permission denied
 
+-- Check individual columns if we don't have table privilege
+SELECT * FROM tststats.priv_test_tbl
+  WHERE a = 1 and tststats.priv_test_tbl.* > (1, 1) is not null;
+
 -- Attempt to gain access using a leaky operator
 CREATE FUNCTION op_leak(int, int) RETURNS bool
     AS 'BEGIN RAISE NOTICE ''op_leak => %, %'', $1, $2; RETURN $1 < $2; END'