Re: fix: plpgsql: return query and dropped columns problem

Jaime Casanova <jcasanov(at)systemguards(dot)com(dot)ec> writes:
>> On Mon, Jul 20, 2009 at 10:09 AM, Alvaro
>>> Getting rid of the check on natts was "ungood" ... it needs to compare
>>> the number of undropped columns of both tupdescs.

> patch attached

This patch is *still* introducing more bugs than it fixes. The reason
is that it has modified validate_tupdesc_compat to allow the tupdescs to
be somewhat different, but has fixed only one of the several call sites
to deal with the consequences of that. The others will now become crash
risks if we apply it as-is.

What I would suggest as a suitable plan for a fix is to modify
validate_tupdesc_compat so that it returns a flag indicating whether the
tupdesc compatibility is exact or requires translation. If translation
is required, provide another routine that does that --- probably using a
mapping data structure set up by validate_tupdesc_compat, since in some
of these cases we'll be processing many tuples. Then the callers just
have to know enough to call the tuple-translation function when
validate_tupdesc_compat tells them to.

There are a number of other places in the system with similar
requirements, although none of them seem to be exact matches.
In particular ExecEvalConvertRowtype() provides a good template for
efficient translation logic, but it's using column name matching
rather than positional matching so you couldn't share the setup logic.
I'm not sure if it's worth moving all this code into the core so that
it can be shared with other future uses (maybe in other PLs), but it's
worth considering that. access/common/heaptuple.c or tupdesc.c might
be a good place for it if we decide to do that.

The executor's junkfilter code is pretty nearly related as well, and
perhaps the Right Thing is to make all of this stuff into applications
of junkfilters with different setup routines for the different
requirements. However the junkfilter code is designed to work with
tuples that are in TupleTableSlots, which isn't particularly helpful for
plpgsql's uses, so maybe trying to unify with that code is more trouble
than it's worth.

I'm marking this patch as Waiting on Author again, although perhaps
Returned with Feedback would be better since my suggestions amount
to wholesale rewrites.

regards, tom lane