Re: API change advice: Passing plan invalidation info from the rewriter into the planner?

Craig Ringer escribió:

> One of the remaining issues with row security is how to pass plan
> invalidation information generated in the rewriter back into the planner.

I think I already asked this, but would it work to extract this info by
walking the rewritten list of queries instead; and in case it would,
would that be any easier than the API change you're proposing?

> We use Query structs throughout the rewriter and planner; it doesn't
> make sense to add a List* field for PlanInvalItem nodes and an Oid field
> for the user ID to the Query node when it's only ever going to get used
> for the top level Query node returned by the rewriter, and only for long
> enough to copy the data into PlannerGlobal.

So there is an assumption that you can't have a subquery that uses a
different role ID than the main query. That sounds fine, and anyway I
don't think we're prepared to deal with differing userids for
subqueries, so the proposal that it belongs only on the top-level node
is acceptable. And from there, it seems that not putting the info in
Query (which would be a waste everywhere else than the toplevel query
node) is sensible.

> The alternative seems to be changing the return type of QueryRewrite,
> introducing a new node type, say:
>
> struct RewriteResult {
> Query *productQuery;
> Oid planUserId;
> List* planInvalItems;
> }
>
> This seems cleaner, and more extensible, but it means changing a fair
> bit of API, including:
>
> pg_plan_query
> planner
> standard_planner
> planner_hook_type
> QueryRewrite

I think we should just bite the bullet and do the change (a new struct,
I assume, not a new node). It will cause an incompatibility to anyone
that has written planner hooks, but probably the number of such hooks is
not very large anyway.

I don't think we should base decisions on the amount of backpatching
pain we cause, for patches that involve new functionality such as this
one. We commit patches that will cause future merge conflicts all the
time.

> I'm inclined to bite the bullet and make the API change. It'll be a
> pain, but I can see future uses for passing global info out of the
> rewriter rather than shoving it into per-Query structures. I'd define a
> RewriteResult and pass that down into all the rewriter internal
> functions, then return the outer query wrapped in it.

Is there already something in Query that could be a toplevel struct
member only?

--
Álvaro Herrera http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services