On 2015-07-08 14:11:59 +0900, Michael Paquier wrote:
> Arg... I thought I triggered a couple of weeks a problem in this code
> path when desc->arg_arraytype[i] is InvalidOid with argtypes == NULL.
> Visibly I did something wrong...
>
> Speaking of which, shouldn't this thing at least use OidIsValid?
> - if (fcinfo->flinfo->fn_oid)
> + if (OidIsValid(fcinfo->flinfo->fn_oid))
> get_func_signature(fcinfo->flinfo->fn_oid, &argtypes, &nargs);
We do the current type of tests in a bunch of places, I'd not modify
code just to change it. But since apparently the whole test is
pointless, I could see replacing it by an assert.
> >> 4) Return result of timestamp2tm is not checked 2 times in
> >> GetCurrentDateTime and GetCurrentTimeUsec, while all the other 40
> >> calls of this function do sanity checks. Returning
> >> ERRCODE_DATETIME_VALUE_OUT_OF_RANGE in case of an error would be good
> >> for consistency. See 0004. (issue reported previously here
> >> CAB7nPqRSk=J8eUdd55fL-w+k=8sDTHLVBt-cgG9jWN=VO2ogBQ@mail.gmail.com)
> >
> > But the other cases accept either arbitrary input or perform timezone
> > conversions. Not the case here, no?
>
> The callers of GetCurrentDateTime() and GetCurrentTimeUsec() do some
> work on pg_tm, see time_timetz() that does accept some input
> DecodeDateTime() for example.
So what? GetCurrentDateTime()'s returned data is still pretty much
guaranteed to be correct unless a lot of things have gone wrong
previously?
> In any case, we are going to need at least (void) in front of those calls.
We're "needing" nothing of the sort.