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

Encode xmin in postgres revisions to respect zedtoken order #930

Merged
merged 10 commits into from
Oct 25, 2022
Merged

Encode xmin in postgres revisions to respect zedtoken order #930

merged 10 commits into from
Oct 25, 2022

Conversation

jakedt
Copy link
Member

@jakedt jakedt commented Oct 23, 2022

No description provided.

@jakedt jakedt requested a review from ecordell October 23, 2022 14:12
@jakedt jakedt requested a review from vroldanbet as a code owner October 23, 2022 14:12
@jakedt jakedt requested review from a team and removed request for a team October 23, 2022 14:12
@github-actions github-actions bot added area/api v1 Affects the v1 API area/datastore Affects the storage system area/dispatch Affects dispatching of requests area/tooling Affects the dev or user toolchain (e.g. tests, ci, build tools) labels Oct 23, 2022
@jakedt jakedt force-pushed the postgres-revisions branch 2 times, most recently from d3fe658 to c038beb Compare October 23, 2022 22:02
ecordell
ecordell reviewed Oct 24, 2022
View reviewed changes
Copy link
Contributor

@ecordell ecordell left a comment

Choose a reason for hiding this comment

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

looks good - one real question and a couple of small things, and I still want to run it and test it in the same setting we saw the reversals before just to double check

internal/datastore/memdb/readonly.go Outdated
@@ -18,7 +19,7 @@ type txFactory func() (*memdb.Txn, error)
type memdbReader struct {
TryLocker
txSource txFactory
revision datastore.Revision
revision decimal.Decimal
Copy link
Contributor

Choose a reason for hiding this comment

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

not revision.Decimal?

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 actually vestigial, removing it.

internal/datastore/postgres/revisions.go
@@ -27,12 +30,17 @@ const (
// %[2] Relationship tuple transaction table
// %[3] Name of timestamp column
// %[4] Quantization period (in nanoseconds)
// %[5] Name of snapshot column
Copy link
Contributor

Choose a reason for hiding this comment

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

%[5] isn't used in the query? did you mean for the column name to be parameterizable?

internal/datastore/postgres/revisions.go
@@ -27,12 +30,17 @@ const (
// %[2] Relationship tuple transaction table
Copy link
Contributor

Choose a reason for hiding this comment

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

in the query description, might be worth adding some info about the xmin. It's really detailed right now, but doesn't mention the xmin at all.

internal/datastore/postgres/gc.go Outdated
// Delete any relationship rows with deleted_transaction <= the transaction ID.
removed.Relationships, err = pgd.batchDelete(
ctx,
tableTuple,
relationTuplePKCols,
sq.LtOrEq{colDeletedXid: transactionFromRevision(txID)},
sq.LtOrEq{colDeletedXid: revision.tx},
Copy link
Contributor

Choose a reason for hiding this comment

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

do we need to query against the xmins too?

I don't know that an application should be reasoning about liveness at the GC threshold, but I think this means you could send in a revision (close to the GC threshold) twice in a row and get different answers.

Ideally what you'd want is just one answer followed by an "revision out of bounds" error, right?

@jakedt jakedt force-pushed the postgres-revisions branch 2 times, most recently from 93ede83 to 585e2cc Compare October 24, 2022 17:50
ecordell
ecordell previously approved these changes Oct 24, 2022
View reviewed changes
Copy link
Contributor

@ecordell ecordell left a comment

Choose a reason for hiding this comment

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

LGTM

Tested and didn't see the issues we were seeing before 👍

@jakedt jakedt force-pushed the postgres-revisions branch from 585e2cc to f1bbcfd Compare October 24, 2022 19:39
ecordell
ecordell previously approved these changes Oct 24, 2022
View reviewed changes
jakedt added 10 commits October 25, 2022 10:06
@jakedt
datastore/postgres: add the snapshot xmin as the fractional part of t…
0e05280 
…he datastore revision
@jakedt
refactor: switch datastore.Revision to a comparable interface everywhere
3f4078d
@jakedt
e2e: update go.mod and go.sum
7e6f545
@jakedt
refactor: make the decimal revisions implementation public
26eb4aa
@jakedt
datastore/postgres: add a test for inverted revisions on concurrent t…
1799cd4 
…ransactions
@jakedt
datasore: test that all datastores serialize revisions compatible wit…
81dc20b 
…h dispatch
@jakedt
datastore/memdb: drop vestigial revision in snapshot reader
430a4f9
@jakedt
datastore/postgres: add index to accelerate aggregate xid8 queries
3d911c0
@jakedt
datastore/postgres: fix gc to only delete data below cleaned transact…
fbaee5b 
…ion xmin
@jakedt
v1alpha1: re-expose revision hashing functions for downstream software
6bf25fd
@jakedt jakedt force-pushed the postgres-revisions branch from f1bbcfd to 6bf25fd Compare October 25, 2022 14:11
ecordell
ecordell approved these changes Oct 25, 2022
View reviewed changes
Copy link
Contributor

@ecordell ecordell left a comment

Choose a reason for hiding this comment

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

LGTM

@vroldanbet vroldanbet removed their request for review October 25, 2022 14:45
@jakedt jakedt enabled auto-merge October 25, 2022 15:23
@jakedt jakedt merged commit f817bf8 into main Oct 25, 2022
@jakedt jakedt deleted the postgres-revisions branch October 25, 2022 15:25
@github-actions github-actions bot locked and limited conversation to collaborators Oct 25, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@ecordell ecordell ecordell approved these changes

Assignees
No one assigned
Labels
area/api v1 Affects the v1 API area/datastore Affects the storage system area/dispatch Affects dispatching of requests 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.
2 participants
@jakedt @ecordell