Re: [HACKERS] Need more reviewers!
On Thu, 4 Sep 2008, Simon Riggs wrote: I think this should be organised with different kinds of reviewer... Great post. Rewrote the intro a bit and turned it into a first bit of reviewer training material at http://wiki.postgresql.org/wiki/Reviewing_a_Patch -- * Greg Smith [EMAIL PROTECTED] http://www.gregsmith.com Baltimore, MD -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Need more reviewers!
On Fri, 5 Sep 2008, Marko Kreen wrote: I think we have better results and more relaxed atmospere if we use following task description for reviewers: I assimilated this and some of your later comments into http://wiki.postgresql.org/wiki/Reviewing_a_Patch as well. I disagree with your feeling that Simon's text was too much; there's value to both a gentle intro and a detailed list of review tasks. -- * Greg Smith [EMAIL PROTECTED] http://www.gregsmith.com Baltimore, MD -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Need more reviewers!
On Sat, 2008-09-06 at 04:03 -0400, Greg Smith wrote: On Thu, 4 Sep 2008, Simon Riggs wrote: I think this should be organised with different kinds of reviewer... Great post. Rewrote the intro a bit and turned it into a first bit of reviewer training material at http://wiki.postgresql.org/wiki/Reviewing_a_Patch Well written, thanks. -- Simon Riggs www.2ndQuadrant.com PostgreSQL Training, Services and Support -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Need more reviewers!
Martijn van Oosterhout wrote: -- Start of PGP signed section. On Thu, Sep 04, 2008 at 09:54:02PM +0100, Simon Riggs wrote: * coding review - does it follow standard code guidelines? Are there portability issues? Will it work on Windows/BSD etc? Are there sufficient comments? * code review - does it do what it says, correctly? Just one thing though, I picked a random patch and started reading. However, the commitfest page doesn't link to anywhere that actually describes *what* the patch is trying to do. Many patches do have the design and the patch in one page, but some don't. I suppose what happens is the original patch comes with design and later a newer version is posted with just changes. The commitfest page points to the latter, losing former in the archive somewhere. Yep, that is a problem; the previous emails about the patch and comments are very valuable for reviewers. -- Bruce Momjian [EMAIL PROTECTED]http://momjian.us EnterpriseDB http://enterprisedb.com + If your life is a hard drive, Christ can be your backup. + -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Need more reviewers!
On Thu, 2008-09-04 at 10:45 -0700, Josh Berkus wrote: We currently have 38 patches pending, and only nine people reviewing them. At this rate, the September commitfest will take three months. If you are a postgresql hacker at all, or even want to be one, we need your help reviewing patches! There are several easy patches in the list, so I can assign them to beginners. Please volunteer now! Hi Josh, I volunteer as a reviewer, assign a patch to me. Regards Abbas www.enterprisedb.com
Re: [HACKERS] Need more reviewers!
On Thu, 2008-09-04 at 10:45 -0700, Josh Berkus wrote: If you are a postgresql hacker at all, or even want to be one, we need your help reviewing patches! There are several easy patches in the list, so I can assign them to beginners. It would be a reasonable rule that all patch submitters also have to do patch reviews. If we made it a strict rule, then sponsoring companies would know that they *must* provide money/time for that aspect also. Otherwise it is almost impossible to get formal approval to do that. I count more than 20 patch authors that are not reviewing, including myself. Of that, there are at least 5 people on their second or subsequent patch (figure probably wildly inaccurate, since don't keep track of that). -- Simon Riggs www.2ndQuadrant.com PostgreSQL Training, Services and Support -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Need more reviewers!
Josh Berkus wrote: Hackers, We currently have 38 patches pending, and only nine people reviewing them. At this rate, the September commitfest will take three months. If you are a postgresql hacker at all, or even want to be one, we need your help reviewing patches! There are several easy patches in the list, so I can assign them to beginners. Please volunteer now! Please assign me one; any of the easy ones. -- Ibrar Ahmed EnterpriseDB http://www.enterprisedb.com -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Need more reviewers!
That way, instead of just an appeal to the masses to volunteer for $NEBULOUS_TASK, we can say something like Please volunteer to review patches. Doing an initial patch review is easy, please see our guide link to learn more. +1. I'll review a patch if you like, but the patch I have in this 'fest is only one of two I've ever submitted, and my understanding of PG guts is very weak, so I confidently predict that me looking at it has maybe 5% of the value of a committer looking at it, so I'm not sure that really gets us anywhere. Even if I think the patch is great, my opinion doesn't and shouldn't carry much weight compared to someone like Simon or Zdenek who are probably perceived by the committers as much more likely to have a clue, so the committer is just going to end up reviewing it all over again anyway. ...Robert -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Need more reviewers!
On Fri, 2008-09-05 at 12:18 +0100, Simon Riggs wrote: It would be a reasonable rule that all patch submitters also have to do patch reviews. This is almost the only way to be accepted as a contributor to Fedora -- and I like it. -- Devrim GÜNDÜZ, RHCE devrim~gunduz.org, devrim~PostgreSQL.org, devrim.gunduz~linux.org.tr http://www.gunduz.org signature.asc Description: This is a digitally signed message part
Re: [HACKERS] Need more reviewers!
Josh Berkus wrote: Hackers, We currently have 38 patches pending, and only nine people reviewing them. At this rate, the September commitfest will take three months. If you are a postgresql hacker at all, or even want to be one, we need your help reviewing patches! There are several easy patches in the list, so I can assign them to beginners. Please volunteer now! Please assign me one; any of the easy ones. -- Ibrar Ahmed EnterpriseDB http://www.enterprisedb.com -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Need more reviewers!
Simon Riggs wrote: On Thu, 2008-09-04 at 10:45 -0700, Josh Berkus wrote: If you are a postgresql hacker at all, or even want to be one, we need your help reviewing patches! There are several easy patches in the list, so I can assign them to beginners. It would be a reasonable rule that all patch submitters also have to do patch reviews. If we made it a strict rule, then sponsoring companies would know that they *must* provide money/time for that aspect also. Otherwise it is almost impossible to get formal approval to do that. All this would do is to deter people from submitting patches. Hard rules like this don't work in FOSS communities. I know it's like herding cats, but persuasion is really our only tool. cheers andrew -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Need more reviewers!
-BEGIN PGP SIGNED MESSAGE- Hash: RIPEMD160 I don't *want* the rule, I just think we *need* the rule because otherwise sponsors/managers/etc make business decisions to exclude that aspect of the software dev process. How exactly would you even begin to enforce such a rule? Retroactively pull otherwise vali patches from the queue? Ban people from sending email to the -patches list? Otherwise we have a patch-and-dump culture that is unsustainable because a few people's benevolence as reviewers turns everything into a bottleneck. It doesn't need to mean loss of control for core and committers. That problem needs a solution, but not the one you proposed. - -- Greg Sabino Mullane [EMAIL PROTECTED] PGP Key: 0x14964AC8 200809050953 http://biglumber.com/x/web?pk=2529DF6AB8F79407E94445B4BC9B906714964AC8 -BEGIN PGP SIGNATURE- iEYEAREDAAYFAkjBOdIACgkQvJuQZxSWSsiFoACgoqOgumuuZq6z2HBPSAPZUWHd kS0An2TgFmOLTgdFWuLkpazFbECY4nnz =ZrYl -END PGP SIGNATURE- -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Need more reviewers!
Hi, Simon Riggs wrote: On Fri, 2008-09-05 at 09:19 -0400, Andrew Dunstan wrote: All this would do is to deter people from submitting patches. Hard rules like this don't work in FOSS communities. I know it's like herding cats, but persuasion is really our only tool. +1 I don't *want* the rule, I just think we *need* the rule because otherwise sponsors/managers/etc make business decisions to exclude that aspect of the software dev process. I agree that making sponsors/managers/etc aware of that aspect of the dev process is necessary and worthwhile. However, I don't think a rule for *patch submitters* helps with that. There must be other ways to convince managers to encourage reviewers. Regards Markus Wanner -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Need more reviewers!
On Fri, 2008-09-05 at 16:03 +0200, Markus Wanner wrote: I don't *want* the rule, I just think we *need* the rule because otherwise sponsors/managers/etc make business decisions to exclude that aspect of the software dev process. I agree that making sponsors/managers/etc aware of that aspect of the dev process is necessary and worthwhile. However, I don't think a rule for *patch submitters* helps with that. There must be other ways to convince managers to encourage reviewers. Such as? You might think those arguments exist and work, but I would say they manifestly do not. Almost all people doing reviews are people that have considerable control over their own time, or are directed by people that understand the Postgres review process and wish to contribute to it for commercial reasons. -- Simon Riggs www.2ndQuadrant.com PostgreSQL Training, Services and Support -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Need more reviewers!
On 9/4/08, Simon Riggs [EMAIL PROTECTED] wrote: On Thu, 2008-09-04 at 10:45 -0700, Josh Berkus wrote: We currently have 38 patches pending, and only nine people reviewing them. At this rate, the September commitfest will take three months. If you are a postgresql hacker at all, or even want to be one, we need your help reviewing patches! There are several easy patches in the list, so I can assign them to beginners. Please volunteer now! Everybody is stuck in I'm not good enough to do a full review. They're right (myself included), so that just means we're organising it wrongly. We can't expect to grow more supermen, but we probably can do more teamwork and delegation. I think this should be organised with different kinds of reviewer: The list is correct but too verbose. And it does not attack the core of the problem. I think the problem is not: What can/should I do? but instead: Can I take the responsibility? Lets say reviewer would like look on coding style or performance. ATM it seems to him he well be now fully responsible for that aspect. I think we have better results and more relaxed atmospere if we use following task description for reviewers: The committer will do in-depth review. You task as a reviewer is to take off load from committers by catching simple problems. Your task is done if you think the patch is ready for in-depth review from committer. Note1 - Yes, the trick is to emphasize that all responsibility lies on committer. Note2 - detailed lists of areas to look at and reviewer types are not useful as each patch is different and each revier is different. Long lists just confuse people. The simpler the better. The main thing is to make easy for reviewer to take the first look. -- marko -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Need more reviewers!
Hi, Simon Riggs wrote: Such as? Dunno. Rules for sponsors? It would probably make sense to not only pay a single developer to create and submit a patch, but instead plan for paying others to review the code as well. You might think those arguments exist and work, but I would say they manifestly do not. Most managers - especially within software companies I'd say - are pretty much aware of how costly quality assurance (or the lack thereof) can be, no? What do you respond to potential sponsors who request that a new feature must be accepted into Postgres itself? Let's tell *them* that review is costly. Encourage them to pay others to review your work, for example. Let's coopete ;-) (or whatever the verb for coopetition is) Maybe we can do more WRT organizing this reviewing process, including payment. Some sort of bounty system or something. Dunno, this is just some brainstorming. Almost all people doing reviews are people that have considerable control over their own time, or are directed by people that understand the Postgres review process and wish to contribute to it for commercial reasons. Sure. I don't quite get where you are going with this argument, sorry. Regards Markus Wanner -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Need more reviewers!
On 9/5/08, Simon Riggs [EMAIL PROTECTED] wrote: On Fri, 2008-09-05 at 16:03 +0200, Markus Wanner wrote: I don't *want* the rule, I just think we *need* the rule because otherwise sponsors/managers/etc make business decisions to exclude that aspect of the software dev process. I agree that making sponsors/managers/etc aware of that aspect of the dev process is necessary and worthwhile. However, I don't think a rule for *patch submitters* helps with that. There must be other ways to convince managers to encourage reviewers. Such as? You might think those arguments exist and work, but I would say they manifestly do not. Almost all people doing reviews are people that have considerable control over their own time, or are directed by people that understand the Postgres review process and wish to contribute to it for commercial reasons. Well, the number of companies who are *interested* their patches getting in is rather small... I think it's more common for companies to think they are already donating to Postgres by encouraging their staff to write patches and publish them. So such applying such strict policy for everyone seems bad idea. Although I quite agree on strongly encouraging patch submitters to review. And those 3-4 companies who have direct commercial interests in Postgres development should probably internally rethink their time allocation. Note also we are only on our 2nd commitfest so its quite normal that people are not used to the process . We need to work few political aspects: * Making reviewers to more at ease. * Encouraging patch submitters to review. And technical aspects: * The (hopefully short and relaxed) rules for reviewers should be more visible. Best would be on (every) Commitfest page. * Wiki editing rules should be visible. Well, and then: * Although the wiki looks nice it's pain to operate. -- marko -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Need more reviewers!
On Fri, 2008-09-05 at 17:19 +0300, Marko Kreen wrote: I think this should be organised with different kinds of reviewer: The list is correct but too verbose. And it does not attack the core of the problem. I think the problem is not: What can/should I do? but instead: Can I take the responsibility? Completely agree. The list was really an example of the different styles of review that are possible, not a rigid categorisation that must be followed. -- Simon Riggs www.2ndQuadrant.com PostgreSQL Training, Services and Support -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Need more reviewers!
On 9/5/08, Marko Kreen [EMAIL PROTECTED] wrote: The list is correct but too verbose. And it does not attack the core of the problem. I think the problem is not: What can/should I do? but instead: Can I take the responsibility? To clarify it - that was the problem I faced last commitfest. Basically, I'm not a newbie, but I'm not deeply familiar with most of the components in Postgres. I'm not afraid to patch any part of code, because I know somebody who *is* familiar with the part will later review it. But if I'm supposed to answer Is this patch commitable? then this is too much for me. But when I convinced myself that my only task is to decrease the amount problems a patch has, so that committers job will be easier, then I felt at ease to take stab on several of them. So I suppose this works for other too and maybe this is worth accepting as official policy - the reviewers job is not to guarantee some level of quality, but just to report any problems they are able to find, so that committer's job will be easier and he can concentrate on the in-depth review. All this assumes you want relatively nobodies doing the reviews. If you want guaranteed quality, then this will scare away lightweights or make them look only one aspect of the patch. This leaves the heavyweights, but as you know, they are busy.. Lets say reviewer would like look on coding style or performance. ATM it seems to him he well be now fully responsible for that aspect. I think we have better results and more relaxed atmospere if we use following task description for reviewers: The committer will do in-depth review. You task as a reviewer is to take off load from committers by catching simple problems. Your task is done if you think the patch is ready for in-depth review from committer. Note1 - Yes, the trick is to emphasize that all responsibility lies on committer. Note2 - detailed lists of areas to look at and reviewer types are not useful as each patch is different and each revier is different. Long lists just confuse people. The simpler the better. The main thing is to make easy for reviewer to take the first look. -- marko -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Need more reviewers!
On Fri, 2008-09-05 at 09:19 -0400, Andrew Dunstan wrote: Simon Riggs wrote: On Thu, 2008-09-04 at 10:45 -0700, Josh Berkus wrote: If you are a postgresql hacker at all, or even want to be one, we need your help reviewing patches! There are several easy patches in the list, so I can assign them to beginners. It would be a reasonable rule that all patch submitters also have to do patch reviews. If we made it a strict rule, then sponsoring companies would know that they *must* provide money/time for that aspect also. Otherwise it is almost impossible to get formal approval to do that. All this would do is to deter people from submitting patches. Hard rules like this don't work in FOSS communities. I know it's like herding cats, but persuasion is really our only tool. I don't *want* the rule, I just think we *need* the rule because otherwise sponsors/managers/etc make business decisions to exclude that aspect of the software dev process. Otherwise we have a patch-and-dump culture that is unsustainable because a few people's benevolence as reviewers turns everything into a bottleneck. It doesn't need to mean loss of control for core and committers. -- Simon Riggs www.2ndQuadrant.com PostgreSQL Training, Services and Support -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Need more reviewers!
On Thu, Sep 04, 2008 at 09:54:02PM +0100, Simon Riggs wrote: * coding review - does it follow standard code guidelines? Are there portability issues? Will it work on Windows/BSD etc? Are there sufficient comments? * code review - does it do what it says, correctly? Just one thing though, I picked a random patch and started reading. However, the commitfest page doesn't link to anywhere that actually describes *what* the patch is trying to do. Many patches do have the design and the patch in one page, but some don't. I suppose what happens is the original patch comes with design and later a newer version is posted with just changes. The commitfest page points to the latter, losing former in the archive somewhere. Have a nice day, -- Martijn van Oosterhout [EMAIL PROTECTED] http://svana.org/kleptog/ Please line up in a tree and maintain the heap invariant while boarding. Thank you for flying nlogn airlines. signature.asc Description: Digital signature
Re: [HACKERS] Need more reviewers!
Martijn van Oosterhout wrote: Just one thing though, I picked a random patch and started reading. However, the commitfest page doesn't link to anywhere that actually describes *what* the patch is trying to do. Many patches do have the design and the patch in one page, but some don't. I suppose what happens is the original patch comes with design and later a newer version is posted with just changes. The commitfest page points to the latter, losing former in the archive somewhere. Hmm, IMO this is a flaw in the commitfest entry for that patch -- it should point to both. -- Alvaro Herrerahttp://www.CommandPrompt.com/ The PostgreSQL Company - Command Prompt, Inc. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Need more reviewers!
Alvaro Herrera [EMAIL PROTECTED] writes: Martijn van Oosterhout wrote: I suppose what happens is the original patch comes with design and later a newer version is posted with just changes. The commitfest page points to the latter, losing former in the archive somewhere. Hmm, IMO this is a flaw in the commitfest entry for that patch -- it should point to both. Yeah. What I think we should do here is that the main entry for a patch should point at the primary reference (first submission, or wherever it's best discussed), and then any later updates of the patch should be added as comments, instead of replacing the primary reference. It's been done this way for quite a few patches, but evidently not every one. Also, readers should remember to look through the whole thread in the archives, not just the single article linked to. (Dunno if that would have helped Martijn in this case, but there's often good material in the rest of the thread.) regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Need more reviewers!
On Thu, Sep 4, 2008 at 1:45 PM, Josh Berkus [EMAIL PROTECTED] wrote: We currently have 38 patches pending, and only nine people reviewing them. At this rate, the September commitfest will take three months. I'll push forward on reviewing and testing Xiao's hash index improvements for inclusion into core. Though, someone will still need to review my stuff. -- Jonah H. Harris, Senior DBA myYearbook.com -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Need more reviewers!
Jonah H. Harris [EMAIL PROTECTED] writes: I'll push forward on reviewing and testing Xiao's hash index improvements for inclusion into core. Though, someone will still need to review my stuff. I think what the hash index patch really needs is some performance testing. I'm willing to take responsibility for the code being okay or not, but I haven't got any production-grade hardware to do realistic performance tests on. I'd like to see a few more scenarios tested than the one provided so far: in particular, wide vs narrow index keys and good vs bad key distributions. It strikes me that there's an intermediate posture that the patch doesn't provide: namely, store *both* hash code and original index key in each index entry. This would make the index larger rather than smaller, but you could still do the intra-page sort by hash code, which I think is likely a big win. In exchange for the bigger index you'd avoid fruitless trips to the heap for chance hashcode matches. So it seems far from clear to me that the hash-code-only solution is necessarily the best. If anyone is willing to do comparative performance testing, I'll volunteer to make up two variant patches that do it both ways and are otherwise equivalent. (I wouldn't be too surprised if testing shows that each way could be a win depending on the situation. In that case we could think about how to provide a switch to let users pick the method. But for the moment let's just test two separate patches.) regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Need more reviewers!
On Thu, Sep 04, 2008 at 02:01:18PM -0400, Tom Lane wrote: Jonah H. Harris [EMAIL PROTECTED] writes: I'll push forward on reviewing and testing Xiao's hash index improvements for inclusion into core. Though, someone will still need to review my stuff. I think what the hash index patch really needs is some performance testing. I'm willing to take responsibility for the code being okay or not, but I haven't got any production-grade hardware to do realistic performance tests on. I'd like to see a few more scenarios tested than the one provided so far: in particular, wide vs narrow index keys and good vs bad key distributions. Tom, Do you mean good vs. bad key distributions with respect to hash value collisions? It strikes me that there's an intermediate posture that the patch doesn't provide: namely, store *both* hash code and original index key in each index entry. This would make the index larger rather than smaller, but you could still do the intra-page sort by hash code, which I think is likely a big win. In exchange for the bigger index you'd avoid fruitless trips to the heap for chance hashcode matches. So it seems far from clear to me that the hash-code-only solution is necessarily the best. Currently, since a trip to the heap is required to validate any tuple even if the exact value is contained in the index, it does not seem like it would be a win to store the value in both places. One of the big improvements is the space efficiency of the index obtained by just storing the hash value. I think that increasing the number of hash bits stored would provide more bang-for-the-buck than storing the exact value. One easy way to provide this functionality without increasing the storage requirements is to take advantage of the knowledge of the bucket number to increase the number of bit, i.e. at 256 buckets, remove the first 8-bits of the hash and add 8-bits to provide a 40-bit hash in the same storage. At 64k buckets, you can now store a 48-bit hash in the same space and at 16m buckets, you can store a 56-bit hash in the original 32-bit space. So as the number of buckets increases, the number of hash bits increases as well as the specificity of the resulting hash thereby reducing the need to visit the heap tuple store. Another idea, takes advantage of smaller bucket size (I think of them as sub-buckets) to add an additional 8-bits of hash value at the 32m or 64m buckets by looking up the hash in one of 2^n sub-buckets where n = 64 or 128 or even 256. This has the advantage of eliminating the binary lookup and insertion within the bucket page. Again, this will need to be tested. If anyone is willing to do comparative performance testing, I'll volunteer to make up two variant patches that do it both ways and are otherwise equivalent. I am in the boat with you, in that I do not have database systems with enough hardware to perform realistic testing. (I wouldn't be too surprised if testing shows that each way could be a win depending on the situation. In that case we could think about how to provide a switch to let users pick the method. But for the moment let's just test two separate patches.) I think, as of yet un-tested, that where storing both would be of value when we do not have to visit the heap for visibility information or when using the space to store more hash bits would be more effective. Cheers, Ken regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Need more reviewers!
Kenneth Marshall [EMAIL PROTECTED] writes: On Thu, Sep 04, 2008 at 02:01:18PM -0400, Tom Lane wrote: I think what the hash index patch really needs is some performance testing. I'm willing to take responsibility for the code being okay or not, but I haven't got any production-grade hardware to do realistic performance tests on. I'd like to see a few more scenarios tested than the one provided so far: in particular, wide vs narrow index keys and good vs bad key distributions. Do you mean good vs. bad key distributions with respect to hash value collisions? Right, I'm just concerned about how badly it degrades with hash value collisions. Those will result in wasted trips to the heap if we omit the original key from the index. AFAICS that's the only downside of doing so; but we should have an idea of how bad it could get before committing to doing this. Currently, since a trip to the heap is required to validate any tuple even if the exact value is contained in the index, it does not seem like it would be a win to store the value in both places. The point is that currently you *don't* need a trip to the heap if the key doesn't match, even if it has the same hash code. I think that increasing the number of hash bits stored would provide more bang-for-the-buck than storing the exact value. Maybe, but that would require an extremely invasive patch that breaks existing user-defined datatypes. You can't just magically get more hash bits from someplace, you need datatype-specific hash functions that will provide more than 32 bits. There's going to have to be a LOT of evidence in support of the value of doing that before I'll buy in. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Need more reviewers!
On Thu, 2008-09-04 at 14:01 -0400, Tom Lane wrote: If anyone is willing to do comparative performance testing, I'll volunteer to make up two variant patches that do it both ways and are otherwise equivalent. Why not do both, set via a reloption? We can then set the default to whichever wins in general testing. There will always be cases where one or the other is a winner, so having both will be useful. -- Simon Riggs www.2ndQuadrant.com PostgreSQL Training, Services and Support -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Need more reviewers!
On Thu, 2008-09-04 at 10:45 -0700, Josh Berkus wrote: We currently have 38 patches pending, and only nine people reviewing them. At this rate, the September commitfest will take three months. If you are a postgresql hacker at all, or even want to be one, we need your help reviewing patches! There are several easy patches in the list, so I can assign them to beginners. Please volunteer now! Everybody is stuck in I'm not good enough to do a full review. They're right (myself included), so that just means we're organising it wrongly. We can't expect to grow more supermen, but we probably can do more teamwork and delegation. I think this should be organised with different kinds of reviewer: * submission review - is patch in standard form, does it apply, does it have reasonable tests, docs etc.. Little or no knowledge of patch required to do that. We have at least 50 people can do that. * usability review - read what patch does. Do we want that? Do we already have it? Does it follow SQL spec? Are there dangers? Have all the bases been covered? We have 100s of people who can do that. * feature test - does feature work as advertised? * performance review - does the patch slow down simple tests? Does it speed things up like it says? Does it slow down other things? We have at least 100 people who can do that. * coding review - does it follow standard code guidelines? Are there portability issues? Will it work on Windows/BSD etc? Are there sufficient comments? * code review - does it do what it says, correctly? * architecture review - is everything done in a way that fits together coherently with other features/modules? are there interdependencies than can cause problems? * review review - did the reviewer cover all the things that kind of reviewer is supposed to do? If we split up things like this, we'll get a better response. Why not go to -perform for performance testers, general for usability and feature testers? If we encourage different types of review, more people will be willing to step up to doing a small amount for the project. Lots of eyeballs, not one good pair of eyes. Also, why has the CommitFest page been hidden? Whats the point of that? We probably need to offer some training and/or orientation for prospective reviewers, so people understand what is expected of them, how to do it and who to tell when they've done it. -- Simon Riggs www.2ndQuadrant.com PostgreSQL Training, Services and Support -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Need more reviewers!
Simon Riggs [EMAIL PROTECTED] writes: On Thu, 2008-09-04 at 14:01 -0400, Tom Lane wrote: If anyone is willing to do comparative performance testing, I'll volunteer to make up two variant patches that do it both ways and are otherwise equivalent. Why not do both, set via a reloption? That is exactly the code I'm unwilling to write until we find out if it's needed. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Need more reviewers!
On Thu, Sep 4, 2008 at 12:01 PM, Tom Lane [EMAIL PROTECTED] wrote: I think what the hash index patch really needs is some performance testing. I'm willing to take responsibility for the code being okay or not, but I haven't got any production-grade hardware to do realistic performance tests on. I'd like to see a few more scenarios tested than the one provided so far: in particular, wide vs narrow index keys and good vs bad key distributions. If anyone is willing to do comparative performance testing, I'll volunteer to make up two variant patches that do it both ways and are otherwise equivalent. I can happily through some hardware at this. Although production-grade is in the eye of the beholder... regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Need more reviewers!
Alex Hunsaker [EMAIL PROTECTED] writes: I can happily through some hardware at this. Although production-grade is in the eye of the beholder... I just posted a revised patch in the pgsql-patches thread. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Need more reviewers!
On Fri, Sep 5, 2008 at 6:54 AM, Simon Riggs [EMAIL PROTECTED] wrote: On Thu, 2008-09-04 at 10:45 -0700, Josh Berkus wrote: Please volunteer now! Everybody is stuck in I'm not good enough to do a full review. They're right (myself included), so that just means we're organising it wrongly. We can't expect to grow more supermen, but we probably can do more teamwork and delegation. As a first-time reviewer, I agree with Simon's comments, and I'd like to make the point that there's currently no written policy for how to review a patch. I let Josh know that I was interesting in joining this commitfest as a round robin reviewer, and he's assigned me a patch. Okay. What am I supposed to do now? I can certainly download the patch, test it, review the code, and write my thoughts to the list. I can then add a review link to the wiki page. Assuming I think the patch is acceptable, what then? Do I hand it off to somebody else for a full review/commit? How do I do that? etc. At the moment, for the review virgin, please volunteer now translates roughly as please elect to join an opaque and undocumented process which has until now been handled entirely by committers. That might have something to do with the low turnout. We have a (really useful) wiki page called Submitting a Patch. I think we need one called Reviewing a Patch. That way, instead of just an appeal to the masses to volunteer for $NEBULOUS_TASK, we can say something like Please volunteer to review patches. Doing an initial patch review is easy, please see our guide link to learn more. Cheers, BJ -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
