At Thu, 7 May 2020 13:17:01 +0900, Fujii Masao <masao.fu...@oss.nttdata.com>
wrote in
>
>
> On 2020/05/07 11:21, Kyotaro Horiguchi wrote:
> > +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);
Mmm.
>
> > + /* 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?
The function is called while executing an expression, so "NaN cannot
be used in this expression" or something like that would work.
> > + 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?
It's just a waste of stack depth by over 200 bytes. I doesn't lead to
an actual problem but it is evidently useless.
> > 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.
In second thought, I found that we don't have enough substitute
functions for the platforms without a native implement. Instead,
there are some overflow-safe uint64 math functions, that is,
pg_add/sub_u64_overflow. This patch defines numeric_pg_lsn which is
substantially numeric_uint64. By using them, for example, we can make
pg_lsn_pli mainly with integer arithmetic as follows.
Datum
pg_lsn_pli(..)
{
XLogRecPtr lsn = PG_GETARG_LSN(0);
Datum num_nbytes = PG_GETARG_DATUM(1);
Datum u64_nbytes =
DatumGetInt64(DirectFunctionCall1(numeric_pg_lsn, num_nbytes));
XLogRecPtr result;
if (pg_add_u64_overflow(lsn, u64_nbytes, &result))
elog(ERROR, "result out of range");
PG_RETURN_LSN(result);
}
If invalid values are given as the addend, the following message would
make sense.
=# select '1/1::pg_lsn + 'NaN'::numeric;
ERROR: cannot use NaN in this expression
=# select '1/1::pg_lsn + '-1'::numeric;
ERROR: numeric value out of range for this expression
regards.
--
Kyotaro Horiguchi
NTT Open Source Software Center
