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