Re: PL/PGSQL: Dynamic Record Introspection

[Neil Conway <neilc(at)samurai(dot)com>]

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