Re: BUG #5748: Invalid oidvector data during binary recv

The following bug has been logged online:

Bug reference: 5748
Logged by: Yeb Havinga
Email address: yebhavinga(at)gmail(dot)com
PostgreSQL version: 9.0.1
Operating system: Linux
Description: Invalid oidvector data during binary recv
Details:

postgres=# create table a as select ''::oidvector;
SELECT 1
postgres=# copy a to '/tmp/test' with binary;
COPY 1
postgres=# copy a from '/tmp/test' with binary;
ERROR: invalid oidvector data

The error caused by the ARR_LBOUND(result)[0] != 0) check in oidvectorrecv,
and after some debugging and looking at common values of the lbound, I
wonder if this check itself is correct.

regards,
Yeb Havinga

On 11.11.2010 16:30, Yeb Havinga wrote:
>
> The following bug has been logged online:
>
> Bug reference: 5748
> Logged by: Yeb Havinga
> Email address: yebhavinga(at)gmail(dot)com
> PostgreSQL version: 9.0.1
> Operating system: Linux
> Description: Invalid oidvector data during binary recv
> Details:
>
> postgres=# create table a as select ''::oidvector;
> SELECT 1
> postgres=# copy a to '/tmp/test' with binary;
> COPY 1
> postgres=# copy a from '/tmp/test' with binary;
> ERROR: invalid oidvector data
>
> The error caused by the ARR_LBOUND(result)[0] != 0) check in oidvectorrecv,
> and after some debugging and looking at common values of the lbound, I
> wonder if this check itself is correct.

That check was added a while ago to make it impossible to inject values
into the system that the text input functions wouldn't accept. There is
no way to create an oidvector with non-zero lower bound through
oidvectorin. But it looks like the check is not right for an empty array.

Will fix, thanks for the report.

--
Heikki Linnakangas
EnterpriseDB http://www.enterprisedb.com

"Yeb Havinga" <yebhavinga(at)gmail(dot)com> writes:
> postgres=# create table a as select ''::oidvector;
> SELECT 1
> postgres=# copy a to '/tmp/test' with binary;
> COPY 1
> postgres=# copy a from '/tmp/test' with binary;
> ERROR: invalid oidvector data

The problem seems to be that array_recv passes back a zero-dimensional
array, *not* a 1-D array, when it observes that the input has no
elements. A zero-D array is not part of the subset of possible arrays
that we allow for oidvector.

I'm less than convinced that this is worth fixing. oidvector is not
intended for general-purpose use anyway. What's the use-case where this
would come up?

regards, tom lane

On 11.11.2010 17:48, Tom Lane wrote:
> "Yeb Havinga"<yebhavinga(at)gmail(dot)com> writes:
>> postgres=# create table a as select ''::oidvector;
>> SELECT 1
>> postgres=# copy a to '/tmp/test' with binary;
>> COPY 1
>> postgres=# copy a from '/tmp/test' with binary;
>> ERROR: invalid oidvector data
>
> The problem seems to be that array_recv passes back a zero-dimensional
> array, *not* a 1-D array, when it observes that the input has no
> elements. A zero-D array is not part of the subset of possible arrays
> that we allow for oidvector.

Yeah, I just reached that conclusion too..

> I'm less than convinced that this is worth fixing. oidvector is not
> intended for general-purpose use anyway. What's the use-case where this
> would come up?

I don't see any use case either, but I guess it would be nice to fix for
the sake of completeness.

--
Heikki Linnakangas
EnterpriseDB http://www.enterprisedb.com

On 2010-11-11 16:48, Tom Lane wrote:
> "Yeb Havinga"<yebhavinga(at)gmail(dot)com> writes:
>> postgres=# create table a as select ''::oidvector;
>> SELECT 1
>> postgres=# copy a to '/tmp/test' with binary;
>> COPY 1
>> postgres=# copy a from '/tmp/test' with binary;
>> ERROR: invalid oidvector data
> The problem seems to be that array_recv passes back a zero-dimensional
> array, *not* a 1-D array, when it observes that the input has no
> elements. A zero-D array is not part of the subset of possible arrays
> that we allow for oidvector.
>
> I'm less than convinced that this is worth fixing. oidvector is not
> intended for general-purpose use anyway. What's the use-case where this
> would come up?
We're currently reading data from a remote pg_statistics, in particular
stavalues1.. etc. Even when our own user defined relations do not make
use of oidvectors (or intvectors), during testing on arbitrary
pg_statistic rows we encountered this error message. Nonetheless we
decided to report it as a bug, since it was not related to anyarray
handling, but clearly a bug that oidvector cannot input binary, what it
can input as text and output binary.

regards,
Yeb Havinga

Heikki Linnakangas <heikki(dot)linnakangas(at)enterprisedb(dot)com> writes:
> On 11.11.2010 17:48, Tom Lane wrote:
>> The problem seems to be that array_recv passes back a zero-dimensional
>> array, *not* a 1-D array, when it observes that the input has no
>> elements. A zero-D array is not part of the subset of possible arrays
>> that we allow for oidvector.

> Yeah, I just reached that conclusion too..

>> I'm less than convinced that this is worth fixing. oidvector is not
>> intended for general-purpose use anyway. What's the use-case where this
>> would come up?

> I don't see any use case either, but I guess it would be nice to fix for
> the sake of completeness.

The least risky fix would be to make oidvectorrecv check for a zero-D
result from array_recv and replace it with an empty 1-D result. I'm
not sufficiently excited about it to do it myself, but if you are,
have at it.

regards, tom lane

On 11.11.2010 18:11, Yeb Havinga wrote:
> On 2010-11-11 16:48, Tom Lane wrote:
>> "Yeb Havinga"<yebhavinga(at)gmail(dot)com> writes:
>>> postgres=# create table a as select ''::oidvector;
>>> SELECT 1
>>> postgres=# copy a to '/tmp/test' with binary;
>>> COPY 1
>>> postgres=# copy a from '/tmp/test' with binary;
>>> ERROR: invalid oidvector data
>> The problem seems to be that array_recv passes back a zero-dimensional
>> array, *not* a 1-D array, when it observes that the input has no
>> elements. A zero-D array is not part of the subset of possible arrays
>> that we allow for oidvector.
>>
>> I'm less than convinced that this is worth fixing. oidvector is not
>> intended for general-purpose use anyway. What's the use-case where this
>> would come up?
> We're currently reading data from a remote pg_statistics, in particular
> stavalues1.. etc. Even when our own user defined relations do not make
> use of oidvectors (or intvectors), during testing on arbitrary
> pg_statistic rows we encountered this error message. Nonetheless we
> decided to report it as a bug, since it was not related to anyarray
> handling, but clearly a bug that oidvector cannot input binary, what it
> can input as text and output binary.

Just reading such an oidvector should not throw an error, you must've
tried to send it back to the server, perhaps to store it to a table. But
pg_statistic.stavalues* are a special anyway. They are defined as
anyarray, but we don't normally allow you to create a table with
anyarray columns.

--
Heikki Linnakangas
EnterpriseDB http://www.enterprisedb.com

BTW ... it strikes me there's another inconsistency between oidvectorin
and oidvectorrecv, namely that the former enforces a maximum of
FUNC_MAX_ARGS elements whereas the latter has no such limit. Should we
do something about that, and if so what? It wouldn't be too hard to get
rid of the former's maximum, but I'm worried about whether any code is
dependent on an assumption that oidvectors can't get toasted. The
type's marked untoastable in pg_type, so maybe enforcing the same limit
in the latter is a safer idea.

regards, tom lane

On 11.11.2010 18:17, Tom Lane wrote:
> BTW ... it strikes me there's another inconsistency between oidvectorin
> and oidvectorrecv, namely that the former enforces a maximum of
> FUNC_MAX_ARGS elements whereas the latter has no such limit.

Yes, it does:

> /* check length for consistency with oidvectorin() */
> if (ARR_DIMS(result)[0] > FUNC_MAX_ARGS)
> ereport(ERROR,
> (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
> errmsg("oidvector has too many elements")));
>

I suspect you're looking at an old version, that was added quite recently.

--
Heikki Linnakangas
EnterpriseDB http://www.enterprisedb.com

Heikki Linnakangas <heikki(dot)linnakangas(at)enterprisedb(dot)com> writes:
> On 11.11.2010 18:17, Tom Lane wrote:
>> BTW ... it strikes me there's another inconsistency between oidvectorin
>> and oidvectorrecv, namely that the former enforces a maximum of
>> FUNC_MAX_ARGS elements whereas the latter has no such limit.

> Yes, it does:

Oh, never mind ...

> I suspect you're looking at an old version, that was added quite recently.

No, just failing to scroll down far enough :-(. Better go find some
more caffeine.

regards, tom lane

On 2010-11-11 17:14, Heikki Linnakangas wrote:
> On 11.11.2010 18:11, Yeb Havinga wrote:
>> We're currently reading data from a remote pg_statistics, in particular
>> stavalues1.. etc. Even when our own user defined relations do not make
>> use of oidvectors (or intvectors), during testing on arbitrary
>> pg_statistic rows we encountered this error message. Nonetheless we
>> decided to report it as a bug, since it was not related to anyarray
>> handling, but clearly a bug that oidvector cannot input binary, what it
>> can input as text and output binary.
>
> Just reading such an oidvector should not throw an error, you must've
> tried to send it back to the server, perhaps to store it to a table.
Hmm.. we're reading with libpq in binary mode and the error is thrown in
the receive function call. No storing yet in a table at that point.
> But pg_statistic.stavalues* are a special anyway. They are defined as
> anyarray, but we don't normally allow you to create a table with
> anyarray columns.
>
Yes, but the test case shows the actual error can be triggered without
any use of anyarray.

If there is going to be a patch fixing things, the value
'{"1"}'::oidvector[] can't be exported and imported through binary send
recv as well.

regards,
Yeb Havinga

Yeb Havinga <yebhavinga(at)gmail(dot)com> writes:
> If there is going to be a patch fixing things, the value
> '{"1"}'::oidvector[] can't be exported and imported through binary send
> recv as well.

That's pilot error, nothing more nor less. (oidvector != oidvector[])

regards, tom lane

On 2010-11-11 17:56, Tom Lane wrote:
> Yeb Havinga<yebhavinga(at)gmail(dot)com> writes:
>> If there is going to be a patch fixing things, the value
>> '{"1"}'::oidvector[] can't be exported and imported through binary send
>> recv as well.
> That's pilot error, nothing more nor less. (oidvector != oidvector[])
We're not trying to import the '{"1"}' binary value with oidvectorrecv.
We hit the error in oidvectorrecv because we binary received an
oidvector[] (through array_recv), so made an initial test case with an
oidvector, not oidvector[].

Anyway I now have troubles to make a good test case to trip the lbound
check. The actual oidvector array that failed was '{"23 23 2275 2281
23","",2281,26,2275,23,"25 25",701,25,20,1700,"23 23","701
701",21,700,"1700 1700","603 603","2281 2281","20 20","600 600","718
718","21 21",1022,"2281 26 2281 23","2281 26 2281 21 2281","17 17","20
23",602,869,1186,"601"}', but when I test with that now, the non-zero
lbound value is on a oidvector that also has a zero ndim.

IOW: sorry for the noise, just trying to make a good bug report.

regards,
Yeb Havinga

Yeb Havinga <yebhavinga(at)gmail(dot)com> writes:
> Anyway I now have troubles to make a good test case to trip the lbound
> check.

I don't believe you ever did have a test case that would trip the lbound
check. What was failing was the ARR_NDIM == 1 check. If you were to
look at the array in gdb, the apparent value of the lbound would be
indeterminate because it's off the end of the allocated space for a
zero-D array. But the code wouldn't be reaching that test.

regards, tom lane

On 2010-11-11 17:02, Heikki Linnakangas wrote:
> On 11.11.2010 17:48, Tom Lane wrote:
>> "Yeb Havinga"<yebhavinga(at)gmail(dot)com> writes:
>>> postgres=# create table a as select ''::oidvector;
>>> SELECT 1
>>> postgres=# copy a to '/tmp/test' with binary;
>>> COPY 1
>>> postgres=# copy a from '/tmp/test' with binary;
>>> ERROR: invalid oidvector data
>>
>> The problem seems to be that array_recv passes back a zero-dimensional
>> array, *not* a 1-D array, when it observes that the input has no
>> elements. A zero-D array is not part of the subset of possible arrays
>> that we allow for oidvector.
>
> Yeah, I just reached that conclusion too..
The patch below changes array_recv, so that it returns an empty 1-D
array when an empty 1-D array was written binary. No changes in
oidvectorrecv or int2vectorrecv are needed.

diff --git a/src/backend/utils/adt/arrayfuncs.c
b/src/backend/utils/adt/arrayfuncs.c
index 4ceb256..98e725a 100644
--- a/src/backend/utils/adt/arrayfuncs.c
+++ b/src/backend/utils/adt/arrayfuncs.c
@@ -1288,7 +1288,7 @@ array_recv(PG_FUNCTION_ARGS)
my_extra->element_type = element_type;
}

- if (nitems == 0)
+ if (ndim == 0)
{
/* Return empty array ... but not till we've validated
element_type */
PG_RETURN_ARRAYTYPE_P(construct_empty_array(element_type));

Make check passes.

regards,
Yeb Havinga

On 15.11.2010 12:08, Yeb Havinga wrote:
> On 2010-11-11 17:02, Heikki Linnakangas wrote:
>> On 11.11.2010 17:48, Tom Lane wrote:
>>> "Yeb Havinga"<yebhavinga(at)gmail(dot)com> writes:
>>>> postgres=# create table a as select ''::oidvector;
>>>> SELECT 1
>>>> postgres=# copy a to '/tmp/test' with binary;
>>>> COPY 1
>>>> postgres=# copy a from '/tmp/test' with binary;
>>>> ERROR: invalid oidvector data
>>>
>>> The problem seems to be that array_recv passes back a zero-dimensional
>>> array, *not* a 1-D array, when it observes that the input has no
>>> elements. A zero-D array is not part of the subset of possible arrays
>>> that we allow for oidvector.
>>
>> Yeah, I just reached that conclusion too..
> The patch below changes array_recv, so that it returns an empty 1-D
> array when an empty 1-D array was written binary. No changes in
> oidvectorrecv or int2vectorrecv are needed.

That seems like a bad idea. array_in() represents an empty array with
zero dimensions, we're not going to change the generic array_recv()
function used for all arrays to behave differently because of some
corner-case in oidvectorrecv.

If we want do anything about this, the right fix is to add a special
case in oidvectorrecv/int2vectorrecv to handle empty array.

--
Heikki Linnakangas
EnterpriseDB http://www.enterprisedb.com

On 2010-11-15 11:40, Heikki Linnakangas wrote:
> On 15.11.2010 12:08, Yeb Havinga wrote:
>> On 2010-11-11 17:02, Heikki Linnakangas wrote:
>>> On 11.11.2010 17:48, Tom Lane wrote:
>>>> "Yeb Havinga"<yebhavinga(at)gmail(dot)com> writes:
>>>>> postgres=# create table a as select ''::oidvector;
>>>>> SELECT 1
>>>>> postgres=# copy a to '/tmp/test' with binary;
>>>>> COPY 1
>>>>> postgres=# copy a from '/tmp/test' with binary;
>>>>> ERROR: invalid oidvector data
>>>>
>>>> The problem seems to be that array_recv passes back a zero-dimensional
>>>> array, *not* a 1-D array, when it observes that the input has no
>>>> elements. A zero-D array is not part of the subset of possible arrays
>>>> that we allow for oidvector.
>>>
>>> Yeah, I just reached that conclusion too..
>> The patch below changes array_recv, so that it returns an empty 1-D
>> array when an empty 1-D array was written binary. No changes in
>> oidvectorrecv or int2vectorrecv are needed.
>
> That seems like a bad idea. array_in() represents an empty array with
> zero dimensions, we're not going to change the generic array_recv()
> function used for all arrays to behave differently because of some
> corner-case in oidvectorrecv.
It's possible to trigger it with any element type, as long as the use
manages to construct a 1-D array. E.g. with contrib/_int.sql loaded,
array substraction is possible. Now:
postgres=# select ARRAY[1]::int4[] - ARRAY[1]::int4[];
?column?
----------
{}
(1 row)

postgres=# select array_ndims(ARRAY[1]::int4[] - ARRAY[1]::int4[]);
array_ndims
-------------
1
(1 row)

Binary send and recv of the value at hand would change it into a 0-D vector.

>
> If we want do anything about this, the right fix is to add a special
> case in oidvectorrecv/int2vectorrecv to handle empty array.
Judging from the fact that it also happens with int4 arrays, this seems
like the wrong place.

regards,
Yeb Havinga

Yeb Havinga <yebhavinga(at)gmail(dot)com> writes:
> Binary send and recv of the value at hand would change it into a 0-D vector.

The reason for that is that in general we don't make a distinction
between zero-size arrays of different dimensions. oidvector and
int2vector are different though. Which is why this should be fixed
locally to those two types, rather than changing the behavior of
regular arrays.

regards, tom lane

I wrote:
> Yeb Havinga <yebhavinga(at)gmail(dot)com> writes:
>> Binary send and recv of the value at hand would change it into a 0-D vector.

> The reason for that is that in general we don't make a distinction
> between zero-size arrays of different dimensions.

Actually, after consuming a bit more caffeine, I see what Yeb is on about.
Even though the system in general doesn't make much of a distinction
between zero-element arrays of different dimensionalities, there *are*
functions that can distinguish --- array_ndims() being the most obvious
one. Shouldn't we ensure that binary dump and reload of an array value
doesn't change the value in any SQL-observable way? If so, I think his
patch is correct, even though it's changing more than just the
originally-complained-of behavior.

While I'm looking at this ... why is it that array_ndims returns NULL
and not 0 for a zero-dimensional array? 0-D arrays might have been
unsupported at one time, but they're certainly considered valid now.

regards, tom lane

On Mon, Nov 15, 2010 at 4:51 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Actually, after consuming a bit more caffeine, I see what Yeb is on about.
> Even though the system in general doesn't make much of a distinction
> between zero-element arrays of different dimensionalities, there *are*
> functions that can distinguish --- array_ndims() being the most obvious
> one.  Shouldn't we ensure that binary dump and reload of an array value
> doesn't change the value in any SQL-observable way?  If so, I think his
> patch is correct, even though it's changing more than just the
> originally-complained-of behavior.

We went to a lot of effort to preserve lower bounds for dumped arrays
so I would agree. I was actually one of the few people that actually
ran into this prior to the fix. We had arrays generated by the intagg
functions which were 0-based and after dumping and reloading were
1-based causing our functions which calculated the array sizes to
misbehave.

>
> While I'm looking at this ... why is it that array_ndims returns NULL
> and not 0 for a zero-dimensional array?  0-D arrays might have been
> unsupported at one time, but they're certainly considered valid now.

Is this the same question as split() on enmpty strings? Do we have a
problem distinguishing between a 0-dimensional array and a
1-dimensional empty array?

--
greg

Greg Stark <gsstark(at)mit(dot)edu> writes:
> Is this the same question as split() on enmpty strings? Do we have a
> problem distinguishing between a 0-dimensional array and a
> 1-dimensional empty array?

Internally they're certainly different. I've just been sniffing around
the code a bit, and the main problem I see with trying to be more
rigorous about this is that array_out just prints '{}' for any
zero-element array, regardless of the recorded dimensionality. We could
reserve that notation to mean a 0-D array (which is what array_in reads
it as). But then we have to figure out what to print for other cases.
The only good idea that comes to mind is
[1:0]={}
(or variants of that depending on what the stored dimensions actually
are). array_in currently rejects that:

regression=# select '[1:0]={}'::int[];
ERROR: upper bound cannot be less than lower bound

but perhaps it wouldn't be too painful to tweak it to allow the case.

It appears to be fairly hard to actually get a non-zero-D empty array
into the system, so while this is pretty ugly I think it wouldn't affect
common usage.

Comments?

regards, tom lane