Change to native pgx/v5 driver with scany #244

lzap · 2022-10-03T12:30:47Z

Replaces database/sql and sqlx with pgx driver and scany library. There are multiple reasons why I want to do this.

The code is cleaner and more simple since pgx driver automatically prepares SQL statements and cache them.
Better errors (error messages do appear to be more detailed).
Native logging via zerolog and tracing via opentelemetry.
The driver natively supports many custom types, including JSONB which we use (which should be faster to encode/decode).
The driver of choice for image builder, this allows us to directly use jobqueue and we could drop our copy from the codebase.

This is a WIP branch, the way I want to approach this is to show how it looks like and go through review first, once the code is okay I will proceed with the all DAO facades. The only implemented facade is: Accounts (integration tests are passing for this DAO).

This uses very recent version of pgx (v5) and scanny (v2) and tern migration library (v2). While pgx/v5 has been out for few weeks, both mentioned libraries are currently in v2-beta stage. The replace statement in go.mod is temporary, I hope the pgx author will merge my small patch that enables to use proper Go mod: jackc/tern#69

Done:

Simplify DAO errors
Remove NameForError functions from DAO
Remove Value/Scan from reservation model (comment // TODO: Use pgx native driver with scany library instead of sqlx which does not)

Things I want to add as separate commits after review is done:

Get rid of error from GetXXXDao methods

Things to be done in other PRs:

Rename Pubkey Resource DAO methods to UnscopedXXXResource
Refactor dejq to PGX
Add Prometheus metrics (Update to pgx v5 IBM/pgxpoolprometheus#5)

lzap · 2022-10-03T12:31:30Z

cmd/pbapi/main.go

@@ -54,7 +54,7 @@ func main() {
 	config.Initialize()

 	// initialize stdout logging and AWS clients first
-	log.Logger = logging.InitializeStdout()
+	logging.InitializeStdout()


The log.Logger is now set by the function itself.

lzap · 2022-10-03T12:31:44Z

cmd/pbapi/main.go

 	if err != nil {
 		log.Fatal().Err(err).Msg("Error initializing database")
 	}
+	defer db.Close()


Connection pool is now properly closed.

lzap · 2022-10-03T12:32:10Z

cmd/pbapi/main.go

@@ -76,10 +76,11 @@ func main() {
 	defer tel.Close(ctx)

 	// initialize the rest
-	err = db.Initialize("public")
+	err = db.Initialize(ctx, "public")


Database initialization and migration now requires Context.

lzap · 2022-10-03T12:33:27Z

go.mod

+// go get github.com/jackc/tern/migrate@v2.0.0-beta.1
+
+replace github.com/jackc/tern => github.com/lzap/tern2 v0.0.0-20220813151204-54e00e2a8f9d
+


This will be removed once tern v2 is properly tagged, the two go get commands are required until stable versions are released tho.

I suggest to go ahead and merge this so we can start testing this on stage.

hump, we will need to convince our linter tho.
Also do we have any timelines for the releases.
I'm fine being early adopter, but only if we know the releases are likely to happen in timely manner.

The replace statement is not needed anymore since tern v2.beta.2 was just tagged, so it's not necessary now.

lzap · 2022-10-03T12:34:34Z

internal/config/0_common_config.go

+	parser.Viper.SetDefault("database.minConn", 2)
+	parser.Viper.SetDefault("database.maxConn", 100)
+	parser.Viper.SetDefault("database.maxLifetime", 2*time.Hour)
+	parser.Viper.SetDefault("database.maxIdleTime", 15*time.Minute)


FYI the default values from the pgx driver are:

Minimum conn: 0

Max conn: 4

Lifetime: 1 hour

Idletime: 30 minutes

Our values are wild guesses, we need to talk with SRE about this in more detail.

lzap · 2022-10-03T12:35:49Z

internal/dao/dao_errors.go

+// TODO let's vastly simplify error handling in DAO, let's just create aliases for native PGX errors
+// and that should be enough:
+// var NoRowsError = pgx.ErrNoRows
+


Once I am done with DAO implementations, I would like to vastly simplify interface handling in our DAO interface. I am hoping to have just aliases/copies of all errors DAO callers need to work with, thus utilizing native PGX errors.

lzap · 2022-10-03T12:36:49Z

internal/dao/pgx/account_pgx.go

+// TODO DAO getters should not return error anymore, we use it everywhere
+func getAccountPGX(ctx context.Context) (dao.AccountDao, error) {
+	return &accountDao{name: "account"}, nil
+}


Once I am done with this, getXXXDao will not return an error because there will be no statement prepare calls. This is a huge deal as we call this function everywhere, so we will save a lot of error handling. I would like to do this change in a separate commit/PR tho as it will be a big bang.

lzap · 2022-10-03T12:37:17Z

internal/dao/tests/account_test.go

@@ -108,7 +108,7 @@ func TestGetByOrgId(t *testing.T) {
 	assert.Equal(t, "1", account.AccountNumber.String)
 }

-func TestGetOrCreateByIdentityGet(t *testing.T) {
+func TestGetOrCreateByIdentityGetAccount(t *testing.T) {
 	accDao, ctx := setupAccount(t)
 	defer teardownAccount(t)
 	account, err := accDao.GetOrCreateByIdentity(ctx, "1", "1")


Renamed these so I can test against "Account" pattern.

What is "Account" pattern? 🤔

You can run tests against a regexp patter, so I was running all tests starting with Account, then with Pubkey etc. This allowed me to refactor and test DAO after DAO.

lzap · 2022-10-03T12:37:50Z

internal/db/connection.go

+// PGX is the main connection pool for the whole application
+var PGX *pgxpool.Pool
+
+// TODO remove after all DAO packages are rewritten
 var DB *sqlx.DB


This will be removed once done with the refactoring.

lzap · 2022-10-03T12:38:12Z

internal/db/connection.go

+			poolConfig.ConnConfig.Tracer = &tracelog.TraceLog{
+				Logger:   zeroLogger,
+				LogLevel: logLevel,
+			}


Tracing and logging with full context support and spans, yay! :-)

lzap · 2022-10-03T12:39:51Z

internal/db/seeds/emed.go

+import "embed"
+
+//go:embed *.sql
+var EmbeddedSeeds embed.FS


Had to move both migrations and seeds into the directory because the way the Tern API is designed today, it does not allow callers to locate files from subdirectories.

lzap · 2022-10-03T16:35:43Z

Pushed new changes:

Updated go.mod it no longer requires replace
Pubkeys pgx implementation
More pubkey tests - all are green now

Last thing to solve is reservations implementation, then I can remove sqlx completely. Tests will be failing until this is fixed.

I am thinking renaming db.PGX to db.Pool which is a better name and also PGX is kinda hard to type.

ezr-ondrej

So if I understand correctly the pgx implementatios are tested, but not used in prod, right? :)
Just making sure, I agree it's a correct approach 👍
I have just one nit and I'd love to hear your opinion on keeping the naming convention for test methods 1:1 with tested methods

ezr-ondrej · 2022-10-04T07:19:42Z

internal/dao/tests/pubkey_resource_test.go

@@ -74,6 +75,13 @@ func TestGetResourceByProviderType(t *testing.T) {
 	assert.Equal(t, resource, createdResource)
 }

+func TestGetPubkeyResourceByProviderTypeNoRows(t *testing.T) {


I'd rather see more test in one function by t.Run() if those are for the same function, I've tried to start this, but we do not have it everywhere, so up to you, but I'd love to have test function per function mapping 1:1 if possible.

ezr-ondrej · 2022-10-04T07:21:54Z

internal/dao/dao_errors.go

+var ErrNoRows = pgx.ErrNoRows
+var ErrAffectedMismatch = errors.New("unexpected affected rows")


I guess linter wants to group the var statements, right?

lzap · 2022-10-04T07:39:51Z

So if I understand correctly the pgx implementatios are tested, but not used in prod, right? :)

Yeah, this is only during the review tho, at the end of this PR, I will remove sqlx completely. Having two implementations for even a week is a dangerous thing (we could accidentally change only one and not the other).

lzap · 2022-10-04T14:53:17Z

The work is done, app is working properly locally.

This is now ready for formal review. After this is confirmed to be working, I want to add two more commits to further simplify and rename some DAO interface functions. These would be pure refactoring, no code changes.

lzap · 2022-10-04T14:57:26Z

(The linter fails because it sees we use unreleased version of the tern library. Let's ignore that for a moment.)

ezr-ondrej · 2022-10-04T15:08:33Z

These would be pure refactoring, no code changes

Can these be done in separate PR then?

lzap · 2022-10-05T11:38:09Z

Can these be done in separate PR then?

Sure, then consider this as done, feel free to review. I will disable the linters complain about use of unreleased versions, both project authors confirmed they intend to release soon.

lzap · 2022-10-05T12:33:04Z

Rebased because tests were failing. I also renamed PGX to Conn which is better.

lzap · 2022-10-05T12:52:52Z

internal/dao/dao_interfaces.go

@@ -71,6 +73,7 @@ type ReservationDao interface {
 	List(ctx context.Context, limit, offset int64) ([]*models.Reservation, error)

 	// ListInstances returns instances associated to a reservation. UNSCOPED.
+	// It currently lists all instances and not instances for a reservation, this is a TODO.
 	ListInstances(ctx context.Context, limit, offset int64) ([]*models.ReservationInstance, error)


https://issues.redhat.com/browse/HMSPROV-298

lzap · 2022-10-05T12:55:07Z

internal/dao/tests/pubkey_test.go

+		require.ErrorIs(t, err, dao.ErrAffectedMismatch)
+	})
+
+	// TODO validators


This TODO is for me to add even more pubkey tests (validators).

I can add it either during rebase of this PR or later.

ezr-ondrej

I've got two filosophical comments, but nothing major, I'm all for getting this in. Looks as great improvement :)

ezr-ondrej · 2022-10-04T15:13:29Z

internal/dao/dao_errors.go

-	Context context.Context
-}
-
-var WrongTenantError = errors.New("trying to manipulate data of different tenant")


I did like throwing the same errors from stubs as from the real implementations, it gave stub methods another layer of real. If we are getting rid of these errors, shall we get rid of them from stubs as well?

ezr-ondrej · 2022-10-04T15:16:05Z

internal/dao/pgx/account_pgx.go

+}
+
+func (x *accountDao) GetById(ctx context.Context, id int64) (*models.Account, error) {
+	query := `SELECT * FROM accounts WHERE id = $1 LIMIT 1`


I kinda liked the queries being together, I get that it's not necessary now, but won't it still be a benefit to have these as package variables (or constants) on top of each DAO file?

I've gotten convinced these are better directly in the functions, as it's usually changed together.

Yeah indeed, we can get to a point that some queries would be shared across functions, in that case let's totally make constants. But so far it's trivial.

lzap · 2022-10-05T13:16:24Z

Merged DAO and DAO stubs errors into a single package, it's now consistent and clean. Added extra validation tests to the pubkey dao tests suite. Rebased.

ezr-ondrej

Great job @lzap ! let's hope the libraries will release soon, but our code looks much healthier now! 🎉

lzap commented Oct 3, 2022

View reviewed changes

lzap force-pushed the pgx5 branch from 201250d to 9972193 Compare October 3, 2022 16:33

ezr-ondrej reviewed Oct 4, 2022

View reviewed changes

lzap force-pushed the pgx5 branch from 9972193 to 3899d4f Compare October 4, 2022 14:49

lzap force-pushed the pgx5 branch 2 times, most recently from 30c3e96 to b958e71 Compare October 5, 2022 12:32

lzap commented Oct 5, 2022

View reviewed changes

ezr-ondrej approved these changes Oct 5, 2022

View reviewed changes

lzap force-pushed the pgx5 branch from b958e71 to b46e1c1 Compare October 5, 2022 13:13

Change to native pgx/v5 driver with scany

995d42c

lzap force-pushed the pgx5 branch from b46e1c1 to 995d42c Compare October 5, 2022 13:15

Drop error from GetXXXDAO functions

85f7170

ezr-ondrej approved these changes Oct 5, 2022

View reviewed changes

ezr-ondrej merged commit ee76648 into RHEnVision:main Oct 5, 2022

lzap deleted the pgx5 branch October 5, 2022 14:36

		// go get github.com/jackc/tern/migrate@v2.0.0-beta.1

		replace github.com/jackc/tern => github.com/lzap/tern2 v0.0.0-20220813151204-54e00e2a8f9d

		var ErrNoRows = pgx.ErrNoRows
		var ErrAffectedMismatch = errors.New("unexpected affected rows")

Change to native pgx/v5 driver with scany #244

Change to native pgx/v5 driver with scany #244

Conversation

lzap commented Oct 3, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lzap commented Oct 3, 2022

ezr-ondrej left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lzap commented Oct 4, 2022

lzap commented Oct 4, 2022

lzap commented Oct 4, 2022

ezr-ondrej commented Oct 4, 2022

lzap commented Oct 5, 2022

lzap commented Oct 5, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ezr-ondrej left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lzap commented Oct 5, 2022

ezr-ondrej left a comment

Choose a reason for hiding this comment

lzap commented Oct 3, 2022 •

edited

Loading