Thread: Remove useless int64 range checks on BIGINT sequence MINVALUE/MAXVALUE values

Hi,

Sequence MINVALUE/MAXVALUE values are read into "int64" variables and
then range-checked according to the sequence data-type.
However, for a BIGINT sequence, checking whether these are <
PG_INT64_MIN or > PG_INT64_MAX always evaluates to false, as an int64
can't hold such values.
I've attached a patch to remove those useless checks.
The MINVALUE/MAXVALUE values are anyway int64 range-checked prior to
this, as part of conversion to int64.

Regards,
Greg Nancarrow
Fujitsu Australia

On Mon, 21 Jun 2021 at 22:10, Greg Nancarrow <gregn4422@gmail.com> wrote:
> Sequence MINVALUE/MAXVALUE values are read into "int64" variables and
> then range-checked according to the sequence data-type.
> However, for a BIGINT sequence, checking whether these are <
> PG_INT64_MIN or > PG_INT64_MAX always evaluates to false, as an int64
> can't hold such values.

It might be worth putting in a comment to mention that the check is
not needed.  Just in case someone looks again one day and thinks the
checks are missing.

Probably best to put this in the July commitfest so it does not get missed.

David

On Mon, Jun 21, 2021 at 8:32 PM David Rowley <dgrowleyml@gmail.com> wrote:
>
> It might be worth putting in a comment to mention that the check is
> not needed.  Just in case someone looks again one day and thinks the
> checks are missing.
>
> Probably best to put this in the July commitfest so it does not get missed.

Updated the patch, and will add it to the Commitfest, thanks.

Regards,
Greg Nancarrow
Fujitsu Australia

Those code comments look good.

On 21.06.21 13:32, Greg Nancarrow wrote:
> On Mon, Jun 21, 2021 at 8:32 PM David Rowley <dgrowleyml@gmail.com> wrote:
>>
>> It might be worth putting in a comment to mention that the check is
>> not needed.  Just in case someone looks again one day and thinks the
>> checks are missing.
>>
>> Probably best to put this in the July commitfest so it does not get missed.
> 
> Updated the patch, and will add it to the Commitfest, thanks.

I don't think this is a good change.  It replaces one perfectly solid, 
harmless, and readable line of code with six lines of comment explaining 
why the code isn't necessary (times two).  And the code is now less 
robust against changes elsewhere.  To maintain this robustness, you'd 
have to add assertions that prove that what the comment is saying is 
actually true, thus adding even more code.

I think we should leave it as is.

On Sat, 3 Jul 2021 at 22:44, Peter Eisentraut
<peter.eisentraut@enterprisedb.com> wrote:
> I don't think this is a good change.

> I think we should leave it as is.

I'm inclined to agree.

When I mentioned adding a comment I'd not imagined it would be quite
so verbose. Plus, I struggle to imagine there's any compiler out there
that someone would use that wouldn't just remove the check anyway. I
had a quick click around on https://godbolt.org/z/PnKeq5bsT and didn't
manage to find any compilers that didn't remove the check.

David

On Sun, 4 Jul 2021 at 20:53, David Rowley <dgrowleyml@gmail.com> wrote:
>
> On Sat, 3 Jul 2021 at 22:44, Peter Eisentraut
> <peter.eisentraut@enterprisedb.com> wrote:
> > I don't think this is a good change.
>
> > I think we should leave it as is.
>
> I'm inclined to agree.

Does anyone object to marking this patch as rejected in the CF app?

David

On Tue, Jul 6, 2021 at 8:43 PM David Rowley <dgrowleyml@gmail.com> wrote:
>
> On Sun, 4 Jul 2021 at 20:53, David Rowley <dgrowleyml@gmail.com> wrote:
> >
> > On Sat, 3 Jul 2021 at 22:44, Peter Eisentraut
> > <peter.eisentraut@enterprisedb.com> wrote:
> > > I don't think this is a good change.
> >
> > > I think we should leave it as is.
> >
> > I'm inclined to agree.
>
> Does anyone object to marking this patch as rejected in the CF app?
>

I think if you're going to reject this patch, a brief comment should
be added to that code to justify why that existing superfluous check
is worthwhile.
(After all, similar checks are not being done elsewhere in the
Postgres code, AFAICS. e.g. "int" variables are not being checked to
see whether they hold values greater than MAXINT).

Regards,
Greg Nancarrow
Fujitsu Australia

On Wed, 7 Jul 2021 at 00:06, Greg Nancarrow <gregn4422@gmail.com> wrote:
> I think if you're going to reject this patch, a brief comment should
> be added to that code to justify why that existing superfluous check
> is worthwhile.

It seems strange to add a comment to explain why it's there. If we're
going to the trouble of doing that, then we should just remove it and
add a very small comment to mention why INT8 sequences don't need to
be checked.

Patch attached

David

On Wed, 7 Jul 2021 at 20:37, David Rowley <dgrowleyml@gmail.com> wrote:
>
> On Wed, 7 Jul 2021 at 00:06, Greg Nancarrow <gregn4422@gmail.com> wrote:
> > I think if you're going to reject this patch, a brief comment should
> > be added to that code to justify why that existing superfluous check
> > is worthwhile.
>
> It seems strange to add a comment to explain why it's there. If we're
> going to the trouble of doing that, then we should just remove it and
> add a very small comment to mention why INT8 sequences don't need to
> be checked.

Any thoughts on this, Greg?

David

On Mon, Jul 12, 2021 at 2:26 PM David Rowley <dgrowleyml@gmail.com> wrote:
>
> > It seems strange to add a comment to explain why it's there. If we're
> > going to the trouble of doing that, then we should just remove it and
> > add a very small comment to mention why INT8 sequences don't need to
> > be checked.
>
> Any thoughts on this, Greg?
>

The patch LGTM (it's the same as my original patch but with short comments).

Regards,
Greg Nancarrow
Fujitsu Australia

On Mon, 12 Jul 2021 at 16:48, Greg Nancarrow <gregn4422@gmail.com> wrote:
>
> On Mon, Jul 12, 2021 at 2:26 PM David Rowley <dgrowleyml@gmail.com> wrote:
> >
> > > It seems strange to add a comment to explain why it's there. If we're
> > > going to the trouble of doing that, then we should just remove it and
> > > add a very small comment to mention why INT8 sequences don't need to
> > > be checked.
> >
> > Any thoughts on this, Greg?
> >
>
> The patch LGTM (it's the same as my original patch but with short comments).

Yeah, it's your patch with the comment reduced down to 2 lines.  This
was to try and address Peter's concern that the comment is too large.
This seemed to put him off the patch.  I also disagreed that it made
sense to remove 2 fairly harmless lines of code to replace them with
12 lines of comments.

What I was trying to get to here was something that was more
reasonable that might make sense to commit.  I'm just not certain
where Peter stands on this now that the latest patch is a net zero
when it comes to adding lines. Peter?

David

On 12.07.21 10:44, David Rowley wrote:
> What I was trying to get to here was something that was more
> reasonable that might make sense to commit.  I'm just not certain
> where Peter stands on this now that the latest patch is a net zero
> when it comes to adding lines. Peter?

Your version looks better to me than the original version, but I'm still 
-0.05 on changing this at all.

On Tue, 13 Jul 2021 at 06:50, Peter Eisentraut
<peter.eisentraut@enterprisedb.com> wrote:
> Your version looks better to me than the original version, but I'm still
> -0.05 on changing this at all.

I was more +0.4.  It does not seem worth the trouble of too much
discussion so, just to try and bring this to a close, instead of
adding a comment to explain why we needlessly check the range of the
INT8 sequence, I just pushed the patch that removes it and adds the 1
line comment to mention why it's not needed.

David