Excellent, Glen!
Thanks for having a look at this.

I have a question: This parameter substitution method seems more and more to me like a mini syntactic sql parser, is that right?

One suggestion:

I think you could simplify your tests by using TestCase attributes: http://nunit.org/index.php?p=testCase&r=2.5

Your test could be something like:

[Test]
[TestCase(@"SELECT 'b''la', @p")]
[TestCase(@"SELECT 1 WHERE ''= 'type(''m.response'')#''O''%' AND :p")]
[TestCase(@"SELECT '''a' || :p")]
public void Excecute_Bugs_240_and_296_query(string query)
{
    using (var cmd = Conn.CreateCommand())
    {
    cmd.CommandText = query;

    cmd.Parameters.AddWithValue("p", DBNull.Value);

    // syntax error at or near ":"
    cmd.ExecuteReader().Dispose();
    }
}

Nunit would take care of calling your test method with the provided values as parameter.

I learned this awesome trick from Shay's test InsertValue : https://github.com/npgsql/Npgsql/blob/master/tests/CommandTests.cs#L1383

I hope it helps.

Yes. Unfortunately. That is the reality of doing client side parameter
substitution. I've put some thought into this, as the last thing I want to
do is implement an SQL parser in Npgsql. But, that train has sailed, so to
speak. I think ideally we would do all parameter substitution at the
backend, and therefore only with prepared statements. But there's a lot of
history in Npgsql with doing client side subs, and removing that
functionality would be a rather tragic breaking change for a lot of people.

On Sat, Aug 2, 2014 at 10:15 PM, Francisco Figueiredo Jr. <
notifications@github.com> wrote:

Excellent, Glen!
Thanks for having a look at this.

I have a question: This parameter substitution method seems more and more
to me like a mini syntactic sql parser, is that right?

—
Reply to this email directly or view it on GitHub
#307 (comment).

Oh, that is interesting, and I had no idea. I'll look at using that
tomorrow morning.

-Glen

On Sat, Aug 2, 2014 at 10:29 PM, Francisco Figueiredo Jr. <
notifications@github.com> wrote:

One suggestion:

I think you could simplify your tests by using TestCase attributes:
http://nunit.org/index.php?p=testCase&r=2.5

Your test could be something like:

[Test]
[TestCase(@"SELECT 'b''la', @p")]
[TestCase(@"SELECT 1 WHERE ''= 'type(''m.response'')#''O''%' AND :p")]
[TestCase(@"SELECT '''a' || :p")]
public void Excecute_Bugs_240_and_296_query(string query)
{
using (var cmd = Conn.CreateCommand())
{
cmd.CommandText = query;

cmd.Parameters.AddWithValue("p", DBNull.Value);

// syntax error at or near ":"
cmd.ExecuteReader().Dispose();
}

}

Nunit would take care of calling your test method with the provided values
as parameter.

I learned this awesome trick from Shay's test InsertValue :
https://github.com/npgsql/Npgsql/blob/master/tests/CommandTests.cs#L1383

I hope it helps.

—
Reply to this email directly or view it on GitHub
#307 (comment).

Yes. Unfortunately. That is the reality of doing client side parameter substitution. I've put some thought into this, as the last thing I want to do is implement an SQL parser in Npgsql. But, that train has sailed, so to speak. I think ideally we would do all parameter substitution at the backend, and therefore only with prepared statements. But there's a lot of history in Npgsql with doing client side subs, and removing that functionality would be a rather tragic breaking change for a lot of people.

I see. I asked that because I was a little worried about its complexity :)

Regarding the all parameter substitution at the backend, I think this wouldn't be possible as we have to send at least the query with the parameter markers $1, $2 etc.

Oh, that is interesting, and I had no idea. I'll look at using that tomorrow morning.

Yes. It is very nice and very powerful! Based on that, I also used the Values attribute http://nunit.org/index.php?p=values&r=2.5 to handle preparing inside the tests. Check the tests in #257

On Sat, Aug 2, 2014 at 10:42 PM, Francisco Figueiredo Jr. <
notifications@github.com> wrote:

Yes. Unfortunately. That is the reality of doing client side parameter
substitution. I've put some thought into this, as the last thing I want to
do is implement an SQL parser in Npgsql. But, that train has sailed, so to
speak. I think ideally we would do all parameter substitution at the
backend, and therefore only with prepared statements. But there's a lot of
history in Npgsql with doing client side subs, and removing that
functionality would be a rather tragic breaking change for a lot of people.

I see. I asked that because I was a little worried about its complexity :)

Regarding the all parameter substitution at the backend, I think this
wouldn't be possible as we have to send at least the query with the
parameter markers $1, $2 etc.

Yep, that's a big part of the breaking change. The actual syntax would
change, in addition to forcing a Prepare() for all queries with parameters.

-Glen

—

Reply to this email directly or view it on GitHub
#307 (comment).

Oh wow, I really was out of my depth. I didn't even really internalize the fact that we do parameter parsing in Npgsql itself... Thanks for all the explanations. It's obviously clear this isn't a discusson for the upcoming 2.2 beta, but it's worth having for the version after that.

Here are some quick thoughts...

• The client-side parameter feature is intended to provide SQL injection protection without the cost associated with prepared statements (extra roundtrips), right? Otherwise there's no reason not to just use a prepared statement?
• Is client-side parameter handling part of ADO.NET, is it standard in any way, or is it an Npgsql feature? Some quick searches in MSDN appear to always talk about parameters in the context of prepared statements... I can also see some examples with parameters that don't explicitly invoke Prepare(), does that mean prepare is done implicitly? Or that client-side sub is performed like in Npgsql?
• I totally agree with @glenebob that this is ideally not something we should be doing in Npgsql... Now that I understand I actually find @glenebob's work on this that much more impressive.
• Are we sure that this will break compatibility in a major way? If ADO.NET allows for implicit Prepare() (as some examples seem to suggest), then can't we silently switch the internal implementation to always prepare as well, rather than do client-side sub?
• One last thought: we were talking about the possibility of the next version (after 2.2) being called 3.0, mainly because of dropping the old .NET versions. I also have some significant refactors in mind for async and otherwise. This could also be a good opportunity to drop support for this feature, assuming we don't believe in it - a super-major version like 3.0 would be the right place to do this kind of breakage.

On Sun, Aug 3, 2014 at 12:25 AM, Shay Rojansky notifications@github.com
wrote:

Oh wow, I really was out of my depth. I didn't even really internalize the
fact that we do parameter parsing in Npgsql itself... Thanks for all the
explanations. It's obviously clear this isn't a discusson for the upcoming
2.2 beta, but it's worth having for the version after that.

Well, but let's be clear. This is a bug fix (and a fairly simple one), and
that part of it is most certainly a discussion for 2.2.

Here are some quick thoughts...

• The client-side parameter feature is intended to provide SQL
  injection protection without the cost associated with prepared statements
  (extra roundtrips), right? Otherwise there's no reason not to just use a
  prepared statement?

Francisco will have to answer this with authority, since client side
parameter sub was a feature in Npgsql long before I ever came along (I just
optimized it... by rewriting it entirely lol). But, I believe it was a
clone of sorts of what the ODBC driver does. I say that because the syntax
seems to match, and I seem to remember that ODBC supports client side
parameters.

• Is client-side parameter handling part of ADO.NET, is it standard in
  any way, or is it an Npgsql feature? Some quick searches in MSDN appear to
  always talk about parameters in the context of prepared statements... I can
  also see some examples with parameters that don't explicitly invoke
  Prepare(), does that mean prepare is done implicitly? Or that client-side
  sub is performed like in Npgsql?

I don't have answers here, except that I don't think implicit prepare is a
fully acceptable thing to do.

• I totally agree with @glenebob https://github.com/glenebob that
  this is ideally not something we should be doing in Npgsql... Now that I
  understand I actually find @glenebob https://github.com/glenebob's
  work on this that much more impressive.

Thank you sir :)

• Are we sure that this will break compatibility in a major way? If
  ADO.NET allows for implicit Prepare() (as some examples seem to
  suggest), then can't we silently switch the internal implementation to
  always prepare as well, rather than do client-side sub?

Implicit prepare is probly a bad thing, and the syntax changes. If we
swizzle the syntax at execute time (:param _name to $ordinal, or whatever
it actually is), then we save nothing. The poor man's parser is still
needed with little change from its current complexity.

• One last thought: we were talking about the possibility of the next
  version (after 2.2) being called 3.0, mainly because of dropping the old
  .NET versions. I also have some significant refactors in mind for async and
  otherwise. This could also be a good opportunity to drop support for this
  feature, assuming we don't believe in it - a super-major version like 3.0
  would be the right place to do this kind of breakage.

I agree here. But, I think we would need to offer a fairly lengthy life
span of a pre-3.0 version that still supports the old syntax and
non-prepared, client side parameter sub. I cannot believe that use of the
existing client-side parameter substitution support is minimal. I expect
heavy breakage if we go this route.

.
-Glen

—
Reply to this email directly or view it on GitHub
#307 (comment).

@glenebob, I pretty much agree with everything you say. And of course your bugfix for 2.2 has nothing to do with this discussion.

There's one possible good outcome here. If:

1. Client-side sub is a non-standard feature (i.e. it makes sense to get rid of it), and
2. It's acceptable in ADO.NET to do implicit prepare (when parameters are defined), and
3. There's no parameter format mismatch crap

Then we can silently get rid of the client-side sub under the hood and nobody will be the wiser. But I'm really not sure about any of the above, of course.

I suggest we do our research, IMHO this is a pretty important subject. I've opened #309 to continue the conversation on this, in the meantime I suggest we merge this and go ahead with 2.2 ASAP.

Note that the build server shows test ConnectionStringWithInvalidParameterValue failing: http://build.npgsql.org/viewLog.html?buildId=1790&tab=buildResultsDiv&buildTypeId=npgsql_net45

On Sun, Aug 3, 2014 at 1:49 AM, Shay Rojansky notifications@github.com
wrote:

@glenebob https://github.com/glenebob, I pretty much agree with
everything you say. And of course your bugfix for 2.2 has nothing to do
with this discussion.

There's one possible good outcome here. If:

1. Client-side sub is a non-standard feature (i.e. it makes sense to
   get rid of it), and
2. It's acceptable in ADO.NET to do implicit prepare (when parameters
   are defined), and

Not sure on these.

1. There's no parameter format mismatch crap

But this is a big problem. Npgsql parameters are named. PG parameters are ordinal.

-Glen

@glenebob I agree it doesn't look good.

Let's do some research on this and see what other ADO.NET providers are doing. I'll look at this report what I find in #309.

Francisco will have to answer this with authority, since client side parameter sub was a feature in Npgsql long before I ever came along (I just optimized it... by rewriting it entirely lol). But, I believe it was a clone of sorts of what the ODBC driver does. I say that because the syntax seems to match, and I seem to remember that ODBC supports client side parameters.

I think the idea of Prepare method in the ADO.Net api was to provide a way for the provider to optimize the query processing. Be it using server side support or doing some client side optimization. To me, when the developer decided to call Prepare() it was signalizing to the provider that she would reuse that query and would just change the parameters. So, the provider could make some type of caching or something like that.

I remember I've seen a provider which had a connection string called preparebydefault, which would make all the commands be prepared by default.

I think that the fact this isn't on by default is because Prepare()'ing a query for reuse could mean more resources being used whose costs would be explained by the reutilization of the query. I think that in the case of Npgsql and postgresql, doing a single query in the simple query path of the protocol has a better performance than doing this same single query in the extended query path.

s client-side parameter handling part of ADO.NET, is it standard in any way, or is it an Npgsql feature? Some quick searches in MSDN appear to always talk about parameters in the context of prepared statements... I can also see some examples with parameters that don't explicitly invoke Prepare(), does that mean prepare is done implicitly? Or that client-side sub is performed like in Npgsql?

In the case of Sql server, when the query is prepared, the provider use sql server support for prepred statements through sp_prepare http://technet.microsoft.com/en-us/library/ff946077%28v=sql.100%29.aspx and sp_execute http://technet.microsoft.com/pt-br/library/ff946038%28v=sql.100%29.aspx

When the developer doesn't call Prepare, I think sqlclient does client side parameter subsitution like Npgsql.

I agree here. But, I think we would need to offer a fairly lengthy life span of a pre-3.0 version that still supports the old syntax and non-prepared, client side parameter sub. I cannot believe that use of the existing client-side parameter substitution support is minimal. I expect heavy breakage if we go this route.

Agreed. I think we need a thoroughly analysis of prepared statements only and its impact (performance and support wise) if we decide to change.

I suggest we do our research, IMHO this is a pretty important subject. I've opened #309 to continue the conversation on this, in the meantime I suggest we merge this and go ahead with 2.2 ASAP.

Agreed. Thanks for opening #309 .

Note that the build server shows test ConnectionStringWithInvalidParameterValue failing: http://build.npgsql.org/viewLog.html?buildId=1790&tab=buildResultsDiv&buildTypeId=npgsql_net45

I noticed that. The last time this error pop up was when we enabled logging. I couldn't reproduce it in my dev machine at the time.
But wait, didn't we disable Npgsql logging? Why is it appearing here again?

But wait, didn't we disable Npgsql logging? Why is it appearing here again?

I just started a build with the "clean all files in the checkout directory before the build" option checked.
Sometimes in the past this cleaned up some cached assemblies.

I looked at doing this, but with all comments explaining the different
queries and where they break, I decided to just leave it as is.

-Glen

On Sat, Aug 2, 2014 at 10:29 PM, Francisco Figueiredo Jr. <
notifications@github.com> wrote:

One suggestion:

I think you could simplify your tests by using TestCase attributes:
http://nunit.org/index.php?p=testCase&r=2.5

Your test could be something like:

[Test]
[TestCase(@"SELECT 'b''la', @p")]
[TestCase(@"SELECT 1 WHERE ''= 'type(''m.response'')#''O''%' AND :p")]
[TestCase(@"SELECT '''a' || :p")]
public void Excecute_Bugs_240_and_296_query(string query)
{
using (var cmd = Conn.CreateCommand())
{
cmd.CommandText = query;

cmd.Parameters.AddWithValue("p", DBNull.Value);

// syntax error at or near ":"
cmd.ExecuteReader().Dispose();
}

}

Nunit would take care of calling your test method with the provided values
as parameter.

I learned this awesome trick from Shay's test InsertValue :
https://github.com/npgsql/Npgsql/blob/master/tests/CommandTests.cs#L1383

I hope it helps.

—
Reply to this email directly or view it on GitHub
#307 (comment).

Guys, this is my bad - I left two statements to turn on logging in the unit test.

@glenebob, just remove the two offending lines here and this PR should pass all tests: https://github.com/glenebob/Npgsql2/commit/d45f620955d02bd04b263d04cfa77db53fbeec0d#diff-d00d89f70ac53244e7449ab9e43ec7c2R3938

Done, Take a look please :)

-Glen

On Mon, Aug 4, 2014 at 8:03 AM, Shay Rojansky notifications@github.com
wrote:

Guys, this is my bad - I left two statements to turn on logging in the
unit test.

@glenebob https://github.com/glenebob, just remove the two offending
lines here and this PR should pass all tests: glenebob@d45f620
#diff-d00d89f70ac53244e7449ab9e43ec7c2R3938
https://github.com/glenebob/Npgsql2/commit/d45f620955d02bd04b263d04cfa77db53fbeec0d#diff-d00d89f70ac53244e7449ab9e43ec7c2R3938

—
Reply to this email directly or view it on GitHub
#307 (comment).

Just look at that nice green checkmark from the build server... Merge away!

I looked at doing this, but with all comments explaining the different queries and where they break, I decided to just leave it as is.

Ahh, ok! You are right. It wouldn't be worthwhile. Thanks for having a look at it.