patch: CHECK FUNCTION statement

Lists: pgsql-hackers
From: Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com>
To: PostgreSQL Hackers <pgsql-hackers(at)postgresql(dot)org>
Subject: patch: CHECK FUNCTION statement
Date: 2011-10-06 11:52:07
Message-ID: CAFj8pRDS+A-iR_0k+gDhck2nMZ5iwL5nXPiT0uwDNQtcQ-oQgw@mail.gmail.com
Views: Raw Message | Whole Thread | Download mbox | Resend email
Lists: pgsql-hackers

Hello

I am sending a version with regress tests and basic documentation

Regards

Pavel Stehule

Attachment Content-Type Size
check_function.diff text/x-patch 77.0 KB

From: Thom Brown <thom(at)linux(dot)com>
To: Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com>
Cc: PostgreSQL Hackers <pgsql-hackers(at)postgresql(dot)org>
Subject: Re: patch: CHECK FUNCTION statement
Date: 2011-11-14 20:04:26
Message-ID: CAA-aLv7rwBe0xeXVTna6kDpWf=PkQjtXQZUPz-Hzk-Kc0QCZPw@mail.gmail.com
Views: Raw Message | Whole Thread | Download mbox | Resend email
Lists: pgsql-hackers

On 6 October 2011 12:52, Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com> wrote:
>
> Hello
>
> I am sending a version with regress tests and basic documentation

Hi Pavel,

I think this sentence needs rewriting:

"checkfunction is the name of a previously registered function that
will be called when a new function in the language is created, to
check the function by statemnt CHECK FUNCTION or CHECK TRIGGER."

to something like:

"checkfunction is the name of an existing function that will be called
whenever a CHECK FUNCTION or CHECK TRIGGER is requested on a function
written in the language."

And shouldn't this apply to ALTER LANGUAGE too?

And there seem to be copy/paste symptoms in
doc/src/sgml/ref/check_function.sgml where it shows the definition of
CREATE FUNCTION and CREATE TRIGGER instead of CHECK FUNCTION and CHECK
TRIGGER.

In src/include/nodes/parsenodes.h there's the error message "there are
no plan for query:". This should probably read "there is no plan for
query:". This appears more than once.

And "cannot to identify real type for record type variable" doesn't
sound right. Firstly "to" shouldn't be in there, and referring to a
"real" type is ambiguous as there is a data type called "real". This
appears at least twice.

In src/pl/plpgsql/src/pl_exec.c:

"cannot to determine a result of dynamic SQL" should be "cannot
determine result of dynamic SQL".

Also, I recommend rebasing this patch as it doesn't apply cleanly. In
particular, the following fail:

src/pl/plpgsql/src/pl_funcs.c
src/test/regress/expected/plpgsql.out
src/test/regress/sql/plpgsql.sql

I haven't tried actually testing the patch itsel, but I will probably
give it a go if a rebased version appears. :)

--
Thom Brown
Twitter: @darkixion
IRC (freenode): dark_ixion
Registered Linux user: #516935

EnterpriseDB UK: http://www.enterprisedb.com
The Enterprise PostgreSQL Company


From: Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com>
To: Thom Brown <thom(at)linux(dot)com>
Cc: PostgreSQL Hackers <pgsql-hackers(at)postgresql(dot)org>
Subject: Re: patch: CHECK FUNCTION statement
Date: 2011-11-14 20:54:35
Message-ID: CAFj8pRCcWE_ArCW-eU7nrae9fEW8jUA=YB=Y3xP3hzOotODZwA@mail.gmail.com
Views: Raw Message | Whole Thread | Download mbox | Resend email
Lists: pgsql-hackers

Hello

2011/11/14 Thom Brown <thom(at)linux(dot)com>:
> On 6 October 2011 12:52, Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com> wrote:
>>
>> Hello
>>
>> I am sending a version with regress tests and basic documentation
>
> Hi Pavel,
>
> I think this sentence needs rewriting:
>
> "checkfunction is the name of a previously registered function that
> will be called when a new function in the language is created, to
> check the function by statemnt CHECK FUNCTION or CHECK TRIGGER."
>
> to something like:
>
> "checkfunction is the name of an existing function that will be called
> whenever a CHECK FUNCTION or CHECK TRIGGER is requested on a function
> written in the language."
>
> And shouldn't this apply to ALTER LANGUAGE too?
>
> And there seem to be copy/paste symptoms in
> doc/src/sgml/ref/check_function.sgml where it shows the definition of
> CREATE FUNCTION and CREATE TRIGGER instead of CHECK FUNCTION and CHECK
> TRIGGER.
>
> In src/include/nodes/parsenodes.h there's the error message "there are
> no plan for query:".  This should probably read "there is no plan for
> query:".  This appears more than once.
>
> And "cannot to identify real type for record type variable" doesn't
> sound right.  Firstly "to" shouldn't be in there, and referring to a
> "real" type is ambiguous as there is a data type called "real".  This
> appears at least twice.

I am not native speaker, so please, fix documentation as you like.

>
> In src/pl/plpgsql/src/pl_exec.c:
>
> "cannot to determine a result of dynamic SQL" should be "cannot
> determine result of dynamic SQL".
>
> Also, I recommend rebasing this patch as it doesn't apply cleanly.  In
> particular, the following fail:
>
> src/pl/plpgsql/src/pl_funcs.c
> src/test/regress/expected/plpgsql.out
> src/test/regress/sql/plpgsql.sql
>
> I haven't tried actually testing the patch itsel, but I will probably
> give it a go if a rebased version appears. :)

There will be more work, I found one area, that was not checked - expr targets.

this new code is on github https://github.com/okbob/plpgsql_lint

this week I plan to redesign this contrib module to CHECK FUNCTION
implementation for 9.2.

Regards

Pavel

>
> --
> Thom Brown
> Twitter: @darkixion
> IRC (freenode): dark_ixion
> Registered Linux user: #516935
>
> EnterpriseDB UK: http://www.enterprisedb.com
> The Enterprise PostgreSQL Company
>


From: Thom Brown <thom(at)linux(dot)com>
To: Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com>
Cc: PostgreSQL Hackers <pgsql-hackers(at)postgresql(dot)org>
Subject: Re: patch: CHECK FUNCTION statement
Date: 2011-11-14 21:10:20
Message-ID: CAA-aLv7gSOoX6Z0D10fq=H79OMdjU=JC4CavNsZYR-meNPMBiQ@mail.gmail.com
Views: Raw Message | Whole Thread | Download mbox | Resend email
Lists: pgsql-hackers

On 14 November 2011 20:54, Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com> wrote:
> Hello
>
> 2011/11/14 Thom Brown <thom(at)linux(dot)com>:
>> On 6 October 2011 12:52, Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com> wrote:
>>>
>>> Hello
>>>
>>> I am sending a version with regress tests and basic documentation
>>
>> Hi Pavel,
>>
>> I think this sentence needs rewriting:
>>
>> "checkfunction is the name of a previously registered function that
>> will be called when a new function in the language is created, to
>> check the function by statemnt CHECK FUNCTION or CHECK TRIGGER."
>>
>> to something like:
>>
>> "checkfunction is the name of an existing function that will be called
>> whenever a CHECK FUNCTION or CHECK TRIGGER is requested on a function
>> written in the language."
>>
>> And shouldn't this apply to ALTER LANGUAGE too?
>>
>> And there seem to be copy/paste symptoms in
>> doc/src/sgml/ref/check_function.sgml where it shows the definition of
>> CREATE FUNCTION and CREATE TRIGGER instead of CHECK FUNCTION and CHECK
>> TRIGGER.
>>
>> In src/include/nodes/parsenodes.h there's the error message "there are
>> no plan for query:".  This should probably read "there is no plan for
>> query:".  This appears more than once.
>>
>> And "cannot to identify real type for record type variable" doesn't
>> sound right.  Firstly "to" shouldn't be in there, and referring to a
>> "real" type is ambiguous as there is a data type called "real".  This
>> appears at least twice.
>
> I am not native speaker, so please, fix documentation as you like.

Well I wasn't entirely confident my interpretations were correct. I'd
prefer to have a rebased patch I can fully apply first, and then I can
provide a corrective patch as I'd like to test it too.

--
Thom Brown
Twitter: @darkixion
IRC (freenode): dark_ixion
Registered Linux user: #516935

EnterpriseDB UK: http://www.enterprisedb.com
The Enterprise PostgreSQL Company