Thread: Optimize postgres protocol for fixed size arrays
Hi,
During conversion of the jdbc driver to use binary encoding when receiving array objects from postgres it was noticed
that for example for int[] arrays the binary encoding is normally 30% to 200% larger in bytes than the text encoding.
This was of concern to some users with slower links to database. Even though the encoded size was larger the binary
encoding was still constantly faster (with 50% speed up for float[]).
Here is a patch that adds a new flag to the protocol that is set when all elements of the array are of same fixed size.
When the bit is set the 4 byte length is only sent once and not for each element. Another restriction is that the flag
can only be set when there are no NULLs in the array.
The postgres part is my first try at the problem and I really need some feedback around the detection of fixed size
elements. I just made a guess and noticed that typlen != 0 seemed to work for the basic types I user for testing.
First the postgres patch:
diff --git a/src/backend/utils/adt/arrayfuncs.c b/src/backend/utils/adt/arrayfuncs.c
index bfb6065..970272f 100644
--- a/src/backend/utils/adt/arrayfuncs.c
+++ b/src/backend/utils/adt/arrayfuncs.c
@@ -86,7 +86,7 @@ static void ReadArrayBinary(StringInfo buf, int nitems,
FmgrInfo *receiveproc, Oid typioparam, int32 typmod,
int typlen, bool typbyval, char typalign,
Datum *values, bool *nulls,
- bool *hasnulls, int32 *nbytes);
+ bool *hasnulls, int32 *nbytes, bool fixedlen);
static void CopyArrayEls(ArrayType *array,
Datum *values, bool *nulls, int nitems,
int typlen, bool typbyval, char typalign,
@@ -1242,7 +1242,7 @@ array_recv(PG_FUNCTION_ARGS)
ndim, MAXDIM)));
flags = pq_getmsgint(buf, 4);
- if (flags != 0 && flags != 1)
+ if ((flags & ~3) != 0)
ereport(ERROR,
(errcode(ERRCODE_INVALID_BINARY_REPRESENTATION),
errmsg("invalid array flags")));
@@ -1327,7 +1327,7 @@ array_recv(PG_FUNCTION_ARGS)
&my_extra->proc, typioparam, typmod,
typlen, typbyval, typalign,
dataPtr, nullsPtr,
- &hasnulls, &nbytes);
+ &hasnulls, &nbytes, (flags & 2) != 0);
if (hasnulls)
{
dataoffset = ARR_OVERHEAD_WITHNULLS(ndim, nitems);
@@ -1390,7 +1390,8 @@ ReadArrayBinary(StringInfo buf,
Datum *values,
bool *nulls,
bool *hasnulls,
- int32 *nbytes)
+ int32 *nbytes,
+ bool fixedlen)
{
int i;
bool hasnull;
@@ -1403,7 +1404,7 @@ ReadArrayBinary(StringInfo buf,
char csave;
/* Get and check the item length */
- itemlen = pq_getmsgint(buf, 4);
+ itemlen = fixedlen ? typlen : pq_getmsgint(buf, 4);
if (itemlen < -1 || itemlen > (buf->len - buf->cursor))
ereport(ERROR,
(errcode(ERRCODE_INVALID_BINARY_REPRESENTATION),
@@ -1496,6 +1497,7 @@ array_send(PG_FUNCTION_ARGS)
int bitmask;
int nitems,
i;
+ int flags;
int ndim,
*dim;
StringInfoData buf;
@@ -1535,6 +1537,8 @@ array_send(PG_FUNCTION_ARGS)
typbyval = my_extra->typbyval;
typalign = my_extra->typalign;
+ flags = ARR_HASNULL(v) ? 1 : (typlen > 0 ? 2 : 0);
+
ndim = ARR_NDIM(v);
dim = ARR_DIMS(v);
nitems = ArrayGetNItems(ndim, dim);
@@ -1543,7 +1547,7 @@ array_send(PG_FUNCTION_ARGS)
/* Send the array header information */
pq_sendint(&buf, ndim, 4);
- pq_sendint(&buf, ARR_HASNULL(v) ? 1 : 0, 4);
+ pq_sendint(&buf, flags, 4);
pq_sendint(&buf, element_type, sizeof(Oid));
for (i = 0; i < ndim; i++)
{
@@ -1571,7 +1575,8 @@ array_send(PG_FUNCTION_ARGS)
itemvalue = fetch_att(p, typbyval, typlen);
outputbytes = SendFunctionCall(&my_extra->proc, itemvalue);
- pq_sendint(&buf, VARSIZE(outputbytes) - VARHDRSZ, 4);
+ if ((flags & 2) == 0)
+ pq_sendint(&buf, VARSIZE(outputbytes) - VARHDRSZ, 4);
pq_sendbytes(&buf, VARDATA(outputbytes),
VARSIZE(outputbytes) - VARHDRSZ);
pfree(outputbytes);
And here is the matching jdbc driver patch (similar changes need to be done to other drivers too):
Index: org/postgresql/jdbc2/AbstractJdbc2Array.java
===================================================================
RCS file: /cvsroot/jdbc/pgjdbc/org/postgresql/jdbc2/AbstractJdbc2Array.java,v
retrieving revision 1.29
diff -u -r1.29 AbstractJdbc2Array.java
--- org/postgresql/jdbc2/AbstractJdbc2Array.java 30 Sep 2011 10:08:17 -0000 1.29
+++ org/postgresql/jdbc2/AbstractJdbc2Array.java 22 Nov 2011 21:06:15 -0000
@@ -175,7 +175,7 @@
private Object readBinaryArray(int index, int count) throws SQLException {
int dimensions = ByteConverter.int4(fieldBytes, 0);
- //int flags = ByteConverter.int4(fieldBytes, 4); // bit 0: 0=no-nulls, 1=has-nulls
+ int flags = ByteConverter.int4(fieldBytes, 4); // bit 0: 0=no-nulls, 1=has-nulls, bit 1: 0=variable-size
objects,2=fixed-size
int elementOid = ByteConverter.int4(fieldBytes, 8);
int pos = 12;
int[] dims = new int[dimensions];
@@ -190,8 +190,12 @@
dims[0] = Math.min(count, dims[0]);
}
Object arr = java.lang.reflect.Array.newInstance(elementOidToClass(elementOid), dims);
+ int len = -1;
+ if ((flags & 2) != 0) {
+ len = fixedBinaryLengthOfOid(elementOid);
+ }
try {
- storeValues((Object[]) arr, elementOid, dims, pos, 0, index);
+ storeValues((Object[]) arr, elementOid, dims, len, pos, 0, index);
}
catch (IOException ioe)
{
@@ -200,18 +204,27 @@
return arr;
}
- private int storeValues(final Object[] arr, int elementOid, final int[] dims, int pos, final int thisDimension,
intindex) throws SQLException, IOException {
+ private int storeValues(final Object[] arr, int elementOid, final int[] dims, int len, int pos, final int
thisDimension,int index) throws SQLException,
IOException {
if (thisDimension == dims.length - 1) {
- for (int i = 1; i < index; ++i) {
- int len = ByteConverter.int4(fieldBytes, pos); pos += 4;
- if (len != -1) {
- pos += len;
+ if (len > 0) {
+ pos += len * (index - 1);
+ } else {
+ for (int i = 1; i < index; ++i) {
+ int l = ByteConverter.int4(fieldBytes, pos); pos += 4;
+ if (l != -1) {
+ pos += l;
+ }
}
}
for (int i = 0; i < dims[thisDimension]; ++i) {
- int len = ByteConverter.int4(fieldBytes, pos); pos += 4;
- if (len == -1) {
- continue;
+ int l;
+ if (len > 0) {
+ l = len;
+ } else {
+ l = ByteConverter.int4(fieldBytes, pos); pos += 4;
+ if (l == -1) {
+ continue;
+ }
}
switch (elementOid) {
case Oid.INT2:
@@ -232,14 +245,14 @@
case Oid.TEXT:
case Oid.VARCHAR:
Encoding encoding = connection.getEncoding();
- arr[i] = encoding.decode(fieldBytes, pos, len);
+ arr[i] = encoding.decode(fieldBytes, pos, l);
break;
}
- pos += len;
+ pos += l;
}
} else {
for (int i = 0; i < dims[thisDimension]; ++i) {
- pos = storeValues((Object[]) arr[i], elementOid, dims, pos, thisDimension + 1, 0);
+ pos = storeValues((Object[]) arr[i], elementOid, dims, len, pos, thisDimension + 1, 0);
}
}
return pos;
@@ -358,6 +371,27 @@
}
}
+ private int fixedBinaryLengthOfOid(int oid) throws SQLException {
+ switch (oid) {
+ case Oid.INT2:
+ return 2;
+ case Oid.INT4:
+ return 4;
+ case Oid.INT8:
+ return 8;
+ case Oid.FLOAT4:
+ return 4;
+ case Oid.FLOAT8:
+ return 8;
+ case Oid.TEXT:
+ case Oid.VARCHAR:
+ return -1;
+ default:
+ throw org.postgresql.Driver.notImplemented(this.getClass(),
+ "readBinaryArray(data,oid)");
+ }
+ }
+
/**
* Build {@link ArrayList} from field's string input. As a result
* of this method {@link #arrayList} is build. Method can be called
On Tue, Nov 22, 2011 at 11:47:22PM +0200, Mikko Tiihonen wrote: > Hi, > > During conversion of the jdbc driver to use binary encoding when receiving array objects from postgres it was noticed > that for example for int[] arrays the binary encoding is normally 30% to 200% larger in bytes than the text encoding. > > This was of concern to some users with slower links to database. Even though the encoded size was larger the binary > encoding was still constantly faster (with 50% speed up for float[]). > > Here is a patch that adds a new flag to the protocol that is set when all elements of the array are of same fixed size. > When the bit is set the 4 byte length is only sent once and not for each element. Another restriction is that the flag > can only be set when there are no NULLs in the array. > Cool. This would be very useful with the DSPAM binary array driver. Although the binary is shorter because the values are 8 byte integers, they would be much shorter without the redundant sizing information. Barring issues: +1 Regards, Ken
On Tue, Nov 22, 2011 at 3:47 PM, Mikko Tiihonen
<mikko.tiihonen@nitorcreations.com> wrote:
> Hi,
>
> During conversion of the jdbc driver to use binary encoding when receiving
> array objects from postgres it was noticed
> that for example for int[] arrays the binary encoding is normally 30% to
> 200% larger in bytes than the text encoding.
>
> This was of concern to some users with slower links to database. Even though
> the encoded size was larger the binary
> encoding was still constantly faster (with 50% speed up for float[]).
>
> Here is a patch that adds a new flag to the protocol that is set when all
> elements of the array are of same fixed size.
> When the bit is set the 4 byte length is only sent once and not for each
> element. Another restriction is that the flag
> can only be set when there are no NULLs in the array.
>
> The postgres part is my first try at the problem and I really need some
> feedback around the detection of fixed size
> elements. I just made a guess and noticed that typlen != 0 seemed to work
> for the basic types I user for testing.
>
> First the postgres patch:
>
> diff --git a/src/backend/utils/adt/arrayfuncs.c
> b/src/backend/utils/adt/arrayfuncs.c
> index bfb6065..970272f 100644
> --- a/src/backend/utils/adt/arrayfuncs.c
> +++ b/src/backend/utils/adt/arrayfuncs.c
> @@ -86,7 +86,7 @@ static void ReadArrayBinary(StringInfo buf, int nitems,
> FmgrInfo *receiveproc, Oid typioparam, int32
> typmod,
> int typlen, bool typbyval, char typalign,
> Datum *values, bool *nulls,
> - bool *hasnulls, int32 *nbytes);
> + bool *hasnulls, int32 *nbytes, bool
> fixedlen);
> static void CopyArrayEls(ArrayType *array,
> Datum *values, bool *nulls, int nitems,
> int typlen, bool typbyval, char typalign,
> @@ -1242,7 +1242,7 @@ array_recv(PG_FUNCTION_ARGS)
> ndim, MAXDIM)));
>
> flags = pq_getmsgint(buf, 4);
> - if (flags != 0 && flags != 1)
> + if ((flags & ~3) != 0)
> ereport(ERROR,
>
> (errcode(ERRCODE_INVALID_BINARY_REPRESENTATION),
> errmsg("invalid array flags")));
> @@ -1327,7 +1327,7 @@ array_recv(PG_FUNCTION_ARGS)
> &my_extra->proc, typioparam, typmod,
> typlen, typbyval, typalign,
> dataPtr, nullsPtr,
> - &hasnulls, &nbytes);
> + &hasnulls, &nbytes, (flags & 2) !=
> 0);
> if (hasnulls)
> {
> dataoffset = ARR_OVERHEAD_WITHNULLS(ndim, nitems);
> @@ -1390,7 +1390,8 @@ ReadArrayBinary(StringInfo buf,
> Datum *values,
> bool *nulls,
> bool *hasnulls,
> - int32 *nbytes)
> + int32 *nbytes,
> + bool fixedlen)
> {
> int i;
> bool hasnull;
> @@ -1403,7 +1404,7 @@ ReadArrayBinary(StringInfo buf,
> char csave;
>
> /* Get and check the item length */
> - itemlen = pq_getmsgint(buf, 4);
> + itemlen = fixedlen ? typlen : pq_getmsgint(buf, 4);
> if (itemlen < -1 || itemlen > (buf->len - buf->cursor))
> ereport(ERROR,
>
> (errcode(ERRCODE_INVALID_BINARY_REPRESENTATION),
> @@ -1496,6 +1497,7 @@ array_send(PG_FUNCTION_ARGS)
> int bitmask;
> int nitems,
> i;
> + int flags;
> int ndim,
> *dim;
> StringInfoData buf;
> @@ -1535,6 +1537,8 @@ array_send(PG_FUNCTION_ARGS)
> typbyval = my_extra->typbyval;
> typalign = my_extra->typalign;
>
> + flags = ARR_HASNULL(v) ? 1 : (typlen > 0 ? 2 : 0);
> +
> ndim = ARR_NDIM(v);
> dim = ARR_DIMS(v);
> nitems = ArrayGetNItems(ndim, dim);
> @@ -1543,7 +1547,7 @@ array_send(PG_FUNCTION_ARGS)
>
> /* Send the array header information */
> pq_sendint(&buf, ndim, 4);
> - pq_sendint(&buf, ARR_HASNULL(v) ? 1 : 0, 4);
> + pq_sendint(&buf, flags, 4);
> pq_sendint(&buf, element_type, sizeof(Oid));
> for (i = 0; i < ndim; i++)
> {
> @@ -1571,7 +1575,8 @@ array_send(PG_FUNCTION_ARGS)
>
> itemvalue = fetch_att(p, typbyval, typlen);
> outputbytes = SendFunctionCall(&my_extra->proc,
> itemvalue);
> - pq_sendint(&buf, VARSIZE(outputbytes) - VARHDRSZ,
> 4);
> + if ((flags & 2) == 0)
> + pq_sendint(&buf, VARSIZE(outputbytes) -
> VARHDRSZ, 4);
> pq_sendbytes(&buf, VARDATA(outputbytes),
> VARSIZE(outputbytes) -
> VARHDRSZ);
> pfree(outputbytes);
>
>
>
>
> And here is the matching jdbc driver patch (similar changes need to be done
> to other drivers too):
>
> Index: org/postgresql/jdbc2/AbstractJdbc2Array.java
> ===================================================================
> RCS file:
> /cvsroot/jdbc/pgjdbc/org/postgresql/jdbc2/AbstractJdbc2Array.java,v
> retrieving revision 1.29
> diff -u -r1.29 AbstractJdbc2Array.java
> --- org/postgresql/jdbc2/AbstractJdbc2Array.java 30 Sep 2011 10:08:17
> -0000 1.29
> +++ org/postgresql/jdbc2/AbstractJdbc2Array.java 22 Nov 2011 21:06:15
> -0000
> @@ -175,7 +175,7 @@
>
> private Object readBinaryArray(int index, int count) throws SQLException
> {
> int dimensions = ByteConverter.int4(fieldBytes, 0);
> - //int flags = ByteConverter.int4(fieldBytes, 4); // bit 0:
> 0=no-nulls, 1=has-nulls
> + int flags = ByteConverter.int4(fieldBytes, 4); // bit 0:
> 0=no-nulls, 1=has-nulls, bit 1: 0=variable-size objects, 2=fixed-size
> int elementOid = ByteConverter.int4(fieldBytes, 8);
> int pos = 12;
> int[] dims = new int[dimensions];
> @@ -190,8 +190,12 @@
> dims[0] = Math.min(count, dims[0]);
> }
> Object arr =
> java.lang.reflect.Array.newInstance(elementOidToClass(elementOid), dims);
> + int len = -1;
> + if ((flags & 2) != 0) {
> + len = fixedBinaryLengthOfOid(elementOid);
> + }
> try {
> - storeValues((Object[]) arr, elementOid, dims, pos, 0, index);
> + storeValues((Object[]) arr, elementOid, dims, len, pos, 0,
> index);
> }
> catch (IOException ioe)
> {
> @@ -200,18 +204,27 @@
> return arr;
> }
>
> - private int storeValues(final Object[] arr, int elementOid, final int[]
> dims, int pos, final int thisDimension, int index) throws SQLException,
> IOException {
> + private int storeValues(final Object[] arr, int elementOid, final int[]
> dims, int len, int pos, final int thisDimension, int index) throws
> SQLException, IOException {
> if (thisDimension == dims.length - 1) {
> - for (int i = 1; i < index; ++i) {
> - int len = ByteConverter.int4(fieldBytes, pos); pos += 4;
> - if (len != -1) {
> - pos += len;
> + if (len > 0) {
> + pos += len * (index - 1);
> + } else {
> + for (int i = 1; i < index; ++i) {
> + int l = ByteConverter.int4(fieldBytes, pos); pos += 4;
> + if (l != -1) {
> + pos += l;
> + }
> }
> }
> for (int i = 0; i < dims[thisDimension]; ++i) {
> - int len = ByteConverter.int4(fieldBytes, pos); pos += 4;
> - if (len == -1) {
> - continue;
> + int l;
> + if (len > 0) {
> + l = len;
> + } else {
> + l = ByteConverter.int4(fieldBytes, pos); pos += 4;
> + if (l == -1) {
> + continue;
> + }
> }
> switch (elementOid) {
> case Oid.INT2:
> @@ -232,14 +245,14 @@
> case Oid.TEXT:
> case Oid.VARCHAR:
> Encoding encoding = connection.getEncoding();
> - arr[i] = encoding.decode(fieldBytes, pos, len);
> + arr[i] = encoding.decode(fieldBytes, pos, l);
> break;
> }
> - pos += len;
> + pos += l;
> }
> } else {
> for (int i = 0; i < dims[thisDimension]; ++i) {
> - pos = storeValues((Object[]) arr[i], elementOid, dims, pos,
> thisDimension + 1, 0);
> + pos = storeValues((Object[]) arr[i], elementOid, dims, len,
> pos, thisDimension + 1, 0);
> }
> }
> return pos;
> @@ -358,6 +371,27 @@
> }
> }
>
> + private int fixedBinaryLengthOfOid(int oid) throws SQLException {
> + switch (oid) {
> + case Oid.INT2:
> + return 2;
> + case Oid.INT4:
> + return 4;
> + case Oid.INT8:
> + return 8;
> + case Oid.FLOAT4:
> + return 4;
> + case Oid.FLOAT8:
> + return 8;
> + case Oid.TEXT:
> + case Oid.VARCHAR:
> + return -1;
> + default:
> + throw org.postgresql.Driver.notImplemented(this.getClass(),
> + "readBinaryArray(data,oid)");
> + }
> + }
> +
> /**
> * Build {@link ArrayList} from field's string input. As a result
> * of this method {@link #arrayList} is build. Method can be called
+1. just be advised that this is a significant change to the binary
wire format and will cause headaches to those that are using it (you
are supposed to expect those headaches though -- using the wire format
is analogous to attending a rock concert). maybe this would be an
appropriate place to discuss a well advertised place in the
documentation that describes changes to the wire format?
libpqtypes users of course will not have to worry about this because
the format change will be handled in the library.
merlin
On 23 November 2011 10:47, Mikko Tiihonen <mikko.tiihonen@nitorcreations.com> wrote: > Here is a patch that adds a new flag to the protocol that is set when all > elements of the array are of same fixed size. > When the bit is set the 4 byte length is only sent once and not for each > element. Another restriction is that the flag > can only be set when there are no NULLs in the array. How does a client detect that this feature is supported? At a glance the JDBC patch doesn't use it on the send path, but presumably clients could use this when sending binary-format arrays to the server - but only if the server understood the format. (Ideally a pair of settings would be useful here - one to say "the server understands the new format" and another the client sets to say "please use the new format" that defaults to off - then you could avoid confusing old clients, too) Oliver
Oliver Jowett <oliver@opencloud.com> writes:
> On 23 November 2011 10:47, Mikko Tiihonen
> <mikko.tiihonen@nitorcreations.com> wrote:
>> Here is a patch that adds a new flag to the protocol that is set when all
>> elements of the array are of same fixed size.
> How does a client detect that this feature is supported?
The only way that anything like this will go in is as part of a protocol
version bump, so discoverability would reduce to knowing which protocol
you're using. We should file this away as one of the things we might
want to do whenever there's sufficient critical mass for a new wire
protocol version.
Note that COPY BINARY files would be affected too, and so we'd want to
make sure that this sort of change is recognizable from a binary file's
header.
regards, tom lane
On Tue, Nov 22, 2011 at 6:52 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote: > Oliver Jowett <oliver@opencloud.com> writes: >> On 23 November 2011 10:47, Mikko Tiihonen >> <mikko.tiihonen@nitorcreations.com> wrote: >>> Here is a patch that adds a new flag to the protocol that is set when all >>> elements of the array are of same fixed size. > >> How does a client detect that this feature is supported? > > The only way that anything like this will go in is as part of a protocol > version bump, so discoverability would reduce to knowing which protocol > you're using. We should file this away as one of the things we might > want to do whenever there's sufficient critical mass for a new wire > protocol version. > > Note that COPY BINARY files would be affected too, and so we'd want to > make sure that this sort of change is recognizable from a binary file's > header. Wire format changes can only be made with a protocol version bump? Is this a new policy? In the past they were just made...for example the money type was bumped to 64 bits. In the past it was always buyer beware for binary format consumers. merlin
Merlin Moncure <mmoncure@gmail.com> writes:
> On Tue, Nov 22, 2011 at 6:52 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> The only way that anything like this will go in is as part of a protocol
>> version bump,
> Wire format changes can only be made with a protocol version bump? Is
> this a new policy? In the past they were just made...for example the
> money type was bumped to 64 bits. In the past it was always buyer
> beware for binary format consumers.
Well, (a) our standards have gone up over time, (b) binary protocol is
getting more widely used (in part due to your own efforts), and (c)
money is a third-class stepchild anyway. I don't think we can get away
with changing the binary representation of such widely used types as
int and float arrays, unless we have some pretty carefully thought
through notion of how the client and server will negotiate what to do.
Now it's possible we could do that without formally calling it a
protocol version change, but I don't care at all for the idea of coming
up with one-off hacks every time somebody decides that some feature is
important enough that they have to have it Right Now instead of waiting
for a sufficient accumulation of reasons to have a protocol flag day.
I think "but we made arrays a bit smaller!" is a pretty lame response
to have to give when somebody complains that Postgres 9.2 broke their
client software. When we do it, I want to have a *long* list of good
reasons.
BTW, so far as the actual array format is concerned, I don't think
the proposal is acceptable as-is: it renders the received array entirely
unreadable unless the reader knows a-priori what the sender thought the
typlen was. It would be a lot better if the fixed-length flag meant
that the typlen is given once in the array header rather than once per
element. I'm not thrilled by the "no nulls" restriction, either,
though I admit I don't have a good idea about avoiding that offhand.
regards, tom lane
On 24 November 2011 05:36, Tom Lane <tgl@sss.pgh.pa.us> wrote: > Now it's possible we could do that without formally calling it a > protocol version change, but I don't care at all for the idea of coming > up with one-off hacks every time somebody decides that some feature is > important enough that they have to have it Right Now instead of waiting > for a sufficient accumulation of reasons to have a protocol flag day. > I think "but we made arrays a bit smaller!" is a pretty lame response > to have to give when somebody complains that Postgres 9.2 broke their > client software. When we do it, I want to have a *long* list of good > reasons. Can we get a mechanism for minor protocol changes in this future version? Something as simple as exchanging a list of protocol features during the initial handshake (then use only features that are present on both sides) would be enough. The difficulty of making any protocol changes at the moment is a big stumbling block. (You could probably retrofit that to the current protocol version) Oliver