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

x/perf/storage: support postgres for db #49121

Open
chriscasola opened this issue Oct 22, 2021 · 7 comments
Open

x/perf/storage: support postgres for db #49121

chriscasola opened this issue Oct 22, 2021 · 7 comments
Labels
FeatureRequest NeedsInvestigation
Milestone

Comments

@chriscasola
Copy link

@chriscasola chriscasola commented Oct 22, 2021

What version of Go are you using (go version)?

$ go version
go version go1.17.1 darwin/amd64

Does this issue reproduce with the latest release?

Yes

What operating system and processor architecture are you using (go env)?

go env Output
$ go env
GO111MODULE=""
GOARCH="amd64"
GOBIN=""
GOCACHE="/Users/ccasola/Library/Caches/go-build"
GOENV="/Users/ccasola/Library/Application Support/go/env"
GOEXE=""
GOEXPERIMENT=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="darwin"
GOINSECURE=""
GOMODCACHE="/Users/ccasola/dev/go/pkg/mod"
GONOPROXY=""
GONOSUMDB="*.factset.com"
GOOS="darwin"
GOPATH="/Users/ccasola/dev/go"
GOPRIVATE=""
GOPROXY="http://goproxy.factset.io,https://proxy.golang.org,direct"
GOROOT="/usr/local/opt/go/libexec"
GOSUMDB="sum.golang.org"
GOTMPDIR=""
GOTOOLDIR="/usr/local/opt/go/libexec/pkg/tool/darwin_amd64"
GOVCS=""
GOVERSION="go1.17.1"
GCCGO="gccgo"
AR="ar"
CC="clang"
CXX="clang++"
CGO_ENABLED="1"
GOMOD="/Users/ccasola/dev/goperf/go.mod"
CGO_CFLAGS="-g -O2"
CGO_CPPFLAGS=""
CGO_CXXFLAGS="-g -O2"
CGO_FFLAGS="-g -O2"
CGO_LDFLAGS="-g -O2"
PKG_CONFIG="pkg-config"
GOGCCFLAGS="-fPIC -arch x86_64 -m64 -pthread -fno-caret-diagnostics -Qunused-arguments -fmessage-length=0 -fdebug-prefix-map=/var/folders/7m/kwm3sc350q3c5w3cq0xkm2_hykbkhg/T/go-build265302748=/tmp/go-build -gno-record-gcc-switches -fno-common"

What did you do?

I'm trying to use the x/perf/storage app in an environment that requires postgresql instead of mysql.

What did you expect to see?

I'd expect postgres to work properly.

What did you see instead?

The create table statements and some of the queries are not compatible with postgres.

I'd be happy to contribute this change if you are open to supporting postgres in addition to the already supported mysql and sqlite.

@gopherbot gopherbot added this to the Unreleased milestone Oct 22, 2021
@dmitshur dmitshur added FeatureRequest NeedsInvestigation labels Oct 25, 2021
@dmitshur
Copy link
Contributor

@dmitshur dmitshur commented Oct 25, 2021

I'm not sure whether it's in scope of x/perf/storage package to support environments other than what's needed for Go's own needs at perfdata.golang.org. There isn't an owner entry for this package, so I'm not sure if someone would have bandwidth to maintain it otherwise.

CC @prattmic.

@prattmic
Copy link
Member

@prattmic prattmic commented Oct 25, 2021

I am slightly surprised to see someone running another production version of this. :) What is your use case? You simply want a custom/internal instance of perf.golang.org for your projects?

I am not particularly opposed to postgres support, but I also worry about maintenance; I suspect that it would break and we wouldn't even notice.

FWIW, we'll probably be changing these packages up a fair bit as part of #48803.

cc @aclements @mknyszek @dr2chase

@chriscasola
Copy link
Author

@chriscasola chriscasola commented Oct 25, 2021

The use case is to track benchmark data for projects that are not open sourced and where we don't want to expose the content of benchmark results. I was hoping to just start up a local instance of perf.golang.org.

It looks like the goal of #48803 is pretty much exactly what I'm trying to accomplish. Is that going to replace perf.golang.org or be an additional tool?

@dr2chase
Copy link
Contributor

@dr2chase dr2chase commented Nov 5, 2021

Current plan (we think) is that perf.golang.org will hold the benchmark data; for your purposes, sounds like we want to make that something that can be (re)configured w/o too much trouble.

@dr2chase
Copy link
Contributor

@dr2chase dr2chase commented Jan 10, 2022

Cleaning open tabs, noticed this, do you have a change-for-postgres already written, and available to look at? I do worry that we might break it, but if it's not large, maybe? I ask because I've messed with postgres in the past, had reason to use it recently in another context.

@chriscasola
Copy link
Author

@chriscasola chriscasola commented Jan 10, 2022

I have a change but I haven't had time to fully test or complete it. chriscasola@0fc0a1c

@dr2chase
Copy link
Contributor

@dr2chase dr2chase commented Jan 11, 2022

Oh dear. Thanks for that work, that makes me grumpy at the sql driver authors, some expect ?, ?, ? and others expect $1, $2, $3. Are you using pgx? I wonder if they would accept an "extension" along those lines.

It might be interesting to get rid of gratuitous incompatibilities, do the BIGINTs really need to be nonstandardly UNSIGNED?

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

No branches or pull requests

5 participants