Re: No-op case in ExecEvalConvertRowtype

I wrote:
> Ashutosh Bapat <ashutosh(dot)bapat(at)enterprisedb(dot)com> writes:
>> In ExecEvalConvertRowtype(), if the input row doesn't require any
>> conversion, we simply return that row as is.

> Huh. That's been like that for a very long time.

So I imagined that this was an ancient bug, and was proceeding on that
basis, until I noticed that the test case I showed doesn't crash in 9.6
or before. Which is pretty interesting because the assertion in
ExecEvalConvertRowtype is certainly there, back to 9.4 (and in an even
stronger fashion in older branches).

Digging further, the reason that the back branches don't crash is that
they don't believe that these tuple conversions are no-ops. And that
traces to this test in convert_tuples_by_name:

/*
* Check to see if the map is one-to-one and the tuple types are the same.
* (We check the latter because if they're not, we want to do conversion
* to inject the right OID into the tuple datum.)
*/
if (indesc->natts == outdesc->natts &&
indesc->tdtypeid == outdesc->tdtypeid)
{
...

Because a ConvertRowtypeExpr would only be inserted to change a tuple's
rowtype from some composite type to some other composite type, it's
basically impossible for convert_tuples_by_name to decide that the mapping
is a no-op, which explains the comment in ExecEvalConvertRowtype doubting
that a no-op case is possible. Moreover, if a no-op case did happen, the
tuple being returned would in fact contain the right type OID, so there's
no bug.

Or at least that's how it is in 9.6. In HEAD, it's been broken by
commit 3838074f8, which as near as I can tell was completely misguided.
Apparently, Robert and Amit misread the comment about "injecting the right
OID" to refer to a possible value in the tuple's OID system column, rather
than the rowtype OID that must be placed in the composite datum's header.

I propose to deal with this by reverting 3838074f8 in toto, and then
trying to clarify that comment, and maybe adding a regression test case
based on the example I showed earlier so that it will be a little more
obvious if someone breaks this again.

However, I see that 3838074f8 touches some partitioning code, which
makes me wonder if there's anything in the partitioning logic that
really depends on this erroneous "optimization".

regards, tom lane