Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Avoid second ExecuteContext for native implementations of RETURNING #14306

Closed
12 tasks done
lukaseder opened this issue Nov 25, 2022 · 9 comments
Closed
12 tasks done

Avoid second ExecuteContext for native implementations of RETURNING #14306

lukaseder opened this issue Nov 25, 2022 · 9 comments
Labels
C: Functionality E: All Editions P: Medium R: Fixed T: Defect T: Incompatible change
Projects
3.19 Other improvements
Milestone
Version 3.19.0

Comments

@lukaseder
Copy link
Member

lukaseder commented Nov 25, 2022 •
edited

The RETURNING clauses's native implementations are fetched using a second ExecuteContext in AbstractDMLQuery. There doesn't seem to be any obvious reason why this would be done.

It makes sense for those dialects where as separte SELECT query needs to be issued, but not when a JDBC ResultSet is available.

This should affect:

  • CockroachDB (RETURNING)
  • Db2 (FINAL TABLE)
  • Firebird (RETURNING)
  • H2 (FINAL TABLE)
  • HANA (Full JDBC generated keys support)
  • HSQLDB (Full JDBC generated keys support)
  • MariaDB (RETURNING)
  • Oracle (RETURNING, or Full JDBC generated keys support)
  • PostgreSQL (RETURNING)
  • SQL Server (OUTPUT)
  • YugabyteDB (RETURNING)

Regression tests don't indicate anything wrong when reusing the original ExecuteContext. But this change could still lead to subtle regressions, so I won't backport it.

Various other, related fixes include:

  • Set the ResultSet on ExecuteContext::resultSet
@lukaseder lukaseder added this to the Version 3.18.0 milestone Nov 25, 2022
@lukaseder lukaseder added this to To do in 3.18 Other improvements via automation Nov 25, 2022
@lukaseder
Copy link
Member Author

Marked as incompatible change as someone might rely on several ExecuteListener::start events, or similar

@lukaseder
Copy link
Member Author

The reason for the current implementation was that otherwise, we'd get too many end events, as the end events are generated eagerly when closing a ResultSet.

This can surely be addressed somehow

@lukaseder lukaseder removed this from To do in 3.18 Other improvements Mar 2, 2023
@lukaseder
Copy link
Member Author

lukaseder commented May 24, 2023 •
edited

See also:

This was referenced May 24, 2023
Closed
Closed
Closed
Closed
@lukaseder lukaseder added this to To do in 3.19 Other improvements via automation May 24, 2023
lukaseder added a commit that referenced this issue May 24, 2023
@lukaseder
[#14306] Avoid second ExecuteContext for native implementations of RE…
04216a1 
…TURNING
@lukaseder
Copy link
Member Author

This produced no regressions in any of the integration tests and leads to more straightforward results, which is great!

3.19 Other improvements automation moved this from To do to Done May 24, 2023
lukaseder added a commit that referenced this issue May 25, 2023
@lukaseder
[#14306] ResultSet local variable is no longer needed
888a619
@lukaseder lukaseder mentioned this issue May 25, 2023
Open
11 tasks
@lukaseder
Copy link
Member Author

Related:

@lukaseder
Copy link
Member Author

This change has introduced a regression of #13574 for Oracle

lukaseder added a commit that referenced this issue May 26, 2023
@lukaseder
[#14306] Fix a regression of [##13574]
28f6261
@lukaseder
Copy link
Member Author

lukaseder commented Jun 1, 2023 •
edited

Another regression has surfaced (I suspect). Multiple tests have unmatched open/close events in the logs (more close than open):

12:04:12,909  INFO [Lifecycle                     ] - CONNECTION PROVIDER LIFECYCLE STATS
12:04:12,909  INFO [Lifecycle                     ] - -----------------------------------
12:04:12,909  INFO [Lifecycle                     ] - Unbalanced               : (open, close): (  1,   3) at testExecuteListenerRunningJDBCStatements
12:04:12,909  INFO [Lifecycle                     ] - Unbalanced               : (open, close): (  2,   1) at testExecuteListenerSkipPrepareStatement
12:04:12,910  INFO [Lifecycle                     ] - Unbalanced               : (open, close): (  3,   4) at testThreadLocalTransactionsNestingOrdinaryTransactions
12:04:12,910  INFO [Lifecycle                     ] - Unbalanced               : (open, close): (  2,   3) at testTransactionsWithExecuteListener
12:04:12,910  INFO [Lifecycle                     ] -                          
12:04:12,910  INFO [Lifecycle                     ] - EXECUTE LISTENER LIFECYCLE STATS
12:04:12,910  INFO [Lifecycle                     ] - --------------------------------
12:04:12,910  INFO [Lifecycle                     ] - Unbalanced               : (open, close): ( 15,  18) at testAdHocConverterFrom
12:04:12,910  INFO [Lifecycle                     ] - Unbalanced               : (open, close): ( 10,  12) at testAttachable
... more

These tests aren't part of the normal integration test suite, which is why the feedback was delayed. Perhaps they should be.

@lukaseder
Copy link
Member Author

These are the call stacks:

Open

Thread [main] (Suspended (breakpoint at line 66 in LifecycleWatcherListener))	
	LifecycleWatcherListener.start(ExecuteContext) line: 66	
	ExecuteListeners.start(ExecuteContext) line: 160	
	InsertQueryImpl<R>(AbstractQuery<R>).execute() line: 283	
	TBookStoreRecord(TableRecordImpl<R>).storeInsert0(Field<?>[]) line: 197	
	TBookStoreRecord(TableRecordImpl<R>).lambda$0(int[], Field[], Record) line: 163	
	0x00000008006bdf68.apply(Object) line: not available	
	RecordDelegate<R>.operate(ThrowingFunction<R,R,E>) line: 144	
	TBookStoreRecord(TableRecordImpl<R>).storeInsert(Field<?>[]) line: 162	
	TBookStoreRecord(UpdatableRecordImpl<R>).store0(Field<?>[]) line: 222	
	TBookStoreRecord(UpdatableRecordImpl<R>).lambda$0(int[], Field[], Record) line: 149	
	0x00000008006bdd48.apply(Object) line: not available	
	RecordDelegate<R>.operate(ThrowingFunction<R,R,E>) line: 144	
	TBookStoreRecord(UpdatableRecordImpl<R>).store(Field<?>...) line: 148	
	TBookStoreRecord(UpdatableRecordImpl<R>).store() line: 140	
	GeneralTests<A,B,S,B2S,BS>.testAttachable() line: 152	
	PostgresTest(jOOQAbstractTest<A,B,S,B2S,BS>).testAttachable() line: 9191

Close 1

Thread [main] (Suspended (breakpoint at line 71 in LifecycleWatcherListener))	
	LifecycleWatcherListener.end(ExecuteContext) line: 71	
	ExecuteListeners.end(ExecuteContext) line: 298	
	Tools.safeClose(ExecuteListener, ExecuteContext, boolean, boolean) line: 3585	
	CursorImpl$CursorResultSet.close() line: 380	
	JDBCUtils.safeClose(ResultSet) line: 643	
	CursorImpl<R>.close() line: 184	
	CursorImpl$CursorIterator.fetchNext() line: 1413	
	CursorImpl$CursorIterator.hasNext() line: 1365	
	CursorImpl<R>.fetchNext(int) line: 173	
	CursorImpl<R>(AbstractCursor<R>).fetch(int) line: 177	
	CursorImpl<R>(AbstractCursor<R>).fetch() line: 88	
	InsertQueryImpl<R>(AbstractDMLQuery<R>).execute(ExecuteContext, ExecuteListener) line: 1227	
	InsertQueryImpl<R>(AbstractQuery<R>).execute() line: 359	
	TBookStoreRecord(TableRecordImpl<R>).storeInsert0(Field<?>[]) line: 197	
	TBookStoreRecord(TableRecordImpl<R>).lambda$0(int[], Field[], Record) line: 163	
	0x00000008006bdf68.apply(Object) line: not available	
	RecordDelegate<R>.operate(ThrowingFunction<R,R,E>) line: 144	
	TBookStoreRecord(TableRecordImpl<R>).storeInsert(Field<?>[]) line: 162	
	TBookStoreRecord(UpdatableRecordImpl<R>).store0(Field<?>[]) line: 222	
	TBookStoreRecord(UpdatableRecordImpl<R>).lambda$0(int[], Field[], Record) line: 149	
	0x00000008006bdd48.apply(Object) line: not available	
	RecordDelegate<R>.operate(ThrowingFunction<R,R,E>) line: 144	
	TBookStoreRecord(UpdatableRecordImpl<R>).store(Field<?>...) line: 148	
	TBookStoreRecord(UpdatableRecordImpl<R>).store() line: 140	
	GeneralTests<A,B,S,B2S,BS>.testAttachable() line: 152	
	PostgresTest(jOOQAbstractTest<A,B,S,B2S,BS>).testAttachable() line: 9191

Close 2

Thread [main] (Suspended (breakpoint at line 71 in LifecycleWatcherListener))	
	LifecycleWatcherListener.end(ExecuteContext) line: 71	
	ExecuteListeners.end(ExecuteContext) line: 298	
	Tools.safeClose(ExecuteListener, ExecuteContext, boolean, boolean) line: 3585	
	Tools.safeClose(ExecuteListener, ExecuteContext, boolean) line: 3548	
	InsertQueryImpl<R>(AbstractQuery<R>).execute() line: 381	
	TBookStoreRecord(TableRecordImpl<R>).storeInsert0(Field<?>[]) line: 197	
	TBookStoreRecord(TableRecordImpl<R>).lambda$0(int[], Field[], Record) line: 163	
	0x00000008006ac800.apply(Object) line: not available	
	RecordDelegate<R>.operate(ThrowingFunction<R,R,E>) line: 144	
	TBookStoreRecord(TableRecordImpl<R>).storeInsert(Field<?>[]) line: 162	
	TBookStoreRecord(UpdatableRecordImpl<R>).store0(Field<?>[]) line: 222	
	TBookStoreRecord(UpdatableRecordImpl<R>).lambda$0(int[], Field[], Record) line: 149	
	0x00000008006bfc88.apply(Object) line: not available	
	RecordDelegate<R>.operate(ThrowingFunction<R,R,E>) line: 144	
	TBookStoreRecord(UpdatableRecordImpl<R>).store(Field<?>...) line: 148	
	TBookStoreRecord(UpdatableRecordImpl<R>).store() line: 140	
	GeneralTests<A,B,S,B2S,BS>.testAttachable() line: 152	
	PostgresTest(jOOQAbstractTest<A,B,S,B2S,BS>).testAttachable() line: 9191

The second close call is the legitimate one. The first one is premature. It happens because we run through the CursorImpl lazy fetching lifecycle, in case of which the closing of the ExecuteListener lifecycle is delayed until the cursor is closed. But we're only using it as an auxiliary means of re-using code.

I wonder if this is actually a separate bug that hasn't been identified yet. I'll just fix it with this one here, without a backport, due to the delicate changes in lifecycles introduced by this fix.

At the same time, I've noticed that truly lazy execution of DML RETURNING statements isn't currently supported:

@lukaseder
Copy link
Member Author

I wonder if this is actually a separate bug that hasn't been identified yet.

No, it's the same issue. The previous value for CursorImpl.keepResultSet was justified, because we wanted to close (end()) the nested ExecuteListener lifecycle. This is now no longer true.

lukaseder added a commit that referenced this issue Jun 1, 2023
@lukaseder
[#14306] Don't eagerly call ExecuteListener::end anymore
37926b5 
Before fixing this issue, there was a nested ExecuteListener lifecycle, which needed to be ended eagerly. This is no longer true, now that we re-use the DML statement's ExecuteListener lifecycle to fetch the ResultSet from the RETURNING clause.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
C: Functionality E: All Editions P: Medium R: Fixed T: Defect T: Incompatible change
Projects
No open projects
3.19 Other improvements
Done
Milestone
Version 3.19.0
Development

No branches or pull requests
1 participant
@lukaseder