-
Notifications
You must be signed in to change notification settings - Fork 61
OPFS Multiple Tab Trigger Invocation #804
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
base: main
Are you sure you want to change the base?
Conversation
🦋 Changeset detectedLatest commit: 5ea40a2 The changes in this PR will be included in the next version bump. This PR includes changesets to release 8 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
simolus3
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm happy with this approach. My main comment is that I'm wondering whether we should unconditionally use stored triggers to simplify our internal logic.
| /** | ||
| * @experimental | ||
| * @internal | ||
| * An interface which exposes which persisted managed SQLite triggers and destination SQLite tables | ||
| * are actively in use. Resource which are not reported as claimed by this interface will be disposed. | ||
| */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The PR description gives a very nice overview of what this interface is supposed to accomplish, I think copying parts of the "The Claim Mechanism" section into this documentation comment might make the purpose of this easier to understand for someone stumbling upon this later. Or at least for me this comment is a bit abstract and doesn't make it obvious why this is necessary.
| */ | ||
| export class MemoryTriggerClaimManager implements TriggerClaimManager { | ||
| // Uses a global store to share the state between potentially multiple instances | ||
| private static CLAIM_STORE = new Map<string, () => Promise<void>>(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: Having a class whose only internal state is a static field feels a bit odd to me, perhaps a global constant implementing TriggerClaimManager as a JS object would make that clearer.
|
|
||
| if (operations.includes(DiffTriggerOperation.INSERT)) { | ||
| const insertTriggerId = `ps_temp_trigger_insert_${id}`; | ||
| const insertTriggerId = this.generateTriggerName(DiffTriggerOperation.INSERT, destination, id); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wouldn't cleanupResources also run for temporary triggers since they have same naming convention? These temporary triggers aren't registered in the claims tracker, so couldn't that lead to them being dropped prematurely?
I guess we're protected from this since we only select from sqlite_master which only includes tables from the main schema. But it would be good to mention this in a comment somewhere.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess we're protected from this since we only select from sqlite_master which only includes tables from the main schema. But it would be good to mention this in a comment somewhere.
This is correct, those triggers won't be returned from sqlite_master. I will add more comments for this.
| * Use storage-backed (non-TEMP) tables and triggers that persist across sessions. | ||
| * These resources are still automatically disposed when no longer claimed. | ||
| */ | ||
| useStorage?: boolean; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Given that we have mechanisms to clean up those triggers reliably (albeit after a small delay), I wonder if it could make sense to unconditionally use stored triggers and to drop support for in-memory triggers?
The only benefit of in-memory triggers is potentially better performance. But not having this as an option would allow us to remove additional complexity in opening logic added to the web database. So if the performance benefit is not that significant, reducing implementation complexity might be a good tradeoff.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's a fair point to mention this.
In my opinion, the added complexity of configuring defaults is worth the potential improved performance.
There probably are a few scenarios where we want to avoid increasing the storage size. Two which come to mind are:
- The
SQL.jsadapter which exports the entire DB to be persisted after eachwriteLock. - The IndexedDB VFS which has historically shown that performance suffers when the database size increases.
We could potentially perform the inverse, where storage is used by default and disabled for those cases - but then we still need to be able to configure the defaults and lose some of those potential performance benefits.
| // (add 2 to account for removed leading '__', plus 2 to skip the '__' before destination) | ||
| // - Destination length excludes the trailing '__' + 36-char UUID: length(name) - _start_index - 37 | ||
| // - UUID is always last 36 chars | ||
| const trackedItems = await ctx.getAll<TrackedTableRecord>(/* sql */ ` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: Just doing SELECT * FROM sqlite_schema WHERE type = 'trigger' AND name LIKE '__ps_temp_trigger_%' and then extracting the table name in JS would probably be easier to understand.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's a fair point. I did consider doing this. A second opinion helps here. I'll update the implementation.
| if (trackedItems.length == 0) { | ||
| // There is nothing to cleanup | ||
| return; | ||
| } | ||
|
|
||
| for (const trackedItem of trackedItems) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need this check? There's nothing after the loop we might have to skip if there aren't any items.
| if (trackedItems.length == 0) { | |
| // There is nothing to cleanup | |
| return; | |
| } | |
| for (const trackedItem of trackedItems) { | |
| for (const trackedItem of trackedItems) { |
| const triggerNames = Object.values(DiffTriggerOperation).map((operation) => | ||
| this.generateTriggerName(operation, trackedItem.table, trackedItem.id) | ||
| ); | ||
| for (const triggerName of triggerNames) { | ||
| // The trigger might not actually exist, we don't track each trigger name and we test all permutations | ||
| await ctx.execute(`DROP TRIGGER IF EXISTS ${triggerName}`); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The query iterates over all triggers, finds the unique affected table names and then this method again expands that to all possible trigger names to delete? Would it be easier to remove the DISTINCT, add the full trigger name as a select column and then just delete that? That means we'd be running DROP TABLE IF EXISTS multiple times for the same table if there are multiple triggers but that should be harmless.
| import { TriggerClaimManager } from '@powersync/common'; | ||
| import { getNavigatorLocks } from '../shared/navigator'; | ||
|
|
||
| export class NavigatorTriggerClaimManager implements TriggerClaimManager { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: Since this also has no internal state it could also be an export const navigatorTriggerClaimManager: TriggerClaimManager = {...}.
Overview
Fixes:
Our SQLite trigger helpers currently allow for tracking INSERT, UPDATE, and DELETE operations on a PowerSync view. These operations are stored in a SQLite table for each managed consumer to process.
Our managed triggers currently only support temporary SQLite triggers, where diff operations are stored in temporary SQLite tables. This approach avoids extra storage overhead for diff records and simplifies cleanup. The temporary nature means the triggers and tables are scoped to the SQLite connection and are disposed automatically once the connection is closed.
The Problem
For OPFS (in Web), we create a separate SQLite connection per tab. Triggers are also scoped to each tab, so each tab creates its own managed trigger (hence its own SQLite trigger and destination table). Having multiple connections across multiple tabs causes issues:
A mutation performed by one connection does not trigger a temporary trigger defined for another connection.
This causes our trigger consumers to miss updates from other tabs.
The Solution
Persisted SQLite triggers are stored in the database. A mutation from any connection will trigger all persisted triggers, ensuring that mutations from other tabs are written to the destination table of other tabs' managed triggers.
This PR adds the ability to optionally use persisted SQLite tables and triggers (enabled by default for OPFS connections). While these resources do use persistence, they are still temporary in nature. The PowerSync SDK will dispose of these resources automatically when they are detected as no longer in use.
Implementation
Using persisted triggers and destination tables solves the multiple SQLite connection problem, but introduces a new challenge: how do we dispose of the persisted tables and triggers when they are no longer needed?
The Claim Mechanism
Disposing stale items for closed tabs (and SQLite connections) is not straightforward. When a tab is closed, there is usually no reliable method for ensuring cleanup has completed successfully.
To address this, the implementation introduces a "claim" mechanism through a new
TriggerClaimManagerinterface. This interface provides two core capabilities: obtaining a claim on a resource (returning a release callback), and checking whether a resource is currently claimed.A heartbeat or time-to-live mechanism was also considered, but this approach seemed vulnerable to slow or frozen tab issues. A tab that's temporarily unresponsive could have its resources incorrectly cleaned up. The hold method avoids this problem since a hold remains valid regardless of how responsive the owning tab is.
The resources and claims to resources are freed if the trigger is disposed using the callback returned from the method which created it.
For cases where the triggers are not explicitly disposed, a cleanup process is implemented. This process runs whenever a new
PowerSyncDatabaseis created and at a 2 minute interval.A unique UUID is associated with each managed trigger. This ID and destination SQLite table are encoded into the names of the SQLite triggers created. The naming convention for triggers is as follows:
__ps_temp_trigger_${operation}__${destination_table}__${trigger_id}The cleanup process:
sqlite_mastertable for all triggers which match the__ps_temp_triggernaming convention. The destination table and trigger id are extractedTriggerHoldManagerif any active claims are present for the trigger idAlternative approaches were considered for the tracking of the managed trigger Ids and tables.
ps_kv: Cant be used since the keys inps_kvare cleared indisconnectAndClear`, which will discard the state.Platform-Specific Claim Managers
The claim mechanism needs to work differently depending on the platform.
For shared-memory SDKs like Node and React Native, a
MemoryTriggerClaimManageruses a global in-memory store to track claims. Since these environments typically share memory within a single process, this straightforward approach works well.For web environments, a
NavigatorTriggerClaimManagerleverages the Navigator Locks API to manage claims across browser tabs. This allows us to determine if any tab is still holding on to a trigger, even when those tabs can't directly communicate with each other.The web
PowerSyncDatabaseenables persistence by default for OPFS VFS modes, while keeping temporary triggers forIDBBatchAtomicVFSwhere multiple connections aren't an issue.