0

MySQL Workbench is hitting me with the following error

The object's DDL statement contains syntax errors. You cannot modify this object until you fix the errors.

as well as the hint

"SELECT" is not valid at this position for this server version, expecting: '(', WITH

for the following code structure, where I'm trying to define a procedure that takes an argument called time_period which is used as SQL time keyword like MINUTE, HOUR, WEEK, etc, and a second argument period_count that will multiply that time period.

DELIMITER //
CREATE PROCEDURE getStuffFromPast(
  IN time_period VARCHAR(10),
  IN period_count INT(1)
)
BEGIN
  SELECT * 
  FROM table_A
  INNER JOIN 
    (SELECT x, y, record_date
     FROM table_B
     WHERE (record_date > (NOW() - INTERVAL period_count time_period))
    ) AS B
  ON table_A.x = B.x
    AND table_A.y = B.y;
END//
DELIIMTER ;

I know it's not a delimiter issue because if I alter the code to

DELIMITER //
CREATE PROCEDURE getStuffFromPast(
  IN time_period VARCHAR(10),
  IN period_count INT(1)
)
BEGIN
  SELECT * 
  FROM table_A
  INNER JOIN 
    (SELECT x, y, record_date
     FROM table_B
     WHERE (record_date > (NOW() - INTERVAL period_count HOUR))
    ) AS B
  ON table_A.x = B.x
    AND table_A.y = B.y;
  SELECT time_period;
END//
DELIIMTER ;

I get the correct first output followed by the correct second output. So I'm pretty sure my problem is syntactically using input parameters as keywords/data types in the procedure.

If there's a more elegant way to do what I want—call this procedure from python client like

def execute_sql(query):
   # Handles DB interactions

def get_data_from_past(time_period: str, count: int):
   sql_statement = f'call getStuffFromPast({time_period}, {count})'
   results = execute_sql(sql_statement)

Actually... as typing, I realized we could convert the time argument in the python method to the fundamental time unit we want to deal with in the stored procedure; but I'd still like to know if there's a way to do what I want, in the above stored procedure.

Vadim Kotov
  • 8,084
  • 8
  • 48
  • 62
Zduff
  • 171
  • 11
  • Using a stored procedure for what is effectively just a query is not always a great plan. Stored procedures are much harder to update than a query expressed in code. Is there any reason you *need* a stored procedure here? Is there anything about this query that couldn't be expressed as, say, a `VIEW` combined with a normal query? – tadman Dec 13 '19 at 19:18
  • That's a good point, a `VIEW` combined with a query seems simpler and more flexible so I may in fact go with that, thank you. The only concern I have is that I might want to control permissions on these types of queries in the future (not sure if that's necessary yet), and I see that stored procedures have that security feature; so can I control permissions similarly, limiting users to `VIEW`s and such? – Zduff Dec 13 '19 at 19:51
  • One of my further concerns is performance and indexing. Which is mentioned here, where @spencer7593 cautions agains the use of views in MySQL, granted that's 2012. https://stackoverflow.com/a/13945079/7082841 – Zduff Dec 13 '19 at 20:12
  • That's a separate issue. Running through a stored procedure does not make anything magically faster. You still need to index things properly. A `VIEW` is like a macro that expands to a regular query, no performance gain or loss, however materialized views, a thing in some RDBMS platforms (MySQL 8.0?) do make them way faster. – tadman Dec 13 '19 at 21:13
  • There are certain cases where a stored procedure improves performance. Not of an individual query, but of a series of queries. For example, if you have a multi-step process where the result of queries determines the next step, it can save some time if you don't have to transfer the results back and forth over the network between the database and the app. But I agree that stored procs are more trouble than they're worth in almost all cases. – Bill Karwin Dec 14 '19 at 20:28

1 Answers1

1

Your Query is essentially ok,but you have to use stmt

DELIMITER //
CREATE PROCEDURE getStuffFromPast(
  IN time_period VARCHAR(10),
  IN period_count INT(1)
)
BEGIN
  IF time_period NOT IN   ('MICROSECOND','SECOND','MINUTE','HOUR','DAY'
    ,'WEEK','MONTH','QUARTER','YEARSECOND_MICROSECOND','MINUTE_MICROSECOND'
    ,'MINUTE_SECOND','HOUR_MICROSECOND','HOUR_SECOND','HOUR_MINUTE'
    ,'DAY_MICROSECOND','DAY_SECOND','DAY_MINUTE','DAY_HOUR','YEAR_MONTH') THEN
    SIGNAL SQLSTATE '45000'
      SET MESSAGE_TEXT = 'Time period is not valid';
  END IF;
  SET @sql = 'SELECT * ';
  SET @sql = CONCAT(@sql,'FROM table_A ');
  SET @sql = CONCAT(@sql,'INNER JOIN ');
  SET @sql = CONCAT(@sql,'(SELECT x, y, record_date ');
  SET @sql = CONCAT(@sql,'FROM table_B ');
  SET @sql = CONCAT(@sql,'WHERE (record_date > (NOW() - INTERVAL ',period_count,' ',time_period,')) ');
  SET @sql = CONCAT(@sql,') AS B ');
  SET @sql = CONCAT(@sql,'ON table_A.x = B.x ');
  SET @sql = CONCAT(@sql,'AND table_A.y = B.y;');

  PREPARE stmt from @sql;
  EXECUTE stmt;
  DEALLOCATE PREPARE stmt;
END//
DELIMITER ;
nbk
  • 45,398
  • 8
  • 30
  • 47
  • This is unfortunately an SQL injection vulnerability. The `time_period` variable should be whitelisted to fix that. – Bill Karwin Dec 14 '19 at 20:30
  • I tried to use the placeholder ? unfortunately mysql doesn't allow it at that point to use it insteag of HOUR for example. I couldn't find a way to secure it- – nbk Dec 14 '19 at 22:35
  • 1
    Right, you can't use a parameter for an SQL keyword. Keywords must be in the string before you `PREPARE` the statement. But you can check the time_period variable against a whitelist and raise an error if it's not one of the allowed keywords like HOUR, YEAR, DAY, etc. Do you mind if I edit your answer to show you what I mean? – Bill Karwin Dec 14 '19 at 23:12
  • go ahead, but now i understand what you mean by white listeening. thx – nbk Dec 14 '19 at 23:14
  • I added the whitelist check above. There are [other valid time interval names](https://dev.mysql.com/doc/refman/8.0/en/expressions.html#temporal-intervals), but I kept the list brief. This is just to demonstrate the technique. – Bill Karwin Dec 14 '19 at 23:21
  • I added the rest, maybe it helps others. thank you very much – nbk Dec 15 '19 at 00:01