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

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
25 messages Options
12
Reply | Threaded
Open this post in threaded view
|

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

Fujii Masao-4
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

pg_lsn_operators_v1.patch (6K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

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

Robert Haas
On Thu, Apr 23, 2020 at 5:21 AM Fujii Masao <[hidden email]> 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


Reply | Threaded
Open this post in threaded view
|

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

Ashutosh Bapat-2
In reply to this post by Fujii Masao-4
On Thu, Apr 23, 2020 at 2:51 PM Fujii Masao <[hidden email]> 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


Reply | Threaded
Open this post in threaded view
|

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

Robert Haas
On Thu, Apr 23, 2020 at 12:28 PM Ashutosh Bapat
<[hidden email]> 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


Reply | Threaded
Open this post in threaded view
|

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

Michael Paquier-2
In reply to this post by Robert Haas
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

signature.asc (849 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

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

Jehan-Guillaume de Rorthais
On Fri, 24 Apr 2020 16:24:14 +0900
Michael Paquier <[hidden email]> 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.


Reply | Threaded
Open this post in threaded view
|

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

Kyotaro Horiguchi-4
At Fri, 24 Apr 2020 12:15:26 +0200, Jehan-Guillaume de Rorthais <[hidden email]> wrote in

> On Fri, 24 Apr 2020 16:24:14 +0900
> Michael Paquier <[hidden email]> 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


Reply | Threaded
Open this post in threaded view
|

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

Robert Haas
On Sun, Apr 26, 2020 at 9:41 PM Kyotaro Horiguchi
<[hidden email]> 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


Reply | Threaded
Open this post in threaded view
|

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

Fujii Masao-4


On 2020/04/28 1:24, Robert Haas wrote:
> On Sun, Apr 26, 2020 at 9:41 PM Kyotaro Horiguchi
> <[hidden email]> 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

pg_lsn_operators_v2.patch (13K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

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

Michael Paquier-2
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

signature.asc (849 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

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

Fujii Masao-4


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


Reply | Threaded
Open this post in threaded view
|

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

Michael Paquier-2
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

signature.asc (849 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

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

Kyotaro Horiguchi-4
In reply to this post by Fujii Masao-4
At Tue, 28 Apr 2020 12:56:19 +0900, Fujii Masao <[hidden email]> 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


Reply | Threaded
Open this post in threaded view
|

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

Fujii Masao-4
In reply to this post by Michael Paquier-2


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


Reply | Threaded
Open this post in threaded view
|

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

Fujii Masao-4
In reply to this post by Kyotaro Horiguchi-4


On 2020/05/07 11:21, Kyotaro Horiguchi wrote:

> At Tue, 28 Apr 2020 12:56:19 +0900, Fujii Masao <[hidden email]> 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


Reply | Threaded
Open this post in threaded view
|

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

Fujii Masao-4
In reply to this post by Fujii Masao-4


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

pg_lsn_operators_v3.patch (12K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

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

Kyotaro Horiguchi-4
In reply to this post by Fujii Masao-4
At Thu, 7 May 2020 13:17:01 +0900, Fujii Masao <[hidden email]> 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


Reply | Threaded
Open this post in threaded view
|

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

Fujii Masao-4


On 2020/05/08 10:00, Kyotaro Horiguchi wrote:

> At Thu, 7 May 2020 13:17:01 +0900, Fujii Masao <[hidden email]> 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


Reply | Threaded
Open this post in threaded view
|

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

Kyotaro Horiguchi-4
At Fri, 8 May 2020 11:31:42 +0900, Fujii Masao <[hidden email]> 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


Reply | Threaded
Open this post in threaded view
|

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

Fujii Masao-4


On 2020/05/08 12:10, Kyotaro Horiguchi wrote:

> At Fri, 8 May 2020 11:31:42 +0900, Fujii Masao <[hidden email]> 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

pg_lsn_operators_v4.patch (16K) Download Attachment
12