On Fri, Feb 11, 2011 at 2:17 PM, Noah Misch <n...@leadboat.com> wrote:
> Good to know. I can envision that perspective, and I share it when the
> savings
> is rather more substantial, say >10% of the patch or >100 lines. Below that
> threshold, the energy I expend grasping two interface changes in one patch
> series exceeds my annoyance at the premature generality.
It's a fair point.
One other thing that may be worth mentioning is that you're diving
into the community at a fairly high level. It's obvious that you have
a pretty solid understanding of the code, better than mine in some
areas, and I don't want to be the dumb guy who won't commit your patch
because I'm too, like, dumb. OTOH, as you've probably realized, our
community dynamic is that the committer is the one who gets yelled at
when something is broken.
>> Then I'll review that separately. I think this change stands on its
>> own, and committing it in steps will be simple for me than doing the
>> whole thing in one go.
>
> That works for me. Know that, barring other suggestions, the followup patch
> will replace AlteredTableInfo.rewrite with the enum field. Just want to make
> sure that's not a surprise. ... And now that I've read your second reply, I
> probably didn't even have to mention it.
It's all good.
You might want to consider a second boolean in lieu of a three way
enum. I'm not sure if that's cleaner but if it lets you write:
if (blah)
at->verify = true;
instead of:
if (blah)
at->worklevel = Min(at->worklevel, WORK_VERIFY);
...then I think that might be cleaner.
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
