Thread: CF 2009-09: initial reviewing assignments
And we're off! Initial reviewing assignments are below. Please go to http://commitfest.postgresql.org/action/commitfest_view/inprogress and edit the patch you've been assigned, adding your name as a reviewer. Please do this right away so that it's clear which patches still need reviewers to be assigned. If possible, please attempt to complete your initial review by end-of-day Saturday (five days from now). When you post a review, please post a comment with your review AND ALSO edit the status of the patch as appropriate - Waiting on Author, Returned with Feedback (if not to be further considered this CF), Ready for Committer, Rejected, etc. Thanks, ...Robert Jeff Davis <pgsql@j-davis.com> - Join optimization for inheritance tables Jeff Janes <jeff.janes@gmail.com> - opportunistic freezing during vacuum Selena Deckelmann <selenamarie@gmail.com> - join removal Robert Haas <robertmhaas@gmail.com> - Separate Heap Fetch from Index Scan Andres Freund <andres@anarazel.de> - GRANT ON ALL IN schema Stephen Frost <sfrost@snowman.net> - Reworks for Access Controls Jaime Casanova <jcasanov@systemguards.com.ec> - Largeobject access controls Jan Urbański <wulczer@wulczer.org> - Default ACLs Brendan Jurd <direvus@gmail.com> - Generalized Index Constraints Joshua Tolley <eggyknap@gmail.com> - DML node in support of writeable CTEs Dan Colish <dan@unencrypted.org> - generic copy options David Fetter <david@fetter.org> - make plpgsql IN args mutable Dimitri Fontaine <dfontaine@hi-media.com> - Anonymous code blocks Nikhil Sontakke <nikhil.sontakke@enterprisedb.com> - Linux LSB init script David E. Wheeler <david@kineticode.com> - hstore enhancements Josh Williams <joshwilliams@ij.net> WINDOWS - Encoding issues in console and eventlog on win32 Bernd Helmle <bernd@oopsware.de> - Allow more complex user/database default GUC settings Logging configuration changes - Abhijit Menon-Sen <ams@toroid.org> gabrielle <gorthx@gmail.com> on behalf of PDXPUG CODE SPRINT - Errcontext support in PL/Perl - MOVE FORWARD n | BACKWARD n - psql: Unicode UTF-8 table formatting for text output - Pgbench Shell command - unicode escapes (time permitting)
On Mon, Sep 14, 2009 at 9:08 PM, Robert Haas <robertmhaas@gmail.com> wrote: Please go to > http://commitfest.postgresql.org/action/commitfest_view/inprogress and > edit the patch you've been assigned, adding your name as a reviewer. > Please do this right away so that it's clear which patches still need > reviewers to be assigned. And by "right away" I mean "as soon as someone gets wwwmaster.postgresql.org back on line so you can log in". :-( It seems all community logins for all apps are down ATM. ...Robert
Robert Haas escreveu: > Initial reviewing assignments are below. > I'm available to review too. I'll take 'Buffer usage in EXPLAIN and pg_stat_statements', ok? -- Euler Taveira de Oliveira http://www.timbira.com/
On Mon, Sep 14, 2009 at 11:34 PM, Euler Taveira de Oliveira <euler@timbira.com> wrote: > Robert Haas escreveu: >> Initial reviewing assignments are below. >> > I'm available to review too. I'll take 'Buffer usage in EXPLAIN and > pg_stat_statements', ok? OK with me. ...Robert
At 2009-09-14 21:08:56 -0400, robertmhaas@gmail.com wrote: > > Logging configuration changes > - Abhijit Menon-Sen <ams@toroid.org> I reviewed this patch. It's fine. Ready for committer as-is. But I suggested some minor changes that I thought were useful, and posted a new version of the patch including those changes. Besides that, I posted a patch to implement a suggestion Peter made in his original posting. What status should I assign the patch now? It's "Needs Review", in a sense, but also "Waiting on Author", "Ready for Committer", and "Returned with Feedback" ;-) The patch is completely trivial, before and after, so I feel a bit silly even to ask this question. Should I just advance it to "Ready"? (I suppose my review will show up on -hackers by the time this message shows up on -rrreviewers.) -- ams
On Wed, Sep 16, 2009 at 1:38 AM, Abhijit Menon-Sen <ams@toroid.org> wrote: > At 2009-09-14 21:08:56 -0400, robertmhaas@gmail.com wrote: >> >> Logging configuration changes >> - Abhijit Menon-Sen <ams@toroid.org> > > I reviewed this patch. It's fine. Ready for committer as-is. > > But I suggested some minor changes that I thought were useful, and > posted a new version of the patch including those changes. Besides > that, I posted a patch to implement a suggestion Peter made in his > original posting. > > What status should I assign the patch now? It's "Needs Review", in > a sense, but also "Waiting on Author", "Ready for Committer", and > "Returned with Feedback" ;-) > > The patch is completely trivial, before and after, so I feel a bit silly > even to ask this question. Should I just advance it to "Ready"? Yep, Ready for Committer sounds good to me. ...Robert
On Wed, Sep 16, 2009 at 12:44 PM, Robert Haas <robertmhaas@gmail.com> wrote: > On Wed, Sep 16, 2009 at 1:38 AM, Abhijit Menon-Sen <ams@toroid.org> wrote: >> At 2009-09-14 21:08:56 -0400, robertmhaas@gmail.com wrote: >>> >>> Logging configuration changes >>> - Abhijit Menon-Sen <ams@toroid.org> >> >> I reviewed this patch. It's fine. Ready for committer as-is. >> >> But I suggested some minor changes that I thought were useful, and >> posted a new version of the patch including those changes. Besides >> that, I posted a patch to implement a suggestion Peter made in his >> original posting. >> >> What status should I assign the patch now? It's "Needs Review", in >> a sense, but also "Waiting on Author", "Ready for Committer", and >> "Returned with Feedback" ;-) >> >> The patch is completely trivial, before and after, so I feel a bit silly >> even to ask this question. Should I just advance it to "Ready"? > > Yep, Ready for Committer sounds good to me. Oh, and make sure to add a comment of type "Patch" with a link to your version of the patch. ...Robert
Hi, > Nikhil Sontakke <nikhil.sontakke@enterprisedb.com> > - Linux LSB init script > Peter is already actively taking a look at this one. Please let me know if I can help elsewhere or can just lurk around since this is a small commitfest. Regards, Nikhils -- http://www.enterprisedb.com
I am free to start on something new. Any suggestions? -- --Dan
On Thu, Sep 17, 2009 at 10:00 PM, Dan Colish <dan@unencrypted.org> wrote: > I am free to start on something new. Any suggestions? If you're game to dive into ECPG, there are a bunch of patches from Zoltan that could use a review. If not, either of the two patches under miscellaneous whose names begin with "syslog" could use some further review. I don't think that either of them are likely to be committed in their present form, so the goal is not so much to ascertain that as to perhaps suggest some other approaches that might be workable. ...Robert
On Tue, Sep 15, 2009 at 9:55 AM, Robert Haas <robertmhaas@gmail.com> wrote: > On Mon, Sep 14, 2009 at 11:34 PM, Euler Taveira de Oliveira > <euler@timbira.com> wrote: >> Robert Haas escreveu: >>> Initial reviewing assignments are below. >>> >> I'm available to review too. I'll take 'Buffer usage in EXPLAIN and >> pg_stat_statements', ok? > > OK with me. If you're going to review this patch, you should edit it and list your name as a reviewer, so that others know that it is already being reviewed. If you're not going to review this patch, please let us know so that someone else can pick it up. ...Robert
On Thu, Sep 17, 2009 at 10:29:11PM -0400, Robert Haas wrote: > On Thu, Sep 17, 2009 at 10:00 PM, Dan Colish <dan@unencrypted.org> wrote: > > I am free to start on something new. Any suggestions? > > If you're game to dive into ECPG, there are a bunch of patches from > Zoltan that could use a review. > > If not, either of the two patches under miscellaneous whose names > begin with "syslog" could use some further review. I don't think > that either of them are likely to be committed in their present form, > so the goal is not so much to ascertain that as to perhaps suggest > some other approaches that might be workable. > > ...Robert I'll take the dynamic cursor support for ECPG -- --Dan
Gabrielle and PDXPUG, Thanks for your reviewing. A little followup: On Mon, Sep 14, 2009 at 9:08 PM, Robert Haas <robertmhaas@gmail.com> wrote: > gabrielle <gorthx@gmail.com> on behalf of PDXPUG CODE SPRINT > - Errcontext support in PL/Perl This was committed. > - MOVE FORWARD n | BACKWARD n This is listed as to be reviewed by Selena Deckelmann and John Naylor, but no review posted yet. > - psql: Unicode UTF-8 table formatting for text output This is still listed as having no reviewer - are you folks planning to post a review? If so, please edit patch and list reviewer names. > - Pgbench Shell command This is waiting on the author to clarify how to use this. The email requesting this clarification should probably be added as a comment, so that it's easy to understand why this is marked as "Waiting on Author". > - unicode escapes (time permitting) Are you guys doing anything with this one? If so, please list reviewer name on app. Thanks, ...Robert
Robert Haas <robertmhaas@gmail.com> writes: > reviewers to be assigned. If possible, please attempt to complete > your initial review by end-of-day Saturday (five days from now). When > [...] > Dimitri Fontaine <dfontaine@hi-media.com> > - Anonymous code blocks Well I can't see being able to review this before tomorow evening, I guess it'd be realistic to target next thursday. Should this slow down too much the fest or what, please feel free to beat me to it :) Regards, -- dim
On Mon, 2009-09-14 at 21:08 -0400, Robert Haas wrote: > Jeff Davis <pgsql@j-davis.com> > - Join optimization for inheritance tables Tom Lane already reviewed this patch, and it looks like a new approach is required, so further review wouldn't be too helpful: http://archives.postgresql.org/message-id/4690.1253300592@sss.pgh.pa.us Can you direct me to review another patch? Regards, Jeff Davis
2009/9/15 Robert Haas <robertmhaas@gmail.com>: > Brendan Jurd <direvus@gmail.com> > - Generalized Index Constraints > It seems that this patch is in the midst of re-design discussion, so I think I'm free to review another patch. Happy to take "CREATE LIKE INCLUDING COMMENTS and STORAGE" but feel free to assign me something else. Cheers, BJ
On Sun, Sep 20, 2009 at 8:32 PM, Jeff Davis <pgsql@j-davis.com> wrote: > On Mon, 2009-09-14 at 21:08 -0400, Robert Haas wrote: >> Jeff Davis <pgsql@j-davis.com> >> - Join optimization for inheritance tables > > Tom Lane already reviewed this patch, and it looks like a new approach > is required, so further review wouldn't be too helpful: > > http://archives.postgresql.org/message-id/4690.1253300592@sss.pgh.pa.us > > Can you direct me to review another patch? Yep, how about Named and mixed notation support? ...Robert
On Sun, Sep 20, 2009 at 8:40 PM, Brendan Jurd <direvus@gmail.com> wrote: > 2009/9/15 Robert Haas <robertmhaas@gmail.com>: >> Brendan Jurd <direvus@gmail.com> >> - Generalized Index Constraints >> > > It seems that this patch is in the midst of re-design discussion, so I > think I'm free to review another patch. > > Happy to take "CREATE LIKE INCLUDING COMMENTS and STORAGE" but feel > free to assign me something else. Sounds good. ...Robert
On Mon, Sep 14, 2009 at 09:08:56PM -0400, Robert Haas wrote: > Joshua Tolley <eggyknap@gmail.com> > - DML node in support of writeable CTEs I hate to say it, but I'm quickly getting the idea that I'll not find the time I need to review this. I'd better ask to pull out before it gets too much later. Sorry, all... -- Joshua Tolley / eggyknap End Point Corporation http://www.endpoint.com
Attachment
On Mon, Sep 21, 2009 at 10:23 AM, Joshua Tolley <eggyknap@gmail.com> wrote: > On Mon, Sep 14, 2009 at 09:08:56PM -0400, Robert Haas wrote: >> Joshua Tolley <eggyknap@gmail.com> >> - DML node in support of writeable CTEs > > I hate to say it, but I'm quickly getting the idea that I'll not find the time > I need to review this. I'd better ask to pull out before it gets too much > later. Sorry, all... OK. I'll assign that one to myself, I've been curious about it anyhow. ...Robert
Hi Robert, Is there another patch you'd like me to work on? Lock wait statistics says it needs review, but the last comment suggests it is waiting on author. Enhancements to COPY (error logging and autopartitioning) says it is waiting on author but last comment suggests perhaps it is ready for review. I've taken a look at ECPG, but I couldn't make heads or tails of it. I guess I could try harder :) It looks like the ECPG patches are not independent and need to applied in a particular order in order for them to apply cleanly to HEAD. So I think I need some guidance on what I should be doing. Thanks, Jeff
On Sat, Sep 26, 2009 at 11:39:45AM -0700, Jeff Janes wrote: > Hi Robert, > > Is there another patch you'd like me to work on? > > Lock wait statistics says it needs review, but the last comment > suggests it is waiting on author. > > Enhancements to COPY (error logging and autopartitioning) says it is > waiting on author but last comment suggests perhaps it is ready for > review. > > I've taken a look at ECPG, but I couldn't make heads or tails of it. > I guess I could try harder :) > > It looks like the ECPG patches are not independent and need to applied > in a particular order in order for them to apply cleanly to HEAD. > > So I think I need some guidance on what I should be doing. > > Thanks, > > Jeff > I've been looking at the dynamic cursor names patch, so if you have any insights I would really appreciate them. I am having some trouble fully reviewing this patch because I am not very familiar with the ecpg code. -- --Dan
On Sat, Sep 26, 2009 at 1:39 PM, Jeff Janes <jeff.janes@gmail.com> wrote: > Hi Robert, > > Is there another patch you'd like me to work on? > > Lock wait statistics says it needs review, but the last comment > suggests it is waiting on author. > some days ago i said i could took this one from saturday (that means today) if nobody has taken it until now i will start reviewing this -- Atentamente, Jaime Casanova Soporte y capacitación de PostgreSQL Asesoría y desarrollo de sistemas Guayaquil - Ecuador Cel. +59387171157
On Sat, Sep 26, 2009 at 2:13 PM, Jaime Casanova <jcasanov@systemguards.com.ec> wrote: > On Sat, Sep 26, 2009 at 1:39 PM, Jeff Janes <jeff.janes@gmail.com> wrote: >> Hi Robert, >> >> Is there another patch you'd like me to work on? >> >> Lock wait statistics says it needs review, but the last comment >> suggests it is waiting on author. >> > > some days ago i said i could took this one from saturday (that means > today) if nobody has taken it until now i will start reviewing this > here there are two patches: Mark Kirkwood's in: http://archives.postgresql.org/pgsql-hackers/2009-08/msg00664.php and Pierre Frédéric Caillau's in: http://archives.postgresql.org/pgsql-hackers/2009-09/msg01260.php the later was submmited on september 18th (3 days after commitfest starts and seems to be only for LWLocks) there is any benefit inf putting it on the next commitfest? while mark says there is some work still pending i will review what is done until now -- Atentamente, Jaime Casanova Soporte y capacitación de PostgreSQL Asesoría y desarrollo de sistemas Guayaquil - Ecuador Cel. +59387171157
>
> On Sat, Sep 26, 2009 at 11:39:45AM -0700, Jeff Janes wrote:
> > Hi Robert,
> >
> > Is there another patch you'd like me to work on?
> >
> > Lock wait statistics says it needs review, but the last comment
> > suggests it is waiting on author.
> >
> > Enhancements to COPY (error logging and autopartitioning) says it is
> > waiting on author but last comment suggests perhaps it is ready for
> > review.
> >
> > I've taken a look at ECPG, but I couldn't make heads or tails of it.
> > I guess I could try harder :)
> >
> > It looks like the ECPG patches are not independent and need to applied
> > in a particular order in order for them to apply cleanly to HEAD.
> >
> > So I think I need some guidance on what I should be doing.
> >
> > Thanks,
> >
> > Jeff
> >
>
> I've been looking at the dynamic cursor names patch, so if you have any
> insights I would really appreciate them. I am having some trouble fully
> reviewing this patch because I am not very familiar with the ecpg code.
>
> --
> --Dan
>
Asketh and you shall be given. :-)
May I help you in understanding ECPG?
The dynamic cursorname patch was started on 8.3.7 first
and we moved to 8.4 (then 8.5) CVS because in 8.4,
ECPG grammar was rewritten to be auto-generated
from the core grammar. I had to dive in kneep deep
before I could modify it...
Basic thing is that ECPG modifies and extends the core
grammar in a way that
1) every token in ECPG is <str> type. New tokens are
defined in ecpg.tokens, types are defined in ecpg.type
2) most tokens from the core grammar are simply converted
to literals concatenated together to form the SQL string
passed to the server, this is done by parse.pl.
3) some rules need side-effects, actions are either added
or completely overridden (compared to the basic token
concatenation) for them, these are defined in ecpg.addons,
the rules for ecpg.addons are explained below.
4) new grammar rules are needed for ECPG metacommands.
These are in ecpg.trailer.
5) ecpg.header contains common functions, etc. used by
actions for grammar rules.
In "ecpg.addons", every modified rule follows this pattern:
ECPG: dumpedtokens postfix
where "dumpedtokens" is simply tokens from core gram.y's
rules concatenated together. e.g. if gram.y has this:
ruleA: tokenA tokenB tokenC {...}
then "dumpedtokens" is "ruleAtokenAtokenBtokenC".
"postfix" above can be:
a) "block" - the automatic rule created by parse.pl is completely
overridden, the code block has to be written completely as
it were in a plain bison grammar
b) "rule" - the automatic rule is extended on, so new syntaxes
are accepted for "ruleA". E.g.:
ECPG: ruleAtokenAtokenBtokenC rule
| tokenD tokenE { action_code; }
...
It will be substituted with:
ruleA: <original syntax forms and actions up to and including
"tokenA tokenB tokenC">
| tokenD tokenE { action_code; }
...
c) "addon" - the automatic action for the rule (SQL syntax constructed
from the tokens concatenated together) is prepended with a new
action code part. This code part is written as is's already inside
the { ... }
Multiple "addon" or "block" lines may appear together with the
new code block if the code block is common for those rules, which
is a very smart thing.
This was what I gathered from the code. The documentation
seems to be missing from the rewritten ECPG grammar in 8.4.
Michael, am I missing something?
Dan, please, start the review in light of the above.
If you have any questions, don't hesitate to ask.
Best regards,
Zoltán Böszörményi
--
Bible has answers for everything. Proof:
"But let your communication be, Yea, yea; Nay, nay: for whatsoever is more
than these cometh of evil." (Matthew 5:37) - basics of digital technology.
"May your kingdom come" - superficial description of plate tectonics
----------------------------------
Zoltán Böszörményi
Cybertec Schönig & Schönig GmbH
http://www.postgresql.at/
On Sat, Sep 26, 2009 at 2:39 PM, Jeff Janes <jeff.janes@gmail.com> wrote: > Hi Robert, > > Is there another patch you'd like me to work on? > > Lock wait statistics says it needs review, but the last comment > suggests it is waiting on author. It looks like there is one outstanding TODO item, but it doesn't seem to be a very big one, so I think that would be an excellent choice. > Enhancements to COPY (error logging and autopartitioning) says it is > waiting on author but last comment suggests perhaps it is ready for > review. Good catch, fixed. > So I think I need some guidance on what I should be doing. Go for lock wait stats. As a side note, what we really need to focus on at this point in the CommitFest is getting closure. If you have reviewed a patch and a new version has been posted, please check whether it looks good and if so mark the patch Ready for Committer. If you have reviewed a patch and a new version has NOT been posted for 4-5 days, post a note saying that you are marking the patch Returned with Feedback and do so. The CommitFest will be half over in 48 hours and we have only closed 19 of 48 patches - that is not good. ...Robert
2009/9/27 Boszormenyi Zoltan <zb@cybertec.at>:
>>
>> On Sat, Sep 26, 2009 at 11:39:45AM -0700, Jeff Janes wrote:
>> > Hi Robert,
>> >
>> > Is there another patch you'd like me to work on?
>> >
>> > Lock wait statistics says it needs review, but the last comment
>> > suggests it is waiting on author.
>> >
>> > Enhancements to COPY (error logging and autopartitioning) says it is
>> > waiting on author but last comment suggests perhaps it is ready for
>> > review.
>> >
>> > I've taken a look at ECPG, but I couldn't make heads or tails of it.
>> > I guess I could try harder :)
>> >
>> > It looks like the ECPG patches are not independent and need to applied
>> > in a particular order in order for them to apply cleanly to HEAD.
>> >
>> > So I think I need some guidance on what I should be doing.
>> >
>> > Thanks,
>> >
>> > Jeff
>> >
>>
>> I've been looking at the dynamic cursor names patch, so if you have any
>> insights I would really appreciate them. I am having some trouble fully
>> reviewing this patch because I am not very familiar with the ecpg code.
>>
>> --
>> --Dan
>>
>
> Asketh and you shall be given. :-)
>
> May I help you in understanding ECPG?
>
> The dynamic cursorname patch was started on 8.3.7 first
> and we moved to 8.4 (then 8.5) CVS because in 8.4,
> ECPG grammar was rewritten to be auto-generated
> from the core grammar. I had to dive in kneep deep
> before I could modify it...
>
> Basic thing is that ECPG modifies and extends the core
> grammar in a way that
> 1) every token in ECPG is <str> type. New tokens are
> defined in ecpg.tokens, types are defined in ecpg.type
> 2) most tokens from the core grammar are simply converted
> to literals concatenated together to form the SQL string
> passed to the server, this is done by parse.pl.
> 3) some rules need side-effects, actions are either added
> or completely overridden (compared to the basic token
> concatenation) for them, these are defined in ecpg.addons,
> the rules for ecpg.addons are explained below.
> 4) new grammar rules are needed for ECPG metacommands.
> These are in ecpg.trailer.
> 5) ecpg.header contains common functions, etc. used by
> actions for grammar rules.
>
> In "ecpg.addons", every modified rule follows this pattern:
> ECPG: dumpedtokens postfix
> where "dumpedtokens" is simply tokens from core gram.y's
> rules concatenated together. e.g. if gram.y has this:
> ruleA: tokenA tokenB tokenC {...}
> then "dumpedtokens" is "ruleAtokenAtokenBtokenC".
> "postfix" above can be:
> a) "block" - the automatic rule created by parse.pl is completely
> overridden, the code block has to be written completely as
> it were in a plain bison grammar
> b) "rule" - the automatic rule is extended on, so new syntaxes
> are accepted for "ruleA". E.g.:
> ECPG: ruleAtokenAtokenBtokenC rule
> | tokenD tokenE { action_code; }
> ...
> It will be substituted with:
> ruleA: <original syntax forms and actions up to and including
> "tokenA tokenB tokenC">
> | tokenD tokenE { action_code; }
> ...
> c) "addon" - the automatic action for the rule (SQL syntax constructed
> from the tokens concatenated together) is prepended with a new
> action code part. This code part is written as is's already inside
> the { ... }
>
> Multiple "addon" or "block" lines may appear together with the
> new code block if the code block is common for those rules, which
> is a very smart thing.
>
> This was what I gathered from the code. The documentation
> seems to be missing from the rewritten ECPG grammar in 8.4.
> Michael, am I missing something?
>
> Dan, please, start the review in light of the above.
> If you have any questions, don't hesitate to ask.
Please move this discussion to -hackers, maybe with a link back to
this post. Good info, wrong place. :-)
...Robert
On Tue, Sep 15, 2009 at 9:55 AM, Robert Haas <robertmhaas@gmail.com> wrote: > On Mon, Sep 14, 2009 at 11:34 PM, Euler Taveira de Oliveira > <euler@timbira.com> wrote: >> Robert Haas escreveu: >>> Initial reviewing assignments are below. >>> >> I'm available to review too. I'll take 'Buffer usage in EXPLAIN and >> pg_stat_statements', ok? > > OK with me. Euler, When can we expect to see this review posted? Thanks, ...Robert
Robert Haas escreveu: > When can we expect to see this review posted? > Sorry for the long delay. I already posted an updated patch that addresses all of the problems I found. Also, I wrote some docs (that are missing). I changed the status to "waiting on author" because I want some feedback for the posted patch. -- Euler Taveira de Oliveira http://www.timbira.com/