knngist patch support
in [PgSql]
|
Prev: [HACKERS] knngist patch support
Next: pgsql: Remove old-style VACUUM FULL (which wasknown for a little while
From: Greg Stark on 11 Feb 2010 09:51 On Thu, Feb 11, 2010 at 1:18 PM, Robert Haas <robertmhaas(a)gmail.com> wrote: > >> In my understanding >> this was always enough to submit code. User's documentation is depend on >> discussion and review and can be added later >> before releasing beta. > > Several people have said this lately, but it doesn't match what I've > seen of our practice over the last year and a half; Perhaps the confusion is that we often say not to worry about the quality of the English in the documentation. That's because it's easy for a reviewer to fix up the English but not so easy to figure out what you intend the behaviour to be. -- greg -- Sent via pgsql-hackers mailing list (pgsql-hackers(a)postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
From: Dimitri Fontaine on 11 Feb 2010 09:19 Robert Haas <robertmhaas(a)gmail.com> writes: > It seems that you're sort of frustrated with the system and the need > to go through a process before committing a patch; I've been handling arround here for years (since 2005 or before) and I think there always was a process. The only change is it's getting more and more formal. But still not any clearer. It's good to try to keep the major releases one year apart, but until now we had some flexibility towards developpers. They could have their own agenda then appear with a big patch and it was getting considered. We never asked contributors to arrange for being able to find a sponsor, do the closed source version, prepare for publishing, and then send a patch in a timely maneer so that to ease the integration and release. Before there was a Commit Fest process, we took some weeks then months at the end of the cycle to consider what had been accumulated. The new process is there for giving more feedback to developpers, and is being considered now as a way to get better control about the release agenda. I'm not sure it's a good tool for that. I'm not sure insisting that much on the release schedule is a good idea. Once more making compromises is easy. What's hard and challenging is making *good* compromises. > IMHO, our system has to work for both developers and users, and it has > to work for both committers and non-committers. That's an easy goal to share. The question is how you get there without losing existing developpers and possibly attracting new developpers on the way. -- dim -- Sent via pgsql-hackers mailing list (pgsql-hackers(a)postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
From: Robert Haas on 12 Feb 2010 13:14 2010/2/11 Oleg Bartunov <oleg(a)sai.msu.su>: > On Thu, 11 Feb 2010, Robert Haas wrote: > >> On Thu, Feb 11, 2010 at 3:00 AM, Oleg Bartunov <oleg(a)sai.msu.su> wrote: >>>> >>>> version I saw hadn't any documentation whatever. It's not committable >>>> on documentation grounds alone, even if everybody was satisfied about >>>> the code. >>> >>> well, there is enough documentation to review patch. >> >> Where is there any documentation at all? There are no changes to doc/ >> at all; no README; and not even a lengthy comment block anywhere that >> I saw. Nor did the email in which the patch was submitted clearly lay >> out the design of the feature. > > Well, initial knngist announce > http://archives.postgresql.org/pgsql-hackers/2009-11/msg01547.php > isn't enough to review ? We made test data available to reproduce results, > see http://www.sai.msu.su/~megera/wiki/2009-11-25 > We are here and open to any reviewer's question. Well, just for example, that doesn't document the changes you made to the opclass infrastructure, and the reasons for those decisions. Actually, I've been working on an email on that topic but haven't gotten around to finishing it. There's some description of the overall goal of the feature but not much about how you got there. Is it enough to review? Perhaps, but it would certainly be nice to have some more discussion of the overall design. IMHO, anyway. ....Robert -- Sent via pgsql-hackers mailing list (pgsql-hackers(a)postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
From: Robert Haas on 12 Feb 2010 16:14 On Fri, Feb 12, 2010 at 3:44 PM, Oleg Bartunov <oleg(a)sai.msu.su> wrote: > This is not fair,Robert. Everything was discussed in -hackers.I assume > reviewer > should follow discussion at least, he is a member of our community. Mailing > list archive was/is/will our the best knowledge base. Dude, there's no fair or unfair here; I'm just telling you what I think. There are not six people who follow this mailing list more closely than I do, and when I started looking at this patch it took me two hours to figure out why you made those changes to the opclass machinery. It's not documented anywhere either in the patch or in the email in which the patch was submitted. That made it hard for me. If that makes me stupid, then I'm stupid, but then probably there are a lot of stupid people around here. > For example, regarding > changes in the opclass infrastructure you complain, you can see your reply > to Teodor's message > http://archives.postgresql.org/message-id/603c8f070912292255u30e1983bi22ed5778bd2ce890(a)mail.gmail.com > which contains description of amcanorderbyop flag. OK, so in one email message that is not the email in which you submitted the patch there is one sentence of explanation that I failed to remember six weeks later when I looked at the patch. > Frankly, I think we see here limit of our resources. Let me explain this. > We splitted patch by several parts - 2 parts are about contrib modules > (rather trivial), 1 - is a gist changes, 1 - planner changes and 1 part - > some proc changes. The most serious are gist and planner patches. We develop > GiST for many years and know almost everything there and could say that > we're > responsible for GiST. I don't know if anybody from -hackers could review our > patch for planner better than Tom, but he is busy and will be busy. > So, any serious feature, which touch planner doomed to be rejected because > of > lack of reviewer. I do think resource limitations play a role. For at least as long as I have been involved in the community, we have relied very heavily on Tom Lane to review nearly every patch and commit more than half of them. As our group of developers grows, that is unsustainable, which I believe to be part of the reason that -core just created four new committers; as well as part of the reason for the CommitFest process, which tries to enlist non-committer reviewers. But none of us are Tom Lane. The part of your patch that took me two hours to figure out probably would have taken Tom twenty minutes (maybe less). But even if we assume that I am as smart as Tom Lane and that I will spend as much time working on PostgreSQL as Tom Lane, both of which may well be false, I won't know the code base as well as he does now until ~2020. The only way we can get past that is, first, by splitting up the work across as many people as possible, and second, by making it as easy as possible for the reviewer to understand what the code is about. And at least if the reviewer is me, *documentation helps*. I'm going to respond to the part of this email that's about moving this patch forward with a separate email. ....Robert -- Sent via pgsql-hackers mailing list (pgsql-hackers(a)postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
From: Robert Haas on 12 Feb 2010 16:29
On Fri, Feb 12, 2010 at 3:44 PM, Oleg Bartunov <oleg(a)sai.msu.su> wrote: > We tried to find compromise for 9.0 (Tom suggests contrib module), but all > variants are ugly and bring incompatibility in future. If there are no > hackers > willing/capable to review our patches, then, please, help us how to save > neighbourhood search without incompatibility in future. Here's what I've gathered from my read through this patch. Let me know if it's right: In CVS HEAD, if an AM is marked amcanorder, that means that the index defines some kind of intrinsic order over the tuples so that, from a given starting point, it makes sense to talk about scanning either forward or backward. GIST indices don't have an intrinsic notion of ordering, but the structure of the index does lend itself to finding index tuples that are "nearby" to some specified point. So this patch wants to introduce the notion of an AM that is marked amcanorderbyop to accelerate queries that order by <indexed column> <op> <constant>. To do that, we need some way of recognizing which operators are candidates for this optimization. This patch implements that by putting the relevant operators into the operator class. That requires relaxing the rule that operator class operators must return bool; so instead when the AM is marked amcanorderbyop we allow the operator class operators to return any type with a default btree operator class. Does that sound right? Tom remarked in another email that he wasn't too happy with the opclass changes. They seem kind of grotty to me, too, but I don't immediately have a better idea. My fear is that there may be places in the code that rely on opclass operators only ever returning bool, and that changing that may break things. It also feels like allowing non-bool-returning opclass members only for this one specific case is kind of a hack: is this an instance of some more general problem that we ought to be solving in some more general way? Not sure. In any case, it seems to me that figuring out how we're going to solve the problem of marking the operators (or otherwise identifying the expression trees) that are index-optimizable is the key issue for this patch. If we can agree on that, the rest should be a SMOP. ....Robert -- Sent via pgsql-hackers mailing list (pgsql-hackers(a)postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers |