Just came across this gem. What's wrong with it??

Yes, the threat of SQL injection here is a legit response, but in this particular case that's not the answer I'm looking for.

Hint: This method gets called a lot!

That connection should be disposed.

Who doesn't enjoy running up against the max connection limits of the DB 🤷‍♂️

Let's just assume the SQL being dropped in was already prepared correctly ... right???

@C0D4 if this is inside some older home made ORM its not so strange.

You can build safe SQL without parameterized queries.

But you will risk loosing dome performance if the query is repeated with different parameters.

On the other hand, I have had queries where the parameterized version actually performed worse snd that is when the values of one param affects the best query plan.

By merging that param into the string you generate different query plans each optimized for that value.

Its an edge case but can have a huge impact if the number of values affected differ a lot depending on that parameter.

Why is the connection not pre-allocated? Establishing a new connection in a hot path burns precious milliseconds.

Also is it not being disposed? Dunno if this has garbage collection.

@Voxera

True, though this is by design.

A prepared statement usually leads to a rendering of one query execution plan on the server side, then whenever it is sent, the server does not recreate the query execution plan.

Behaviour can be changed depending on RDBMs, however: That is by design. Creating a query execution plan on the server side is time consuming, doing it per query inefficient.

That's exactly what prepared statements try to solve.

When the query plan goes awry depending on values, one should take a close look at why. Cause most of the time, this applies on a larger scale, whether prepared statement are used or not... nothing's less fun than a table where the query plan can become a random time bomb - either running fast or being abysmally slow.

That said, databases are tricky.

E.g. this example shows a lack of severe misunderstanding of how a database works.

Connection pooling is important, but even more important is to handle oopsies.

Reason why there are few connection pooling libraries out there: It's hard.

What @Voxera said could have another reason, though I'm thinking that that's not the case: Devs trying to be too smart for their own good.

Connection pooling usually leaves N connections open, but closes and reopens them at fixed intervals.

Reason is simple: a database connection represents a state.

A connection in database lingo is more an interactive session with specific behaviour etc. Closing the connection and reopening prevents leaks, cleans up temporary resources, etc.

Reason I'm not a fan of keeping connections open forever. Thread pooling should close and reopen connections at fixed intervals, to keep the database safe from resource leakage.

Does SqlConnection use a static pool and act as a singleton factory in its constructor?

Oh that looks like a nice way to reach max connections quickly.

@netikras if normally configured there is a connection pool behind that you should not need to handle, but you should have using or at least a close statement, the pool will under the hood decide to lose or just clear the connection.

And as to the sql as a string, my comment was just that there exists cases where it is the right way BUT it is the exception. Unless you KNOW why you need to use a string query you should use parameters always.

Even in the edge case you probably want parameters for most variable values.

Yep. The lack of a using/dispose is what put a spotlight on this piece of code because of the max connections exception that was thrown. Easy mistake to overlook.

The code above is in a one-time-use utility app that migrates data from one db schema to a refactored schema in another db which is why I wasn't concerned about the prepared statement that doesn't utilize parameterization.

Great discussion everybody, and thanks @Voxera for the interesting points you brought up

@IntrusionCM you had unsafe sex with Chatgpt? Nice info tho

What do you mean @retoor ?

@IntrusionCM just that's it a lot of text and a nice Summary

@retoor I'm sorry for being informative, I cannot help writing lots of texts.

*sad sobbing*

@Voxera didn't mean to be rude, of all the usual ranters here you're definitely one I respect and value your input.

:-) *have a 🥞*

@IntrusionCM no worries :)