pg.ddx.io pgsql-hackers@postgresql.org mailing list archive help / color / mirror / Atom feed From: movead.li@highgo.ca <movead.li@highgo.ca>
To: ashutosh.bapat <ashutosh.bapat@2ndquadrant.com>
Cc: pgsql-hackers <pgsql-hackers@lists.postgresql.org>
Subject: Re: A bug when use get_bit() function for a long bytea string
Date: Wed, 18 Mar 2020 10:48:09 +0800
Message-ID: <2020031810480643107833@highgo.ca> (raw )
References: <20200312115135445367128@highgo.ca >
<CAExHW5vUSi4-oPvgMdFWvzuyaTmMtbY=JgdZ+u3T7ZdzQmubig@mail.gmail.com >
<20200313111842791351178@highgo.ca >
<CAG-ACPXeQEuvdVBzz+NWgFYoBNwcJfv1W=pJExp4LM_3fgar6w@mail.gmail.com >
Hello thanks for the detailed review,
>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.
Yes, it makes sence and followed.
> Some more comments on the patch
> struct pg_encoding
> {
>- unsigned (*encode_len) (const char *data, unsigned dlen);
>+ int64 (*encode_len) (const char *data, unsigned dlen);
> unsigned (*decode_len) (const char *data, unsigned dlen);
> unsigned (*encode) (const char *data, unsigned dlen, char *res);
> 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 = enc->encode(VARDATA_ANY(data), datalen, VARDATA(result));
> /* Make this FATAL 'cause we've trodden on memory ... */
>- if (res > resultlen)
>+ if ((int64)res > resultlen)
>
>if we change return type of all those functions to int64, we won't need this cast.
I change the 'encode' function, it needs an int64 return type, but keep other
functions in 'pg_encoding', because I think it of no necessary reason.
>Right now we are using int64 because bytea can be 1GB, but what if we increase
>that limit tomorrow, will int64 be sufficient? That may be unlikely in the near
>future, but nevertheless a possibility. Should we then define a new datatype
>which resolves to int64 right now but really depends upon the bytea length. I
>am not suggesting that we have to do it right now, but may be something to
>think about.
I decide to use int64 because if we really want to increase the limit, it should be
the same change with 'text', 'varchar' which have the same limit. So it may need
a more general struct. Hence I give up the idea.
> hex_enc_len(const char *src, unsigned srclen)
> {
>- return srclen << 1;
>+ return (int64)(srclen << 1);
>
>why not to convert srclen also to int64. That will also change the pg_encoding
>member definitions. But if encoded length requires int64 to fit the possibly
>values, same would be true for the source lengths. Why can't the source length
>also be int64?
>If still we want the cast, I think it should be ((int64)srclen << 1) rather
>than casting the result.
I prefer the '((int64)srclen << 1)' way.
> /* 3 bytes will be converted to 4, linefeed after 76 chars */
>- return (srclen + 2) * 4 / 3 + srclen / (76 * 3 / 4);
>+ return (int64)((srclen + 2) * 4 / 3 + srclen / (76 * 3 / 4));
>similar comments as above.
Followed.
>It might help to add a test where we could pass the second argument something
>greater than 1G. But it may be difficult to write such a test case.
Add two test cases.
Highgo Software (Canada/China/Pakistan)
URL : www.highgo.ca
EMAIL: mailto:movead(dot)li(at)highgo(dot)ca
Attachments:
[application/octet-stream] long_bytea_string_bug_fix_ver3.patch (9.4K, ../2020031810480643107833@highgo.ca/3-long_bytea_string_bug_fix_ver3.patch)
download | inline diff:
diff --git a/src/backend/utils/adt/encode.c b/src/backend/utils/adt/encode.c
index b8d9ec7e00..d0cbbb272b 100644
--- a/src/backend/utils/adt/encode.c
+++ b/src/backend/utils/adt/encode.c
@@ -20,9 +20,9 @@
struct pg_encoding
{
- unsigned (*encode_len) (const char *data, unsigned dlen);
+ int64 (*encode_len) (const char *data, unsigned dlen);
unsigned (*decode_len) (const char *data, unsigned dlen);
- unsigned (*encode) (const char *data, unsigned dlen, char *res);
+ int64 (*encode) (const char *data, unsigned dlen, char *res);
unsigned (*decode) (const char *data, unsigned dlen, char *res);
};
@@ -39,9 +39,8 @@ binary_encode(PG_FUNCTION_ARGS)
Datum name = PG_GETARG_DATUM(1);
text *result;
char *namebuf;
- int datalen,
- resultlen,
- res;
+ int datalen;
+ int64 res,resultlen;
const struct pg_encoding *enc;
datalen = VARSIZE_ANY_EXHDR(data);
@@ -76,8 +75,8 @@ binary_decode(PG_FUNCTION_ARGS)
bytea *result;
char *namebuf;
int datalen,
- resultlen,
res;
+ unsigned resultlen;
const struct pg_encoding *enc;
datalen = VARSIZE_ANY_EXHDR(data);
@@ -122,7 +121,7 @@ static const int8 hexlookup[128] = {
-1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1,
};
-unsigned
+int64
hex_encode(const char *src, unsigned len, char *dst)
{
const char *end = src + len;
@@ -133,7 +132,7 @@ hex_encode(const char *src, unsigned len, char *dst)
*dst++ = hextbl[*src & 0xF];
src++;
}
- return len * 2;
+ return (int64)len * 2;
}
static inline char
@@ -184,10 +183,10 @@ hex_decode(const char *src, unsigned len, char *dst)
return p - dst;
}
-static unsigned
+static int64
hex_enc_len(const char *src, unsigned srclen)
{
- return srclen << 1;
+ return (int64)srclen << 1;
}
static unsigned
@@ -214,7 +213,7 @@ static const int8 b64lookup[128] = {
41, 42, 43, 44, 45, 46, 47, 48, 49, 50, 51, -1, -1, -1, -1, -1,
};
-static unsigned
+static int64
pg_base64_encode(const char *src, unsigned len, char *dst)
{
char *p,
@@ -331,11 +330,11 @@ pg_base64_decode(const char *src, unsigned len, char *dst)
}
-static unsigned
+static int64
pg_base64_enc_len(const char *src, unsigned srclen)
{
/* 3 bytes will be converted to 4, linefeed after 76 chars */
- return (srclen + 2) * 4 / 3 + srclen / (76 * 3 / 4);
+ return ((int64)srclen + 2) * 4 / 3 + (int64)srclen / (76 * 3 / 4);
}
static unsigned
@@ -361,12 +360,12 @@ pg_base64_dec_len(const char *src, unsigned srclen)
#define VAL(CH) ((CH) - '0')
#define DIG(VAL) ((VAL) + '0')
-static unsigned
+static int64
esc_encode(const char *src, unsigned srclen, char *dst)
{
const char *end = src + srclen;
char *rp = dst;
- int len = 0;
+ int64 len = 0;
while (src < end)
{
@@ -448,11 +447,11 @@ esc_decode(const char *src, unsigned srclen, char *dst)
return len;
}
-static unsigned
+static int64
esc_enc_len(const char *src, unsigned srclen)
{
const char *end = src + srclen;
- int len = 0;
+ int64 len = 0;
while (src < end)
{
diff --git a/src/backend/utils/adt/varbit.c b/src/backend/utils/adt/varbit.c
index c3257553bd..e1bcd0cc66 100644
--- a/src/backend/utils/adt/varbit.c
+++ b/src/backend/utils/adt/varbit.c
@@ -1787,7 +1787,7 @@ Datum
bitsetbit(PG_FUNCTION_ARGS)
{
VarBit *arg1 = PG_GETARG_VARBIT_P(0);
- int32 n = PG_GETARG_INT32(1);
+ int64 n = PG_GETARG_INT64(1);
int32 newBit = PG_GETARG_INT32(2);
VarBit *result;
int len,
@@ -1798,10 +1798,10 @@ bitsetbit(PG_FUNCTION_ARGS)
bitNo;
bitlen = VARBITLEN(arg1);
- if (n < 0 || n >= bitlen)
+ if (n < 0 || n >= (int64)bitlen)
ereport(ERROR,
(errcode(ERRCODE_ARRAY_SUBSCRIPT_ERROR),
- errmsg("bit index %d out of valid range (0..%d)",
+ errmsg("bit index "INT64_FORMAT" out of valid range (0..%d)",
n, bitlen - 1)));
/*
@@ -1822,8 +1822,8 @@ bitsetbit(PG_FUNCTION_ARGS)
memcpy(r, p, VARBITBYTES(arg1));
- byteNo = n / BITS_PER_BYTE;
- bitNo = BITS_PER_BYTE - 1 - (n % BITS_PER_BYTE);
+ byteNo = (int32)(n / BITS_PER_BYTE);
+ bitNo = BITS_PER_BYTE - 1 - (int32)(n % BITS_PER_BYTE);
/*
* Update the byte.
@@ -1849,23 +1849,23 @@ Datum
bitgetbit(PG_FUNCTION_ARGS)
{
VarBit *arg1 = PG_GETARG_VARBIT_P(0);
- int32 n = PG_GETARG_INT32(1);
+ int64 n = PG_GETARG_INT64(1);
int bitlen;
bits8 *p;
int byteNo,
bitNo;
bitlen = VARBITLEN(arg1);
- if (n < 0 || n >= bitlen)
+ if (n < 0 || n >= (int64)bitlen)
ereport(ERROR,
(errcode(ERRCODE_ARRAY_SUBSCRIPT_ERROR),
- errmsg("bit index %d out of valid range (0..%d)",
+ errmsg("bit index "INT64_FORMAT" out of valid range (0..%d)",
n, bitlen - 1)));
p = VARBITS(arg1);
- byteNo = n / BITS_PER_BYTE;
- bitNo = BITS_PER_BYTE - 1 - (n % BITS_PER_BYTE);
+ byteNo = (int32)(n / BITS_PER_BYTE);
+ bitNo = BITS_PER_BYTE - 1 - (int32)(n % BITS_PER_BYTE);
if (p[byteNo] & (1 << bitNo))
PG_RETURN_INT32(1);
diff --git a/src/backend/utils/adt/varlena.c b/src/backend/utils/adt/varlena.c
index 907b5ab7b0..b8b06d5e32 100644
--- a/src/backend/utils/adt/varlena.c
+++ b/src/backend/utils/adt/varlena.c
@@ -388,7 +388,7 @@ byteaout(PG_FUNCTION_ARGS)
{
/* Print traditional escaped format */
char *vp;
- int len;
+ int64 len;
int i;
len = 1; /* empty string has 1 char */
@@ -3463,11 +3463,11 @@ byteaGetBit(PG_FUNCTION_ARGS)
len = VARSIZE_ANY_EXHDR(v);
- if (n < 0 || n >= len * 8)
+ if (n < 0 || (int64)n >= (int64)len * 8)
ereport(ERROR,
(errcode(ERRCODE_ARRAY_SUBSCRIPT_ERROR),
- errmsg("index %d out of valid range, 0..%d",
- n, len * 8 - 1)));
+ errmsg("index %d out of valid range, 0.."INT64_FORMAT,
+ n, (int64)len * 8 - 1)));
byteNo = n / 8;
bitNo = n % 8;
@@ -3534,11 +3534,11 @@ byteaSetBit(PG_FUNCTION_ARGS)
len = VARSIZE(res) - VARHDRSZ;
- if (n < 0 || n >= len * 8)
+ if (n < 0 || (int64)n >= (int64)len * 8)
ereport(ERROR,
(errcode(ERRCODE_ARRAY_SUBSCRIPT_ERROR),
- errmsg("index %d out of valid range, 0..%d",
- n, len * 8 - 1)));
+ errmsg("index %d out of valid range, 0.."INT64_FORMAT,
+ n, (int64)len * 8 - 1)));
byteNo = n / 8;
bitNo = n % 8;
diff --git a/src/include/catalog/pg_proc.dat b/src/include/catalog/pg_proc.dat
index 7fb574f9dc..e3c819e46d 100644
--- a/src/include/catalog/pg_proc.dat
+++ b/src/include/catalog/pg_proc.dat
@@ -3810,10 +3810,10 @@
proname => 'overlay', prorettype => 'bit', proargtypes => 'bit bit int4',
prosrc => 'bitoverlay_no_len' },
{ oid => '3032', descr => 'get bit',
- proname => 'get_bit', prorettype => 'int4', proargtypes => 'bit int4',
+ proname => 'get_bit', prorettype => 'int4', proargtypes => 'bit int8',
prosrc => 'bitgetbit' },
{ oid => '3033', descr => 'set bit',
- proname => 'set_bit', prorettype => 'bit', proargtypes => 'bit int4 int4',
+ proname => 'set_bit', prorettype => 'bit', proargtypes => 'bit int8 int4',
prosrc => 'bitsetbit' },
# for macaddr type support
diff --git a/src/include/utils/builtins.h b/src/include/utils/builtins.h
index e2016a8bc2..40cf539401 100644
--- a/src/include/utils/builtins.h
+++ b/src/include/utils/builtins.h
@@ -32,7 +32,7 @@ extern int errdatatype(Oid datatypeOid);
extern int errdomainconstraint(Oid datatypeOid, const char *conname);
/* encode.c */
-extern unsigned hex_encode(const char *src, unsigned len, char *dst);
+extern int64 hex_encode(const char *src, unsigned len, char *dst);
extern unsigned hex_decode(const char *src, unsigned len, char *dst);
/* int.c */
diff --git a/src/test/regress/expected/bit.out b/src/test/regress/expected/bit.out
index a1fab7ebcb..06d100dbdf 100644
--- a/src/test/regress/expected/bit.out
+++ b/src/test/regress/expected/bit.out
@@ -656,6 +656,38 @@ SELECT set_bit(B'0101011000100100', 15, 1);
SELECT set_bit(B'0101011000100100', 16, 1); -- fail
ERROR: bit index 16 out of valid range (0..15)
+SELECT get_bit(
+ set_bit((repeat('Postgres', 512 * 1024 * 1024 / 8))::bytea, 0, 0)
+ ,0);
+ get_bit
+---------
+ 0
+(1 row)
+
+SELECT get_bit(
+ set_bit((repeat('Postgres', 512 * 1024 * 1024 / 8))::bytea, 0, 1)
+ ,0);
+ get_bit
+---------
+ 1
+(1 row)
+
+select get_bit(
+ set_bit((repeat('Postgres', 512 * 1024 * 1024 / 8))::bytea, 1024 * 1024 * 1024 + 1, 0)
+ ,1024 * 1024 * 1024 + 1);
+ get_bit
+---------
+ 0
+(1 row)
+
+select get_bit(
+ set_bit((repeat('Postgres', 512 * 1024 * 1024 / 8))::bytea, 1024 * 1024 * 1024 + 1, 1)
+ ,1024 * 1024 * 1024 + 1);
+ get_bit
+---------
+ 1
+(1 row)
+
-- Overlay
SELECT overlay(B'0101011100' placing '001' from 2 for 3);
overlay
diff --git a/src/test/regress/sql/bit.sql b/src/test/regress/sql/bit.sql
index 7681d4ab4d..5ef4aa42da 100644
--- a/src/test/regress/sql/bit.sql
+++ b/src/test/regress/sql/bit.sql
@@ -200,6 +200,19 @@ DROP TABLE VARBIT_SHIFT_TABLE;
SELECT get_bit(B'0101011000100', 10);
SELECT set_bit(B'0101011000100100', 15, 1);
SELECT set_bit(B'0101011000100100', 16, 1); -- fail
+SELECT get_bit(
+ set_bit((repeat('Postgres', 512 * 1024 * 1024 / 8))::bytea, 0, 0)
+ ,0);
+SELECT get_bit(
+ set_bit((repeat('Postgres', 512 * 1024 * 1024 / 8))::bytea, 0, 1)
+ ,0);
+select get_bit(
+ set_bit((repeat('Postgres', 512 * 1024 * 1024 / 8))::bytea, 1024 * 1024 * 1024 + 1, 0)
+ ,1024 * 1024 * 1024 + 1);
+select get_bit(
+ set_bit((repeat('Postgres', 512 * 1024 * 1024 / 8))::bytea, 1024 * 1024 * 1024 + 1, 1)
+ ,1024 * 1024 * 1024 + 1);
+
-- Overlay
SELECT overlay(B'0101011100' placing '001' from 2 for 3);
view thread (27+ messages) latest in thread
Message-ID: <2020031810480643107833@highgo.ca>
Permalink: ../2020031810480643107833@highgo.ca/
Also on: postgresql.org/message-id/2020031810480643107833@highgo.ca
copy link · copy postgr.es · copy git am
reply Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-hackers@postgresql.org
Cc: movead.li@highgo.ca, ashutosh.bapat@2ndquadrant.com, pgsql-hackers@lists.postgresql.org
Subject: Re: A bug when use get_bit() function for a long bytea string
In-Reply-To: <2020031810480643107833@highgo.ca>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox