Thread: +(pg_lsn, int8) and -(pg_lsn, int8) operators
Hi, I'd like to propose to introduce the +(pg_lsn, int8) and -(pg_lsn, int8) operators. The + operator allows us to add the number of bytes into pg_lsn, resulting new pg_lsn. The - operator allows us to substract the number of bytes from pg_lsn, resulting new pg_lsn. Thought? I sometimes need these features for debuging purpose. Attached is the patch implementing those operators. Of course, this is the dev item for v14. Regards, -- Fujii Masao Advanced Computing Technology Center Research and Development Headquarters NTT DATA CORPORATION
Attachment
On Thu, Apr 23, 2020 at 5:21 AM Fujii Masao <masao.fujii@oss.nttdata.com> wrote: > I'd like to propose to introduce the +(pg_lsn, int8) and -(pg_lsn, int8) > operators. The + operator allows us to add the number of bytes into pg_lsn, > resulting new pg_lsn. The - operator allows us to substract the number > of bytes from pg_lsn, resulting new pg_lsn. Thought? > I sometimes need these features for debuging purpose. For anyone who missed it, this idea was popular on Twitter: https://twitter.com/fujii_masao/status/1252652020487487488 -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
On Thu, Apr 23, 2020 at 2:51 PM Fujii Masao <masao.fujii@oss.nttdata.com> wrote: > > Hi, > > I'd like to propose to introduce the +(pg_lsn, int8) and -(pg_lsn, int8) > operators. The + operator allows us to add the number of bytes into pg_lsn, > resulting new pg_lsn. The - operator allows us to substract the number > of bytes from pg_lsn, resulting new pg_lsn. Thought? > I sometimes need these features for debuging purpose. As it's presented in the patch I don't see much value in calling it as LSN arithmetic. If we could do something like LSN of Nth WAL record +/- <number of WAL records, n> = LSN of N+/- n th log record that would be interesting. :) -- Best Wishes, Ashutosh Bapat
On Thu, Apr 23, 2020 at 12:28 PM Ashutosh Bapat <ashutosh.bapat.oss@gmail.com> wrote: > As it's presented in the patch I don't see much value in calling it as > LSN arithmetic. If we could do something like LSN of Nth WAL record > +/- <number of WAL records, n> = LSN of N+/- n th log record that > would be interesting. :) Well, that would mean that the value of x + 1 would depend not only on x but on the contents of WAL, and that it would be uncomputable without having the WAL available, and that adding large values would be quite expensive. I much prefer Fujii Masao's proposal. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
On Thu, Apr 23, 2020 at 08:09:22AM -0400, Robert Haas wrote: > For anyone who missed it, this idea was popular on Twitter: > > https://twitter.com/fujii_masao/status/1252652020487487488 (For the sake of the archives) To which Alvaro, Robert, Fabrízio de Royes Mello, Julien Rouhaud and I answered positively to. -- Michael
Attachment
On Fri, 24 Apr 2020 16:24:14 +0900 Michael Paquier <michael@paquier.xyz> wrote: > On Thu, Apr 23, 2020 at 08:09:22AM -0400, Robert Haas wrote: > > For anyone who missed it, this idea was popular on Twitter: > > > > https://twitter.com/fujii_masao/status/1252652020487487488 > > (For the sake of the archives) > To which Alvaro, Robert, Fabrízio de Royes Mello, Julien Rouhaud and I > answered positively to. And me, discretely, with a little heart.
At Fri, 24 Apr 2020 12:15:26 +0200, Jehan-Guillaume de Rorthais <jgdr@dalibo.com> wrote in > On Fri, 24 Apr 2020 16:24:14 +0900 > Michael Paquier <michael@paquier.xyz> wrote: > > > On Thu, Apr 23, 2020 at 08:09:22AM -0400, Robert Haas wrote: > > > For anyone who missed it, this idea was popular on Twitter: > > > > > > https://twitter.com/fujii_masao/status/1252652020487487488 > > > > (For the sake of the archives) > > To which Alvaro, Robert, Fabrízio de Royes Mello, Julien Rouhaud and I > > answered positively to. > > And me, discretely, with a little heart. +1. I actually sometimes need it. y the way, -(pg_lsn, pg_lsn) yields a numeric. I feel that it could be confusing that the new operators takes a bigint. We need to cast the second term to bigint in the following expression. '2/20'::pg_lsn + ('1/10'::pg_lsn - '1/5'::pg_lsn) The new + operator is not commutative. I'm not sure it is the right desgin to make it commutative, but it would be irritatibe if it is not. (Or maybe we should implement them as functions rather than operators..) regards. -- Kyotaro Horiguchi NTT Open Source Software Center
On Sun, Apr 26, 2020 at 9:41 PM Kyotaro Horiguchi <horikyota.ntt@gmail.com> wrote: > +1. I actually sometimes need it. > > y the way, -(pg_lsn, pg_lsn) yields a numeric. It might be a good idea to use numeric here, too. Because int8 is signed, it's not big enough to cover the whole range of LSNs. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
On 2020/04/28 1:24, Robert Haas wrote: > On Sun, Apr 26, 2020 at 9:41 PM Kyotaro Horiguchi > <horikyota.ntt@gmail.com> wrote: >> +1. I actually sometimes need it. >> >> y the way, -(pg_lsn, pg_lsn) yields a numeric. > > It might be a good idea to use numeric here, too. Because int8 is > signed, it's not big enough to cover the whole range of LSNs. 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. Regards, -- Fujii Masao Advanced Computing Technology Center Research and Development Headquarters NTT DATA CORPORATION
Attachment
On Tue, Apr 28, 2020 at 12:56:19PM +0900, Fujii Masao wrote:
> 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.
- those write-ahead log locations.
+ those write-ahead log locations. Also the number of bytes can be added
+ into and substracted from LSN using the <literal>+</literal> and
+ <literal>-</literal> operators, respectively.
That's short. Should this mention the restriction with numeric (or
just recommend its use) because we don't have a 64b unsigned type
internally, basically Robert's point?
+ /* 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")));
That would be good to test, and an error sounds fine to me.
--
Michael
Attachment
On 2020/04/28 15:03, Michael Paquier wrote:
> On Tue, Apr 28, 2020 at 12:56:19PM +0900, Fujii Masao wrote:
>> 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.
>
> - those write-ahead log locations.
> + those write-ahead log locations. Also the number of bytes can be added
> + into and substracted from LSN using the <literal>+</literal> and
> + <literal>-</literal> operators, respectively.
> That's short. Should this mention the restriction with numeric (or
> just recommend its use) because we don't have a 64b unsigned type
> internally, basically Robert's point?
Thanks for the review! What about the following description?
-----------------
Also the number of bytes can be added into and substracted from LSN using the
<literal>+(pg_lsn,numeric)</literal> and <literal>-(pg_lsn,numeric)</literal>
operators, respectively. Note that the calculated LSN should be in the range
of <type>pg_lsn</type> type, i.e., between <literal>0/0</literal> and
<literal>FFFFFFFF/FFFFFFFF</literal>.
-----------------
>
> + /* 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")));
> That would be good to test, and an error sounds fine to me.
You mean that we should add the test that goes through this code block,
into the regression test?
Regards,
--
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION
On Thu, Apr 30, 2020 at 11:40:59PM +0900, Fujii Masao wrote:
> Also the number of bytes can be added into and substracted from LSN using the
> <literal>+(pg_lsn,numeric)</literal> and <literal>-(pg_lsn,numeric)</literal>
> operators, respectively. Note that the calculated LSN should be in the range
> of <type>pg_lsn</type> type, i.e., between <literal>0/0</literal> and
> <literal>FFFFFFFF/FFFFFFFF</literal>.
> -----------------
That reads fine.
>> + /* 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")));
>> That would be good to test, and an error sounds fine to me.
>
> You mean that we should add the test that goes through this code block,
> into the regression test?
Yes, that looks worth making sure to track, especially if the behavior
of this code changes in the future.
--
Michael
Attachment
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
On 2020/05/02 11:29, Michael Paquier wrote:
> On Thu, Apr 30, 2020 at 11:40:59PM +0900, Fujii Masao wrote:
>> Also the number of bytes can be added into and substracted from LSN using the
>> <literal>+(pg_lsn,numeric)</literal> and <literal>-(pg_lsn,numeric)</literal>
>> operators, respectively. Note that the calculated LSN should be in the range
>> of <type>pg_lsn</type> type, i.e., between <literal>0/0</literal> and
>> <literal>FFFFFFFF/FFFFFFFF</literal>.
>> -----------------
>
> That reads fine.
Ok, I will update the docs in that way.
>
>>> + /* 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")));
>>> That would be good to test, and an error sounds fine to me.
>>
>> You mean that we should add the test that goes through this code block,
>> into the regression test?
>
> Yes, that looks worth making sure to track, especially if the behavior
> of this code changes in the future.
Ok, I will add that regression test.
Regards,
--
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION
On 2020/05/07 11:21, Kyotaro Horiguchi wrote:
> 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)
Thanks! Will fix this.
>
> +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);
>
> + /* 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?
>
> + 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?
>
> 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.
Regards,
--
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION
On 2020/05/07 13:15, Fujii Masao wrote:
>
>
> On 2020/05/02 11:29, Michael Paquier wrote:
>> On Thu, Apr 30, 2020 at 11:40:59PM +0900, Fujii Masao wrote:
>>> Also the number of bytes can be added into and substracted from LSN using the
>>> <literal>+(pg_lsn,numeric)</literal> and <literal>-(pg_lsn,numeric)</literal>
>>> operators, respectively. Note that the calculated LSN should be in the range
>>> of <type>pg_lsn</type> type, i.e., between <literal>0/0</literal> and
>>> <literal>FFFFFFFF/FFFFFFFF</literal>.
>>> -----------------
>>
>> That reads fine.
>
> Ok, I will update the docs in that way.
Done.
>
>>
>>>> + /* 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")));
>>>> That would be good to test, and an error sounds fine to me.
>>>
>>> You mean that we should add the test that goes through this code block,
>>> into the regression test?
>>
>> Yes, that looks worth making sure to track, especially if the behavior
>> of this code changes in the future.
>
> Ok, I will add that regression test.
Done. Attached is the updated version of the patch!
Regards,
--
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION
Attachment
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
On 2020/05/08 10:00, Kyotaro Horiguchi wrote:
> 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.
This sounds ambiguous. I like to use clearer messages like
cannot add NaN to pg_lsn
cannot subtract NaN from pg_lsn
>>> + 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.
Sorry, I'm not sure what the benefit of this approach...
>
> 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
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.
Regards,
--
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION
At Fri, 8 May 2020 11:31:42 +0900, Fujii Masao <masao.fujii@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
On 2020/05/08 12:10, Kyotaro Horiguchi wrote: > At Fri, 8 May 2020 11:31:42 +0900, Fujii Masao <masao.fujii@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. Ok, I updated pg_lsn_pli() and pg_lsn_mii() so that they emit an error when NaN is specified as the number of bytes. >>>> 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. Ok, I changed the size of buf[] to 32. Attached is the updated version of the patch. Regards, -- Fujii Masao Advanced Computing Technology Center Research and Development Headquarters NTT DATA CORPORATION
Attachment
At Sat, 9 May 2020 23:40:15 +0900, Fujii Masao <masao.fujii@oss.nttdata.com> wrote in > > > On 2020/05/08 12:10, Kyotaro Horiguchi wrote: > > At Fri, 8 May 2020 11:31:42 +0900, Fujii Masao > > <masao.fujii@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. > > Ok, I updated pg_lsn_pli() and pg_lsn_mii() so that they emit an error > when NaN is specified as the number of bytes. It's fine with me. > > 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. > > Ok, I changed the size of buf[] to 32. > Attached is the updated version of the patch. Thank you very much! The patch looks good to me. regard. -- Kyotaro Horiguchi NTT Open Source Software Center
The following review has been posted through the commitfest application: make installcheck-world: tested, passed Implements feature: tested, passed Spec compliant: tested, passed Documentation: not tested The patch looks good to me. The new status of this patch is: Ready for Committer
On 2020/06/30 19:54, Asif Rehman wrote: > The following review has been posted through the commitfest application: > make installcheck-world: tested, passed > Implements feature: tested, passed > Spec compliant: tested, passed > Documentation: not tested > > The patch looks good to me. > > The new status of this patch is: Ready for Committer Thanks for the review! Pushed. Regards, -- Fujii Masao Advanced Computing Technology Center Research and Development Headquarters NTT DATA CORPORATION