Content-Type: multipart/mixed; boundary="------------NxaS8Gx25ldpOfVEt4foodgR"
Message-ID: <81a75261-2dec-53f6-40f9-5d3ed13f42ee@gmail.com>
Date: Mon, 4 Sep 2023 17:00:00 +0300
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.4.2
Subject: Re: BUG #17950: Incorrect memory access in gtsvector_picksplit()
Content-Language: en-US
To: Michael Paquier <michael@paquier.xyz>
Cc: Tom Lane <tgl@sss.pgh.pa.us>, pgsql-bugs@lists.postgresql.org
References: <17950-6c80a8d2b94ec695@postgresql.org>
 <da453149-fa9e-67fc-fbce-80cca399bce5@gmail.com>
 <e2f36991-0519-bfcf-1da9-c31ca645b4ec@gmail.com>
 <665394.1691937343@sss.pgh.pa.us>
 <09fec706-0691-a131-5c6c-3e64ecc134fc@gmail.com>
 <ZPV6vblm1jmgs1VW@paquier.xyz>
From: Alexander Lakhin <exclusion@gmail.com>
In-Reply-To: <ZPV6vblm1jmgs1VW@paquier.xyz>
Archived-At: 
 <https://www.postgresql.org/message-id/81a75261-2dec-53f6-40f9-5d3ed13f42ee%40gmail.com>
Precedence: bulk

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

Hi Mchael,

Thank you for committing the fix!

04.09.2023 09:35, Michael Paquier wrote:
> Regarding the changes in gtsvectorout(), the output produced is indeed
> confusing when ISALLTRUE is set.
>
> -        int            siglen = GETSIGLEN(key);
> -        int            cnttrue = (ISALLTRUE(key)) ? SIGLENBIT(siglen) : sizebitvec(GETSIGN(key), siglen);
> +        if (ISALLTRUE(key))
> +            sprintf(outbuf, "all true bits");
> +        else
> +        {
> +            int            siglen = GETSIGLEN(key);
> +            int            cnttrue = (ISALLTRUE(key)) ? SIGLENBIT(siglen) : sizebitvec(GETSIGN(key), siglen);
>   
> -        sprintf(outbuf, SINGOUTSTR, cnttrue, (int) SIGLENBIT(siglen) - cnttrue);
> +            sprintf(outbuf, SINGOUTSTR, cnttrue, (int) SIGLENBIT(siglen) - cnttrue);
> +        }
>
> In the false branch of ISALLTRUE(key), why isn't cnttrue always
> calculated with sizebitvec()?

Yes, that expression should be simpler there. Please look at the v2 attached.

> It's also not something I would
> backpatch.  That's confusing, for sure, but there is also the argument
> of keeping a consistent output in the stable branches.

I agree. Let's not backpatch it.

Best regards,
Alexander
--------------NxaS8Gx25ldpOfVEt4foodgR
Content-Type: text/x-patch; charset=UTF-8; name="v2-02-fix-gtsvectorout.patch"
Content-Disposition: attachment; filename="v2-02-fix-gtsvectorout.patch"
Content-Transfer-Encoding: base64

ZGlmZiAtLWdpdCBhL3NyYy9iYWNrZW5kL3V0aWxzL2FkdC90c2dpc3RpZHguYyBiL3NyYy9i
YWNrZW5kL3V0aWxzL2FkdC90c2dpc3RpZHguYwppbmRleCAxMmE0MTExZTk5Li4yNzNmOWIw
OTI1IDEwMDY0NAotLS0gYS9zcmMvYmFja2VuZC91dGlscy9hZHQvdHNnaXN0aWR4LmMKKysr
IGIvc3JjL2JhY2tlbmQvdXRpbHMvYWR0L3RzZ2lzdGlkeC5jCkBAIC0xMTUsMTAgKzExNSwx
NSBAQCBndHN2ZWN0b3JvdXQoUEdfRlVOQ1RJT05fQVJHUykKIAkJc3ByaW50ZihvdXRidWYs
IEFSUk9VVFNUUiwgKGludCkgQVJSTkVMRU0oa2V5KSk7CiAJZWxzZQogCXsKLQkJaW50CQkJ
c2lnbGVuID0gR0VUU0lHTEVOKGtleSk7Ci0JCWludAkJCWNudHRydWUgPSAoSVNBTExUUlVF
KGtleSkpID8gU0lHTEVOQklUKHNpZ2xlbikgOiBzaXplYml0dmVjKEdFVFNJR04oa2V5KSwg
c2lnbGVuKTsKKwkJaWYgKElTQUxMVFJVRShrZXkpKQorCQkJc3ByaW50ZihvdXRidWYsICJh
bGwgdHJ1ZSBiaXRzIik7CisJCWVsc2UKKwkJeworCQkJaW50CQkJc2lnbGVuID0gR0VUU0lH
TEVOKGtleSk7CisJCQlpbnQJCQljbnR0cnVlID0gc2l6ZWJpdHZlYyhHRVRTSUdOKGtleSks
IHNpZ2xlbik7CiAKLQkJc3ByaW50ZihvdXRidWYsIFNJTkdPVVRTVFIsIGNudHRydWUsIChp
bnQpIFNJR0xFTkJJVChzaWdsZW4pIC0gY250dHJ1ZSk7CisJCQlzcHJpbnRmKG91dGJ1Ziwg
U0lOR09VVFNUUiwgY250dHJ1ZSwgKGludCkgU0lHTEVOQklUKHNpZ2xlbikgLSBjbnR0cnVl
KTsKKwkJfQogCX0KIAogCVBHX0ZSRUVfSUZfQ09QWShrZXksIDApOwo=

--------------NxaS8Gx25ldpOfVEt4foodgR--