Reimplemented labels as HStore field

[mdellweg]

This solves the imminent performance issues.

fixes #3400

[dralley]

I believe this is going to result in the same kind of downstream impact as the UUID change, which is that plugins with models that inherit directly from BaseModel will think they need migrations when used with a newer pulpcore, and also when they have applied the migrations and are used with older pulpcore.

[mdellweg]

Yes, when we want to introduce it on the BaseModel. That's why i did not do that yet. But yes, we might be bitten again by the abstract base model.

[dralley]

Is the plan to put it on MasterModel only temporarily?

[mdellweg]

Is the plan to put it on MasterModel only temporarily?

I don't have a final Plan. I can even just put it on the very master models we provide label functionality today.

[decko]

Are you thinking about write a test for this filter? (or if makes any sense for testing it)

[mdellweg]

I am not adding net new functionality here. Tests are here: https://github.com/pulp/pulp_file/blob/main/pulp_file/tests/functional/api/test_labels.py#L125

cc @ggainey

[mdellweg]

and here: https://github.com/pulp/pulp-cli/blob/main/tests/scripts/pulp_file/test_label.sh

[ggainey]

Just to clarify for me - we are implementing this at the individual-model-level "for now", for just the models that currently expose labels. "Later", we may rework this to be provided at either Base or Master level, to expand label-access more generally - but that's much more invasive, esp in the migrations, and we want to address the discovered performance-problem "Now". Is that understanding correct?

Other than that - I'll second decko's request for a test for label-filtering.

[dralley]

Just to clarify for me - we are implementing this at the individual-model-level "for now", for just the models that currently expose labels. "Later", we may rework this to be provided at either Base or Master level, to expand label-access more generally - but that's much more invasive, esp in the migrations, and we want to address the discovered performance-problem "Now". Is that understanding correct?

I'd probably go a bit stronger and say that once we pick an approach realistically we won't be able to back down from it. Django doesn't like moving fields from a child to a parent model table one bit. It can probably be done, but not without a flag day for all plugins, e.g. Pulp 4.

[mdellweg]

Just to clarify for me - we are implementing this at the individual-model-level "for now", for just the models that currently expose labels. "Later", we may rework this to be provided at either Base or Master level, to expand label-access more generally - but that's much more invasive, esp in the migrations, and we want to address the discovered performance-problem "Now". Is that understanding correct?

I'd probably go a bit stronger and say that once we pick an approach realistically we won't be able to back down from it. Django doesn't like moving fields from a child to a parent model table one bit. It can probably be done, but not without a flag day for all plugins, e.g. Pulp 4.

Well, Django allows us to move the field from the (non-abstract) master models up in the hierarchy to the abstract MasterModel. But this is already marginally dangerous, because we expose MasterModel in the plugin api making this a breaking change. Adding the field to the abstract BaseModel is definitely breaking existing plugins. This is almost the BaseDistribution dilemma reloaded. Bottom line: Do not change abstract base classes (removing the generic relation is not a database change...).
Upside of this approach: We do not add labels to Content (would not match the pulp philosophy) and other tables like Workers (that could be potentially interesting if we add atomic label change endpoints). We are intentional about the things we can label.

[daviddavis]

This change looks great. 👍 We considered hstore in the original proposal but I assumed that when the postgresql docs mentioned "rarely examined" data, it probably wasn't indexed but it seems that it is indexed.

One thing I wanted to mention though is that the label filter should be able to handle filtering by 6 labels but I suspect there's some problem with the query. It might be worth using QuerySet.explain() to figure out the exact problem but I am pretty sure the problem is that GenericRelationModel isn't indexed AT ALL and it probably should be. From the django docs on GenericForeignKey:

Unlike for the ForeignKey, a database index is not automatically created on the GenericForeignKey, so it’s recommended that you use Meta.indexes to add your own multiple column index. This behavior may change in the future.

[daviddavis]

Actually it looks like the Label model doesn't extend GenericRelationModel so I was wrong. It still is probably worth adding an index to GenericRelationModel though.

Tests already exist that exercise the changing codepath

[ggainey]

We need to get this performance-knee addressed, and this looks like it does the needful. Approved

[decko]

Actually it looks like the Label model doesn't extend GenericRelationModel so I was wrong. It still is probably worth adding an index to GenericRelationModel though.

Maybe an explain could provide a better answer to it. ☝️

And, do the solution proposed by this PR addresses the performance issues of the Label feature?

[mdellweg]

And, do the solution proposed by this PR addresses the performance issues of the Label feature?

Yes, it does:
#3400 (comment)

And we may eventually decide to even add an index to the hstore field if it's still not sufficiently performant.

[decko]

And, do the solution proposed by this PR addresses the performance issues of the Label feature?

Yes, it does: #3400 (comment)

And we may eventually decide to even add an index to the hstore field if it's still not sufficiently performant.

Sorry @mdellweg. The question was not for you. I should have mention @daviddavis.