fix: mark generateIdentifierFromStoredProcedure functions as VOLATILE

[vera]

What this PR does / why we need it:

This PR makes two changes:

• It updates the two example generateIdentifierFromStoredProcedure functions shown in the docs to be VOLATILE instead of IMMUTABLE.
• It adds a Flyway migration for existing generateIdentifierFromStoredProcedure functions to make them VOLATILE.

The functions cannot be IMMUTABLE, because they would need to be "guaranteed to return the same results given the same arguments forever" [1], which neither of them are.

In the documented example functions, now() and nextval('datasetidentifier_seq') return different values over time, so the function's output changes even though it takes no arguments. In general, we can assume that any implementation of generateIdentifierFromStoredProcedure should return a different result every time you call it, because it's supposed to generate unique identifiers.

When the DB assumes the functions are IMMUTABLE, it may cache the function result and occasionally just return the same cached identifier over and over. This would cause the loop in generateIdentifierFromStoredProcedureIndependent to get stuck.

See also: my Zulip post, which has a bit more detail.

[1] https://www.postgresql.org/docs/current/xfunc-volatility.html

Which issue(s) this PR closes:

None

Special notes for your reviewer:

/

Suggestions on how to test this:

For confirming this bug, you could set up a Dataverse with a PID provider that uses "storedProcGenerated" and then create a number of datasets via API in quick succession.

Without the change in this PR, the "create dataset" API sometimes gets stuck and doesn't return a response.

With this change, it no longer gets stuck.

Does this PR introduce a user interface change? If mockups are available, please link/include them here:

/

Is there a release notes update needed for this change?:

I added a short release note

Additional documentation:

/

[qqmyers]

Overall, this makes sense - a nice catch! I have two comments:

• I think the Flyway name has to update to V6.11* - whatever's next in the sequence
• I made a separate note questioning whether the time-based example method still has the potential for a loop as it appears that now() is STABLE and can return the same value within the same transaction. I'm not sure what the answer is and agree that the update already in the PR is better than leaving it IMMUTABLE, so I'm OK with calling further change out-of-scope.

I'll mark this as Request changes just to keep it in review for now, but I don't think it needs re-review if all that changes is the Flyway script name and could just go forward unless @vera wants to update the identifier_from_time example.

[qqmyers]

According to https://aws.amazon.com/blogs/database/volatility-classification-in-postgresql/ (search for now() ) - now() is STABLE. Does that mean either that this method can be STABLE rather than VOLATILE or that something VOLATILE is needed here (e.g. clock_timestamp() )? Marking this method as VOLATILE seems like it would fix the main issue you saw, but I don't know if there's still a potential loop if we keep calling now() within one transaction.)

(In contrast nextval() used above is VOLATILE, so making the calling function VOLATILE as well make sense.)

[vera]

Hmm. That's a good point. I assume this means that yes, to truly prevent potential loops, we'd need to use something other than now() here.

I assume that the loop in generateIdentifierFromStoredProcedureIndependent() that keeps calling the stored procedure until a unique PID is returned runs within a single DB transaction. So if now() always returns the same timestamp within one transaction, the loop would still get stuck, even if we've marked the function as VOLATILE now.

With one side note: that would require two DB transactions whose start timestamps round/truncate to the same millisecond, otherwise two executions of curr_time_msec := extract(epoch from now())*1000; shouldn't return the same value anyways, right? That might be a bit unlikely.

But I think the fix may really be as simple as exchanging now() for clock_timestamp():

dataverse=# begin;
BEGIN
dataverse=*# select extract(epoch from now())*1000;
       ?column?
----------------------
 1784209780161.016000
(1 row)

dataverse=*# select extract(epoch from now())*1000;
       ?column?
----------------------
 1784209780161.016000
(1 row)

dataverse=*# select extract(epoch from now())*1000;
       ?column?
----------------------
 1784209780161.016000
(1 row)

dataverse=*# select extract(epoch from clock_timestamp())*1000;
       ?column?
----------------------
 1784209788986.411000
(1 row)

dataverse=*# select extract(epoch from clock_timestamp())*1000;
       ?column?
----------------------
 1784209791351.397000
(1 row)

[qqmyers]

If you're willing, I'd suggest going ahead and making this change. If there is a problem, this is just an example in the guides so it would be easy for someone to revert. I'd also suggest just noting that people using the identifier_from_timestamp function may want to update to the new one in the release notes (versus trying to automate that in flyway - hard to know if someone has made minor tweaks from what we've suggested unless you check the whole method).

I'm not sure what level of QA is planned - seems easy enough to just try installing both scripts and regression testing that both work to generate PIDs. I don't know how hard it would be to trigger the bug to see if both VOLATILE and clock_timestamp() are needed.

[vera]

Just tested the function with the clock_timestamp change and PID generation worked:

$ curl -X POST localhost:8080/api/dataverses/root/datasets -H "X-Dataverse-key: c1c02563-1f56-4c9e-b90e-9cedca401e90" -H "Content-Type: application/json" --data @/home/vera/Documents/dataverse/src/test/resources/json/dataset-finch1.json
{"status":"OK","data":{"id":2,"persistentId":"perma:test-mrnmlz05"}}

So I'll push the change.

Re: triggering the bug, for me it previously worked to simply execute that curl command a bunch of times in quick succession (see https://dataverse.zulipchat.com/#narrow/channel/379673-dev/topic/generateIdentifierFromStoredProcedure.20as.20a.20VOLATILE.20function/with/594230064).

[vera]

• I think the Flyway name has to update to V6.11* - whatever's next in the sequence

Done.

• I made a separate note questioning whether the time-based example method still has the potential for a loop as it appears that now() is STABLE and can return the same value within the same transaction. I'm not sure what the answer is and agree that the update already in the PR is better than leaving it IMMUTABLE, so I'm OK with calling further change out-of-scope.

Replied above. I think we could take the extra step to prevent potential loops by switching from now() to clock_timestamp(). It seems like a small enough change to me that I'd be fine with including it in this PR. But I haven't tested the changed function in an actual running Dataverse, so I can't be completely sure the change wouldn't break anything (edit: have tested and pushed that change now, see above).