Re: check function patch

Hello Alvaro

here is new version - merged Peter's doc changes. I created a new
header "functioncmds.h". This file contains lines related to checker
only. I didn't want to unclean this patch by header files
reorganization.

Regards

Pavel

2012/3/8 Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com>:
> Hello
>
> there are other version related to your last comments. I removed magic
> constants.
>
> This is not merged with Peter's changes. I'll do it tomorrow. Probably
> there will be some bigger changes in header files, but this can be
> next step.
>
> Regards
>
> Pavel
>
> 2012/3/8 Alvaro Herrera <alvherre(at)alvh(dot)no-ip(dot)org>:
>> Hi guys,
>>
>> sorry, I'm stuck in an unfamiliar webmail.
>>
>> I checked the patch Petr just posted.
>> http://archives.postgresql.org/pgsql-hackers/2012-03/msg00482.php
>>
>> I have two comments.  First, I notice that the documentation changes has two
>> places that describe the columns that a function checker returns -- one in
>> the "plhandler" page, another in the create language page.  I think it
>> should exist only on one of those, probably the create language one; and the
>> plhandler page should just say "the checker should comply with the
>> specification at <create language>". Or something like that.   Also, the
>> fact that the tuple description is prose makes it hard to read; I think it
>> should be a table -- three columns: name, type, description.
>>
>> My second comment is that the checker tuple descriptor seems to have changed
>> in the code.  In the patch I posted, the FunctionCheckerDesc() function was
>> not static; in this patch it has been made static.  But what I intended was
>> that the other places that need a descriptor for anything would use this
>> function to get one, instead of building them by hand.  There are two such
>> places currently, one in CreateProceduralLanguage. I think this should be
>> simply walking the tupdesc->attrs array to create the arrays it needs for
>> the ProcedureCreate call -- shoud be a rather easy change.  The other place
>> is plpgsql's report_error(). Honestly I don't like this function at all due
>> to the way it's assuming what the tupledesc looks like.  I'm not sure how to
>> improve it, however, but it seems wrong to me.
>
> One reason to do this this
>> way (i.e. centralize knowledge of what the tupdesc looks like) is that
>> otherwise they get out of sync -- I notice that CreateProcedureLanguage now
>> knows that there are 15 columns while the other places believe there are
>> only 11.
>>
>>