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(dot)fujii(at)oss(dot)nttdata(dot)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