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