HHH-14344 - potential fix for insert statement ordering issue related to inheritance

[ngavars]

A potential fix for incorrect cycle detection between insert statement batches, which causes performance degradation when large objects are persisted.

https://hibernate.atlassian.net/browse/HHH-14344

[gavinking]

I guess this impacts Hibernate Reactive.

[ngavars]

By the way - do I need to create a separate pull request for the 5.4 code stream or is master enough?

[ngavars]

I guess this impacts Hibernate Reactive.

I looked at the ReactiveActionQueue and I don't see any cycle detection code in the batch sorter loop. Which means that Hibernate Reactive is probably not affected.

[gavinking]

I guess this impacts Hibernate Reactive.

I looked at the ReactiveActionQueue and I don't see any cycle detection code in the batch sorter loop. Which means that Hibernate Reactive is probably not affected.

Hrm but that might mean that there's some other change we missed porting over. They should probably be identical, don't you think @DavideD?

[ngavars]

Hrm but that might mean that there's some other change we missed porting over. They should probably be identical, don't you think @DavideD?

Cycle detection between batches was added as part of HHH-13068

[gavinking]

I guess this impacts Hibernate Reactive.

I looked at the ReactiveActionQueue and I don't see any cycle detection code in the batch sorter loop.

@ngavars Could you please clarify which lines of code you're talking about that are missing in HR?

Thanks

[ngavars]

@ngavars Could you please clarify which lines of code you're talking about that are missing in HR?

Thanks

If you look at the InsertActionSorter::sort() which is a private class inside the ActionQueue, there is a loop that iterates insert statement batches and sorts them according to dependency graph.

In ActionQueue the following if block is different:

if ( batchIdentifier.hasParent( nextBatchIdentifier ) ) {
  		if ( nextBatchIdentifier.hasParent( batchIdentifier ) ) {
  			//cycle detected, no need to continue
  			break sort;
  		}

  		latestBatches.remove( batchIdentifier );
  		latestBatches.add( j, batchIdentifier );

  		continue sort;
}

when compared to the same code block in ReactiveActionQueue:

if ( batchIdentifier.hasParent( nextBatchIdentifier ) && !nextBatchIdentifier.hasParent(batchIdentifier ) ) {

  		latestBatches.remove( batchIdentifier );
  		latestBatches.add( j, batchIdentifier );

  		continue sort;
}

In some specific cases ActionQueue insert statement batch sorter incorrectly detects a loop between batches and exits without sorting. This pull request is a potential fix for that problem. As for the ReactiveActionQueue - it will not abandon sorting in this case from what I can tell.

[gavinking]

As for the ReactiveActionQueue - it will not abandon sorting in this case from what I can tell.

Hrm. Thanks. This is a little confusing to me, because the fix for HHH-13068 which added those lines was in 2018, apparently, but ReactiveActionQueue was forked in 2019.

So perhaps @DavideD deliberately changed that code when he did the fork .... it's the only explanation I can think of ....

Do you recall anything about this Davide?

[DavideD]

So perhaps @DavideD deliberately changed that code when he did the fork .... it's the only explanation I can think of ....

I don't remember doing anything specific to that class, but it's possible that in the beginning, while prototyping ,I made some mistakes porting it.

@FroMage also did a lot of work to fix my initial implementation of the class. It's possible that something has been lost in the process

[FroMage]

Certainly not changed by design.

[ngavars]

Gents, this PR has been sitting for a while now so I'll just ask here for some guidance. I am not familiar with the process, so excuse the dumb questions. Should someone take interest in the associated Jira ticket, before this PR gets reviewed? Or does it happen the other way around and I should tag some people here instead? Perhaps I can ask @NathanQingyangXu for help, as Nathan helped with a similar issue recently.

[NathanQingyangXu]

As a matter of fact, I am a community contributor and recently I am busy with my own job with little free cycles left.

On Mon., Feb. 8, 2021, 16:26 Normunds Gavars, [email protected] wrote:

Gents, this PR has been sitting for a while now so I'll just ask here for some guidance. I am not familiar with the process, so excuse the dumb questions. Should someone take interest in the associated Jira ticket, before this PR gets reviewed? Or does it happen the other way around and I should tag some people here instead? Perhaps I can @NathanQingyangXu https://github.com/NathanQingyangXu for a help, as Nathan helped with a similar issue recently.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/hibernate/hibernate-orm/pull/3660#issuecomment-775473649, or unsubscribe https://github.com/notifications/unsubscribe-auth/AB6UYAXXTZA2SNCSYRV7SN3S6BJJTANCNFSM4T7ZL7WA .

[ngavars]

No worries, @NathanQingyangXu. I appreciate you taking time to reply. I should probably rephrase my question. What would be the best way to ask someone knowledgable to review my pull request? I am more than happy to jump through the hoops to get this merged myself. I just noticed that there is a chat channel.. I'll try that.

[hibernate-github-bot[bot]]

Thanks for your pull request!

This pull request does not follow the contribution rules. Could you have a look?

❌ All commit messages should start with a JIRA issue key matching pattern HHH-\d+     ↳ Offending commits: [44448286e5808af8cae508c11a088bd50a56fbab]

› This message was automatically generated.