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

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)

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

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

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

By the way coudln't we use int128 instead for internal arithmetic? I
think that makes the code simpler.

regards.

--
Kyotaro Horiguchi
NTT Open Source Software Center