Thread: Re: [PATCHES] Trivial patch to double vacuum speed on tables with no indexes
stark <stark@enterprisedb.com> writes:
> There isn't really any need for the second pass in lazy vacuum if the table
> has no indexes.
How often does that case come up in the real world, for tables that are
large enough that you'd care about vacuum performance?
regards, tom lane
Re: [PATCHES] Trivial patch to double vacuum speed on tables with no indexes
From
Gregory Stark
Date:
Tom Lane <tgl@sss.pgh.pa.us> writes: > stark <stark@enterprisedb.com> writes: >> There isn't really any need for the second pass in lazy vacuum if the table >> has no indexes. > > How often does that case come up in the real world, for tables that are > large enough that you'd care about vacuum performance? Admittedly it's not the most common scenario. But it does come up. ETL applications for example that load data, then perform some manipulation of the data before loading the data. If they have many updates to do they'll probably have to do vacuums between some of them. Arguably if you don't have any indexes on a large table it's quite likely to be *because* you're planning on doing some big updates such that it'll be faster to simply rebuild the indexes when you're done anyways. I would have had the same objection if it resulted in substantially more complex code but it was so simple that it doesn't seem like a concern. -- Gregory Stark EnterpriseDB http://www.enterprisedb.com
Gregory Stark <stark@enterprisedb.com> writes:
> Tom Lane <tgl@sss.pgh.pa.us> writes:
>> How often does that case come up in the real world, for tables that are
>> large enough that you'd care about vacuum performance?
> I would have had the same objection if it resulted in substantially more
> complex code but it was so simple that it doesn't seem like a concern.
The reason the patch is so short is that it's a kluge. If we really
cared about supporting this case, more wide-ranging changes would be
needed (eg, there's no need to eat maintenance_work_mem worth of RAM
for the dead-TIDs array); and a decent respect to the opinions of
mankind would require some attention to updating the header comments
and function descriptions, too.
regards, tom lane
Re: [PATCHES] Trivial patch to double vacuum speed on tables with no indexes
From
Gregory Stark
Date:
Tom Lane <tgl@sss.pgh.pa.us> writes:
> The reason the patch is so short is that it's a kluge. If we really
> cared about supporting this case, more wide-ranging changes would be
> needed (eg, there's no need to eat maintenance_work_mem worth of RAM
> for the dead-TIDs array); and a decent respect to the opinions of
> mankind would require some attention to updating the header comments
> and function descriptions, too.
The only part that seems klugy to me is how it releases the lock and
reacquires it rather than wait in the first place until it can acquire the
lock. Fixed that and changed lazy_space_alloc to allocate only as much space
as is really necessary.
Gosh, I've never been accused of offending all mankind before.
--- vacuumlazy.c 31 Jul 2006 21:09:00 +0100 1.76
+++ vacuumlazy.c 28 Aug 2006 09:58:41 +0100
@@ -16,6 +16,10 @@
* perform a pass of index cleanup and page compaction, then resume the heap
* scan with an empty TID array.
*
+ * As a special exception if we're processing a table with no indexes we can
+ * vacuum each page as we go so we don't need to allocate more space than
+ * enough to hold as many heap tuples fit on one page.
+ *
* We can limit the storage for page free space to MaxFSMPages entries,
* since that's the most the free space map will be willing to remember
* anyway. If the relation has fewer than that many pages with free space,
@@ -106,7 +110,7 @@
TransactionId OldestXmin);
static BlockNumber count_nondeletable_pages(Relation onerel,
LVRelStats *vacrelstats, TransactionId OldestXmin);
-static void lazy_space_alloc(LVRelStats *vacrelstats, BlockNumber relblocks);
+static void lazy_space_alloc(LVRelStats *vacrelstats, BlockNumber relblocks, unsigned nindexes);
static void lazy_record_dead_tuple(LVRelStats *vacrelstats,
ItemPointer itemptr);
static void lazy_record_free_space(LVRelStats *vacrelstats,
@@ -206,7 +210,8 @@
* This routine sets commit status bits, builds lists of dead tuples
* and pages with free space, and calculates statistics on the number
* of live tuples in the heap. When done, or when we run low on space
- * for dead-tuple TIDs, invoke vacuuming of indexes and heap.
+ * for dead-tuple TIDs, or after every page if the table has no indexes
+ * invoke vacuuming of indexes and heap.
*
* It also updates the minimum Xid found anywhere on the table in
* vacrelstats->minxid, for later storing it in pg_class.relminxid.
@@ -247,7 +252,7 @@
vacrelstats->rel_pages = nblocks;
vacrelstats->nonempty_pages = 0;
- lazy_space_alloc(vacrelstats, nblocks);
+ lazy_space_alloc(vacrelstats, nblocks, nindexes);
for (blkno = 0; blkno < nblocks; blkno++)
{
@@ -282,8 +287,14 @@
buf = ReadBuffer(onerel, blkno);
- /* In this phase we only need shared access to the buffer */
- LockBuffer(buf, BUFFER_LOCK_SHARE);
+ /* In this phase we only need shared access to the buffer unless we're
+ * going to do the vacuuming now which we do if there are no indexes
+ */
+
+ if (nindexes)
+ LockBuffer(buf, BUFFER_LOCK_SHARE);
+ else
+ LockBufferForCleanup(buf);
page = BufferGetPage(buf);
@@ -450,6 +461,12 @@
{
lazy_record_free_space(vacrelstats, blkno,
PageGetFreeSpace(page));
+ } else if (!nindexes) {
+ /* If there are no indexes we can vacuum the page right now instead
+ * of doing a second scan */
+ lazy_vacuum_page(onerel, blkno, buf, 0, vacrelstats);
+ lazy_record_free_space(vacrelstats, blkno, PageGetFreeSpace(BufferGetPage(buf)));
+ vacrelstats->num_dead_tuples = 0;
}
/* Remember the location of the last page with nonremovable tuples */
@@ -891,16 +908,20 @@
* See the comments at the head of this file for rationale.
*/
static void
-lazy_space_alloc(LVRelStats *vacrelstats, BlockNumber relblocks)
+lazy_space_alloc(LVRelStats *vacrelstats, BlockNumber relblocks, unsigned nindexes)
{
long maxtuples;
int maxpages;
- maxtuples = (maintenance_work_mem * 1024L) / sizeof(ItemPointerData);
- maxtuples = Min(maxtuples, INT_MAX);
- maxtuples = Min(maxtuples, MaxAllocSize / sizeof(ItemPointerData));
- /* stay sane if small maintenance_work_mem */
- maxtuples = Max(maxtuples, MaxHeapTuplesPerPage);
+ if (nindexes) {
+ maxtuples = (maintenance_work_mem * 1024L) / sizeof(ItemPointerData);
+ maxtuples = Min(maxtuples, INT_MAX);
+ maxtuples = Min(maxtuples, MaxAllocSize / sizeof(ItemPointerData));
+ /* stay sane if small maintenance_work_mem */
+ maxtuples = Max(maxtuples, MaxHeapTuplesPerPage);
+ } else {
+ maxtuples = MaxHeapTuplesPerPage;
+ }
vacrelstats->num_dead_tuples = 0;
vacrelstats->max_dead_tuples = (int) maxtuples;
--
Gregory Stark
EnterpriseDB http://www.enterprisedb.com
Re: [PATCHES] Trivial patch to double vacuum speed on tables with no indexes
From
Alvaro Herrera
Date:
Gregory Stark wrote: > > Tom Lane <tgl@sss.pgh.pa.us> writes: > > > The reason the patch is so short is that it's a kluge. If we really > > cared about supporting this case, more wide-ranging changes would be > > needed (eg, there's no need to eat maintenance_work_mem worth of RAM > > for the dead-TIDs array); and a decent respect to the opinions of > > mankind would require some attention to updating the header comments > > and function descriptions, too. > > The only part that seems klugy to me is how it releases the lock and > reacquires it rather than wait in the first place until it can acquire the > lock. Fixed that and changed lazy_space_alloc to allocate only as much space > as is really necessary. > > Gosh, I've never been accused of offending all mankind before. Does that feel good or bad? I won't comment on the spirit of the patch but I'll observe that you should respect mankind a little more by observing brace position in if/else ;-) -- Alvaro Herrera http://www.CommandPrompt.com/ The PostgreSQL Company - Command Prompt, Inc.
Patch applied. Thanks.
---------------------------------------------------------------------------
Gregory Stark wrote:
>
> Tom Lane <tgl@sss.pgh.pa.us> writes:
>
> > The reason the patch is so short is that it's a kluge. If we really
> > cared about supporting this case, more wide-ranging changes would be
> > needed (eg, there's no need to eat maintenance_work_mem worth of RAM
> > for the dead-TIDs array); and a decent respect to the opinions of
> > mankind would require some attention to updating the header comments
> > and function descriptions, too.
>
> The only part that seems klugy to me is how it releases the lock and
> reacquires it rather than wait in the first place until it can acquire the
> lock. Fixed that and changed lazy_space_alloc to allocate only as much space
> as is really necessary.
>
> Gosh, I've never been accused of offending all mankind before.
>
>
>
> --- vacuumlazy.c 31 Jul 2006 21:09:00 +0100 1.76
> +++ vacuumlazy.c 28 Aug 2006 09:58:41 +0100
> @@ -16,6 +16,10 @@
> * perform a pass of index cleanup and page compaction, then resume the heap
> * scan with an empty TID array.
> *
> + * As a special exception if we're processing a table with no indexes we can
> + * vacuum each page as we go so we don't need to allocate more space than
> + * enough to hold as many heap tuples fit on one page.
> + *
> * We can limit the storage for page free space to MaxFSMPages entries,
> * since that's the most the free space map will be willing to remember
> * anyway. If the relation has fewer than that many pages with free space,
> @@ -106,7 +110,7 @@
> TransactionId OldestXmin);
> static BlockNumber count_nondeletable_pages(Relation onerel,
> LVRelStats *vacrelstats, TransactionId OldestXmin);
> -static void lazy_space_alloc(LVRelStats *vacrelstats, BlockNumber relblocks);
> +static void lazy_space_alloc(LVRelStats *vacrelstats, BlockNumber relblocks, unsigned nindexes);
> static void lazy_record_dead_tuple(LVRelStats *vacrelstats,
> ItemPointer itemptr);
> static void lazy_record_free_space(LVRelStats *vacrelstats,
> @@ -206,7 +210,8 @@
> * This routine sets commit status bits, builds lists of dead tuples
> * and pages with free space, and calculates statistics on the number
> * of live tuples in the heap. When done, or when we run low on space
> - * for dead-tuple TIDs, invoke vacuuming of indexes and heap.
> + * for dead-tuple TIDs, or after every page if the table has no indexes
> + * invoke vacuuming of indexes and heap.
> *
> * It also updates the minimum Xid found anywhere on the table in
> * vacrelstats->minxid, for later storing it in pg_class.relminxid.
> @@ -247,7 +252,7 @@
> vacrelstats->rel_pages = nblocks;
> vacrelstats->nonempty_pages = 0;
>
> - lazy_space_alloc(vacrelstats, nblocks);
> + lazy_space_alloc(vacrelstats, nblocks, nindexes);
>
> for (blkno = 0; blkno < nblocks; blkno++)
> {
> @@ -282,8 +287,14 @@
>
> buf = ReadBuffer(onerel, blkno);
>
> - /* In this phase we only need shared access to the buffer */
> - LockBuffer(buf, BUFFER_LOCK_SHARE);
> + /* In this phase we only need shared access to the buffer unless we're
> + * going to do the vacuuming now which we do if there are no indexes
> + */
> +
> + if (nindexes)
> + LockBuffer(buf, BUFFER_LOCK_SHARE);
> + else
> + LockBufferForCleanup(buf);
>
> page = BufferGetPage(buf);
>
> @@ -450,6 +461,12 @@
> {
> lazy_record_free_space(vacrelstats, blkno,
> PageGetFreeSpace(page));
> + } else if (!nindexes) {
> + /* If there are no indexes we can vacuum the page right now instead
> + * of doing a second scan */
> + lazy_vacuum_page(onerel, blkno, buf, 0, vacrelstats);
> + lazy_record_free_space(vacrelstats, blkno, PageGetFreeSpace(BufferGetPage(buf)));
> + vacrelstats->num_dead_tuples = 0;
> }
>
> /* Remember the location of the last page with nonremovable tuples */
> @@ -891,16 +908,20 @@
> * See the comments at the head of this file for rationale.
> */
> static void
> -lazy_space_alloc(LVRelStats *vacrelstats, BlockNumber relblocks)
> +lazy_space_alloc(LVRelStats *vacrelstats, BlockNumber relblocks, unsigned nindexes)
> {
> long maxtuples;
> int maxpages;
>
> - maxtuples = (maintenance_work_mem * 1024L) / sizeof(ItemPointerData);
> - maxtuples = Min(maxtuples, INT_MAX);
> - maxtuples = Min(maxtuples, MaxAllocSize / sizeof(ItemPointerData));
> - /* stay sane if small maintenance_work_mem */
> - maxtuples = Max(maxtuples, MaxHeapTuplesPerPage);
> + if (nindexes) {
> + maxtuples = (maintenance_work_mem * 1024L) / sizeof(ItemPointerData);
> + maxtuples = Min(maxtuples, INT_MAX);
> + maxtuples = Min(maxtuples, MaxAllocSize / sizeof(ItemPointerData));
> + /* stay sane if small maintenance_work_mem */
> + maxtuples = Max(maxtuples, MaxHeapTuplesPerPage);
> + } else {
> + maxtuples = MaxHeapTuplesPerPage;
> + }
>
> vacrelstats->num_dead_tuples = 0;
> vacrelstats->max_dead_tuples = (int) maxtuples;
> --
> Gregory Stark
> EnterpriseDB http://www.enterprisedb.com
>
> ---------------------------(end of broadcast)---------------------------
> TIP 4: Have you searched our list archives?
>
> http://archives.postgresql.org
--
Bruce Momjian bruce@momjian.us
EnterpriseDB http://www.enterprisedb.com
+ If your life is a hard drive, Christ can be your backup. +
Bruce Momjian <bruce@momjian.us> writes:
> Patch applied. Thanks.
Wait a minute. This patch changes the behavior so that
LockBufferForCleanup is applied to *every* heap page, not only the ones
where there are removable tuples. It's not hard to imagine scenarios
where that results in severe system-wide performance degradation.
Has there been any real-world testing of this idea?
regards, tom lane
Tom Lane wrote: > Bruce Momjian <bruce@momjian.us> writes: > > Patch applied. Thanks. > > Wait a minute. This patch changes the behavior so that > LockBufferForCleanup is applied to *every* heap page, not only the ones > where there are removable tuples. It's not hard to imagine scenarios > where that results in severe system-wide performance degradation. > Has there been any real-world testing of this idea? I see the no-index case now: + if (nindexes) + LockBuffer(buf, BUFFER_LOCK_SHARE); + else + LockBufferForCleanup(buf); Let's see what Greg says, or revert. -- Bruce Momjian bruce@momjian.us EnterpriseDB http://www.enterprisedb.com + If your life is a hard drive, Christ can be your backup. +
Re: [PATCHES] Trivial patch to double vacuum speed on tables with no indexes
From
Gregory Stark
Date:
Bruce Momjian <bruce@momjian.us> writes: > Tom Lane wrote: >> Bruce Momjian <bruce@momjian.us> writes: >> > Patch applied. Thanks. >> >> Wait a minute. This patch changes the behavior so that >> LockBufferForCleanup is applied to *every* heap page, not only the ones >> where there are removable tuples. It's not hard to imagine scenarios >> where that results in severe system-wide performance degradation. >> Has there been any real-world testing of this idea? > > I see the no-index case now: > > + if (nindexes) > + LockBuffer(buf, BUFFER_LOCK_SHARE); > + else > + LockBufferForCleanup(buf); > > Let's see what Greg says, or revert. Hm, that's a good point. I could return it to the original method where it released the share lock and did he LockBufferForCleanup only if necessary. I thought it was awkward to acquire a lock then release it to acquire a different lock on the same buffer but it's true that it doesn't always have to acquire the second lock. -- Gregory Stark EnterpriseDB http://www.enterprisedb.com
Re: [PATCHES] Trivial patch to double vacuum speed on tables with no indexes
From
Alvaro Herrera
Date:
Gregory Stark wrote: > > Bruce Momjian <bruce@momjian.us> writes: > > > Tom Lane wrote: > >> Bruce Momjian <bruce@momjian.us> writes: > >> > Patch applied. Thanks. > >> > >> Wait a minute. This patch changes the behavior so that > >> LockBufferForCleanup is applied to *every* heap page, not only the ones > >> where there are removable tuples. It's not hard to imagine scenarios > >> where that results in severe system-wide performance degradation. > >> Has there been any real-world testing of this idea? > > > > I see the no-index case now: > > > > + if (nindexes) > > + LockBuffer(buf, BUFFER_LOCK_SHARE); > > + else > > + LockBufferForCleanup(buf); > > > > Let's see what Greg says, or revert. > > Hm, that's a good point. I could return it to the original method where it > released the share lock and did he LockBufferForCleanup only if necessary. I > thought it was awkward to acquire a lock then release it to acquire a > different lock on the same buffer but it's true that it doesn't always have to > acquire the second lock. This rush to apply patches just because no one seems to be capable of keeping up with them not being reviewed, is starting to get a bit worrisome. -- Alvaro Herrera http://www.CommandPrompt.com/ PostgreSQL Replication, Consulting, Custom Development, 24x7 support
Alvaro Herrera wrote: > > > I see the no-index case now: > > > > > > + if (nindexes) > > > + LockBuffer(buf, BUFFER_LOCK_SHARE); > > > + else > > > + LockBufferForCleanup(buf); > > > > > > Let's see what Greg says, or revert. > > > > Hm, that's a good point. I could return it to the original method where it > > released the share lock and did he LockBufferForCleanup only if necessary. I > > thought it was awkward to acquire a lock then release it to acquire a > > different lock on the same buffer but it's true that it doesn't always have to > > acquire the second lock. > > This rush to apply patches just because no one seems to be capable of > keeping up with them not being reviewed, is starting to get a bit > worrisome. When things are placed in the patches queue, I need to get feedback if there is a problem with them. I am not sure what other process we can follow, unless we just keep patches there indefinitely, or just ignore them and never place them in the queue. -- Bruce Momjian bruce@momjian.us EnterpriseDB http://www.enterprisedb.com + If your life is a hard drive, Christ can be your backup. +
Bruce Momjian <bruce@momjian.us> writes:
> Alvaro Herrera wrote:
>> This rush to apply patches just because no one seems to be capable of
>> keeping up with them not being reviewed, is starting to get a bit
>> worrisome.
> When things are placed in the patches queue, I need to get feedback if
> there is a problem with them. I am not sure what other process we can
> follow, unless we just keep patches there indefinitely, or just ignore
> them and never place them in the queue.
The problem with the process you're using is that it defaults to
applying patches --- and in fact, lately it seems like it takes a
threat of mayhem to prevent you from applying a patch.
Now, apply-unless-objected-to was the right default back in 1997, when
the code was in bad enough shape that it was hard to make it worse ;-).
I do not believe it's the right default anymore though. The system is a
lot larger and more complicated than it was when it left Berkeley, and
our quality standards are an order of magnitude higher too. We need to
default to *not* applying patches until they've passed some amount of
review.
I don't want to be too hard-nosed about this, since the last thing we
need is another level of bureaucracy added to our processes, especially
for simple trivial stuff. But when there's been some discussion or
objection to a patch, ISTM that just because the patch submitter has
put up a second version should not mean that it's okay to apply. At
that point some actual review is needed.
I'm also having a bit of a problem with the silence-means-assent rule.
Most of the time I'm OK with it, but right now I simply don't have the
time to look at everything that comes in as soon as it comes in;
especially not second versions of patches.
I don't have a concrete proposal to make, but I do think that the
current patch-queue process is not suited to the project as it stands
today. Maybe if this issue-tracking stuff gets off the ground, we
could let developers place ACK or NAK flags on patches they've looked
at, and have some rule about ACK-vs-NAK requirements for something to go
in.
regards, tom lane
> I don't have a concrete proposal to make, but I do think that the > current patch-queue process is not suited to the project as it stands > today. Maybe if this issue-tracking stuff gets off the ground, we > could let developers place ACK or NAK flags on patches they've looked > at, and have some rule about ACK-vs-NAK requirements for something to go > in. How about *requiring* test cases that prove the patch? Sincerely, Joshua D. Drake > > regards, tom lane > > ---------------------------(end of broadcast)--------------------------- > TIP 2: Don't 'kill -9' the postmaster > -- === The PostgreSQL Company: Command Prompt, Inc. === Sales/Support: +1.503.667.4564 || 24x7/Emergency: +1.800.492.2240 Providing the most comprehensive PostgreSQL solutionssince 1997 http://www.commandprompt.com/
Tom Lane wrote: > Bruce Momjian <bruce@momjian.us> writes: > > Alvaro Herrera wrote: > >> This rush to apply patches just because no one seems to be capable of > >> keeping up with them not being reviewed, is starting to get a bit > >> worrisome. > > > When things are placed in the patches queue, I need to get feedback if > > there is a problem with them. I am not sure what other process we can > > follow, unless we just keep patches there indefinitely, or just ignore > > them and never place them in the queue. > > The problem with the process you're using is that it defaults to > applying patches --- and in fact, lately it seems like it takes a > threat of mayhem to prevent you from applying a patch. I am just trying to keep everyone happy, the developers, user community, and patch submitters. > Now, apply-unless-objected-to was the right default back in 1997, when > the code was in bad enough shape that it was hard to make it worse ;-). > I do not believe it's the right default anymore though. The system is a > lot larger and more complicated than it was when it left Berkeley, and > our quality standards are an order of magnitude higher too. We need to > default to *not* applying patches until they've passed some amount of > review. > > I don't want to be too hard-nosed about this, since the last thing we > need is another level of bureaucracy added to our processes, especially > for simple trivial stuff. But when there's been some discussion or > objection to a patch, ISTM that just because the patch submitter has > put up a second version should not mean that it's okay to apply. At > that point some actual review is needed. > > I'm also having a bit of a problem with the silence-means-assent rule. > Most of the time I'm OK with it, but right now I simply don't have the > time to look at everything that comes in as soon as it comes in; > especially not second versions of patches. > > I don't have a concrete proposal to make, but I do think that the > current patch-queue process is not suited to the project as it stands > today. Maybe if this issue-tracking stuff gets off the ground, we > could let developers place ACK or NAK flags on patches they've looked > at, and have some rule about ACK-vs-NAK requirements for something to go > in. Yes, I realize this is a very hard time. I know you were pushing for end-of-week beta, and though I don't think we can hit that date, I am trying to move things along. I agree it is that second version of the patch that often doesn't get the thorough review. Should I increase the amount of time something is in the queue, or ask for someone to state it is OK to apply, and just keep asking until I get a "yes" from someone? I can do that pretty easily. -- Bruce Momjian bruce@momjian.us EnterpriseDB http://www.enterprisedb.com + If your life is a hard drive, Christ can be your backup. +
Joshua D. Drake wrote: > > > I don't have a concrete proposal to make, but I do think that the > > current patch-queue process is not suited to the project as it stands > > today. Maybe if this issue-tracking stuff gets off the ground, we > > could let developers place ACK or NAK flags on patches they've looked > > at, and have some rule about ACK-vs-NAK requirements for something to go > > in. > > How about *requiring* test cases that prove the patch? That doesn't hit most of the failures, which can be portability, performance, or missing features, or bad style. -- Bruce Momjian bruce@momjian.us EnterpriseDB http://www.enterprisedb.com + If your life is a hard drive, Christ can be your backup. +
"Joshua D. Drake" <jd@commandprompt.com> writes:
> How about *requiring* test cases that prove the patch?
There are lots and lots of things that can't really be tested with
pg_regress-based tests, and in any case a test does not prove the
absence of bugs; particularly not a test devised by the code author,
since almost by definition it won't test cases he didn't think of.
Certainly test cases have their place, but I have a lot more faith in
code-reading by knowledgeable developers as a way to spot problems
that got past the original author.
regards, tom lane
On Mon, 4 Sep 2006, Joshua D. Drake wrote: > > > I don't have a concrete proposal to make, but I do think that the > > current patch-queue process is not suited to the project as it stands > > today. Maybe if this issue-tracking stuff gets off the ground, we > > could let developers place ACK or NAK flags on patches they've looked > > at, and have some rule about ACK-vs-NAK requirements for something to go > > in. > > How about *requiring* test cases that prove the patch? People including regression tests is not a replacement for code review. For a non-trivial patch, an SQL test will only exercise a few code paths. Moreover, it wont say anything about code quality, maintainability or general correctness or completeness. It will still have to be reviewed. Thanks Gavin
Gavin Sherry wrote: > On Mon, 4 Sep 2006, Joshua D. Drake wrote: > >>> I don't have a concrete proposal to make, but I do think that the >>> current patch-queue process is not suited to the project as it stands >>> today. Maybe if this issue-tracking stuff gets off the ground, we >>> could let developers place ACK or NAK flags on patches they've looked >>> at, and have some rule about ACK-vs-NAK requirements for something to go >>> in. >> How about *requiring* test cases that prove the patch? > > People including regression tests is not a replacement for code review. Uhmmm, of course not? :). A test case does however help show that the person thought through what they were doing :) Even if they were cranked in the process. Sincerely, Joshua D. Drake > For a non-trivial patch, an SQL test will only exercise a few code paths . > Moreover, it wont say anything about code quality, maintainability or > general correctness or completeness. It will still have to be reviewed. > > Thanks > > Gavin > > ---------------------------(end of broadcast)--------------------------- > TIP 2: Don't 'kill -9' the postmaster > -- === The PostgreSQL Company: Command Prompt, Inc. === Sales/Support: +1.503.667.4564 || 24x7/Emergency: +1.800.492.2240 Providing the most comprehensive PostgreSQL solutionssince 1997 http://www.commandprompt.com/
Ühel kenal päeval, E, 2006-09-04 kell 16:51, kirjutas Joshua D. Drake: > > I don't have a concrete proposal to make, but I do think that the > > current patch-queue process is not suited to the project as it stands > > today. Maybe if this issue-tracking stuff gets off the ground, we > > could let developers place ACK or NAK flags on patches they've looked > > at, and have some rule about ACK-vs-NAK requirements for something to go > > in. > > How about *requiring* test cases that prove the patch? Test cases are good for checking if some latter patch does not break the functionality provided by current patch. They are almost useless for proving that current patch is correct. > Sincerely, > > Joshua D. Drake > > > > > > regards, tom lane > > > > ---------------------------(end of broadcast)--------------------------- > > TIP 2: Don't 'kill -9' the postmaster > > > > -- ---------------- Hannu Krosing Database Architect Skype Technologies OÜ Akadeemia tee 21 F, Tallinn, 12618, Estonia Skype me: callto:hkrosing Get Skype for free: http://www.skype.com