Re: On disable_cost

On Mon, May 6, 2024 at 3:26 PM Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Nah, I'm wrong: we do treat it as leakproof, and the comment about
> that in contain_leaked_vars_walker shows that the interaction with
> RLS quals *was* thought about. What wasn't thought about was the
> possibility of RLS quals that themselves could be usable as tidquals,
> which breaks this assumption in TidQualFromRestrictInfoList:
>
> * Stop as soon as we find any usable CTID condition. In theory we
> * could get CTID equality conditions from different AND'ed clauses,
> * in which case we could try to pick the most efficient one. In
> * practice, such usage seems very unlikely, so we don't bother; we
> * just exit as soon as we find the first candidate.

Right. I had noticed this but didn't spell it out.

> The executor doesn't seem to be prepared to cope with multiple AND'ed
> TID clauses (only OR'ed ones). So we need to fix this at least to the
> extent of looking for a CurrentOfExpr qual, and preferring that over
> anything else.
>
> I'm also now wondering about this assumption in the executor:
>
> /* CurrentOfExpr could never appear OR'd with something else */
> Assert(list_length(tidstate->tss_tidexprs) == 1 ||
> !tidstate->tss_isCurrentOf);
>
> It still seems OK, because anything that might come in from RLS quals
> would be AND'ed not OR'ed with the CurrentOfExpr.

This stuff I had not noticed.

> > In general I think you're right that something less rickety than
> > the disable_cost hack would be a good idea to ensure the desired
> > TidPath gets chosen, but this problem is not the fault of that.
> > We're not making the TidPath with the correct contents in the first
> > place.
>
> Still true.

I'll look into this, unless you want to do it.

Incidentally, another thing I just noticed is that
IsCurrentOfClause()'s test for (node->cvarno == rel->relid) is
possibly dead code. At least, there are no examples in our test suite
where it fails to hold. Which seems like it makes sense, because if it
didn't, then how did the clause end up in baserestrictinfo? Maybe this
is worth keeping as defensive coding, or maybe it should be changed to
an Assert or something.

--
Robert Haas
EDB: http://www.enterprisedb.com