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: Sat, 28 Mar 2020 16:59:01 +0800
Message-ID: <2020032816585865712320@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 >
>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.
>I think we need a similar 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
> 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.
>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 res,resultlen;
Done
>We need those on separate lines, possibly.
>+ byteNo = (int32)(n / BITS_PER_BYTE);
>Does it hurt to have byteNo as int64 so as to avoid a cast. Otherwise, please
>add a comment explaining the reason for the cast. The comment applies at other
>places where this change appears.
>- int len;
>+ int64 len;
>Why do we need this change?
> int i;
It is my mistake as describe above, it should not be 'bitgetbit()/bitsetbit()' to be changed.
>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.
>+
>+select get_bit(
>+ set_bit((repeat('Postgres', 512 * 1024 * 1024 / 8))::bytea, 1024 * 1024 * 1024 + 1, 0)
>+ ,1024 * 1024 * 1024 + 1);
>This bit position is still within int4.
>postgres=# select pg_column_size(1024 * 1024 * 1024 + 1);
> pg_column_size
>----------------
> 4
>(1 row)
>You want something like
>postgres=# select pg_column_size(512::bigint * 1024 * 1024 * 8);
> pg_column_size
>----------------
> 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/Pakistan)
URL : www.highgo.ca
EMAIL: mailto:movead(dot)li(at)highgo(dot)ca
Attachments:
[application/octet-stream] long_bytea_string_bug_fix_ver4.patch (10.0K, ../2020032816585865712320@highgo.ca/3-long_bytea_string_bug_fix_ver4.patch)
download | inline diff:
diff --git a/src/backend/utils/adt/encode.c b/src/backend/utils/adt/encode.c
index 7293d66de5..ca90992d39 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,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);
@@ -75,9 +74,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 +120,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 +131,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 +150,7 @@ get_hex(char c)
return (char) res;
}
-unsigned
+int64
hex_decode(const char *src, unsigned len, char *dst)
{
const char *s,
@@ -184,16 +182,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 +212,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 +259,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 +329,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 +359,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 +398,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 +446,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 +467,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 1587b4ed02..ab6ee28a44 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 */
@@ -3435,7 +3435,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;
@@ -3443,14 +3443,14 @@ 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)));
+ errmsg("index "INT64_FORMAT" out of valid range, 0.."INT64_FORMAT,
+ n, (int64)len * 8 - 1)));
- byteNo = n / 8;
- bitNo = n % 8;
+ byteNo = (int)(n / 8);
+ bitNo = (int)(n % 8);
byte = ((unsigned char *) VARDATA_ANY(v))[byteNo];
@@ -3504,7 +3504,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,
@@ -3514,14 +3514,14 @@ 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)));
+ errmsg("index "INT64_FORMAT" out of valid range, 0.."INT64_FORMAT,
+ n, (int64)len * 8 - 1)));
- byteNo = n / 8;
- bitNo = n % 8;
+ 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 d352f9a10a..b9621de88d 100644
--- a/src/include/catalog/pg_proc.dat
+++ b/src/include/catalog/pg_proc.dat
@@ -1388,10 +1388,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 a3cd7d26fa..7135cd8994 100644
--- a/src/include/utils/builtins.h
+++ b/src/include/utils/builtins.h
@@ -30,8 +30,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: <2020032816585865712320@highgo.ca>
Permalink: ../2020032816585865712320@highgo.ca/
Also on: postgresql.org/message-id/2020032816585865712320@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: <2020032816585865712320@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