Обсуждение: September 2015 Commitfest
Hi, Two days ago the September Commitfest started. I'm going to be your host^W commitfest manager this time round. To start off I went through all entries and tried to make sure their state is accurate. Right now we have: Needs review: 47 Waiting on Author: 24 Ready for Committer: 7 Committed: 7 Rejected: 3 Returned with Feedback: 3 Total: 91 Of the open entries 41 currently have no assigned reviewer(s). My impression from the last commitfests and the current state of this fest is that several authors with, in some cases numerous or large, patches are not doing the corresponding amount of review on other patches. Let's change that! Author or not, please review patches. Even if you don't want to sign yourself up for an individual patch, just send a review. Don't let yourself be stopped because a well known person has also signed up for a patch! To review (copying Heikki's message): 1. Pick a patch from the list at: https://commitfest.postgresql.org/6/ 2. Review it. Test it. Try to break it. Do we want the feature? Any weird interactions in corner-cases? Does it have the intended or an unintended performance impact? 3. Reply on the thread on pgsql-hackers with your findings. It is perfectly OK to review a patch that someone else has signed up for as reviewer - different people tend to pay attention to different things. Greetings, Andres Freund
On 09/03/2015 03:06 PM, Andres Freund wrote: > Hi, > > Two days ago the September Commitfest started. I'm going to be your > host^W commitfest manager this time round. > > To start off I went through all entries and tried to make sure their > state is accurate. Right now we have: > > Needs review: 47 > Waiting on Author: 24 > Ready for Committer: 7 > Committed: 7 > Rejected: 3 > Returned with Feedback: 3 > Total: 91 > > Of the open entries 41 currently have no assigned reviewer(s). I've just noticed that we're not tracking the hashjoin alloc bugfix, which was reported back in June. Can you please add it to this CF? This is the last message in the thread: http://www.postgresql.org/message-id/55E78322.10201@2ndquadrant.com I've also noticed the 'multivariate stats' patch was returned with feedback (as agreed on 25/8), but it was not added to this CF. Maybe I missed something, but I assumed that happens automatically. I plan to submit a new version, reflecting the discussion. Can we add it? BTW is there some place tracking these commitfest rules? > My impression from the last commitfests and the current state of > this fest is that several authors with, in some cases numerous or > large, patches are not doing the corresponding amount of review on > other patches. Let's change that! Given the size of the multivariate stats patch, I guess I'm one of those slackers, Will fix. regards -- Tomas Vondra http://www.2ndQuadrant.com PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On Thu, Sep 3, 2015 at 10:06 PM, Andres Freund <andres@anarazel.de> wrote: > To review (copying Heikki's message): > > 1. Pick a patch from the list at: > > https://commitfest.postgresql.org/6/ > > 2. Review it. Test it. Try to break it. Do we want the feature? Any > weird interactions in corner-cases? Does it have the intended or an > unintended performance impact? > > 3. Reply on the thread on pgsql-hackers with your findings. > > It is perfectly OK to review a patch that someone else has signed up for as > reviewer - different people tend to pay attention to different things. We are close to the end of October, and the numbers are a bit more encouraging than at the beginning: Needs review: 27. Waiting on Author: 12. Ready for Committer: 5. Committed: 30 Among the five patches marked as ready for committer, one is a bug fix that should be back-patched (ahem). Shouldn't we move on with those entries first? Also, to all reviewers and authors, please be sure that the status of your patch is correctly updated. -- Michael
On 2015-10-22 10:52:36 +0900, Michael Paquier wrote: > We are close to the end of October, and the numbers are a bit more > encouraging than at the beginning: FWIW, I think this has been a commitfest with frustratingly low review participation outside a few patches. > Among the five patches marked as ready for committer, one is a bug fix > that should be back-patched (ahem). Shouldn't we move on with those > entries first? I think at this point we essentially can just move all entries to the next. Will do that, and note down which patches haven't gotten any real review. > Also, to all reviewers and authors, please be sure that the status of > your patch is correctly updated. Yes, please! Greetings, Andres Freund
On Thu, Oct 22, 2015 at 9:21 AM, Andres Freund wrote: >> Among the five patches marked as ready for committer, one is a bug fix >> that should be back-patched (ahem). Shouldn't we move on with those >> entries first? > > I think at this point we essentially can just move all entries to the > next. Will do that, and note down which patches haven't gotten any real > review. We are close to the end of the month. Should I move on to do the vacuuming or are you planning to do it? At this stage, to be fair with people whose patches are in "waiting on author" state and because there is not much time until the next CF begins, I propose to remove all the remaining 43 entries with the same status as currently listed: Needs review: 26. Waiting on Author: 11. Ready for Committer: 6. Regards, -- Michael
On Fri, Oct 30, 2015 at 10:47 AM, Michael Paquier <michael.paquier@gmail.com> wrote: > On Thu, Oct 22, 2015 at 9:21 AM, Andres Freund wrote: >>> Among the five patches marked as ready for committer, one is a bug fix >>> that should be back-patched (ahem). Shouldn't we move on with those >>> entries first? >> >> I think at this point we essentially can just move all entries to the >> next. Will do that, and note down which patches haven't gotten any real >> review. > > We are close to the end of the month. Should I move on to do the > vacuuming or are you planning to do it? At this stage, to be fair with > people whose patches are in "waiting on author" state and because > there is not much time until the next CF begins, I propose to remove > all the remaining 43 entries with the same status as currently listed: > Needs review: 26. Waiting on Author: 11. Ready for Committer: 6. Gosh, that's a lot of stuff that didn't get reviewed. :-( -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
On Fri, Oct 30, 2015 at 2:02 PM, Robert Haas wrote: > On Fri, Oct 30, 2015 at 10:47 AM, Michael Paquier wrote: >> On Thu, Oct 22, 2015 at 9:21 AM, Andres Freund wrote: >>>> Among the five patches marked as ready for committer, one is a bug fix >>>> that should be back-patched (ahem). Shouldn't we move on with those >>>> entries first? >>> >>> I think at this point we essentially can just move all entries to the >>> next. Will do that, and note down which patches haven't gotten any real >>> review. >> >> We are close to the end of the month. Should I move on to do the >> vacuuming or are you planning to do it? At this stage, to be fair with >> people whose patches are in "waiting on author" state and because >> there is not much time until the next CF begins, I propose to remove >> all the remaining 43 entries with the same status as currently listed: >> Needs review: 26. Waiting on Author: 11. Ready for Committer: 6. So, seeing nothing happening I have done the above, opened 2015-11 CF and closed the current one. > Gosh, that's a lot of stuff that didn't get reviewed. :-( Yep. -- Michael
On 2015-10-31 00:42:54 +0100, Michael Paquier wrote: > On Fri, Oct 30, 2015 at 2:02 PM, Robert Haas wrote: > > On Fri, Oct 30, 2015 at 10:47 AM, Michael Paquier wrote: > >> On Thu, Oct 22, 2015 at 9:21 AM, Andres Freund wrote: > >>>> Among the five patches marked as ready for committer, one is a bug fix > >>>> that should be back-patched (ahem). Shouldn't we move on with those > >>>> entries first? > >>> > >>> I think at this point we essentially can just move all entries to the > >>> next. Will do that, and note down which patches haven't gotten any real > >>> review. > >> > >> We are close to the end of the month. Should I move on to do the > >> vacuuming or are you planning to do it? At this stage, to be fair with > >> people whose patches are in "waiting on author" state and because > >> there is not much time until the next CF begins, I propose to remove > >> all the remaining 43 entries with the same status as currently listed: > >> Needs review: 26. Waiting on Author: 11. Ready for Committer: 6. > > So, seeing nothing happening I have done the above, opened 2015-11 CF > and closed the current one. You seemingly moved all entries, even the ones which were waiting-on-author for a long while, over? I think we should return items on there with lot of prejudice. Otherwise we're never going to get anywhere. > > Gosh, that's a lot of stuff that didn't get reviewed. :-( > > Yep. Yea, this is probably one of the worst commitfests ever from the point of reviewer participation.
Andres Freund <andres@anarazel.de> writes:
> On 2015-10-31 00:42:54 +0100, Michael Paquier wrote:
>> On Fri, Oct 30, 2015 at 2:02 PM, Robert Haas wrote:
>>> Gosh, that's a lot of stuff that didn't get reviewed. :-(
>> Yep.
> Yea, this is probably one of the worst commitfests ever from the point
> of reviewer participation.
FWIW, I'm expecting to be rather less AWOL for upcoming 'fests than
I have been for the last year or so. I don't think I can fix this
problem by myself, though.
regards, tom lane
On Sat, Oct 31, 2015 at 12:55 AM, Andres Freund <andres@anarazel.de> wrote: > On 2015-10-31 00:42:54 +0100, Michael Paquier wrote: >> On Fri, Oct 30, 2015 at 2:02 PM, Robert Haas wrote: >> > On Fri, Oct 30, 2015 at 10:47 AM, Michael Paquier wrote: >> >> On Thu, Oct 22, 2015 at 9:21 AM, Andres Freund wrote: >> >>>> Among the five patches marked as ready for committer, one is a bug fix >> >>>> that should be back-patched (ahem). Shouldn't we move on with those >> >>>> entries first? >> >>> >> >>> I think at this point we essentially can just move all entries to the >> >>> next. Will do that, and note down which patches haven't gotten any real >> >>> review. >> >> >> >> We are close to the end of the month. Should I move on to do the >> >> vacuuming or are you planning to do it? At this stage, to be fair with >> >> people whose patches are in "waiting on author" state and because >> >> there is not much time until the next CF begins, I propose to remove >> >> all the remaining 43 entries with the same status as currently listed: >> >> Needs review: 26. Waiting on Author: 11. Ready for Committer: 6. >> >> So, seeing nothing happening I have done the above, opened 2015-11 CF >> and closed the current one. > > You seemingly moved all entries, even the ones which were > waiting-on-author for a long while, over? I think we should return items > on there with lot of prejudice. Otherwise we're never going to get > anywhere. I know. We should normally begin the cleanup activity far earlier IMO, like at the end of the commit fest month to give patch authors a couple of weeks to rework what they have if they would like to resend something for the next commit fest. At this stage this seems a little bit too abrupt to just return with feedback patches without notice, this gives patch authors no room to submit new patches, assuming that authors were waiting for the patch to be marked as returned with feedback to move on to a new approach suggested by the reviewers. -- Michael
On Sat, Oct 31, 2015 at 6:50 AM, Michael Paquier <michael.paquier@gmail.com> wrote: > I know. We should normally begin the cleanup activity far earlier IMO, > like at the end of the commit fest month to give patch authors a > couple of weeks to rework what they have if they would like to resend > something for the next commit fest. At this stage this seems a little > bit too abrupt to just return with feedback patches without notice, > this gives patch authors no room to submit new patches, assuming that > authors were waiting for the patch to be marked as returned with > feedback to move on to a new approach suggested by the reviewers. +1. FWIW, I'm willing to review some patches for this CommitFest, but if the committers have to do first-round review as well as committer-review of every patch in the CommitFest, this is going to be long, ugly, and painful. We need to have a substantial pool of non-committers involved in the review process so that at least some of the work gets spread out. Expecting the 6-10 reasonably active committers to handle all the review work for 50-100 patches is a fail. This is not directed at you personally, Michael; you've done a ton of review. Unfortunately, you've been one of only a few. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
On 2015-10-31 06:50:09 +0100, Michael Paquier wrote: > On Sat, Oct 31, 2015 at 12:55 AM, Andres Freund <andres@anarazel.de> wrote: > > You seemingly moved all entries, even the ones which were > > waiting-on-author for a long while, over? I think we should return items > > on there with lot of prejudice. Otherwise we're never going to get > > anywhere. > > I know. We should normally begin the cleanup activity far earlier IMO, > like at the end of the commit fest month to give patch authors a > couple of weeks to rework what they have if they would like to resend > something for the next commit fest. I don't buy this. Patches that have been marked as Waiting-on-Author for weeks already had the chance to be updated.
On Sat, Oct 31, 2015 at 08:03:59AM +0100, Robert Haas wrote: > +1. FWIW, I'm willing to review some patches for this CommitFest, but > if the committers have to do first-round review as well as > committer-review of every patch in the CommitFest, this is going to be > long, ugly, and painful. We need to have a substantial pool of > non-committers involved in the review process so that at least some of > the work gets spread out. As a non-committer, let me offer my thoughts. First, I would ask that every patch include a commit id that the patch was generated against. For example, I was looking at the "group command option for psql" patch. I created a branch off of master, but the patch doesn't apply cleanly. On further investigation, it looks like Adam Brightwell noted this on September 21, but the patch hasn't been updated. If I knew which commit id the patch was created against, I could create a branch from there and test the patch, but without, I need to figure that out which is more work, or I need to re-create the patch, which is also more work. Second, it would be convenient if there were a make target that would set up a test environment. Effectively do what the 'make check' does, but don't run the tests and leave the database up. It should probably drop you into a shell that has the paths set up as well. Another target should be available to shut it down. So, what would be cool, and make testing easier is if I could do a 'git checkout -b feature abcdef' (or whatever the commit id is and branch name you want to use) Then from there a make make check make testrig <whatever tests you want to do> make testshutdown These two would go a long way to making the process of actually doing a review easier. Back to the patch in question, so Mr Brightwell noted that the patch doesn't apply against master. Should someone then mark the patch as waiting on author? Is failing to apply against master a 'waiting on author' cause? Is the answer different if the patch author has supplied a commit id that the patch was generated from? There was then some further discussion on the interface, and what to do with startup files, and nothing was really decided, and then the thread petered out. This potential reviewer is then left with the conclusion that this patch really can't be reviewed, and it's not sure if it's even wanted as is anyway. So I move on to something else. There was a bunch of discussion by a bunch of committers, and no-one updated the status of the patch in the commitfest, and there's no clear guidelines on what the status should be in this case. If I were needing to decide, I would say that the patch should either be marked as "Waiting on Author" on the grounds that the patch doesn't currently apply, or "Returned with feedback" on the grounds that there was unaddressed feedback on the details of the patch, and it was noted as a "proof of concept" when submitted anyway. But I'm unwilling to just change it to that without more clear definitions of the meaning of each status. A link to definitions and when the status should be changed would help. What is "ready for committer" anyway? It's clearly not "a committer will apply the patch", since things sit in that status without being committed. If I think the patch is good and should be applied, do I mark it as ready for committer, and then later a committer will also review the patch? If so, is doing anything other than the sanity checks, and possibly offering an opinion, on the patch even useful? -- nw
Nathan Wagner <nw+pg@hydaspes.if.org> writes:
> Second, it would be convenient if there were a make target that would
> set up a test environment. Effectively do what the 'make check' does,
> but don't run the tests and leave the database up. It should probably
> drop you into a shell that has the paths set up as well. Another
> target should be available to shut it down.
As far as that goes, I don't think it's really the makefiles' place to
establish a manual-testing convention. What I do, and what I think
most other longtimers do, is create test installations in nondefault
places.
It goes roughly like this:
./configure --with-pgport=5495 --prefix=/home/postgres/version95 ...
make -j8 -s
make -s install
export PATH="/home/postgres/version95/bin:$PATH"
export PGDATA=/home/postgres/version95/data
initdb
# optionally, adjust $PGDATA/postgresql.conf
pg_ctl start
make installcheck # optional
psql # and do whatever manual testing you want
to clean up:
pg_ctl stop
rm -rf /home/postgres/version95
I make a point of keeping around a test installation like this for
each supported PG branch, which is why I put major version numbers
into the installation paths and use per-version port numbers (so
that all these postmasters can be alive concurrently). For one-off
testing against a modified version of HEAD you probably would not
want to bother with that; you just need to pick an installation
location that won't clobber your "real" installation, and not use
the standard PGPORT number.
You could imagine putting something into the standard makefiles
that did some subset of this, but I think it would be too rigid
to be useful. As an example, what if you wanted to compare the
behaviors of both unmodified HEAD and your patched code? It's
not very hard to set up two temporary installations along the
lines of the recipe I've just given, but I can't see the makefiles
handling that.
regards, tom lane
On Sat, Oct 31, 2015 at 12:08:58PM -0400, Tom Lane wrote: > Nathan Wagner <nw+pg@hydaspes.if.org> writes: > > Second, it would be convenient if there were a make target that would > > set up a test environment. Effectively do what the 'make check' does, > > but don't run the tests and leave the database up. It should probably > > drop you into a shell that has the paths set up as well. Another > > target should be available to shut it down. > > As far as that goes, I don't think it's really the makefiles' place to > establish a manual-testing convention. What I do, and what I think > most other longtimers do, is create test installations in nondefault > places. [snip description on how to set this up ] > You could imagine putting something into the standard makefiles > that did some subset of this, but I think it would be too rigid > to be useful. I think it would be very useful to just be able to tell the system "fire this up for me so I can test it". I don't think it needs to handle every possible testing scenario, just making it easier to leave up the test postmaster from make check would be very useful, at least to me. > As an example, what if you wanted to compare the behaviors of both > unmodified HEAD and your patched code? It's not very hard to set up > two temporary installations along the lines of the recipe I've just > given, but I can't see the makefiles handling that. They could pick up make or environment variables. We already do that for psql. Something like PGPORT=5495 PGPATH=~/pg95 make startit or some such. I'm not actually proposing this, I'm just noting how the makefiles could handle it fairly easily. All I'd really like is a way to leave the database used for 'make check' running so I can do any additional poking around by hand that I might want to do more easily. -- nw
[ separate response for questions that are about process not mechanics ]
Nathan Wagner <nw+pg@hydaspes.if.org> writes:
> Back to the patch in question, so Mr Brightwell noted that the patch
> doesn't apply against master. Should someone then mark the patch as
> waiting on author? Is failing to apply against master a 'waiting on
> author' cause?
I would say so, if the fix isn't obvious. ("Obvious" might be different
for different people, but if you wanted to review the patch and couldn't
make it apply, I think it's fair to put the ball into the author's court.)
> Is the answer different if the patch author has supplied
> a commit id that the patch was generated from?
While that would give reviewers a chance to play with the original form
of the patch, it's still gonna have to get updated at some point if it's
ever to get committed. And if there are conflicting patches that already
went in, there's a substantial risk that those patches may have broken
the new patch in some non-obvious way. So I'm not sure that testing an
already-obsolete patch is all that valuable. This is going to be
situation-dependent in many cases, but there is certainly nothing wrong
with just booting the patch back to the author when in doubt.
> What is "ready for committer" anyway? It's clearly not "a committer
> will apply the patch", since things sit in that status without being
> committed. If I think the patch is good and should be applied, do I
> mark it as ready for committer, and then later a committer will also
> review the patch? If so, is doing anything other than the sanity
> checks, and possibly offering an opinion, on the patch even useful?
Our expectation is that committers will review whatever they commit,
because ultimately what goes in is their responsibility. But that doesn't
make reviews by non-committers useless. In the first place, you might
spot something the committer would have missed (nobody's perfect, so more
eyeballs are always better than fewer). Even if the committer would have
found it, committer cycles are a limited resource around here, so it's
better if a bug gets found before it gets to the committer's review.
That's why we'd like at least one non-committer to have signed off on a
patch before a committer picks it up.
And there's a meta-agenda behind the whole CF process: getting more people
to study the Postgres code, even if it's a byproduct of examining patches
that ultimately get rejected, improves the community's abilities overall.
The way you get to having committer-grade skills is to spend a lot of time
reading and modifying the PG code. Reviewing other peoples' patches is
one means to that end, especially if you spend time thinking about how
they did whatever they are trying to do and whether there are other better
ways to do it. So ultimately we hope to get more committers out of the
process as well.
regards, tom lane
On 10/31/15 12:42 AM, Michael Paquier wrote: > So, seeing nothing happening I have done the above, opened 2015-11 CF > and closed the current one. Are we doing these in an Australian time zone now? It was quite unpleasant to find that the 2015-11 is "in progress" already and two of my patches will not be in there. AFAIR the submission deadline used to be around UTC midnight of the first day of the month the CF nominally begins on. .m
On Sat, Oct 31, 2015 at 03:43:13PM +0000, Nathan Wagner wrote: > There was then some further discussion on the interface, and what to do > with startup files, and nothing was really decided, and then the thread > petered out. This potential reviewer is then left with the conclusion > that this patch really can't be reviewed, and it's not sure if it's even > wanted as is anyway. So I move on to something else. There was a bunch > of discussion by a bunch of committers, and no-one updated the status of > the patch in the commitfest, and there's no clear guidelines on what the > status should be in this case. There are no clear guidelines, agreed. > If I were needing to decide, I would say that the patch should either be > marked as "Waiting on Author" on the grounds that the patch doesn't > currently apply, or "Returned with feedback" on the grounds that there > was unaddressed feedback on the details of the patch, and it was noted > as a "proof of concept" when submitted anyway. But I'm unwilling to > just change it to that without more clear definitions of the meaning of > each status. A link to definitions and when the status should be > changed would help. By default, the author is responsible for driving the thread to a conclusion. If a patch still has "Needs review" status after an inconclusive discussion petered out, I would set it to "Waiting on Author". Changing directly from "Needs review" to "Returned with feedback" is unusual; folks do so when a review calls for extensive edits approaching a full rewrite. More often, a patch first spends some time in "Waiting on Author". > What is "ready for committer" anyway? It's clearly not "a committer > will apply the patch", since things sit in that status without being > committed. "Ready for Committer" is the reviewer saying, "Based on my review, I would have committed this if I had been the patch's committer." You can further qualify that statement in your review message, e.g. "I'm marking this RfC, but I'm not sure about the spinlock usage in function FooBar()." > If I think the patch is good and should be applied, do I > mark it as ready for committer, and then later a committer will also > review the patch? If so, is doing anything other than the sanity > checks, and possibly offering an opinion, on the patch even useful? Yes and yes; see Tom's reply for details. Thanks, nm
On Sun, Nov 1, 2015 at 1:53 AM, Marko Tiikkaja <marko@joh.to> wrote: > On 10/31/15 12:42 AM, Michael Paquier wrote: >> >> So, seeing nothing happening I have done the above, opened 2015-11 CF >> and closed the current one. > > > Are we doing these in an Australian time zone now? It was quite unpleasant > to find that the 2015-11 is "in progress" already and two of my patches will > not be in there. AFAIR the submission deadline used to be around UTC > midnight of the first day of the month the CF nominally begins on. Er, well. Sorry for that... I did all this stuff on Friday evening before leaving back for Japan with not much time on the table. I have switched the CF back to an open status for now. And I'll switch it back to in-progress in 24 hours. If there are patches you would like attached to the CF app don't hesitate to ping me. -- Michael
On 11/1/15 11:36 AM, Michael Paquier wrote: > On Sun, Nov 1, 2015 at 1:53 AM, Marko Tiikkaja <marko@joh.to> wrote: >> Are we doing these in an Australian time zone now? It was quite unpleasant >> to find that the 2015-11 is "in progress" already and two of my patches will >> not be in there. AFAIR the submission deadline used to be around UTC >> midnight of the first day of the month the CF nominally begins on. > > Er, well. Sorry for that... I did all this stuff on Friday evening > before leaving back for Japan with not much time on the table. I have > switched the CF back to an open status for now. And I'll switch it > back to in-progress in 24 hours. If there are patches you would like > attached to the CF app don't hesitate to ping me. Thanks. I managed to add the two patches just fine now. .m
On 10/31/15 11:19 AM, Nathan Wagner wrote: >> >You could imagine putting something into the standard makefiles >> >that did some subset of this, but I think it would be too rigid >> >to be useful. > I think it would be very useful to just be able to tell the system "fire > this up for me so I can test it". I don't think it needs to handle > every possible testing scenario, just making it easier to leave up the > test postmaster from make check would be very useful, at least to me. I've wished that the cluster setup and teardown behavior of pg_regress was available outside of pg_regress itself. Would that mostly suffice for what you're looking for? -- Jim Nasby, Data Architect, Blue Treble Consulting, Austin TX Experts in Analytics, Data Architecture and PostgreSQL Data in Trouble? Get it in Treble! http://BlueTreble.com
Jim Nasby <Jim.Nasby@BlueTreble.com> writes:
> On 10/31/15 11:19 AM, Nathan Wagner wrote:
>> I think it would be very useful to just be able to tell the system "fire
>> this up for me so I can test it". I don't think it needs to handle
>> every possible testing scenario, just making it easier to leave up the
>> test postmaster from make check would be very useful, at least to me.
> I've wished that the cluster setup and teardown behavior of pg_regress
> was available outside of pg_regress itself. Would that mostly suffice
> for what you're looking for?
I should think not. pg_regress is not merely not encouraging of outside
connections to the started postmaster; if such is even possible, it's
likely to be regarded as a security bug. What Nathan is looking for is
arguably useful, but I do not think pg_regress should be expected to
support it. It needs to be a different tool, with different security
parameters.
regards, tom lane
On Sun, Nov 1, 2015 at 7:36 PM, Michael Paquier <michael.paquier@gmail.com> wrote: > On Sun, Nov 1, 2015 at 1:53 AM, Marko Tiikkaja <marko@joh.to> wrote: >> On 10/31/15 12:42 AM, Michael Paquier wrote: >>> >>> So, seeing nothing happening I have done the above, opened 2015-11 CF >>> and closed the current one. >> >> >> Are we doing these in an Australian time zone now? It was quite unpleasant >> to find that the 2015-11 is "in progress" already and two of my patches will >> not be in there. AFAIR the submission deadline used to be around UTC >> midnight of the first day of the month the CF nominally begins on. > > Er, well. Sorry for that... I did all this stuff on Friday evening > before leaving back for Japan with not much time on the table. I have > switched the CF back to an open status for now. And I'll switch it > back to in-progress in 24 hours. If there are patches you would like > attached to the CF app don't hesitate to ping me. And now CF begins officially. The axe has fallen as promised 26 hours after. -- Michael
On Mon, Nov 2, 2015 at 9:35 PM, Michael Paquier wrote: > And now CF begins officially. The axe has fallen as promised 26 hours after. Seeing no volunteers around, I can take the CFM hat for November's CF. Any objections/complaints/remarks? -- Michael
On Tue, Nov 3, 2015 at 8:12 AM, Michael Paquier <michael.paquier@gmail.com> wrote: > On Mon, Nov 2, 2015 at 9:35 PM, Michael Paquier wrote: >> And now CF begins officially. The axe has fallen as promised 26 hours after. > > Seeing no volunteers around, I can take the CFM hat for November's CF. > Any objections/complaints/remarks? I think that's great. What, in your opinion, would be the most helpful thing for me to do to move things along? -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
On Wed, Nov 4, 2015 at 2:38 AM, Robert Haas <robertmhaas@gmail.com> wrote: > On Tue, Nov 3, 2015 at 8:12 AM, Michael Paquier > <michael.paquier@gmail.com> wrote: >> On Mon, Nov 2, 2015 at 9:35 PM, Michael Paquier wrote: >>> And now CF begins officially. The axe has fallen as promised 26 hours after. >> >> Seeing no volunteers around, I can take the CFM hat for November's CF. >> Any objections/complaints/remarks? > > I think that's great. What, in your opinion, would be the most > helpful thing for me to do to move things along? Hm, I does not seem to me that you are the best fit for the patches currently stated as "Ready for committer" as you did not much participate in the threads related to them, except perhaps "SQL function to report log message". I would think that Heikki is the one who should take care of wrapping the patch for libpq and OOM, and the stuff of pg_rewind. Andres would be suited for the bgwriter for XLOG_RUNNING_XACTS. Finally the in-core regression test suite is a bit more general... Perhaps Noah who worked on improving things in this area in light of CVE-2014-0067, but I won't force any committer on it either, I can understand that we're all busy. So at this stage, I guess the main issue is to reduce the stack of patches waiting for reviews with some first-level reviews. But this applies to everybody here. -- Michael
Hello, >> +1. FWIW, I'm willing to review some patches for this CommitFest, but >> if the committers have to do first-round review as well as >> committer-review of every patch in the CommitFest, this is going to be >> long, ugly, and painful. We need to have a substantial pool of >> non-committers involved in the review process so that at least some of >> the work gets spread out. > > As a non-committer, let me offer my thoughts. > > First, I would ask that every patch include a commit id that the patch > was generated against. For example, I was looking at the "group command > option for psql" patch. I created a branch off of master, but the patch > doesn't apply cleanly. On further investigation, it looks like Adam > Brightwell noted this on September 21, but the patch hasn't been > updated. If I knew which commit id the patch was created against, I > could create a branch from there and test the patch, but without, I need > to figure that out which is more work, or I need to re-create the patch, > which is also more work. + 1 > Second, it would be convenient if there were a make target that would > set up a test environment. Effectively do what the 'make check' does, > but don't run the tests and leave the database up. It should probably > drop you into a shell that has the paths set up as well. Another > target should be available to shut it down. So, what would be cool, > and make testing easier is if I could do a 'git checkout -b feature > abcdef' (or whatever the commit id is and branch name you want to use) > Then from there a > > make > make check > make testrig > <whatever tests you want to do> > make testshutdown > > These two would go a long way to making the process of actually > doing a review easier. From my point of view it is very hard to review a patch without having much time. My C knowledge is very very basic. I read many patches just to get better with this, but as you (not) noticed, there is rarely feedback from me. Often it is unclear what to test. The threads about the features are very long and mostly very technical. While interesting for a good C-programmer this is not helpful for an end user. When there is a patch i am missing: - a short description how to set up an test installation (just a link to the wiki would be very helpful) - what is the patch proposed to do - what should it not do - how can it be tested - how can the updated documentation - if existent - generated and used Often terms, configuration options or syntax changes between the patches. This is needed and very good - but currently requires me to read the complete thread, which has many many information i do not understand because of the missing inside. I believe that we need to lower the barrier for testing. This would require another step of work. But this step is not necessarily done by the author itself. I am under the impression that there are many readers at the mailing list willing but unable to participate. This could be a "grunt-work" task like in this discussion about the bugtracker. I could even imagine to set up an open for everyone test-instance of HEAD where users are allowed to test like the wanted. Than the barrier is reduced to "connect to PostgreSQL and execute SQL". Greetings, Torsten
On 5 November 2015 at 15:59, Torsten Zühlsdorff <mailinglists@toco-domains.de> wrote: > Hello, > >>> +1. FWIW, I'm willing to review some patches for this CommitFest, but >>> if the committers have to do first-round review as well as >>> committer-review of every patch in the CommitFest, this is going to be >>> long, ugly, and painful. We need to have a substantial pool of >>> non-committers involved in the review process so that at least some of >>> the work gets spread out. >> >> >> As a non-committer, let me offer my thoughts. >> >> First, I would ask that every patch include a commit id that the patch >> was generated against. > > + 1 Including a git ref in the commitfest entry helps a lot with that. So does using "git format-patch" when generating patches, now that everyone seems to have pretty much stopped caring about context diffs. >> Second, it would be convenient if there were a make target that would >> set up a test environment. Yes, a "make check NOSTOP=1" would be handy. That said, it's not hard to "make temp-install" then "PGPORT=whatever make installcheck" > From my point of view it is very hard to review a patch without having much > time. That's the eternal problem with patch review. I know the feeling. > My C knowledge is very very basic. I read many patches just to get > better with this, but as you (not) noticed, there is rarely feedback from > me. Reading C is useful for learning, but learned massively faster when I started writing extensions, etc. Then my reading tended to be more directed too, and with better retention. > Often it is unclear what to test. The threads about the features are very > long and mostly very technical. While interesting for a good C-programmer > this is not helpful for an end user. When there is a patch i am missing: > - a short description how to set up an test installation (just a link to the > wiki would be very helpful) Wiki links for patches would be a huge plus. The CF app "Links" section can be used for that, and should be more than it is. Peter Geoghegan did this with the insert ... on commit update patch, to useful effect. I try to include enough documentation directly in the patch - either in the commit message(s) from git format-patch, or in in-patch README text etc - for this not to be necessary. But for bigger patches it's hard to avoid, and I'd love it if more people did it. > Often terms, configuration options or syntax changes between the patches. > This is needed and very good - but currently requires me to read the > complete thread, which has many many information i do not understand because > of the missing inside. Especially when threads reference things that were discussed months or years prior! > I believe that we need to lower the barrier for testing. While I agree, I'd also like to note that formulaic testing is often of limited utility. Good testing still requires a significant investment of time and effort to understand the changes made by a patch, which areas need focused attention, think about corner cases, etc. "make check passes" doesn't really tell anyone that much. > I could even imagine to set up an open for everyone test-instance of HEAD > where users are allowed to test like the wanted. Than the barrier is reduced > to "connect to PostgreSQL and execute SQL". Gee, that'd be fun to host ;) More seriously, it's not HEAD that's of that much interest, it's HEAD + [some patch or set of patches]. There are systems that can pull in patchsets, build a project, and run it. But for something like PostgreSQL it'd be pretty hard to offer wide public access, given the trivial potential for abuse. -- Craig Ringer http://www.2ndQuadrant.com/PostgreSQL Development, 24x7 Support, Training & Services
On 05.11.2015 13:49, Craig Ringer wrote: >> I believe that we need to lower the barrier for testing. > > While I agree, I'd also like to note that formulaic testing is often > of limited utility. Good testing still requires a significant > investment of time and effort to understand the changes made by a > patch, which areas need focused attention, think about corner cases, > etc. Yes, you are right. But a limited test is better than no test at all. But of course not enough. For me it is easy to check comments or sql commands, but not the c code. But with lower barriers it would be easier to test 2 of the 3 mentioned items. At the moment its often none, because its hard. > "make check passes" doesn't really tell anyone that much. > >> I could even imagine to set up an open for everyone test-instance of HEAD >> where users are allowed to test like the wanted. Than the barrier is reduced >> to "connect to PostgreSQL and execute SQL". > > Gee, that'd be fun to host ;)> > More seriously, it's not HEAD that's of that much interest, it's HEAD > + [some patch or set of patches]. > > There are systems that can pull in patchsets, build a project, and run > it. But for something like PostgreSQL it'd be pretty hard to offer > wide public access, given the trivial potential for abuse. Yes, but i would do this. Creating a FreeBSD Jail which is reset regularly is no great deal and very secure. My bigger problem is the lack of IPv4 addresses. At the moment i am limited to IPv6 only hosts. Greetings, Torsten