Re: +(pg_lsn, int8) and -(pg_lsn, int8) operators

On 2020/05/08 10:00, Kyotaro Horiguchi wrote:
> At Thu, 7 May 2020 13:17:01 +0900, Fujii Masao <masao(dot)fujii(at)oss(dot)nttdata(dot)com> wrote in
>>
>>
>> On 2020/05/07 11:21, Kyotaro Horiguchi wrote:
>>> +static bool
>>> +numericvar_to_uint64(const NumericVar *var, uint64 *result)
>>> Other numricvar_to_xxx() functions return an integer value that means
>>> success by 0 and failure by -1, which is one of standard signature of
>>> this kind of functions. I don't see a reason for this function to
>>> have different signatures from them.
>>
>> Unless I'm missing something, other functions also return boolean.
>> For example,
>>
>> static bool numericvar_to_int32(const NumericVar *var, int32 *result);
>> static bool numericvar_to_int64(const NumericVar *var, int64 *result);
>
> Mmm.
>
>>
>>> + /* XXX would it be better to return NULL? */
>>> + if (NUMERIC_IS_NAN(num))
>>> + ereport(ERROR,
>>> + (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
>>> + errmsg("cannot convert NaN to pg_lsn")));
>>> The ERROR seems perfect to me since NaN is out of the domain of
>>> LSN. log(-1) results in a similar error.
>>> On the other hand, the code above makes the + operator behave as the
>>> follows.
>>> =# SELECT '1/1'::pg_lsn + 'NaN'::numeric;
>>> ERROR: cannot convert NaN to pg_lsn
>>> This looks somewhat different from what actually wrong is.
>>
>> You mean that pg_lsn_pli() and pg_lsn_mii() should emit an error like
>> "the number of bytes to add/subtract cannnot be NaN" when NaN is
>> specified?
>
> The function is called while executing an expression, so "NaN cannot
> be used in this expression" or something like that would work.

This sounds ambiguous. I like to use clearer messages like

cannot add NaN to pg_lsn
cannot subtract NaN from pg_lsn

>>> + char buf[256];
>>> +
>>> + /* Convert to numeric */
>>> + snprintf(buf, sizeof(buf), UINT64_FORMAT, lsn);
>>> The values larger than 2^64 is useless. So 32 (or any value larger
>>> than 21) is enough for the buffer length.
>>
>> Could you tell me what the actual problem is when buf[256] is used?
>
> It's just a waste of stack depth by over 200 bytes. I doesn't lead to
> an actual problem but it is evidently useless.
>
>>> By the way coudln't we use int128 instead for internal arithmetic? I
>>> think that makes the code simpler.
>>
>> I'm not sure if int128 is available in every environments.
>
> In second thought, I found that we don't have enough substitute
> functions for the platforms without a native implement. Instead,
> there are some overflow-safe uint64 math functions, that is,
> pg_add/sub_u64_overflow. This patch defines numeric_pg_lsn which is
> substantially numeric_uint64. By using them, for example, we can make
> pg_lsn_pli mainly with integer arithmetic as follows.

Sorry, I'm not sure what the benefit of this approach...

>
> Datum
> pg_lsn_pli(..)
> {
> XLogRecPtr lsn = PG_GETARG_LSN(0);
> Datum num_nbytes = PG_GETARG_DATUM(1);
> Datum u64_nbytes =
> DatumGetInt64(DirectFunctionCall1(numeric_pg_lsn, num_nbytes));
> XLogRecPtr result;
>
> if (pg_add_u64_overflow(lsn, u64_nbytes, &result))
> elog(ERROR, "result out of range");
>
> PG_RETURN_LSN(result);
> }
>
> If invalid values are given as the addend, the following message would
> make sense.
>
> =# select '1/1::pg_lsn + 'NaN'::numeric;
> ERROR: cannot use NaN in this expression
> =# select '1/1::pg_lsn + '-1'::numeric;
> ERROR: numeric value out of range for this expression

Could you tell me why we should reject this calculation?
IMO it's ok to add the negative number, and which is possible
with the latest patch.

Regards,

--
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION