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

On 2020/05/07 11:21, Kyotaro Horiguchi wrote:
> At Tue, 28 Apr 2020 12:56:19 +0900, Fujii Masao <masao.fujii@oss.nttdata.com> wrote in
>> Yes. Attached is the updated version of the patch, which introduces
>> +(pg_lsn, numeric) and -(pg_lsn, numeric) operators.
>> To implement them, I added also numeric_pg_lsn() function that
>> converts numeric to pg_lsn.
> 
> +    into and substracted from LSN using the <literal>+</literal> and
> 
> s/substracted/subtracted/
> (This still remains in the latest version)

Thanks! Will fix this.

> 
> +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);


> 
> +    /* 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?

> 
> +    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?

> 
> 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.

Regards,

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