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

At Tue, 28 Apr 2020 12:56:19 +0900, Fujii Masao  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 + 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