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

Hi

2014-07-26 20:39 GMT+02:00 Marko Tiikkaja <marko(at)joh(dot)to>:

> 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.
>
>
+1

Regards

Pavel

>
> .marko
>
>
> --
> Sent via pgsql-hackers mailing list (pgsql-hackers(at)postgresql(dot)org)
> To make changes to your subscription:
> http://www.postgresql.org/mailpref/pgsql-hackers
>
>

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.

2014-08-12 15:09 GMT+02:00 Fabien COELHO <coelho(at)cri(dot)ensmp(dot)fr>:

>
> 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.
>
>
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 ?? Or
we have to documented it as potential compatibility issue

Regards

Pavel

> --
> Fabien.
>
>
>
> --
> Sent via pgsql-hackers mailing list (pgsql-hackers(at)postgresql(dot)org)
> To make changes to your subscription:
> http://www.postgresql.org/mailpref/pgsql-hackers
>

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

2014-08-12 19:14 GMT+02:00 Fabien COELHO <coelho(at)cri(dot)ensmp(dot)fr>:

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

I prefer error, although there is possibility to control a force of
exception - error or warning via extra plpgsql checks - this kind of error
is strange - and stored procedures usually can be fixed later because
plpgsql source is available.

There was a precedent with more precious query syntax check more times.
Just it should be well documented.

Regards

Pavel

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