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