Opened 5 years ago

Closed 5 years ago

Last modified 5 years ago

#2803 closed defect (fixed)

[raster] ST_MapAlgebra checks callbacks parameters but not strictness

Reported by: strk Owned by: Bborie Park
Priority: medium Milestone: PostGIS 2.1.4
Component: raster Version: 2.1.x
Keywords: history Cc:

Description

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 ?

Change History (9)

comment:1 Changed 5 years ago by Bborie Park

Status: newassigned

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.

comment:2 Changed 5 years ago by strk

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

comment:3 Changed 5 years ago by strk

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

comment:4 Changed 5 years ago by Bborie Park

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.

comment:5 Changed 5 years ago by strk

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

comment:6 Changed 5 years ago by Bborie Park

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.

comment:7 Changed 5 years ago by 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).

comment:8 Changed 5 years ago by Bborie Park

Keywords: history added
Milestone: PostGIS 2.1.4
Resolution: fixed
Status: assignedclosed
Version: trunk2.1.x

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

comment:9 Changed 5 years ago by 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

Note: See TracTickets for help on using tickets.