Received: from malur.postgresql.org ([217.196.149.56]) by arkaria.postgresql.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_CBC_SHA1:256) (Exim 4.92) (envelope-from ) id 1jI7Tc-0007po-F5 for pgsql-hackers@arkaria.postgresql.org; Sat, 28 Mar 2020 09:10:00 +0000 Received: from localhost ([127.0.0.1] helo=malur.postgresql.org) by malur.postgresql.org with esmtp (Exim 4.89) (envelope-from ) id 1jI7Tb-0001Cz-02 for pgsql-hackers@arkaria.postgresql.org; Sat, 28 Mar 2020 09:09:58 +0000 Received: from magus.postgresql.org ([2a02:c0:301:0:ffff::29]) by malur.postgresql.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_CBC_SHA1:256) (Exim 4.89) (envelope-from ) id 1jI7Ta-0001Cr-Lr for pgsql-hackers@lists.postgresql.org; Sat, 28 Mar 2020 09:09:58 +0000 Received: from sender4-op-o13.zoho.com ([136.143.188.13]) by magus.postgresql.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_CBC_SHA1:256) (Exim 4.92) (envelope-from ) id 1jI7TS-0005dN-7K for pgsql-hackers@lists.postgresql.org; Sat, 28 Mar 2020 09:09:58 +0000 ARC-Seal: i=1; a=rsa-sha256; t=1585386585; cv=none; d=zohomail.com; s=zohoarc; b=a1GFS4Vq9NySGCKYfcFIjVCeROecyAVRG6yGXk6pMA3tUSoCt23UpuFFN9IDtws59YcxGiSugcwKKLlc4J1Ms5bsRIVsOmyJeV+XHlMX4aR1/uRVEiARWlzn0YnyOZI9gnTz/hmTwIToC/5IPBAL3UCxC6XkTq2/n5D1CiXsmMg= ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=zohomail.com; s=zohoarc; t=1585386585; h=Content-Type:Cc:Date:From:MIME-Version:Message-ID:References:Subject:To; bh=XapHsRNZ9shS0jd4yFKB0QIEPz/clEJGVjDZlvOWzp8=; b=j5xC8o4J9FPOMtbhYGZ9j3P3lcCZ6gVvgTQ6lbVGByPkPQFUqCyFgvAnsgsRPx9VJF5T9G7YKpS/AIVxbpfMnVCjgyt9nG4tkjD5fsgrVyArnXmQ58eInOudVIRME7H7H9kby+bDtZtFyK6XFiD4QOkS6Y2BrLnZY+BuxxLTZOs= ARC-Authentication-Results: i=1; mx.zohomail.com; dkim=pass header.i=highgo.ca; spf=pass smtp.mailfrom=movead.li@highgo.ca; dmarc=pass header.from= header.from= DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; t=1585386585; s=zoho; d=highgo.ca; i=movead.li@highgo.ca; h=Date:From:To:Cc:Subject:References:Mime-Version:Message-ID:Content-Type; bh=XapHsRNZ9shS0jd4yFKB0QIEPz/clEJGVjDZlvOWzp8=; b=cfboWZ4mw0C9o3MjHxlDOcPUmNzakOR/gs2trMRFCi7SwDRwiyQAJ+wtuLMtfsEE L7CUndfqDju1ceEadJnRf0utmCSs1AlyQ+JliXwhV7LEIqwYNaOHTxjOi1szls8dwJW GaNt/lTOW2ZubeVwGol02rVz51hYb7NNyvD0rp44= Received: from USER-20191109OA (112.229.109.42 [112.229.109.42]) by mx.zohomail.com with SMTPS id 1585386583185153.84027735123686; Sat, 28 Mar 2020 02:09:43 -0700 (PDT) Date: Sat, 28 Mar 2020 17:09:37 +0800 From: "movead.li@highgo.ca" To: ashutosh.bapat Cc: pgsql-hackers Subject: Re: A bug when use get_bit() function for a long bytea string References: <20200312115135445367128@highgo.ca>, , <20200313111842791351178@highgo.ca>, , <2020031810480643107833@highgo.ca>, X-Priority: 3 X-GUID: C6E9A01F-6928-4A34-B077-B34A18D472D9 X-Has-Attach: no X-Mailer: Foxmail 7.2.15.80[cn] Mime-Version: 1.0 Message-ID: <2020032817093521193529@highgo.ca> Content-Type: multipart/alternative; boundary="----=_001_NextPart227217524607_=----" X-ZohoMailClient: External List-Id: List-Help: List-Subscribe: List-Post: List-Owner: List-Archive: Precedence: bulk This is a multi-part message in MIME format. ------=_001_NextPart227217524607_=---- Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: base64 SSB3YW50IHRvIHJlc2VudCB0aGUgbWFpbCwgYmVjYXVzZSBsYXN0IG9uZSBpcyBpbiB3cm9uZyBm b3JtYXQgYW5kIGhhcmRseSB0byByZWFkLg0KDQpJbiBhZGRpdGlvbiwgSSB0aGluayAnU2l6ZScg b3IgJ3NpemVfdCcgaXMgcmVseSBvbiBwbGF0Zm9ybSwgdGhleSBtYXkgY2FuJ3Qgd29yayBvbiAz MmJpdA0Kc3lzdGVtLiBTbyBJIGNob29zZSAnaW50NjQnIGFmdGVyIGFzaHV0b3NoJ3MgcmV2aWV3 Lg0KDQo+PkkgdGhpbmsgdGhlIHNlY29uZCBhcmd1bWVudCBpbmRpY2F0ZXMgdGhlIGJpdCBwb3Np dGlvbiwgd2hpY2ggd291bGQgYmUgbWF4IGJ5dGVhIGxlbmd0aCAqIDguIElmIG1heCBieXRlYSBs ZW5ndGggY292ZXJzIHdob2xlIGludDMyLCB0aGUgc2Vjb25kIGFyZ3VtZW50ID5uZWVkcyB0byBi ZSB3aWRlciBpLmUuIGludDY0Lg0KPlllcywgaXQgbWFrZXMgc2VuY2UgYW5kIGZvbGxvd2VkLg0K DQo+PkkgdGhpbmsgd2UgbmVlZCBhIHNpbWlsYXIgY2hhbmdlIGluIGJ5dGVhR2V0Qml0KCkgYW5k IGJ5dGVhU2V0Qml0KCkgYXMgd2VsbC4NClNvcnJ5LCBJIHRoaW5rIGl0J3MgbXkgbWlzdGFrZSwg aXQgaXMgdGhlIHR3byBmdW5jdGlvbnMgYWJvdmUgc2hvdWxkIGJlIGNoYW5nZWQuDQoNCg0KPj5T b21lIG1vcmUgY29tbWVudHMgb24gdGhlIHBhdGNoDQo+PiBzdHJ1Y3QgcGdfZW5jb2RpbmcNCj4+ IHsNCj4+LSB1bnNpZ25lZCAoKmVuY29kZV9sZW4pIChjb25zdCBjaGFyICpkYXRhLCB1bnNpZ25l ZCBkbGVuKTsNCj4+KyBpbnQ2NCAoKmVuY29kZV9sZW4pIChjb25zdCBjaGFyICpkYXRhLCB1bnNp Z25lZCBkbGVuKTsNCj4+ICB1bnNpZ25lZCAoKmRlY29kZV9sZW4pIChjb25zdCBjaGFyICpkYXRh LCB1bnNpZ25lZCBkbGVuKTsNCj4+ICB1bnNpZ25lZCAoKmVuY29kZSkgKGNvbnN0IGNoYXIgKmRh dGEsIHVuc2lnbmVkIGRsZW4sIGNoYXIgKnJlcyk7DQo+PiAgdW5zaWduZWQgKCpkZWNvZGUpIChj b25zdCBjaGFyICpkYXRhLCB1bnNpZ25lZCBkbGVuLCBjaGFyICpyZXMpOw0KPj4gV2h5IG5vdCB1 c2UgcmV0dXJuIHR5cGUgb2YgaW50NjQgZm9yIHJlc3Qgb2YgdGhlIGZ1bmN0aW9ucyBoZXJlIGFz IHdlbGw/DQo+PiAgcmVzID0gZW5jLT5lbmNvZGUoVkFSREFUQV9BTlkoZGF0YSksIGRhdGFsZW4s IFZBUkRBVEEocmVzdWx0KSk7DQo+PiAgLyogTWFrZSB0aGlzIEZBVEFMICdjYXVzZSB3ZSd2ZSB0 cm9kZGVuIG9uIG1lbW9yeSAuLi4gKi8NCj4+LSBpZiAocmVzID4gcmVzdWx0bGVuKQ0KPj4rIGlm ICgoaW50NjQpcmVzID4gcmVzdWx0bGVuKQ0KPj4NCj4+aWYgd2UgY2hhbmdlIHJldHVybiB0eXBl IG9mIGFsbCB0aG9zZSBmdW5jdGlvbnMgdG8gaW50NjQsIHdlIHdvbid0IG5lZWQgdGhpcyBjYXN0 Lg0KPkkgY2hhbmdlIHRoZSAnZW5jb2RlJyBmdW5jdGlvbiwgaXQgbmVlZHMgYW4gaW50NjQgcmV0 dXJuIHR5cGUsIGJ1dCBrZWVwIG90aGVyIA0KPmZ1bmN0aW9ucyBpbiAncGdfZW5jb2RpbmcnLCBi ZWNhdXNlIEkgdGhpbmsgaXQgb2Ygbm8gbmVjZXNzYXJ5IHJlYXNvbi4NCg0KPj5PaywgbGV0J3Mg bGVhdmUgaXQgZm9yIGEgY29tbWl0dGVyIHRvIGRlY2lkZS4gDQpXZWxsLCBJIGNoYW5nZSBhbGwg b2YgdGhlbSB0aGlzIHRpbWUsIGJlY2F1c2UgVG9tIExhbmUgc3VwcG9ydHMgb24gbmV4dCBtYWls Lg0KDQogDQo+U29tZSBtb3JlIHJldmlldyBjb21tZW50cy4NCj4rICAgaW50NjQgICAgICAgcmVz LHJlc3VsdGxlbjsNCj5XZSBuZWVkIHRob3NlIG9uIHNlcGFyYXRlIGxpbmVzLCBwb3NzaWJseS4N CkRvbmUNCg0KPisgICBieXRlTm8gPSAoaW50MzIpKG4gLyBCSVRTX1BFUl9CWVRFKTsNCj5Eb2Vz IGl0IGh1cnQgdG8gaGF2ZSBieXRlTm8gYXMgaW50NjQgc28gYXMgdG8gYXZvaWQgYSBjYXN0LiBP dGhlcndpc2UsIHBsZWFzZQ0KPmFkZCBhIGNvbW1lbnQgZXhwbGFpbmluZyB0aGUgcmVhc29uIGZv ciB0aGUgY2FzdC4gVGhlIGNvbW1lbnQgYXBwbGllcyBhdCBvdGhlcg0KPnBsYWNlcyB3aGVyZSB0 aGlzIGNoYW5nZSBhcHBlYXJzLg0KPi0gICAgICAgaW50ICAgICAgICAgbGVuOw0KPisgICAgICAg aW50NjQgICAgICAgbGVuOw0KPldoeSBkbyB3ZSBuZWVkIHRoaXMgY2hhbmdlPw0KPiAgICAgICAg aW50ICAgICAgICAgaTsNCkl0IGlzIG15IG1pc3Rha2UgYXMgZGVzY3JpYmUgYWJvdmUsIGl0IHNo b3VsZCBub3QgYmUgJ2JpdGdldGJpdCgpL2JpdHNldGJpdCgpJyAgdG8gYmUgY2hhbmdlZC4NCg0K DQoNCj4+SXQgbWlnaHQgaGVscCB0byBhZGQgYSB0ZXN0IHdoZXJlIHdlIGNvdWxkIHBhc3MgdGhl IHNlY29uZCBhcmd1bWVudCBzb21ldGhpbmcNCj4+Z3JlYXRlciB0aGFuIDFHLiBCdXQgaXQgbWF5 IGJlIGRpZmZpY3VsdCB0byB3cml0ZSBzdWNoIGEgdGVzdCBjYXNlLg0KPkFkZCB0d28gdGVzdCBj YXNlcy4NCiANCj4rDQo+K3NlbGVjdCBnZXRfYml0KA0KPisgICAgICAgIHNldF9iaXQoKHJlcGVh dCgnUG9zdGdyZXMnLCA1MTIgKiAxMDI0ICogMTAyNCAvIDgpKTo6Ynl0ZWEsIDEwMjQgKiAxMDI0 ICogMTAyNCArIDEsIDApDQo+KyAgICAgICAsMTAyNCAqIDEwMjQgKiAxMDI0ICsgMSk7DQoNCj5U aGlzIGJpdCBwb3NpdGlvbiBpcyBzdGlsbCB3aXRoaW4gaW50NC4NCj5wb3N0Z3Jlcz0jIHNlbGVj dCBwZ19jb2x1bW5fc2l6ZSgxMDI0ICogMTAyNCAqIDEwMjQgKyAxKTsgDQo+IHBnX2NvbHVtbl9z aXplDQo+LS0tLS0tLS0tLS0tLS0tLQ0KPiAgICAgICAgICAgICAgNCAgIA0KPigxIHJvdykNCg0K PllvdSB3YW50IHNvbWV0aGluZyBsaWtlDQo+cG9zdGdyZXM9IyBzZWxlY3QgcGdfY29sdW1uX3Np emUoNTEyOjpiaWdpbnQgKiAxMDI0ICogMTAyNCAqIDgpOyANCj4gcGdfY29sdW1uX3NpemUNCj4t LS0tLS0tLS0tLS0tLS0tDQo+ICAgICAgICAgICAgICA4ICAgDQo+KDEgcm93KQ0KSSBpbnRlbmQg dG8gdGVzdCBzaXplIGxhcmdlIHRoZW4gMUcsIGFuZCBub3cgSSB0aGluayB5b3UgZ2l2ZSBhIGJl dHRlciBpZGVhIGFuZCBmb2xsb3dlZC4NCg0KDQoNCg0KDQpIaWdoZ28gU29mdHdhcmUgKENhbmFk YS9DaGluYS9QYWtpc3RhbikgDQpVUkwgOiB3d3cuaGlnaGdvLmNhIA0KRU1BSUw6IG1haWx0bzpt b3ZlYWQoZG90KWxpKGF0KWhpZ2hnbyhkb3QpY2ENCg0K ------=_001_NextPart227217524607_=---- Content-Type: text/html; charset="UTF-8" Content-Transfer-Encoding: quoted-printable =0A
=
I want to resent the mail, because last= one is in wrong format and hardly to read.

In ad= dition, I think 'Size' or 'size_t' is rely on platform, they may can't wor= k on 32bit
system. So I choose 'int64' after ashutosh's review.

<= /div>
>>I think the second argument indicates the bit = position, which would be max bytea length * 8. If max bytea length covers = whole int32, the second argument >needs to be wider i.e. int64.<= /div>
>Yes, it makes sence and followed.

>>I think we need a simil= ar change in byteaGetBit() and byteaSetBit() as well.
Sorry= , I think it's my mistake, it is the two functions above should be changed= .


>>Some more comments on the patch
><= span style=3D"font-size: 10.5pt; line-height: 1.5; background-color: trans= parent;">> struct pg_encoding
&g= t;> {
>&= gt;- unsigned (*encode_len) (const char *data, unsigned d= len);
>>+ int64 (*encode= _len) (const char *data, unsigned dlen);
>&= gt;  unsigned (*decode_len) (c= onst char *data, unsigned dlen);
>>  unsigned (*encode) (const char *da= ta, unsigned dlen, char *res);
>><= span style=3D"font-size: 10.5pt; line-height: 1.5; background-color: trans= parent;">  unsigned (*decode) (const char *data, unsigned dlen, = char *res);
>> Why = not use return type of int64 for rest of the functions here as well?
>>  res =3D enc->= ;encode(VARDATA_ANY(data), datalen, VARDATA(result));
>= ;>  /* Make this FATAL 'cause we'v= e trodden on memory ... */
>>- if (res > resultlen)
>><= span style=3D"font-size: 10.5pt; line-height: 1.5; background-color: trans= parent;">+ if ((int64)res > resultlen)
>>
>= ;>if we change return type of all those func= tions to int64, we won't need this cast.
&= gt;I change the 'encode' function, it needs an int64 return type, b= ut keep other 
>functions in 'pg_e= ncoding', because I think it of no necessary reason.=
=
>>Ok, let's leave it for a committer= to decide. 
Well, I change all of them this time, because = Tom Lane supports on next mail.

 
=
>Some more review comments.
>+   int64   &n= bsp;   res,resultlen;
>We need those on separate lines, possibl= y.
Done

>+   byteNo =3D (int32)(n / BITS_P= ER_BYTE);
>Does it hurt to have byteNo as int64 so as to avoid a cas= t. Otherwise, please
>add a comment explaining the reason for the ca= st. The comment applies at other
>places where this change appears.<= br>>-       int         len;
>= +       int64       len;
>Why do we ne= ed this change?
>        int      = ;   i;
It is my mistake as describe above, it should = not be 'bitgetbit()/bitsetbit()'  to be ch= anged.



>>It might hel= p to add a test where we could pass the second argument something
>&= gt;greater than 1G. But it may be difficult to write such a test case.
=
>Add two test cases.
 
>+
>+select get_b= it(
>+        set_bit((repeat('Postgres', 512 * = 1024 * 1024 / 8))::bytea, 1024 * 1024 * 1024 + 1, 0)
>+    = ;   ,1024 * 1024 * 1024 + 1);

>This bit position is still w= ithin int4.
>postgres=3D# select pg_column_size(1024 * 1024 * 1024 += 1); 
> pg_column_size
>----------------
>&nbs= p;             4   
>(1 row)=

>You want something like
>postgres=3D# select pg_column_s= ize(512::bigint * 1024 * 1024 * 8); 
> pg_column_size
&= gt;----------------
>             = ; 8   
>(1 row)
I intend to test size large then= 1G, and now I think you give a better idea and followed.



Highgo Software (Canada/China/P= akistan)
URL : www.highgo.ca
EMAIL: mailto:movead(dot)li(at)high= go(dot)ca

=0A ------=_001_NextPart227217524607_=------