Hi, On 2019-01-19 17:04:13 +1300, Edmund Horner wrote: > On Sat, 19 Jan 2019 at 05:35, Tom Lane <t...@sss.pgh.pa.us> wrote: > > > Edmund Horner <ejr...@gmail.com> writes: > > > My patch uses the same path type and executor for all extractable > > tidquals. > > > > > This worked pretty well, but I am finding it difficult to reimplement it > > in > > > the new tidpath.c code. > > > > I didn't like that approach to begin with, and would suggest that you go > > over to using a separate path type and executor node. I don't think the > > amount of commonality for the two cases is all that large, and doing it > > as you had it required some ugly ad-hoc conventions about the semantics > > of the tidquals list. Where I think this should go is that the tidquals > > list still has OR semantics in the existing path type, but you use AND > > semantics in the new path type, so that "ctid > ? AND ctid < ?" is just > > represented as an implicit-AND list of two simple RestrictInfos. > > > > Thanks for the advice. This approach resembles my first draft, which had a > separate executor type. However, it did have a combined path type, with an > enum TidPathMethod to determine how tidquals was interpreted. At this > point, I think a different path type is clearer, though generation of both > types can live in tidpath.c (just as indxpath.c generates different index > path types). > > > > Now admittedly, this wouldn't give us an efficient way to execute > > queries with conditions like "WHERE ctid = X OR (ctid > Y AND ctid < Z)", > > but I find myself quite unable to get excited about supporting that. > > I see no reason for the new code to worry about any cases more complex > > than one or two TID inequalities at top level of the restriction list. > > > > I'm a bit sad to see support for multiple ranges go, though I never saw > such queries as ever being particularly common. (And there was always a > nagging feeling that tidpath.c was beginning to perform feats of boolean > acrobatics out of proportion to its importance. Perhaps in some distant > future, TID quals will become another way of supplying TIDs to a bitmap > heap scan, which would enable complicated boolean queries using both > indexes and TID scans. But that's just musing, not a proposal.) > > > In the query information given to the path generator, there is no existing > > > RestrictInfo relating to the whole expression "ctid > ? AND ctid < ?". I > > > am still learning about RestrictInfos, but my understanding is it doesn't > > > make sense to have a RestrictInfo for an AND clause, anyway; you're > > > supposed to have them for the sub-expressions of it. > > > > FWIW, the actual data structure for cases like that is that there's > > a RestrictInfo for the whole clause ctid = X OR (ctid > Y AND ctid < Z), > > and if you look into its "orclause" field, you will find RestrictInfos > > attached to the primitive clauses ctid = X, ctid > Y, ctid < Z. (The > > old code in tidpath.c didn't know that, because it'd never been rewritten > > since RestrictInfos were invented.) However, I think this new code should > > not worry about OR cases at all, but just pull out top-level TID > > comparison clauses. > > > > Thanks for the explanation. > > > And it doesn't seem a good idea to try to create new RestrictInfos in the > > > path generation just to pass the tidquals back to plan creation. > > > > No, you should avoid that. There are places that assume there's only > > one RestrictInfo for any given original clause (or sub-clause).
The commitfest has ended, and you've not updated the patch to address the feedback yet. Are you planning to do so soon? Otherwise I think we ought to mark the patch as returned with feedback? Greetings, Andres Freund
