Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

tlp: investigate whether TLP with prepared statements can catch #71002 #71530

Closed
mgartner opened this issue Oct 13, 2021 · 2 comments
Closed
Assignees
Labels
C-enhancement Solution expected to add code/behavior + preserve backward-compat (pg compat issues are exception) T-sql-queries SQL Queries Team

Comments

@mgartner
Copy link
Collaborator

Once #71323 is merged, TLP should be able to catch the bug described in #71002. We should test this assumption. If TLP fails to catch it, we should figure out why and address the issue.

@nehageorge attempted this already without success so far:

Tried it three times so far and no luck yet :( ... I think we may have to run it many many times since getBoolExprContext most often returns true, false, or NULL, and we can't add placeholders to those expressions.

Perhaps we need to change the random predicate generated so that it returns true, false, and NULL less often.

@mgartner mgartner added the C-enhancement Solution expected to add code/behavior + preserve backward-compat (pg compat issues are exception) label Oct 13, 2021
@blathers-crl blathers-crl bot added the T-sql-queries SQL Queries Team label Oct 13, 2021
@nehageorge nehageorge self-assigned this Oct 13, 2021
@nehageorge
Copy link

Looked into this. I was able to get TLP to reproduce the known issue locally. However, I had to make my own custom function that builds "compound" comparator expressions with placeholders. By compound comparator expressions, I mean conjunctions and disjunctions of expressions like "col1 = $1"

With the existing TLP implementation, like I hinted at earlier, the probability of creating a statement with multiple "datum" expressions is very slim, which is fine for non-prepared statements. However, this creates an issue for testing prepared statements as we end up having many statements that don't use placeholders at all.

My proposed solution is to redefine makeBoolExprWithPlaceholders to implement what I have implemented locally. This will ensure that many valid prepared statements with placeholders are being tested. Since makeBoolExpr ensures test coverage for statements that are not just comparator conjuncts and disjuncts, I don't think that this proposes a grave test coverage issue.

My focus might be skewed by knowing the specifics of where issue #71002 was seen. I'd love to hear others' thoughts on this, especially regarding the test coverage of my proposed approach. Do you think we need to be testing other types of prepared statements?

@mgartner

@nehageorge
Copy link

Talked to @mgartner async. Closing this issue for now as we've proved that we could have caught #71002 with TLP. However, a general issue with using sqlsmith infrastructure for TLP testing is the fact that makeBoolExpr so often generates true, false, and NULL as predicates. We should instead test more meaningful and less redundant predicates. See #71675.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-enhancement Solution expected to add code/behavior + preserve backward-compat (pg compat issues are exception) T-sql-queries SQL Queries Team
Projects
Archived in project
Development

No branches or pull requests

2 participants