Presto: Watchdog for queries using system.jdbc.*

[amiorin]

At Zalando, we are also having harmful queries generated by SQL clients during auto-completion and catalog browsing (see Lyft article). It would be useful to have a watchdog that kills these queries after a time-out or a regex.

Examples

-- this query doesn't make sense at all
-- triggered after some random CTRL+SPACE hits
SELECT TABLE_CAT, TABLE_SCHEM, TABLE_NAME, TABLE_TYPE, REMARKS,
  TYPE_CAT, TYPE_SCHEM, TYPE_NAME,   SELF_REFERENCING_COL_NAME, REF_GENERATION
FROM system.jdbc.tables
WHERE TABLE_CAT = 'hive' AND TABLE_NAME LIKE 'select' ESCAPE '\'
ORDER BY TABLE_TYPE, TABLE_CAT, TABLE_SCHEM, TABLE_NAME

-- missing escaping of _ in the where condition
-- 5 hours !?!
SELECT TABLE_CAT, TABLE_SCHEM, TABLE_NAME, COLUMN_NAME, DATA_TYPE,
  TYPE_NAME, COLUMN_SIZE, BUFFER_LENGTH, DECIMAL_DIGITS, NUM_PREC_RADIX,
  NULLABLE, REMARKS, COLUMN_DEF, SQL_DATA_TYPE, SQL_DATETIME_SUB,
  CHAR_OCTET_LENGTH, ORDINAL_POSITION, IS_NULLABLE,
  SCOPE_CATALOG, SCOPE_SCHEMA, SCOPE_TABLE,
  SOURCE_DATA_TYPE, IS_AUTOINCREMENT, IS_GENERATEDCOLUMN
FROM system.jdbc.columns
WHERE TABLE_CAT = 'hive' AND TABLE_SCHEM LIKE 'event_insider' ESCAPE '\' AND TABLE_NAME LIKE 'borg_competitive_data' ESCAPE '\' AND COLUMN_NAME LIKE '%' ESCAPE '\'
ORDER BY TABLE_CAT, TABLE_SCHEM, TABLE_NAME, ORDINAL_POSITION

-- missing escaping of _ in the where condition
-- 1 hour
SELECT TABLE_CAT, TABLE_SCHEM, TABLE_NAME, TABLE_TYPE, REMARKS,
  TYPE_CAT, TYPE_SCHEM, TYPE_NAME,   SELF_REFERENCING_COL_NAME, REF_GENERATION
FROM system.jdbc.tables
WHERE TABLE_CAT = 'hive' AND TABLE_SCHEM LIKE 'argo_confidential' ESCAPE '\' AND TABLE_NAME LIKE '%' ESCAPE '\' AND TABLE_TYPE IN ('TABLE', 'VIEW')
ORDER BY TABLE_TYPE, TABLE_CAT, TABLE_SCHEM, TABLE_NAME

[guyco33]

Maybe it will be nicer to have a system / session parameter toggle that will automatically escape _ values for such cases.
replacing LIKE 'borg_competitive_data' ESCAPE '\'
with LIKE 'borg\_competitive\_data' ESCAPE '\'

[shubhamtagra]

You can use event-listener for this purpose. We created one to kill some rogue queries that used to scan information_schema.columns multiple times using UNION ALL: https://github.com/stagraqubole/infoschemaquerykiller.
This is against older version of Presto but the idea still holds and you can create one for your purpose on similar lines.

[amiorin]

We will implement the event-listener. Thx @stagraqubole.

[electrum]

There is no need to have a watchdog or an event listener. Presto has built-in support for query time limits. The query_max_run_time session property simply needs to be set appropriately for these queries. However, we need some way to set this property automatically for such queries. SessionPropertyConfigurationManager is the obvious choice, but doesn't work because it happens before analysis.

Note that the provided event listener will not work in the latest version of Presto, as exceptions thrown from the EventListener methods will simply be logged as warnings. The previous behavior of failing the query was a bug.

[amiorin]

Thx David! Does it make sense to add queryMaxRunTime to the Resource Group Properties?

[electrum]

Resource groups only control query ingest / queuing. Session properties can be set by a session property manager which has the same matching power as a resource group (and can actually match based on the resource group, so you don't need to repeat the matching rule).

[tooptoop4]

@electrum do u know the commit that stopped allowing eventlistener to fail queries?

[electrum]

https://github.com/prestosql/presto/commit/72a7511a5353cf995d34d6571862f065260b9dfe

[tooptoop4]

https://eng.lyft.com/presto-infrastructure-at-lyft-b10adb9db01 mentions "We use EventListener to intercept when new query lands and we block a few types of queries that we deem harmful for system (eg. some tools actively cache column names and query system.jdbc.columns or catalog.information_schema.columns and cause additional overload on the system)."

in prestosql <= 331 you can put logic in an event listener to throw an error like https://github.com/stagraqubole/infoschemaquerykiller

slack alerts:
https://github.com/thecubed/prestowatcher

[guyco33]

FYI, Jetbrains DataGrip fixed those bad queries. See https://youtrack.jetbrains.com/issue/DBE-10319

[amiorin]

As in #3833, we are now evaluating to write an "admission controller" (the name comes from K8s) inside our Presto proxy. It will parse the query and reject queries that are not compliant. We don't need this PR anymore. Thank you for the review.