Turning on hard_delete seems to delete more data than intended

[oliviagunton]

While investigating data that was unexpectedly missing after we turned on hard_delete in our config, we noticed that this delete statement seems to delete all rows:

            if self.config["hard_delete"] is True:
                delete_stmt = sa.delete(target_table).where(
                    sa.or_(
                        target_table.c[self.version_column_name].is_(None),
                        target_table.c[self.version_column_name] <= new_version,
                    )
                )

Is it possible that comparison operator should be < rather than <= and it should preserve rows where target_table.c[self.version_column_name] == new_version? (It turns out hard_delete won't work for us for a variety of other reasons related to Redshift, but figured we should say something in case this impacts others.)

[visch]

hmm,

Here's the test we're using to verify this https://github.com/MeltanoLabs/target-postgres/blob/main/target_postgres/tests/test_target_postgres.py#L584-L613 . I've taken a look and it seems fine, but I must admit, I don't typically use hard delete myself. I value your expertise and would like us to work together to reproduce what you're seeing in a test. Once we've done that, we can delve deeper into the issue.

One issue I see with a lot of taps (and this is even seen in our data for the test here https://github.com/MeltanoLabs/target-postgres/blob/main/target_postgres/tests/data_files/test_activate_version_hard.singer is a lot of taps send an activate version message before and after the data is all sent. The issue with this is that it means the steps in order are

1. Truncate the data
2. Populate the table
3. Attempt to get rid of all the old data (should leave the new rows in place)

The reality here is we should probably use the pipeline-wise activate version motto and instead require an activate version message first, and after all the data is sent, follow this workflow:

1. After the first activate version message is sent - create a temp table for the data to go to
2. Populate the temp table (If something fails, it only impacts the temp table, and we don't touch the production table)
3. After the second activate version message is sent - truncate the production table, and insert the temp table (all in on transaction)

[oliviagunton]

Attempt to get rid of all the old data (should leave the new rows in place)

Isn't "new rows" referring to rows where the version number equals new_version? If so, even without an automated test, you can see by reading the logic that those rows will get deleted. (The only rows that would not meet the deletion condition would be rows where version number is greater than new_version.)

Happy to try to track down whatever additional context you would need to construct a test. We're unfortunately not a great candidate for verifying the happy path since hard_delete apparently doesn't work at all with Redshift, but hopefully we can at least change the failure mode from "delete extra data" to "don't delete any data".

[visch]

You're almost certainly right, here's the commit that swapped it a60bfe8

I'm laying out how to fix this for someone, we should fix the test first as the test should not be passing right now.