CommitFest 2009-09, two weeks on
in [PgSql]
|
From: Boszormenyi Zoltan on 1 Oct 2009 09:47 Michael Meskes �rta: > On Wed, Sep 30, 2009 at 12:34:23PM -0400, Tom Lane wrote: > >> qualified to review them. (I don't actually think we have anybody >> except Michael who's really familiar with ecpg.) >> > > I'm afraid I'm simply not able to spend much time on this in the near future as > I'm simply too busy atm. I spend some time on these the last time, but wasn't > even able to see how Zoltan changed the points we mentioned back then, but I'm > sure he has. > > As already noted the patches stack on each other. There doesn't seem to be a > technical reason for this at least not for some of those dependencies. With the > first patch changing the grammar file and thus taking quite some reviewing > effort this slows things down even more because one needs more effort to review > for instance the sqlda addition, although that one seems to be quite easy to > review. > You're not being fair with me. The dependencies are quite technical. First, Tom Lane suggested to unify core and ecpg FETCH syntaxes so both will accept optional FROM/IN, which I did. SQLDA support adds new FETCH forms (Informix-specific ones) so naturally these patches clash. There's no simple way to make they separately applicable. With the first version, the same technical dependency were also there, because of the (already explained) grammar problem, I got 2 shift/reduce problems in the FETCH/MOVE stmts unless I de-factorized FORWARD and BACKWARD out of fetch_direction. The new FETCH forms with SQLDA touched the same areas in ecpg.addon. Second, DESCRIBE support and SQLDA support also overlap, because SQLDA is a new descriptor form, and DESCRIBE has to support both SQL descriptors and SQLDA. Well, I can split the DESCRIBE patch in half, so it will be usable on SQL descriptors but the other part would depend on both SQLDA and basic DESCRIBE support. Technical dependency again. What non-technical dependencies are you talking about? Please explain, so I may fix them. Saying it vaguely doesn't help. When I first posted the split patchset, you didn't tell me that the split is no good. I tried everything help I could to explain why I did what. Also, the current reviewer (Dan Colish) haven't contacted me despite I offered help privately. I can't review my own patches, that's clear. But I can't do anything else to speed review up but to offer my help and wait for the help/explanation request that didn't arrive. Also, I have sent some independent very small patches, that don't even need much review. One of them (the typo in pgc.l) was already applied and I thank you Michael, but the memory leak fix for two improperly freed numerics is still behind. Please, look at that patch, it should be really obvious. > All of this is written from the top of my head, so please bear with me if I > missed any changes in the patches. > > Michael > 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/ -- 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: Alvaro Herrera on 1 Oct 2009 11:02 Boszormenyi Zoltan escribi�: > First, Tom Lane suggested to unify core and ecpg FETCH > syntaxes so both will accept optional FROM/IN, which I did. > SQLDA support adds new FETCH forms (Informix-specific > ones) so naturally these patches clash. There's no simple way > to make they separately applicable. With the first version, > the same technical dependency were also there, because of > the (already explained) grammar problem, I got 2 shift/reduce > problems in the FETCH/MOVE stmts unless I de-factorized > FORWARD and BACKWARD out of fetch_direction. > The new FETCH forms with SQLDA touched the same areas > in ecpg.addon. Probably the parts that touch the core grammar can be reviewed and applied separately. -- Alvaro Herrera http://www.CommandPrompt.com/ The PostgreSQL Company - Command Prompt, Inc. -- 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: Michael Meskes on 1 Oct 2009 12:12 On Thu, Oct 01, 2009 at 03:47:07PM +0200, Boszormenyi Zoltan wrote: > You're not being fair with me. The dependencies are quite > technical. I'm sorry that you interpreted my email this way, it wasn't at all meant to offend you. > First, Tom Lane suggested to unify core and ecpg FETCH > syntaxes so both will accept optional FROM/IN, which I did. > SQLDA support adds new FETCH forms (Informix-specific This is actually one I was talking about. Adding SQLDA *only* seems like an easy one, as soon as it also hits the parser it becomes more complicated. This is not to say that it is not doable, but it wasn't for me with my time constraints. I was just explaining why I didn't delve into these any more so far. > When I first posted the split patchset, you didn't tell me that > the split is no good. I tried everything help I could to explain > why I did what. Well actually I did, but that's not the problem here. I have no idea why you are ranting like this just because of me excusing myself for a lack of time. > Also, the current reviewer (Dan Colish) haven't contacted me despite > I offered help privately. I can't review my own patches, that's clear. > But I can't do anything else to speed review up but to offer my help > and wait for the help/explanation request that didn't arrive. Okay, so it's all my fault. Do you feel better now? > Also, I have sent some independent very small patches, that don't > even need much review. One of them (the typo in pgc.l) was already > applied and I thank you Michael, but the memory leak fix for two > improperly freed numerics is still behind. Please, look at that patch, > it should be really obvious. My last commit was one of your patches, obviously I didn't do anything on ecpg since. I'm not sure what you are trying to tell me, you might want to explain the last two sentences. But keep in mind you are *not* the one to decide how I spend my spare time. Michael -- Michael Meskes Michael at Fam-Meskes dot De, Michael at Meskes dot (De|Com|Net|Org) Michael at BorussiaFan dot De, Meskes at (Debian|Postgresql) dot Org ICQ: 179140304, AIM/Yahoo/Skype: michaelmeskes, Jabber: meskes(a)jabber.org Go VfL Borussia! Go SF 49ers! Use Debian GNU/Linux! Use PostgreSQL! -- 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: Boszormenyi Zoltan on 1 Oct 2009 13:21 Michael Meskes �rta: > On Thu, Oct 01, 2009 at 03:47:07PM +0200, Boszormenyi Zoltan wrote: > >> You're not being fair with me. The dependencies are quite >> technical. >> > > I'm sorry that you interpreted my email this way, it wasn't at all meant to > offend you. > Please, accept my apologies, I only tried to express my frustration, this is not a good situation for either of us. You were busy with your job and other occupations. We have a serious project going on that depend on ECPG being more compatible to Informix. >> First, Tom Lane suggested to unify core and ecpg FETCH >> syntaxes so both will accept optional FROM/IN, which I did. >> SQLDA support adds new FETCH forms (Informix-specific >> > > This is actually one I was talking about. Adding SQLDA *only* seems like an > easy one, as soon as it also hits the parser it becomes more complicated. This > is not to say that it is not doable, but it wasn't for me with my time > constraints. I was just explaining why I didn't delve into these any more so > far. > This is now clear, I will separate that part out and post it. But this means that the same core grammar parts will be touched twice because two basic things are done there: - optional FROM/IN in core grammar, ECPG grammar updated - "name" -> "cursor_name" transition in cursor-related statements in the core grammar and the dynamic cursorname patch So, technically dependant patches again, but maybe easier on the reviewer. Would this be accepted this way? Or the two modification washed into one? >> When I first posted the split patchset, you didn't tell me that >> the split is no good. I tried everything help I could to explain >> why I did what. >> > > Well actually I did, but that's not the problem here. I have no idea why you > are ranting like this just because of me excusing myself for a lack of time. > I am very sorry, under stress sometimes I get like a steam engine overloaded. Not a good behaviour at 37, I know. I am very sorry. >> Also, the current reviewer (Dan Colish) haven't contacted me despite >> I offered help privately. I can't review my own patches, that's clear. >> But I can't do anything else to speed review up but to offer my help >> and wait for the help/explanation request that didn't arrive. >> > > Okay, so it's all my fault. Do you feel better now? > It's not your fault at all, it's a difficult situation and I had to express it somehow. My tone was not proper. BTW, thanks for adding my small collected knowledge about the ECPG grammar as official documentation. >> Also, I have sent some independent very small patches, that don't >> even need much review. One of them (the typo in pgc.l) was already >> applied and I thank you Michael, but the memory leak fix for two >> improperly freed numerics is still behind. Please, look at that patch, >> it should be really obvious. >> > > My last commit was one of your patches, Yes, the pgc.l one-liner. Thanks again for committing that. > obviously I didn't do anything on ecpg > since. I'm not sure what you are trying to tell me, you might want to explain > the last two sentences. I thought you forgot that patch, the "please, look at that patch" was an (I thought) polite request, it's really two one-liners. > But keep in mind you are *not* the one to decide how I > spend my spare time. > Obviously. Please, accept my apologies. 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/ -- 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: Michael Meskes on 1 Oct 2009 14:11
On Thu, Oct 01, 2009 at 07:21:54PM +0200, Boszormenyi Zoltan wrote: > Please, accept my apologies, I only tried to express my > frustration, this is not a good situation for either of us. Apologies accepted, email is a difficult means of communication anyway. It leads to misunderstanding IMO. > You were busy with your job and other occupations. > We have a serious project going on that depend on > ECPG being more compatible to Informix. Please keep in mind that the needs of your business project cannot and will not influence the way PostgreSQL as on OSS project will work. > Would this be accepted this way? Or the two modification washed into one? It is accepted either way. I was just pointing out that it might be easier to review/commit at least parts of your patches if they can be applied seperately. > I thought you forgot that patch, the "please, look at that patch" > was an (I thought) polite request, it's really two one-liners. Well, this is true as the patch was buried in the long thread containing all the other ones. And yes, now that you brought it into my memory again, I already committed it. Sorry for missing it. Michael -- Michael Meskes Michael at Fam-Meskes dot De, Michael at Meskes dot (De|Com|Net|Org) Michael at BorussiaFan dot De, Meskes at (Debian|Postgresql) dot Org ICQ: 179140304, AIM/Yahoo/Skype: michaelmeskes, Jabber: meskes(a)jabber.org Go VfL Borussia! Go SF 49ers! Use Debian GNU/Linux! Use PostgreSQL! -- Sent via pgsql-hackers mailing list (pgsql-hackers(a)postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers |