From: Simon Riggs on

On Thu, 2009-09-17 at 09:54 +0300, Heikki Linnakangas wrote:

> > This is a pretty small CommitFest, so there
> > shouldn't be any shortage of reviewers, though Heikki's time may be
> > stretched a little thin, since Streaming Replication is also in the
> > queue, and he is working on index-only scans. That's really for him
> > to comment on, though.
>
> I'm going to put the index-only scans aside for now to focus on hot
> standby and streaming replication. Both are big patches, so there's
> plenty of work in those two alone, and not only for me.

That's very good of you, thanks.

It was already clear to a few people that your time would bottleneck
trying to review both at the same time. I personally wasn't expecting
you to jump into immediate action on HS. We have time.

--
Simon Riggs www.2ndQuadrant.com


--
Sent via pgsql-hackers mailing list (pgsql-hackers(a)postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

From: Robert Haas on
On Thu, Sep 17, 2009 at 2:54 AM, Heikki Linnakangas
<heikki.linnakangas(a)enterprisedb.com> wrote:
> Robert Haas wrote:
>> On Wed, Sep 16, 2009 at 6:05 PM, Josh Berkus <josh(a)agliodbs.com> wrote:
>>> Now that Simon has submitted this, can some of the heavy-hitters here
>>> review it?  Heikki?
>>>
>>> Nobody's name is next to it.
>>
>> I don't think anyone is planning to ignore this patch, but it wasn't
>> included in the first round of round-robin reviewing assignments
>> because it wasn't submitted until the following day, after the
>> announced deadline for submissions had already passed.  So most people
>> are probably busy with with some other patch at the moment, but that's
>> a temporary phenomenon.
>
> Right, I've added myself as reviewer now.
>
>>  This is a pretty small CommitFest, so there
>> shouldn't be any shortage of reviewers, though Heikki's time may be
>> stretched a little thin, since Streaming Replication is also in the
>> queue, and he is working on index-only scans.  That's really for him
>> to comment on, though.
>
> I'm going to put the index-only scans aside for now to focus on hot
> standby and streaming replication. Both are big patches, so there's
> plenty of work in those two alone, and not only for me.

What is the best way to attack this? Should I keep reviewing
index-only scans so that you have feedback for when you get back to
it, or should I move on to something else? If something else, does it
make more sense for me to look at HS since I did a bit of work with a
previous version, or would it be better for me to just pick one of the
other patches from the CommitFest and work on that?

Also, stepping back from me personally, should we try to assign some
additional reviewers to these patches? Is there some way we can
divide up review tasks among multiple people so that we're not
repeating each others work?

Thoughts appreciated, from Heikki, Simon, or others.

....Robert

--
Sent via pgsql-hackers mailing list (pgsql-hackers(a)postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

From: Simon Riggs on

On Thu, 2009-09-17 at 19:01 -0400, Robert Haas wrote:
> >
> > I'm going to put the index-only scans aside for now to focus on hot
> > standby and streaming replication. Both are big patches, so there's
> > plenty of work in those two alone, and not only for me.
>
> What is the best way to attack this? Should I keep reviewing
> index-only scans so that you have feedback for when you get back to
> it, or should I move on to something else? If something else, does it
> make more sense for me to look at HS since I did a bit of work with a
> previous version, or would it be better for me to just pick one of the
> other patches from the CommitFest and work on that?
>
> Also, stepping back from me personally, should we try to assign some
> additional reviewers to these patches? Is there some way we can
> divide up review tasks among multiple people so that we're not
> repeating each others work?
>
> Thoughts appreciated, from Heikki, Simon, or others.

I think this is a great opportunity to widen the pool of people
contributing to reviews.

I suggest the creation of a second group of people, performing
round-robin testing of patches. These people would be able to verify
* documentation matches implemented features (does it do what it says on
the tin?)
* usability of explicit features (do the features work well?)
* usability gap of unimplemented features (what else do we need?)
* are there any bugs?

These questions are often quickly answered for smaller patches, but HS's
scope mean that such a task properly executed could take a full week, if
not longer.

Second group of people are just as skilled Postgres people as reviewers,
in some cases more so, apart from they have less detailed knowledge of
the codebase. We have many such people and it would be good to encourage
them to perform thorough reviews rather than "tire kicking".

I'm not sure that Heikki needs additional reviewers. He now has
significant knowledge of the patch and is good at focusing on key
aspects of the internals. Other code reviewers are welcome, of course.

--
Simon Riggs www.2ndQuadrant.com


--
Sent via pgsql-hackers mailing list (pgsql-hackers(a)postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

From: Dimitri Fontaine on
Robert Haas <robertmhaas(a)gmail.com> writes:
> Also, stepping back from me personally, should we try to assign some
> additional reviewers to these patches? Is there some way we can
> divide up review tasks among multiple people so that we're not
> repeating each others work?
>
> Thoughts appreciated, from Heikki, Simon, or others.

How about this proposal:
http://archives.postgresql.org/pgsql-hackers/2009-08/msg00764.php

Regards,
--
dim

--
Sent via pgsql-hackers mailing list (pgsql-hackers(a)postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

From: Jeff Janes on
On Tue, Sep 15, 2009 at 2:41 PM, Simon Riggs <simon(a)2ndquadrant.com> wrote:

>
> OK, here is the latest version of the Hot Standby patchset. This is
> about version 30+ by now, but we should regard this as 0.2.1
> Patch against CVS HEAD (now): clean apply, compile, no known bugs.
>


Hi Simon,

Is there a reason that you remove the WAL_DEBUG shown below?


*************** begin:;
*** 899,923 ****
FIN_CRC32(rdata_crc);
record->xl_crc = rdata_crc;

- #ifdef WAL_DEBUG
- if (XLOG_DEBUG)
- {
- StringInfoData buf;
-
- initStringInfo(&buf);
- appendStringInfo(&buf, "INSERT @ %X/%X: ",
- RecPtr.xlogid,
RecPtr.xrecoff);
- xlog_outrec(&buf, record);
- if (rdata->data != NULL)
- {
- appendStringInfo(&buf, " - ");
- RmgrTable[record->xl_rmid].rm_desc(&buf,
record->xl_info, rdata->data);
- }
- elog(LOG, "%s", buf.data);
- pfree(buf.data);
- }
- #endif
-
/* Record begin of record in appropriate places */
ProcLastRecPtr = RecPtr;
Insert->PrevRecord = RecPtr;
--- 947,952 ----



Thanks,

Jeff