Thanks for the patch. I tested it on REL_18_STABLE-equivalent sources (18.6,
built from source), since that is the branch this would need to be
back-patched to. Short version: it fixes the bug, it passes the full
regression suite, and I believe it also rejects a legitimate plan.
What works
----------
Applied cleanly (hunk offset 2 lines). With the test module:
before: Index Only Scan using i_nokey -> ERROR: no data returned...
after: Seq Scan (disabled) -> 3 correct
make check: all 231 tests passed.
What I think is a false positive
--------------------------------
The guard keys off bms_is_empty(index_canreturn_attrs), but that bitmapset
is
also empty for an index whose columns are all expressions, because the
loop
just above skips them:
/*
* For the moment, we just ignore index expressions. It might be nice
* to do something with them, later.
*/
if (attno == 0)
continue;
So "empty" does not mean "the AM can return nothing", it means "no plain
columns are returnable". A btree over an expression can feed an index-only
scan perfectly well. Measured on 18.6, with enable_seqscan off:
CREATE TABLE t_expr (a int, b int);
INSERT INTO t_expr SELECT g, g*2 FROM generate_series(1,50000) g;
CREATE INDEX i_expr ON t_expr ((a + b));
VACUUM ANALYZE t_expr;
SELECT count(*) FROM t_expr;
unpatched: Aggregate -> Index Only Scan using i_expr on t_expr
patched: Aggregate -> Seq Scan on t_expr (disabled)
Both return 50000, so this is a plan regression rather than a correctness
one -- counting can no longer walk the smaller index. A control with an
ordinary column index (CREATE INDEX i_col ON t_col (a)) keeps its index-only
scan under the patch, so the effect is specific to expression-only indexes.
Worth noting: make check does not catch this. The suite passed 231/231 with
the patch applied, so this would go in unnoticed.
A variant that avoids it
------------------------
Attached as a patch this time, rather than an archive. It tests the AM's
capability directly instead of the bitmapset:
if (result)
{
bool any_canreturn = false;
for (i = 0; i < index->ncolumns; i++)
{
if (index->canreturn[i])
{
any_canreturn = true;
break;
}
}
if (!any_canreturn)
result = false;
}
index->canreturn[] is filled per column from index_can_return() in
plancat.c,
including expression columns, so an expression btree has a true entry
while an
AM with amcanreturn == NULL has none.
Measured on 18.6 with that variant:
the reproducer -> 3, correct (bug fixed)
count(*) over i_expr -> Index Only Scan (no regression)
count(*) over i_col -> Index Only Scan (unchanged)
make check -> all 231 tests passed
The patch is against 18.6 sources, since that is what I tested on; it should
apply to master with an offset.
I have not tried to judge which shape you would prefer, and there may be a
reason to keep it keyed off the bitmapset that I am not seeing. I can rerun
any of this on 19beta2 as well if that is useful.
The attached tarball includes that script as alcance.sql.
In the future, it would be better to attach patches rather than archives.
I kept the fix minimal: one guard after bms_is_subset() in check_index_only(), rejecting the plan when no key column is returnable.
Regards,
Rachitskiy Andrey
-- Saludos cordiales,
Manuel Reyes
