On 01/16/2015 09:21 PM, Faith, Jeremy wrote:
> Don't worry about the '-1' test I was just trying to determine, in the server side prepare case, if it was adding
quotesi.e. the string I was supplying already had single quotes and was not accepted.
>
> I've had a quick look at the new patches.
>
> patch 0001
> To be extra extra careful maybe valid_int_literal() should check 'len' before accessing the first byte and second
bytesof 'str' in case len is 0 or 1. This may not be necessary it depends on where 'str' comes from.
Ah, you're right. Fixed.
> Otherwise the patch looks fine but maybe you should add a couple of test cases for SQL_SMALLINT.
Added.
> patch 0002
> In convert.c you have added the comment and line
> /* XXX: should we use param_pgtype here instead? */
> *pgType = sqltype_to_bind_pgtype(conn, param_sqltype);
> this really requires someone with more in depth knowledge than I have.
I don't fully understand that either. It's not clear to me when
param_pgtype is set, and where the value comes from. I decided to ignore
it for now, which makes on which datatype is chosen simpler, and I think
is closer to the old behaviour.
> Again maybe a couple of test cases for SQL_SMALLINT.
Added.
> I have done a few very quick tests myself and have not found anything wrong so far.
>
> Looks like I opened up a can of worms here, oops, but the end result should be more secure and more consistent with
regardsto the server side and client side prepare behaviour, which is good. Also your helping people out by forcing
integercomparison more often.
> I will try to do some more testing next week.
Thanks, I've pushed these changes now.
- Heikki
