Re: Index Skip Scan (new UniqueKeys)

On 24/10/2020 19:45, Dmitry Dolgov wrote:
> Here is a new version which doesn't require "scanstart" argument and
> contains few other changes to address the issues mentioned earlier. It's
> also based on the latest UniqueKeys patches with the valgrind issue
> fixed (as before they're attached also just for the references, you can
> find more in the original thread). I didn't rework commentaries yet,
> will post it soon (need to get an inspiration first, probably via
> reading Shakespeare unless someone has better suggestions).

I had a quick look at this patch. I haven't been following this thread, 
so sorry if I'm repeating old arguments, but here we go:

- I'm surprised you need a new index AM function (amskip) for this. 
Can't you just restart the scan with index_rescan()? The btree AM can 
check if the new keys are on the same page, and optimize the rescan 
accordingly, like amskip does. That would speed up e.g. nested loop 
scans too, where the keys just happen to be clustered.

- Does this optimization apply to bitmap index scans?

- This logic in build_index_paths() is not correct:

> +        /*
> +         * Skip scan is not supported when there are qual conditions, which are not
> +         * covered by index. The reason for that is that those conditions are
> +         * evaluated later, already after skipping was applied.
> +         *
> +         * TODO: This implementation is too restrictive, and doesn't allow e.g.
> +         * index expressions. For that we need to examine index_clauses too.
> +         */
> +        if (root->parse->jointree != NULL)
> +        {
> +            ListCell *lc;
> +
> +            foreach(lc, (List *)root->parse->jointree->quals)
> +            {
> +                Node *expr, *qual = (Node *) lfirst(lc);
> +                Var *var;
> +                bool found = false;
> +
> +                if (!is_opclause(qual))
> +                {
> +                    not_empty_qual = true;
> +                    break;
> +                }
> +
> +                expr = get_leftop(qual);
> +
> +                if (!IsA(expr, Var))
> +                {
> +                    not_empty_qual = true;
> +                    break;
> +                }
> +
> +                var = (Var *) expr;
> +
> +                for (int i = 0; i < index->ncolumns; i++)
> +                {
> +                    if (index->indexkeys[i] == var->varattno)
> +                    {
> +                        found = true;
> +                        break;
> +                    }
> +                }
> +
> +                if (!found)
> +                {
> +                    not_empty_qual = true;
> +                    break;
> +                }
> +            }
> +        }

If you care whether the qual is evaluated by the index AM or not, you 
need to also check that the operator is indexable. Attached is a query 
that demonstrates that problem.

I'm actually a bit confused why we need this condition. The IndexScan 
executor node should call amskip() only after checking the additional 
quals, no?

Also, you should probably check that the index quals are in the operator 
family as that used for the DISTINCT.

- Heikki