Обсуждение: 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@gmail.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@gmail.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@gmail.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@gmail.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@gmail.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.linnakangas@enterprisedb.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@gmail.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.linnakangas@enterprisedb.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@gmail.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@gmail.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@gmail.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@gmail.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@gmail.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@gmail.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@gmail.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@gmail.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@sss.pgh.pa.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. =A0Shouldn't we ensure that binary dump and reload of an array value > doesn't change the value in any SQL-observable way? =A0If 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? =A00-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? --=20 greg
Greg Stark <gsstark@mit.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