When deleting SQL journal entries also delete corresponding metadata entries

[hirzraimund]

When deleting persistence journal entries through the common sql implementation also delete the entries in the metadata table. This solves #3205 (violation of primary key constraint when the value already exists) and ensures that there exists at most one entry per PersistenceId value.

Added @ in front of the parameters for the DeleteBatchSql command to be in line with the other usage of AddParameter.

[Aaronontheweb]

Looks good to me. Do we have any specs to verify that the contents of this table have been cleaned up? cc @Horusiath

[Horusiath]

The main reason for the metadata table to exist in the first place is keep the information about the latest sequence number for given persistence id - so that we don't accidentally drop track of actor's sequence number when we decide to delete all events.

This PR in it's current implementation, defeats the whole purpose of having that table at all.

[Aaronontheweb]

Good to know.

[hirzraimund]

I am pretty sure that it still works as intended since in the methods AbstractQueryExecutor.DeleteBatchAsync and in BatchingSqlJournal.HandleDeleteMessagesTo , which are the only places where the delete query is used, the highest sequence number is insert into metadata table if it is inside the deleted range. The insert is done inside the same transaction as the delete and therefore it is ensured that both or none of these commands are executed.

[hirzraimund]

@Horusiath

[Aaronontheweb]

@hirzraimund @Horusiath I'll reopen the PR, but ya'll need to get to the bottom of what's really going on under the covers here before I close it again or merge it :p

Unit test it, IMHO.

[ismaelhamed]

@Aaronontheweb @Horusiath @hirzraimund In the current state, maybe it should be advisable for plugins to resolve this with the proper upsert (i.e, a MERGE in Sql Server and Oracle).

Another option would be to get rid of the Metadata table all together, and make a soft delete on the last event instead (the jdbc plugin way). By keeping just the last event for reference, this keeps track of the highest sequence number in case the user cleans up the journal while remaining as an internal implementation detail.

[Horusiath]

@ismaelhamed We could do a proper upsert in metadata table as a separate PR (only there, as event journal is append only). Regarding removal of Metadata table. It could be done' but I'm not sure if we wouldn't break backward compatibility this way.

[Aaronontheweb]

I'd like to see some sort of spec that covers this change in behavior; I'm sure we might already have one but I'd like to look it over before merging this.

[Aaronontheweb]

LGTM