From: | Tomas Vondra <tomas(dot)vondra(at)2ndquadrant(dot)com> |
---|---|
To: | Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com> |
Cc: | Alvaro Herrera <alvherre(at)2ndquadrant(dot)com>, nospam-abuse(at)bloodgate(dot)com, David Steele <david(at)pgmasters(dot)net>, Stephen Frost <sfrost(at)snowman(dot)net>, Michael Paquier <michael(dot)paquier(at)gmail(dot)com>, Daniel Gustafsson <daniel(at)yesql(dot)se>, Marko Tiikkaja <marko(at)joh(dot)to>, Jim Nasby <jim(dot)nasby(at)bluetreble(dot)com>, PostgreSQL Hackers <pgsql-hackers(at)postgresql(dot)org> |
Subject: | Re: [HACKERS] plpgsql - additional extra checks |
Date: | 2018-07-24 23:56:17 |
Message-ID: | ae2ea2f7-d1e9-dec9-50bf-4db76729fd53@2ndquadrant.com |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
On 07/22/2018 10:24 PM, Tomas Vondra wrote:
>
>
> On 07/19/2018 02:50 PM, Pavel Stehule wrote:
>>
>>
>> 2018-07-15 1:38 GMT+02:00 Tomas Vondra <tomas(dot)vondra(at)2ndquadrant(dot)com
>> <mailto:tomas(dot)vondra(at)2ndquadrant(dot)com>>:
>>
>> Hi,
>>
>> I've been looking at the version submitted on Thursday, planning to
>> commit it, but I think it needs a wee bit more work on the error
>> messages.
>>
>> 1) The example in sgml docs does not work, because it's missing a
>> semicolon after the CREATE FUNCTION. And the output was not updated
>> after tweaking the messages, so it still has uppercase+comma.
>>
>>
>> fixed
>>
>>
>> 2) The
>>
>> /* translator: %s represent a name of extra check */
>>
>> comment should be
>>
>> /* translator: %s represents a name of an extra check */
>>
>> 3) Both Andres and Alvaro suggested to pass extra_errors/extra_warnings
>> as a variable too, turning the errhint into
>>
>> errhint("%s check of %s is active.",
>> "too_many_rows",
>> (too_many_rows_level == ERROR) ? "extra_errors" :
>> "extra_checks");
>>
>> or something like that. Any particular reason not to do that?
>>
>>
>> errdetail was used on first place already.
>>
>> Now, I skip detail in this first case, and elsewhere I move this info to
>> detail, and hint has text that you proposed.
>>
>>
>> Sorry for the bikeshedding, but I'd like my first commit not to be
>> immediately torn apart by wolves ;-)
>>
>>
>> no problem
>>
>>
>> 4) I think the errhint() is used incorrectly. The message should be
>> about how to fix the issue, but messages like
>>
>> HINT: strict_multi_assignement check of extra_warnings is active.
>>
>> clearly does not help in this regard. This information should be either
>> in errdetail() is deemed useful, or left out entirely. And the errhint()
>> should say something like:
>>
>> errdetail("Make sure the query returns a single row, or use LIMIT 1.")
>>
>> and
>>
>> errdetail("Make sure the query returns the exact list of columns.")
>>
>>
>> should be fixed too
>>
>
> Seems OK to me. I'll go over the patch once more tomorrow and I plan to
> get it committed.
>
Committed, with some minor changes:
1) The too_many_rows check in exec_stmt_execsql included the errhint
conditionally, depending on force_error variable
force_error = stmt->strict || stmt->mod_stmt;
use_errhint = !force_error;
That is, the hint was not included when force_error=true, which does not
seem quite necessary. Based on off-list discussion with Pavel this was
unnecessary, so the hint is now included always.
2) The comment in plpgsql.h still mentioned "compile-time" checks only,
but the new checks are run-time checks. So tweaked accordingly.
3) A couple of minor formatting / code style changes.
regards
--
Tomas Vondra http://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
From | Date | Subject | |
---|---|---|---|
Next Message | Tom Lane | 2018-07-24 23:57:49 | Re: Early WIP/PoC for inlining CTEs |
Previous Message | Andres Freund | 2018-07-24 23:54:16 | Re: Early WIP/PoC for inlining CTEs |