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

On 2020/05/08 12:10, Kyotaro Horiguchi wrote:
> At Fri, 8 May 2020 11:31:42 +0900, Fujii Masao <masao.fujii@oss.nttdata.com> wrote in
>>>> 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
> 
> They works fine to me.

Ok, I updated pg_lsn_pli() and pg_lsn_mii() so that they emit an error
when NaN is specified as the number of bytes.


>>>> 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...
> 
> (If we don't allow negative nbytes,)
> We accept numeric so that the operators can accept values out of range
> of int64, but we don't need to perform all arithmetic in numeric. That
> approach does less numeric arithmetic, that is, faster and simpler.
> We don't need to string'ify LSN with it. That avoid stack consumption.
> 
>>> 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.
> 
> Sorry, I misread the patch as it rejected -1 for *nbytes*, by seeing
> numeric_pg_lsn.
> 
> Finally, I'm convinced that we lack required integer arithmetic
> infrastructure to perform the objective.
> 
> The patch looks good to me except the size of buf[], but I don't
> strongly object to that.

Ok, I changed the size of buf[] to 32.
Attached is the updated version of the patch.

Regards,

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