Re: [HACKERS] plpgsql - additional extra checks

2018-03-04 13:37 GMT+01:00 Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com>:

>
>
> 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.
>>
>
I am sending updated patch with Tomas changes

Regards

Pavel

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