Tom Lane wrote:
> Bruce Momjian <pgman@candle.pha.pa.us> writes:
> > Tom Lane wrote:
> >> It's still gonna need a configure test and so on.
>
> > The submitter believes the C macro test is sufficient:
>
> Do I get to revert the patch the moment that fails in buildfarm?

In fact, I will revert it when it fails in the build farm. :-)

--
Bruce Momjian | http://candle.pha.pa.us
pgman@candle.pha.pa.us | (610) 359-1001
+ If your life is a hard drive, | 13 Roberts Road
+ Christ can be your backup. | Newtown Square, Pennsylvania 19073

---------------------------(end of broadcast)---------------------------
TIP 5: don't forget to increase your free space map settings

Tom Lane wrote:
> Bruce Momjian <pgman@candle.pha.pa.us> writes:
> > Tom Lane wrote:
> >> More to the point, the utility of the patch remains unproven.
> >> We are not in the habit of adding OS dependencies on speculation.
>
> > He ran tests, though it is speculation because non-caching is a pretty
> > hard thing to find a benefit from except under low memory situations.
>
> Well, the tests (a) didn't show any particularly good speedup, and
> (b) were not on the platforms that this is speculated to be useful on
> (ie, those without O_DIRECT).
>
> I really don't think that an adequate case has been made for adding
> a new OS dependency.

Well, I think the patch should be applied, and the submitter does too,
so unless I hear other votes, it is going in.

--
Bruce Momjian | http://candle.pha.pa.us
pgman@candle.pha.pa.us | (610) 359-1001
+ If your life is a hard drive, | 13 Roberts Road
+ Christ can be your backup. | Newtown Square, Pennsylvania 19073

---------------------------(end of broadcast)---------------------------
TIP 2: Don't 'kill -9' the postmaster

On Mon, Feb 13, 2006 at 02:59:43PM -0500, Bruce Momjian wrote:
> Tom Lane wrote:
> > Bruce Momjian <pgman@candle.pha.pa.us> writes:
> > > Tom Lane wrote:
> > >> More to the point, the utility of the patch remains unproven.
> > >> We are not in the habit of adding OS dependencies on speculation.
> >
> > > He ran tests, though it is speculation because non-caching is a pretty
> > > hard thing to find a benefit from except under low memory situations.
> >
> > Well, the tests (a) didn't show any particularly good speedup, and
> > (b) were not on the platforms that this is speculated to be useful on
> > (ie, those without O_DIRECT).
> >
> > I really don't think that an adequate case has been made for adding
> > a new OS dependency.
>
> Well, I think the patch should be applied, and the submitter does too,
> so unless I hear other votes, it is going in.

I vote no for whatever that is worth. A "performance" change needs to
actually demonstrate improved performance. If the change is really
desireable to clean up some messy code, then add it as a cleanup change
without the extra system calls. Otherwise it just adds one more bit of
mystery for future maintainers who may be decieved into thinking that posix
advise calls are important voodoo.

-dg

--
David Gould daveg@sonic.net
If simplicity worked, the world would be overrun with insects.

---------------------------(end of broadcast)---------------------------
TIP 9: In versions below 8.0, the planner will ignore your desire to
choose an index scan if your joining column's datatypes do not
match

daveg wrote:
> On Mon, Feb 13, 2006 at 02:59:43PM -0500, Bruce Momjian wrote:
> > Tom Lane wrote:
> > > Bruce Momjian <pgman@candle.pha.pa.us> writes:
> > > > Tom Lane wrote:
> > > >> More to the point, the utility of the patch remains unproven.
> > > >> We are not in the habit of adding OS dependencies on speculation.
> > >
> > > > He ran tests, though it is speculation because non-caching is a pretty
> > > > hard thing to find a benefit from except under low memory situations.
> > >
> > > Well, the tests (a) didn't show any particularly good speedup, and
> > > (b) were not on the platforms that this is speculated to be useful on
> > > (ie, those without O_DIRECT).
> > >
> > > I really don't think that an adequate case has been made for adding
> > > a new OS dependency.
> >
> > Well, I think the patch should be applied, and the submitter does too,
> > so unless I hear other votes, it is going in.
>
> I vote no for whatever that is worth. A "performance" change needs to
> actually demonstrate improved performance. If the change is really
> desireable to clean up some messy code, then add it as a cleanup change
> without the extra system calls. Otherwise it just adds one more bit of
> mystery for future maintainers who may be decieved into thinking that posix
> advise calls are important voodoo.

Yes, your vote counts very much. What if I apply the patch, but mark
the posix_advise() call in a NOT_USED macro block, so it will be ready
for people to test, but will not be used until we are sure.

--
Bruce Momjian | http://candle.pha.pa.us
pgman@candle.pha.pa.us | (610) 359-1001
+ If your life is a hard drive, | 13 Roberts Road
+ Christ can be your backup. | Newtown Square, Pennsylvania 19073

---------------------------(end of broadcast)---------------------------
TIP 9: In versions below 8.0, the planner will ignore your desire to
choose an index scan if your joining column's datatypes do not
match

Bruce Momjian <pgman@candle.pha.pa.us> writes:
> Yes, your vote counts very much. What if I apply the patch, but mark
> the posix_advise() call in a NOT_USED macro block, so it will be ready
> for people to test, but will not be used until we are sure.

Sounds like a recipe for ensuring it never will be tested. What's
needed here is some actual tests, not preparation...

regards, tom lane

---------------------------(end of broadcast)---------------------------
TIP 2: Don't 'kill -9' the postmaster

Tom Lane wrote:
> Bruce Momjian <pgman@candle.pha.pa.us> writes:
>
>>Yes, your vote counts very much. What if I apply the patch, but mark
>>the posix_advise() call in a NOT_USED macro block, so it will be ready
>>for people to test, but will not be used until we are sure.
>
>
> Sounds like a recipe for ensuring it never will be tested. What's
> needed here is some actual tests, not preparation...
>

Gotta second that.

Does the OP have a test scenario that those of us with appropriate OS's
could try? Come to think of it, what are the appropriate OS's? (I see
NetBSD mentioned so I suppose all the *BSDs, but what others?).

Cheers

Mark

---------------------------(end of broadcast)---------------------------
TIP 1: if posting/reading through Usenet, please send an appropriate
subscribe-nomail command to majordomo@postgresql.org so that your
message can get through to the mailing list cleanly

Mark Kirkwood wrote:
> Come to think of it, what are the appropriate OS's? (I see
> NetBSD mentioned so I suppose all the *BSDs, but what others?).
>

FWIW FreeBSD (6.0) does *not* have posix_fadvise, only posix_madvise.

regards

Mark


---------------------------(end of broadcast)---------------------------
TIP 1: if posting/reading through Usenet, please send an appropriate
subscribe-nomail command to majordomo@postgresql.org so that your
message can get through to the mailing list cleanly

On Mon, 2006-02-13 at 23:33 -0500, Tom Lane wrote:
> Bruce Momjian <pgman@candle.pha.pa.us> writes:
> > Yes, your vote counts very much. What if I apply the patch, but mark
> > the posix_advise() call in a NOT_USED macro block, so it will be ready
> > for people to test, but will not be used until we are sure.
>
> Sounds like a recipe for ensuring it never will be tested. What's
> needed here is some actual tests, not preparation...

Without discussing this particular patch, IMHO we need a clear checklist
of items that are required before a patch is accepted onto the patches
awaiting application list.

We seem to have had a few patches rejected on grounds that could have
been picked up by other people, not just Tom and Bruce. This wastes
everybody's time because the patch writer might have fixed whatever was
wrong with it a while back if the patch had been rejected earlier. It
will also eventually bring the process into disrepute if one accepts
patches onto the queue and then another raises major objections that
could easily have been rectified earlier.

So let's agree a checklist beforehand. That way patch submitters can be
told to resubmit much earlier by other list watchers, with less wasted
time (elapsed and from core folk) and perhaps a gentler experience for
first-time submitters. Control can and should still lie with committers.

Suggested checklist:
1. has patch been discussed previously? Y/N
- if Y, give direct link to archive of message, and/or bug#
- if N discuss on appropriate list, or expect to be rejected
2. patch target: cvstip or stated branch(es)
3. patch application location: root directory only
4. patch format: diff -c only
5. confirm licence is BSD: Y/N
6. port specific: Y/N, if Y list ports
7. confirm passes make check (on listed ports)
8. provide implementation overview, preferably in code comments
9. if it is a performance patch, provide confirming test results. It is
OK to post patches without these, though the patch will not be applied
until *somebody* has tested the patches and found a valuable performance
effect directly attributable to the patch.
10. If it is a new feature patch, confirm that it has been tested for
all desired scenarios. If it has not, this should be clearly stated as a
request for a particular kind of test to be performed. Note that the
patch will go no further until that test has been performed.
11. if it is a new feature patch, does it break any existing defaults?
Explain why this is *required* or patch will be rejected. New feature
patches should be accompanied by doc patches also.
12. Even if you pass all of the above, the patch may still be rejected
for other technical reasons. You should be prepared to listen to
comments received and perform any agreed rework. Even if you have
received positive comments from some community members, others may spot
problems with your approach, coding style or many other issues.
13. Successful patches will be notified to you by email and you will be
credited for that work in the next set of release notes.

I would also suggest that we have two patch queues:
- patches awaiting performance testing
- patches awaiting application (current one)

That way anybody wanting to test new performance add-ons can do so and
reply to the list with confirmation that the patch can now be added to
the second (main) list of patches.

Of course, this suggestion will be immediately rejected because it
wasn't discussed on -hackers ;-)

Best Regards, Simon Riggs


---------------------------(end of broadcast)---------------------------
TIP 6: explain analyze is your friend

Mark Kirkwood <markir@paradise.net.nz> writes:
> Tom Lane wrote:
>> Sounds like a recipe for ensuring it never will be tested. What's
>> needed here is some actual tests, not preparation...

> Does the OP have a test scenario that those of us with appropriate OS's
> could try? Come to think of it, what are the appropriate OS's? (I see
> NetBSD mentioned so I suppose all the *BSDs, but what others?).

The test run by the OP was just pgbench, which is probably not the
greatest scenario for showing the benefits of this patch, but at least
it's neutral ground. You need a situation in which the kernel is under
memory stress, else early free of disk cache buffers isn't going to make
any difference whatever --- so choose a pgbench scale factor that makes
the database noticeably larger than the test machine's RAM. Other than
that, follow the usual guidelines for producing trustworthy pgbench
numbers: number of clients smaller than scale factor, number of
transactions per client at least 1000 or so (to eliminate startup
transients), repeat test a couple times to make sure numbers are
reproducible.

regards, tom lane

---------------------------(end of broadcast)---------------------------
TIP 5: don't forget to increase your free space map settings

On Tue, 2006-02-14 at 12:54 +0000, Simon Riggs wrote:
> On Mon, 2006-02-13 at 23:33 -0500, Tom Lane wrote:
> > Bruce Momjian <pgman@candle.pha.pa.us> writes:
> > > Yes, your vote counts very much. What if I apply the patch, but mark
> > > the posix_advise() call in a NOT_USED macro block, so it will be ready
> > > for people to test, but will not be used until we are sure.
> >
> > Sounds like a recipe for ensuring it never will be tested. What's
> > needed here is some actual tests, not preparation...
>
> Without discussing this particular patch, IMHO we need a clear checklist
> of items that are required before a patch is accepted onto the patches
> awaiting application list.

This was supposed to be a serious suggestion, so apologies if this came
across stronger than it was meant.

The onus is of course upon the patch submitter to improve their game,
but there seems only benefit in setting out the (simpler) rules of the
game to show people what is unacceptable, even before they submit.

Best Regards, Simon Riggs



---------------------------(end of broadcast)---------------------------
TIP 2: Don't 'kill -9' the postmaster