Content-Type: multipart/mixed; boundary="------------4l6AnDx0ONEyDv8uXO3qUCB3"
Message-ID: <5a8acd6d-79ba-1017-e1df-46dd948347f1@enterprisedb.com>
Date: Thu, 9 Feb 2023 17:18:47 +0100
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.7.1
Subject: Re: BUG #17774: Assert triggered on brin_minmax_multi.c
Content-Language: en-US
To: John Naylor <john.naylor@enterprisedb.com>, Tom Lane <tgl@sss.pgh.pa.us>
Cc: Dmitry Dolgov <9erthalion6@gmail.com>, tharakan@gmail.com,
 pgsql-bugs@lists.postgresql.org
References: <17774-c6f3e36dd4471e67@postgresql.org>
 <CAFBsxsEMigeTVhBOpsBHw+SJS1n776akNqVC8ATEDmGo_OPgrw@mail.gmail.com>
 <20230208100658.mz2kkeruoma6iqtw@ddolgov.remote.csb>
 <3485884.1675868581@sss.pgh.pa.us>
 <20230208153313.fym7at3ubgrec67a@ddolgov.remote.csb>
 <3492169.1675872316@sss.pgh.pa.us>
 <CAFBsxsG36dYXDFCNLsZ-muj1zKdBe-bB8ToOnXaCBp5uwQ8voA@mail.gmail.com>
From: Tomas Vondra <tomas.vondra@enterprisedb.com>
In-Reply-To: 
 <CAFBsxsG36dYXDFCNLsZ-muj1zKdBe-bB8ToOnXaCBp5uwQ8voA@mail.gmail.com>
Archived-At: 
 <https://www.postgresql.org/message-id/5a8acd6d-79ba-1017-e1df-46dd948347f1%40enterprisedb.com>
Precedence: bulk

This is a multi-part message in MIME format.
--------------4l6AnDx0ONEyDv8uXO3qUCB3
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit


On 2/9/23 04:53, John Naylor wrote:
> 
> On Wed, Feb 8, 2023 at 11:05 PM Tom Lane <tgl@sss.pgh.pa.us
> <mailto:tgl@sss.pgh.pa.us>> wrote:
>>
>> Dmitry Dolgov <9erthalion6@gmail.com <mailto:9erthalion6@gmail.com>>
> writes:
>> >> On Wed, Feb 08, 2023 at 10:03:01AM -0500, Tom Lane wrote:
>> >> I'd believe this argument more readily if the calculation weren't being
>> >> done in float arithmetic.  Since it is, you're at the mercy of roundoff
>> >> error ... and that small negative delta could certainly pass for
>> >> roundoff error.
>>
>> > Hmm...yeah, good point. In both the reproducer I've posted and the
>> > backtrace from the thread the delta is indeed rather small.
>>
>> I bet also it only fails when dealing with IPv6 addresses.
>> With 32-bit IPv4 addresses, a float8 would have enough mantissa
>> bits that the calculation wouldn't become imprecise.
> 
> Addresses from different families are treated as a distance of one, so I
> don't think inserting a single IPv6 address would get this far, and in
> fact I still get a crash in an empty table by taking out the IPv6
> address from Dmitry's example:
> 
> insert into brin_test values('127.0.0.1/0' <http://127.0.0.1/0'>);
> insert into brin_test values('0.0.0.0/12' <http://0.0.0.0/12'>);
> 
> Adding some debug calls shows it starts off negative from the start:
> 
> =# insert into brin_test values('127.0.0.1/0' <http://127.0.0.1/0'>);
> INSERT 0 1
> =#  insert into brin_test values('0.0.0.0/12' <http://0.0.0.0/12'>);
> NOTICE:  idx: 3 before div: -1.000000
> NOTICE:  idx: 3 after div: -0.003906
> NOTICE:  idx: 2 before div: -0.003906
> NOTICE:  idx: 2 after div: -0.000015
> NOTICE:  idx: 1 before div: -0.000015
> NOTICE:  idx: 1 after div: -0.000000
> NOTICE:  idx: 0 before div: -0.000000
> NOTICE:  idx: 0 after div: -0.000000
> 
> ...so something else is wrong here but I haven't dug deeper yet.
> 

I believe the bug is pretty trivial - the code applies the netmask
incorrectly, so that with 127.0.0.1/0 it ends with 0.0.0.1, and because
it assumes 0.0.0.1 < 0.0.0.0 it ends with negative delta.

In particular, the issue is that the code does this:

    lena = ip_bits(ipa);     -- 0
    len = ip_addrsize(ipa);  -- 4

    for (for (i = 0; i < len; i++)
    {
      nbits = lena - (i * 8);
      ...
      mask = (0xFF << (8 - nbits));
      ...
    }

But for 127.0.0.1/0 we get lena=0, so for i>0 nbits gets negative, and
the shift is probably going to do something silly (not sure what
exactly, but AFAICS it's undefined behavior).

Attached is a fixup that resolves this failure for me. I need to look a
bit closer if there are some other issues (e.g. with the float rounding
errors, etc.).

Good thing is this is this won't break anything without the assert - we
may pick incorrect ranges to merge, so the index is a bit less
efficient, but still valid/correct.


-- 
Tomas Vondra
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
--------------4l6AnDx0ONEyDv8uXO3qUCB3
Content-Type: text/x-patch; charset=UTF-8; name="brin-inet-fixup.patch"
Content-Disposition: attachment; filename="brin-inet-fixup.patch"
Content-Transfer-Encoding: base64

ZGlmZiAtLWdpdCBhL3NyYy9iYWNrZW5kL2FjY2Vzcy9icmluL2JyaW5fbWlubWF4X211bHRp
LmMgYi9zcmMvYmFja2VuZC9hY2Nlc3MvYnJpbi9icmluX21pbm1heF9tdWx0aS5jCmluZGV4
IGFjNjcwZmQwMmQ3Li5iZTAzZDYxYzRjYSAxMDA2NDQKLS0tIGEvc3JjL2JhY2tlbmQvYWNj
ZXNzL2JyaW4vYnJpbl9taW5tYXhfbXVsdGkuYworKysgYi9zcmMvYmFja2VuZC9hY2Nlc3Mv
YnJpbi9icmluX21pbm1heF9tdWx0aS5jCkBAIC0yMzY0LDE0ICsyMzY0LDE0IEBAIGJyaW5f
bWlubWF4X211bHRpX2Rpc3RhbmNlX2luZXQoUEdfRlVOQ1RJT05fQVJHUykKIAkJdW5zaWdu
ZWQgY2hhciBtYXNrOwogCQlpbnQJCQluYml0czsKIAotCQluYml0cyA9IGxlbmEgLSAoaSAq
IDgpOworCQluYml0cyA9IE1heCgwLCBsZW5hIC0gKGkgKiA4KSk7CiAJCWlmIChuYml0cyA8
IDgpCiAJCXsKIAkJCW1hc2sgPSAoMHhGRiA8PCAoOCAtIG5iaXRzKSk7CiAJCQlhZGRyYVtp
XSA9IChhZGRyYVtpXSAmIG1hc2spOwogCQl9CiAKLQkJbmJpdHMgPSBsZW5iIC0gKGkgKiA4
KTsKKwkJbmJpdHMgPSBNYXgoMCwgbGVuYiAtIChpICogOCkpOwogCQlpZiAobmJpdHMgPCA4
KQogCQl7CiAJCQltYXNrID0gKDB4RkYgPDwgKDggLSBuYml0cykpOwo=

--------------4l6AnDx0ONEyDv8uXO3qUCB3--