Thread: Patch queue concern
Right now, all the patches I think are ready for review are in the patch queue: http://momjian.postgresql.org/cgi-bin/pgpatches However, with feature freeze coming on Sunday, I am worried because there are a significant number of patches that have are not ready for review because they have not been completed by their authors. -- Bruce Momjian <bruce@momjian.us> http://momjian.us EnterpriseDB http://www.enterprisedb.com + If your life is a hard drive, Christ can be your backup. +
Bruce, > However, with feature freeze coming on Sunday, I am worried because > there are a significant number of patches that have are not ready for > review because they have not been completed by their authors. Can you flag those somehow? -- Josh Berkus PostgreSQL @ Sun San Francisco
Hello, I found in queue patch simply "custom variables protection, Pavel Stehule" which you removed and didn't find my patch for scrollable cursors in plpgsql. Regards Pavel Stehule _________________________________________________________________ Emotikony a pozadi programu MSN Messenger ozivi vasi konverzaci. http://messenger.msn.cz/
Josh Berkus wrote: > Bruce, > > > However, with feature freeze coming on Sunday, I am worried because > > there are a significant number of patches that have are not ready for > > review because they have not been completed by their authors. > > Can you flag those somehow? I have sent out email on every one in the past few days, with the lists cc'ed. -- Bruce Momjian <bruce@momjian.us> http://momjian.us EnterpriseDB http://www.enterprisedb.com + If your life is a hard drive, Christ can be your backup. +
"Bruce Momjian" <bruce@momjian.us> writes: > Right now, all the patches I think are ready for review are in the patch > queue: > > http://momjian.postgresql.org/cgi-bin/pgpatches > > However, with feature freeze coming on Sunday, I am worried because > there are a significant number of patches that have are not ready for > review because they have not been completed by their authors. That seems like a bit of a whacky criterion to use before reviewing a patch. It favours people who are short-sighted and don't see what possible improvements their code has. No code in an ongoing project like this is ever "completed" anyways. It's also an artifact of the working model we have where patches are sent in big chunks and reviewed much later during a feature freeze. If we were committing directly into a CVS repository we would have wanted to commit these changes as soon as they were ready for committing, not wait until they're "completed". Then continue working and commit further changes. It's only because there's a two step process and the reviews are mainly happening during the feature freeze that there's any sense that some of them are "completed". In fact they're not of course, there will be further changes in the same area once the freeze is lifted. I think you should be asking people whether they think the code is in a state where it can be committed, not whether they've finished working on it. Just because they see further work that can be done is no reason not to commit useful patches that are functional as they are. In fact Postgres historically has had an even looser standard. If the code is ready to be committed modulo bugs then it's been included in the feature freeze in the past. -- Gregory Stark EnterpriseDB http://www.enterprisedb.com
On Tue, 2007-03-27 at 21:15 -0400, Bruce Momjian wrote: > Right now, all the patches I think are ready for review are in the patch > queue: > > http://momjian.postgresql.org/cgi-bin/pgpatches > > However, with feature freeze coming on Sunday, I am worried because > there are a significant number of patches that have are not ready for > review because they have not been completed by their authors. It's probably a good idea to have a queue of those too, to allow others to finish them if the original author hasn't/can't/won't. I'm not sure which ones you mean. I have at least 2 patches that depend upon other patches in the queue. I'm not sure how to go about completing them, so any advice or guidance would be welcome: - Scan_recycle_buffers depends upon synchronised scans because we agreed we would use the same parameter (if any exists) to govern the behaviour. Should I write a patch-on-patch? What happens if the patch changes after review? ISTM I should just wait until the first one is applied and then I can make the necessary changes in about an hour. The patch's main functionality is complete. - Fast cluster conflicts with Heikki's cluster patch, so one of them will need fixing depending which is applied first. I don't mind if its me going second. I also have proposed an additional mode on VACUUM FULL that builds upon Heikki's patch - should I submit that also, even though it cannot be applied? -- Simon Riggs EnterpriseDB http://www.enterprisedb.com
Gregory Stark wrote: > "Bruce Momjian" <bruce@momjian.us> writes: > > > Right now, all the patches I think are ready for review are in the patch > > queue: > > > > http://momjian.postgresql.org/cgi-bin/pgpatches > > > > However, with feature freeze coming on Sunday, I am worried because > > there are a significant number of patches that have are not ready for > > review because they have not been completed by their authors. > > That seems like a bit of a whacky criterion to use before reviewing a patch. "wacky"? > It favours people who are short-sighted and don't see what possible > improvements their code has. No code in an ongoing project like this is ever > "completed" anyways. It favors those who do not wait until the last minute, but complete them well before the freeze date. > It's also an artifact of the working model we have where patches are sent in > big chunks and reviewed much later during a feature freeze. If we were > committing directly into a CVS repository we would have wanted to commit these > changes as soon as they were ready for committing, not wait until they're > "completed". Then continue working and commit further changes. It's only This would have CVS containing uncomplete features --- and before beta, we would either have to beg the authors to complete them, or rip them out, neither of which we want to do. > because there's a two step process and the reviews are mainly happening during > the feature freeze that there's any sense that some of them are "completed". > In fact they're not of course, there will be further changes in the same area > once the freeze is lifted. > > I think you should be asking people whether they think the code is in a state > where it can be committed, not whether they've finished working on it. Just > because they see further work that can be done is no reason not to commit > useful patches that are functional as they are. OK, but we don't want something that is ready to be committed, we need it complete. > In fact Postgres historically has had an even looser standard. If the code is > ready to be committed modulo bugs then it's been included in the feature > freeze in the past. Well, if we know something has bugs, we fix them. Things are committed with bugs only because we don't know it has bugs when it was committed. -- Bruce Momjian <bruce@momjian.us> http://momjian.us EnterpriseDB http://www.enterprisedb.com + If your life is a hard drive, Christ can be your backup. +
Simon Riggs wrote: > On Tue, 2007-03-27 at 21:15 -0400, Bruce Momjian wrote: > > Right now, all the patches I think are ready for review are in the patch > > queue: > > > > http://momjian.postgresql.org/cgi-bin/pgpatches > > > > However, with feature freeze coming on Sunday, I am worried because > > there are a significant number of patches that have are not ready for > > review because they have not been completed by their authors. > > It's probably a good idea to have a queue of those too, to allow others > to finish them if the original author hasn't/can't/won't. I'm not sure > which ones you mean. At this point, with four days left before feature freeze, if the authors don't finish them, I doubt someone else is going to be able to do it. > I have at least 2 patches that depend upon other patches in the queue. > I'm not sure how to go about completing them, so any advice or guidance > would be welcome: > > - Scan_recycle_buffers depends upon synchronised scans because we agreed > we would use the same parameter (if any exists) to govern the behaviour. > Should I write a patch-on-patch? What happens if the patch changes after > review? ISTM I should just wait until the first one is applied and then > I can make the necessary changes in about an hour. The patch's main > functionality is complete. Yes, that is fine. I was unaware that is why the patch wasn't "done". Once synchronised scans is in, I will go back to you and ask for a new version against CVS. I will put your email in the patch queue as a reminder. > - Fast cluster conflicts with Heikki's cluster patch, so one of them > will need fixing depending which is applied first. I don't mind if its > me going second. I also have proposed an additional mode on VACUUM FULL > that builds upon Heikki's patch - should I submit that also, even though > it cannot be applied? OK, same rules. I am just glad that is all that was hold them up. I was worried. What about the delayed fsync patch? -- Bruce Momjian <bruce@momjian.us> http://momjian.us EnterpriseDB http://www.enterprisedb.com + If your life is a hard drive, Christ can be your backup. +
at seems like a bit of a whacky criterion to use before reviewing a patch.
>
> "wacky"?
>
>> It favours people who are short-sighted and don't see what possible
>> improvements their code has. No code in an ongoing project like this is ever
>> "completed" anyways.
>
> It favors those who do not wait until the last minute, but complete them
> well before the freeze date.
But wouldn't it hurt those that are continuously working the patch with
the community? Just asking.
>
>> It's also an artifact of the working model we have where patches are sent in
>> big chunks and reviewed much later during a feature freeze. If we were
>> committing directly into a CVS repository we would have wanted to commit these
>> changes as soon as they were ready for committing, not wait until they're
>> "completed". Then continue working and commit further changes. It's only
>
> This would have CVS containing uncomplete features --- and before beta,
> we would either have to beg the authors to complete them, or rip them
> out, neither of which we want to do.
I agree here.
>> I think you should be asking people whether they think the code is in a state
>> where it can be committed, not whether they've finished working on it. Just
>> because they see further work that can be done is no reason not to commit
>> useful patches that are functional as they are.
>
> OK, but we don't want something that is ready to be committed, we need
> it complete.
Right, feature complete does not mean bug free that is what the testing
period is for.
>
>> In fact Postgres historically has had an even looser standard. If the code is
>> ready to be committed modulo bugs then it's been included in the feature
>> freeze in the past.
>
> Well, if we know something has bugs, we fix them. Things are committed
> with bugs only because we don't know it has bugs when it was committed.
Yep :)
Joshua D. Drake
--
=== The PostgreSQL Company: Command Prompt, Inc. ===
Sales/Support: +1.503.667.4564 || 24x7/Emergency: +1.800.492.2240
Providing the most comprehensive PostgreSQL solutions since 1997 http://www.commandprompt.com/
Donate to the PostgreSQL Project: http://www.postgresql.org/about/donate
PostgreSQL Replication: http://www.commandprompt.com/products/
Joshua D. Drake wrote: > at seems like a bit of a whacky criterion to use before reviewing a patch. > > > > "wacky"? > > > >> It favours people who are short-sighted and don't see what possible > >> improvements their code has. No code in an ongoing project like this is ever > >> "completed" anyways. > > > > It favors those who do not wait until the last minute, but complete them > > well before the freeze date. > > But wouldn't it hurt those that are continuously working the patch with > the community? Just asking. Yea, it might, and it certainly hampers complex patches. I was caught up on the patch queue until the start of March, when I went on vacation, Tom started on cache invalidation, _and_ more complex patches started appearing. With those three, we had a perfect storm and the patch queue has gotten clogged, and I am afraid it isn't going to get unclogged until after feature freeze. I talked to Tom about this yesterday and he and I feel there isn't much we can do to change that, in the sense we are already doing the best we can, and clearing the remaining patches after feature freeze isn't that bad. One thing committers have to be willing to do is to give authors ample time after feature freeze to adjust patches after receiving feedback, because technically they should have received feedback _before_ feature freeze. Hopefully this will not significantly lengthen feature freeze. -- Bruce Momjian <bruce@momjian.us> http://momjian.us EnterpriseDB http://www.enterprisedb.com + If your life is a hard drive, Christ can be your backup. +
On Wed, 2007-03-28 at 15:48 -0400, Bruce Momjian wrote: > What about the delayed fsync patch? All complete bar two fiddly items, as of Mar 11, design-to-complete posted along with patch. Working on those now. -- Simon Riggs EnterpriseDB http://www.enterprisedb.com
"Bruce Momjian" <bruce@momjian.us> writes: > Simon Riggs wrote: > >> It's probably a good idea to have a queue of those too, to allow others >> to finish them if the original author hasn't/can't/won't. I'm not sure >> which ones you mean. > > At this point, with four days left before feature freeze, if the authors > don't finish them, I doubt someone else is going to be able to do it. This isn't the standard that we've used in the past. In the past patches that are mostly done and need some extra work done to polish them off are considered to have met the feature freeze. In any case I think Simon and you have fallen into the trap of thinking of development as a single-person project. Most developers here, especially first-time contributors, don't just work in the dark on their own and turn up with a finished patch. They have questions and need help in areas. If you insist on a "finished" patch before you even consider reviewing their work it's not going to work. -- Gregory Stark EnterpriseDB http://www.enterprisedb.com
Gregory Stark wrote: > > "Bruce Momjian" <bruce@momjian.us> writes: > > > Simon Riggs wrote: > > > >> It's probably a good idea to have a queue of those too, to allow others > >> to finish them if the original author hasn't/can't/won't. I'm not sure > >> which ones you mean. > > > > At this point, with four days left before feature freeze, if the authors > > don't finish them, I doubt someone else is going to be able to do it. > > This isn't the standard that we've used in the past. In the past patches that > are mostly done and need some extra work done to polish them off are > considered to have met the feature freeze. My assumption is if authors don't finish them in the next few days, they are unlikely to finish them during some grace period during feature freeze. And the extra time is usually allowed for changes requested by committers, while at this point the authors aren't done and haven't even gotten to committer review. > In any case I think Simon and you have fallen into the trap of thinking of > development as a single-person project. Most developers here, especially > first-time contributors, don't just work in the dark on their own and turn up > with a finished patch. They have questions and need help in areas. If you > insist on a "finished" patch before you even consider reviewing their work > it's not going to work. Fine, if they need help, let them ask, but many authors are not asking for help --- they are just not completing the patches. Or they are going to surprise us by completing them on March 31. -- Bruce Momjian <bruce@momjian.us> http://momjian.us EnterpriseDB http://www.enterprisedb.com + If your life is a hard drive, Christ can be your backup. +
On Wed, 2007-03-28 at 17:02 -0400, Bruce Momjian wrote: > they It would be good to know who/what you're talking about, specifically. Some patchers may think they have completed their work. Not a name-and-shame, just fair warning their work is considered incomplete and is about to be rejected as a result. -- Simon Riggs EnterpriseDB http://www.enterprisedb.com
Simon Riggs wrote: > On Wed, 2007-03-28 at 17:02 -0400, Bruce Momjian wrote: > > > they > > It would be good to know who/what you're talking about, specifically. > > Some patchers may think they have completed their work. > > Not a name-and-shame, just fair warning their work is considered > incomplete and is about to be rejected as a result. Not sure how to do this without name-and-shame. I sent out emails to the list asking where we were on various open patches. I can do it again tomorrow so there is some context in the requests. Would that help? -- Bruce Momjian <bruce@momjian.us> http://momjian.us EnterpriseDB http://www.enterprisedb.com + If your life is a hard drive, Christ can be your backup. +
Bruce Momjian wrote: > Gregory Stark wrote: > >> "Bruce Momjian" <bruce@momjian.us> writes: >> >> >>> Simon Riggs wrote: >>> >>> >>>> It's probably a good idea to have a queue of those too, to allow others >>>> to finish them if the original author hasn't/can't/won't. I'm not sure >>>> which ones you mean. >>>> >>> At this point, with four days left before feature freeze, if the authors >>> don't finish them, I doubt someone else is going to be able to do it. >>> >> This isn't the standard that we've used in the past. In the past patches that >> are mostly done and need some extra work done to polish them off are >> considered to have met the feature freeze. >> > > My assumption is if authors don't finish them in the next few days, they > are unlikely to finish them during some grace period during feature > freeze. And the extra time is usually allowed for changes requested by > committers, while at this point the authors aren't done and haven't even > gotten to committer review. > Well hold on Bruce, that isn't quite fair. I know there are patches in this cycle that have been waiting on reviewers/comitters not the other way around. Clustered indexes for example. I know that Gavin is "this close" to having vacuum finished for bitmap index on disk. Alvaro's vacuum patch isn't done either, although he has submitted WIP. Perhaps it makes sense to say: Feature Freeze: April 1st., no "new" patches accepted for 8.3 Patch Freeze April 15th., Authors have until the 15th to address any committer concerns ? Sincerely, Joshua D. Drake >
Joshua D. Drake wrote: > > My assumption is if authors don't finish them in the next few days, they > > are unlikely to finish them during some grace period during feature > > freeze. And the extra time is usually allowed for changes requested by > > committers, while at this point the authors aren't done and haven't even > > gotten to committer review. > > > Well hold on Bruce, that isn't quite fair. I know there are patches in > this cycle that have been waiting on reviewers/comitters not the other > way around. > Clustered indexes for example. I know that Gavin is "this close" to > having vacuum finished for bitmap index on disk. Alvaro's vacuum patch > isn't done > either, although he has submitted WIP. Yes, for one, I am worried about bitmap indexes, and the performance testing time we are going to need to decide if we want it. In general, I am more concerned about patches where I don't see public patches/commit, like bitmap indexes, rather than patches like HOT that are being publicly advanced. All the patches might be advancing, but of course, I only see the public ones, and those are the only ones I can guess are near completion. I am speaking of my concerns now, rather than after feature freeze, because author options are more limited after feature freeze. > Perhaps it makes sense to say: > > Feature Freeze: April 1st., no "new" patches accepted for 8.3 > Patch Freeze April 15th., Authors have until the 15th to address any > committer concerns Well, I am OK with that, but we need _community_ agreement on that. I realize it isn't fair that committers are behind on patches, while we are expecting submitters to make the deadline, but there are far fewer committers than submitters, and there was never a promise to commit everything before feature freeze. -- Bruce Momjian <bruce@momjian.us> http://momjian.us EnterpriseDB http://www.enterprisedb.com + If your life is a hard drive, Christ can be your backup. +
>
>> Perhaps it makes sense to say:
>>
>> Feature Freeze: April 1st., no "new" patches accepted for 8.3
>> Patch Freeze April 15th., Authors have until the 15th to address any
>> committer concerns
>
> Well, I am OK with that, but we need _community_ agreement on that.
>
> I realize it isn't fair that committers are behind on patches, while we
> are expecting submitters to make the deadline, but there are far fewer
> committers than submitters, and there was never a promise to commit
> everything before feature freeze.
Yeah that was kind of my thinking is that everyone knows that the
committers are behind (and overworked). So if we have this two week
breather where it is all about patch review...
Joshua D. Drake
--
=== The PostgreSQL Company: Command Prompt, Inc. ===
Sales/Support: +1.503.667.4564 || 24x7/Emergency: +1.800.492.2240
Providing the most comprehensive PostgreSQL solutions since 1997 http://www.commandprompt.com/
Donate to the PostgreSQL Project: http://www.postgresql.org/about/donate
PostgreSQL Replication: http://www.commandprompt.com/products/
On Wed, 2007-03-28 at 17:12 -0400, Bruce Momjian wrote: > Simon Riggs wrote: > > On Wed, 2007-03-28 at 17:02 -0400, Bruce Momjian wrote: > > > > > they > > > > It would be good to know who/what you're talking about, specifically. > > > > Some patchers may think they have completed their work. > > > > Not a name-and-shame, just fair warning their work is considered > > incomplete and is about to be rejected as a result. > > Not sure how to do this without name-and-shame. I sent out emails to > the list asking where we were on various open patches. I can do it > again tomorrow so there is some context in the requests. Would that > help? Please publish the list. I'm sure it will raise eyebrows, but we can sort out any misunderstandings; there's no shame in attempting something and meeting a blockage - thats normal. If everybody knows where everybody stands then we'll all be better off. There may be other dependencies that need resolution, or last minute decisions required to allow authors to finish. Plus I want to check whether I'm on it, or not. -- Simon Riggs EnterpriseDB http://www.enterprisedb.com
On Wed, 28 Mar 2007, Simon Riggs wrote: > On Wed, 2007-03-28 at 17:12 -0400, Bruce Momjian wrote: > > If everybody knows where everybody stands then we'll all be better off. > There may be other dependencies that need resolution, or last minute > decisions required to allow authors to finish. Wasn't this the purpose of the wiki page that was set up? I notice it has not been updated in a while... http://developer.postgresql.org/index.php/Todo:WishlistFor83 -- If the aborigine drafted an IQ test, all of Western civilization would presumably flunk it. -- Stanley Garn
On Wed, 2007-03-28 at 17:37 -0400, Bruce Momjian wrote: > I realize it isn't fair that committers are behind on patches, while we > are expecting submitters to make the deadline, but there are far fewer > committers than submitters, and there was never a promise to commit > everything before feature freeze. I'm expecting to review patches after freeze and I'm much more free to do that now than I have been previously. It seems important we have a tiered review process so that some of the more obvious flaws can be driven out of patches as early as possible. If we can set expectations that every developer has to contribute review time, committer or not, then we'll all be better off. That need not take away authority from committers, nor give it to reviewers. Anybody and everybody is certainly welcome to comment on my own patches. My feeling is we should have more regular sync points where the patch queue is emptied and everything committed or rejected. That way rejection is less of a problem and we will all have more opportunity to build upon each others good work. -- Simon Riggs EnterpriseDB http://www.enterprisedb.com
"Bruce Momjian" <bruce@momjian.us> writes: >> It favours people who are short-sighted and don't see what possible >> improvements their code has. No code in an ongoing project like this is ever >> "completed" anyways. > > It favors those who do not wait until the last minute, but complete them > well before the freeze date. What is this "complete" you keep talking about? Should I stop working on the sort/limit patch even though Heikki pointed out a few things to clean up and the cost model isn't updated yet just so that you'll consider it "complete" and put it on the patch queue? If I don't stop working on it you think we should just ignore it even if it's in a usable state now? Even the cost model changes could be done pretty easily with some guidance from a review. >> It's also an artifact of the working model we have where patches are sent in >> big chunks and reviewed much later during a feature freeze. If we were >> committing directly into a CVS repository we would have wanted to commit these >> changes as soon as they were ready for committing, not wait until they're >> "completed". Then continue working and commit further changes. It's only > > This would have CVS containing uncomplete features --- and before beta, > we would either have to beg the authors to complete them, or rip them > out, neither of which we want to do. You don't want to commit something if it's in an unusable state and would have to be ripped out without more work. I said "as soon as they're ready for committing" as opposed to "completed". You're asking people if they've stopped working on patches and you're surprised to find that there are a lot of patches people are still working on. That's silly, of course people are still working on them, many of these tasks are open ended and can be improved as long as we have time. just because they're still working on them doesn't necessarily mean what they have so far isn't worth committing as is yet. > OK, but we don't want something that is ready to be committed, we need > it complete. So how many more releases before you think Postgres is "complete"? -- Gregory Stark EnterpriseDB http://www.enterprisedb.com
Gregory Stark wrote: > "Bruce Momjian" <bruce@momjian.us> writes: > That's silly, of course people are still working on them, many of these tasks > are open ended and can be improved as long as we have time. just because > they're still working on them doesn't necessarily mean what they have so far > isn't worth committing as is yet. > >> OK, but we don't want something that is ready to be committed, we need >> it complete. > > So how many more releases before you think Postgres is "complete"? You are using the word complete as in final and unalterable. That's not, it seems to me, what Bruce means. Bruce has a point, and a valid and sensible one at that. A patch that is ready to be committed does not mean it is usable. Just because you can commit a patch does not mean that the patch will be useful. Well, if a patch author has promised to supply a patch for the X function, and has not completed a stable and generally usable patch for X, then the patch is not worth committing. Thank you very much. Best Regards, Carlo -- Carlo Florendo Softare Engineer/Network Co-Administrator Astra Philippines Inc. UP-Ayala Technopark, Diliman 1101, Quezon City Philippines http://www.astra.ph -- The Astra Group of Companies 5-3-11 Sekido, Tama City Tokyo 206-0011, Japan http://www.astra.co.jp
Gregory Stark wrote: > In any case I think Simon and you have fallen into the trap of thinking of > development as a single-person project. Most developers here, especially > first-time contributors, don't just work in the dark on their own and turn up > with a finished patch. They have questions and need help in areas. If you > insist on a "finished" patch before you even consider reviewing their work > it's not going to work. This isn't about "finished" patches. It's about "commit-worthy" patches, and since the term is very subjective, there has to be some way for an arbiter to be able to say that such a patch is worth committing. And I think the arbiter should not come from any of the two opposing sides with diametrically opposed claims or opinions. Thank you very much. Best Regards, Carlo -- Carlo Florendo Softare Engineer/Network Co-Administrator Astra Philippines Inc. UP-Ayala Technopark, Diliman 1101, Quezon City Philippines http://www.astra.ph -- The Astra Group of Companies 5-3-11 Sekido, Tama City Tokyo 206-0011, Japan http://www.astra.co.jp
"Simon Riggs" <simon@2ndquadrant.com> writes:
> My feeling is we should have more regular sync points where the patch
> queue is emptied and everything committed or rejected.
No doubt, but the real problem here is that reviewing/committing other
people's patches is not fun, it's just work :-(. So it's no surprise
that it tends to get put off. Not sure what to do about that.
regards, tom lane
> > My feeling is we should have more regular sync points where the patch > > queue is emptied and everything committed or rejected. > > No doubt, but the real problem here is that > reviewing/committing other people's patches is not fun, it's > just work :-(. So it's no surprise that it tends to get put > off. Not sure what to do about that. In my experience it mostly pays to keep people directly responsible for their own work. Every intermediate tester/reviewer/coordinator tends to reduce the submitter's feeling for responsibility. So I could imagine a modus operandi where a submitter states: I feel confident that you can commit without review and will be availabe for fixes/additional work required. Maybe we have that in the form of committers that commit their own work already. But I do feel that some patches Bruce is talking about need aggreement and help, not only review. Andreas
On Thu, 2007-03-29 at 01:34 -0400, Tom Lane wrote: > "Simon Riggs" <simon@2ndquadrant.com> writes: > > My feeling is we should have more regular sync points where the patch > > queue is emptied and everything committed or rejected. > > No doubt, but the real problem here is that reviewing/committing other > people's patches is not fun, it's just work :-(. Gosh, you always seemed to enjoy my patches so much ;-) We can all see how hard you and Bruce work and its very much appreciated, even if we don't often say so. That's why everybody else works so hard too. Sometimes we only communicate the tensions caused by external expectations. > So it's no surprise > that it tends to get put off. Not sure what to do about that. Well, one thing I can do is say Thanks now and try to do that more regularly in the future. The enjoyment you and others take from working on PostgreSQL is infectious, so whatever else we do its gotta stay fun. -- Simon Riggs EnterpriseDB http://www.enterprisedb.com
"Tom Lane" <tgl@sss.pgh.pa.us> writes: > "Simon Riggs" <simon@2ndquadrant.com> writes: >> My feeling is we should have more regular sync points where the patch >> queue is emptied and everything committed or rejected. > > No doubt, but the real problem here is that reviewing/committing other > people's patches is not fun, it's just work :-(. So it's no surprise > that it tends to get put off. Not sure what to do about that. Obviously a big part of that is that we just don't have enough committers. I'm hopeful that in time that situation will improve but in the meantime we do have a problem and the burden falls unfairly on a few. Is there anything others can do to help? If non-committers like Simon or I reviewed patches would it be easier for you to give a quick agreement to the comments or "that's not an issue" comment? It seems like we do have a few committers who should be able to review code quality but are uncertain about making major design decisions. If, for example, Bruce or Jan reviewed patches more invasive than they usually do for code quality and checked with you on design questions would that be helpful? -- Gregory Stark EnterpriseDB http://www.enterprisedb.com
Gregory Stark wrote: > Obviously a big part of that is that we just don't have enough committers. I'm > hopeful that in time that situation will improve but in the meantime we do > have a problem and the burden falls unfairly on a few. > > Is there anything others can do to help? If non-committers like Simon or I > reviewed patches would it be easier for you to give a quick agreement to the > comments or "that's not an issue" comment? > > It seems like we do have a few committers who should be able to review code > quality but are uncertain about making major design decisions. If, for > example, Bruce or Jan reviewed patches more invasive than they usually do for > code quality and checked with you on design questions would that be helpful? > > I try to review things that I feel are well within my area of competence (e.g plperl, sql level commands) but I feel more hesitant about things very deep inside the backend - there's more danger I'll miss something subtle there. Outside events have conspired to make both reviewing and coding harder for me to get done this cycle. As for "major design decisions", these should not be in the hands of a reviewer anyway - they should be explicitly discussed on list. There is plenty of scope for people to review patches if they aren't committers. In fact, it is highly encouraged. Please review anything on the patch list you feel able to. cheers andrew
Andrew Dunstan <andrew@dunslane.net> writes:
> There is plenty of scope for people to review patches if they aren't
> committers. In fact, it is highly encouraged. Please review anything on
> the patch list you feel able to.
Sure. Even if you miss things, every problem you do spot is one less...
and there's no guarantee that the eventual committer would have seen it.
regards, tom lane
Gregory Stark wrote: > > "Bruce Momjian" <bruce@momjian.us> writes: > > >> It favours people who are short-sighted and don't see what possible > >> improvements their code has. No code in an ongoing project like this is ever > >> "completed" anyways. > > > > It favors those who do not wait until the last minute, but complete them > > well before the freeze date. > > What is this "complete" you keep talking about? Should I stop working on the > sort/limit patch even though Heikki pointed out a few things to clean up and > the cost model isn't updated yet just so that you'll consider it "complete" > and put it on the patch queue? If I don't stop working on it you think we > should just ignore it even if it's in a usable state now? Even the cost model > changes could be done pretty easily with some guidance from a review. Complete means the author _thinks_ he is done, and has responded to all community comments on the patch. > >> It's also an artifact of the working model we have where patches are sent in > >> big chunks and reviewed much later during a feature freeze. If we were > >> committing directly into a CVS repository we would have wanted to commit these > >> changes as soon as they were ready for committing, not wait until they're > >> "completed". Then continue working and commit further changes. It's only > > > > This would have CVS containing uncomplete features --- and before beta, > > we would either have to beg the authors to complete them, or rip them > > out, neither of which we want to do. > > You don't want to commit something if it's in an unusable state and would have > to be ripped out without more work. I said "as soon as they're ready for > committing" as opposed to "completed". > > You're asking people if they've stopped working on patches and you're > surprised to find that there are a lot of patches people are still working on. > > That's silly, of course people are still working on them, many of these tasks > are open ended and can be improved as long as we have time. just because > they're still working on them doesn't necessarily mean what they have so far > isn't worth committing as is yet. We don't want open-ended a few days before feature feeze. We want them to be as done, at some complete stopping point, and submitted. > > OK, but we don't want something that is ready to be committed, we need > > it complete. > > So how many more releases before you think Postgres is "complete"? I am getting tired of your semantic games, here, Greg. I have no idea what you are trying to accomplish, but I have better things to do. -- Bruce Momjian <bruce@momjian.us> http://momjian.us EnterpriseDB http://www.enterprisedb.com + If your life is a hard drive, Christ can be your backup. +
> We don't want open-ended a few days before feature feeze. We want them
> to be as done, at some complete stopping point, and submitted.
>
>>> OK, but we don't want something that is ready to be committed, we need
>>> it complete.
>> So how many more releases before you think Postgres is "complete"?
>
> I am getting tired of your semantic games, here, Greg. I have no idea
> what you are trying to accomplish, but I have better things to do.
I have to concur here. Everyone is doing the best that they can. Greg,
how about reviewing some patches?
Joshua D. Drake
--
=== The PostgreSQL Company: Command Prompt, Inc. ===
Sales/Support: +1.503.667.4564 || 24x7/Emergency: +1.800.492.2240
Providing the most comprehensive PostgreSQL solutions since 1997 http://www.commandprompt.com/
Donate to the PostgreSQL Project: http://www.postgresql.org/about/donate
PostgreSQL Replication: http://www.commandprompt.com/products/
Tom Lane wrote:
> "Simon Riggs" <simon@2ndquadrant.com> writes:
> > My feeling is we should have more regular sync points where the patch
> > queue is emptied and everything committed or rejected.
>
> No doubt, but the real problem here is that reviewing/committing other
> people's patches is not fun, it's just work :-(. So it's no surprise
> that it tends to get put off. Not sure what to do about that.
Of course, writing patches isn't totally _fun_ either.
The big problem is shown in this chart:
P a t c h C o m p l e x i t y Developer | Simple Complex
---------------------------------------------- Experienced | Easy Medium Novice
| Medium Hard
The basic problem is we have a lot of complex patches coming in, and
many from people who do not have years of experience with submitting
patches to PostgreSQL. A complex patch from a novice user takes a lot
of time to review, and frankly, we don't have enough experienced
developers doing such reviews. If the patch deals with an area of the
code where I am not experienced, often even I am incapable of reviewing
the patch.
The bottom line is that we are getting more novice developers faster
than we grow experienced developers. This is no big surprise, and I
don't see a simple solution. Odds are this is going to continue.
-- Bruce Momjian <bruce@momjian.us> http://momjian.us EnterpriseDB
http://www.enterprisedb.com
+ If your life is a hard drive, Christ can be your backup. +
Bruce Momjian wrote: >>> OK, but we don't want something that is ready to be committed, we need >>> it complete. >> So how many more releases before you think Postgres is "complete"? > > I am getting tired of your semantic games, here, Greg. I have no idea > what you are trying to accomplish, but I have better things to do. Why not just post a specific list of the patches you are thinking of? Is it the patch queue list in total? Did I miss it? Without specifics these things just spiral on forever, as all debates about code do when there is no code to actually look at. With specifics it is self documenting and definitional. You are thinking / concerned about x patches. Folks can look at how to move them forward, and it would probably help guide community attention.
Gregory Stark wrote: > > "Tom Lane" <tgl@sss.pgh.pa.us> writes: > > > "Simon Riggs" <simon@2ndquadrant.com> writes: > >> My feeling is we should have more regular sync points where the patch > >> queue is emptied and everything committed or rejected. > > > > No doubt, but the real problem here is that reviewing/committing other > > people's patches is not fun, it's just work :-(. So it's no surprise > > that it tends to get put off. Not sure what to do about that. > > Obviously a big part of that is that we just don't have enough committers. I'm > hopeful that in time that situation will improve but in the meantime we do > have a problem and the burden falls unfairly on a few. > > Is there anything others can do to help? If non-committers like Simon or I > reviewed patches would it be easier for you to give a quick agreement to the > comments or "that's not an issue" comment? Just to clarify, the committing is the easy part. I can do that all day and not break a sweat. It is making sure the patch is correct in all aspects --- functionality, clarity, modularity, reliability, design, etc. that takes lots of time, and really can be done by anyone in the community. We already have people commenting on other peoples patches, and new versions appearing, and every new version makes the final job of review/commit easier, because someone was going to have to make those changes before the patch was applied. > It seems like we do have a few committers who should be able to review code > quality but are uncertain about making major design decisions. If, for > example, Bruce or Jan reviewed patches more invasive than they usually do for > code quality and checked with you on design questions would that be helpful? I wish that would work, but the big trick is getting the entire problem into your head, matching user behavior with our existing code, and making those link up. There is really no _stage_ nature of final patch review. People can still comment on the patch, and improve it, but the final decision has to be a holistic one that makes sure the entire patch is in harmony. (I am sounding new-age here. :-) ) You might remember during I think 8.1 I started pushing patches because no one was objecting to the patches, and people complained because the patches we not complete. The patches had problems, but I was unable to fully understand some of the patches, and the patches had to be backed out. Since then, I haven't applied anything I didn't fully understand, so the patches not languish in the patch queue until an experienced PostgreSQL developer who does fully understand them can give me a green light on it. -- Bruce Momjian <bruce@momjian.us> http://momjian.us EnterpriseDB http://www.enterprisedb.com + If your life is a hard drive, Christ can be your backup. +
Bruce Momjian <bruce@momjian.us> writes:
> The basic problem is we have a lot of complex patches coming in, and
> many from people who do not have years of experience with submitting
> patches to PostgreSQL. A complex patch from a novice user takes a lot
> of time to review, and frankly, we don't have enough experienced
> developers doing such reviews.
Part of the issue is that we have a lot of new developers who are trying
to solve hard problems without having learned their way around the code
by fixing easy stuff. It was easier some years ago for people to learn
that way, because there was way more low-hanging fruit back then. But
there's still some out there. I have a distinct sense that we are
getting patches from people who are trying to run before they've learned
to walk.
regards, tom lane