Re: Converting SetOp to read its two inputs separately

I wrote:
> So after digging into it, I realized that I'd been completely misled
> by setop_fill_hash_table's not failing while reading the first input.
> That's not because the code was correct, it was because none of our
> test cases manage to reach TupleHashTableMatch() while reading the
> first input. Apparently there are no regression test cases where
> tuples of the first input have identical hash codes. But as soon
> as I tried a case with duplicate rows in the first input, kaboom!

I didn't quite believe that theory, and so I went back to look again.
Indeed we do have test cases exercising duplicate-row removal, so
why wasn't TupleHashTableMatch crashing? The answer turns out to
be less about duplicates (although we need at least a hash-code
match to reach the problem) and more about the form of the input.
This test case from union.sql doesn't crash (with my v2 patch):

SELECT q2 FROM int8_tbl INTERSECT SELECT q1 FROM int8_tbl ORDER BY 1;

but this does:

SELECT * FROM int8_tbl INTERSECT SELECT * FROM int8_tbl ORDER BY 1;

The reason is that "SELECT *" doesn't have to do a projection,
so the SeqScan node just returns its scan tuple slot, which is
a BufferHeapTuple slot, and that triggers the wrong-slot-type
crash. But "SELECT q2" does have to do a projection, so what
it returns is a Virtual tuple slot, and that's okay! That's
not so much about restrictions of LookupTupleHashEntry as it
is about this optimization in ExecComputeSlotInfo:

/* if the slot is known to always virtual we never need to deform */
if (op->d.fetch.fixed && op->d.fetch.kind == &TTSOpsVirtual)
return false;

That causes us not to emit the EEOP_INNER_FETCHSOME opcode that
is spitting up in the BufferHeapTuple case. And apparently all
the rest of LookupTupleHashEntry is good with a virtual slot.

I can't avoid the feeling that this has all been optimized a little
too far, because it's darn near impossible to understand what has
gone wrong when something goes wrong.

Anyway, it's certainly unsafe for nodeSetOp.c to assume that its
two inputs will return slots of identical types. Since
LookupTupleHashEntry does call this tuple-comparison code that is
happy to wire in assumptions about what the input slot type is,
we had better insert a buffering slot that all the data goes through,
as the v3 patch does.

I'm inclined to add a couple of test cases similar to

SELECT * FROM int8_tbl INTERSECT SELECT q2, q1 FROM int8_tbl ORDER BY 1,2;

so that we get some coverage of cases where the input slots
are dissimilar.

regards, tom lane