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

At Thu, 7 May 2020 13:17:01 +0900, Fujii Masao <masao.fujii@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