check function patch

From: Alvaro Herrera <alvherre(at)alvh(dot)no-ip(dot)org>
To: "pgsql-hackers(at)postgresql(dot)org" <pgsql-hackers(at)postgresql(dot)org>
Cc: Petr Jelinek <pjmodos(at)pjmodos(dot)net>, Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com>
Subject: check function patch
Date: 2012-03-08 12:06:02
Message-ID: 1331208362.4f58a0aa3562d@webmail.no-ip.com
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

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. 

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Martin Pihlak 2012-03-08 12:34:05 Re: Patch review for logging hooks (CF 2012-01)
Previous Message Petr Jelinek 2012-03-08 11:30:14 Re: poll: CHECK TRIGGER?