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

Fix: no table selected #643

Closed
wants to merge 1 commit into from
Closed

Fix: no table selected #643

wants to merge 1 commit into from

Conversation

bajinsheng
Copy link
Collaborator

When no table is selected, the execution should be aborted to avoid execution errors in QPG.

@bajinsheng bajinsheng linked an issue Dec 16, 2022 that may be closed by this pull request
@bajinsheng bajinsheng enabled auto-merge (squash) December 16, 2022 10:07
@bajinsheng bajinsheng enabled auto-merge (squash) December 16, 2022 10:08
Copy link
Contributor

@mrigger mrigger left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rather than modifying the existing method, perhaps we could use a getRandomTableOrBailout method such as the one below that throws an exception when no table exists? In many contexts, it might be clear that a table exists, and not throwing an exception in such cases might help detect otherwise overlooked bugs (in SQLancer).

@bajinsheng
Copy link
Collaborator Author

I think it should be a bug as all dbs assume the returned list has at least one table and directly operate the object.
https://github.com/sqlancer/sqlancer/search?q=getRandomTable

@bajinsheng
Copy link
Collaborator Author

I cannot remember it exactly, but it should be some corner cases in which no table is returned, but there is no bug.

@bajinsheng
Copy link
Collaborator Author

Like this, it gets a random table and assumes the returned list is not empty:

sb.append(globalState.getSchema().getRandomTable(t -> !t.isView()).getName());

But its assumption is not always true. It is a potential bug that was not exposed before.

@bajinsheng
Copy link
Collaborator Author

We can close it as it is, and apply this fix when observing this issue again.

@bajinsheng bajinsheng closed this Dec 16, 2022
auto-merge was automatically disabled December 16, 2022 11:38

Pull request was closed

@mrigger
Copy link
Contributor

mrigger commented Dec 16, 2022

I think it should be a bug as all dbs assume the returned list has at least one table and directly operate the object.

Not sure if I would call this a bug - all implementations relying on this method indeed assume that at least one table exists, and I can't think of a circumstance where this doesn't hold. Did QPG result in a corner case where this assumption no longer holds? If so, perhaps we can rename getRandomTable to getRandomTableOrBailout to follow the naming convention in this class?

@mrigger
Copy link
Contributor

mrigger commented Dec 16, 2022

We can close it as it is, and apply this fix when observing this issue again.

Okay, sounds good.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support QPG
2 participants