ptrdom commented on code in PR #515: URL: https://github.com/apache/pekko-persistence-r2dbc/pull/515#discussion_r4422897906
########## ddl-scripts/migration/snapshot_start_from_snapshot_postgres.sql: ########## @@ -0,0 +1,13 @@ +-- Adds the snapshot columns and index that are needed by the queries that use snapshots as starting points +-- (eventsBySlicesStartingFromSnapshots, eventsByPersistenceIdStartingFromSnapshot and their current variants). +-- Use this for both the BYTEA and the JSONB schema. See the migration guide for a rolling update procedure. + +ALTER TABLE snapshot ADD COLUMN IF NOT EXISTS db_timestamp timestamp with time zone; +ALTER TABLE snapshot ADD COLUMN IF NOT EXISTS tags TEXT ARRAY; + +-- populate the new columns from the corresponding events +UPDATE snapshot s SET db_timestamp = e.db_timestamp, tags = e.tags + FROM event_journal e + WHERE s.persistence_id = e.persistence_id AND s.seq_nr = e.seq_nr; Review Comment: Following up on my own suggestion here: filtering `deleted = false` has a side effect I missed. `deleteMessages(toSeqNr)` replaces the row at `toSeqNr` with a delete marker (`deleted = true`). So with the common "snapshot, then `deleteMessages(snapshotSeqNr)`" pattern, the update no longer matches and the snapshot's `db_timestamp` stays NULL. The by-slice snapshot query filters on `db_timestamp >= ?`, so that snapshot is never emitted. Its events are deleted as well, so `eventsBySlicesStartingFromSnapshots` misses the entity completely. I reproduced it on PostgreSQL. I created the old schema and wrote two entities with the feature disabled: one that saves a snapshot at seq 2 and then calls `deleteMessages(2)`, and one that only saves a snapshot. After running the migration script with the feature enabled, `currentEventsBySlicesStartingFromSnapshots` emitted only the second entity's snapshot. The first entity's snapshot had `db_timestamp` NULL. I still think the `deleted = false` filter is right, since a delete marker isn't the snapshot's event. Its timestamp is the time of the delete and it has no tags. Dropping the filter does make this case work, with the marker's timestamp. But it doesn't help when events were deleted past the snapshot's seqNr. I ran that case too: snapshot at seq 2, then `deleteMessages(3)`. The marker is at seq 3, the join matches nothing, and the snapshot stays NULL and is missed with or without the filter. `saveAsync`, the `MigrationTool` and `collectSerializedSnapshot` all fall back to `write_timestamp` when the event isn't found. Could the scripts do the same after the join update? For example: ```sql -- snapshots whose event is deleted or missing, same fallback as the snapshot plugin UPDATE snapshot SET db_timestamp = to_timestamp(write_timestamp / 1000.0) WHERE db_timestamp IS NULL; ``` With this added, the snapshot was emitted in both experiments. MySQL could use `FROM_UNIXTIME(write_timestamp / 1000)`. Then the migration guide should say that snapshots without a matching event get the snapshot's write timestamp and no tags. The same applies to the Yugabyte and MySQL scripts. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
