From: | Noah Misch <noah(at)leadboat(dot)com> |
---|---|
To: | Robert Haas <robertmhaas(at)gmail(dot)com> |
Cc: | pgsql-hackers(at)postgresql(dot)org |
Subject: | Re: ALTER TYPE 2: skip already-provable no-work rewrites |
Date: | 2011-02-11 19:17:47 |
Message-ID: | 20110211191747.GD30425@tornado.leadboat.com |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
On Fri, Feb 11, 2011 at 01:55:45PM -0500, Robert Haas wrote:
> On Fri, Feb 11, 2011 at 1:08 PM, Noah Misch <noah(at)leadboat(dot)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 <noah(at)leadboat(dot)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
From | Date | Subject | |
---|---|---|---|
Next Message | Magnus Hagander | 2011-02-11 19:18:21 | Re: Debian readline/libedit breakage |
Previous Message | Dimitri Fontaine | 2011-02-11 19:16:52 | Re: ALTER EXTENSION UPGRADE, v3 |