+(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
Attachments:
pg_lsn_operators_v1.patchtext/plain; charset=UTF-8; name=pg_lsn_operators_v1.patch; x-mac-creator=0; x-mac-type=0Download+96-1
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:
(For the sake of the archives)
To which Alvaro, Robert, Fabrízio de Royes Mello, Julien Rouhaud and I
answered positively to.
--
Michael
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:
(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:
(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
Attachments:
pg_lsn_operators_v2.patchtext/plain; charset=UTF-8; name=pg_lsn_operators_v2.patch; x-mac-creator=0; x-mac-type=0Download+238-1
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.
--
MichaelOn 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
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_lsnThis 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
Attachments:
pg_lsn_operators_v3.patchtext/plain; charset=UTF-8; name=pg_lsn_operators_v3.patch; x-mac-creator=0; x-mac-type=0Download+278-1
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 expressionCould 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_lsnThey 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 expressionCould 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
