Custom violating queries should be checked for correctness

[alanking]

please consider an upgrade - can we auto detect and replace this specific query? does that tie too much into the main server since this is a plugin? do plugins have an upgrade hook where they can do their own migration tasks?

_Originally posted by @trel in https://github.com/irods/irods_capability_storage_tiering/pull/230#discussion_r1408117300_

---

Due to changes in #228 / #230, any installation using storage tiering will have to tweak any non-default violating queries on upgrade for storage tiering to continue to work because the user's zone name is now required to be returned from the query.

Please investigate whether / how to do this automatically on upgrade.

[korydraughn]

One possibility is to make the plugin check the queries on plugin start. We know the violating query must request N columns in a specific order. That means we can easily parse the select-clause of the violating query.

Now, this only works for the violating query. If there are other queries, this may not be the right approach.

Or, we just document the changes very well since providing custom queries is a feature of the plugin. Perhaps the README grows a section detailing the query-related changes.

[trel]

on plugin start

that means... every agent every time?

[korydraughn]

Yes. That's one downside, but the price may be negligible because we're just splitting strings. No database interaction needed. This can also be accomplished without allocating memory.

[trel]

oh i see... we'd just be checking the query we've already gathered, and making sure it's okay before running it...

and if it has the wrong number of columns... we ... do what?

[korydraughn]

We log the problem very well and stop the agent.

[korydraughn]

We don't allow the system to proceed without the admin stepping in and correcting the issue.

[trel]

got it. hard stop because known-broken.

[alanking]

So... the answer to the question seems to be "No, the queries should not be updated on upgrade, and we will make detection of incorrect queries better."?

[korydraughn]

That is just one proposal. Definitely open to other ideas.

[alanking]

Attempting to do this automatically on upgrade is fraught, especially if we are intending to include more fixes in a new 4.2.12.x release. If the plugin is upgraded as part of a server upgrade, the server will not be running when the plugin is upgraded, so any interactions involving the server will fail in that case (e.g. scanning resources for relevant metadata).

I think making a good error message when the query is wrong and documenting the change in the release notes is sufficient, personally.

We could provide a helper script (a la /var/lib/irods/scripts/update_deprecated_database_columns.py) to perform this task after upgrade should the administrator choose to do so in an semi-automated fashion.

[alanking]

Speaking of upgrades, because we allow installing/upgrading plugins without stopping the server, there could be an issue if data is actively being scheduled for migration and the upgrade occurs. In particular, the query delta here will result in failures.

[alanking]

After some more discussion, we have decided that we will not update the custom violating queries on upgrade. We will examine the custom violating query right before it is going to be used and determine whether the right columns are being selected and in the right order. If it is not found to be in the expected state, we will return an error and log a detailed message describing what is wrong and how to fix it.

[alanking]

Just for future reference...

Looks like the option for checking the number of columns is already implemented: https://github.com/irods/irods_capability_storage_tiering/blob/6f3823697da754a3596643c9323bb75ed008add0/storage_tiering.cpp#L598-L606

The easy/not-"smart" approach would be to just update the 4 to a 5, but we want to ensure that the correct columns are being selected in the correct order as well.

[trel]

And remember, there could be more than one violating query...

[trel]

And I'm concerned about that < 4... feels like it should always be 4, or rather, now... very equal to 5.

And in the right order...

[alanking]

After thinking on it some more and some discussion, we've decided that tightening up that check is all we can really do here. Specific queries cannot be reasonably parsed so all we can do is log the query being run to signal to the administrator that something is wrong.

So, we will update the number of columns check to look for exactly 5 instead of "at least 4" and log a detailed message when this is found to not be the case.