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 d2aa8d0ca3f88a8519aef57a56dee5990c50a9f2..dfd20d0c90f27d3ebbc86b01ecd531542a892ffa 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, bool inh,
  * 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 8f5fd546ebd21dd244a7da70e0ae639aab3cae58..4cb298da69a45a482e760c66f4863d5a5b571ac0 100644 (file)
--- a/src/test/regress/expected/stats_ext.out
+++ b/src/test/regress/expected/stats_ext.out
@@ -3213,6 +3213,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 5fd865f5091839c319ec9228973801e17bdd0640..07fcfc5b76527c2cbbc976674fdba79f249c4151 100644 (file)
--- a/src/test/regress/sql/stats_ext.sql
+++ b/src/test/regress/sql/stats_ext.sql
@@ -1609,6 +1609,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'