Обсуждение: commitfest 2016-11 status summary

On Tue, Nov 1, 2016 at 3:48 PM, Haribabu Kommi <kommi.haribabu@gmail.com> wrote:
> There are plenty of patches that are in "ready for committer" state,
> committers please have a look at those patches and give some conclusion
> on them.

Yes, we really need some more committer attention on a lot of these
patches.  I've been trying to do what I can to whittle that list down,
but it's long and contains many complex patches, as well as quite a
few patches in areas that I don't really understand.  For example, I
have no idea whether these things are good ideas, because I don't know
Windows:

https://commitfest.postgresql.org/11/604/ - pgwin32_is_service not
checking if SECURITY_SERVICE_SID is disabled
https://commitfest.postgresql.org/11/620/ - Updating Windows
environment variables

Any chance that some Windows-savvy committer can take a look at those?

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On Fri, Nov 4, 2016 at 12:36 AM, Robert Haas <robertmhaas@gmail.com> wrote:
> On Tue, Nov 1, 2016 at 3:48 PM, Haribabu Kommi <kommi.haribabu@gmail.com> wrote:
> https://commitfest.postgresql.org/11/604/ - pgwin32_is_service not
> checking if SECURITY_SERVICE_SID is disabled

This is quite a particular fix in a particular context. This is as
well waiting some input from the reporter to be sure that the patch
proposed is fixing what his case. The patch should fix it, but that's
a matter to be sure now.

> https://commitfest.postgresql.org/11/620/ - Updating Windows
> environment variables

This one is not that particular. Any committer input is welcome. All
the patches proposed should be HEAD-only considering the lack of
complaints across the years.
-- 
Michael

On Sun, Nov 6, 2016 at 3:33 AM, Michael Paquier
<michael.paquier@gmail.com> wrote:
> On Fri, Nov 4, 2016 at 12:36 AM, Robert Haas <robertmhaas@gmail.com> wrote:
>> On Tue, Nov 1, 2016 at 3:48 PM, Haribabu Kommi <kommi.haribabu@gmail.com> wrote:
>> https://commitfest.postgresql.org/11/604/ - pgwin32_is_service not
>> checking if SECURITY_SERVICE_SID is disabled
>
> This is quite a particular fix in a particular context. This is as
> well waiting some input from the reporter to be sure that the patch
> proposed is fixing what his case. The patch should fix it, but that's
> a matter to be sure now.

Well, then it should be marked "Waiting on Author" or "Needs Review",
not "Ready for Committer".

Or maybe just "Returned with Feedback", if the reporter hasn't poked
his head up for a few weeks.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On Wed, Nov 9, 2016 at 10:53 AM, Robert Haas <robertmhaas@gmail.com> wrote:
> Well, then it should be marked "Waiting on Author" or "Needs Review",
> not "Ready for Committer".
>
> Or maybe just "Returned with Feedback", if the reporter hasn't poked
> his head up for a few weeks.

Since yesterday with the help of Tsunakawa-san, we found a simple way
to test things by patching pg_ctl.c so "Ready for Committer"
definitely applies now. This has allowed us to draft patches that fix
the original issue as well as another issue we bumped on the way:
Postgres being started by the Local System user does not ensure that
it is a service. The instance may have been started by another service
expecting Postgres to report to stderr.

There is a refactoring patch that simplifies the code of
win32security.c for HEAD, as well as a simple back-patchable fix for
other branches.
-- 
Michael

On Mon, Dec 5, 2016 at 2:50 PM, Haribabu Kommi <kommi.haribabu@gmail.com> wrote:
> Micheal,  I need your help in closing the commitfest.

And done. Thanks for doing this work! I was ready to use my CF vacuum
cleaner as need be, but that does not seem to be needed.

> I closed the commitfest using the following assumptions.
>
> Moved to next CF with needs review.
> 1. patch doesn't receive any full review in the commitfest
> 2. Patch received feedback at the end of commitfest.
>
> Moved to next CF with waiting on author:
> 1. Patch doesn't apply to HEAD, but didn't receive any feedback.
>
> Returned with feedback:
> 1. Patch received feedback, but author hasn't responded yet.
> 2. Author is expected to share an updated patch.
>
> Rejected:
> 1. Any -1 from committer to the approach of the patch

All that sounds fair to me.

> May be these assumptions needs to be updated, as this is the first
> time as CFM for me.
>
> I definitely may missed judging the current state of the patch. Please feel
> free to update the actual status.

That's definitely up to the authors and reviewers to double-check what
has been done and correct anything they think is not. And far as I can
see you have done an admirable work in keeping it on tracks, so huge
+1 from me.
-- 
Michael

On Mon, Dec 5, 2016 at 11:56 AM, Michael Paquier
<michael.paquier@gmail.com> wrote:
> On Mon, Dec 5, 2016 at 2:50 PM, Haribabu Kommi <kommi.haribabu@gmail.com> wrote:
>> I definitely may missed judging the current state of the patch. Please feel
>> free to update the actual status.
>
> That's definitely up to the authors and reviewers to double-check what
> has been done and correct anything they think is not. And far as I can
> see you have done an admirable work in keeping it on tracks, so huge
> +1 from me.
>

I also think Hari has done a good job in wrapping up this commitfest.
However, we might want to consider sending one consolidated mail for
the patches "moved to next CF" or "Returned with Feedback" or
"Rejected" rather than sending independent mails.  That might save
some traffic on the list.

-- 
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com

* Amit Kapila (amit.kapila16@gmail.com) wrote:
> On Mon, Dec 5, 2016 at 11:56 AM, Michael Paquier
> <michael.paquier@gmail.com> wrote:
> > On Mon, Dec 5, 2016 at 2:50 PM, Haribabu Kommi <kommi.haribabu@gmail.com> wrote:
> >> I definitely may missed judging the current state of the patch. Please feel
> >> free to update the actual status.
> >
> > That's definitely up to the authors and reviewers to double-check what
> > has been done and correct anything they think is not. And far as I can
> > see you have done an admirable work in keeping it on tracks, so huge
> > +1 from me.
>
> I also think Hari has done a good job in wrapping up this commitfest.
> However, we might want to consider sending one consolidated mail for
> the patches "moved to next CF" or "Returned with Feedback" or
> "Rejected" rather than sending independent mails.  That might save
> some traffic on the list.

Agreed.  The status change is noted in the CF app, it doesn't need to be
included in the thread.  Further, having the info about what happened to
each patch in one consolidated email would be a lot easier for people to
review than an email on every thread.

Thanks!

Stephen

On Mon, Dec 5, 2016 at 12:50 AM, Haribabu Kommi
<kommi.haribabu@gmail.com> wrote:
> Moved to next CF with needs review.
> 1. patch doesn't receive any full review in the commitfest
> 2. Patch received feedback at the end of commitfest.

The number of patches that got this treatment seems quite large.  If
the people who are submitting patches are also reviewing other patches
of comparable difficulty, this really shouldn't be happening.
Something isn't working well, here.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On 12/5/16 3:27 PM, Robert Haas wrote:
> On Mon, Dec 5, 2016 at 12:50 AM, Haribabu Kommi
> <kommi.haribabu@gmail.com> wrote:
>> Moved to next CF with needs review.
>> 1. patch doesn't receive any full review in the commitfest
>> 2. Patch received feedback at the end of commitfest.
>
> The number of patches that got this treatment seems quite large.  If
> the people who are submitting patches are also reviewing other patches
> of comparable difficulty, this really shouldn't be happening.
> Something isn't working well, here.

Yeah. :( (and I'm embarrassed to admit I'm part of the problem :()

I think it would be good to have a different status for patches that got 
bumped to the next CF due to insufficient review. It's already unfair to 
authors, but at least if there was a separate status those patches could 
be prioritized next time around. It would also allow tracking how big a 
problem that is from one CF to the next.
-- 
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
855-TREBLE2 (855-873-2532)