4

Someone told me that using text input for column names and formatting it, like I do below, is rarely a good idea. When I asked why, however, an answer wasn't given. That was on the postgresql IRC, and those guys seem to know their stuff. So I'd like to know why is it not advised ? I'm mostly wondering if it opens the door for sql injection.

create or replace function getItemsOrderBy(order_by_p text)
RETURNS TABLE (id int) AS $$


BEGIN

    return query EXECUTE format('
    SELECT id
    FROM items 
    ORDER BY %s', order_by_p) ;

END;

He also said to use execute with using instead, so what's the difference between this:

return query EXECUTE format('
SELECT id
FROM items 
ORDER BY %s', order_by_p) ;

and this :

return query EXECUTE '
SELECT id
FROM items 
ORDER BY $1' USING order_by_p ;

My function is more complex than what is above - the format part is only part of it. I have a choice to either create one function that can deal with multiple cases (for ordering) or create a bunch of them to deal with every ordering. I felt like doing only one was more practical. Having no function at all isn't an option.

I am actually using pg-promise but I was under the impression that since I'm doing a lot of back and forth between the back end and the DB (send something, wait response, compute something else, send again..) I should go with function and let everything happen all at once.

  • If possible, have pg-promise generate one big query that does it in one trip. If not possible, take solace knowing that it's IO-bound you'll have multiple queries going on at the same time fairly easily. I can't see your exact work load in the question. Feel free to ask for a review of it on Database Administrators. – Evan Carroll May 17 '17 at 23:43
2

There are two questions here,

  1. What is the difference between EXECUTE .. USING and EXECUTE FORMAT()
  2. Why is wrapping or generating simple SQL statements in a procedural function a bad idea?

The difference between EXECUTE .. USING and EXECUTE FORMAT()

From the docs,

The command string can use parameter values, which are referenced in the command as $1, $2, etc. These symbols refer to values supplied in the USING clause. This method is often preferable to inserting data values into the command string as text: it avoids run-time overhead of converting the values to text and back, and it is much less prone to SQL-injection attacks since there is no need for quoting or escaping. An example is:

EXECUTE 'SELECT count(*) FROM mytable WHERE inserted_by = $1 AND inserted <= $2'
  INTO c
  USING checked_user, checked_date;

Note that parameter symbols can only be used for data values — if you want to use dynamically determined table or column names, you must insert them into the command string textually. For example, if the preceding query needed to be done against a dynamically selected table, you could do this:

EXECUTE 'SELECT count(*) FROM '
    || quote_ident(tabname)
    || ' WHERE inserted_by = $1 AND inserted <= $2'
   INTO c
   USING checked_user, checked_date;

A cleaner approach is to use format()'s %I specification for table or column names (strings separated by a newline are concatenated):

EXECUTE format('SELECT count(*) FROM %I '
   'WHERE inserted_by = $1 AND inserted <= $2', tabname)
   INTO c
   USING checked_user, checked_date;

An EXECUTE with a simple constant command string and some USING parameters, as in the first example above, is functionally equivalent to just writing the command directly in PL/pgSQL and allowing replacement of PL/pgSQL variables to happen automatically. The important difference is that EXECUTE will re-plan the command on each execution, generating a plan that is specific to the current parameter values; whereas PL/pgSQL may otherwise create a generic plan and cache it for re-use. In situations where the best plan depends strongly on the parameter values, it can be helpful to use EXECUTE to positively ensure that a generic plan is not selected.

So you have a few arguments here.

  1. You can use both EXECUTE FORMAT() ... USING
  2. USING allows the plan to be cached.
  3. USING allows symbols to stay symbols and stops them from having to be converted to text and re-escaped.
  4. USING can not be used with identifiers, only literals.

Wrapping and generating simple SQL statements in a procedural function is a bad idea.

As for the other part of the question,

So I'd like to know why is it not advised? (Wrapping simple SQL statements in plpgsql.)

There are a lot of reasons for that,

  • functions as such obscure the costs (plural) to the planner, and require the user to explicitly
    1. set the execution cost (or use a rather silly estimate)
    2. 9.6+ establish if it is parallel-safe safe/restricted/unsafe
    3. 9.6+ establish if it has side effects strict/immutable/volatile
  • they obscure the internals to the user.
  • they prevent predicate-pushdown.
  • they complicate permissions (now you need access to the function too).
  • they raise the barrier for maintenance, now you have to define the result set the function returns TABLE (id int)
  • they present all kinds of problems with ORMs.

And, it's not SQL. You're building a new language on top of a DBMS. Why?

As for the dynamic component, there are other ways to engineer around the problem. Take for instance the exact statement provided, the worst case scenario is where ever you see that

SELECT * FROM getItemsOrderBy($col);

You have to explicitly write out the order-by. As bad as it, it's a better solution in my opinion.

SELECT id FROM items ORDER BY col1
SELECT id FROM items ORDER BY col2
SELECT id FROM items ORDER BY col3

A step even further would be to use a library which provides some kind of assistance for generating , like pg-promise

let args = { orderBy: "col1" };
if ( args.orderBy !== 'col1' ) {
  throw new Error "invalid orderBy Column";
}
db.manyOrNone( 'SELECT id FROM items ORDER BY ${orderBy~}', args );

Or, DBIx::Abstract, or an ORM like DBIx::Class.

  • If you're going to use format at least use the %I format-specifier instead of %s so you get identifier quoting. – Craig Ringer May 18 '17 at 2:22
  • @CraigRinger I was thinking about it, as a general rule, could we say that in the context of dynamic sql in plpgsql, to only use format with %I and never with %s and for the literals to use EXECUTE ... USING – Evan Carroll May 18 '17 at 2:30
  • 4
    Mostly. USING only works for plannable statements, so you need %L for literals in things like the DEFAULT clause of ALTER TABLE ... ADD COLUMN ... or adding constraints, that sort of thing. Another common one is COPY. And sometimes you want to deliberately compose a SQL statement in chunks, in which case you'd use %s. But for most cases EXECUTE format('... %I ... $1', identname) USING (param) is the desirable form. The docs should emphasise this more. – Craig Ringer May 18 '17 at 3:42
  • That's really well said, and it considers a case I hadn't have thought of. But, I do agree on the docs emphasizing that more. – Evan Carroll May 18 '17 at 4:52
2

Only addressing your second question about EXECUTE and USING:

The elephant in the room: the suggested alternative is utter nonsense for multiple reasons.

The USING clause of EXECUTE is used to pass values. Not literals, not identifiers, not other syntax elements, only values. The same values you can pass to prepared statements because, internally, that's exactly what happens. The statement is prepared an then executed using values provided by USING.

This is many-fold nonsense:

return query EXECUTE '
SELECT id
FROM items 
ORDER BY $1' USING order_by_p;
  1. ORDER BY does not make sense with a constant value, would be just noise.
  2. order_by_p is supposed to be a column name, which is supposed to be interpreted as identifier, not as data value. You never use USING for that. See above.
  3. If you concatenate identifiers into SQL strings, you must escape them to avoid SQL injection and other sneaky errors. With format() using the %I specifier or with quote_ident().

Would work like this:

CREATE OR REPLACE FUNCTION get_items_orderby(order_by_p text)
  RETURNS TABLE(id int) AS
$func$
BEGIN
   RETURN QUERY EXECUTE format('
   SELECT id
   FROM   items 
   ORDER  BY %I', order_by_p);
END
$func$ LANGUAGE plpgsql;

As long as you don't nest this function in outer queries (other than SELECT * FROM my_func()), and you don't need to optimize performance (like for many repeated calls in the same session), there is nothing wrong it.

Note that identifiers are case sensitive and you have to provide correct spelling. Related answers:

  • I'm not sure this warrants another question, but I suspect the answe is very short, if not I'll do a follow up: You hint that using EXECUTE with FORMAT prevents statements to be prepared. If I want to create a function that does what's in the snippet above (select an item and have a ordering as parameter) and I would like statements to be prepared; Would I have to create separate functions for each ordering ? It would be quite nasty to do and hard to maintain (have to rewrite the same thing in multiple place except for ordering). – Ced May 19 '17 at 1:16
  • @Ced: format() has nothing to do with it, that's just a string manipulation function. (The currently accepted answer is misleading there.) It's all about EXECUTE, which forces the SQL statement to be re-planned every time (may be a feature, see related answer I linked above). It's worth another question. There might be a solution giving you the best of both worlds. Provide all relevant information for PostgreSQL performance questions, and whether LIMIT is involved. – Erwin Brandstetter May 19 '17 at 12:38
  • 1
    The nonsense might be explained by how similar %1, %I and %i look in some fonts. – ypercubeᵀᴹ May 19 '17 at 13:43

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.