Re: [HACKERS] Implementation of SQLCODE and SQLERRM variables

- You should write some regression tests for this functionality

- You should update the documentation

- Is there a reason why you've made the type of SQLCODE `text', rather
than integer?

Pavel Stehule wrote:
> + fict_vars_sect :
> + {
> + plpgsql_ns_setlocal(false);
> + PLpgSQL_variable *var;
> + var = plpgsql_build_variable(strdup("sqlcode"), 0,
> + plpgsql_build_datatype(TEXTOID, -1), true);
> + $$.sqlcode_varno = var->dno;
> + var = plpgsql_build_variable(strdup("sqlerrm"), 0,
> + plpgsql_build_datatype(TEXTOID, -1), true);

This shouldn't be strdup'ing its first argument (and even if it needed
to make a copy, it should use pstrdup). Also, my personal preference
would be to implement this without creating a new production (i.e. just
include it inline in the body of the pl_block production).

> *** src.old/pl_exec.c 2005-02-24 02:11:40.000000000 +0100
> --- src/pl_exec.c 2005-03-07 09:53:52.630243888 +0100
> ***************
> *** 809,814 ****
> --- 809,828 ----
> int i;
> int n;
>
> + /* setup SQLCODE and SQLERRM */
> + PLpgSQL_var *var;
> +
> + var = (PLpgSQL_var *) (estate->datums[block->sqlcode_varno]);
> + var->isnull = false;
> + var->freeval = false;
> + var->value = DirectFunctionCall1(textin, CStringGetDatum("00000"));
> +
> + var = (PLpgSQL_var *) (estate->datums[block->sqlerrm_varno]);
> + var->isnull = false;
> + var->freeval = false;
> + var->value = DirectFunctionCall1(textin, CStringGetDatum("Sucessful completion"));

`freeval' should be true, no? (Not sure it actually matters, but text is
certainly not pass-by-value).

> ***************
> *** 918,923 ****
> --- 932,957 ----
[...]
> + var = (PLpgSQL_var *) (estate->datums[block->sqlcode_varno]);
> + var->value = DirectFunctionCall1(textin, CStringGetDatum(tbuf));
> +
> + var = (PLpgSQL_var *) (estate->datums[block->sqlerrm_varno]);
> + var->value = DirectFunctionCall1(textin, CStringGetDatum(edata->message));

You should probably pfree() the old values before replacing them.

-Neil