On Fri, Feb 11, 2011 at 01:55:45PM -0500, Robert Haas wrote: > On Fri, Feb 11, 2011 at 1:08 PM, Noah Misch <n...@leadboat.com> wrote: > > Even supposing we push off all scan-only cases to another patch, it would be > > good to have the tablecmds.c-internal representation of that in mind. ?No > > sense > > in simplifying a 12-line change to an 8-line change, only to redo it next > > patch. > > Incidentally, I don't really agree with this, as a philosophical > point. There can be a lot of point to simplifying things, even if it > means redoing a little work, if it makes them easier to understand, > both for the people reviewing at the time and for the benefit of > people reading the commit log in the future.
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. On Fri, Feb 11, 2011 at 01:34:14PM -0500, Robert Haas wrote: > On Fri, Feb 11, 2011 at 1:08 PM, Noah Misch <n...@leadboat.com> wrote: > >> Upon examination, it appeared to me that trying to make the > >> AlteredTableInfo contain an enum indicating whether we were doing a > >> scan, a rewrite, or nothing was requiring more code change than I > >> could really justify. > > > > Offhand, I count 12 changed lines to introduce the enum. ?There may be other > > good reasons not to do it, but surely that wasn't it? > > I was unable to see what were getting out of that logic, and I > couldn't readily verify that it was correct. The value probably gets clearer when you need to use all three states. > I think what I'd like to do if there are no major objections is commit > this as-is, and then if you could perhaps provide a patch containing > the set of changes that are necessary to recapture the cases I > inadvertently failed to handle, namely: > > > 2) Skip rewrite for T -> constraint-free domain over T > > 3) Downgrade rewrite to scan for T -> constrained domain over T > > 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. Thanks again, nm -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
