Re: POC, WIP: OR-clause support for indexes - Mailing list pgsql-hackers
From | Tomas Vondra |
---|---|
Subject | Re: POC, WIP: OR-clause support for indexes |
Date | |
Msg-id | 0c2fb2ac-332d-6c70-3128-34ba9d13f7e5@2ndquadrant.com Whole thread Raw |
In response to | Re: POC, WIP: OR-clause support for indexes (Teodor Sigaev <teodor@sigaev.ru>) |
Responses |
Re: POC, WIP: OR-clause support for indexes
|
List | pgsql-hackers |
Hi Teodor, Sadly the v4 does not work for me - I do get assertion failures. For example with the example Andreas Karlsson posted in this thread: CREATE EXTENSION btree_gin; CREATE TABLE test (a int, b int, c int); CREATE INDEX ON test USING gin (a, b, c); INSERT INTO test SELECT i % 7, i % 9, i % 11 FROM generate_series(1, 1000000) i; EXPLAIN ANALYZE SELECT * FROM test WHERE (a = 3 OR b = 5) AND c = 2; It seems working, but only until I run ANALYZE on the table. Once I do that, I start getting crashes at this line *qualcols = list_concat(*qualcols, list_copy(idx_path->indexqualcols)); in convert_bitmap_path_to_index_clause. Apparently one of the lists is T_List while the other one is T_IntList, so list_concat() errors out. My guess is that the T_BitmapOrPath branch should do oredqualcols = list_concat(oredqualcols, li_qualcols); ... *qualcols = list_concat(qualcols, oredqualcols); instead of oredqualcols = lappend(oredqualcols, li_qualcols); ... *qualcols = lappend(*qualcols, oredqualcols); but once I fixed that I got some other assert failures further down, that I haven't tried to fix. So the patch seems to be broken, and I suspect this might be related to the broken index condition reported by Andreas (although I don't see that - I either see correct condition or assertion failures). On 03/17/2016 06:19 PM, Teodor Sigaev wrote: ... >> >> 7) I find it rather ugly that the paths are built by converting BitmapOr >> paths. Firstly, it means indexes without amgetbitmap can't benefit from >> this change. Maybe that's reasonable limitation, though? > I based on following thoughts: > 1 code which tries to find OR-index path will be very similar to existing > generate_or_bitmap code. Obviously, it should not be duplicated. > 2 all existsing indexes have amgetbitmap method, only a few don't. > amgetbitmap > interface is simpler. Anyway, I can add an option for generate_or_bitmap > to use any index, but, in current state it will just repeat all work. I agree that the code should not be duplicated, but is this really a good solution. Perhaps a refactoring that'd allow sharing most of the code would be more appropriate. >> >> But more importantly, this design already has a bunch of unintended >> consequences. For example, the current code completely ignores >> enable_indexscan setting, because it merely copies the costs from the >> bitmap path.> > I'd like to add separate enable_indexorscan That may be useful, but why shouldn't enable_indexscan=off also disable indexorscan? I would find it rather surprising if after setting enable_indexscan=off I'd still get index scans for OR-clauses. > >> That's pretty dubious, I guess. So this code probably needs to be made >> aware of enable_indexscan - right now it entirely ignores startup_cost >> in convert_bitmap_path_to_index_clause(). But of course if there are >> multiple IndexPaths, the enable_indexscan=off will be included multiple >> times. ... and it does not address this at all. I really doubt a costing derived from the bitmap index scan nodes will make much sense - you essentially need to revert unknown parts of the costing to only include building the bitmap once, etc. regards -- Tomas Vondra http://www.2ndQuadrant.com PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
pgsql-hackers by date: