Re: [HACKERS] plpgsql - additional extra checks

2018-03-04 2:46 GMT+01:00 Tomas Vondra <tomas(dot)vondra(at)2ndquadrant(dot)com>:

> On 03/02/2018 10:30 PM, Pavel Stehule wrote:
> > Hi
> >
> > 2018-03-01 21:14 GMT+01:00 David Steele <david(at)pgmasters(dot)net
> > <mailto:david(at)pgmasters(dot)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.
>

Tomas, thank you for correction.

Regards

Pavel

>
>
> 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
>