Thread: Patch review

Patch review

From
Gregory Stark
Date:
so.... I went to look for the held patches queue to start reviewing patches.
There are over 2000 messages in the queue in 300 separate threads. At that
rate it would probably be just as easy to scan the patches and hackers mailing
list.

Is someone working on dumping the list into a table on the wiki? I could
download the mbox files from the web site and filter them into a table.

Some part of me thinks this data should be in a postgres database so I can do
SQL queries against it to find a good patch to review.

--  Gregory Stark EnterpriseDB          http://www.enterprisedb.com Ask me about EnterpriseDB's 24x7 Postgres support!


Re: Patch review

From
Alvaro Herrera
Date:
Gregory Stark wrote:

> so.... I went to look for the held patches queue to start reviewing patches.
> There are over 2000 messages in the queue in 300 separate threads. At that
> rate it would probably be just as easy to scan the patches and hackers mailing
> list.

Pretty unwieldy, yes.  I'm not sure -patchers of -hackers is really
"just as easy" though.

> Is someone working on dumping the list into a table on the wiki? I could
> download the mbox files from the web site and filter them into a table.
> 
> Some part of me thinks this data should be in a postgres database so I can do
> SQL queries against it to find a good patch to review.

It's hard to put this stuff in a database.  Truth is that it's highly
unstructured.

IMHO an mbox is not the right interface either, though.  I guess there
must be something in the middle, like a *cough*patch manager*cough*.  At
least there should be a way to mark patches: a "is this a patch" (or
merely discussion) boolean; and a free-form field where other people can
make comments.  Well, I guess that's what Review Board is for.  I think
we should start asking patch submitters to load their patches on RB.

-- 
Alvaro Herrera                                http://www.CommandPrompt.com/
The PostgreSQL Company - Command Prompt, Inc.


Re: Patch review

From
Magnus Hagander
Date:
On Fri, Feb 08, 2008 at 12:22:08PM -0300, Alvaro Herrera wrote:
> Gregory Stark wrote:
> > Is someone working on dumping the list into a table on the wiki? I could
> > download the mbox files from the web site and filter them into a table.
> > 
> > Some part of me thinks this data should be in a postgres database so I can do
> > SQL queries against it to find a good patch to review.
> 
> It's hard to put this stuff in a database.  Truth is that it's highly
> unstructured.
> 
> IMHO an mbox is not the right interface either, though.  I guess there
> must be something in the middle, like a *cough*patch manager*cough*.  At
> least there should be a way to mark patches: a "is this a patch" (or
> merely discussion) boolean; and a free-form field where other people can
> make comments.  Well, I guess that's what Review Board is for.  I think
> we should start asking patch submitters to load their patches on RB.

I agree with the need of some kind of patch tracking thats nicer than what
we have now, but let's at least try out the product all the way before we
ask people to use it! :-)

//Magnus


Re: Patch review

From
Bruce Momjian
Date:
Alvaro Herrera wrote:
> Gregory Stark wrote:
> 
> > so.... I went to look for the held patches queue to start reviewing patches.
> > There are over 2000 messages in the queue in 300 separate threads. At that
> > rate it would probably be just as easy to scan the patches and hackers mailing
> > list.
> 
> Pretty unwieldy, yes.  I'm not sure -patchers of -hackers is really
> "just as easy" though.
> 
> > Is someone working on dumping the list into a table on the wiki? I could
> > download the mbox files from the web site and filter them into a table.
> > 
> > Some part of me thinks this data should be in a postgres database so I can do
> > SQL queries against it to find a good patch to review.
> 
> It's hard to put this stuff in a database.  Truth is that it's highly
> unstructured.

Yep, _unstructured_ is the word for it.

We have a commit-fest coming March 1 I think so I will have the hold
patch queue cleaned up by then and only valid items will be left there.

> IMHO an mbox is not the right interface either, though.  I guess there
> must be something in the middle, like a *cough*patch manager*cough*.  At
> least there should be a way to mark patches: a "is this a patch" (or
> merely discussion) boolean; and a free-form field where other people can
> make comments.  Well, I guess that's what Review Board is for.  I think
> we should start asking patch submitters to load their patches on RB.

Frankly I think the structuring of the data is the hard part.  For 8.3
we had that web page that tracked the outstanding patches and that was
very useful because the patches were addressed over a 4-5 month period.

Ideally we could have that status for all patches all the time, but the
time required to structure/categorize them often isn't worth it.  We
could have the submitters do the categorizing or the patch appliers, but
in many cases the structuring is more work than just getting the patch
applied and completed.

If you think it would be easy to get patch submitters to categorize,
realize we have some very skilled people who don't even send email
reports of bugs, they just report them on IRC and can't be bothered with
email.  If email is a burden for them, imagine filling in a web form.

If patch appliers categorize, why would then do that if they are just
going to go an apply the patch and complete it?

Perhaps we can have two ways of submitting patches --- one that
categorizes and another that is more freeform via email?  Patch appliers
would have to deal with both.

--  Bruce Momjian  <bruce@momjian.us>        http://momjian.us EnterpriseDB
http://postgres.enterprisedb.com
 + If your life is a hard drive, Christ can be your backup. +


Re: Patch review

From
Tom Lane
Date:
Bruce Momjian <bruce@momjian.us> writes:
> If you think it would be easy to get patch submitters to categorize,
> realize we have some very skilled people who don't even send email
> reports of bugs, they just report them on IRC and can't be bothered with
> email.  If email is a burden for them, imagine filling in a web form.
> ...
> Perhaps we can have two ways of submitting patches --- one that
> categorizes and another that is more freeform via email?  Patch appliers
> would have to deal with both.

Let's not swat flies with steam hammers.  There are various sizes of
problem here, and they need different sizes of solution.  In particular,
we see a lot of bugs that can be fixed-the-same-day, and those shouldn't
get into the patch queue mechanism at all.  It's the stuff that needs
some extended thought and discussion that needs tracking.
        regards, tom lane


Re: Patch review

From
Magnus Hagander
Date:
Bruce Momjian wrote:
>> IMHO an mbox is not the right interface either, though.  I guess there
>> must be something in the middle, like a *cough*patch manager*cough*.  At
>> least there should be a way to mark patches: a "is this a patch" (or
>> merely discussion) boolean; and a free-form field where other people can
>> make comments.  Well, I guess that's what Review Board is for.  I think
>> we should start asking patch submitters to load their patches on RB.

Not sure if this is platform-specific, but I keep being annoyed by not 
being able to actually *view* the patches in the queue. I have to 
download them, and then open them separately. I can't just view them in 
the browser, because they're all named ".bin" and come out as mime type 
"application/octet-stream".
(That particular problem would be fixed if they were accessible through 
for example IMAP)


> Frankly I think the structuring of the data is the hard part.  For 8.3
> we had that web page that tracked the outstanding patches and that was
> very useful because the patches were addressed over a 4-5 month period.

As Tom already said, we need to differ between the long-review patches 
and the quick fixes. The quick fixes work fine the way we do it now, the 
more advanced long-review patches don't really.


> Ideally we could have that status for all patches all the time, but the
> time required to structure/categorize them often isn't worth it.  We
> could have the submitters do the categorizing or the patch appliers, but
> in many cases the structuring is more work than just getting the patch
> applied and completed.

It may not be worth it for the quick-fixes, but it certainly is for the 
longer ones. The wiki page was very useful.

It would also be better to be able to off-load it to more than one 
person. For example, I would like to be able to get into the unapplied 
patches list and remove the email about events on 8.3RC1. First of all, 
it's not a patch, but it's listed under it. But more importantly, it has 
been fixed and should just be removed. So I now have to email you to ask 
you to remove it, and then you have to do the actual work, which means 
double work.

In fact, those two mbox archives need to be renamed - they're not 
actually patches, from what I can tell, they're more "patches and 
discussions that appear interesting for this release", no?

> If you think it would be easy to get patch submitters to categorize,
> realize we have some very skilled people who don't even send email
> reports of bugs, they just report them on IRC and can't be bothered with
> email.  If email is a burden for them, imagine filling in a web form.

A whole lot of people find it much easier, and less of a burden, to 
submit something like this through a web interface, than through an 
email. For several reasons, one being that almost everything else they 
do "on the net" is through web interfaces of different kinds. Another 
being that they don't have to subscribe to anything, they can just post it.



//Magnus


Re: Patch review

From
Tom Lane
Date:
Magnus Hagander <magnus@hagander.net> writes:
> Not sure if this is platform-specific, but I keep being annoyed by not 
> being able to actually *view* the patches in the queue. I have to 
> download them, and then open them separately. I can't just view them in 
> the browser, because they're all named ".bin" and come out as mime type 
> "application/octet-stream".

Yeah, it happens that way for me too.  The other huge, huge problem with
it is the lack of stability of URLs for the items in the list, which
makes it difficult to identify which item you're talking about.

Personally I'd be happier with an editable wiki page consisting of links
to the original messages in the mail list archives, plus free-format
annotations (such as status).  This should be trivial to set up and
reasonably flexible.  With experience we might find we need something
fancier, but let's not overdesign our solution at the start.
        regards, tom lane


Re: Patch review

From
Stefan Kaltenbrunner
Date:
Tom Lane wrote:
> Magnus Hagander <magnus@hagander.net> writes:
>> Not sure if this is platform-specific, but I keep being annoyed by not 
>> being able to actually *view* the patches in the queue. I have to 
>> download them, and then open them separately. I can't just view them in 
>> the browser, because they're all named ".bin" and come out as mime type 
>> "application/octet-stream".
> 
> Yeah, it happens that way for me too.  The other huge, huge problem with
> it is the lack of stability of URLs for the items in the list, which
> makes it difficult to identify which item you're talking about.

yeah that is fairly annoying :-(

> 
> Personally I'd be happier with an editable wiki page consisting of links
> to the original messages in the mail list archives, plus free-format
> annotations (such as status).  This should be trivial to set up and
> reasonably flexible.  With experience we might find we need something
> fancier, but let's not overdesign our solution at the start.

I will see if I can come up with a proposal on the developer wiki for a 
list that looks like that tomorrow.

_IF_ we are looking for an (over)designed solution I just resurrected 
what was left over from the last years discussion on that topic (and the 
creating of the now defunct tracker mailinglist).
What was discussed back then was a set of requirements for a 
(bug|feature) tracker and based on that I hacked up a prototype using a 
modified/enhanced version of bugzilla 3.0.

some of the discussions/criterias from back than are summarized on:

http://developer.postgresql.org/index.php/TrackerDiscussion

the demo installation itself is on:

http://trackerdemo.postgresql.org

with a modified bug submission form on

http://pgweb.kaltenbrunner.cc/support/submitbug

basic concepts and docs are on the wiki too.


Stefan


Re: Patch review

From
Gregory Stark
Date:
"Stefan Kaltenbrunner" <stefan@kaltenbrunner.cc> writes:

> Tom Lane wrote:
>> Magnus Hagander <magnus@hagander.net> writes:
>>> Not sure if this is platform-specific, but I keep being annoyed by not being
>>> able to actually *view* the patches in the queue. I have to download them,
>>> and then open them separately. I can't just view them in the browser,
>>> because they're all named ".bin" and come out as mime type
>>> "application/octet-stream".
>>
>> Yeah, it happens that way for me too.  The other huge, huge problem with
>> it is the lack of stability of URLs for the items in the list, which
>> makes it difficult to identify which item you're talking about.
>
> yeah that is fairly annoying :-(

Strangely enough I have the opposite complaint. Sometimes the patch is
displayed inline (seems to be pretty unpredictable) and that makes it hard to
download and apply. If you download the page you have to un-html-escape it and
if you copy-paste the contents the whitespace is messed up.

It would be nice if the patches list were fed into reviewboard or viewcvs or
something like it. Even if we only used it for viewing patches it would at
least make that nice.

I couldn't see how to make it work with reviewboard at first glance. It seems
to assume you're starting form a working tree and wants to generate the diff
and send it in itself. If you generate it yourself then you have to tell it
precisely what it's a diff against. It doesn't seem to expect to be used as a
general purpose patch viewer.

>> Personally I'd be happier with an editable wiki page consisting of links
>> to the original messages in the mail list archives, plus free-format
>> annotations (such as status).  This should be trivial to set up and
>> reasonably flexible.  With experience we might find we need something
>> fancier, but let's not overdesign our solution at the start.
>
> I will see if I can come up with a proposal on the developer wiki for a list
> that looks like that tomorrow.

How powerful are these wiki things? Can you write a script which automatically
inserts a new line in a table in the wiki whenever an email to the patches
list containing a patch comes in?

Can you add buttons on the wiki itself to do common actions like removing the
item from the table or joining it up with another item? That way we would only
have to actually edit the wiki content if we have something substantive to add
such as review comments.

> _IF_ we are looking for an (over)designed solution

I'm against overdesigned solutions. If we want more than a wiki I think it
would be reviewboard. I think something which fills a specific hole like
reviewboard is much more likely to work in the long term than an overarching
solution whose problem description is as vague as a "tracker". As soon as our
process changes the "tracker" will be obsolete whereas a problem-specific
solution like reviewboard will always be useful as long as there are patches
to review.

--  Gregory Stark EnterpriseDB          http://www.enterprisedb.com Get trained by Bruce Momjian - ask me about
EnterpriseDB'sPostgreSQL training!
 


Re: Patch review

From
Stefan Kaltenbrunner
Date:
Gregory Stark wrote:
> "Stefan Kaltenbrunner" <stefan@kaltenbrunner.cc> writes:
> 
>> Tom Lane wrote:
>>> Magnus Hagander <magnus@hagander.net> writes:
>>>> Not sure if this is platform-specific, but I keep being annoyed by not being
>>>> able to actually *view* the patches in the queue. I have to download them,
>>>> and then open them separately. I can't just view them in the browser,
>>>> because they're all named ".bin" and come out as mime type
>>>> "application/octet-stream".
>>> Yeah, it happens that way for me too.  The other huge, huge problem with
>>> it is the lack of stability of URLs for the items in the list, which
>>> makes it difficult to identify which item you're talking about.
>> yeah that is fairly annoying :-(
> 
> Strangely enough I have the opposite complaint. Sometimes the patch is
> displayed inline (seems to be pretty unpredictable) and that makes it hard to
> download and apply. If you download the page you have to un-html-escape it and
> if you copy-paste the contents the whitespace is messed up.
> 
> It would be nice if the patches list were fed into reviewboard or viewcvs or
> something like it. Even if we only used it for viewing patches it would at
> least make that nice.

hmm that could be made to work but why not submit them to reviewboard in 
the first place and let it send an mail to the list ?

> 
> I couldn't see how to make it work with reviewboard at first glance. It seems
> to assume you're starting form a working tree and wants to generate the diff
> and send it in itself. If you generate it yourself then you have to tell it
> precisely what it's a diff against. It doesn't seem to expect to be used as a
> general purpose patch viewer.

yeah - I think that dave  was looking into checking if something like 
that would be possible.

> 
>>> Personally I'd be happier with an editable wiki page consisting of links
>>> to the original messages in the mail list archives, plus free-format
>>> annotations (such as status).  This should be trivial to set up and
>>> reasonably flexible.  With experience we might find we need something
>>> fancier, but let's not overdesign our solution at the start.
>> I will see if I can come up with a proposal on the developer wiki for a list
>> that looks like that tomorrow.
> 
> How powerful are these wiki things? Can you write a script which automatically
> inserts a new line in a table in the wiki whenever an email to the patches
> list containing a patch comes in?

yes

> 
> Can you add buttons on the wiki itself to do common actions like removing the
> item from the table or joining it up with another item? That way we would only
> have to actually edit the wiki content if we have something substantive to add
> such as review comments.

not sure on that one

> 
>> _IF_ we are looking for an (over)designed solution
> 
> I'm against overdesigned solutions. If we want more than a wiki I think it
> would be reviewboard. I think something which fills a specific hole like
> reviewboard is much more likely to work in the long term than an overarching
> solution whose problem description is as vague as a "tracker". As soon as our
> process changes the "tracker" will be obsolete whereas a problem-specific
> solution like reviewboard will always be useful as long as there are patches
> to review.

well the trackerdemo installation could do something very similiar to 
what you are describing (I only considered that process for -bugs up to 
know but it could be adapted to patches I guess).

* change the bugreporting for to one that posts to the tracker and have 
it generate the (bug)id
* subscribe the tracker to the list (say pgsql-bugs)
* have people discuss stuff on either the list or via the GUI of the 
tracker - the tracker will be able automatically track the mails and add 
them as comments to the "bug" or not
* have that tracker generate a short summary report based on "open" 
issues on either the wiki (plugins are available for that) or use one of 
the integrated reporting features

Has anybody actually looked into how suitable reviewboard is for 
reviewing large patches ? it seems to have some limitations based on the 
observation that patches are only commented on but are not expected to 
be changed/modified by the reviewer (in fact I think the reviewer cannot 
even upload a modified version of the patch but I could be wrong there).



Stefan


Re: Patch review

From
Stefan Kaltenbrunner
Date:
Stefan Kaltenbrunner wrote:
> Tom Lane wrote:
[...]
>> Personally I'd be happier with an editable wiki page consisting of links
>> to the original messages in the mail list archives, plus free-format
>> annotations (such as status).  This should be trivial to set up and
>> reasonably flexible.  With experience we might find we need something
>> fancier, but let's not overdesign our solution at the start.
> 
> I will see if I can come up with a proposal on the developer wiki for a 
> list that looks like that tomorrow.

I have simply converted the old "8.3 patch status" page for now by 
removing all the applied and rejected patches:

http://developer.postgresql.org/index.php/Todo:PatchStatus

I'm slowly adding some more stuff from the patches_hold list there but 
given the way that list is structured this is a fairly difficult and 
tendious task(a lot there is only discussion/design sketches and in 
effect one has to check every single mail in there to see if should be 
added) :-(


Stefan


Re: Patch review

From
Decibel!
Date:
On Feb 9, 2008, at 1:08 AM, Tom Lane wrote:
> Let's not swat flies with steam hammers.


What the heck is a steam hammer? :P
-- 
Decibel!, aka Jim C. Nasby, Database Architect  decibel@decibel.org
Give your computer some brain candy! www.distributed.net Team #1828



Re: Patch review

From
tomas@tuxteam.de
Date:
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On Mon, Feb 11, 2008 at 05:09:45PM -0600, Decibel! wrote:
> On Feb 9, 2008, at 1:08 AM, Tom Lane wrote:
> >Let's not swat flies with steam hammers.
> 
> 
> What the heck is a steam hammer? :P
 <http://en.wikipedia.org/wiki/Creusot_steam_hammer>

The difficult part would be to convince the fly to stay in place...

Regrds
- -- tomás
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.1 (GNU/Linux)

iD8DBQFHsSVoBcgs9XrR2kYRAq/ZAJ9yx58rlPZaLklbR4gr7YVMUHjbOACfVWxJ
JUSd7OVhxi0PPeAS18Ku6Uk=
=dexU
-----END PGP SIGNATURE-----


Re: Patch review

From
Bruce Momjian
Date:
Magnus Hagander wrote:
> It would also be better to be able to off-load it to more than one 
> person. For example, I would like to be able to get into the unapplied 
> patches list and remove the email about events on 8.3RC1. First of all, 
> it's not a patch, but it's listed under it. But more importantly, it has 
> been fixed and should just be removed. So I now have to email you to ask 
> you to remove it, and then you have to do the actual work, which means 
> double work.

True.  Those web pages are emails pulled from the stream of emails that
I think are worthy of discussion during 8.4 development.

If we assume we want to continue communicating via email I need a way to
pull items out and collect them, and unfortunately right now it is hard
for others to help in that.

I have a few ideas.  First I could easily create an email address that
would allow others to _add_ emails to the web page (via bounce) but that
doesn't solve the issue of allowing people to comment on and delete
items.  

Do most email readers support bouncing emails to another address, so the
to/from fields are not modified?  (Seems there is a Thunderbird
extension to do it,
http://blog.mecworks.com/articles/2005/04/20/bouncing-mail-in-thunderbird/.)

There is no reason I have to host the list here.  I can _bounce_ emails
to any address.  Is there a service we can use that allows emails to be
accepted and displayed on a web site and that allows deletions and
comments, and has stable URLs for every email message?  Is there
software I can install on my server to do this?

If we want to communicate via a web interface, I would still need a way
to collect specific messages (not always entire threads).

--  Bruce Momjian  <bruce@momjian.us>        http://momjian.us EnterpriseDB
http://postgres.enterprisedb.com
 + If your life is a hard drive, Christ can be your backup. +


Re: Patch review

From
Martijn van Oosterhout
Date:
On Tue, Feb 12, 2008 at 05:49:12AM -0500, Bruce Momjian wrote:
> There is no reason I have to host the list here.  I can _bounce_ emails
> to any address.  Is there a service we can use that allows emails to be
> accepted and displayed on a web site and that allows deletions and
> comments, and has stable URLs for every email message?  Is there
> software I can install on my server to do this?

debbugs? *duck*

Personally I like trac for its simplicity. You can add comments and
post patches and track status. Which seems more than enough for what we
do.

Have a nice day,
--
Martijn van Oosterhout   <kleptog@svana.org>   http://svana.org/kleptog/
> Those who make peaceful revolution impossible will make violent revolution inevitable.
>  -- John F Kennedy

Re: Patch review

From
Gregory Stark
Date:
"Bruce Momjian" <bruce@momjian.us> writes:

> If we want to communicate via a web interface, I would still need a way
> to collect specific messages (not always entire threads).

You're talking about collecting messages. Everyone else wants to collect
patches or proposed changes. There's not a one-to-one mapping between the two.

One example of what we would want is a table in a wiki where it was easy to
insert a reference to the most current patch, a few pointers to messages where
it was discussed, and the current status.

Or something like the review-board dashboard which lists the patches under
consideration with their current status. Ideally with pgsql-patches gatewayed
to create new patches in the list and comments gatewayed back to the list.

I'm sure we want to communicate by email. We also want to have an interface we
can all use to maintain a queue of patches under consideration. They're two
separate, but related, things.

--  Gregory Stark EnterpriseDB          http://www.enterprisedb.com Ask me about EnterpriseDB's On-Demand Production
Tuning


Re: Patch review

From
Stefan Kaltenbrunner
Date:
Gregory Stark wrote:
> "Bruce Momjian" <bruce@momjian.us> writes:
> 
>> If we want to communicate via a web interface, I would still need a way
>> to collect specific messages (not always entire threads).
> 
> You're talking about collecting messages. Everyone else wants to collect
> patches or proposed changes. There's not a one-to-one mapping between the two.
> 
> One example of what we would want is a table in a wiki where it was easy to
> insert a reference to the most current patch, a few pointers to messages where
> it was discussed, and the current status.

yeah but generating that list is not so easy - in a perfect world that 
short overview list would be generated by whatever tool we use.

> 
> Or something like the review-board dashboard which lists the patches under
> consideration with their current status. Ideally with pgsql-patches gatewayed
> to create new patches in the list and comments gatewayed back to the list.

most modern trackers can do that more or less (like BZ can track 
conversations provided it has "some" way to infer what mail might be 
related to what bug/patch/featurerequest).
I'm trying to hack that feature up for the demo install to simply do 
that transparently for -bugs so that we can see this working for the 
simple case of -bugs without having to do any chances ...

> 
> I'm sure we want to communicate by email. We also want to have an interface we
> can all use to maintain a queue of patches under consideration. They're two
> separate, but related, things.

yes well but that interface could very well be a more featureful tracker 
that we use to generate that list ...


Stefan


Re: Patch review

From
Stefan Kaltenbrunner
Date:
Martijn van Oosterhout wrote:
> On Tue, Feb 12, 2008 at 05:49:12AM -0500, Bruce Momjian wrote:
>> There is no reason I have to host the list here.  I can _bounce_ emails
>> to any address.  Is there a service we can use that allows emails to be
>> accepted and displayed on a web site and that allows deletions and
>> comments, and has stable URLs for every email message?  Is there
>> software I can install on my server to do this?
> 
> debbugs? *duck*

debbugs is nice but it is more or less developed purely for debian with 
rarely any regular releases afaik ?


Stefan


Re: Patch review

From
"Joshua D. Drake"
Date:
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On Mon, 11 Feb 2008 17:09:45 -0600
Decibel! <decibel@decibel.org> wrote:

> On Feb 9, 2008, at 1:08 AM, Tom Lane wrote:
> > Let's not swat flies with steam hammers.
> 
> 
> What the heck is a steam hammer? :P

http://en.wikipedia.org/wiki/Steam_hammer

Joshua D. Drake

- -- 
The PostgreSQL Company since 1997: http://www.commandprompt.com/ 
PostgreSQL Community Conference: http://www.postgresqlconference.org/
Donate to the PostgreSQL Project: http://www.postgresql.org/about/donate
PostgreSQL SPI Liaison | SPI Director |  PostgreSQL political pundit

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.6 (GNU/Linux)

iD8DBQFHsdBfATb/zqfZUUQRAnkwAKCo5TaaIOWHVYFWCW/i5s0mHd5vDQCfUw1P
0sh/PuDHAbvQQITVuQbeT3s=
=EUBM
-----END PGP SIGNATURE-----

Re: Patch review

From
Bruce Momjian
Date:
Stefan Kaltenbrunner wrote:
> Stefan Kaltenbrunner wrote:
> > Tom Lane wrote:
> [...]
> >> Personally I'd be happier with an editable wiki page consisting of links
> >> to the original messages in the mail list archives, plus free-format
> >> annotations (such as status).  This should be trivial to set up and
> >> reasonably flexible.  With experience we might find we need something
> >> fancier, but let's not overdesign our solution at the start.
> > 
> > I will see if I can come up with a proposal on the developer wiki for a 
> > list that looks like that tomorrow.
> 
> I have simply converted the old "8.3 patch status" page for now by 
> removing all the applied and rejected patches:
> 
> http://developer.postgresql.org/index.php/Todo:PatchStatus
> 
> I'm slowly adding some more stuff from the patches_hold list there but 
> given the way that list is structured this is a fairly difficult and 
> tendious task(a lot there is only discussion/design sketches and in 
> effect one has to check every single mail in there to see if should be 
> added) :-(

I have added message-id's to both patches web sites.  The message id
appears next to the author in the thread listing, and at the top of the
message page.

Unfortunately you can't search by message id in our archives, but if
that is fixed, I can add HTML to get that to work too.

I was hoping someone had a completed solution, but I guess not.

--  Bruce Momjian  <bruce@momjian.us>        http://momjian.us EnterpriseDB
http://postgres.enterprisedb.com
 + If your life is a hard drive, Christ can be your backup. +


Re: Patch review

From
Andrew Dunstan
Date:

Joshua D. Drake wrote:
>> What the heck is a steam hammer? :P
>>     
>
> http://en.wikipedia.org/wiki/Steam_hammer
>
>
>   

The same people went on to invent the steam television ...

cheers

andrew


Re: Patch review

From
Tom Lane
Date:
Bruce Momjian <bruce@momjian.us> writes:
> I have added message-id's to both patches web sites.  The message id
> appears next to the author in the thread listing, and at the top of the
> message page.

That's an improvement, but it doesn't solve the other fundamental
problem, which is the lack of any way to annotate the list.
        regards, tom lane


Re: Patch review

From
Bruce Momjian
Date:
Tom Lane wrote:
> Bruce Momjian <bruce@momjian.us> writes:
> > I have added message-id's to both patches web sites.  The message id
> > appears next to the author in the thread listing, and at the top of the
> > message page.
> 
> That's an improvement, but it doesn't solve the other fundamental
> problem, which is the lack of any way to annotate the list.

Agreed, and it doesn't allow people to delete items either.  Hold, I
think I can add annotations if that's what people want; you can see an
example in my blog:
http://momjian.us/main/blogs/blog.html

--  Bruce Momjian  <bruce@momjian.us>        http://momjian.us EnterpriseDB
http://postgres.enterprisedb.com
 + If your life is a hard drive, Christ can be your backup. +


Re: Patch review

From
Bruce Momjian
Date:
Bruce Momjian wrote:
> Tom Lane wrote:
> > Bruce Momjian <bruce@momjian.us> writes:
> > > I have added message-id's to both patches web sites.  The message id
> > > appears next to the author in the thread listing, and at the top of the
> > > message page.
> > 
> > That's an improvement, but it doesn't solve the other fundamental
> > problem, which is the lack of any way to annotate the list.
> 
> Agreed, and it doesn't allow people to delete items either.  Hold, I
> think I can add annotations if that's what people want; you can see an
> example in my blog:
> 
>     http://momjian.us/main/blogs/blog.html

OK, comments added, and they are based on message-id, so they will not
change over time.  

I am using JS-Kit, http://js-kit.com/comments/, article
http://blog.wired.com/monkeybites/2006/11/jskit_add_comme.html.

--  Bruce Momjian  <bruce@momjian.us>        http://momjian.us EnterpriseDB
http://postgres.enterprisedb.com
 + If your life is a hard drive, Christ can be your backup. +


Re: Patch review

From
Bruce Momjian
Date:
Bruce Momjian wrote:
> Bruce Momjian wrote:
> > Tom Lane wrote:
> > > Bruce Momjian <bruce@momjian.us> writes:
> > > > I have added message-id's to both patches web sites.  The message id
> > > > appears next to the author in the thread listing, and at the top of the
> > > > message page.
> > > 
> > > That's an improvement, but it doesn't solve the other fundamental
> > > problem, which is the lack of any way to annotate the list.
> > 
> > Agreed, and it doesn't allow people to delete items either.  Hold, I
> > think I can add annotations if that's what people want; you can see an
> > example in my blog:
> > 
> >     http://momjian.us/main/blogs/blog.html
> 
> OK, comments added, and they are based on message-id, so they will not
> change over time.  
> 
> I am using JS-Kit, http://js-kit.com/comments/, article
> http://blog.wired.com/monkeybites/2006/11/jskit_add_comme.html.

Seems the comments slowed down the page load so I now have the thread
broken down into 50 emails per page.

--  Bruce Momjian  <bruce@momjian.us>        http://momjian.us EnterpriseDB
http://postgres.enterprisedb.com
 + If your life is a hard drive, Christ can be your backup. +


Re: Patch review

From
Bruce Momjian
Date:
Bruce Momjian wrote:
> Bruce Momjian wrote:
> > Bruce Momjian wrote:
> > > Tom Lane wrote:
> > > > Bruce Momjian <bruce@momjian.us> writes:
> > > > > I have added message-id's to both patches web sites.  The message id
> > > > > appears next to the author in the thread listing, and at the top of the
> > > > > message page.
> > > > 
> > > > That's an improvement, but it doesn't solve the other fundamental
> > > > problem, which is the lack of any way to annotate the list.
> > > 
> > > Agreed, and it doesn't allow people to delete items either.  Hold, I
> > > think I can add annotations if that's what people want; you can see an
> > > example in my blog:
> > > 
> > >     http://momjian.us/main/blogs/blog.html
> > 
> > OK, comments added, and they are based on message-id, so they will not
> > change over time.  
> > 
> > I am using JS-Kit, http://js-kit.com/comments/, article
> > http://blog.wired.com/monkeybites/2006/11/jskit_add_comme.html.
> 
> Seems the comments slowed down the page load so I now have the thread
> broken down into 50 emails per page.

I got a message from someone saying I was trying too hard to avoid using
a tracker.

I think I was clear what functionality I needed and no one had any
ready-made solutions.  Let me give a practical example of what I need.

Suppose we were using a web-based discussion forum, rather than email. 
Assume it is something like Slashdot, where you have article titles and
comments.

For the patches lists I need to take sometimes entire threads, sometimes
groups of comments, and store them in a format so people can review them
as a digest.  And I want to allow comments on these items, and ideally
allow multiple people to delete them.

If I link to a comment URL, how do people know if they should look at
that comment or all comments below it?  

If I had omment URLs, how would I present those in a threaded way?  A
flat list of URL would be too crude and lack titles.   So, ultimately I
would need a threaded way to present the URLs, which is pretty much what
mhonarc does now.  Comments would certainly be easier because you could
just add comments to the discussion threads.  Deleting URLs would be the
same as mhonarc, except the URLs would be static.

So, if we did have a tracker, how would it be different?  Comments would
be more integrated but I am unclear how the patches_hold queue would be
different.

Basically what I do now is to take the email stream and chop pieces out
of it for later review.  I see a tracker making some of that process
easier and more distributed/shared, but some of it harder and more
complex.

--  Bruce Momjian  <bruce@momjian.us>        http://momjian.us EnterpriseDB
http://postgres.enterprisedb.com
 + If your life is a hard drive, Christ can be your backup. +


Re: Patch review

From
"Brendan Jurd"
Date:
On Feb 13, 2008 10:45 AM, Bruce Momjian <bruce@momjian.us> wrote:
> For the patches lists I need to take sometimes entire threads, sometimes
> groups of comments, and store them in a format so people can review them
> as a digest.  And I want to allow comments on these items, and ideally
> allow multiple people to delete them.
>

Hi Bruce,

If we were using a tracker, why would you need to produce this
"digest" at all?  Why would you not simply refer people to, e.g., the
tracker's report of all outstanding tickets in 8.4?

Regards,
BJ


Re: Patch review

From
Bruce Momjian
Date:
Brendan Jurd wrote:
> On Feb 13, 2008 10:45 AM, Bruce Momjian <bruce@momjian.us> wrote:
> > For the patches lists I need to take sometimes entire threads, sometimes
> > groups of comments, and store them in a format so people can review them
> > as a digest.  And I want to allow comments on these items, and ideally
> > allow multiple people to delete them.
> >
> 
> Hi Bruce,
> 
> If we were using a tracker, why would you need to produce this
> "digest" at all?  Why would you not simply refer people to, e.g., the
> tracker's report of all outstanding tickets in 8.4?

Uh, well, it is kind of hard because it assumes discussions are linear,
meaning an item has only one ticket, and the ticket is clear on what to
do.  In practice, that is not usually the case.

--  Bruce Momjian  <bruce@momjian.us>        http://momjian.us EnterpriseDB
http://postgres.enterprisedb.com
 + If your life is a hard drive, Christ can be your backup. +


Re: Patch review

From
Gregory Stark
Date:
"Bruce Momjian" <bruce@momjian.us> writes:

> Suppose we were using a web-based discussion forum, rather than email. 

That would be crazy, why would I suppose such a thing?

> For the patches lists I need to take sometimes entire threads, sometimes
> groups of comments, and store them in a format so people can review them
> as a digest.  

Why do you need to do any such thing? What does any of that have to do with a
patches queue?

> If I link to a comment URL, how do people know if they should look at
> that comment or all comments below it?  

They should look at whatever they want to. I usually have to back up several
messages to understand the context and then follow several messages later. You
can't possibly know how much context people will need to understand. You can't
try to control people to that level of detail.

> If I had omment URLs, how would I present those in a threaded way?  

> So, if we did have a tracker, how would it be different?  Comments would
> be more integrated but I am unclear how the patches_hold queue would be
> different.

A patches queue is just a list of patches with their current status. Not a
replacement for our mailing lists. You're trying to solve the wrong problem.

The current status for a patch is just something like "waiting for feedback on
questions from message [link]" or "waiting for new version addressing issues
raised in review [link]". That's it.

The critical information we need are: What's the most recent version of the
patch? what is it blocking on? and who is it blocking on? 

--  Gregory Stark EnterpriseDB          http://www.enterprisedb.com Ask me about EnterpriseDB's Slony Replication
support!


Re: Patch review

From
Bruce Momjian
Date:
Gregory Stark wrote:
> "Bruce Momjian" <bruce@momjian.us> writes:
> 
> > Suppose we were using a web-based discussion forum, rather than email. 
> 
> That would be crazy, why would I suppose such a thing?
> 
> > For the patches lists I need to take sometimes entire threads, sometimes
> > groups of comments, and store them in a format so people can review them
> > as a digest.  
> 
> Why do you need to do any such thing? What does any of that have to do with a
> patches queue?

TODO items and patches are often in the middle of threads.

> > If I link to a comment URL, how do people know if they should look at
> > that comment or all comments below it?  
> 
> They should look at whatever they want to. I usually have to back up several
> messages to understand the context and then follow several messages later. You
> can't possibly know how much context people will need to understand. You can't
> try to control people to that level of detail.
> 
> > If I had omment URLs, how would I present those in a threaded way?  
> 
> > So, if we did have a tracker, how would it be different?  Comments would
> > be more integrated but I am unclear how the patches_hold queue would be
> > different.
> 
> A patches queue is just a list of patches with their current status. Not a
> replacement for our mailing lists. You're trying to solve the wrong problem.
> 
> The current status for a patch is just something like "waiting for feedback on
> questions from message [link]" or "waiting for new version addressing issues
> raised in review [link]". That's it.
> 
> The critical information we need are: What's the most recent version of the
> patch? what is it blocking on? and who is it blocking on? 

The discussion was mostly related to the 8.3 patches_hold queue where
people wanted help processing it.

--  Bruce Momjian  <bruce@momjian.us>        http://momjian.us EnterpriseDB
http://postgres.enterprisedb.com
 + If your life is a hard drive, Christ can be your backup. +


Re: Patch review

From
Gregory Stark
Date:
"Bruce Momjian" <bruce@momjian.us> writes:

> Gregory Stark wrote:
>> "Bruce Momjian" <bruce@momjian.us> writes:
>> 
>> > For the patches lists I need to take sometimes entire threads, sometimes
>> > groups of comments, and store them in a format so people can review them
>> > as a digest.  
>> 
>> Why do you need to do any such thing? What does any of that have to do with a
>> patches queue?
>
> TODO items and patches are often in the middle of threads.

No, messages which propose the items and patches are in the middle of the
thread. What we need is a list of the actual items. We already have an archive
of the messages.

Looking at your held queue, how many patches are in there? Who are the
authors? Which ones are waiting for feedback before they can continue?

>> The critical information we need are: What's the most recent version of the
>> patch? what is it blocking on? and who is it blocking on? 
>
> The discussion was mostly related to the 8.3 patches_hold queue where
> people wanted help processing it.

Yes, the information I listed is the what we need to do that.

--  Gregory Stark EnterpriseDB          http://www.enterprisedb.com Ask me about EnterpriseDB's PostGIS support!


Re: Patch review

From
Greg Smith
Date:
On Wed, 13 Feb 2008, Gregory Stark wrote:

> "Bruce Momjian" <bruce@momjian.us> writes:
>
>> If I link to a comment URL, how do people know if they should look at
>> that comment or all comments below it?
>
> They should look at whatever they want to. I usually have to back up several
> messages to understand the context and then follow several messages later.

If you look at how the archives store things, the threading in there 
sometimes isn't sufficient to support this.  As an example, I was just 
trying to read all the messages in the "Group Commit" thread that Bruce 
has tracked on "Patches Held For PostgreSQL 8.4", and for reasons I can't 
figure out it's split into two threads in the archives.  If you just 
caught the earlier thread in there it would not be obvious that there's a 
later one.  My e-mail archive, like Bruce's that he converts onto the web 
page, doesn't have that problem, but people not there for the original 
discussion wouldn't have that available.

I think your basic argument that "patches queue" and "relevant discussion 
archive" can be managed independantly still holds, but there is some value 
to the way Bruce collects up the more interesting posts in the thread.

--
* Greg Smith gsmith@gregsmith.com http://www.gregsmith.com Baltimore, MD


Re: Patch review

From
Alvaro Herrera
Date:
Greg Smith wrote:

> If you look at how the archives store things, the threading in there  
> sometimes isn't sufficient to support this.  As an example, I was just  
> trying to read all the messages in the "Group Commit" thread that Bruce  
> has tracked on "Patches Held For PostgreSQL 8.4", and for reasons I can't 
> figure out it's split into two threads in the archives.

Perhaps it's because it was split in two by a monthly boundary?  (I
didn't look.)

-- 
Alvaro Herrera                                http://www.CommandPrompt.com/
The PostgreSQL Company - Command Prompt, Inc.


Re: Patch review

From
Tom Lane
Date:
Alvaro Herrera <alvherre@commandprompt.com> writes:
> Greg Smith wrote:
>> If you look at how the archives store things, the threading in there  
>> sometimes isn't sufficient to support this.  As an example, I was just  
>> trying to read all the messages in the "Group Commit" thread that Bruce  
>> has tracked on "Patches Held For PostgreSQL 8.4", and for reasons I can't 
>> figure out it's split into two threads in the archives.

> Perhaps it's because it was split in two by a monthly boundary?  (I
> didn't look.)

Most likely.  Somebody on the www team really ought to make an effort
to fix that sometime --- it reduces the value of the archives noticeably
if you can't rely on the thread links.
        regards, tom lane


Re: [pgsql-www] Patch review

From
"Dave Page"
Date:
On Feb 13, 2008 3:27 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> > Perhaps it's because it was split in two by a monthly boundary?  (I
> > didn't look.)
>
> Most likely.  Somebody on the www team really ought to make an effort
> to fix that sometime --- it reduces the value of the archives noticeably
> if you can't rely on the thread links.

The current system makes that pretty much unfixable. I have spent time
on a PG backed replacement which does solve that and other problems
but unfortunately I just haven't had time for it in a few months. If
someone with PHP skillz wants to pick it up please let me know.

-- 
Dave Page
EnterpriseDB UK: http://www.enterprisedb.com
The Oracle-compatible database company


Re: Patch review

From
Greg Smith
Date:
On Wed, 13 Feb 2008, Alvaro Herrera wrote:

> Perhaps it's because it was split in two by a monthly boundary?  (I
> didn't look.)

That looks to be it.  There's also another split it did manage to catch 
where the original author started a new thread themselves that got linked 
in.  That sort of thing (renamed or improperly continued thread on the 
same topic) is another spot where someone keeping track of things manually 
can end up with a more consistant view of the discussion than the archives 
provide.

--
* Greg Smith gsmith@gregsmith.com http://www.gregsmith.com Baltimore, MD


Re: Patch review

From
Gregory Stark
Date:
"Greg Smith" <gsmith@gregsmith.com> writes:

> On Wed, 13 Feb 2008, Alvaro Herrera wrote:
>
>> Perhaps it's because it was split in two by a monthly boundary?  (I
>> didn't look.)
>
> That looks to be it.  There's also another split it did manage to catch where
> the original author started a new thread themselves that got linked in.  That
> sort of thing (renamed or improperly continued thread on the same topic) is
> another spot where someone keeping track of things manually can end up with a
> more consistant view of the discussion than the archives provide.

There's no reason we can't include links to messages from the patch queue, but
the links are not sufficient in themselves to be considered the actual patch
queue.

The point of shared resources like a patch queue is to get us all on the same
page about the status of a patch and remind us which ones are in our domain,
whether we're a developer, reviewer, or committer.

Pointing to mail messages doesn't help us with any of that. We have to go back
and read the original message and make a judgement ourselves what state it's
in. If our judgement disagrees with others patches will just sit there with
everyone assuming someone else is looking at it.

--  Gregory Stark EnterpriseDB          http://www.enterprisedb.com Ask me about EnterpriseDB's Slony Replication
support!


Re: Patch review

From
Alvaro Herrera
Date:
Gregory Stark wrote:

> Pointing to mail messages doesn't help us with any of that. We have to go back
> and read the original message and make a judgement ourselves what state it's
> in. If our judgement disagrees with others patches will just sit there with
> everyone assuming someone else is looking at it.

Agreed.  We need that sort of info stored outside the mail list
discussion itself.

-- 
Alvaro Herrera                                http://www.CommandPrompt.com/
The PostgreSQL Company - Command Prompt, Inc.


Re: Patch review

From
Bruce Momjian
Date:
Alvaro Herrera wrote:
> Gregory Stark wrote:
> 
> > Pointing to mail messages doesn't help us with any of that. We have to go back
> > and read the original message and make a judgement ourselves what state it's
> > in. If our judgement disagrees with others patches will just sit there with
> > everyone assuming someone else is looking at it.
> 
> Agreed.  We need that sort of info stored outside the mail list
> discussion itself.

Now that we have comments on the patch queue people can actually claim
items.

--  Bruce Momjian  <bruce@momjian.us>        http://momjian.us EnterpriseDB
http://postgres.enterprisedb.com
 + If your life is a hard drive, Christ can be your backup. +