At Fri, 8 May 2020 11:31:42 +0900, Fujii Masao <masao.fu...@oss.nttdata.com> wrote in > >> 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. > > This sounds ambiguous. I like to use clearer messages like > > cannot add NaN to pg_lsn > cannot subtract NaN from pg_lsn
They works fine to me. > >> 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. > > Sorry, I'm not sure what the benefit of this approach... (If we don't allow negative nbytes,) We accept numeric so that the operators can accept values out of range of int64, but we don't need to perform all arithmetic in numeric. That approach does less numeric arithmetic, that is, faster and simpler. We don't need to string'ify LSN with it. That avoid stack consumption. > > 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 > > Could you tell me why we should reject this calculation? > IMO it's ok to add the negative number, and which is possible > with the latest patch. Sorry, I misread the patch as it rejected -1 for *nbytes*, by seeing numeric_pg_lsn. Finally, I'm convinced that we lack required integer arithmetic infrastructure to perform the objective. The patch looks good to me except the size of buf[], but I don't strongly object to that. regards. -- Kyotaro Horiguchi NTT Open Source Software Center
