Re: Proof of concept: auto updatable views [Review of Patch] - Mailing list pgsql-hackers

Continuing to work on this ... I found multiple things I didn't like
about the permission-field update code.  Attached are some heavily
commented extracts from the code as I've changed it.  Does anybody
object to either the code or the objectives given in the comments?
        regards, tom lane


/** adjust_view_column_set - map a set of column numbers according to targetlist** This is used with simply-updatable
viewsto map column-permissions sets for* the view columns onto the matching columns in the underlying base relation.*
Thetargetlist is expected to be a list of plain Vars of the underlying* relation (as per the checks above in
view_is_auto_updatable).*/
static Bitmapset *
adjust_view_column_set(Bitmapset *cols, List *targetlist)
{Bitmapset  *result = NULL;Bitmapset  *tmpcols;AttrNumber    col;
tmpcols = bms_copy(cols);while ((col = bms_first_member(tmpcols)) >= 0){    /* bit numbers are offset by
FirstLowInvalidHeapAttributeNumber*/    AttrNumber    attno = col + FirstLowInvalidHeapAttributeNumber;
 
    if (attno == InvalidAttrNumber)    {        /*         * There's a whole-row reference to the view.  For
permissions        * purposes, treat it as a reference to each column available from         * the view.  (We should
*not*convert this to a whole-row         * reference to the base relation, since the view may not touch         * all
columnsof the base relation.)         */        ListCell   *lc;
 
        foreach(lc, targetlist)        {            TargetEntry *tle = (TargetEntry *) lfirst(lc);            Var
   *var;
 
            if (tle->resjunk)                continue;            var = (Var *) tle->expr;            Assert(IsA(var,
Var));           result = bms_add_member(result,                     var->varattno -
FirstLowInvalidHeapAttributeNumber);       }    }    else    {        /*         * Views do not have system columns, so
wedo not expect to see         * any other system attnos here.  If we do find one, the error         * case will apply.
       */        TargetEntry *tle = get_tle_by_resno(targetlist, attno);
 
        if (tle != NULL && IsA(tle->expr, Var))        {            Var           *var = (Var *) tle->expr;
            result = bms_add_member(result,                     var->varattno - FirstLowInvalidHeapAttributeNumber);
   }        else            elog(ERROR, "attribute number %d not found in view targetlist",                 attno);
}}bms_free(tmpcols);
return result;
}


/* * Mark the new target RTE for the permissions checks that we want to * enforce against the view owner, as distinct
fromthe query caller.  At * the relation level, require the same INSERT/UPDATE/DELETE permissions * that the query
callerneeds against the view.  We drop the ACL_SELECT * bit that is presumably in new_rte->requiredPerms initially. * *
Note:the original view RTE remains in the query's rangetable list. * Although it will be unused in the query plan, we
needit there so that * the executor still performs appropriate permissions checks for the * query caller's use of the
view.*/new_rte->checkAsUser = view->rd_rel->relowner;new_rte->requiredPerms = view_rte->requiredPerms;
 
/* * Now for the per-column permissions bits. * * Initially, new_rte contains selectedCols permission check bits for
all* base-rel columns referenced by the view, but since the view is a SELECT * query its modifiedCols is empty.  We set
modifiedColsto include all * the columns the outer query is trying to modify, adjusting the column * numbers as needed.
But we leave selectedCols as-is, so the view owner * must have read permission for all columns used in the view
definition,* even if some of them are not read by the upper query.  We could try to * limit selectedCols to only
columnsused in the transformed query, but * that does not correspond to what happens in ordinary SELECT usage of a *
view:all referenced columns must have read permission, even if * optimization finds that some of them can be discarded
duringquery * transformation.  The flattening we're doing here is an optional * optimization, too.  (If you are
unpersuadedand want to change this, * note that applying adjust_view_column_set to view_rte->selectedCols is * clearly
*not*the right answer, since that neglects base-rel columns * used in the view's WHERE quals.)
*/Assert(bms_is_empty(new_rte->modifiedCols));new_rte->modifiedCols= adjust_view_column_set(view_rte->modifiedCols,
                                         view_targetlist);