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