Re: Feature freeze progress report

On Mon, 2007-30-04 at 08:56 -0300, Heikki Linnakangaspgsql wrote:
> Date: Mon, 30 Apr 2007 09:18:36 +0100
> From: Heikki Linnakangas <heikki(at)enterprisedb(dot)com>
> To: Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>
> Cc: Dave Page <dpage(at)postgresql(dot)org>, Simon Riggs
> <simon(at)2ndquadrant(dot)com>,
> Bruce Momjian <bruce(at)momjian(dot)us>,
> PostgreSQL-development <pgsql-hackers(at)postgresql(dot)org>
> Subject: Re: Feature freeze progress report
> Message-ID: <4635A65C(dot)70005(at)enterprisedb(dot)com>

> If we had a 1-2 lines status blurp attached to each patch in the
> queue,
> like "waiting for review", "author is fixing issue XX", etc., that
> might
> help. Bruce would need to do that if we keep the current patch queue
> system unmodified otherwise, or we'd need to switch to something else.

Would it be possible to also automatically determine some sort of
bit-rot status? What I had in mind was an automated process that would
apply each patch to HEAD on a daily basis and report whether the patch
still applies cleanly and still allows all regression tests to pass on
at least one platform. If and when the result of these tests changes
from pass to fail, the patch submitter would be automatically
notified.

The patch status could then also show the last time at which the patch
applied cleanly, and the last time that regression tests ran
successfully.

__
Marc

Marc Munro wrote:
> On Mon, 2007-30-04 at 08:56 -0300, Heikki Linnakangaspgsql wrote:
>
>> Date: Mon, 30 Apr 2007 09:18:36 +0100
>> From: Heikki Linnakangas <heikki(at)enterprisedb(dot)com>
>> To: Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>
>> Cc: Dave Page <dpage(at)postgresql(dot)org>, Simon Riggs
>> <simon(at)2ndquadrant(dot)com>,
>> Bruce Momjian <bruce(at)momjian(dot)us>,
>> PostgreSQL-development <pgsql-hackers(at)postgresql(dot)org>
>> Subject: Re: Feature freeze progress report
>> Message-ID: <4635A65C(dot)70005(at)enterprisedb(dot)com>
>>
>
>
>> If we had a 1-2 lines status blurp attached to each patch in the
>> queue,
>> like "waiting for review", "author is fixing issue XX", etc., that
>> might
>> help. Bruce would need to do that if we keep the current patch queue
>> system unmodified otherwise, or we'd need to switch to something else.
>>
>
> Would it be possible to also automatically determine some sort of
> bit-rot status? What I had in mind was an automated process that would
> apply each patch to HEAD on a daily basis and report whether the patch
> still applies cleanly and still allows all regression tests to pass on
> at least one platform. If and when the result of these tests changes
> from pass to fail, the patch submitter would be automatically
> notified.
>
> The patch status could then also show the last time at which the patch
> applied cleanly, and the last time that regression tests ran
> successfully.
>
>
>

This or something similar has been discussed in the past w.r.t. the
buildfarm. One major problem is that most sane system owners won't want
to apply, compile and run an arbitrary patch. It could well have an
intended or unintended trojan horse, for example. So you'd need some
level of sanity checking to be done by some trusted person even to get
it to this stage.

cheers

andrew

marc(at)bloodnok(dot)com (Marc Munro) writes:
> On Mon, 2007-30-04 at 08:56 -0300, Heikki Linnakangaspgsql wrote:
>> Date: Mon, 30 Apr 2007 09:18:36 +0100
>> From: Heikki Linnakangas <heikki(at)enterprisedb(dot)com>
>> To: Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>
>> Cc: Dave Page <dpage(at)postgresql(dot)org>, Simon Riggs
>> <simon(at)2ndquadrant(dot)com>,
>> Bruce Momjian <bruce(at)momjian(dot)us>,
>> PostgreSQL-development <pgsql-hackers(at)postgresql(dot)org>
>> Subject: Re: Feature freeze progress report
>> Message-ID: <4635A65C(dot)70005(at)enterprisedb(dot)com>
>
>> If we had a 1-2 lines status blurp attached to each patch in the
>> queue,
>> like "waiting for review", "author is fixing issue XX", etc., that
>> might
>> help. Bruce would need to do that if we keep the current patch queue
>> system unmodified otherwise, or we'd need to switch to something else.
>
> Would it be possible to also automatically determine some sort of
> bit-rot status? What I had in mind was an automated process that would
> apply each patch to HEAD on a daily basis and report whether the patch
> still applies cleanly and still allows all regression tests to pass on
> at least one platform. If and when the result of these tests changes
> from pass to fail, the patch submitter would be automatically
> notified.
>
> The patch status could then also show the last time at which the patch
> applied cleanly, and the last time that regression tests ran
> successfully.

Hmm. That would be an interesting extension to the build farm.

If only the timing were right for us to get a GSoC person or something
such to add such functionality...
--
let name="cbbrowne" and tld="acm.org" in String.concat "@" [name;tld];;
http://linuxdatabases.info/info/emacs.html
"Lisp is an eternal thought in the mind of God."
--Crassus, mutatis mutandi

I believe the suggestion was to have an automated process that only ran
on known, sane patches. I don't think he was suggesting a mechanism for
the great unwashed masses to dump arbitrary code into and have it
applied in the buildfarm. You'd have an inventory of patches (you could
use a hash to ensure they hadn't changed just before they ar
automatically applied) that were verified as good, and the system would
apply them to HEAD periodically.

Even if the patch inventory wasn't kept right up to date, this system
could potentially help many regression issues or bugs to surface sooner,
and as it would require zero work once set up besides system maintenance
(which should be low if it is implemented in a reasonably intelligent
manner) I feel that it is a great idea. Generally, I am all for
automating mundane tasks as much as possible.

Regards,
- Naz.

Andrew Dunstan wrote:
> Marc Munro wrote:
>> On Mon, 2007-30-04 at 08:56 -0300, Heikki Linnakangaspgsql wrote:
>>
>>> Date: Mon, 30 Apr 2007 09:18:36 +0100
>>> From: Heikki Linnakangas <heikki(at)enterprisedb(dot)com>
>>> To: Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>
>>> Cc: Dave Page <dpage(at)postgresql(dot)org>, Simon Riggs
>>> <simon(at)2ndquadrant(dot)com>, Bruce Momjian <bruce(at)momjian(dot)us>,
>>> PostgreSQL-development <pgsql-hackers(at)postgresql(dot)org>
>>> Subject: Re: Feature freeze progress report
>>> Message-ID: <4635A65C(dot)70005(at)enterprisedb(dot)com>
>>>
>>
>>
>>> If we had a 1-2 lines status blurp attached to each patch in the
>>> queue, like "waiting for review", "author is fixing issue XX", etc.,
>>> that
>>> might help. Bruce would need to do that if we keep the current patch
>>> queue system unmodified otherwise, or we'd need to switch to
>>> something else.
>>>
>>
>> Would it be possible to also automatically determine some sort of
>> bit-rot status? What I had in mind was an automated process that would
>> apply each patch to HEAD on a daily basis and report whether the patch
>> still applies cleanly and still allows all regression tests to pass on
>> at least one platform. If and when the result of these tests changes
>> from pass to fail, the patch submitter would be automatically
>> notified.
>> The patch status could then also show the last time at which the patch
>> applied cleanly, and the last time that regression tests ran
>> successfully.
>>
>>
>>
>
> This or something similar has been discussed in the past w.r.t. the
> buildfarm. One major problem is that most sane system owners won't
> want to apply, compile and run an arbitrary patch. It could well have
> an intended or unintended trojan horse, for example. So you'd need
> some level of sanity checking to be done by some trusted person even
> to get it to this stage.
>
> cheers
>
> andrew
>
> ---------------------------(end of broadcast)---------------------------
> TIP 7: You can help support the PostgreSQL project by donating at
>
> http://www.postgresql.org/about/donate
>

"Naz Gassiep" <naz(at)mira(dot)net> writes:

> Even if the patch inventory wasn't kept right up to date, this system
> could potentially help many regression issues or bugs to surface sooner,

I just don't understand what this would accomplish. People run regressions
before submitting anyways. They can't run them on all architectures but bugs
that only affect some architectures are uncommon.

This seems to be merely institutionalizing having a large backlog of patches
which survive for long periods of time. But even in that situation I don't see
what it buys us. Detecting bitrot isn't terribly helpful and it doesn't help
us actually deal with the bitrot once it's happened.

--
Gregory Stark
EnterpriseDB http://www.enterprisedb.com

Naz Gassiep wrote:
> I believe the suggestion was to have an automated process that only ran
> on known, sane patches.
>

How do we know in advance of reviewing them that they are sane?

What is more, we often run into situations where patch a will require
changes in patch b, so testing them individually against CVS is not
likely to be terribly useful.

Frankly, our problems are not primarily technological. They have to do
mainly with scarcity of available time from competent reviewers. No
amount of automation will fix that.

cheers

andrew

On Tue, 2007-01-05 at 02:54 +0100, Gregory Stark wrote:
> "Naz Gassiep" <naz(at)mira(dot)net> writes:
>
> > Even if the patch inventory wasn't kept right up to date, this system
> > could potentially help many regression issues or bugs to surface sooner,
>
> I just don't understand what this would accomplish. People run regressions
> before submitting anyways. They can't run them on all architectures but bugs
> that only affect some architectures are uncommon.

The intention is to help keep patches, which may remain in the queue for
extended lengths of time, reasonably current. The mechanism aims to
help with these specific problems:

- patches accumulates bitrot and are consequently harder to apply and
understand
- the author, by the time review occurs, no longer has the details of
the patch uppermost in their mind

If the author can be automatically prodded when the patch no longer
cleanly applies, or when it suddenly breaks regression tests, they will
be able to keep the patch current, may discover bugs in it themselves
prior to review, and it will remain more fresh in their minds.

For sure, there will be classes of patch for which this mechanism
provides no benefit. For instance, where a patch contains code that is
for discussion only, or a patch that is dependant on another patch. In
these cases, the mechanism would simply note that they don't apply
cleanly, or don't pass tests, and would do nothing further. I can see
no harm here.

> This seems to be merely institutionalizing having a large backlog of patches
> which survive for long periods of time. But even in that situation I don't see
> what it buys us. Detecting bitrot isn't terribly helpful and it doesn't help
> us actually deal with the bitrot once it's happened.

I hope that it would not encourage reviewers to leave things in the
patch queue. I don't see why it would, so don't think this would
institutionalize a backlog.

I also disagree that detecting bitrot is not helpful. If I had eagerly
submitted a patch, I would definitely want to fix any bitrot that
occurred and would be thankful to be automatically informed.

And to clarify, I do not think the buildfarm should be involved in this
process.

__
Marc

Andrew Dunstan wrote:
> Naz Gassiep wrote:
>> I believe the suggestion was to have an automated process that only ran
>> on known, sane patches.
> How do we know in advance of reviewing them that they are sane?
Same way as happens now. I would assume this mechanism would only be
applied to patches that had already been approved to contrib, or some
other measure that can be used to isolate only those patches that we
*expect* to already be working. The intention of this mechanism, in my
head, is to just help us make sure that regression issues on patches get
detected sooner.
> What is more, we often run into situations where patch a will require
> changes in patch b, so testing them individually against CVS is not
> likely to be terribly useful.
Yeap, given that this proposition is for an automated system, perhaps it
could be designed to apply combinations of patches together to look for
conflicts.
> Frankly, our problems are not primarily technological. They have to do
> mainly with scarcity of available time from competent reviewers. No
> amount of automation will fix that.
I fully understand that. However I find the idea of an automated process
checking for big issues while we're all sleeping to be... sexy. I'm not
sure how difficult a system like this would be to set up but it doesn't
seem to me to be the sort of thing that requires more than a few simple
scripts. If it's not too had to set up, even if it only yields small and
rare benefits, it will have been a worthwhile exercise.

My 2c (adjusted for inflation).

Regards,
- Naz

Naz Gassiep <naz(at)mira(dot)net> writes:
> Andrew Dunstan wrote:
>> How do we know in advance of reviewing them that they are sane?

> Same way as happens now. I would assume this mechanism would only be
> applied to patches that had already been approved to contrib, or some
> other measure that can be used to isolate only those patches that we
> *expect* to already be working.

What is "approved to contrib"?

The problem here is that having reasonable certainty that a patch is
not malicious requires having gone over it in some detail; at which
point you might as well apply the thing. Or if you didn't apply it,
you bounced it for reasons that are unlikely to have anything to do
with needing more automated testing.

ISTM this idea can only work if we have a "second tier" of reviewers
who are considered good enough to vet patches as safe, but not quite
good enough to certify them as commitable. I'm not seeing a large pool
of people volunteering to hold that position --- at best it'd be a
transitory state before attaining committerdom. If you are relying
on a constant large influx of new people, you are doomed to failure
(see "Ponzi scheme" for a counterexample).

regards, tom lane

> What is "approved to contrib"?
>
> The problem here is that having reasonable certainty that a patch is
> not malicious requires having gone over it in some detail; at which
> point you might as well apply the thing. Or if you didn't apply it,
> you bounced it for reasons that are unlikely to have anything to do
> with needing more automated testing.
>
> ISTM this idea can only work if we have a "second tier" of reviewers
> who are considered good enough to vet patches as safe, but not quite
> good enough to certify them as commitable. I'm not seeing a large pool
> of people volunteering to hold that position --- at best it'd be a
> transitory state before attaining committerdom. If you are relying
> on a constant large influx of new people, you are doomed to failure
> (see "Ponzi scheme" for a counterexample).
Yep. For the record, Ponzi died in poverty, so it's not a counter
example, just proves that any gains that are had will be short lived and
increase the size of the crash when crunch time comes. :)
- Naz.

Naz Gassiep wrote:
> Andrew Dunstan wrote:
>
>> Naz Gassiep wrote:
>>
>>> I believe the suggestion was to have an automated process that only ran
>>> on known, sane patches.
>>>
>> How do we know in advance of reviewing them that they are sane?
>>
> Same way as happens now.
>

The question was rhetorical ... there is no list of "certified sane but
unapplied" patches. You are proceeding on the basis of a faulty
understanding of how our processes work.

cheers

andrew

On Wed, 2007-02-05 at 08:27 -0400, Andrew Dunstan wrote:
>
> Naz Gassiep wrote:
> > Andrew Dunstan wrote:
> >
> >> Naz Gassiep wrote:
> >>
> >>> I believe the suggestion was to have an automated process that only ran
> >>> on known, sane patches.
> >>>
> >> How do we know in advance of reviewing them that they are sane?
> >>
> > Same way as happens now.
> >
>
> The question was rhetorical ... there is no list of "certified sane but
> unapplied" patches. You are proceeding on the basis of a faulty
> understanding of how our processes work.

Why do we need to know the patch is sane? If it does not apply cleanly
or causes regression tests to fail, the process would figure that out
quickly and cheaply. There is little cost in attempting to apply a
non-sane patch.

I am not sure that I have explained exactly what I was suggesting. Some
people seem to grok it, others seem to be talking something slightly
different. To clarify, here it is in pseudo-code:

for each patch in the queue
regression_success := false
patch_success := attempt to apply patch to head
if patch_success
regression_success := attempt to run regression tests
-- (On one machine only, not on the buildfarm)
end if
if this is a new patch
maybe mail the author and tell them patch_success and
regression_success
else
if status is different from last time
mail the author and tell them their patch has changed status
end
end
record the status for this patch
end loop

__
Marc

Marc Munro wrote:
> On Wed, 2007-02-05 at 08:27 -0400, Andrew Dunstan wrote:
>>
>> Naz Gassiep wrote:
>> > Andrew Dunstan wrote:
>> >
>> >> Naz Gassiep wrote:
>> >>
>> >>> I believe the suggestion was to have an automated process that only
>> ran
>> >>> on known, sane patches.
>> >>>
>> >> How do we know in advance of reviewing them that they are sane?
>> >>
>> > Same way as happens now.
>> >
>>
>> The question was rhetorical ... there is no list of "certified sane but
>> unapplied" patches. You are proceeding on the basis of a faulty
>> understanding of how our processes work.
>
> Why do we need to know the patch is sane?

Because not doing so is dangerous and a major security hole. I won't run
arbitrary code on my machine and I won't create infrastructure (e.g.
buildfarm) to get others to do it either.

You are also conveniently ignoring all the other reasons why this won't
help anyone much (e.g. see Bruce's comments upthread).

cheers

andrew

Marc Munro <marc(at)bloodnok(dot)com> writes:
> On Wed, 2007-02-05 at 08:27 -0400, Andrew Dunstan wrote:
>> The question was rhetorical ... there is no list of "certified sane but
>> unapplied" patches. You are proceeding on the basis of a faulty
>> understanding of how our processes work.

> Why do we need to know the patch is sane? If it does not apply cleanly
> or causes regression tests to fail, the process would figure that out
> quickly and cheaply. There is little cost in attempting to apply a
> non-sane patch.

Unless it contains a trojan horse. I don't think many buildfarm owners
are running the tests inside a sandbox so tight that they don't care
how nasty the code that runs there might be.

regards, tom lane