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

[!] add support for pgx.Batch, closes #152 #179

Closed

Conversation

makubit
Copy link

@makubit makubit commented Dec 11, 2023

This PR introduces functionality to properly test SendBatch() function.
In the scope of this PR, there was created new helper structs:BatchElement, Batch, and BatchResults.

An example, how to use batches in tests:

// defining the batch that will be passed to the SendBatch() function
	expectedBatch := mock.NewBatch().
		AddBatchElements(
			NewBatchElement("SELECT *", 1),
			NewBatchElement("SELECT *"),
		)

// using rows implementation to properly simulate Query() and QueryRow() functions in BatchResults interface
	rows := NewRows([]string{"id", "name", "email"}).
		AddRow("some-id-1", "some-name-1", "some-email-1").
		AddRow("some-id-2", "some-name-2", "some-email-2")

// mock for pgx.BatchResults
	batchResultsMock := NewBatchResults().WillReturnRows(rows).AddCommandTag(pgconn.NewCommandTag("SELECT 2"))

// real batch call, that we mocked above
	batch := new(pgx.Batch)
	batch.Queue("SELECT * FROM TABLE", 1)
	batch.Queue("SELECT * FROM TABLE")

// our expectation accepts Batch mock and BatchResults mock (in additional builder function) as parameters
	mock.ExpectSendBatch(expectedBatch).
		WillReturnBatchResults(batchResultsMock)

// the call of the function, here we can use br.AnyFunctionCall() implemented in pgx.BatchResults interface
	br := mock.SendBatch(context.Background(), batch)
	a.NotNil(br)
	r, err := br.Query()

I would appreciate any comments and feedback.
~Magda

@pashagolub pashagolub self-assigned this Dec 11, 2023
@pashagolub pashagolub added the enhancement New feature or request label Dec 11, 2023
@pashagolub
Copy link
Owner

@makubit would you please check the golint warnings and fix them? Not urgent, just to know where we are with your PR :)

@pashagolub pashagolub changed the title Feature/152 Support Batch Operations [!] add support for pgx.Batch, closes #152 Dec 18, 2023
@makubit
Copy link
Author

makubit commented Dec 18, 2023

Yes, I will look at the warnings later this week, I was pretty busy recently, so I didn't have much time to look into that.

@pashagolub
Copy link
Owner

No problem at all! Take your time! I'm not planning to spend much time until the new year anyway :)

@makubit
Copy link
Author

makubit commented Jan 16, 2024

@pashagolub PR should be fine now, I resolved the linter issues. Starting this week I have more spare time, so I should be more responsive in case there are some other comments/issues.

batch.go Outdated Show resolved Hide resolved
batch.go Outdated Show resolved Hide resolved
@pashagolub
Copy link
Owner

Also raised jackc/pgx#1878 discussion about making Batch and QueuedQuery properties public

@makubit makubit requested a review from pashagolub January 19, 2024 10:01
@pashagolub
Copy link
Owner

Something I cannot implement with the new functionality. Would you please help me to build a test case for the code snippet from the pgx manual:

conn, err := pgx.Connect(ctx, os.Getenv("PGX_TEST_DATABASE"))
if err != nil {
	fmt.Printf("Unable to establish connection: %v", err)
	return
}

batch := &pgx.Batch{}
batch.Queue("select 1 + 1").QueryRow(func(row pgx.Row) error {
	var n int32
	err := row.Scan(&n)
	if err != nil {
		return err
	}

	fmt.Println(n)

	return err
})

batch.Queue("select 1 + 2").QueryRow(func(row pgx.Row) error {
	var n int32
	err := row.Scan(&n)
	if err != nil {
		return err
	}

	fmt.Println(n)

	return err
})

batch.Queue("select 2 + 3").QueryRow(func(row pgx.Row) error {
	var n int32
	err := row.Scan(&n)
	if err != nil {
		return err
	}

	fmt.Println(n)

	return err
})

err = conn.SendBatch(ctx, batch).Close()
if err != nil {
	fmt.Printf("SendBatch error: %v", err)
	return
}

The trick here is every QueuedQuery returns at least one row. But in your code we have only one result set for all queued queries. Am I correct?

@makubit
Copy link
Author

makubit commented Feb 8, 2024

Hi, yes, you are correct, I didn't notice that. I will work on some better solutions, so that QueryRow() can have more sophisticated results :)

@pashagolub
Copy link
Owner

No problem!

I didn't notice that.

Me neither. But before you'll spend more time on this, I have an idea, that this feature should look like ExpectedPrepare. In fact the current implementation of ExpectedPrepare should be deleted, it's just a leftover of how lib/sql works.

So in a few words, we just can replace implementation of ExpectedPrepare with ExpectedBatch plus some renaming and tweaks.

I understand it basically means to throw your code away, sorry about that. I will completely understand if you won't work on this implementation. On the other hand I listed this feature for this year GSoC. Maybe it's worth to work on it during the GSoC program? If you're eligible to be a participant I'd love to accept your proposal. If you're not eligible for participating but eligible to be a mentor, I'd love to onboard you as a GSoC mentor. :)

Copy link
Owner

@pashagolub pashagolub left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🙃

@pashagolub pashagolub closed this Feb 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants