plpgsql - DECLARE - cannot to use %TYPE or %ROWTYPE for composite types

2015-10-19 9:52 GMT+02:00 Pavel Stehule <pavel.stehule@gmail.com>:

Hi,

We cannot to declare variable with referenced type on other composite
variable. This limit is probably artificial, because any composite type is
any type too in PostgreSQL.

The issue:

referencing on composite variables doesn't work

do $$ declare x int; y x%type; begin end; $$; -- ok
do $$ declare x pg_class; y x%type; begin end; $$; -- invalid type name
"x%type"
do $$ declare x pg_class; y x%rowtype; begin end; $$; -- relation "x" does
not exist

The %ROWTYPE needs a record in pg_class. Probably we should not to change
it. The change can bring a compatibility issues. So there are two
possibilities:

1. %TYPE can be used for any kind of variables. This behave will be
consistent with polymorphic parameters - we have "anyelement", and we have
not "anyrow".

2. introduce new keyword - %RECTYPE .. it can work, but there will be gap
between polymorphic parameters.

Comments, notices?

Hi

I am sending patch that enables to use references to polymorphic parameters
of row types. Another functionality is possibility to get array or element
type of referenced variable. It removes some gaps when polymorphic
parameters are used.

create type test_composite_type as (x int, y int);
create or replace function test_simple(src anyelement)
returns anyelement as $$
declare dest src%type;
begin
dest := src;
return dest;
end;
$$ language plpgsql;
select test_simple(10);
test_simple
-------------
10
(1 row)

select test_simple('hoj'::text);
test_simple
-------------
hoj
(1 row)

select test_simple((10,20)::test_composite_type);
test_simple
-------------
(10,20)
(1 row)

create or replace function test_poly_element(x anyelement)
returns anyarray as $$
declare result x%arraytype;
begin
result := ARRAY[x];
raise notice '% %', pg_typeof(result), result;
return result;
end;
$$ language plpgsql;
select test_poly_element(1);
NOTICE: integer[] {1}
test_poly_element
-------------------
{1}
(1 row)

select test_poly_element('hoj'::text);
NOTICE: text[] {hoj}
test_poly_element
-------------------
{hoj}
(1 row)

select test_poly_element((10,20)::test_composite_type);
NOTICE: test_composite_type[] {"(10,20)"}
test_poly_element
-------------------
{"(10,20)"}
(1 row)

create or replace function test_poly_array(x anyarray)
returns anyelement as $$
declare result x%elementtype;
begin
result := x[1];
raise notice '% %', pg_typeof(result), result;
return result;
end;
$$ language plpgsql;
select test_poly_array(ARRAY[1]);
NOTICE: integer 1
test_poly_array
-----------------
1
(1 row)

select test_poly_array(ARRAY['hoj'::text]);
NOTICE: text hoj
test_poly_array
-----------------
hoj
(1 row)

select test_poly_array(ARRAY[(10,20)::test_composite_type]);
NOTICE: test_composite_type (10,20)
test_poly_array
-----------------
(10,20)
(1 row)

Regards

Pavel

On 10/30/15 6:01 AM, Pavel Stehule wrote:

I am sending patch that enables to use references to polymorphic
parameters of row types. Another functionality is possibility to get
array or element type of referenced variable. It removes some gaps when
polymorphic parameters are used.

Did this make it into a commitfest?
--
Jim Nasby, Data Architect, Blue Treble Consulting, Austin TX
Experts in Analytics, Data Architecture and PostgreSQL
Data in Trouble? Get it in Treble! http://BlueTreble.com

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

2015-12-21 1:06 GMT+01:00 Jim Nasby <Jim.Nasby@bluetreble.com>:

On 10/30/15 6:01 AM, Pavel Stehule wrote:

I am sending patch that enables to use references to polymorphic
parameters of row types. Another functionality is possibility to get
array or element type of referenced variable. It removes some gaps when
polymorphic parameters are used.

Did this make it into a commitfest?

yes, it is relative trivial small patch without any side effects or
possible performance issues.

The important (and possible disputable) part of this patch is new syntax

DECLARE
var othervar%arraytype,
var othervar%elementtype;

It is consistent with current state, and doesn't increase a complexity of
DECLARE part in plpgsql parser - what was reason for reject this idea 5
years ago (no necessary reserved keywords, ...) .

Regards

Pavel

Hi.
I have tried to do some review of this patch. Below are my comments.

Introduction:

This patch fixes and adds the following functionality:
- %TYPE - now can be used for composite types.
- %ARRAYTYPE - new functionality, provides the array type from a
variable or table column.
- %ELEMENTTYPE - new funcitonality, provides the element type of a given
array.

New regression tests are included in the patch. Changes to the
documentation are not provided.

Initial Run:

The patch applies correctly to HEAD. Regression tests pass successfully,
without errors. It seems that the patch work as you expected.

Performance:

It seems patch have not possible performance issues for the existing
functionality.

Coding:

The style looks fine. I attached the patch that does some corrections in
code and documentation. I have corrected indentation in pl_comp.c and
"read_datatype" function in pl_gram.y. I think changes in
"read_datatype" function would be better to avoid a code duplication.
But I could be wrong of course.

Conclusion:

The patch could be applied on master with documentation corrections. But
I'm not sure that your task could be resloved only by adding %ARRAYTYPE
and %ELEMENTTYPE. Maybe you will give some examples?

--
Artur Zakirov
Postgres Professional: http://www.postgrespro.com
Russian Postgres Company

Hi

2015-12-21 16:21 GMT+01:00 Artur Zakirov <a.zakirov@postgrespro.ru>:

Hi.
I have tried to do some review of this patch. Below are my comments.

Introduction:

This patch fixes and adds the following functionality:
- %TYPE - now can be used for composite types.
- %ARRAYTYPE - new functionality, provides the array type from a variable
or table column.
- %ELEMENTTYPE - new funcitonality, provides the element type of a given
array.

New regression tests are included in the patch. Changes to the
documentation are not provided.

Initial Run:

The patch applies correctly to HEAD. Regression tests pass successfully,
without errors. It seems that the patch work as you expected.

Performance:

It seems patch have not possible performance issues for the existing
functionality.

Coding:

The style looks fine. I attached the patch that does some corrections in
code and documentation. I have corrected indentation in pl_comp.c and
"read_datatype" function in pl_gram.y. I think changes in "read_datatype"
function would be better to avoid a code duplication. But I could be wrong
of course.

Conclusion:

The patch could be applied on master with documentation corrections. But
I'm not sure that your task could be resloved only by adding %ARRAYTYPE and
%ELEMENTTYPE. Maybe you will give some examples?

Thank you for review. The changes in code are good idea.

I waited with documentation if there will be some objections to syntax. The
month later, there are not any known objection.

The target of this feature isn't using for storing of database objects
only, but for storing the values of polymorphic parameters.

CREATE OR REPLACE FUNCTION buble_sort(a anyarray)
RETURNS anyarray AS $$
DECLARE
aux a%ELEMENTTYPE;
repeat_again boolean DEFAULT true;
BEGIN
-- Don't use this code for large arrays!
-- use builtin sort
WHILE repeat_again
repeat_again := false;
FOR i IN array_lower(a, 1) .. array_upper(a, 2)
LOOP
IF a[i] > a[i+1] THEN
aux := a[i+1];
a[i+1] := a[i]; a[i] := aux;
repeat_again := true;
END IF;
END LOOP;
END WHILE;
RETURN a;
END;
$$ LANGUAGE plpgsql;

CREATE OR REPLACE FUNCTION array_init(v anyelement, size integer)
RETURNS anyarray AS $$
DECLARE result v%ARRAYTYPE DEFAULT '{}';
BEGIN
-- prefer builtin function array_fill
FOR i IN 1 .. size
LOOP
result := result || v;
END LOOP;
RETURN result;
END;
$$ LANGUAGE plpgsql;

Regards

Pavel

Hi

2015-12-21 16:21 GMT+01:00 Artur Zakirov <a.zakirov@postgrespro.ru>:

Hi.
I have tried to do some review of this patch. Below are my comments.

Introduction:

This patch fixes and adds the following functionality:
- %TYPE - now can be used for composite types.
- %ARRAYTYPE - new functionality, provides the array type from a variable
or table column.
- %ELEMENTTYPE - new funcitonality, provides the element type of a given
array.

New regression tests are included in the patch. Changes to the
documentation are not provided.

Initial Run:

The patch applies correctly to HEAD. Regression tests pass successfully,
without errors. It seems that the patch work as you expected.

Performance:

It seems patch have not possible performance issues for the existing
functionality.

Coding:

The style looks fine. I attached the patch that does some corrections in
code and documentation. I have corrected indentation in pl_comp.c and
"read_datatype" function in pl_gram.y. I think changes in "read_datatype"
function would be better to avoid a code duplication. But I could be wrong
of course.

I merged Artur's patch and appended examples to doc.

Conclusion:

The patch could be applied on master with documentation corrections. But
I'm not sure that your task could be resloved only by adding %ARRAYTYPE and
%ELEMENTTYPE. Maybe you will give some examples?

It fixes the most missed/known features related to this part of plpgsql,
what I found. But any ideas for this or follofup patches are welcome.

Regards

Pavel

On Tue, Dec 22, 2015 at 5:59 PM, Pavel Stehule <pavel.stehule@gmail.com> wrote:

Hi

2015-12-21 16:21 GMT+01:00 Artur Zakirov <a.zakirov@postgrespro.ru>:

Hi.
I have tried to do some review of this patch. Below are my comments.

Introduction:

This patch fixes and adds the following functionality:
- %TYPE - now can be used for composite types.
- %ARRAYTYPE - new functionality, provides the array type from a variable
or table column.
- %ELEMENTTYPE - new funcitonality, provides the element type of a given
array.

New regression tests are included in the patch. Changes to the
documentation are not provided.

Initial Run:

The patch applies correctly to HEAD. Regression tests pass successfully,
without errors. It seems that the patch work as you expected.

Performance:

It seems patch have not possible performance issues for the existing
functionality.

Coding:

The style looks fine. I attached the patch that does some corrections in
code and documentation. I have corrected indentation in pl_comp.c and
"read_datatype" function in pl_gram.y. I think changes in "read_datatype"
function would be better to avoid a code duplication. But I could be wrong
of course.

I merged Artur's patch and appended examples to doc.

Conclusion:

The patch could be applied on master with documentation corrections. But
I'm not sure that your task could be resloved only by adding %ARRAYTYPE and
%ELEMENTTYPE. Maybe you will give some examples?

It fixes the most missed/known features related to this part of plpgsql,
what I found. But any ideas for this or follofup patches are welcome.

Patch moved to next CF, this entry is still very active.
--
Michael

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

diff --git a/src/pl/plpgsql/src/pl_comp.c b/src/pl/plpgsql/src/pl_comp.c
new file mode 100644
index 1ae4bb7..c819517
*** a/src/pl/plpgsql/src/pl_comp.c
--- b/src/pl/plpgsql/src/pl_comp.c
*************** plpgsql_parse_tripword(char *word1, char
*** 1617,1622 ****
--- 1617,1677 ----
return false;
}

+ /*
+  * Derive type from ny base type controlled by reftype_mode
+  */
+ static PLpgSQL_type *
+ derive_type(PLpgSQL_type *base_type, int reftype_mode)
+ {
+ 	Oid typoid;

I think you should add a typedef to the REFTYPE enum, and have this
function take that type rather than int.

+ 		case PLPGSQL_REFTYPE_ARRAY:
+ 		{
+ 			/*
+ 			 * Question: can we allow anyelement (array or nonarray) -> array direction.
+ 			 * if yes, then probably we have to modify enforce_generic_type_consistency,
+ 			 * parse_coerce.c where still is check on scalar type -> raise error
+ 			 * ERROR:  42704: could not find array type for data type integer[]
+ 			 *
+ 			if (OidIsValid(get_element_type(base_type->typoid)))
+ 				return base_type;
+ 			*/

I think it would be better to resolve this question outside a code
comment.

+ 			typoid = get_array_type(base_type->typoid);
+ 			if (!OidIsValid(typoid))
+ 				ereport(ERROR,
+ 						(errcode(ERRCODE_DATATYPE_MISMATCH),
+ 						 errmsg("there are not array type for type %s",
+ 								format_type_be(base_type->typoid))));

nodeFuncs.c uses this wording:
errmsg("could not find array type for data type %s",
which I think you should adopt.

--- 1681,1687 ----
* ----------
*/
PLpgSQL_type *
! plpgsql_parse_wordtype(char *ident, int reftype_mode)
{
PLpgSQL_type *dtype;
PLpgSQL_nsitem *nse;

Use the typedef'ed enum, as above.

--- 1699,1721 ----
switch (nse->itemtype)
{
case PLPGSQL_NSTYPE_VAR:
! 			{
! 				dtype = ((PLpgSQL_var *) (plpgsql_Datums[nse->itemno]))->datatype;
! 				return derive_type(dtype, reftype_mode);
! 			}

! case PLPGSQL_NSTYPE_ROW:
! {
! dtype = ((PLpgSQL_row *) (plpgsql_Datums[nse->itemno]))->datatype;
! return derive_type(dtype, reftype_mode);
! }

+ 			/*
+ 			 * XXX perhaps allow REC here? Probably it has not any sense, because
+ 			 * in this moment, because PLpgSQL doesn't support rec parameters, so
+ 			 * there should not be any rec polymorphic parameter, and any work can
+ 			 * be done inside function.
+ 			 */

I think you should remove from the "?" onwards in that comment, i.e.
just keep what was already in the original comment (minus the ROW)

--- 1757,1763 ----
* ----------
*/
PLpgSQL_type *
! plpgsql_parse_cwordtype(List *idents, int reftype_mode)
{
PLpgSQL_type *dtype = NULL;
PLpgSQL_nsitem *nse;

Typedef.

--- 2720,2737 ----
tok = yylex();
if (tok_is_keyword(tok, &yylval,
K_TYPE, "type"))
! 				result = plpgsql_parse_wordtype(dtname, PLPGSQL_REFTYPE_TYPE);
! 			else if (tok_is_keyword(tok, &yylval,
! 									K_ELEMENTTYPE, "elementtype"))
! 				result = plpgsql_parse_wordtype(dtname, PLPGSQL_REFTYPE_ELEMENT);
! 			else if (tok_is_keyword(tok, &yylval,
! 									K_ARRAYTYPE, "arraytype"))
! 				result = plpgsql_parse_wordtype(dtname, PLPGSQL_REFTYPE_ARRAY);
else if (tok_is_keyword(tok, &yylval,
K_ROWTYPE, "rowtype"))
result = plpgsql_parse_wordrowtype(dtname);
! 			if (result)
! 				return result;
}

This plpgsql parser stuff is pretty tiresome. (Not this patch's fault
-- just saying.)

*************** extern bool plpgsql_parse_dblword(char *
*** 961,968 ****
PLwdatum *wdatum, PLcword *cword);
extern bool plpgsql_parse_tripword(char *word1, char *word2, char *word3,
PLwdatum *wdatum, PLcword *cword);
! extern PLpgSQL_type *plpgsql_parse_wordtype(char *ident);
! extern PLpgSQL_type *plpgsql_parse_cwordtype(List *idents);
extern PLpgSQL_type *plpgsql_parse_wordrowtype(char *ident);
extern PLpgSQL_type *plpgsql_parse_cwordrowtype(List *idents);
extern PLpgSQL_type *plpgsql_build_datatype(Oid typeOid, int32 typmod,
--- 973,980 ----
PLwdatum *wdatum, PLcword *cword);
extern bool plpgsql_parse_tripword(char *word1, char *word2, char *word3,
PLwdatum *wdatum, PLcword *cword);
! extern PLpgSQL_type *plpgsql_parse_wordtype(char *ident, int reftype_mode);
! extern PLpgSQL_type *plpgsql_parse_cwordtype(List *idents, int reftype_mode);
extern PLpgSQL_type *plpgsql_parse_wordrowtype(char *ident);
extern PLpgSQL_type *plpgsql_parse_cwordrowtype(List *idents);
extern PLpgSQL_type *plpgsql_build_datatype(Oid typeOid, int32 typmod,

By the way, these functions are misnamed after this patch. They are
called "wordtype" and "cwordtype" originally because they accept
"word%TYPE" and "compositeword%TYPE", but after the patch they not only
accept TYPE at the right of the percent sign but also ELEMENTTYPE and
ARRAYTYPE. Not sure that this is something we want to be too strict
about.

*** a/src/test/regress/expected/plpgsql.out
--- b/src/test/regress/expected/plpgsql.out
*************** end;
*** 5573,5575 ****
--- 5573,5667 ----

I think you should also add your array_init() example to the test set.

--
�lvaro Herrera http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

FWIW the reason I read through this patch is that I wondered if there
was anything in common with this other patch
https://commitfest.postgresql.org/8/459/ -- and the answer seems to be
"no". What that patch does is add a new construct
TYPE(1+1)
which in this case returns "int4"; I guess if we wanted to augment that
functionality to cover Pavel's use case we would additionally need
ELEMENTTYPE(somearray)
and
ARRAYTYPE(some-non-array)
in the core grammar ... sounds like a hard sell.

BTW are we all agreed that enabling
foo%ARRAYTYPE
and
foo%ELEMENTYPE
in plpgsql's DECLARE section is what we want for this?

--
�lvaro Herrera http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On Mon, Jan 18, 2016 at 3:51 PM, Alvaro Herrera
<alvherre@2ndquadrant.com> wrote:

BTW are we all agreed that enabling
foo%ARRAYTYPE
and
foo%ELEMENTYPE
in plpgsql's DECLARE section is what we want for this?

I know that Oracle uses syntax of this general type, but I've always
found it ugly. It's also pretty non-extensible. You could want
similar things for range types and any other container types we might
get in the future, but clearly adding new reserved words for each one
is no good.

One idea that occurs to me is: If you can DECLARE BAR FOO%TYPE, but
then you want to make BAR an array of that type rather than a scalar,
why not write that as DECLARE BAR FOO%TYPE[]? That seems quite
natural to me.

I think the part of this patch that makes %TYPE work for more kinds of
types is probably a good idea, although I haven't carefully studied
exactly what it does.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

2016-01-18 22:21 GMT+01:00 Robert Haas <robertmhaas@gmail.com>:

On Mon, Jan 18, 2016 at 3:51 PM, Alvaro Herrera
<alvherre@2ndquadrant.com> wrote:

BTW are we all agreed that enabling
foo%ARRAYTYPE
and
foo%ELEMENTYPE
in plpgsql's DECLARE section is what we want for this?

I know that Oracle uses syntax of this general type, but I've always
found it ugly. It's also pretty non-extensible. You could want
similar things for range types and any other container types we might
get in the future, but clearly adding new reserved words for each one
is no good.

It doesn't use reserved worlds.

One idea that occurs to me is: If you can DECLARE BAR FOO%TYPE, but
then you want to make BAR an array of that type rather than a scalar,
why not write that as DECLARE BAR FOO%TYPE[]? That seems quite
natural to me.

what you propose for syntax for taking a element of array?

I think the part of this patch that makes %TYPE work for more kinds of
types is probably a good idea, although I haven't carefully studied
exactly what it does.

I invite any ideas, but currently used notation is only in direction
type->array. The working with symbols looks more difficult, than using
words (in design area).

More - the textual form is more near to our system of polymorphics types:
anyelement, anyarray, ... We have not anyelement[]

Regards

Pavel

On Mon, Jan 18, 2016 at 4:35 PM, Pavel Stehule <pavel.stehule@gmail.com> wrote:

I know that Oracle uses syntax of this general type, but I've always
found it ugly. It's also pretty non-extensible. You could want
similar things for range types and any other container types we might
get in the future, but clearly adding new reserved words for each one
is no good.

It doesn't use reserved worlds.

OK - keywords, then.

One idea that occurs to me is: If you can DECLARE BAR FOO%TYPE, but
then you want to make BAR an array of that type rather than a scalar,
why not write that as DECLARE BAR FOO%TYPE[]? That seems quite
natural to me.

what you propose for syntax for taking a element of array?

No idea.

I think the part of this patch that makes %TYPE work for more kinds of
types is probably a good idea, although I haven't carefully studied
exactly what it does.

I invite any ideas, but currently used notation is only in direction
type->array. The working with symbols looks more difficult, than using words
(in design area).

More - the textual form is more near to our system of polymorphics types:
anyelement, anyarray, ... We have not anyelement[]

True, but this is hardly a straightforward extension of what we have
today either.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

2016-01-18 22:48 GMT+01:00 Robert Haas <robertmhaas@gmail.com>:

On Mon, Jan 18, 2016 at 4:35 PM, Pavel Stehule <pavel.stehule@gmail.com>
wrote:

I know that Oracle uses syntax of this general type, but I've always
found it ugly. It's also pretty non-extensible. You could want
similar things for range types and any other container types we might
get in the future, but clearly adding new reserved words for each one
is no good.

It doesn't use reserved worlds.

OK - keywords, then.

One idea that occurs to me is: If you can DECLARE BAR FOO%TYPE, but
then you want to make BAR an array of that type rather than a scalar,
why not write that as DECLARE BAR FOO%TYPE[]? That seems quite
natural to me.

what you propose for syntax for taking a element of array?

No idea.

the syntax for "array from" is natural, but for any other is hard. So it is
reason, why I used text form. Using Oracle's pattern source%operation
allows to use nonreserved keywords. Probably any text can be there. The
keywords isn't necessary (not tested).

I think the part of this patch that makes %TYPE work for more kinds of
types is probably a good idea, although I haven't carefully studied
exactly what it does.

I invite any ideas, but currently used notation is only in direction
type->array. The working with symbols looks more difficult, than using

words

(in design area).

More - the textual form is more near to our system of polymorphics types:
anyelement, anyarray, ... We have not anyelement[]

True, but this is hardly a straightforward extension of what we have
today either.

It is, but sometime the polymorphic types can help.

The proposed feature/syntax has sense primary for polymorphic types. It
should to follow our polymorphic types. The primary pair is
"anyarray","anyelement" -> "arraytype","elemementtype".

If you don't use polymorphic parameters in plpgsql, then proposed feature
can look like useless.
Regards

Pavel

On Tue, Jan 19, 2016 at 4:53 AM, Pavel Stehule <pavel.stehule@gmail.com> wrote:

It is, but sometime the polymorphic types can help.

The proposed feature/syntax has sense primary for polymorphic types. It
should to follow our polymorphic types. The primary pair is
"anyarray","anyelement" -> "arraytype","elemementtype".

If you don't use polymorphic parameters in plpgsql, then proposed feature
can look like useless.

I don't think it's useless, but I do think the syntax is ugly. Maybe
it's the best we can do and we should just live with it, but Alvaro
asked for opinions, so there's mine.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

2016-01-20 0:34 GMT+01:00 Robert Haas <robertmhaas@gmail.com>:

On Tue, Jan 19, 2016 at 4:53 AM, Pavel Stehule <pavel.stehule@gmail.com>
wrote:

It is, but sometime the polymorphic types can help.

The proposed feature/syntax has sense primary for polymorphic types. It
should to follow our polymorphic types. The primary pair is
"anyarray","anyelement" -> "arraytype","elemementtype".

If you don't use polymorphic parameters in plpgsql, then proposed feature
can look like useless.

I don't think it's useless, but I do think the syntax is ugly. Maybe
it's the best we can do and we should just live with it, but Alvaro
asked for opinions, so there's mine.

ok

5 years ago, maybe more - I proposed more nice syntax - and it was rejected
as too complex (reserved worlds was required). So this solution try to
attack it from different side. It is simple and effective.

Regards

Pavel

Hi

2016-01-18 21:37 GMT+01:00 Alvaro Herrera <alvherre@2ndquadrant.com>:

diff --git a/src/pl/plpgsql/src/pl_comp.c b/src/pl/plpgsql/src/pl_comp.c
new file mode 100644
index 1ae4bb7..c819517
*** a/src/pl/plpgsql/src/pl_comp.c
--- b/src/pl/plpgsql/src/pl_comp.c
*************** plpgsql_parse_tripword(char *word1, char
*** 1617,1622 ****
--- 1617,1677 ----
return false;
}

+ /*
+  * Derive type from ny base type controlled by reftype_mode
+  */
+ static PLpgSQL_type *
+ derive_type(PLpgSQL_type *base_type, int reftype_mode)
+ {
+     Oid typoid;

I think you should add a typedef to the REFTYPE enum, and have this
function take that type rather than int.

done

+             case PLPGSQL_REFTYPE_ARRAY:
+             {
+                     /*
+                      * Question: can we allow anyelement (array or

nonarray) -> array direction.

+ * if yes, then probably we have to modify

enforce_generic_type_consistency,

+ * parse_coerce.c where still is check on scalar

type -> raise error

+ * ERROR: 42704: could not find array type for

data type integer[]

+ *
+ if

(OidIsValid(get_element_type(base_type->typoid)))

+                             return base_type;
+                     */

I think it would be better to resolve this question outside a code
comment.

done

+                     typoid = get_array_type(base_type->typoid);
+                     if (!OidIsValid(typoid))
+                             ereport(ERROR,
+

(errcode(ERRCODE_DATATYPE_MISMATCH),

+ errmsg("there are not

array type for type %s",

+

format_type_be(base_type->typoid))));

nodeFuncs.c uses this wording:
errmsg("could not find array type for data type %s",
which I think you should adopt.

sure, fixed

--- 1681,1687 ----
* ----------
*/
PLpgSQL_type *
! plpgsql_parse_wordtype(char *ident, int reftype_mode)
{
PLpgSQL_type *dtype;
PLpgSQL_nsitem *nse;

Use the typedef'ed enum, as above.

--- 1699,1721 ----
switch (nse->itemtype)
{
case PLPGSQL_NSTYPE_VAR:
!                     {
!                             dtype = ((PLpgSQL_var *)

(plpgsql_Datums[nse->itemno]))->datatype;

! return derive_type(dtype, reftype_mode);
! }

! case PLPGSQL_NSTYPE_ROW:
! {
! dtype = ((PLpgSQL_row *)

(plpgsql_Datums[nse->itemno]))->datatype;

! return derive_type(dtype, reftype_mode);
! }

+                     /*
+                      * XXX perhaps allow REC here? Probably it has not

any sense, because

+ * in this moment, because PLpgSQL doesn't support

rec parameters, so

+ * there should not be any rec polymorphic

parameter, and any work can

+                      * be done inside function.
+                      */

I think you should remove from the "?" onwards in that comment, i.e.
just keep what was already in the original comment (minus the ROW)

I tried to fix it, not sure if understood well.

--- 1757,1763 ----
* ----------
*/
PLpgSQL_type *
! plpgsql_parse_cwordtype(List *idents, int reftype_mode)
{
PLpgSQL_type *dtype = NULL;
PLpgSQL_nsitem *nse;

Typedef.

--- 2720,2737 ----
tok = yylex();
if (tok_is_keyword(tok, &yylval,
K_TYPE, "type"))
!                             result = plpgsql_parse_wordtype(dtname,

PLPGSQL_REFTYPE_TYPE);

! else if (tok_is_keyword(tok, &yylval,
!

K_ELEMENTTYPE, "elementtype"))

! result = plpgsql_parse_wordtype(dtname,

PLPGSQL_REFTYPE_ELEMENT);

! else if (tok_is_keyword(tok, &yylval,
!

K_ARRAYTYPE, "arraytype"))

! result = plpgsql_parse_wordtype(dtname,

PLPGSQL_REFTYPE_ARRAY);

else if (tok_is_keyword(tok, &yylval,

K_ROWTYPE, "rowtype"))

result = plpgsql_parse_wordrowtype(dtname);
! if (result)
! return result;
}

This plpgsql parser stuff is pretty tiresome. (Not this patch's fault
-- just saying.)

*************** extern bool plpgsql_parse_dblword(char *
*** 961,968 ****
PLwdatum *wdatum, PLcword

*cword);

extern bool plpgsql_parse_tripword(char *word1, char *word2, char

*word3,

PLwdatum *wdatum, PLcword

*cword);

! extern PLpgSQL_type *plpgsql_parse_wordtype(char *ident);
! extern PLpgSQL_type *plpgsql_parse_cwordtype(List *idents);
extern PLpgSQL_type *plpgsql_parse_wordrowtype(char *ident);
extern PLpgSQL_type *plpgsql_parse_cwordrowtype(List *idents);
extern PLpgSQL_type *plpgsql_build_datatype(Oid typeOid, int32 typmod,
--- 973,980 ----
PLwdatum *wdatum, PLcword

*cword);

extern bool plpgsql_parse_tripword(char *word1, char *word2, char

*word3,

PLwdatum *wdatum, PLcword

*cword);

! extern PLpgSQL_type *plpgsql_parse_wordtype(char *ident, int

reftype_mode);

! extern PLpgSQL_type *plpgsql_parse_cwordtype(List *idents, int

reftype_mode);

extern PLpgSQL_type *plpgsql_parse_wordrowtype(char *ident);
extern PLpgSQL_type *plpgsql_parse_cwordrowtype(List *idents);
extern PLpgSQL_type *plpgsql_build_datatype(Oid typeOid, int32 typmod,

By the way, these functions are misnamed after this patch. They are
called "wordtype" and "cwordtype" originally because they accept
"word%TYPE" and "compositeword%TYPE", but after the patch they not only
accept TYPE at the right of the percent sign but also ELEMENTTYPE and
ARRAYTYPE. Not sure that this is something we want to be too strict
about.

Understand - used name ***reftype instead ****type

*** a/src/test/regress/expected/plpgsql.out
--- b/src/test/regress/expected/plpgsql.out
*************** end;
*** 5573,5575 ****
--- 5573,5667 ----

I think you should also add your array_init() example to the test set.

done

Thank you for your comment

Attached updated patch

Regards

Pavel

It seems all fixes are done. I tested the patch and regression tests passed.

On 27.01.2016 20:58, Pavel Stehule wrote:

--- 1681,1687 ----
* ----------
*/
PLpgSQL_type *
! plpgsql_parse_wordtype(char *ident, int reftype_mode)
{
PLpgSQL_type *dtype;
PLpgSQL_nsitem *nse;

Use the typedef'ed enum, as above.

--- 1699,1721 ----
switch (nse->itemtype)
{
case PLPGSQL_NSTYPE_VAR:
!                     {
!                             dtype = ((PLpgSQL_var *)

(plpgsql_Datums[nse->itemno]))->datatype;

! return derive_type(dtype,

reftype_mode);

! }

! case PLPGSQL_NSTYPE_ROW:
! {
! dtype = ((PLpgSQL_row *)

(plpgsql_Datums[nse->itemno]))->datatype;

! return derive_type(dtype,

reftype_mode);

! }

+                     /*
+                      * XXX perhaps allow REC here? Probably it

has not any sense, because

+ * in this moment, because PLpgSQL doesn't

support rec parameters, so

+ * there should not be any rec polymorphic

parameter, and any work can

+                      * be done inside function.
+                      */

I think you should remove from the "?" onwards in that comment, i.e.
just keep what was already in the original comment (minus the ROW)

I tried to fix it, not sure if understood well.

I think here Alvaro means that you should keep original comment without
the ROW. Like this:

/* XXX perhaps allow REC here? */

*************** extern bool plpgsql_parse_dblword(char *
*** 961,968 ****
PLwdatum *wdatum, PLcword

*cword);

extern bool plpgsql_parse_tripword(char *word1, char *word2,

char *word3,

PLwdatum *wdatum,

PLcword *cword);

! extern PLpgSQL_type *plpgsql_parse_wordtype(char *ident);
! extern PLpgSQL_type *plpgsql_parse_cwordtype(List *idents);
extern PLpgSQL_type *plpgsql_parse_wordrowtype(char *ident);
extern PLpgSQL_type *plpgsql_parse_cwordrowtype(List *idents);
extern PLpgSQL_type *plpgsql_build_datatype(Oid typeOid, int32

typmod,

--- 973,980 ----
PLwdatum *wdatum, PLcword

*cword);

extern bool plpgsql_parse_tripword(char *word1, char *word2,

char *word3,

PLwdatum *wdatum,

PLcword *cword);

! extern PLpgSQL_type *plpgsql_parse_wordtype(char *ident, int

reftype_mode);

! extern PLpgSQL_type *plpgsql_parse_cwordtype(List *idents, int

reftype_mode);

extern PLpgSQL_type *plpgsql_parse_wordrowtype(char *ident);
extern PLpgSQL_type *plpgsql_parse_cwordrowtype(List *idents);
extern PLpgSQL_type *plpgsql_build_datatype(Oid typeOid, int32

typmod,

By the way, these functions are misnamed after this patch. They are
called "wordtype" and "cwordtype" originally because they accept
"word%TYPE" and "compositeword%TYPE", but after the patch they not only
accept TYPE at the right of the percent sign but also ELEMENTTYPE and
ARRAYTYPE. Not sure that this is something we want to be too strict
about.

Understand - used name ***reftype instead ****type

I am not sure, but it seems that new names is a little worse. I think
original names are good too. They accept a word and return the
PLpgSQL_type structure.

Thank you for your comment

Attached updated patch

Regards

Pavel

--
�lvaro Herrera http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

I noticed a little typo in the comment in the derive_type():
/* Return base_type, when it is a array already */

should be:
/* Return base_type, when it is an array already */

--
Artur Zakirov
Postgres Professional: http://www.postgrespro.com
Russian Postgres Company

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Hi

I am sending updated version - the changes are related to fix comments.

2016-02-19 10:41 GMT+01:00 Artur Zakirov <a.zakirov@postgrespro.ru>:

It seems all fixes are done. I tested the patch and regression tests
passed.

I think here Alvaro means that you should keep original comment without
the ROW. Like this:

/* XXX perhaps allow REC here? */

I tried rewording this comment

By the way, these functions are misnamed after this patch. They are
called "wordtype" and "cwordtype" originally because they accept
"word%TYPE" and "compositeword%TYPE", but after the patch they not
only
accept TYPE at the right of the percent sign but also ELEMENTTYPE and
ARRAYTYPE. Not sure that this is something we want to be too strict
about.

Understand - used name ***reftype instead ****type

I am not sure, but it seems that new names is a little worse. I think
original names are good too. They accept a word and return the PLpgSQL_type
structure.

The "TYPE" word in this name was related to syntax %TYPE. And because new
syntax allows more constructs, then the change name is correct. I am think.
But choosing names is hard work. The new name little bit more strongly show
relation to work with referenced types.

I noticed a little typo in the comment in the derive_type():
/* Return base_type, when it is a array already */

should be:
/* Return base_type, when it is an array already */

fixed

Regards

Pavel

On 21.02.2016 11:31, Pavel Stehule wrote:

Hi

I am sending updated version - the changes are related to fix comments.

Great.

I am new in reviewing, I think Pavel took into account all comments.
This patch is compiled and regression tests are passed. So I change its
status to "Ready for Committer".

By the way, these functions are misnamed after this patch.
They are
called "wordtype" and "cwordtype" originally because they
accept
"word%TYPE" and "compositeword%TYPE", but after the patch
they not only
accept TYPE at the right of the percent sign but also
ELEMENTTYPE and
ARRAYTYPE. Not sure that this is something we want to be
too strict
about.

Understand - used name ***reftype instead ****type

I am not sure, but it seems that new names is a little worse. I
think original names are good too. They accept a word and return the
PLpgSQL_type structure.

The "TYPE" word in this name was related to syntax %TYPE. And because
new syntax allows more constructs, then the change name is correct. I am
think. But choosing names is hard work. The new name little bit more
strongly show relation to work with referenced types.

Agree.

--
Artur Zakirov
Postgres Professional: http://www.postgrespro.com
Russian Postgres Company

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers