Re: PL/PGSQL: Dynamic Record Introspection - Mailing list pgsql-patches

Titus von Boxberg wrote:
> With the following patch it's possible to
> - extract all field names of a record into an array
> - extract field count of a record
> - address a single field of a record with a variable
>   containing the field name (additional to the usual record.fieldname
>   notation where the fieldname is hardcoded).

I wonder if this is the right syntax. record%identifier is doing
something fundamentally different from record.identifier, but the syntax
doesn't make that clear. I don't have any concrete suggestions for
improvement, mind you... :)

> Test code for the patch can be extracted from an example I put into
> plpgsql.sgml

Can you supply some proper regression tests, please? i.e. patch
sql/plpgsql.sql and expected/plpgsql.out in src/test/regress

A few minor comments from skimming the patch:

> ***************
> *** 995,1001 ****
>
>                   new = palloc(sizeof(PLpgSQL_recfield));
>                   new->dtype = PLPGSQL_DTYPE_RECFIELD;
> !                 new->fieldname = pstrdup(cp[1]);
>                   new->recparentno = ns->itemno;
>
>                   plpgsql_adddatum((PLpgSQL_datum *) new);
> --- 995,1002 ----
>
>                   new = palloc(sizeof(PLpgSQL_recfield));
>                   new->dtype = PLPGSQL_DTYPE_RECFIELD;
> !                 new->fieldindex.fieldname = strdup(cp[1]);
> !                 new->fieldindex_flag = RECFIELD_USE_FIELDNAME;
>                   new->recparentno = ns->itemno;
>
>                   plpgsql_adddatum((PLpgSQL_datum *) new);

Should be pstrdup().

> ***************
> *** 1101,1107 ****
>
>                   new = palloc(sizeof(PLpgSQL_recfield));
>                   new->dtype = PLPGSQL_DTYPE_RECFIELD;
> !                 new->fieldname = pstrdup(cp[2]);
>                   new->recparentno = ns->itemno;
>
>                   plpgsql_adddatum((PLpgSQL_datum *) new);
> --- 1102,1109 ----
>
>                   new = palloc(sizeof(PLpgSQL_recfield));
>                   new->dtype = PLPGSQL_DTYPE_RECFIELD;
> !                 new->fieldindex.fieldname = strdup(cp[2]);
> !                 new->fieldindex_flag = RECFIELD_USE_FIELDNAME;
>                   new->recparentno = ns->itemno;
>
>                   plpgsql_adddatum((PLpgSQL_datum *) new);

Ibid.

> +     switch (ns1->itemtype)
> +     {
> +         case PLPGSQL_NSTYPE_REC:
> +             {
> +                 PLpgSQL_recfieldproperties *new;
> +
> +                 new = malloc(sizeof(PLpgSQL_recfieldproperties));
> +                 new->dtype = PLPGSQL_DTYPE_NRECFIELD;
> +                 new->recparentno = ns1->itemno;
> +                 plpgsql_adddatum((PLpgSQL_datum *) new);
> +                 plpgsql_yylval.scalar = (PLpgSQL_datum *) new;
> +                 ret =  T_SCALAR;    /* ??? */
> +                 break;

Should be palloc().

> +     return ret;
> + } /* plpgsql_parse_wordnfields */

The Postgres convention is not to include comments like this.

> +         case PLPGSQL_NSTYPE_REC:
> +             {
> +                 PLpgSQL_recfieldproperties *new;
> +
> +                 new = malloc(sizeof(PLpgSQL_recfieldproperties));
> +                 new->dtype = PLPGSQL_DTYPE_RECFIELDNAMES;
> +                 new->recparentno = ns1->itemno;
> +                 plpgsql_adddatum((PLpgSQL_datum *) new);
> +                 plpgsql_yylval.scalar = (PLpgSQL_datum *) new;
> +                 ret =  T_SCALAR;    /* ??? */
> +                 break;
> +             }

Should be palloc().

> !                 /* construct_array copies data; free temp elem array */
> ! #if    0
> !                 for ( fc = 0; fc < tfc; ++fc )
> !                     pfree(DatumGetPointer(arrayelems[fc]);
> !                 pfree(arrayelems);
> ! #endif

Unexplained #if 0 blocks aren't good.

-Neil