[raster] ST_MapAlgebra checks callbacks parameters but not strictness

[strk]

I've spent all morning wondering why ST_MapAlgebra was not calling my callback function, to finally find the culprit was the function being defined as being "strict": ​https://github.com/postgis/postgis/blob/svn-trunk/raster/rt_pg/rtpg_mapalgebra.c#L476-L478

I guess the null being passed is the userarg, which I'm not requesting. That argument, in the callback, is marked as being "variadic".

I don't know if it would be safe to call a strict function with a null value as a variadic argument (I'd guess so) but the point of this ticket is that silently skipping the callback invocation is not a nice thing to do, as the user is left wondering why the callback is not being invoked.

What we could do is raise an EXCEPTION if any NULL is to be passed to a STRICT function. For a start, that'd give the user an hint.

If there are any other possible NULL values that could get passed, and would depend on the input raster/band/values rather than on the call to ST_MapAlgebra then the message could become a WARNING instead, but, does such case exist ?

[dustymugs]

You can't have a parameter be NULL for a STRICT function. I'll see about adding a check for STRICT callback functions and then raise an EXCEPTION. There wouldn't be any other NULL parameter values passed to the callback from ST_MapAlgebra.

[strk]

I'm actually testing a patch already, if you hadn't started, don't.

[strk]

Pushed fix in r12720, should it be backported, what do you think Bborie ?

[dustymugs]

I don't think the user should be penalized for having a STRICT callback function and not providing any userarg. Maybe the correct solution is for the internal callback handler to pass an empty text array to the user's callback function.

[strk]

empty text array sounds good to me. Does the code check that the callback's 3rd argument is an array ?

[dustymugs]

The code checks that the argument is not NULL. Otherwise, the argument will be an array as PostgreSQL itself would have complained before getting to us.

[strk]

I meant the 3rd argument expected by the user callback signature. Does ST_MapAlgebra code checks that the 3rd argument expected by the user callback signature is of the "array" type ? To me, the less requirements we enforce, the better. Like if a user doesn't need custom arguments passed to the callback I would not even require a 3rd argument…

In any case, for the sake of this ticket, anything that complains if a requirement is not met (rather than doing something which can be unexpected) is ok. Would be better to use other tickets for other enhancements (like changing the requirements).

[dustymugs]

Fixed in -trunk as of r12734. Fixed in -2.1 as of r12736

[strk]

Thanks! Maybe the comment in line 653 here should be updated: trac.osgeo.org/postgis/changeset/12736/branches/2.1/raster/test/regress/rt_mapalgebra.sql