Re: [HACKERS] plpgsql - additional extra checks - Mailing list pgsql-hackers

On 03/02/2018 10:30 PM, Pavel Stehule wrote:
> Hi
> 
> 2018-03-01 21:14 GMT+01:00 David Steele <david@pgmasters.net
> <mailto:david@pgmasters.net>>:
> 
>     Hi Pavel,
> 
>     On 1/7/18 3:31 AM, Pavel Stehule wrote:
>     >
>     >     There, now it's in the correct Waiting for Author state. :)
>     >
>     > thank you for comments. All should be fixed in attached patch
> 
>     This patch no longer applies (and the conflicts do not look trivial).
>     Can you provide a rebased patch?
> 
>     $ git apply -3 ../other/plpgsql-extra-check-180107.patch
>     error: patch failed: src/pl/plpgsql/src/pl_exec.c:5944
>     Falling back to three-way merge...
>     Applied patch to 'src/pl/plpgsql/src/pl_exec.c' with conflicts.
>     U src/pl/plpgsql/src/pl_exec.c
> 
>     Marked as Waiting on Author.
> 
> 
> I am sending updated code. It reflects Tom's changes - now, the rec is
> used as row type too, so the checks must be on two places. With this
> update is related one change. When result is empty, then the extra
> checks doesn't work - PLpgSQL runtime doesn't pass necessary tupledesc.
> But usually, when result is empty, then there are not problems with
> missing values, because every value is NULL.
> 

I've looked at this patch today, and in general it seems in fairly good
shape - I don't really see any major issues in it that would mean it
can't be promoted to RFC soon.

A couple of comments though:

1) I think the docs are OK, but there are a couple of keywords that
should be wrapped in <literal> or <command> tags, otherwise the
formatting will be incorrect.

I've done that in the attached patch, as it's easier than listing which
keywords/where etc. I haven't wrapped the lines, though, to make it
easier to see the difference in meld or similar tools.


2) The does does a bunch of checks of log level, in the form

    if (too_many_rows_level >= WARNING)

which is perhaps a bit too verbose, because the default value of that
variable is 0. So

    if (too_many_rows_level)

would be enough, and it makes the checks a bit shorter. Again, this is
done in the attached patch.


3) There is a couple of typos in the comments, like "stric_" instead of
"strict_" and so on. Again, fixed in the patch, along with slightly
rewording a bunch of comments like

    /* no source for destination column */

instead of

    /* there are no data */

and so on.


4) I have also reworded the text of the two checks. Firstly, I've replaced

    query returned more than one row

with

    SELECT INTO query returned more than one row

which I think provides additional useful context to the user.

I've also replaced

    Number of evaluated fields does not match expected.

with

    Number of source and target fields in assignment does not match.

because the original text seems a bit cumbersome to me. It might be
useful to also include the expected/actual number of fields, to provide
a bit more context. That's valuable particularly for WARNING messages,
which do not include information about line numbers (or even function
name). So anything that helps to locate the query (of possibly many in
that function) is valuable.


Stephen: I see you're listed as reviewer on this patch - do you see an
issue blocking this patch from getting RFC? I see you did a review in
January, but Pavel seems to have resolved the issues you identified.


regards

-- 
Tomas Vondra                  http://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services