Skip to content

Conversation

@fulghum
Copy link
Contributor

@fulghum fulghum commented Oct 23, 2024

Implementations of db.Ping(ctx) from Go's database/sql package typically send some sort of empty query to test the liveness of a database connection. For example, the pq library sends ; and the pgx library sends -- ping.

Before this change, Doltgres' parser would return an empty statement, instead of Vitess' ErrEmpty error, which caused GMS to try and execute a nil analyzed plan for these statements, return an error, and the ping check would fail. This change makes the Doltgres parser return the same Vitess ErrEmpty error that the Vitess parser throws, sending the same signal to GMS to handle an empty statement without returning an error to the client. The related PR dolthub/go-mysql-server#2716 updates the Parser interface documentation to explicitly call out the requirement to return ErrEmpty in order for empty statements to be handled correctly.

For testing, I've added some no-op statements to the smoke tests, but since these tests go through as prepared statements, they don't follow the exact code path as db.Ping(ctx), so I've also added calls to db.Ping(ctx) in the test framework, which do reproduce this error. I've also added a unit test for Doltgres' Parser implementation that explicitly tests empty statement parsing.

Fixes: #884
Related to: dolthub/go-mysql-server#2716

@github-actions
Copy link
Contributor

github-actions bot commented Oct 24, 2024

Main PR
Total 42090 42090
Successful 12867 12876
Failures 29223 29214
Partial Successes1 4892 4892
Main PR
Successful 30.5702% 30.5916%
Failures 69.4298% 69.4084%

${\color{lightgreen}Progressions}$

comments

QUERY: /* and this is the end of the file */

insert_conflict

QUERY: ;
QUERY: ;
QUERY: ;
QUERY: ;

portals

QUERY: CLOSE ALL;
QUERY: CLOSE ALL;

Footnotes

  1. These are tests that we're marking as Successful, however they do not match the expected output in some way. This is due to small differences, such as different wording on the error messages, or the column names being incorrect while the data itself is correct.

@fulghum fulghum marked this pull request as draft October 24, 2024 00:26
@fulghum fulghum marked this pull request as ready for review October 24, 2024 20:06
@fulghum fulghum requested a review from jennifersp October 24, 2024 20:13
Copy link
Contributor

@jennifersp jennifersp left a comment

Choose a reason for hiding this comment

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

LGTM! 👏

@fulghum fulghum merged commit b5b0193 into main Oct 24, 2024
13 checks passed
@fulghum fulghum deleted the fulghum/ping branch October 24, 2024 21:10
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.

db.Ping results in error

3 participants