Re: Refactoring IndexPath representation of index conditions
Alvaro Herrera writes: > On 2019-Feb-02, Tom Lane wrote: >> Another idea that I looked into is to not create RestrictInfos for >> derived indexqual clauses, with the hope that that would further >> reduce the added overhead for the commuted-clause case. However >> that crashed and burned when I found out that the extended-stats >> machinery punts when given a bare clause rather than a RestrictInfo. >> It could possibly be fixed to not do that, but it looks like the >> consequences would be extra lookups that'd probably cost more than >> we saved by omitting the RestrictInfo. Also, having RestrictInfos >> means that we can cache selectivity estimates across multiple >> calls. I'm not entirely sure how much that matters in this >> context, but it's probably not negligible. > Is it reasonable to give ext-stats the option to receive either a > "plain" clause or a RestrictInfo, and if the former have it construct > the RestrictInfo and return it? No, I don't think it'd be sane to have ext-stats modify that data structure after-the-fact. Too much risk of trouble (he says while eyeing the GEQO machinery warily); plus, if we did it like that, we'd *definitely* be giving up the ability to cache and share cost/selectivity numbers between ext-stats and other places. > It seems a pity to waste effort to > cater for ext-stats, only to be used in the rare case where any > ext-stats actually exist ... most of the time, it'd be wasted effort. I'm not sure it's a good idea to design on the assumption that ext-stats are rare. I think they'll get more common over time. Right now that machinery is hardly built out at all, but it's coming. regards, tom lane
Re: Refactoring IndexPath representation of index conditions
On 2019-Feb-02, Tom Lane wrote: > In any case I think it makes things simpler and clearer, which is > worth a good deal. Looking at the patch, I agree -- this is clearer than what was there before. > Another idea that I looked into is to not create RestrictInfos for > derived indexqual clauses, with the hope that that would further > reduce the added overhead for the commuted-clause case. However > that crashed and burned when I found out that the extended-stats > machinery punts when given a bare clause rather than a RestrictInfo. > It could possibly be fixed to not do that, but it looks like the > consequences would be extra lookups that'd probably cost more than > we saved by omitting the RestrictInfo. Also, having RestrictInfos > means that we can cache selectivity estimates across multiple > calls. I'm not entirely sure how much that matters in this > context, but it's probably not negligible. Is it reasonable to give ext-stats the option to receive either a "plain" clause or a RestrictInfo, and if the former have it construct the RestrictInfo and return it? It seems a pity to waste effort to cater for ext-stats, only to be used in the rare case where any ext-stats actually exist ... most of the time, it'd be wasted effort. -- Álvaro Herrerahttps://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Re: Refactoring IndexPath representation of index conditions
Andres Freund writes: > On 2019-02-02 11:29:10 -0500, Tom Lane wrote: >> I think that the original idea here was that we should do as little >> work as possible "up front", since most index paths will get discarded >> before we reach createplan.c. But to the extent that that was valid >> at all, it's gotten overtaken by circumstances. In particular, >> postponing work to expand_indexqual_conditions (which is called by >> create_index_path) is just stupid, because these days we typically >> call that several times with the same index conditions. It's really >> dubious that postponing commutation to createplan.c is a net win either, > It seems your approach isn't particularly in contradiction to the > stated historical goal. We could create the new struct, but just not > populate it eagerly, right? Not really. A large part of the point here is to record what match_clause_to_indexcol has found out rather than have to rediscover it later (perhaps repeatedly). Concretely, the main extra cost that I've added in this patch (that wouldn't have been paid anyway by the time we've finished create_index_path) is creation of a commuted OpExpr and a RestrictInfo for it, in cases where we have an operator that matches the index but the index column is on the right. I was able to reduce that cost quite a bit by adding a bespoke "commute_restrictinfo" function, but it's still a few palloc's more than we did before. However, I think the benefits are substantial: subsequent code can uniformly assume that indexquals have indexcol-on-left, rather than having to figure that out repeatedly, and we can avoid repeated syscache lookups to find out the OID of the commutator operator. The patch as posted is still doing one more commutator lookup than it needs to, but that'll be fixed by folding expand_indexqual_conditions and match_clause_to_indexcol into one step. Also I'm not sure if I've found all the places that are expending now-useless effort for indexcol-on-right or not; I've not looked exhaustively. So at the worst this choice seems to be a wash in terms of cycles, but I have hopes that it'll be a win before all the dust settles. In any case I think it makes things simpler and clearer, which is worth a good deal. Another idea that I looked into is to not create RestrictInfos for derived indexqual clauses, with the hope that that would further reduce the added overhead for the commuted-clause case. However that crashed and burned when I found out that the extended-stats machinery punts when given a bare clause rather than a RestrictInfo. It could possibly be fixed to not do that, but it looks like the consequences would be extra lookups that'd probably cost more than we saved by omitting the RestrictInfo. Also, having RestrictInfos means that we can cache selectivity estimates across multiple calls. I'm not entirely sure how much that matters in this context, but it's probably not negligible. regards, tom lane
Re: Refactoring IndexPath representation of index conditions
Hi, On 2019-02-02 11:29:10 -0500, Tom Lane wrote: > I think that the original idea here was that we should do as little > work as possible "up front", since most index paths will get discarded > before we reach createplan.c. But to the extent that that was valid > at all, it's gotten overtaken by circumstances. In particular, > postponing work to expand_indexqual_conditions (which is called by > create_index_path) is just stupid, because these days we typically > call that several times with the same index conditions. It's really > dubious that postponing commutation to createplan.c is a net win either, It seems your approach isn't particularly in contradiction to the stated historical goal. We could create the new struct, but just not populate it eagerly, right? > Thoughts? If there's not objections I'd like to push this soon. Seems reasonable from a very very quick skim. Andres
