GC improvements: GC only on a single node and add a missing index in PG #2159

josephschorr · 2024-12-09T20:46:07Z

Have GC lock so that it only runs on a single node at a time
Add a missing index in the Postgres datastore for GC

This should reduce datastore CPU pressure

vroldanbet · 2024-12-10T11:21:51Z

internal/datastore/postgres/common/locks.go

+
+// RunWithLocksClient runs the provided function with a pglock.Client. Should only be used
+// for migrations.
+func RunWithLocksClient(conn *pgx.Conn, runner func(client *pglock.Client) error) error {


Make it very clear this is only for migrations.

Suggested change

func RunWithLocksClient(conn *pgx.Conn, runner func(client *pglock.Client) error) error {

func RunWithLocksClientForMigrations(conn *pgx.Conn, runner func(client *pglock.Client) error) error {

vroldanbet · 2024-12-10T11:27:10Z

internal/datastore/postgres/common/locks.go

+	client, err := pglock.UnsafeNew(db,
+		pglock.WithCustomTable(locksTableName),
+		pglock.WithLeaseDuration(timeout),
+		pglock.WithHeartbeatFrequency(heartbeatFrequency),


The default gcTimeout is 1m, meaning the lease will last 1 minute. The heartbeat is 2 seconds by default. If folks change the gcTimeout via SpiceDB configuration, the heartbeat won't scale with it. WithHeartBeatFrequency docs indicates it should never be greater than half of the timeout.

// WithHeartbeatFrequency defines the frequency of the heartbeats. Heartbeats
// should have no more than half of the duration of the lease.

Given that user-provided configuration could violate this, I propose we use a fraction of the timeout: e.g. 1/3.

You should also use max(heartbeatFrequency, timeout/3) to ensure we never go to zero or into a very frequent rate.

vroldanbet · 2024-12-10T11:32:34Z

internal/datastore/postgres/gc.go

+
+func (pgd *pgDatastore) LockGCRun(ctx context.Context, timeout time.Duration, gcRun func(context.Context) error) (bool, error) {
+	if pgd.gcInterval < lockMinimumInterval {
+		return true, gcRun(ctx)


it seems like with a gcInterval less than 30 seconds, locks will be bypassed. If customer reconfigures SpiceDB GC, they could end up regressing the singleflighted GC runs, overload the datastore, and cause an incident. Seems like dangerous behaviour we should prevent.

I don't think this should be compared against lockMinimumInterval, but against timeout, which is what's used for the lock duration. If that's the case, we can validate this during application bootstrap, instead of failing later on, when the service is considered healthy. I vaguely recall we already had such safeguards, better double check.

vroldanbet · 2024-12-10T11:35:54Z

internal/datastore/postgres/gc.go

+		// Run the GC process under the lock.
+		currentTimestampData, err := time.Now().UTC().MarshalBinary()
+		if err != nil {
+			return fmt.Errorf("failed to marshal current timestamp: %w", err)
+		}


it does not seem like this is being used to drive the lock. If the reason for this is jumping into the postgres instance and troubleshooting the locks, then please clarify this with a comment accordingly.

vroldanbet · 2024-12-10T11:37:36Z

internal/datastore/common/gc.go

+	// If the implementation does not support locking, it should just execute
+	// the function and return true.
+	// If GC was run within the last interval, the function should return false.
+	LockGCRun(ctx context.Context, timeout time.Duration, gcRun func(context.Context) error) (bool, error)


I don't see any new tests added - please add one to pg integration test suite, simulating multiple GC instances running at once.

vroldanbet · 2024-12-10T11:39:36Z

internal/datastore/postgres/migrations/zz_migration.0022_add_gc_lock_table.go

+	if err := DatabaseMigrations.Register("add-gc-lock-table", "add-expiration-support",
+		func(ctx context.Context, conn *pgx.Conn) error {
+			return common.RunWithLocksClient(conn, func(client *pglock.Client) error {
+				return client.TryCreateTable()


Makes me uncomfortable being at the mercy of a third-party library migration code. How do we know bumping the library does not completely change the definition of the table and cause subsequent migrations to fail?

vroldanbet · 2024-12-10T11:41:04Z

internal/datastore/postgres/migrations/zz_migration.0023_add_missing_gc_index.go

+	"github.com/jackc/pgx/v5"
+)
+
+const addGCIndexForRelationTupleTransaction = `CREATE INDEX CONCURRENTLY 


Please add documentation to the PR body on why this was added, the previous explain, and the new explain.

Added a comment into the code

josephschorr · 2024-12-10T21:11:21Z

Redesigned to use the native locks as discussed

1) Have GC lock so that it only runs on a single node at a time 2) Add a missing index in the Postgres datastore for GC This should reduce datastore CPU pressure

ecordell

LGTM, though need to resolve the test failures

This is necessary because GC takes an exclusive lock now

vroldanbet

Changes look good to me, BUT:

Please note this only solves a part of the problem: it prevents multiple nodes from GC'ing simultaneously, which will avoid contention and spikes, but it does not elect a leader node who is the only one that should be running GC.

For example, if 100 nodes are doing GC and have their timers sufficiently skewed, it's possible to have 100 nodes GC'ing one after the other (this is an extreme, worst-case scenario). It won't happen at the same time, but it is still an inefficient use of datastore compute and can cause a sustained load on the datastore, proportional to the number of nodes.

vroldanbet · 2024-12-11T10:13:21Z

internal/datastore/mysql/locks.go

+}
+
+func (mds *Datastore) releaseLock(ctx context.Context, lockName lockName) error {
+	_, err := mds.db.ExecContext(ctx, `


Please add comment from MySQL docs:

Suggested change

_, err := mds.db.ExecContext(ctx, `

// A lock obtained with GET_LOCK() is released explicitly by executing RELEASE_LOCK()

// or implicitly when your session terminates (either normally or abnormally).

_, err := mds.db.ExecContext(ctx, `

vroldanbet · 2024-12-11T10:15:06Z

internal/datastore/postgres/migrations/zz_migration.0022_add_missing_gc_index.go

+	ON relation_tuple_transaction (xid DESC, timestamp);`
+
+func init() {
+	if err := DatabaseMigrations.Register("add-missing-gc-index", "add-expiration-support",


please rename to add-index-for-transaction-gc

vroldanbet · 2024-12-11T10:17:12Z

internal/datastore/mysql/locks.go

+)
+
+func (mds *Datastore) tryAcquireLock(ctx context.Context, lockName lockName) (bool, error) {
+	// Acquire the lock, with max 1s timeout.


// please add comment indicating GET_LOCK is reentrant. From the docs:

It is even possible for a given session to acquire multiple locks for the same name. Other sessions cannot acquire a lock with that name until the acquiring session releases all its locks for the name.

vroldanbet · 2024-12-11T10:19:21Z

internal/datastore/postgres/locks.go

+)
+
+func (pgd *pgDatastore) tryAcquireLock(ctx context.Context, lockID lockID) (bool, error) {
+	// Acquire the lock.


Please link to: https://www.postgresql.org/docs/current/explicit-locking.html#ADVISORY-LOCKS

Please also mention that pg locks are reentrant, but with a caveat

A lock can be acquired multiple times by its owning process; for each completed lock request there must be a corresponding unlock request before the lock is actually released ... If a session already holds a given advisory lock, additional requests by it will always succeed, even if other sessions are awaiting the lock; this statement is true regardless of whether the existing lock hold and new request are at session level or transaction level.

vroldanbet · 2024-12-11T10:31:29Z

internal/datastore/postgres/locks.go

+	_, err := pgd.writePool.Exec(ctx, `
+		SELECT pg_advisory_unlock($1)
+	`, lockID)


Please check the result. If it returns false, we know something is wrong, so we should at least log it.

josephschorr requested review from vroldanbet and a team as code owners December 9, 2024 20:46

github-actions bot added area/datastore Affects the storage system area/dependencies Affects dependencies area/tooling Affects the dev or user toolchain (e.g. tests, ci, build tools) labels Dec 9, 2024

josephschorr changed the title ~~Have Postgres GC lock so that it only runs on a single node at a time~~ Postgres GC improvements: single node GC and a new index Dec 9, 2024

josephschorr force-pushed the postgres-gc-lock branch 4 times, most recently from a96f3fb to 62550af Compare December 9, 2024 22:00

vroldanbet reviewed Dec 10, 2024

View reviewed changes

josephschorr force-pushed the postgres-gc-lock branch from 62550af to 9fcc5c1 Compare December 10, 2024 21:10

josephschorr changed the title ~~Postgres GC improvements: single node GC and a new index~~ GC improvements: GC only on a single node and add a missing index in PG Dec 10, 2024

Garbage Collection improvements in MySQL and Postgres

c5d52b1

1) Have GC lock so that it only runs on a single node at a time 2) Add a missing index in the Postgres datastore for GC This should reduce datastore CPU pressure

josephschorr force-pushed the postgres-gc-lock branch from 9fcc5c1 to c5d52b1 Compare December 10, 2024 21:12

ecordell previously approved these changes Dec 10, 2024

View reviewed changes

josephschorr dismissed ecordell’s stale review via 58c7602 December 10, 2024 21:58

josephschorr force-pushed the postgres-gc-lock branch 3 times, most recently from 11e2df9 to 62c8697 Compare December 10, 2024 22:24

Move GC tests in PG out of parallel running

b7d77b7

This is necessary because GC takes an exclusive lock now

josephschorr force-pushed the postgres-gc-lock branch from 405bec5 to b7d77b7 Compare December 10, 2024 23:48

vroldanbet reviewed Dec 11, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

GC improvements: GC only on a single node and add a missing index in PG #2159

GC improvements: GC only on a single node and add a missing index in PG #2159

josephschorr commented Dec 9, 2024 •

edited

Loading

vroldanbet Dec 10, 2024

vroldanbet Dec 10, 2024

vroldanbet Dec 10, 2024

vroldanbet Dec 10, 2024

vroldanbet Dec 10, 2024

vroldanbet Dec 10, 2024

vroldanbet Dec 10, 2024

josephschorr Dec 10, 2024

josephschorr commented Dec 10, 2024

ecordell left a comment

vroldanbet left a comment

vroldanbet Dec 11, 2024

vroldanbet Dec 11, 2024

vroldanbet Dec 11, 2024

vroldanbet Dec 11, 2024

vroldanbet Dec 11, 2024

vroldanbet Dec 11, 2024

	func RunWithLocksClient(conn pgx.Conn, runner func(client pglock.Client) error) error {
	func RunWithLocksClientForMigrations(conn pgx.Conn, runner func(client pglock.Client) error) error {

GC improvements: GC only on a single node and add a missing index in PG #2159

Are you sure you want to change the base?

GC improvements: GC only on a single node and add a missing index in PG #2159

Conversation

josephschorr commented Dec 9, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

josephschorr commented Dec 10, 2024

ecordell left a comment

Choose a reason for hiding this comment

vroldanbet left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

josephschorr commented Dec 9, 2024 •

edited

Loading