2018-03-04 2:46 GMT+01:00 Tomas Vondra <tomas.von...@2ndquadrant.com>:
> On 03/02/2018 10:30 PM, Pavel Stehule wrote: > > Hi > > > > 2018-03-01 21:14 GMT+01:00 David Steele <da...@pgmasters.net > > <mailto:da...@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. > 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 >
