issue a checkpoint when head revision moved outside an application transaction #2139
Conversation
vroldanbet
changed the title
vroldanbet
force-pushed
the
pg-checkpoint-offband-revision-bumps
branch
2 times, most recently
from
9d930ab to
8b9d0ae
Compare
github-actions
bot
added
the
area/tooling
vroldanbet
force-pushed
the
pg-checkpoint-offband-revision-bumps
branch
2 times, most recently
from
c7a9c69 to
5a0030d
Compare
vroldanbet
force-pushed
the
pg-checkpoint-offband-revision-bumps
branch
from
5a0030d to
d664a7f
Compare
| @@ -197,9 +219,18 @@ func (pgd *pgDatastore) Watch( | |||
| return updates, errs | |||
| } | |||
|
|
|||
| func (pgd *pgDatastore) getNewRevisions(ctx context.Context, afterTX postgresRevision) ([]postgresRevision, error) { | |||
| func (pgd *pgDatastore) getNewRevisions(ctx context.Context, afterTX postgresRevision, returnHeadRevision bool) ([]postgresRevision, *postgresRevision, error) { | |||
There was a problem hiding this comment.
personally I think I'd find it easier to come back to this code if it were split into getNewRevisions and getNewRevisionsWithHead
Then you wouldn't have to do any nil checking for the optionalHeadRevision (because you'd just return an error from getNewRevisionsWithHead)
There was a problem hiding this comment.
so this method is only used in 1 place, that's the postgres watch loop. And we pass returnHeadRevision arg to it as true if the client requested datastore.WatchCheckpoints, which is optional (e.g. SpiceDB Watch API does not currently return checkpoints), so it was added as an optimization in case someone does not need checkpoints.
so that refactor would be useful if we used the method in 2 different places, and one of them needed either always head, or never head, but that's not the case - it's entirely driven by the Datastore.Watch call inputs. Does that make sense?
…ansaction it's possible for `pg_current_snapshot()` (used for HeadRevision) in PG to return a new xmin:xmax outside of application transactions. For example, running `ANALYZE;` could bump it. In this situation, we need to emit a Checkpoint, in case clients triggered a call to Watch API based on a different HeadRevision from what they had cached locally (e.g. relevant to compute change deltas between revisions). The proposal is to compute head revision in the same transaction where we compute new revisions, but only if checkpoints were requested as an optimization. If the transaction determines there are no new SpiceDB transactions, we return the compute head revision, if and only if checkpoints were requested. This Watch API poll loop into 2 queries instead of 1, which can add an extra load to the database, but at least it only happens when checkpoints are being requested.
vroldanbet
force-pushed
the
pg-checkpoint-offband-revision-bumps
branch
from
d664a7f to
74dc7b2
Compare
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
it's possible for
pg_current_snapshot()(used for HeadRevision) in PG to return a new xmin:xmax outside of application transactions. For example, runningANALYZE;could bump it.In this situation, we need to emit a Checkpoint, in case clients triggered a call to Watch API based on a different HeadRevision from what they had cached locally (e.g. relevant to compute change deltas between revisions).
The proposal is to call
HeadRevisionif there are no changes. This turns the Watch API heartbeat into 2 queries instead of 1, which can add an extra load to the database.