Re: [PATCH] Incremental sort (was: PoC: Partial sort)

On Thu, Apr 16, 2020 at 1:10 PM Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>
> James Coleman <jtc331(at)gmail(dot)com> writes:
> > On Fri, Apr 10, 2020 at 10:12 AM James Coleman <jtc331(at)gmail(dot)com> wrote:
> >> One thing I just noticed and had a question about: in
> >> preparePresortedCols (which sets up a function call context), do we
> >> need to call pg_proc_aclcheck?
>
> > Background: this came up because I noticed that pg_proc_aclcheck is
> > called in the scalar array op case in execExpr.c.
>
> > However grepping through the source code I see several places where a
> > function (including an equality op for an ordering op, like the case
> > we have here) gets looked up without calling pg_proc_aclcheck, but
> > then other places where the acl check is invoked.
>
> Rule of thumb is that we don't apply ACL checks to functions/ops
> we get out of an opclass; adding a function to an opclass is tantamount
> to giving public execute permission on it. If the function/operator
> reference came directly from the SQL query it must be checked.

All right, in that case I believe we're OK here without modification.
We're looking up the equality op based on the ordering op the planner
has already selected for sorting the query, and I'm assuming that
looking that up via the op family is in the same category as "getting
out of an opclass" (since opclasses are part of an opfamily).

Thanks for the explanation.

> > In addition, I haven't been able to discern a reason for why sometimes
> > InvokeFunctionExecuteHook gets called with the function after lookup,
> > but not others.
>
> I would not stand here and say that that hook infrastructure is worth
> anything at all. Maybe the coverage is sufficient for some use-cases,
> but who's to say?

Interesting. It does look to be particularly underused. Just grepping
for that hook invocation macro shows, for example, that it's not used
in nodeSort.c or tuplesort.c, so clearly it's not executed for the
functions we'd use in regular sort. Given that...I think we can
proceed without it here too.

James