Thread: ecpg array support, or lack thereof

While looking at the memory leaks in ecpg that Coverity warned about and 
Michael just fixed, I started wondering if the code is ever used.

The leaks were in the code that takes a host variable, and converts it 
into a string for sending to the server as a query parameter. In 
particular, it was in code that deals with arrays. However, the fine 
manual says:

> SQL-level arrays are not directly supported in ECPG. It is not
> possible to simply map an SQL array into a C array host variable.
> This will result in undefined behavior. Some workarounds exist,
> however.

That very clearly says that the code that was fixed is not actually 
supported.

We do have a test case, 'arrays', that tests the code, but it only tests 
integer arrays. The leaks were in 'interval', 'timestamp', and 'numeric' 
arrays. And it turns out that there are two bugs there:

1. In timestamp, interval, and date, the array offset is calculated 
incorrectly:

str = quote_postgres(PGTYPESinterval_to_asc((interval *) ((var + 
var->offset * element)->value)), quote, lineno);

That "var + var->offset * element" has C datatype "struct variable *", 
not "char *" as the code assumes, so the calculated offset is wrong, 
leading to bogus parameters or segfault

2. The constructed array looks like this (for timestamp):

array [2005-06-23 11:22:33,2004-06-23 11:22:33]

That's bogus. It's sent as a query parameter, not embedded into an SQL 
string, so the syntax should be '{...}'.



It would be nice to fix these, and add a test case to cover all kinds of 
arrays. Although technically, it works as advertised, because the manual 
says that it will result in "undefined behaviour".

- Heikki

Heikki Linnakangas <hlinnakangas@vmware.com> writes:
> While looking at the memory leaks in ecpg that Coverity warned about and 
> Michael just fixed, I started wondering if the code is ever used.

Michael Meskes would be the authority on that question, so I've cc'd
him to make sure he notices this thread ...

> The leaks were in the code that takes a host variable, and converts it 
> into a string for sending to the server as a query parameter. In 
> particular, it was in code that deals with arrays. However, the fine 
> manual says:

>> SQL-level arrays are not directly supported in ECPG. It is not
>> possible to simply map an SQL array into a C array host variable.
>> This will result in undefined behavior. Some workarounds exist,
>> however.

> That very clearly says that the code that was fixed is not actually 
> supported.

It seems quite possible to me that this manual text is out of date.

> We do have a test case, 'arrays', that tests the code, but it only tests 
> integer arrays. The leaks were in 'interval', 'timestamp', and 'numeric' 
> arrays. And it turns out that there are two bugs there:

> 1. In timestamp, interval, and date, the array offset is calculated 
> incorrectly:

> str = quote_postgres(PGTYPESinterval_to_asc((interval *) ((var + 
> var->offset * element)->value)), quote, lineno);

> That "var + var->offset * element" has C datatype "struct variable *", 
> not "char *" as the code assumes, so the calculated offset is wrong, 
> leading to bogus parameters or segfault

> 2. The constructed array looks like this (for timestamp):

> array [2005-06-23 11:22:33,2004-06-23 11:22:33]

> That's bogus. It's sent as a query parameter, not embedded into an SQL 
> string, so the syntax should be '{...}'.

> It would be nice to fix these, and add a test case to cover all kinds of 
> arrays. Although technically, it works as advertised, because the manual 
> says that it will result in "undefined behaviour".
        regards, tom lane

On Wed, Feb 04, 2015 at 10:07:07AM -0500, Tom Lane wrote:
> Michael Meskes would be the authority on that question, so I've cc'd
> him to make sure he notices this thread ...

Thanks Tom.

> > The leaks were in the code that takes a host variable, and converts it 
> > into a string for sending to the server as a query parameter. In 
> > particular, it was in code that deals with arrays. However, the fine 
> > manual says:
> 
> >> SQL-level arrays are not directly supported in ECPG. It is not
> >> possible to simply map an SQL array into a C array host variable.
> >> This will result in undefined behavior. Some workarounds exist,
> >> however.
> 
> > That very clearly says that the code that was fixed is not actually 
> > supported.
> 
> It seems quite possible to me that this manual text is out of date.

It is, at least partly. 

> > 1. In timestamp, interval, and date, the array offset is calculated 
> > incorrectly:

Same for numeric it seems.

> > 2. The constructed array looks like this (for timestamp):
> 
> > array [2005-06-23 11:22:33,2004-06-23 11:22:33]
> 
> > That's bogus. It's sent as a query parameter, not embedded into an SQL 
> > string, so the syntax should be '{...}'.

Right. Now I can imagine that this is due to changes over the years. What worries my, though, is that this was fixed
forintegers, but only for integers, not even for short or long variants thereof. No idea how that happened.
 

> > It would be nice to fix these, and add a test case to cover all kinds of 
> > arrays. Although technically, it works as advertised, because the manual 
> > says that it will result in "undefined behaviour".

:)

At the very least it should cover the different types that actually get code copied and pasted.

I'm on it as my time permits.

Michael
-- 
Michael Meskes
Michael at Fam-Meskes dot De, Michael at Meskes dot (De|Com|Net|Org)
Michael at BorussiaFan dot De, Meskes at (Debian|Postgresql) dot Org
Jabber: michael.meskes at gmail dot com
VfL Borussia! Força Barça! Go SF 49ers! Use Debian GNU/Linux, PostgreSQL