No warning on unsupported statements #621
Conversation
… toxiproxy (if one exists). This helps with re-running the tests, even if it fails in the middle (making the tests execution idempotent).
…d by `sqlparser`.
…ument to avoid doing a clean build.
mdashti
changed the title
…s on non-clean builds.
| @@ -10,7 +10,7 @@ set -o xtrace | |||
| # for inspection. | |||
| function start_pgcat() { | |||
| kill -s SIGINT $(pgrep pgcat) || true | |||
| RUST_LOG=${1} ./target/debug/pgcat .circleci/pgcat.toml & | |||
| ./target/debug/pgcat .circleci/pgcat.toml --log-level ${1} & | |||
There was a problem hiding this comment.
Without this change, the log-level is not respected.
| @@ -29,6 +29,9 @@ PGPASSWORD=sharding_user pgbench -h 127.0.0.1 -U sharding_user shard2 -i | |||
| LOG_LEVEL=error toxiproxy-server & | |||
| sleep 1 | |||
|
|
|||
| # Remove toxiproxy if it exists. | |||
| toxiproxy-cli delete postgres_replica || true | |||
There was a problem hiding this comment.
It is needed for local development, if you're going to run the tests multiple times.
| set -e | ||
|
|
||
| CLEAN_BUILD=true | ||
|
|
||
| if [ $1 = "no-clean" ]; then | ||
| echo "INFO: clean build is NOT going to be performed." | ||
| CLEAN_BUILD=false | ||
| find /app/target/debug/deps -name *.gcda -exec rm {} \; | ||
| fi | ||
|
|
||
| if $CLEAN_BUILD ; then | ||
| rm -rf /app/target/ || true | ||
| fi |
There was a problem hiding this comment.
This helps if you want to run the tests multiple times, without waiting for a clean build. You'd just pass no-clean as an argument.
| @@ -26,7 +26,7 @@ UPDATE pgbench_branches SET bbalance = bbalance + :delta WHERE bid = :bid; | |||
|
|
|||
| INSERT INTO pgbench_history (tid, bid, aid, delta, mtime) VALUES (:tid, :bid, :aid, :delta, CURRENT_TIMESTAMP); | |||
|
|
|||
| END; | |||
| COMMIT; | |||
There was a problem hiding this comment.
END is not supported by sqlparser and leads to a pgcatWARN.
|
@levkk Any comment? |
|
Why don't we just demote the warn to an info or debug? We don't need this additional machinery to hide a warning I don't think unless I'm missing some foundational change for a new feature you're thinking of adding? |
|
@levkk I'm OK with changing the |
I suspect the list of unsupported queries is not exhaustive and we'll end up maintaining our own list while sqlparser will continue to improve and eventually parse what we consider an unsupported query. I think the right thing to do here is to submit a patch for sqlparser with the fix. Considering we are looking at moving away from sqlparser, having an intermediate state just to hide a warning seems odd. I would rather we continue to log errors to let the user know this is an unsupported query instead of hiding it and having unexpected side effects. E.g. |
|
Now that I'm thinking more about this, I would rather we log a warning than nothing at all. This is an error condition that we're currently not handling well, so we need to either fix it or notify users that we aren't doing something they expect us to do. |
Currently, not all acceptable PostgreSQL queries are parsed correctly by
sqlparser. As a long-term direction, I think we should invest in making it possible. I created issue #620 to track that.However, for now, we can avoid producing warning messages on queries that we know will fail. This removes the warning message from the test outputs, as well.
In addition, this PR applies some improvements to the test script.