Re: ALTER TYPE 3: add facility to identify further no-work cases

On Mon, Jan 24, 2011 at 07:18:47PM -0500, Robert Haas wrote:
> On Sun, Jan 9, 2011 at 5:03 PM, Noah Misch <noah(at)leadboat(dot)com> wrote:
> > Here I add the notion of an "exemptor function", a property of a cast that
> > determines when calls to the cast would be superfluous. ?Such calls can be
> > removed, reduced to RelabelType expressions, or annotated (via a new field in
> > FuncExpr) with the applicable exemptions. ?I modify various parse_coerce.c
> > functions to retrieve, call, and act on these exemptor functions; this includes
> > GetCoerceExemptions() from the last patch. ?I did opt to make
> > find_typmod_coercion_function return COERCION_PATH_RELABELTYPE when no work is
> > needed, rather than COERCION_PATH_NONE; this makes it consistent with
> > find_coercion_pathway's use of that enumeration.
> >
> > To demonstrate the functionality, I add exemptor functions for varchar and xml.
> > Originally I was only going to start with varchar, but xml tests the other major
> > code path, and the exemptor function for xml is dead simple.
> >
> > This helps on conversions like varchar(4)->varchar(8) and text->xml.
>
> I've read through this patch somewhat. As I believe Tom also
> commented previously, exemptor is a pretty bad choice of name.

I spent awhile with a thesaurus before coining that. Since Tom's comment, I've
been hoping the next person to complain would at least suggest a better name.
Shooting in the dark on a nomenclature issue isn't fun, and I'm not picky.

> More
> generally, I think this patch is a case of coding something
> complicated without first achieving consensus on what the behavior
> should be. I think we need to address that problem first, and then
> decide what code needs to be written, and then write the code.

Well, that's why I started the design thread "Avoiding rewrite in ALTER TABLE
ALTER TYPE" before writing any code. That thread petered out rather than reach
any consensus. What would you have done then?

> It seems to me that patch two in this series has the right idea: skip
> rewriting the table in cases where the types are binary coercible
> (WITHOUT FUNCTION) or one is an unconstrained domain over the other.
> IIUC, the problem this patch is trying to address is that our current
> CAST infrastructure is insufficient to identify all cases where this
> is true - in particular, it makes the decision solely based on the
> type OIDs, without consideration of the typmods. In general, varchar
> -> varchar is not binary coercible, but varchar(4) -> varchar(8) is
> binary coercible. I think what this patch is trying to do is tag the
> call to the cast function with the information that we might not need
> to call it, but ISTM it would be better to just notice that the
> function call is unnecessary and insert a RelabelType node instead.

This patch does exactly that for varchar(4) -> varchar(8) and similar cases.

> *reads archives*
>
> In fact, Tom already made pretty much exactly this suggestion, on a
> thread entitled "Avoiding rewrite in ALTER TABLE ALTER TYPE". The
> only possible objection I can see to that line of attack is that it's
> not clear what the API should be, or whether there might be too much
> runtime overhead for the amount of benefit that can be obtained.

I believe the patch does implement Tom's suggestion, at least in spirit. He
mentioned expression_planner() as the place to do it. In principle, We could do
it *right there* and avoid any changes to coerce_to_target_type(), but the
overhead would increase. Specifically, we would be walking an already-built
expression tree looking for casts to remove, probably doing a syscache lookup
for every cast to grab the exemptor function OID. Doing it there would prevent
us from directly helping or harming plain old queries. Since ExecRelCheck(),
FormIndexDatum() and other notable paths call expression_planner(), we wouldn't
want to casually add an expensive algorithm there. To avoid the extra syscache
lookups, we'd piggyback off those already done in coerce_to_target_type(). That
brings us to the current implementation. Better to make the entire decision in
coerce_to_target_type() and never add the superfluous node to the expression
tree in the first place.

coerce_to_target_type() and callees seemed like the natural, low cost place to
make the changes. By doing it that way, did I miss some important detail or
implication of Tom's suggestion?

> This is, incidentally, another problem with this patch - if you want
> to make wide-ranging changes to the system like this, you need to
> provide a convincing argument that it's not going to compromise
> correctness or performance. Just submitting the patch without making
> an argument for why it's correct is no good, unless it's so obviously
> correct as to be unquestionable, which is certainly not the case here.
> You've got to write up some kind of submission notes so that the
> reviewer isn't trying to guess why you think this is the right
> approach, or spending a lot of time thinking through approaches you've
> discarded for good reason. If there is a danger of performance
> regression, you need to refute it, preferably with actual benchmarks.
> A particular aspect I'm concerned about with this patch in particular:
> it strikes me that the overhead of calling the exemptor functions in
> the ALTER TABLE case is negligible, but that this might not be true in
> other contexts where some of this logic may get invoked - it appears
> to implicate the casting machinery much more generally.

I assumed that readers of this patch email had read the series introduction
email, which referred them to the design discussion that you found. Surely that
design answered more than zero of "why this is correct". What questions in
particular did you find unanswered?

As for performance, coerce_to_target_type() is certainly in wide use, but it's
not exactly a hot path. I prepared and ran the attached bench-coerce-worst.sql
and bench-coerce-best.sql. Both are quite artificial. The first one basically
asks "Can we measure the new overhead at all?" The second one asks "Would any
ordinary query benefit from removing a superfluous cast from an expression
tree?" The worst case had an 4% _speedup_, and the best case had a 27% speedup,
suggesting answers of "no" and "yes (but not dramatically)". The "worst-case"
speedup doesn't make sense, so it's probably an artifact of some recent change
on master creating a larger performance dip in this pathological test case. I
could rebase the patch and rerun the benchmark, but I doubt an exact figure
would be much more meaningful. Unfortunately, I can't think of any more-natural
tests (help welcome) that would still illustrate any performance difference.

> I think it's a mistake to merge the handling of the rewrite-vs-noop
> case with the check-vs-noop case. They're fundamentally dissimilar.
> As noted above, being able to notice the noop case seems like it could
> save run-time work during ordinary query execution. But detecting the
> "check" case is useless work except in the specific case of a table
> rewrite. If we're going to do that at all, it seems to me that it
> needs to be done via a code-path that's only going to get invoked in
> the case of ALTER TABLE, not something that's going to happen every
> time we parse an expression tree.

I disagree that they're fundamentally dissimilar. They're _fundamentally_
similar, in that they are both fences, standing at different distances, around
the necessity of a cast function. Your argument only mentions performance, and
it boils down to a suggestion that we proceed to optimize things now by
separating these questions. Perhaps we should, but that's a different question.
Based on my benchmarks, I'm not seeing a need to micro-optimize.

> But it seems to me that it might be
> better to take the suggestion I made before: forget about the
> check-only case for now, and focus on the do-nothing case.

Let's revisit this when we're on the same page about the above points.

Thanks,
nm