Re: guc comment changes (was Re: Getting a move on

Bruce Momjian <bruce(at)momjian(dot)us> writes:
> ... The GUC comment/default patch had tons of
> emails, but no other committers got involved to review or commit the
> patch. Peter, who knows GUC well, looked at it, but said he didn't
> review it enough.

Peter has made it pretty clear that he didn't care for the
refactorization aspect of that patch.

> I just spent 1/2 hour fixing the multi-value UPDATE
> patch for the code drift caused by UPDATE/RETURNING. The patch is a
> simple grammar macro. Any coder could have taken that, reviewed it, and
> applied it, but no one did.

Perhaps that's because nobody but you wanted it to go in.

Some amount of the issue here is that people won't work on patches they
don't approve of; that's certainly the case for me. I have more than
enough to do working on patches I do think should go in, and I get tired
of having to repeatedly object to the same bad patch. Do you remember
Sturgeon's Law? It applies to patches too.

regards, tom lane

Tom Lane wrote:
> Bruce Momjian <bruce(at)momjian(dot)us> writes:
> > ... The GUC comment/default patch had tons of
> > emails, but no other committers got involved to review or commit the
> > patch. Peter, who knows GUC well, looked at it, but said he didn't
> > review it enough.
>
> Peter has made it pretty clear that he didn't care for the
> refactorization aspect of that patch.

Peter asked why it was done, a good answer was given, and Peter did not
reply.

> > I just spent 1/2 hour fixing the multi-value UPDATE
> > patch for the code drift caused by UPDATE/RETURNING. The patch is a
> > simple grammar macro. Any coder could have taken that, reviewed it, and
> > applied it, but no one did.
>
> Perhaps that's because nobody but you wanted it to go in.

We got tons of people who wanted that.

> Some amount of the issue here is that people won't work on patches they
> don't approve of; that's certainly the case for me. I have more than
> enough to do working on patches I do think should go in, and I get tired
> of having to repeatedly object to the same bad patch. Do you remember
> Sturgeon's Law? It applies to patches too.

Sure, you have to want the patch to be in to be motivated to work on it.
I think I am more willing to work with imperfection.

--
Bruce Momjian bruce(at)momjian(dot)us
EnterpriseDB http://www.enterprisedb.com

+ If your life is a hard drive, Christ can be your backup. +

>>> I just spent 1/2 hour fixing the multi-value UPDATE
>>> patch for the code drift caused by UPDATE/RETURNING. The patch is a
>>> simple grammar macro. Any coder could have taken that, reviewed it, and
>>> applied it, but no one did.
>> Perhaps that's because nobody but you wanted it to go in.
>
> We got tons of people who wanted that.

To further this, I didn't even know it was an issue. If it was only half
an hour and it needed to be done, why wasn't it put out there?

Thanks goes to Bruce for fixing it but I didn't know it was an issue, I
have 5 C developers, if any of them could have done it -- then some
communication is needed and I would have allocated someone to it.

I am sure that is the case with others as well.

I am not saying it is anyone's responsibility to speak up, but I can say
had I known some help was needed (especially something so trivial) I
would have gladly donated some time.

Sincerely,

Joshua D. Drake

--

=== The PostgreSQL Company: Command Prompt, Inc. ===
Sales/Support: +1.503.667.4564 || 24x7/Emergency: +1.800.492.2240
Providing the most comprehensive PostgreSQL solutions since 1997
http://www.commandprompt.com/

Bruce Momjian <bruce(at)momjian(dot)us> writes:
> Tom Lane wrote:
>> Peter has made it pretty clear that he didn't care for the
>> refactorization aspect of that patch.

> Peter asked why it was done, a good answer was given, and Peter did not
> reply.

Au contraire, he's reiterated since then that he didn't like it.

>> Perhaps that's because nobody but you wanted it to go in.

> We got tons of people who wanted that.

But no committers, else it would've got done.

There was some remark upthread about reviewers getting discouraged
because their comments seem to fall on deaf ears. I know exactly
what is meant by that. I'm getting tired of arguing with you about
bad patches, because it's obvious that you put no weight on my
objections --- and it looks like Peter has got the same problem.
How is it that you are willing to apply submissions from newbie
developers over the objections of core developers?

regards, tom lane

Tom Lane wrote:
> Bruce Momjian <bruce(at)momjian(dot)us> writes:
> > Tom Lane wrote:
> >> Peter has made it pretty clear that he didn't care for the
> >> refactorization aspect of that patch.
>
> > Peter asked why it was done, a good answer was given, and Peter did not
> > reply.
>
> Au contraire, he's reiterated since then that he didn't like it.

The thread order was: patch, Peter comments, submitter gives reasons,
patch put in the queue, Peter comments again, I reply that the change is
not just "refactoring" but is needed based on submitters comments, and
no reply from Peter:

http://archives.postgresql.org/pgsql-patches/2006-08/msg00334.php

Without a reply from Peter, I have to assume the patch is valid.

> >> Perhaps that's because nobody but you wanted it to go in.
>
> > We got tons of people who wanted that.
>
> But no committers, else it would've got done.

True that it was of more interest to non-committers than committers,
probably because it is the committers who are going to have to remove it
when full SQL functionality is added. :-}

> There was some remark upthread about reviewers getting discouraged
> because their comments seem to fall on deaf ears. I know exactly
> what is meant by that. I'm getting tired of arguing with you about
> bad patches, because it's obvious that you put no weight on my
> objections --- and it looks like Peter has got the same problem.
> How is it that you are willing to apply submissions from newbie
> developers over the objections of core developers?

Well, as I see it, core developers have no special weight in votes,
except by their ability to influence the community by their posted
opinions. If we want core people to have a larger weight in making
decisions, I think the community should decide that. I have never heard
that stated by anyone in this group.

I think the UPDATE ... SET (val, ...) patch is illustrative. Some
committers didn't want it because it didn't add new functionality, and
because it has to be remove later when full functionality is added. But
a lot of users wanted it because it implemented part of the spec, and
because it will make porting easier. Based on how many users wanted it,
and because the patch is very localized (gram.y), I thought it should be
in 8.2. The Win32 port is an extreme case of this dichotomy ---
headache for developers, boon for the user community.

We all started as newbie patch submitters (some better than others, of
course). I think as leaders if we don't like a patch for some reason,
we have to give feedback on how the submitter can fix it, or
community-agreed reasons why the patch will never be accepted. Another
example is the FETCH/MOVE int64 patch. If there is a performance
concern, should we just ask the submitters to do testing. You mentioned
there is a way to do MOVE >2gig already, but did not supply the method.
I can't figure out what it is, and I am sure others don't know either.
If we can do it, that should be stated so we can document it and all
understand why FETCH/MOVE int64 isn't a good addition.

Basically, the people who have been around longer have to spend time to
educate patch submitters to improve. If we don't, we don't grow our
pool of experienced developers.

The fundamental problem is if someone objects to a patch, and then
someone reasonably replies to that objection, lack of a reply means to
me that the last reply was valid. I realize this places a burden on
people to discuss patches, but I see no better way to do it. I don't
think we want to go down the road where we have individuals who object
to patches, but are not required to engage in discussion about the
patch. I think many dysfunctional open source projects have this
problem.

On a personal note, Tom, I do place great weight on your opinions, and
try to adjust things to make sure you, the submitter, and everyone else
is satisified with the result.

--
Bruce Momjian bruce(at)momjian(dot)us
EnterpriseDB http://www.enterprisedb.com

+ If your life is a hard drive, Christ can be your backup. +

Joshua D. Drake wrote:
>
> >>> I just spent 1/2 hour fixing the multi-value UPDATE
> >>> patch for the code drift caused by UPDATE/RETURNING. The patch is a
> >>> simple grammar macro. Any coder could have taken that, reviewed it, and
> >>> applied it, but no one did.
> >> Perhaps that's because nobody but you wanted it to go in.
> >
> > We got tons of people who wanted that.
>
> To further this, I didn't even know it was an issue. If it was only half
> an hour and it needed to be done, why wasn't it put out there?

I did, sort of:

http://archives.postgresql.org/pgsql-hackers/2006-08/msg00869.php

The author replied to me privately, and I said I would adjust it.

> Thanks goes to Bruce for fixing it but I didn't know it was an issue, I
> have 5 C developers, if any of them could have done it -- then some
> communication is needed and I would have allocated someone to it.
>
> I am sure that is the case with others as well.
>
> I am not saying it is anyone's responsibility to speak up, but I can say
> had I known some help was needed (especially something so trivial) I
> would have gladly donated some time.

The patch also required a little adjustment, as I posted, so it wasn't a
trival issue you could have just thrown at someone.

--
Bruce Momjian bruce(at)momjian(dot)us
EnterpriseDB http://www.enterprisedb.com

+ If your life is a hard drive, Christ can be your backup. +

Bruce Momjian wrote:
> Without a reply from Peter, I have to assume the patch is valid.

To make it more explicit: I think the patch is stupid, but if someone
wants to review it, go ahead. But I am not comfortable with the "if no
one objects, I'll just commit it" mode that is sometimes going on. Has
anyone actually tested the patch?

--
Peter Eisentraut
http://developer.postgresql.org/~petere/

Peter Eisentraut wrote:
> Bruce Momjian wrote:
> > Without a reply from Peter, I have to assume the patch is valid.
>
> To make it more explicit: I think the patch is stupid, but if someone
> wants to review it, go ahead. But I am not comfortable with the "if no
> one objects, I'll just commit it" mode that is sometimes going on. Has
> anyone actually tested the patch?

Are you saying you don't like the patch, or the goal of resetting the
value to their defaults when they are commented out? If the first, do
you have a suggestion, if the later, it is something confuses many
users.

--
Bruce Momjian bruce(at)momjian(dot)us
EnterpriseDB http://www.enterprisedb.com

+ If your life is a hard drive, Christ can be your backup. +

Am Montag, 4. September 2006 04:10 schrieb Bruce Momjian:
> Are you saying you don't like the patch,

That's it.

--
Peter Eisentraut
http://developer.postgresql.org/~petere/

Peter Eisentraut <peter_e(at)gmx(dot)net> writes:
> Bruce Momjian wrote:
>> Without a reply from Peter, I have to assume the patch is valid.

> To make it more explicit: I think the patch is stupid, but if someone
> wants to review it, go ahead. But I am not comfortable with the "if no
> one objects, I'll just commit it" mode that is sometimes going on. Has
> anyone actually tested the patch?

Perhaps more to the point: a refactorization patch is all about beauty
in the eye of the beholder. If Peter, the original author of the guc
code, thinks that it's a disimprovement, I think it's a hard argument
to make that the patch should go in anyway.

regards, tom lane

Tom Lane wrote:
> Peter Eisentraut <peter_e(at)gmx(dot)net> writes:
> > Bruce Momjian wrote:
> >> Without a reply from Peter, I have to assume the patch is valid.
>
> > To make it more explicit: I think the patch is stupid, but if someone
> > wants to review it, go ahead. But I am not comfortable with the "if no
> > one objects, I'll just commit it" mode that is sometimes going on. Has
> > anyone actually tested the patch?
>
> Perhaps more to the point: a refactorization patch is all about beauty
> in the eye of the beholder. If Peter, the original author of the guc
> code, thinks that it's a disimprovement, I think it's a hard argument
> to make that the patch should go in anyway.

How many times do I have to say this: IT IS NOT A REFACTOR PATCH AS
REPORTED BY THE AUTHOR, AND PETER HAS NOT REFUTED THAT.

It fixes a bug reported by the author, and Peter's inability to reply to
the comments the author made is exactly the behavior I am talking about.
If Peter does not want to engage in a technical discussion about the
patch, I don't think we can consider his opinion valid.

Seems I will have to call for a vote on this patch.

--
Bruce Momjian bruce(at)momjian(dot)us
EnterpriseDB http://www.enterprisedb.com

+ If your life is a hard drive, Christ can be your backup. +

Bruce Momjian wrote:
> How many times do I have to say this: IT IS NOT A REFACTOR PATCH AS
> REPORTED BY THE AUTHOR, AND PETER HAS NOT REFUTED THAT.

The initial patch was the feature plus some code refactoring included.
That was what the author said. I asked him to submit the refactoring
and the feature as two separate patches. What I got was a refactoring
subpatch that actually made the code longer in terms of lines, which
must be the very first code refactoring ever to achieve that. I did
not get a satisfying answer on why that has to be, so I sort of lost
interest in working with that patch.

That does not mean that the patch is bad, and I certainly support the
feature change. But I can't efficiently review the patch. If someone
else wants to do it, go ahead.

--
Peter Eisentraut
http://developer.postgresql.org/~petere/

Peter Eisentraut wrote:
> Bruce Momjian wrote:
> > How many times do I have to say this: IT IS NOT A REFACTOR PATCH AS
> > REPORTED BY THE AUTHOR, AND PETER HAS NOT REFUTED THAT.
>
> The initial patch was the feature plus some code refactoring included.
> That was what the author said. I asked him to submit the refactoring
> and the feature as two separate patches. What I got was a refactoring
> subpatch that actually made the code longer in terms of lines, which
> must be the very first code refactoring ever to achieve that. I did
> not get a satisfying answer on why that has to be, so I sort of lost
> interest in working with that patch.

Sure, thanks. Here is his reply from the patch author as to why the
patch isn't just refactoring:

http://archives.postgresql.org/pgsql-patches/2006-08/msg00103.php

The next email in the thread is two weeks later from the patch author
asking about the status of the patch.

If we don't need the refactoring part, great, but I want to be sure.

--
Bruce Momjian bruce(at)momjian(dot)us
EnterpriseDB http://www.enterprisedb.com

+ If your life is a hard drive, Christ can be your backup. +

On Sat, Sep 02, 2006 at 09:26:36PM -0700, Joshua D. Drake wrote:
>
> >>>I just spent 1/2 hour fixing the multi-value UPDATE
> >>>patch for the code drift caused by UPDATE/RETURNING. The patch is a
> >>>simple grammar macro. Any coder could have taken that, reviewed it, and
> >>>applied it, but no one did.
> >>Perhaps that's because nobody but you wanted it to go in.
> >
> >We got tons of people who wanted that.
>
> To further this, I didn't even know it was an issue. If it was only half
> an hour and it needed to be done, why wasn't it put out there?
>
> Thanks goes to Bruce for fixing it but I didn't know it was an issue, I
> have 5 C developers, if any of them could have done it -- then some
> communication is needed and I would have allocated someone to it.
>
> I am sure that is the case with others as well.
>
> I am not saying it is anyone's responsibility to speak up, but I can say
> had I known some help was needed (especially something so trivial) I
> would have gladly donated some time.

There's been talk in the past of having some kind of system that
automatically attempts to build things that are in the patch queue, both
as an initial sanity-check and as a means to detect when something
bit-rots... perhaps it's becoming worthwhile to set that up.
--
Jim Nasby jim(at)nasby(dot)net
EnterpriseDB http://enterprisedb.com 512.569.9461 (cell)

Tom Dunstan wrote:

> The idea was that patch authors could either run it manually or stick it
> in a cron so they could get emailed when the patch no longer cleanly
> applied, or when the patched source failed in make, make check etc.
> Obviously my motivation was to keep the enum patch up to date until we
> hit 8.3 and someone looks at it. To that end it might also be useful for
> it to die if duplicate_oids finds anything.
>
> I submitted a patch to Andrew, but it needed a couple of tweaks
> (disabling patching on vpath builds, for example) and I don't think I
> ever got around to resubmitting it, but if there's more general interest
> I'll do so.

Huh, why would you disable patching on vpath builds?

> Note that it was intended for patch authors to run themselves rather
> than any kind of central mechanism to test the patch queue.

Well, I'd think that one important benefit of passing patches through
the buildfarm is detecting possible portability problems in it.

> While it would obviously be nice to know what the current status of
> any given patch in the queue is, the thing about the patch queue is
> that it contains patches that we haven't had time to review yet. It'll
> only take one patch to get into the queue containing a security
> vulnerability, or worse, a trojan, for it to seem unfortunate.

We could have a register of developers allowed to upload patches to the
"patched build queue", and have a policy that says that you should only
upload a patch if it has already passed some review.

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

Tom Dunstan wrote:
> Alvaro Herrera wrote:
> >>I submitted a patch to Andrew, but it needed a couple of tweaks
> >>(disabling patching on vpath builds, for example) and I don't think I
> >>ever got around to resubmitting it, but if there's more general interest
> >>I'll do so.
> >
> >Huh, why would you disable patching on vpath builds?
>
> As I understand it, vpath builds only do a checkout to a single place,
> and then build against that (from a different directory). Non-vpath
> builds take a copy of the checked out source and build in the copy. If
> we patched the source in vpath builds, the patch would stick around when
> updating the source tree from CVS, and the next patch attempt would fail
> etc. Non-vpath builds will get a clean copy to patch in subsequent runs.
> I suppose I could have made vpath builds take a copy when doing a patch,
> but it hardly seemed worth it.

Huh, but the patch can be applied with -R to revert it after the
buildfarm run ... the one problem I can see is if the patch fails for
some reason; for which I'd suggest running a patch --dry-run as a first
step, checking that it applies cleanly, and only continue in that case.

[nice ideas snipped (not sniped)]

> Note that the existing patch queue mechanism wouldn't suffice, since
> there's no standard way to pull patches through via http or whatever.
> We'd probably want to back it with a small database and webapp, which
> might just be a hacked buildfarm server.

Oh, sure. I'd say there is no "existing patch queue", just a Mhonarc
archive which is just useful as a reminder of what patches are there
outstanding.

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

Alvaro Herrera wrote:
>
> Huh, but the patch can be applied with -R to revert it after the
> buildfarm run ... the one problem I can see is if the patch fails for
> some reason; for which I'd suggest running a patch --dry-run as a first
> step, checking that it applies cleanly, and only continue in that case.
>
>

Unfortunately, this won't fly as we rely on file timestamps to tell us
which files have changed.

Tom's idea of making a temp copy of the repo and patching that would work,
but if you're going to do that why do a vpath build anyway?

Regarding the idea of a list of approved patch authorisers, don't we have
such a group now? i.e. "committers".

cheers

andrew

Jim C. Nasby wrote:
> There's been talk in the past of having some kind of system that
> automatically attempts to build things that are in the patch queue, both
> as an initial sanity-check and as a means to detect when something
> bit-rots... perhaps it's becoming worthwhile to set that up.

After writing the enum patch, I hacked the buildfarm client code to
apply a patch to the checked out code before building. You could then
run it thusly:

./run_build.pl --nosend --nostatus --verbose \
--patch=/home/tom/src/enums-v1.patch --patch-level=1

The idea was that patch authors could either run it manually or stick it
in a cron so they could get emailed when the patch no longer cleanly
applied, or when the patched source failed in make, make check etc.
Obviously my motivation was to keep the enum patch up to date until we
hit 8.3 and someone looks at it. To that end it might also be useful for
it to die if duplicate_oids finds anything.

I submitted a patch to Andrew, but it needed a couple of tweaks
(disabling patching on vpath builds, for example) and I don't think I
ever got around to resubmitting it, but if there's more general interest
I'll do so.

Note that it was intended for patch authors to run themselves rather
than any kind of central mechanism to test the patch queue. While it
would obviously be nice to know what the current status of any given
patch in the queue is, the thing about the patch queue is that it
contains patches that we haven't had time to review yet. It'll only take
one patch to get into the queue containing a security vulnerability, or
worse, a trojan, for it to seem unfortunate.

I had thoughts of hacking the buildfarm server to allow the posting of a
patch along with results, so that authors could report results for their
own patches, but ran out of time. Is there interest in doing that?
Obviously it'd be a different server to the existing buildfarm.

Cheers

Tom

Jim C. Nasby wrote:
> There's been talk in the past of having some kind of system that
> automatically attempts to build things that are in the patch queue, both
> as an initial sanity-check and as a means to detect when something
> bit-rots... perhaps it's becoming worthwhile to set that up.

After writing the enum patch, I hacked the buildfarm client code to
apply a patch to the checked out code before building. You could then
run it thusly:

./run_build.pl --nosend --nostatus --verbose \
--patch=/home/tom/src/enums-v1.patch --patch-level=1

The idea was that patch authors could either run it manually or stick it
in a cron so they could get emailed when the patch no longer cleanly
applied, or when the patched source failed in make, make check etc.
Obviously my motivation was to keep the enum patch up to date until we
hit 8.3 and someone looks at it. To that end it might also be useful for
it to die if duplicate_oids finds anything.

I submitted a patch to Andrew, but it needed a couple of tweaks
(disabling patching on vpath builds, for example) and I don't think I
ever got around to resubmitting it, but if there's more general interest
I'll do so.

Note that it was intended for patch authors to run themselves rather
than any kind of central mechanism to test the patch queue. While it
would obviously be nice to know what the current status of any given
patch in the queue is, the thing about the patch queue is that it
contains patches that we haven't had time to review yet. It'll only take
one patch to get into the queue containing a security vulnerability, or
worse, a trojan, for it to seem unfortunate.

I had thoughts of hacking the buildfarm server to allow the posting of a
patch along with results, so that authors could report results for their
own patches, but ran out of time. Is there interest in doing that?
Obviously it'd be a different server to the existing buildfarm.

Cheers

Tom

On Sep 13, 2006, at 6:56 PM, Tom Dunstan wrote:
>> Regarding the idea of a list of approved patch authorisers, don't
>> we have
>> such a group now? i.e. "committers".
>
> Right, and if committers or others are willing to put in the time
> required to verify that patches aren't nasty before going onto the
> blessed patch queue, the idea could quite possibly work and provide
> some value. Note that all we really need to test for here is that
> the patch isn't malicious; patches that are bad design or whatever
> are unlikely to open security holes or fry your box. A major point
> of the queue is that the appropriate committer often doesn't have
> the time to review the patch right now. There might be some benefit
> in allowing a wider set of contributors to bless patches as non-
> nasty for testing purposes, rather than waste the limited time of
> qualified committers. Maybe such an army doesn't exist, though.

That's something I'd be willing to do. And for many people that
aren't committers but are still trusted in the community, we could
probably bypass the checking.

Another possibility would be to test these patches in some kind of
virtual machine that gets blown away every X days, so that even if
someone did get something malicious in there it wouldn't last long.
--
Jim Nasby jim(at)nasby(dot)net
EnterpriseDB http://enterprisedb.com 512.569.9461 (cell)

Alvaro Herrera wrote:
>> I submitted a patch to Andrew, but it needed a couple of tweaks
>> (disabling patching on vpath builds, for example) and I don't think I
>> ever got around to resubmitting it, but if there's more general interest
>> I'll do so.
>
> Huh, why would you disable patching on vpath builds?

As I understand it, vpath builds only do a checkout to a single place,
and then build against that (from a different directory). Non-vpath
builds take a copy of the checked out source and build in the copy. If
we patched the source in vpath builds, the patch would stick around when
updating the source tree from CVS, and the next patch attempt would fail
etc. Non-vpath builds will get a clean copy to patch in subsequent runs.
I suppose I could have made vpath builds take a copy when doing a patch,
but it hardly seemed worth it.

> Well, I'd think that one important benefit of passing patches through
> the buildfarm is detecting possible portability problems in it.

Absolutely. As long as they're blessed as mention below...

> We could have a register of developers allowed to upload patches to the
> "patched build queue", and have a policy that says that you should only
> upload a patch if it has already passed some review.

This was where I was originally heading when I first thought about this
functionality. I didn't go that far with my fairly modest patch since a)
it wasn't clear that there was manpower available to do the initial
review to bless the patches, and b) what I did do solved my immediate
problem.

If there is support for doing something like this, there are all kinds
of things that could be done. For example, you could check which patches
break which other ones. An even more way-out idea might be to have
performance patches run pgbench or some other set of benchmarks. You
have a performance related patch? Let's stick it in the queue and see if
it really improves things or not.

Note that the existing patch queue mechanism wouldn't suffice, since
there's no standard way to pull patches through via http or whatever.
We'd probably want to back it with a small database and webapp, which
might just be a hacked buildfarm server.

Cheers

Tom

On Wed, 13 Sep 2006, Tom Dunstan wrote:

> > Another possibility would be to test these patches in some kind of virtual
> > machine that gets blown away every X days, so that even if someone did get
> > something malicious in there it wouldn't last long.

Or just have a snapshot which is reverted after each run, and read-only
access to files used to do the build. I know vmware supports this,
probably others too...

> Yeah, nasties could be roughly separated into two categories: stuff which
> affects your box, or stuff which uses your box to affect someone else. A VM
> fixes the first, and a firewall blocking outgoing connections (with exceptions
> for the CVS server and patch buildfarm or whatever it is server) largely fixes
> the second.
>
> I was under the impression that most VM products are x86 centric, which
> wouldn't lead to huge amounts of diversity in the buildfarm results. At least,
> not as far as architecture goes.

I have played with QEmu (www.qemu.org) which is open source and supports
multiple target architectures. I'm not sure how stable all of the
different targets are, I know that sparc64 is not quite done yet.

--
The problem with engineers is that they tend to cheat in order to get
results.

The problem with mathematicians is that they tend to work on toy
problems in order to get results.

The problem with program verifiers is that they tend to cheat at toy
problems in order to get results.

Andrew Dunstan wrote:
> Tom's idea of making a temp copy of the repo and patching that would work,
> but if you're going to do that why do a vpath build anyway?

In this case, the answer is to make sure that your patch works when
*someone else* does a vpath build.

> Regarding the idea of a list of approved patch authorisers, don't we have
> such a group now? i.e. "committers".

Right, and if committers or others are willing to put in the time
required to verify that patches aren't nasty before going onto the
blessed patch queue, the idea could quite possibly work and provide some
value. Note that all we really need to test for here is that the patch
isn't malicious; patches that are bad design or whatever are unlikely to
open security holes or fry your box. A major point of the queue is that
the appropriate committer often doesn't have the time to review the
patch right now. There might be some benefit in allowing a wider set of
contributors to bless patches as non-nasty for testing purposes, rather
than waste the limited time of qualified committers. Maybe such an army
doesn't exist, though.

Cheers

Tom

Jim Nasby wrote:
> That's something I'd be willing to do. And for many people that aren't
> committers but are still trusted in the community, we could probably
> bypass the checking.

That's a worthwhile point. How many patches come from the general
community vs out of the blue? Patches from regulars could probably get a
free pass, which might cut down the review burden substantially.

> Another possibility would be to test these patches in some kind of
> virtual machine that gets blown away every X days, so that even if
> someone did get something malicious in there it wouldn't last long.

Yeah, nasties could be roughly separated into two categories: stuff
which affects your box, or stuff which uses your box to affect someone
else. A VM fixes the first, and a firewall blocking outgoing connections
(with exceptions for the CVS server and patch buildfarm or whatever it
is server) largely fixes the second.

I was under the impression that most VM products are x86 centric, which
wouldn't lead to huge amounts of diversity in the buildfarm results. At
least, not as far as architecture goes.

Cheers

Tom

Jeremy Drake wrote:
> On Wed, 13 Sep 2006, Tom Dunstan wrote:
>
>> I was under the impression that most VM products are x86 centric, which
>> wouldn't lead to huge amounts of diversity in the buildfarm results. At least,
>> not as far as architecture goes.
>
> I have played with QEmu (www.qemu.org) which is open source and supports
> multiple target architectures. I'm not sure how stable all of the
> different targets are, I know that sparc64 is not quite done yet.

Oh, I didn't realize Qemu did non-x86 architectures. Is it considered
good enough at emulating e.g. a sparc for it to be useful to us? PearPC
was a PowerPC emulator that got some press a while ago, although it
appears that the project has stagnated a bit (probably because people
who wanted to run OSX on intel hardware have a legit way to do it now :) )

The problem with these things is if something goes wrong, was it the
patch that failed or the not-quite-perfect VM product? To cut down on
those sorts of problems, I suppose we could have it do a clean,
non-patched run first, and then only do the patched version if the clean
version passed. We'd have to be reasonably unlucky to have a patch
trigger a VM bug under those circumstance, I would think.

Cheers

Tom

Hi, Jeremy,

Jeremy Drake wrote:

>>> Another possibility would be to test these patches in some kind of virtual
>>> machine that gets blown away every X days, so that even if someone did get
>>> something malicious in there it wouldn't last long.
>
> Or just have a snapshot which is reverted after each run, and read-only
> access to files used to do the build. I know vmware supports this,
> probably others too...

A chroot / fakeroot combined with unionfs should do the same, probably
with less effort. There are other user-mode jail projects that also
block networking.

Markus

--
Markus Schaber | Logical Tracking&Tracing International AG
Dipl. Inf. | Software Development GIS

Fight against software patents in Europe! www.ffii.org
www.nosoftwarepatents.org

Peter Eisentraut <peter_e(at)gmx(dot)net> writes:
> That does not mean that the patch is bad, and I certainly support the
> feature change. But I can't efficiently review the patch. If someone
> else wants to do it, go ahead.

I've finally taken a close look at this patch, and I don't like it any
more than Peter does. The refactoring might or might not be good at its
core, but as presented it is horrid. As just one example, it replaces one
reasonably well-commented function with three misnamed, poorly commented
functions. In place of

/*
! * Sets option `name' to given value. The value should be a string
! * which is going to be parsed and converted to the appropriate data
! * type. The context and source parameters indicate in which context this
! * function is being called so it can apply the access restrictions
! * properly.
! *
! * If value is NULL, set the option to its default value. If the
! * parameter changeVal is false then don't really set the option but do all
! * the checks to see if it would work.
! *
! * If there is an error (non-existing option, invalid value) then an
! * ereport(ERROR) is thrown *unless* this is called in a context where we
! * don't want to ereport (currently, startup or SIGHUP config file reread).
! * In that case we write a suitable error message via ereport(DEBUG) and
! * return false. This is working around the deficiencies in the ereport
! * mechanism, so don't blame me. In all other cases, the function
! * returns true, including cases where the input is valid but we chose
! * not to apply it because of context or source-priority considerations.
! *
! * See also SetConfigOption for an external interface.
*/
! bool
! set_config_option(const char *name, const char *value,
! GucContext context, GucSource source,
! bool isLocal, bool changeVal)

we find

/*
! * Try to parse value. Determine what is type and call related
! * parsing function or if newval is equal to NULL, reset value
! * to default or bootval. If the value parsed okay return true,
! * else false.
*/
! static bool
! parse_value(int elevel, const struct config_generic *record,
! const char *value, GucSource *source, bool changeVal,
! union config_var_value *retval)

which doesn't tell you quite what the parameters do, but more
fundamentally is misnamed because one would expect "parse_value"
returning bool to merely check whether the value is syntactically
correct. Well, it doesn't do that: it applies the value too.
Another broken-out routine is

! /*
! * Check if the option can be set at this time. See guc.h for the precise
! * rules.
! */
! static bool
! checkContext(int elevel, struct config_generic *record, GucContext context)

which is again a misleading description because it doesn't bother to
explain that control may not come back if the option is rejected
(depending on elevel). One might also think, given that description,
that the caller is supposed to emit an error message on false result.
Lastly we have

+ /*
+ * Verify if option exists and value is valid.
+ * It is primary used for validation of items in configuration file.
+ */
+ bool
+ verify_config_option(const char *name, const char *value,
+ GucContext context, GucSource source,
+ bool *isNewEqual, bool *isContextOK)

which again is far south of my ideas for adequate documentation of a
function with a fairly complicated API. And guess what, this one has
side effects too, which it surely should not (and that leads directly
to a bug: GUC_IN_CONFFILE could remain set in a variable after a
parsing failure).

It's possible that a refactoring along these lines could be an
improvement if it were well coded and well documented, but this patch
is not it.

The comment-reversion part of the patch is not any better. It's poorly
factored (what the heck is guc-file.l doing patching up the source
settings after calling set_config_option?), which is surprising
considering the whole point of the refactoring was to support this.
And the handling of reversion to a PGC_S_ENV_VAR setting is just a kluge
involving duplicated code (what was that about refactoring again?).

In short, whether or not it has any remaining undetected bugs, this
patch is a severe disimprovement from the standpoint of source code
quality, and I recommend rejecting it.

regards, tom lane

OK, patch sent to the 8.3 hold queue for later work, open item removed.

---------------------------------------------------------------------------

Tom Lane wrote:
> Peter Eisentraut <peter_e(at)gmx(dot)net> writes:
> > That does not mean that the patch is bad, and I certainly support the
> > feature change. But I can't efficiently review the patch. If someone
> > else wants to do it, go ahead.
>
> I've finally taken a close look at this patch, and I don't like it any
> more than Peter does. The refactoring might or might not be good at its
> core, but as presented it is horrid. As just one example, it replaces one
> reasonably well-commented function with three misnamed, poorly commented
> functions. In place of
>
> /*
> ! * Sets option `name' to given value. The value should be a string
> ! * which is going to be parsed and converted to the appropriate data
> ! * type. The context and source parameters indicate in which context this
> ! * function is being called so it can apply the access restrictions
> ! * properly.
> ! *
> ! * If value is NULL, set the option to its default value. If the
> ! * parameter changeVal is false then don't really set the option but do all
> ! * the checks to see if it would work.
> ! *
> ! * If there is an error (non-existing option, invalid value) then an
> ! * ereport(ERROR) is thrown *unless* this is called in a context where we
> ! * don't want to ereport (currently, startup or SIGHUP config file reread).
> ! * In that case we write a suitable error message via ereport(DEBUG) and
> ! * return false. This is working around the deficiencies in the ereport
> ! * mechanism, so don't blame me. In all other cases, the function
> ! * returns true, including cases where the input is valid but we chose
> ! * not to apply it because of context or source-priority considerations.
> ! *
> ! * See also SetConfigOption for an external interface.
> */
> ! bool
> ! set_config_option(const char *name, const char *value,
> ! GucContext context, GucSource source,
> ! bool isLocal, bool changeVal)
>
> we find
>
> /*
> ! * Try to parse value. Determine what is type and call related
> ! * parsing function or if newval is equal to NULL, reset value
> ! * to default or bootval. If the value parsed okay return true,
> ! * else false.
> */
> ! static bool
> ! parse_value(int elevel, const struct config_generic *record,
> ! const char *value, GucSource *source, bool changeVal,
> ! union config_var_value *retval)
>
> which doesn't tell you quite what the parameters do, but more
> fundamentally is misnamed because one would expect "parse_value"
> returning bool to merely check whether the value is syntactically
> correct. Well, it doesn't do that: it applies the value too.
> Another broken-out routine is
>
> ! /*
> ! * Check if the option can be set at this time. See guc.h for the precise
> ! * rules.
> ! */
> ! static bool
> ! checkContext(int elevel, struct config_generic *record, GucContext context)
>
> which is again a misleading description because it doesn't bother to
> explain that control may not come back if the option is rejected
> (depending on elevel). One might also think, given that description,
> that the caller is supposed to emit an error message on false result.
> Lastly we have
>
> + /*
> + * Verify if option exists and value is valid.
> + * It is primary used for validation of items in configuration file.
> + */
> + bool
> + verify_config_option(const char *name, const char *value,
> + GucContext context, GucSource source,
> + bool *isNewEqual, bool *isContextOK)
>
> which again is far south of my ideas for adequate documentation of a
> function with a fairly complicated API. And guess what, this one has
> side effects too, which it surely should not (and that leads directly
> to a bug: GUC_IN_CONFFILE could remain set in a variable after a
> parsing failure).
>
> It's possible that a refactoring along these lines could be an
> improvement if it were well coded and well documented, but this patch
> is not it.
>
> The comment-reversion part of the patch is not any better. It's poorly
> factored (what the heck is guc-file.l doing patching up the source
> settings after calling set_config_option?), which is surprising
> considering the whole point of the refactoring was to support this.
> And the handling of reversion to a PGC_S_ENV_VAR setting is just a kluge
> involving duplicated code (what was that about refactoring again?).
>
> In short, whether or not it has any remaining undetected bugs, this
> patch is a severe disimprovement from the standpoint of source code
> quality, and I recommend rejecting it.
>
> regards, tom lane
>
> ---------------------------(end of broadcast)---------------------------
> TIP 3: Have you checked our extensive FAQ?
>
> http://www.postgresql.org/docs/faq

--
Bruce Momjian bruce(at)momjian(dot)us
EnterpriseDB http://www.enterprisedb.com

+ If your life is a hard drive, Christ can be your backup. +

Am Freitag, 15. September 2006 20:32 schrieb Tom Lane:
> I've finally taken a close look at this patch, and I don't like it any
> more than Peter does. The refactoring might or might not be good at its
> core, but as presented it is horrid.

Joachim Wieland is in the process of reworking the original feature patch
(resetting commented out parameters) in a much more compact form. But it
turns out that there are a couple of very tricky situations involving custom
variables, which may need more discussion than we have time for now.

--
Peter Eisentraut
http://developer.postgresql.org/~petere/

On Mon, Sep 18, 2006 at 01:13:45PM +0200, Peter Eisentraut wrote:
> Joachim Wieland is in the process of reworking the original feature patch
> (resetting commented out parameters) in a much more compact form. But it
> turns out that there are a couple of very tricky situations involving custom
> variables, which may need more discussion than we have time for now.

The problem with custom variables is the definition of their default value.
First I defined it to be "variable does not exist" and tried to have a close
analogy to what happens on set/set local/reset with normal variables.
However that means that custom variables have to vanish (where other
variables show their default value) and revive (where other variables show a
changed value). For example if you have:

begin;
savepoint a;
set foo.var to 3;
(here we delete foo.var from the configuration file and send SIGHUP)
savepoint b;
reset foo.var;

There seem to exist quite a few possible definitions as to what happens if you
run "show foo.var" after the "reset" directly or after rolling back to the
savepoints a and b.

The result of the discussion with Peter was that all that seems not worth
doing since it is a rare special case. In the current form of the patch if you
delete a custom variable from the configuration file it gets deleted
internally and all reference to it results in error. There is still one
special case which is:

begin;
set foo.var to 3;
(here we delete foo.var from the configuration file and send SIGHUP)
commit;

This transaction will fail because commit cannot change the value of the
variable to 3 as requested by the user.

I have the patch almost ready in the described form, if there is any chance
it will make it into 8.2 I will clean it up and post it ASAP but Peter wrote
me that chances are close to zero and so I stopped working on it for now.

Joachim

--
Joachim Wieland joe(at)mcknight(dot)de
GPG key available

Joachim Wieland <joe(at)mcknight(dot)de> writes:
> I have the patch almost ready in the described form, if there is any chance
> it will make it into 8.2 I will clean it up and post it ASAP but Peter wrote
> me that chances are close to zero and so I stopped working on it for now.

If you'd mentioned it a bit sooner I would have encouraged you to get
it done. But given your description it sounds a bit too
hairy/potentially-destabilizing to be dropping in the day before beta.
(I certainly hadn't thought about custom vars at all :-()
Let's plan it for 8.3 instead.

regards, tom lane

Tom Lane wrote:
> Peter Eisentraut <peter_e(at)gmx(dot)net> writes:
>> That does not mean that the patch is bad, and I certainly support the
>> feature change. But I can't efficiently review the patch. If someone
>> else wants to do it, go ahead.
>
> I've finally taken a close look at this patch, and I don't like it any
> more than Peter does. The refactoring might or might not be good at its
> core, but as presented it is horrid. As just one example, it replaces one
> reasonably well-commented function with three misnamed, poorly commented
> functions. In place of

Thanks Tom for your time to look on the code and for your feedback. It
is very useful for me.

Thanks Zdenek

On Wed, Sep 13, 2006 at 22:22:12 -0700,
Tom Dunstan <pgsql(at)tomd(dot)cc> wrote:
>
> That's a worthwhile point. How many patches come from the general
> community vs out of the blue? Patches from regulars could probably get a
> free pass, which might cut down the review burden substantially.

And how were you planning to tell if a patch cam from a regular? Hopefully
you weren't planning on blindly trusting the "from" header.
Misuse of the build farm in a way the effects other sites could get the
project a big black eye, so you want to be very careful building and
executing code from the patch queue.

On Fri, Sep 22, 2006 at 11:58:04AM -0500, Bruno Wolff III wrote:
> On Wed, Sep 13, 2006 at 22:22:12 -0700,
> Tom Dunstan <pgsql(at)tomd(dot)cc> wrote:
> >
> > That's a worthwhile point. How many patches come from the general
> > community vs out of the blue? Patches from regulars could probably get a
> > free pass, which might cut down the review burden substantially.
>
> And how were you planning to tell if a patch cam from a regular? Hopefully
> you weren't planning on blindly trusting the "from" header.
> Misuse of the build farm in a way the effects other sites could get the
> project a big black eye, so you want to be very careful building and
> executing code from the patch queue.

Of course not, but there's any number of ways we could handle that
problem.
--
Jim Nasby jim(at)nasby(dot)net
EnterpriseDB http://enterprisedb.com 512.569.9461 (cell)

Jim C. Nasby wrote:
> On Fri, Sep 22, 2006 at 11:58:04AM -0500, Bruno Wolff III wrote:
>> On Wed, Sep 13, 2006 at 22:22:12 -0700,
>> Tom Dunstan <pgsql(at)tomd(dot)cc> wrote:
>>> That's a worthwhile point. How many patches come from the general
>>> community vs out of the blue? Patches from regulars could probably get a
>>> free pass, which might cut down the review burden substantially.
>> And how were you planning to tell if a patch cam from a regular? Hopefully
>> you weren't planning on blindly trusting the "from" header.
>> Misuse of the build farm in a way the effects other sites could get the
>> project a big black eye, so you want to be very careful building and
>> executing code from the patch queue.
>
> Of course not, but there's any number of ways we could handle that
> problem.

pgp signed patches?

--

=== The PostgreSQL Company: Command Prompt, Inc. ===
Sales/Support: +1.503.667.4564 || 24x7/Emergency: +1.800.492.2240
Providing the most comprehensive PostgreSQL solutions since 1997
http://www.commandprompt.com/

On Sep 22, 2006, at 2:50 PM, Joshua D. Drake wrote:
>>> And how were you planning to tell if a patch cam from a regular?
>>> Hopefully
>>> you weren't planning on blindly trusting the "from" header.
>>> Misuse of the build farm in a way the effects other sites could
>>> get the
>>> project a big black eye, so you want to be very careful building and
>>> executing code from the patch queue.
>> Of course not, but there's any number of ways we could handle that
>> problem.
>
> pgp signed patches?

Just one possibility. Submitting the patches via a web page that you
have to log into is another.
--
Jim Nasby jimn(at)enterprisedb(dot)com
EnterpriseDB http://enterprisedb.com 512.569.9461 (cell)