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, 1 Apr 2020 11:59:39 +0800
Message-ID: <2020040111593809632067@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 >
<2020031810480643107833@highgo.ca >
<CAG-ACPUbQwpCHhn_KkLAGEc08A91yZhARazj2BGjXvK+eFCVog@mail.gmail.com >
<2020032817093521193529@highgo.ca >
<CAG-ACPWtjb2y5RaoWP8f76OmvSvNP3xwT82spa9VDLkNJEOdDg@mail.gmail.com >
>+ int64 res,resultlen;
>It's better to have them on separate lines.
Sorry for that, done.
>-unsigned
>+int64
> hex_decode(const char *src, unsigned len, char *dst)
>Do we want to explicitly cast the return value to int64? Will build on some platform crib if not done so?
>I don't know of such a platform but my knowledge in this area is not great.
I think current change can make sure nothing wrong.
>+ byteNo = (int)(n / 8);
>+ bitNo = (int)(n % 8);
>some comment explaining why this downcasting is safe here?
Done
>- proname => 'get_bit', prorettype => 'int4', proargtypes => 'bytea int4',
>+ proname => 'get_bit', prorettype => 'int4', proargtypes => 'bytea int8',
> prosrc => 'byteaGetBit' },
> { oid => '724', descr => 'set bit',
>- proname => 'set_bit', prorettype => 'bytea', proargtypes => 'bytea int4 int4',
>+ proname => 'set_bit', prorettype => 'bytea', proargtypes => 'bytea int8 int4',
> prosrc => 'byteaSetBit' },
>Shouldn't we have similar changes for following entries as well?
>{ oid => '3032', descr => 'get bit',
> proname => 'get_bit', prorettype => 'int4', proargtypes => 'bit int4',
> prosrc => 'bitgetbit' },
>{ oid => '3033', descr => 'set bit',
> proname => 'set_bit', prorettype => 'bit', proargtypes => 'bit int4 int4',
> prosrc => 'bitsetbit' },
Because 'bitsetbit' and 'bitgetbit' do not have to calculate bit size by 'multiply 8',
so I think it seems need not to change it.
>The tests you have added are for bytea variant which ultimately calles byteaGet/SetBit().
>But I think we also need tests for bit variants which will ultimately call bitgetbit and bitsetbit functions.
As above, it need not to touch 'bitgetbit' and 'bitsetbit'.
>Once you address these comments, I think the patch is good for a committer.
>So please mark the commitfest entry as such when you post the next version of patch.
Thanks a lot for the detailed review again, and changed patch attached.
Regards,
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_ver5.patch (10.3K, ../2020040111593809632067@highgo.ca/3-long_bytea_string_bug_fix_ver5.patch)
download | inline diff:
diff --git a/src/backend/utils/adt/encode.c b/src/backend/utils/adt/encode.c
index b8d9ec7e00..585fe0c4bb 100644
--- a/src/backend/utils/adt/encode.c
+++ b/src/backend/utils/adt/encode.c
@@ -20,10 +20,10 @@
struct pg_encoding
{
- unsigned (*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);
+ int64 (*encode_len) (const char *data, unsigned dlen);
+ int64 (*decode_len) (const char *data, unsigned dlen);
+ int64 (*encode) (const char *data, unsigned dlen, char *res);
+ int64 (*decode) (const char *data, unsigned dlen, char *res);
};
static const struct pg_encoding *pg_find_encoding(const char *name);
@@ -39,9 +39,9 @@ binary_encode(PG_FUNCTION_ARGS)
Datum name = PG_GETARG_DATUM(1);
text *result;
char *namebuf;
- int datalen,
- resultlen,
- res;
+ int datalen;
+ int64 res;
+ int64 resultlen;
const struct pg_encoding *enc;
datalen = VARSIZE_ANY_EXHDR(data);
@@ -75,9 +75,8 @@ binary_decode(PG_FUNCTION_ARGS)
Datum name = PG_GETARG_DATUM(1);
bytea *result;
char *namebuf;
- int datalen,
- resultlen,
- res;
+ int datalen;
+ int64 resultlen, res;
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
@@ -152,7 +151,7 @@ get_hex(char c)
return (char) res;
}
-unsigned
+int64
hex_decode(const char *src, unsigned len, char *dst)
{
const char *s,
@@ -184,16 +183,16 @@ 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
+static int64
hex_dec_len(const char *src, unsigned srclen)
{
- return srclen >> 1;
+ return (int64)srclen >> 1;
}
/*
@@ -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,
@@ -261,7 +260,7 @@ pg_base64_encode(const char *src, unsigned len, char *dst)
return p - dst;
}
-static unsigned
+static int64
pg_base64_decode(const char *src, unsigned len, char *dst)
{
const char *srcend = src + len,
@@ -331,17 +330,17 @@ 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
+static int64
pg_base64_dec_len(const char *src, unsigned srclen)
{
- return (srclen * 3) >> 2;
+ return ((int64)srclen * 3) >> 2;
}
/*
@@ -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)
{
@@ -400,12 +399,12 @@ esc_encode(const char *src, unsigned srclen, char *dst)
return len;
}
-static unsigned
+static int64
esc_decode(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)
{
@@ -469,11 +468,11 @@ esc_enc_len(const char *src, unsigned srclen)
return len;
}
-static unsigned
+static int64
esc_dec_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/varlena.c b/src/backend/utils/adt/varlena.c
index 907b5ab7b0..47aa5be3a9 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 */
@@ -3455,7 +3455,7 @@ Datum
byteaGetBit(PG_FUNCTION_ARGS)
{
bytea *v = PG_GETARG_BYTEA_PP(0);
- int32 n = PG_GETARG_INT32(1);
+ int64 n = PG_GETARG_INT64(1);
int byteNo,
bitNo;
int len;
@@ -3463,14 +3463,17 @@ byteaGetBit(PG_FUNCTION_ARGS)
len = VARSIZE_ANY_EXHDR(v);
- if (n < 0 || n >= len * 8)
+ if (n < 0 || n >= (int64)len * 8)
ereport(ERROR,
(errcode(ERRCODE_ARRAY_SUBSCRIPT_ERROR),
- errmsg("index %d out of valid range, 0..%d",
- n, len * 8 - 1)));
-
- byteNo = n / 8;
- bitNo = n % 8;
+ errmsg("index "INT64_FORMAT" out of valid range, 0.."INT64_FORMAT,
+ n, (int64)len * 8 - 1)));
+ /*
+ * The value of 'n' is smaller than 'len * 8', and 'len' is by int type,
+ * so it is safe to downcasting 'n' from int64 to int.
+ */
+ byteNo = (int)(n / 8);
+ bitNo = (int)(n % 8);
byte = ((unsigned char *) VARDATA_ANY(v))[byteNo];
@@ -3524,7 +3527,7 @@ Datum
byteaSetBit(PG_FUNCTION_ARGS)
{
bytea *res = PG_GETARG_BYTEA_P_COPY(0);
- int32 n = PG_GETARG_INT32(1);
+ int64 n = PG_GETARG_INT64(1);
int32 newBit = PG_GETARG_INT32(2);
int len;
int oldByte,
@@ -3534,14 +3537,17 @@ byteaSetBit(PG_FUNCTION_ARGS)
len = VARSIZE(res) - VARHDRSZ;
- if (n < 0 || n >= len * 8)
+ if (n < 0 || n >= (int64)len * 8)
ereport(ERROR,
(errcode(ERRCODE_ARRAY_SUBSCRIPT_ERROR),
- errmsg("index %d out of valid range, 0..%d",
- n, len * 8 - 1)));
-
- byteNo = n / 8;
- bitNo = n % 8;
+ errmsg("index "INT64_FORMAT" out of valid range, 0.."INT64_FORMAT,
+ n, (int64)len * 8 - 1)));
+ /*
+ * The value of 'n' is smaller than 'len * 8', and 'len' is by int type,
+ * so it is safe to downcasting 'n' from int64 to int.
+ */
+ byteNo = (int64)(n / 8);
+ bitNo = (int64)(n % 8);
/*
* sanity check!
diff --git a/src/include/catalog/pg_proc.dat b/src/include/catalog/pg_proc.dat
index a6a708cca9..218af5ab2f 100644
--- a/src/include/catalog/pg_proc.dat
+++ b/src/include/catalog/pg_proc.dat
@@ -1403,10 +1403,10 @@
proname => 'set_byte', prorettype => 'bytea',
proargtypes => 'bytea int4 int4', prosrc => 'byteaSetByte' },
{ oid => '723', descr => 'get bit',
- proname => 'get_bit', prorettype => 'int4', proargtypes => 'bytea int4',
+ proname => 'get_bit', prorettype => 'int4', proargtypes => 'bytea int8',
prosrc => 'byteaGetBit' },
{ oid => '724', descr => 'set bit',
- proname => 'set_bit', prorettype => 'bytea', proargtypes => 'bytea int4 int4',
+ proname => 'set_bit', prorettype => 'bytea', proargtypes => 'bytea int8 int4',
prosrc => 'byteaSetBit' },
{ oid => '749', descr => 'substitute portion of string',
proname => 'overlay', prorettype => 'bytea',
diff --git a/src/include/utils/builtins.h b/src/include/utils/builtins.h
index e2016a8bc2..55925ead8b 100644
--- a/src/include/utils/builtins.h
+++ b/src/include/utils/builtins.h
@@ -32,8 +32,8 @@ 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 unsigned hex_decode(const char *src, unsigned len, char *dst);
+extern int64 hex_encode(const char *src, unsigned len, char *dst);
+extern int64 hex_decode(const char *src, unsigned len, char *dst);
/* int.c */
extern int2vector *buildint2vector(const int16 *int2s, int n);
diff --git a/src/test/regress/expected/bit.out b/src/test/regress/expected/bit.out
index a1fab7ebcb..8e56289968 100644
--- a/src/test/regress/expected/bit.out
+++ b/src/test/regress/expected/bit.out
@@ -656,6 +656,40 @@ 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,
+ 512::bigint * 1024 * 1024 * 8 - 1, 0)
+ ,512::bigint * 1024 * 1024 * 8 - 1);
+ get_bit
+---------
+ 0
+(1 row)
+
+select get_bit(
+ set_bit((repeat('Postgres', 512 * 1024 * 1024 / 8))::bytea,
+ 512::bigint * 1024 * 1024 * 8 - 1, 1)
+ ,512::bigint * 1024 * 1024 * 8 - 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..5914095941 100644
--- a/src/test/regress/sql/bit.sql
+++ b/src/test/regress/sql/bit.sql
@@ -200,6 +200,21 @@ 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,
+ 512::bigint * 1024 * 1024 * 8 - 1, 0)
+ ,512::bigint * 1024 * 1024 * 8 - 1);
+select get_bit(
+ set_bit((repeat('Postgres', 512 * 1024 * 1024 / 8))::bytea,
+ 512::bigint * 1024 * 1024 * 8 - 1, 1)
+ ,512::bigint * 1024 * 1024 * 8 - 1);
+
-- Overlay
SELECT overlay(B'0101011100' placing '001' from 2 for 3);
view thread (27+ messages) latest in thread
Message-ID: <2020040111593809632067@highgo.ca>
Permalink: ../2020040111593809632067@highgo.ca/
Also on: postgresql.org/message-id/2020040111593809632067@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: <2020040111593809632067@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