Skip to content
This repository has been archived by the owner on Jan 9, 2023. It is now read-only.

Fixes a Pulp2Content unique-constraint that wasn't very unique. #331

Merged
merged 1 commit into from
Mar 9, 2021

Conversation

ggainey
Copy link
Collaborator

@ggainey ggainey commented Mar 9, 2021

In Postgres, all NULLs are unique - so if a nullable field is included
in a UiniqueConstraint, it allows rows that differ only by the NULL.

Build two constraints, one when the 'optional' field is NULL, and
one where it has content.

fixes #8329
[nocoverage]

@ggainey ggainey added the WIP label Mar 9, 2021
@ggainey
Copy link
Collaborator Author

ggainey commented Mar 9, 2021

This is in draft for two reasons:

First - Making the UQ 'correct' means we probably do NOT need to make sure errata store repo_id in subid. After discussion around this approach, we may remove the Pulp2Content(type=erratum).update() from the migration, and the code that copies repo-id into subid.

Second, and more painful - with the corrected UQ in place, the reproducer from https://pulp.plan.io/issues/8329#note-3 fails with the stacktrace noted in comment-10. Discussion needed on how we address the problem.

@ggainey
Copy link
Collaborator Author

ggainey commented Mar 9, 2021

Second, and more painful - with the corrected UQ in place, the reproducer from https://pulp.plan.io/issues/8329#note-3 fails with the stacktrace noted in comment-10. Discussion needed on how we address the problem.

bulk_create(pulp2content, ignore_conflicts=True) causes more trouble than it's worth :(

In Postgres, all NULLs are unique - so if a nullable field is included
in a UiniqueConstraint, it allows rows that differ only by the NULL.

Build two constraints, one when the 'optional' field is NULL, and
one where it has content.

fixes #8329
[nocoverage]
@ggainey
Copy link
Collaborator Author

ggainey commented Mar 9, 2021

This is in draft for two reasons:

First - Making the UQ 'correct' means we probably do NOT need to make sure errata store repo_id in subid. After discussion around this approach, we may remove the Pulp2Content(type=erratum).update() from the migration, and the code that copies repo-id into subid.

Second, and more painful - with the corrected UQ in place, the reproducer from https://pulp.plan.io/issues/8329#note-3 fails with the stacktrace noted in comment-10. Discussion needed on how we address the problem.

Both of these issues should now be resolved.

Copy link
Member

@goosemania goosemania left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you! :octocat:

@ggainey ggainey removed the WIP label Mar 9, 2021
@ggainey ggainey merged commit 86f0f99 into pulp:master Mar 9, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants