Hi,
here's a minor update of the patch, rebased to a current master and
addressing a couple issues reported by cfbot. Most are minor tweaks, but
the last one (4) is a somewhat more serious issue.
1) "tid" might have not been initialized in the IndexNext loop
2) add enable_indexonlyfilter GUC to postgresql.conf.sample (which is
checked by one regression test)
3) accepts a couple plan changes, either switching to index scan (thanks
to the costing changes) or showing the extra index-only filters in the
explain output. The plan changes seem reasonable.
4) problems with opcintype != opckeytype (name_ops)
While running the tests, I ran into an issue with name_ops, causing
failures for \dT and other catalog queries. The root cause is that
name_ops has opcintype = name, but opckeytype = cstring. The index-only
clauses are copied from the table, with Vars mutated to reference the
INDEX_VAR. But the type is not, so when we get to evaluating the
expressions, CheckVarSlotCompatibility() fails because the Var has name,
but the iss_IndexSlot (created with index tuple descriptor) has cstring.
The rebased patch fixes this by explicitly adjusting types of the
descriptor in ExecInitIndexScan().
However, maybe this indicates the very idea of evaluating expressions
using slot with index tuple descriptor is misguided. This made me look
at regular index-only scan (nodeIndexonlyscan.c), and that uses a slot
with the "table" structure, and instead of evaluating the expression on
the index index tuple it expands the index tuple into the table slot.
Which is what StoreIndexTuple() does.
So maybe this should do what IOS does - expand the index tuple into
"table slot" and evaluate the expression on that. That'd also make the
INDEX_VAR tweak in createplan.c unnecessary - in fact, that seemed a bit
strange anyway, so ditching fix_indexfilter_mutator would be good.
However, I wonder if the stuff StoreIndexTuple() is doing is actually
safe. I mean, it's essentially copying values from the index tuple into
the slot, ignoring the type difference. What if opcintype and opckeytype
are not binary compatible? Is it possible to define an opclass with such
opckeytype? I haven't notice any check enforcing such compatibility ...
Also, it's a bit confusing the SGML docs say opckeytype is not supported
for btree, but name_ops clearly does that. Later I found it's actually
mentioned in pg_opclass.dat as a hack, to save space in catalogs.
But then btree also has amstorage=false ...
regards
--
Tomas Vondra
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company