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

fix(finisher_api): compose sqls and vars statement #6567

Closed

Conversation

AntonioKichaev
Copy link

  • Do only one thing
  • Non breaking API changes
  • Tested

What did this pull request do?

fixed #5511
after every subtx statement reseted, idk if it is good fix, but if u have any ideas i would be happy to make it

User Case Description

package main

import (
	"fmt"

	"github.com/brianvoe/gofakeit"
	"gorm.io/driver/postgres"
	_ "gorm.io/driver/postgres"
	"gorm.io/gorm"
)

type User struct {
	ID   int
	Name string
	Sex  string
}

func main() {
	db, err := gorm.Open(postgres.Open(fmt.Sprintf(
		"postgres://%s:%s@%s/%s?sslmode=%s",
		"postgres",
		"postgres",
		"localhost:5437",
		"postgres",
		"disable",
	)), &gorm.Config{})

	if err != nil {
		fmt.Printf("storage connect error: %v", err)
		return
	}

	err = db.AutoMigrate(User{})

	if err != nil {
		fmt.Printf("storage connect error: %v", err)
		return
	}

	users := []User{
		{
			ID:   1,
			Name: gofakeit.Name(),
			Sex:  "a",
		},
		{
			ID:   2,
			Name: gofakeit.UUID(),
			Sex:  "bbb",
		},
		{
			ID:   3,
			Name: gofakeit.Name(),
			Sex:  "a",
		},
		{
			ID:   4,
			Name: gofakeit.UUID(),
			Sex:  "a",
		},
	}

	sql := db.ToSQL(func(tx *gorm.DB) *gorm.DB {
		return tx.Model(&User{}).CreateInBatches(users, 2)
	})
	fmt.Println("sql", sql)

}

output

sql INSERT INTO "users" ("name","sex","id") VALUES ('Selmer Orn','a',1),('45681709-805a-4fee-9ded-781aff72c90f','bbb',2) RETURNING "id";INSERT INTO "users" ("name","sex","id") VALUES ('Selmer Orn','a',1),('45681709-805a-4fee-9ded-781aff72c90f','bbb',2) RETURNING "id";

@saeidee
Copy link
Member

saeidee commented Sep 1, 2023

Thanks for your contribution @AntonioKichaev, Could you please cover it with a test case?

@AntonioKichaev
Copy link
Author

Thanks for your contribution @AntonioKichaev, Could you please cover it with a test case?

I also pushed test cases.
I deeply pondered this problem, and also wrote test, and there is a problem with postgres.
that's why
when we create batch sql for postgres we use placeHolder $ and for every args plus 1/2/3/4/5 etc and then we have next sql

INSERT INTO "users" ("created_at","updated_at","deleted_at","name","age","birthday","company_id","manager_id","active") VALUES ($1,$2,$3,$4,$5,$6,$7,$8,$9),($10,$11,$12,$13,$14,$15,$16,$17,$18) RETURNING "id";

it good work for one batch.
but if we have over batch values i mean, batchSize=2, value>2, we created two sql statement they will be the same
example :

bs := 2
m := gorm.Model{
    CreatedAt: date,
    UpdatedAt: date,
}
sql = DB.ToSQL(func(tx *gorm.DB) *gorm.DB {
		return tx.Model(&User{}).
			CreateInBatches(
				[]User{
					{
						Name: "foo", Age: 20, Model: m,
					},
					{
						Name: "bar", Age: 30, Model: m,
					},
					{
						Name: "foo bar", Age: 40, Model: m,
					},
					{
						Name: "bar foo", Age: 50, Model: m,
					},
				},
				bs,
			)
	})
INSERT INTO "users" ("created_at","updated_at","deleted_at","name","age","birthday","company_id","manager_id","active") VALUES ($1,$2,$3,$4,$5,$6,$7,$8,$9),($10,$11,$12,$13,$14,$15,$16,$17,$18) RETURNING "id";
INSERT INTO "users" ("created_at","updated_at","deleted_at","name","age","birthday","company_id","manager_id","active") VALUES ($1,$2,$3,$4,$5,$6,$7,$8,$9),($10,$11,$12,$13,$14,$15,$16,$17,$18) RETURNING "id";

and when we try to exec db.ToSql
it upserts only data from model1/model2

and here i have no idea how to fix it, should i close this PR and copy this msg to issue?

anyway there are 2 probles,

  1. when value <= batchSize it wasn't wokr (before this pr didn't work db.ToSql)
  2. when value > batchSize we have problem with postgres because Statement.Sql are the same

i can do pr which solve the first problem, and create issue with second problem.

i commited 2 test cases, this one // create batch, models are equal batch size is passed

@saeidee
Copy link
Member

saeidee commented Sep 12, 2023

@AntonioKichaev what do you think if you add the tests for other dialects except Postgres and fix the first issue with another PR?

@AntonioKichaev
Copy link
Author

@AntonioKichaev what do you think if you add the tests for other dialects except Postgres and fix the first issue with another PR?

sure, added test.

@jinzhu
Copy link
Member

jinzhu commented Oct 10, 2023

afaik, this will broken if the driver not configured to accept multiple statements, so maybe it is not a good idea to make this behaviour by default.

@jinzhu jinzhu closed this Oct 10, 2023
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.

how to dump a sql when using CreateInBatches
3 participants