On Sat, Oct 25, 2014 at 8:12 AM, Robert Haas <robertmh...@gmail.com> wrote: >> Generating index paths for the UPDATE is a waste of cycles. >> Theoretically, there could be an (a, b, c) unique index and a (c,b,a) >> unique index, and those two might have a non-equal cost to scan. But >> that almost certainly isn't going to happen in practice, since that's >> a rather questionable indexing strategy, and even when it does, you're >> going to have to insert into all the unique indexes a good proportion >> of the time anyway, making the benefits of that approach pale in >> comparison to the costs. > > I don't care whether you actually generate index-paths or not, and in > fact I suspect it makes no sense to do so. But I do care that you do > a cost comparison between the available indexes and pick the one that > looks cheapest. If somebody's got a bloated index and builds a new > one, they will want it to get used even before they drop the old one.
That seems extremely narrow. I am about ready to just do what you say, by costing the index based on something like relpages, but for the record I see no point. If I see no point, and you're sure that there is a point, then there is a danger that I'll miss the point, which contributes to my giving push back. I know I said that already, but I reiterate it once more for emphasis. > Even if that weren't an issue, though, the fact that you can't > re-parse but you can re-plan is a killer point AFAICS. It means you > are borked if the statement gets re-executed after the index you > picked is dropped. That simply isn't the case. As specifically noted in comments in the patch, relcache invalidation works in such a way as to invalidate the query proper, even when only an index has been dropped. For a time during development, the semantic dependence was more directly represented by actually having extract_query_dependencies() extract the arbitrating unique index pg_class Oid as a dependency for the benefit of the plan cache, but I ultimately decided against doing anything more than noting the potential future problem (the problem that arises in a world where relcache invalidation is more selective). But, I digress: I'll have inference occur during planning during the next revision since you think that's important. >> From my point of view, I spent a significant amount of time making the >> patch more or less match your proposed design for unique index >> inference. It is discouraging to hear that you think I'm not >> cooperating with community process. I'm doing my best. I think it >> would be a bad idea for me to not engage with the community for an >> extended period at this point. There were plenty of other issues >> address by V1.3 that were not the CONFLICTING()/EXCLUDING thing that >> you highlighted (or the other thing you highlighted around where to do >> unique index inference). > > I think this gets at a point that has been bugging me and, perhaps, > other people here. You often post a new patch with some fixes but > without fixes for the issues that reviewers have indicated are > top-of-mind for them. Sometimes, but not always, you say something > like "I know this doesn't fix X but I'd like comments on other aspects > of the patch". Even when you do, though, it can be difficult to > overlook something that appears to be a fundamental structural problem > to comment on details, and sometimes it feels like that's what we're > being asked to do. I think that it's accurate to say that I asked the community to do that once, last year. I was even very explicit about it at the time. I'm surprised that you think that's the case now, though. I learned my lesson a little later: don't do that, because fellow contributors are unwilling to go along with it, even for something as important as this. As recently as a few months ago, you wanted me to go a *completely* different direction with this (i.e. attempt an UPDATE first). I'm surprised that you're emphasizing the EXCLUDED()/CONFLICTING() thing so much. Should I take it that I more or less have your confidence that the way the patch fits together at a high level is sound? Because, that's the only way I can imagine you'd think that the EXCLUDED()/CONFLICTING() thing is of such fundamental importance at this point. It is after all split out as a much smaller commit on top of the main implementation (a commit which could easily be deferred). While it's important, it is very clearly subsidiary to the overall structure of my design, which I think that there has been precious little discussion of on this thread (e.g. the whole way I use EvalPlanQual(), the general idea of a special auxiliary plan that is executed in a special way, etc). That concerns me, because I suspect that the reason there has been next to no discussion of those aspects is because they're particularly hard things to have an opinion on, and yet are the things of immediate importance. Please correct me if I have it wrong. I am in no position to demand that you or anyone else discuss any particular aspect of the patch, of course. I am just conveying my concerns. Like all of us, I very much want to get this feature into the next release of PostgreSQL. > When I'm reviewing, I tend to find issues more or > less in proportion to the time I spend on the patch. If things that I > complained about before haven't been fixed, I tend to find the same > ones again, but not necessarily all that much faster than I found them > the first time. So that's not efficient for me. I would not make an > absolute rule that no patch should ever be re-posted without > addressing every issue; I wouldn't be able to follow that rule in > every case myself. But I try to follow it as often as I can, and I > would suggest that you try to lean quite a bit more firmly in that > direction. I think you will make more progress, and spend less time > arguing with others. I'll try. But let me point out that the *very first* thing you complained about, in relation to version 1.0, was that the plan structure was funny. V1.3 cleaned that up, making a "sequential scan" always occur, and having EXPLAIN treat that as a strict implementation detail, while still attributing some aspects of the hidden, unexecuted "sequential scan" (e.g. the qual) to its parent where that makes sense. This seems like something that squarely addressed your *original* concern. A little later, you (rightly) complained about the lack of support for inheritance, and to a lesser extent updatable views. As of V1.3, I have reasonable support for both of those things. And, of course, you wanted a unique index to be inferred from a set of columns/expressions, which (most notably) v1.3 now does. Also, I incorporated feedback from Kevin on some of those same items, as well as the behavior of statement-level triggers. Furthermore, I incorporated the feedback of Simon in having way more tests, in particular, two new isolation tests, one of which illustrates the qualitatively new "MVCC violation" in more detail. In short, I think I've done a pretty good job of incorporating feedback, and where I haven't I have been quite clear about it (it certainly didn't take you long to figure it out in this most recent instance). There is always room for improvement, but in my book V1.3 made a lot more than small, incremental progress. I am surprised by your remarks suggesting that the progress of each revision is excessively gradual in addressing your concerns. AFAICT, it's just that V1.3 wasn't totally comprehensive in doing so. In reality, the main reason for that is: getting this feature to a point where it might plausibly be committed is bloody difficult, as evidenced by the fact that it took this long for someone to produce a patch. Please don't lose sight of that. -- Peter Geoghegan -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
