Thread: PL/PgSQL: RAISE and the number of parameters

Me again,

Here's a patch for making PL/PgSQL throw an error during compilation
(instead of runtime) if the number of parameters passed to RAISE don't
match the number of placeholders in error message.  I'm sure people can
see the pros of doing it this way.


.marko

Hello Marko,

> Here's a patch for making PL/PgSQL throw an error during compilation (instead 
> of runtime) if the number of parameters passed to RAISE don't match the 
> number of placeholders in error message.  I'm sure people can see the pros of 
> doing it this way.

Patch scanned, applied & tested without problem on head.

The compile-time raise parameter checking is a good move.

3 minor points:

- I would suggest to avoid "continue" within a loop so that the code is 
simpler to understand, at least for me.
 - I would suggest to update the documentation accordingly.
 - The execution code now contains error detections which should never be 
raised, but I suggest to keep it in place anyway. However I would suggest 
to add comments about the fact that it should not be triggered.

See the attached patch which implements these suggestions on top of your 
patch.

-- 
Fabien.

Hi Fabien,

On 8/12/14 1:09 PM, Fabien COELHO wrote:
>> Here's a patch for making PL/PgSQL throw an error during compilation (instead
>> of runtime) if the number of parameters passed to RAISE don't match the
>> number of placeholders in error message.  I'm sure people can see the pros of
>> doing it this way.
>
> Patch scanned, applied & tested without problem on head.

Thanks!

> The compile-time raise parameter checking is a good move.
>
> 3 minor points:
>
> - I would suggest to avoid "continue" within a loop so that the code is
> simpler to understand, at least for me.

I personally find the code easier to read with the continue.

>    - I would suggest to update the documentation accordingly.

I scanned the docs trying to find any mentions of the run-time error but 
couldn't find any.  I don't object to adding this, though.

>    - The execution code now contains error detections which should never be
> raised, but I suggest to keep it in place anyway. However I would suggest
> to add comments about the fact that it should not be triggered.

Good point.  I actually realized I hadn't sent the latest version of the 
patch, sorry about that.  I did this:

https://github.com/johto/postgres/commit/1acab6fc5387c893ca29fed9284e09258e0c5c56

to also turn the ereport()s into elog()s since the user should never see 
them.

> See the attached patch which implements these suggestions on top of your
> patch.

Thanks for reviewing!  I'll incorporate the doc changes, but I'm going 
to let others decide on the logic in the check_raise_parameters() loop 
before doing any changes.

Will send an updated patch shortly.


.marko

Hello,

>> - I would suggest to avoid "continue" within a loop so that the code is
>> simpler to understand, at least for me.
>
> I personally find the code easier to read with the continue.

Hmmm. I had to read the code to check it, and I did it twice. The point is 
that there is 3 exit points instead of 1 in the block, which is not in 
itself very bad, but:
  for(...) {    if (ccc)      xxx;    ...    foo++;  }

It looks like "foo++" is always executed, and you have to notice that xxx 
a few lines above is a continue to realise that it is only when ccc is 
false. This is also disconcerting if it happens to be the "rare" case, 
i.e. here most of the time the char is not '%', so most of the time foo is 
not incremented, although it is a the highest level. Also the code with 
continue does not really put forward that what is counted is '%' followed 
by a non '%'. Note that the corresponding execution code 
(pgplsql/src/pl_exec.c) uses one continue to get rid of the second '%', 
which is quite defendable in that case as it is a kind of exception, but 
the main condition remains a simple if block. Final argument, the 
structured version is shorter than the unstructured version, with just the 
two continues removed, and one negation also removed.

> to also turn the ereport()s into elog()s since the user should never see 
> them.

I'm not sure why elog is better than ereport in that case: ISTM that it is 
an error worth reporting if it ever happens, say there is another syntax 
added later on which is not caught for some reason by the compile-time 
check, so I would not change it.

-- 
Fabien.

> one note: this patch can enforce a compatibility issues - a partially 
> broken functions, where some badly written RAISE statements was executed 
> newer.

> I am not against this patch, but it should be in extra check probably ??

I'm not sure about what you mean by "it should be in extra check".

> Or we have to documented it as potential compatibility issue.

Indeed, as a potential execution error is turned into a certain 
compilation error.

If this compatibility point is a blocker, the compilation error can be 
turned into a warning, but I would prefer to keep it an error: I'm quite 
sure I fell into that pit at least once or twice.

-- 
Fabien.

On 2014-08-12 13:23, I wrote:
>> The compile-time raise parameter checking is a good move.
>>
>> 3 minor points:
>>
>> - I would suggest to avoid "continue" within a loop so that the code is
>> simpler to understand, at least for me.
>
> I personally find the code easier to read with the continue.

I've changed the loop slightly.  Do you find this more readable than the
way the loop was previously written?

>>     - I would suggest to update the documentation accordingly.

I've incorporated these changes into this version of the patch, with
small changes.

On 2014-08-12 15:09, Fabien COELHO wrote:
 > I'm not sure why elog is better than ereport in that case: ISTM that
it is
 > an error worth reporting if it ever happens, say there is another syntax
 > added later on which is not caught for some reason by the compile-time
 > check, so I would not change it.

With elog(ERROR, ..) it's still reported, but the user isn't fooled into
thinking that the error is to be expected, and hopefully we would see a
bug report.  If it's impossible to tell the two errors apart, we might
have subtly broken code carried around for who knows how long.

Please let me know what you think about this patch.  Thanks for your
work so far.


.marko

Hello Marko,

> I've changed the loop slightly.  Do you find this more readable than the way 
> the loop was previously written?

It is 50% better:-)

It is no big deal, but I still fail to find the remaining continue as 
usefull in this case. If you remove the "continue" line and invert the 
condition, it works exactly the same, so it is just one useless 
instruction within that loop. From a logical point of view the loop is 
looking for '%' and then check whether the next char is '%' or not, so the 
straightforward code helps my understanding as it does exactly that, and 
the continue is just an hindrance to comprehension.

Note that I would buy it if it helped avoid indenting further a 
significant portion of complex code, but this is not the case here.

> [doc] I've incorporated these changes into this version of the patch, 
> with small changes.

Ok.

> With elog(ERROR, ..) it's still reported, but the user isn't fooled into 
> thinking that the error is to be expected, and hopefully we would see a bug 
> report.  If it's impossible to tell the two errors apart, we might have 
> subtly broken code carried around for who knows how long.

Ok.

In that case, it would make sense to keep distinct wordings of both 
exceptions in the execution code, so that they also can be set apart,
i.e. keep the "too many/few" somewhere in the error?

-- 
Fabien.

On 09/02/2014 11:52 AM, Fabien COELHO wrote:
>
>> I've changed the loop slightly.  Do you find this more readable than the way
>> the loop was previously written?
>
> It is 50% better:-)
>
> It is no big deal, but I still fail to find the remaining continue as
> usefull in this case. If you remove the "continue" line and invert the
> condition, it works exactly the same, so it is just one useless
> instruction within that loop. From a logical point of view the loop is
> looking for '%' and then check whether the next char is '%' or not, so the
> straightforward code helps my understanding as it does exactly that, and
> the continue is just an hindrance to comprehension.
>
> Note that I would buy it if it helped avoid indenting further a
> significant portion of complex code, but this is not the case here.

FWIW, I agree.

>> [doc] I've incorporated these changes into this version of the patch,
>> with small changes.
>
> Ok.
>
>> With elog(ERROR, ..) it's still reported, but the user isn't fooled into
>> thinking that the error is to be expected, and hopefully we would see a bug
>> report.  If it's impossible to tell the two errors apart, we might have
>> subtly broken code carried around for who knows how long.
>
> Ok.
>
> In that case, it would make sense to keep distinct wordings of both
> exceptions in the execution code, so that they also can be set apart,
> i.e. keep the "too many/few" somewhere in the error?

Well, you can do "set log_error_verbosity='verbose'" if you run into that.

I think this patch has been thoroughly reviewed now. Committed, thanks!

- Heikki

Hi,

On 2014-09-02 15:04, Heikki Linnakangas wrote:
> I think this patch has been thoroughly reviewed now. Committed, thanks!

Thank you, Heikki.  And also big thanks to Fabien for the review!


.marko