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

From: Alvaro Herrera <alvherre(at)2ndquadrant(dot)com>
To: Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com>
Cc: Artur Zakirov <a(dot)zakirov(at)postgrespro(dot)ru>, PostgreSQL Hackers <pgsql-hackers(at)postgresql(dot)org>
Subject: Re: plpgsql - DECLARE - cannot to use %TYPE or %ROWTYPE for composite types
Date: 2016-01-18 20:37:33
Message-ID: 20160118203733.GA123075@alvherre.pgsql
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: 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

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Alvaro Herrera 2016-01-18 20:46:45 Re: Removing service-related code in pg_ctl for Cygwin
Previous Message Robert Haas 2016-01-18 20:31:06 Re: Interesting read on SCM upending software and hardware architecture