pgcat
pgcat copied to clipboard
postgresml
No warning on unsupported statements
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.
@levkk This PR is ready for code-review. The changes mainly help pgcat developers to avoid seeing a lot of (unnecessary) warnings in the test outputs. It's helped me find/fix a bug (in PR #600) already.
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 warning to debug (which is related to these lines: https://github.com/postgresml/pgcat/pull/621/files#diff-f078eca5d7bf721ab0c80995b0cbb209c87bfda478f20788f7289ae3b79cd8a4R393-R401), but the issue is still there that we will end up trying to parse an unsupported query (which sqlparser fails to parse) twice. The main change is to avoid parsing twice (if it fails).
the issue is still there that we will end up trying to parse an unsupported query
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. vacuum needs to run on the primary, while set can run on a replica, and the query router should be able to handle this or notify the user very loudly that something went wrong.
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.