Thread

1. check function patch

   Alvaro Herrera <alvherre@alvh.no-ip.org> — 2012-03-08T12:06:02Z

   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 ". 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. 
   
   
   

2. Re: check function patch

   Pavel Stehule <pavel.stehule@gmail.com> — 2012-03-08T15:58:40Z

   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@alvh.no-ip.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.
   >
   >
   

3. Re: check function patch

   Pavel Stehule <pavel.stehule@gmail.com> — 2012-03-09T18:21:29Z

   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.stehule@gmail.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@alvh.no-ip.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.
   >>
   >>