Re: patch: enhanced get diagnostics statement 2

2011/7/14 Alvaro Herrera <alvherre@commandprompt.com>:
> A couple items for this patch:
>
> The docs state that the variable to receive each diagnostic value needs
> to be "of the right data type" but fails to specify what it is.  I think
> it'd be good to turn that <itemizedlist> into a table with three
> columns: name, type, description.
>
> This seems poor style:
>
> +                               case PLPGSQL_GETDIAG_ERROR_CONTEXT:
> +                               case PLPGSQL_GETDIAG_ERROR_DETAIL:
> +                               case PLPGSQL_GETDIAG_ERROR_HINT:
> +                               case PLPGSQL_GETDIAG_RETURNED_SQLSTATE:
> +                               case PLPGSQL_GETDIAG_MESSAGE_TEXT:
> +                                   if (!$2)
> +                                       ereport(ERROR,
> +                                           (errcode(ERRCODE_SYNTAX_ERROR),
> +                                            errmsg("EXCEPTION_CONTEXT or EXCEPTION_DETAIL or EXCEPTION_HINT or RETURNED_SQLSTATE or MESSAGE_TEXT are not allowed in current diagnostics statement"),
> +                                                    parser_errposition(@1)));
> +
>
>
> I think we could replace this with something like
>
> +                                   if (!$2)
> +                                       ereport(ERROR,
> +                                           (errcode(ERRCODE_SYNTAX_ERROR),
> +                                            errmsg("diagnostic value %s is not allowed in GET CURRENT DIAGNOSTICS statement", stringify(ditem->kind)),
>
>
> Other than that, and a few grammar fixes in code comments, this seems
> good to me.
>

it is good idea

Regards

Pavel

> --
> Álvaro Herrera <alvherre@commandprompt.com>
> The PostgreSQL Company - Command Prompt, Inc.
> PostgreSQL Replication, Consulting, Custom Development, 24x7 support
>