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.

Sign up for GitHub

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

Jump to bottom

Quantize revisions for memdb, postgres datastores #527

Merged
merged 12 commits into from
Apr 14, 2022
Merged

Quantize revisions for memdb, postgres datastores #527

merged 12 commits into from
Apr 14, 2022

Conversation

jakedt
Copy link
Member

@jakedt jakedt commented Apr 13, 2022

No description provided.

@github-actions github-actions bot added area/datastore Affects the storage system area/tooling Affects the dev or user toolchain (e.g. tests, ci, build tools) labels Apr 13, 2022
josephschorr
josephschorr reviewed Apr 13, 2022
View reviewed changes
internal/datastore/postgres/postgres.go
if quantizationPeriodNanos < 1 {
quantizationPeriodNanos = 1
}
revisionQuery := fmt.Sprintf(
Copy link
Member

Choose a reason for hiding this comment

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

Since most of the parameters are "static", could we somehow include them above and only parameterize on the quantizationPeriodNanos?

Copy link
Member Author

Choose a reason for hiding this comment

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

This is only done once at initialization time.

internal/datastore/postgres/revisions.go Outdated
revisionTx := transactionFromRevision(revision)
offset := uint64(pgd.gcWindowInverted.Seconds())

queryValid := fmt.Sprintf(
Copy link
Member

Choose a reason for hiding this comment

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

Same question re: parameterization

Copy link
Member Author

Choose a reason for hiding this comment

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

I can fill in most of them at initialization time, and then pass one as an arg.

internal/datastore/postgres/revisions.go Outdated

queryValid := fmt.Sprintf(
queryValidTransaction,
revisionTx,
Copy link
Member

Choose a reason for hiding this comment

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

Any way we could escape the revisionTx such that if it does contain SQL, it won't cause a problem?

Copy link
Member Author

Choose a reason for hiding this comment

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

it's an int, how could it contain sql?

Copy link
Member

Choose a reason for hiding this comment

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

For some reason I thought it was a string

josephschorr
josephschorr reviewed Apr 13, 2022
View reviewed changes
internal/datastore/postgres/revisions.go Outdated
}

func createNewTransaction(ctx context.Context, tx pgx.Tx) (newTxnID uint64, err error) {
ctx, span := tracer.Start(ctx, "computeNewTransaction")
Copy link
Member

Choose a reason for hiding this comment

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

Fix the name of the span

josephschorr
josephschorr reviewed Apr 13, 2022
View reviewed changes
internal/datastore/postgres/postgres.go Outdated
@@ -61,6 +55,20 @@ const (
tracingDriverName = "postgres-tracing"

batchDeleteSize = 1000

querySelectRevision = `
Copy link
Member

Choose a reason for hiding this comment

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

Mind adding a comment here explaining each of the queries?

@github-actions github-actions bot added the area/CLI Affects the command line label Apr 13, 2022
josephschorr
josephschorr reviewed Apr 13, 2022
View reviewed changes
internal/datastore/postgres/revisions.go Outdated

// querySelectRevision will round the database's timestamp down to the nearest
// quantization period, and then find the first transaction after that. If there
// are no transactions newer than the quantization period, it just picks the last
Copy link
Member

Choose a reason for hiding this comment

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

last -> latest

@jzelinskie jzelinskie changed the title Optimized revision Quantize revisions for memdb, postgres datastores Apr 13, 2022
josephschorr
josephschorr reviewed Apr 14, 2022
View reviewed changes
internal/datastore/postgres/revisions.go Outdated
// querySelectRevision will round the database's timestamp down to the nearest
// quantization period, and then find the first transaction after that. If there
// are no transactions newer than the quantization period, it just picks the latest
// transaction
Copy link
Member

Choose a reason for hiding this comment

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

add period at end of comment

josephschorr
josephschorr reviewed Apr 14, 2022
View reviewed changes
internal/datastore/postgres/revisions.go
// are no transactions newer than the quantization period, it just picks the latest
// transaction
querySelectRevision = `
SELECT COALESCE(
Copy link
Member

Choose a reason for hiding this comment

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

I wonder whether we should have a test that explicitly runs these queries and validates their outputs?

Copy link
Member Author

Choose a reason for hiding this comment

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

Added.

@jakedt jakedt requested a review from a team as a code owner April 14, 2022 18:16
josephschorr
josephschorr previously approved these changes Apr 14, 2022
View reviewed changes
Copy link
Member

@josephschorr josephschorr left a comment

Choose a reason for hiding this comment

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

LGTM

@github-actions github-actions bot added the area/schema Affects the Schema Language label Apr 14, 2022
@josephschorr josephschorr mentioned this pull request Apr 14, 2022
Closed
josephschorr
josephschorr approved these changes Apr 14, 2022
View reviewed changes
Copy link
Member

@josephschorr josephschorr left a comment

Choose a reason for hiding this comment

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

LGTM

@josephschorr josephschorr merged commit c3c4b6d into main Apr 14, 2022
@josephschorr josephschorr deleted the optimized-revision branch April 14, 2022 19:32
@github-actions github-actions bot locked and limited conversation to collaborators Apr 14, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@josephschorr josephschorr josephschorr approved these changes

Assignees
No one assigned
Labels
area/CLI Affects the command line area/datastore Affects the storage system area/schema Affects the Schema Language area/tooling Affects the dev or user toolchain (e.g. tests, ci, build tools)
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@jakedt @josephschorr