CommitFest 2009-09, two weeks on
It's now been two weeks since we started this CommitFest, so it seems
like a good time to review where we are. Here are my thoughts, for
what that's worth.
Our overall rate of progress is significantly slower than it was last
time around. At a similar point in the July CommitFest, 19 patches
had been committed (not counting 3 that were committed before the
start of the CommitFest), 11 had been returned with feedback (again,
not counting 2 from before the start of the CommitFest), and 3 had
been rejected. The corresponding numbers for this CommitFest are 8,
7, and 3, which means that the rate of returning patches with feedback
and/or rejecting them is only modestly lower, but the rate of
committing is much lower. I'm not sure whether this is because the
patches are more complex, because the committers have been busy with
other issues, or some other reason.
We also have fewer patches than we did last time around. I believe we
started the last CommitFest with a bit more than 75 patches (there are
fewer now, as some were moved to this CommitFest) and we started this
one with just 48. This somewhat balances out the slower rate of
grinding through the patch queue, but I'm still a bit worried about
the rate at which we're making progress. It would be nice to be done
on time, and I'm not sure we're going to make it.
With respect to individual patches:
- There are three ECPG patches for which it's been difficult to find a
reviewer. It seems we don't have any reviewers familiar with ECPG.
If anyone is able to help review these, it would be much appreciated.
- There is one dblink pach left over from last CommitFest. Joe Conway
was going to review it the weekend of July 18th-19th, but that didn't
end up happening and so that patch is still waiting. We might be able
to find someone else to review it, but I'm not sure whether that will
help unless there is a committer other than Joe with bandwidth to do
the final review and commit.
- Hot Standby and Streaming Replication are both huge new features in
this CommitFest, and there seems to be a fair amount of activity
around both patches. Heikki previously expressed optimism about
getting Hot Standby done this CommitFest, but I am not sure whether he
is still feeling optimistic, or what his feelings are about Streaming
Replication, which is currently waiting on Fujii Masao for a new
version.
On the whole, it seems like patch authors have done a better job than
last time of responding to feedback in a timely fashion - very little
is falling out due to submitter inattention. That is good, although
it also means that the percentage of patches that will require
substantive action (rather than, say, summary rejection for
non-communication) is apt to be higher. I am also generally under the
impression that we have a larger number of complex patches this time
around. Some of that may be because much good feedback was given in
the last CommitFest, and previously half-baked ideas are coming back a
little more well done.
...Robert
Robert Haas wrote:
- Hot Standby and Streaming Replication are both huge new features in
this CommitFest, and there seems to be a fair amount of activity
around both patches. Heikki previously expressed optimism about
getting Hot Standby done this CommitFest, but I am not sure whether he
is still feeling optimistic,
There's a lot of small things that need fixing, but nothing major. I'm
not so much optimistic, but I think we should spend the extra effort
required on hot standby to force it in in this commitfest. It's a big
feature and it really could use some alpha-testing earlier rather than
later. It would also leave time for any extra features or tweaks to be
made in the later commitfests.
OTOH, I'd hate to hold the commitfest hostage for that. Perhaps it
should be returned to author at this point, I should move on to other
patches to get the commitfest closed ASAP, and continue reviewing and
polishing that right after the commitfest.
or what his feelings are about Streaming
Replication, which is currently waiting on Fujii Masao for a new
version.
I'm undecided on whether walreceiver should be a subprocess of the
startup process, or of postmaster as it was submitted. I'd appreciate if
others would take a look into that too and give opinions. And then
there's the small list of things I asked Fujii-san to work on.
--
Heikki Linnakangas
EnterpriseDB http://www.enterprisedb.com
Heikki Linnakangas <heikki.linnakangas@enterprisedb.com> writes:
OTOH, I'd hate to hold the commitfest hostage for that. Perhaps it
should be returned to author at this point, I should move on to other
patches to get the commitfest closed ASAP, and continue reviewing and
polishing that right after the commitfest.
FWIW, I'd rather you kept focusing on those two patches while other
committers work on the rest. From what I've seen you're finding a
whole lot of broken or at least questionable stuff, and even if they're
individually minor issues, that adds up to a lot of instability.
I agree that these patches need special attention and should not be
treated exactly the same as we'd treat smaller patches.
regards, tom lane
On Wed, Sep 30, 2009 at 11:36 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
Heikki Linnakangas <heikki.linnakangas@enterprisedb.com> writes:
OTOH, I'd hate to hold the commitfest hostage for that. Perhaps it
should be returned to author at this point, I should move on to other
patches to get the commitfest closed ASAP, and continue reviewing and
polishing that right after the commitfest.FWIW, I'd rather you kept focusing on those two patches while other
committers work on the rest. From what I've seen you're finding a
whole lot of broken or at least questionable stuff, and even if they're
individually minor issues, that adds up to a lot of instability.I agree that these patches need special attention and should not be
treated exactly the same as we'd treat smaller patches.
I tend to agree. I think it's reasonable for you (meaning Heikki) to
devote far more time and effort to those patches than you would to
other patches implementing more run-of-the-mill features, and it seems
like there is no shortage of things to find and fix. I don't think
that having you take a break to work on other patches is going to be
to the overall benefit of the project (and many of the more
significant remaining patches look like they are right up Tom's alley
anyway).
That having been said, if Hot Standby is still closer to commit than
Streaming Replication, it might make sense to push Streaming
Replication off until November, or at least post-CommitFest. Do you
have any sense of how soon you'll feel confident to commit either
patch?
...Robert
Robert Haas wrote:
- There is one dblink pach left over from last CommitFest. Joe Conway
was going to review it the weekend of July 18th-19th, but that didn't
end up happening and so that patch is still waiting. We might be able
to find someone else to review it, but I'm not sure whether that will
help unless there is a committer other than Joe with bandwidth to do
the final review and commit.
I will get to it before the end of this commitfest, but I have to admit
I'm not all that excited about this patch in the first place. I don't
know that I agree with the need.
Joe
Robert Haas wrote:
On Wed, Sep 30, 2009 at 11:36 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
Heikki Linnakangas <heikki.linnakangas@enterprisedb.com> writes:
OTOH, I'd hate to hold the commitfest hostage for that. Perhaps it
should be returned to author at this point, I should move on to other
patches to get the commitfest closed ASAP, and continue reviewing and
polishing that right after the commitfest.FWIW, I'd rather you kept focusing on those two patches while other
committers work on the rest. From what I've seen you're finding a
whole lot of broken or at least questionable stuff, and even if they're
individually minor issues, that adds up to a lot of instability.I agree that these patches need special attention and should not be
treated exactly the same as we'd treat smaller patches.I tend to agree. I think it's reasonable for you (meaning Heikki) to
devote far more time and effort to those patches than you would to
other patches implementing more run-of-the-mill features, and it seems
like there is no shortage of things to find and fix. I don't think
that having you take a break to work on other patches is going to be
to the overall benefit of the project (and many of the more
significant remaining patches look like they are right up Tom's alley
anyway).
Ok, good, I'm more than happy to continue fine-combing hot standby.
That having been said, if Hot Standby is still closer to commit than
Streaming Replication, it might make sense to push Streaming
Replication off until November, or at least post-CommitFest.
Commitfest or no-commitfest, I'm planning to continue working on the
streaming replication patch in any case until it's committed.
Do you
have any sense of how soon you'll feel confident to commit either
patch?
I'm bad at estimating. Not this week for sure, and next week I'm
traveling and won't be able to spend as much time on it as I am right
now. If no new major issues are found, and all the outstanding issues
are resolved by me or Simon by then, maybe the week after that.
--
Heikki Linnakangas
EnterpriseDB http://www.enterprisedb.com
Joe Conway <mail@joeconway.com> writes:
Robert Haas wrote:
- There is one dblink pach left over from last CommitFest. Joe Conway
was going to review it the weekend of July 18th-19th, but that didn't
end up happening and so that patch is still waiting. We might be able
to find someone else to review it, but I'm not sure whether that will
help unless there is a committer other than Joe with bandwidth to do
the final review and commit.
I will get to it before the end of this commitfest, but I have to admit
I'm not all that excited about this patch in the first place. I don't
know that I agree with the need.
Well, you're the dblink expert. If you think it should be rejected
I doubt many of us will argue with you.
regards, tom lane
Robert Haas <robertmhaas@gmail.com> writes:
... (and many of the more
significant remaining patches look like they are right up Tom's alley
anyway).
FWIW, if left to my own devices I will eventually get to everything
except the dblink, ecpg, and encoding/win32 patches. I don't intend
to touch any of those because there are other committers better
qualified to review them. (I don't actually think we have anybody
except Michael who's really familiar with ecpg.)
However, if no other committers are working on it it's going to be
a long commitfest ...
The other problem is that most of the patches are not Ready for
Committer anyway.
regards, tom lane
On Wed, Sep 30, 2009 at 12:27 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
Joe Conway <mail@joeconway.com> writes:
Robert Haas wrote:
- There is one dblink pach left over from last CommitFest. Joe Conway
was going to review it the weekend of July 18th-19th, but that didn't
end up happening and so that patch is still waiting. We might be able
to find someone else to review it, but I'm not sure whether that will
help unless there is a committer other than Joe with bandwidth to do
the final review and commit.I will get to it before the end of this commitfest, but I have to admit
I'm not all that excited about this patch in the first place. I don't
know that I agree with the need.Well, you're the dblink expert. If you think it should be rejected
I doubt many of us will argue with you.
Yep. CommitFest doesn't mean "commit it"; it means "decide whether to
commit it". Things being rejected or returned with feedback for
further improvement is fine; we're just trying to avoid long periods
with no response at all.
...Robert
On Wed, Sep 30, 2009 at 12:34 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
Robert Haas <robertmhaas@gmail.com> writes:
... (and many of the more
significant remaining patches look like they are right up Tom's alley
anyway).FWIW, if left to my own devices I will eventually get to everything
except the dblink, ecpg, and encoding/win32 patches. I don't intend
to touch any of those because there are other committers better
qualified to review them. (I don't actually think we have anybody
except Michael who's really familiar with ecpg.)
Thanks, I think that's helpful information.
However, if no other committers are working on it it's going to be
a long commitfest ...
That is my concern as well.
The other problem is that most of the patches are not Ready for
Committer anyway.
I (and hopefully the people who agreed to help with patch-chasing) can
work on this, but given that there are 5 that are Ready for Committer
and probably as many more that are close, and further given that in
the past 7 days exactly 1 patch from the CommitFest has been
committed, I'm not sure there's a real problem here. If you
commit/bounce all 5 of those afternoon I will spend the evening making
sure you have a few more to tackle tomorrow.
...Robert
Robert Haas wrote:
On Wed, Sep 30, 2009 at 12:34 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
Robert Haas <robertmhaas@gmail.com> writes:
... (and many of the more
significant remaining patches look like they are right up Tom's alley
anyway).FWIW, if left to my own devices I will eventually get to everything
except the dblink, ecpg, and encoding/win32 patches. I don't intend
to touch any of those because there are other committers better
qualified to review them. (I don't actually think we have anybody
except Michael who's really familiar with ecpg.)Thanks, I think that's helpful information.
However, if no other committers are working on it it's going to be
a long commitfest ...That is my concern as well.
I have been (and still am somewhat) slammed, but I can probably make
space to work on "Encoding issues in console and eventlog on win32
<https://commitfest.postgresql.org/action/patch_view?id=148>" some time
in the next day or three. After that, if I still have time and nobody
else has grabbed it, I'll move on to "CREATE LIKE INCLUDING COMMENTS and
STORAGE <https://commitfest.postgresql.org/action/patch_view?id=172>".
cheers
andrew
Robert Haas wrote:
On Wed, Sep 30, 2009 at 12:27 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
Joe Conway <mail@joeconway.com> writes:
Robert Haas wrote:
- There is one dblink pach left over from last CommitFest. Joe Conway
was going to review it the weekend of July 18th-19th, but that didn't
end up happening and so that patch is still waiting. We might be able
to find someone else to review it, but I'm not sure whether that will
help unless there is a committer other than Joe with bandwidth to do
the final review and commit.I will get to it before the end of this commitfest, but I have to admit
I'm not all that excited about this patch in the first place. I don't
know that I agree with the need.Well, you're the dblink expert. If you think it should be rejected
I doubt many of us will argue with you.Yep. CommitFest doesn't mean "commit it"; it means "decide whether to
commit it". Things being rejected or returned with feedback for
further improvement is fine; we're just trying to avoid long periods
with no response at all.
The issue is not so much technical as it is philosophical.
The patch basically forces all use of libpq by dblink to be asynchronous
(internally) so that a cancel can be sensed and passed down to the
remote side and everything cleaned up. Possibly the right thing to do,
but dblink already allows the use of async queries, and the current
synchronous method uses standard libpq calls. If all of this is really
necessary, doesn't every libpq client have the same issue? If so why
have the synchronous libpq functions at all?
So while I can vet the patch technically, and spend more time
understanding the use case, and maybe explaining it better, I think
other people should weigh in on the change as it is significant and
points to other potential issues.
Joe
On Wed, Sep 30, 2009 at 18:34, Tom Lane <tgl@sss.pgh.pa.us> wrote:
Robert Haas <robertmhaas@gmail.com> writes:
... (and many of the more
significant remaining patches look like they are right up Tom's alley
anyway).FWIW, if left to my own devices I will eventually get to everything
except the dblink, ecpg, and encoding/win32 patches. I don't intend
to touch any of those because there are other committers better
qualified to review them. (I don't actually think we have anybody
except Michael who's really familiar with ecpg.)
I can certainly review the win32 encoding patch, but I was rather
hoping for some comments from others on if we're interested in a win32
only solution, or if we want something more generic. Should we just go
with the win32-only one for now?
--
Magnus Hagander
Me: http://www.hagander.net/
Work: http://www.redpill-linpro.com/
Magnus Hagander escribi�:
On Wed, Sep 30, 2009 at 18:34, Tom Lane <tgl@sss.pgh.pa.us> wrote:
Robert Haas <robertmhaas@gmail.com> writes:
... (and many of the more
significant remaining patches look like they are right up Tom's alley
anyway).FWIW, if left to my own devices I will eventually get to everything
except the dblink, ecpg, and encoding/win32 patches. �I don't intend
to touch any of those because there are other committers better
qualified to review them. �(I don't actually think we have anybody
except Michael who's really familiar with ecpg.)I can certainly review the win32 encoding patch, but I was rather
hoping for some comments from others on if we're interested in a win32
only solution, or if we want something more generic. Should we just go
with the win32-only one for now?
Just a couple of days ago a question came on the spanish list because
someone was getting mixed UTF8 and Latin1 output in a log file. This
was in Fedora IIRC, so maybe we do want something more general.
--
Alvaro Herrera http://www.CommandPrompt.com/
PostgreSQL Replication, Consulting, Custom Development, 24x7 support
Magnus Hagander <magnus@hagander.net> writes:
I can certainly review the win32 encoding patch, but I was rather
hoping for some comments from others on if we're interested in a win32
only solution, or if we want something more generic. Should we just go
with the win32-only one for now?
That was actually the only substantive comment I had about it. I don't
see why it's a win32-only problem or why a win32-only solution is a good
approach.
regards, tom lane
On Wed, Sep 30, 2009 at 21:38, Tom Lane <tgl@sss.pgh.pa.us> wrote:
Magnus Hagander <magnus@hagander.net> writes:
I can certainly review the win32 encoding patch, but I was rather
hoping for some comments from others on if we're interested in a win32
only solution, or if we want something more generic. Should we just go
with the win32-only one for now?That was actually the only substantive comment I had about it. I don't
see why it's a win32-only problem or why a win32-only solution is a good
approach.
Yeah, that's my thought as well.
If we want a complete one, we should reject this patch and ask for one
that does that.
If we are fine with a win32 only one, I can review this one and get it in.
I'm leaning towards us wanting a general one, but I'm unsure how much
work that will take.
--
Magnus Hagander
Me: http://www.hagander.net/
Work: http://www.redpill-linpro.com/
Joe Conway <mail@joeconway.com> writes:
The issue is not so much technical as it is philosophical.
The patch basically forces all use of libpq by dblink to be asynchronous
(internally) so that a cancel can be sensed and passed down to the
remote side and everything cleaned up. Possibly the right thing to do,
but dblink already allows the use of async queries, and the current
synchronous method uses standard libpq calls. If all of this is really
necessary, doesn't every libpq client have the same issue?
Well, only the ones that want to implement cancel and don't have access
to the app's own signal handling functions. (Which suggests that a
possible answer is to allow dblink to hook into the SIGINT catcher,
but frankly hooks in that location scare me ...)
I would argue that it's not necessarily a good idea at all: one of the
typical uses for dblink is to fake "autonomous transactions", and in
that application I don't think you *want* a cancel to propagate to the
other session. If we did put this behavior into all dblink operations,
we'd need a way to turn it off.
Since dblink_cancel_query is already available, people who do want
cancels to propagate have the ability to do that. I'm inclined to
think that this is complexity we don't need.
regards, tom lane
Joe Conway <mail@joeconway.com> wrote:
The patch basically forces all use of libpq by dblink to be asynchronous
(internally) so that a cancel can be sensed and passed down to the
remote side and everything cleaned up. Possibly the right thing to do,
but dblink already allows the use of async queries, and the current
synchronous method uses standard libpq calls.
The point is *memory leak* in dblink when a query is canceled or
become time-out. I think it is a bug, and my patch could fix it.
Regards,
---
ITAGAKI Takahiro
NTT Open Source Software Center
Magnus Hagander <magnus@hagander.net> wrote:
I can certainly review the win32 encoding patch, but I was rather
hoping for some comments from others on if we're interested in a win32
only solution, or if we want something more generic. Should we just go
with the win32-only one for now?
Yes, because Windows is only platform that supports UTF-16 encoding natively.
I believe my patch is the best solution for Windows even if we have another
approach for other platforms.
Regards,
---
ITAGAKI Takahiro
NTT Open Source Software Center
On Thu, Oct 1, 2009 at 04:11, Itagaki Takahiro
<itagaki.takahiro@oss.ntt.co.jp> wrote:
Magnus Hagander <magnus@hagander.net> wrote:
I can certainly review the win32 encoding patch, but I was rather
hoping for some comments from others on if we're interested in a win32
only solution, or if we want something more generic. Should we just go
with the win32-only one for now?Yes, because Windows is only platform that supports UTF-16 encoding natively.
I believe my patch is the best solution for Windows even if we have another
approach for other platforms.
Actually, I think a better argument is that since Windows will *never*
accept UTF8 logging, and that's what most databases will be in, much
of this patch will be required anyway. So I should probably review and
get this part in while we think about other solutions *as well* for
other platforms.
--
Magnus Hagander
Me: http://www.hagander.net/
Work: http://www.redpill-linpro.com/
