Fix#95: support host and port arguments configuration for different DBMSs in SQLancer · Pull Request #315 · sqlancer/sqlancer · GitHub

What problem does this PR solve?

Issue number: #95

Summary

Supporting host/port arguments for the follow databases:

PostgreSQL
MySQL
MariaDB
TiDB
Clickhouse
CockRoachDB
Citus extends PostgreSQL

Release note

No release note

Thanks a lot for the PR. The changes look great overall.

I think that we need to expose the default port and host to the user in some way. See my thoughts in this respect as an in-line comment. Let me know if you have any follow-up questions or thoughts.

Thanks again for the great PR!

Why are these (and equivalent variables in other classes) fields and not local variables? They are only used in the createDatabase method, no?

I do not really understand. Could you expand on what issues there might be when using a local variable rather than a field?

Ideally, we would still use local variables here, but if you are reluctant to do this for this PR, I can also do this in a future follow-up PR.

As mentioned before, I think that this check makes it difficult to understand what's going on. I think that we want that the user has a clear understanding of the default values for the host and port. Currently, "sqlancer" is displayed as a default value for the host and port, but actually, these default values are overwritten by DBMS-specific ones that the user cannot see. I see two potential options to address this:

we put these options in a base option class that is used by all the DBMS-specific option classes and instantiated with suitable default values. This is the option that I further explained in my previous comment.
We set host and port to be null, and include the default values as part of the DBMS description in the options. For example, every provider implementation could implement a method int getDefaultPort() and String getDefaultHost() that is then used in the main class to print the default values. For example, for ClickHouse, the following would be printed:

  clickhouse      ClickHouse (default port: 8123, default host: localhost)
      Usage: clickhouse [options]
        Options:
        ...

Previously, you mentioned that the host and port can be null, but would this not result in an invalid URL?

This process actually involves two points:
(1) The user does not specify the host and port, then we need to replace it with the default value, which is different in different databases. This may be better set in xxOptions.java of each database.
(2) If the user sets one of host and port, we should use the value provided by the user to replace the default value of these systems. This should probably be set in xxProvider.java.

This may be modified in several places, refactoring several parts...（-:

Yes, there are multiple ways to implement this. It would be great to have this, so that the user knows about the default values.

Typo: this should be default.

Typo: this should be default.

I will modify it next time pull request.

Since these options are not supported by all DBMSs (yet): Would it be possible to insert a (static) check method that is called by every provider that does not support these? This is optional for this PR, but it might be a useful future improvement.

Since these options are not supported by all DBMSs (yet): Would it be possible to insert a (static) check method that is called by every provider that does not support these? This is optional for this PR, but it might be a useful future improvement.

We modify the description information to remind users that this value will be reset by the system. Is it feasible?

@Parameter(names = "--host", description = "The host used to log into the DBMS. If the user does not specify this value, it will be reset to localhost by the system")
    private String host = "sqlancer"; // NOPMD

@Parameter(names = "--host", description = "The host used to log into the DBMS. If the user does not specify this value, it will be reset to database system default port by the system")
    private String host = "sqlancer"; // NOPMD

In fact, another important function of placeholders is to facilitate us to determine whether the user has specified this parameter, so invalid values should be used as placeholders. The value sqlancer is a good choice. These two values are actually essentially different from username and password. The default of host is localhost. The default of port is database parameter.

what do you think? :) (-:

I do not yet understand yet why we need any placeholders in the options. None of the other options have deliberately invalid default values, I believe. If you want to indicate that no port or host have been set, should they be null?

I do not yet understand yet why we need any placeholders in the options. None of the other options have deliberately invalid default values, I believe. If you want to indicate that no port or host have been set, should they be null?

Yes, null is also possible. But according to the previous design, does the system still have to reset the host and port to the corresponding to localhost and the default port of database respectively? If you think null is more appropriate, I am willing to modify it. LMK, please.

Yes, I believe that either null, while also printing the default values when the help text is printed, or implementing them as DBMS-specific options (rather than in MainOptions) would be ways to achieve this.

TBR. Please let me know if this version can be merged? Thank you @mrigger

Sorry for the delayed response! As mentioned, it would be great to address the two concerns I mentioned: (1) we want to display the default values that each DBMS uses to the user as part of the help text and (2) not use invalid default values (i.e., either use null or implement them as DBMS-specific options). Please let me know if there are any unclarities, and I will provide more details.

TBR @mrigger
If there is anything inappropriate, please let me know!

Rather than hard-coding the port and localhost, could you please introduce constants for them that you use in both the description and when setting the values?

For example, in CockroachDBOptions you can have a field final static int DEFAULT_PORT = 26257; and reference it both here as "CockroachDB (default port: " + CockroachDBOptions.DEFAULT_PORT + ... and below in createDatabase. By doing so, the help text and actual default values will always be consistent.

Indeed, the current help text and default value can be consistent here.
Thank you for your suggestions.
TBR @mrigger

Sorry again for the late response - I had some full-day obligations in the last couple of days, and will be able to respond much faster in the future.

Why does using the constant in the annotation text not work?

Rather than checking for -1, could you please introduce a constant like NO_PORT_SET and use it also in the createDatabase methods?

Ideally, we could remove the commented-out code (but you don't need to do this for this PR).

TBR @mrigger
If there is anything inappropriate, please let me know!

Thanks a lot again for your changes and for your patience! After introducing constants and using them to replace the hard-coded values, we can likely merge the PR.

TBR @mrigger Thanks for your suggestions. Please review the latest revisions, and if necessary, please suggest revisions.

Thanks a lot for your PR and for incorporating my improvement suggestions. I highly appreciate it! Merging the PR now.

For future PRs, please pay attention to a "clean" commit history by avoid merges as well as rewriting the history for incorporating feedback (and removing accidentally committed files), see https://github.com/sqlancer/sqlancer/blob/master/CONTRIBUTING.md#commit-history.

Thanks a lot for your PR and for incorporating my improvement suggestions. I highly appreciate it! Merging the PR now.

For future PRs, please pay attention to a "clean" commit history by avoid merges as well as rewriting the history for incorporating feedback (and removing accidentally committed files), see https://github.com/sqlancer/sqlancer/blob/master/CONTRIBUTING.md#commit-history.

Thanks for your suggestions. It’s a great honor to contribute to this project.

abcdabcd3899 and others added 10 commits March 9, 2021 12:15

suuport host/port arguments for postgres

b7501c7

support host/port arguments for MySQL

5fac753

support host/port arguments for mariadb

96f22a0

support host/port arguments for clickhouse

3051257

support host/port arguments for cockroachdb

33b9ef9

support host/port arguments for TiDB

fceb0ac

format

decb8ff

modify format

4618937

Update TiDBProvider.java

1ca57bf

Update Main.java

7a3ebf4

mrigger reviewed Mar 16, 2021

View reviewed changes

Ethan-DB and others added 2 commits March 17, 2021 11:11

modify typo error

03f8d6f

using String.format replace + in String

17e79ae

abcdabcd3899 added 2 commits March 20, 2021 15:50

adding the help text and modifing the default values of host and port

4b45629

Merge remote-tracking branch 'upstream/master'

9a5370b

mrigger reviewed Mar 20, 2021

View reviewed changes

abcdabcd3899 added 2 commits March 20, 2021 21:24

constant representation for the default value of host and port

f201549

delete comments

dc2bc44

abcdabcd3899 added 5 commits March 25, 2021 10:18

Merge remote-tracking branch 'upstream/master'

ba7d856

add constant support for the different DBMSs

c601f4f

add constant support for the different DBMSs

ddd2d02

Merge branch 'master' of https://github.com/EthanDBer/sqlancer

b931c73

add const support for the different DBMSs and use local variables

c520480

Delete 1

318272f

mrigger merged commit 695dda5 into sqlancer:master Mar 25, 2021

                   public SQLConnection createDatabase(ClickHouseGlobalState globalState) throws SQLException {
+                      host = globalState.getOptions().getHost();
+                      port = globalState.getOptions().getPort();
+                      if ("sqlancer".equals(host)) {

                       globalState.setClickHouseOptions(clickHouseOptions);
-                      String url = "jdbc:clickhouse://localhost:8123/default";
+                      // String url = "jdbc:clickhouse://localhost:8123/default";
+                      String url = "jdbc:clickhouse://" + host + ":" + port + "/defult";

               import sqlancer.mysql.gen.tblmaintenance.MySQLRepair;
               public class MySQLProvider extends SQLProviderAdapter<MySQLGlobalState, MySQLOptions> {
+                  protected String username;

                   private String password = "sqlancer"; // NOPMD
+                  @Parameter(names = "--host", description = "The host used to log into the DBMS")
+                  private String host = "sqlancer"; // NOPMD

               import sqlancer.common.oracle.TestOracle;
-              @Parameters(separators = "=", commandDescription = "Test CockroachDB")
+              @Parameters(separators = "=", commandDescription = "CockroachDB (default port: 26257, default host: localhost)")

+                  private String host = null; // NOPMD
+                  @Parameter(names = "--port", description = "The port used to log into the DBMS")
+                  private int port = -1; // NOPMD

                       String databaseName = globalState.getDatabaseName();
-                      String url = "jdbc:mysql://127.0.0.1:4000/";
+                      // String url = "jdbc:mysql://127.0.0.1:4000/";

Fix#95: support host and port arguments configuration for different DBMSs in SQLancer · Pull Request #315 · sqlancer/sqlancer · GitHub

Uh oh!

Fix#95: support host and port arguments configuration for different DBMSs in SQLancer - #315

Fix#95: support host and port arguments configuration for different DBMSs in SQLancer#315mrigger merged 22 commits intomastersqlancer/sqlancer:masterfrom unknown repositoryCopy head branch name to clipboard

Conversation

ghost commented Mar 12, 2021 • edited by ghost Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What problem does this PR solve?

Summary

Release note

Uh oh!

mrigger left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ghost Mar 17, 2021 • edited by ghost Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ghost Mar 17, 2021 • edited by ghost Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ghost commented Mar 18, 2021 • edited by ghost Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mrigger commented Mar 19, 2021

Uh oh!

ghost commented Mar 20, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mrigger commented Mar 20, 2021

Uh oh!

ghost commented Mar 21, 2021

Uh oh!

mrigger commented Mar 25, 2021

Uh oh!

ghost commented Mar 25, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Fix#95: support host and port arguments configuration for different DBMSs in SQLancer#315
mrigger merged 22 commits into
masterfrom
unknown repository

ghost commented Mar 12, 2021 •
edited by ghost
Loading

ghost Mar 17, 2021 •
edited by ghost
Loading

ghost Mar 17, 2021 •
edited by ghost
Loading

ghost commented Mar 18, 2021 •
edited by ghost
Loading