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

Hi

2016-01-18 21:37 GMT+01:00 Alvaro Herrera <alvherre(at)2ndquadrant(dot)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

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