tiup icon indicating copy to clipboard operation
tiup copied to clipboard

Update github.com/xo/usql v0.9.5 -> v0.19.x

Open ottok opened this issue 4 months ago • 10 comments

Feature Request

Currently, TiUP depends on a version of usql from 2021:

github.com/xo/usql v0.9.5

See https://github.com/pingcap/tiup/blob/f6aa0ac09b9d6405929c2cea3c0207880e53d199/go.mod#L48

This version is of usql is so old that it itself has outdated and unmaintained dependencies, such as for example in https://github.com/xo/usql/blame/6228ccb8c84c99d34bfbd02da339edbb8a9c3c2e/env/env.go#L23 it uses zaf/temp, which has been deprecated since 2022 and its functionality has been included in the Go standard library since version 1.14 in the os package (os.CreateTemp and os.MkdirTemp).

ottok avatar Aug 22 '25 19:08 ottok

  1. I'm not convinced that keeping usql is the right solution for the long run. This is because tiup client (which uses usql) tries to mimic a Postgres client. This means that it uses a non-standard SQL mode etc. to change the quoting behavior. And this doesn't match with our documentation (which uses the MySQL Client). Also tiup client is used only for tiup playground, but not in situations where TiDB is deployed with tiup cluster or the k8s operator. A solution might be to callout to mysql with the right options instead of using usql. I do think usql is a good and useful tool, but maybe not in the way that we use it.
  2. Updating usql will probably pull in many new dependencies. I think it might have some ways to build with only a subset of database drivers, but I'm not sure if we can limit the set of dependencies in go.mod.
  3. We can try to update usql to the latest version (and then try to keep it up to date) or we can update it to a slightly newer version than we have now if that's easier and if there is a plan to eventually get rid of it.

dveeden avatar Aug 25 '25 07:08 dveeden

Looks like the API for github.com/xo/usql/rline changed a bit.

dveeden avatar Aug 25 '25 07:08 dveeden

@ottok @xhebox I created two PRs for this. One does a upgrade to the latest version and the other upgrades to a slightly newer version, allowing to do this in multiple steps.

dveeden avatar Aug 26 '25 12:08 dveeden

I created a third option: to remove usql

dveeden avatar Aug 26 '25 17:08 dveeden

Thanks for looking into this. Just to be explicit, this is not a blocker for tiup in Debian and I am not in a position to decide if TiUP really needs this or not. I filed this mainly because I noticed that usql seems kind of old. The version I am in progress of uploading to Debian is usql 0.19.19.

ottok avatar Aug 28 '25 17:08 ottok

In #2602 I have a tiny commit that makes TiUP compatible with usql 0.19.19:

Subject: [PATCH 4/6] Make TiUP compatible with usql v0.19.19

As the  golang-github-xo-usql-dev package v0.19.19 in Debian is
newer than the v0.9.5 in go.mod of TiUP, the Debian build failed on:

    src/github.com/pingcap/tiup/components/client/main.go:98:33: not enough arguments in call to rline.New
    	have (bool, string, string)
    	want (bool, bool, bool, string, string)
    src/github.com/pingcap/tiup/components/client/main.go:102:70: not enough arguments in call to handler.New
    	have (rline.IO, *user.User, string, bool)
    	want (rline.IO, *user.User, string, billy.Filesystem, bool)

Apply these changes on TiUP to make it compatible:

- `rline.New` now requires two additional boolean arguments. These are
  set to `false` as safe defaults.

- `handler.New` now requires a `billy.Filesystem` argument. This is
  provided by importing `osfs` and creating a new `osfs.New` instance
  using the instance data directory.
---
 components/client/main.go | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/components/client/main.go b/components/client/main.go
index 1e03eb0560..a12d68ac2e 100644
--- a/components/client/main.go
+++ b/components/client/main.go
@@ -23,6 +23,7 @@ import (
 	"os/user"
 	"path"
+	"github.com/go-git/go-billy/v5/osfs"
 	ui "github.com/gizak/termui/v3"
 	"github.com/gizak/termui/v3/widgets"
 	"github.com/pingcap/tiup/pkg/environment"
@@ -95,11 +96,12 @@ func connect(target string) error {
 	if err != nil {
 		return fmt.Errorf("can't get current user: %s", err.Error())
 	}
-	l, err := rline.New(false, "", env.HistoryFile(u))
+	l, err := rline.New(false, false, false, "", env.HistoryFile(u))
 	if err != nil {
 		return fmt.Errorf("can't open history file: %s", err.Error())
 	}
-	h := handler.New(l, u, os.Getenv(localdata.EnvNameInstanceDataDir), true)
+	dataDir := os.Getenv(localdata.EnvNameInstanceDataDir)
+	h := handler.New(l, u, dataDir, osfs.New(dataDir), true)
 	if err = h.Open(context.TODO(), ep.dsn); err != nil {
 		return fmt.Errorf("can't open connection to %s: %s", ep.dsn, err.Error())
 	}

ottok avatar Sep 10 '25 21:09 ottok

In #2594 I have a similar, but slightly different solution: https://github.com/pingcap/tiup/pull/2594/files#diff-b0901027b8959f66be5eb63095655f04324c7fbc83348dcf9868699d6d258f4a

dveeden avatar Sep 11 '25 06:09 dveeden

Yours is for usql v0.19.25 and fine to merge. I will just bump usql in Debian to v0.19.25 and we are good with that.

ottok avatar Sep 11 '25 06:09 ottok

#2595 merged

xhebox avatar Nov 12 '25 06:11 xhebox

Merged commit https://github.com/pingcap/tiup/commit/c57cefb1f1394613e7f3a80ccdcb03668569a43c updated to usql v0.14.0.

ottok avatar Nov 12 '25 06:11 ottok