Re: ExecRTCheckPerms() and many prunable partitions

On Thu, Jul 28, 2022 at 6:18 AM Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> ... One more thing: maybe we should rethink where to put
> extraUpdatedCols. Between the facts that it's not used for
> actual permissions checks, and that it's calculated by the
> rewriter not parser, it doesn't seem like it really belongs
> in RelPermissionInfo. Should we keep it in RangeTblEntry?
> Should it go somewhere else entirely? I'm just speculating,
> but now is a good time to think about it.

After fixing the issue related to this mentioned by Andres, I started
thinking more about this, especially the "Should it go somewhere else
entirely?" part.

I've kept extraUpdatedCols in RangeTblEntry in the latest patch, but
perhaps it makes sense to put that into Query? So, the stanza in
RewriteQuery() that sets extraUpdatedCols will be changed as:

@@ -3769,7 +3769,8 @@ RewriteQuery(Query *parsetree, List *rewrite_events)
NULL, 0, NULL);

/* Also populate extraUpdatedCols (for generated columns) */
- fill_extraUpdatedCols(rt_entry, rt_perminfo, rt_entry_relation);
+ parsetree->extraUpdatedCols =
+ get_extraUpdatedCols(rt_entry, rt_perminfo, rt_entry_relation);
}
else if (event == CMD_MERGE)
{

Then, like withCheckOptionLists et al, have grouping_planner()
populate an extraUpdatedColsBitmaps in ModifyTable.
ExecInitModifyTable() will assign them directly into ResultRelInfos.
That way, anyplace in the executor that needs to look at
extraUpdatedCols of a given result relation can get that from its
ResultRelInfo, rather than from the RangeTblEntry as now.

Thoughts?

--
Thanks, Amit Langote
EDB: http://www.enterprisedb.com