Do not commit read-only transactions following the C API documentation:

It is not necessary to commit a read-only transaction – you can simply call fdb_transaction_destroy().

No forms of testing were done as I cannot locate a test suite for the Go bindings; would be happy to contribute some in a separate PR.

Code-Reviewer Section

The general pull request guidelines can be found here.

Please check each of the following things and check all boxes before accepting a PR.

[x] The PR has a description, explaining both the problem and the solution.
[x] The description mentions which forms of testing were done and the testing seems reasonable.
[x] Every function/class/actor that was touched is reasonably well documented.

For Release-Branches

If this PR is made against a release-branch, please also check the following:

[ ] This change/bugfix is a cherry-pick from the next younger branch (younger release-branch or main if this is the youngest branch)
[ ] There is a good reason why this PR needs to go into a release branch and this reason is documented (either in the description above or in a linked GitHub issue)

May 06 '24 15:05 gm42

@johscheuer what do you think about starting to add a test suite for the Go binding? (in separate PRs)

May 07 '24 16:05 gm42

Result of foundationdb-pr-macos-m1 on macOS Ventura 13.x

Commit ID: f40de3c2e7cf5796513f65b5adcf6f8f7e3c5ab6
Duration 0:34:16
Result: :white_check_mark: SUCCEEDED
Error: N/A
Build Log terminal output (available for 30 days)
Build Workspace zip file of the working directory (available for 30 days)

May 07 '24 19:05 foundationdb-ci

Result of foundationdb-pr-clang-ide on Linux CentOS 7

Commit ID: f40de3c2e7cf5796513f65b5adcf6f8f7e3c5ab6
Duration 0:39:06
Result: :white_check_mark: SUCCEEDED
Error: N/A
Build Log terminal output (available for 30 days)
Build Workspace zip file of the working directory (available for 30 days)

May 07 '24 19:05 foundationdb-ci

Result of foundationdb-pr-macos on macOS Ventura 13.x

Commit ID: f40de3c2e7cf5796513f65b5adcf6f8f7e3c5ab6
Duration 0:47:02
Result: :white_check_mark: SUCCEEDED
Error: N/A
Build Log terminal output (available for 30 days)
Build Workspace zip file of the working directory (available for 30 days)

May 07 '24 19:05 foundationdb-ci

Result of foundationdb-pr-cluster-tests on Linux CentOS 7

Commit ID: f40de3c2e7cf5796513f65b5adcf6f8f7e3c5ab6
Duration 1:14:14
Result: :white_check_mark: SUCCEEDED
Error: N/A
Build Log terminal output (available for 30 days)
Build Workspace zip file of the working directory (available for 30 days)
Cluster Test Logs zip file of the test logs (available for 30 days)

May 07 '24 19:05 foundationdb-ci

Result of foundationdb-pr on Linux CentOS 7

Commit ID: f40de3c2e7cf5796513f65b5adcf6f8f7e3c5ab6
Duration 1:52:07
Result: :white_check_mark: SUCCEEDED
Error: N/A
Build Log terminal output (available for 30 days)
Build Workspace zip file of the working directory (available for 30 days)

May 07 '24 20:05 foundationdb-ci

Result of foundationdb-pr-clang on Linux CentOS 7

Commit ID: f40de3c2e7cf5796513f65b5adcf6f8f7e3c5ab6
Duration 1:56:00
Result: :white_check_mark: SUCCEEDED
Error: N/A
Build Log terminal output (available for 30 days)
Build Workspace zip file of the working directory (available for 30 days)

May 07 '24 20:05 foundationdb-ci

There are tests, those are located here: https://github.com/apple/foundationdb/blob/main/bindings/go/src/fdb/fdb_test.go. Those tests require a running FDB cluster.

Thanks! Is there any area which needs more coverage?

I'm trying to understand what is the benefit of this change and how you came across this.

I came across this by reading the C API documentation; the benefit is at the very least to clarify whether this is necessary to do, or not. If not, it should not commit and possibly do it even better than I am proposing here e.g. not relying on GC for the transaction destruction.

The performance improvement seems very small, I found it hard to measure in Go so far but I can try again. I would probably also want to measure the difference using a C program; by looking at how commits of R/O transactions are discarded in the fdbclient code, it makes sense that the benefit is small as there is little operations and memory reads involved (all on client side).

May 08 '24 09:05 gm42

I think it's fine (until we have some benchmark tests in the bindings, which sounds like a good idea). Like I said I was mostly curious about the motivation of the change :)

Inquiries are very welcome in any case :) for example with this change one could uncover a bug somewhere else, due to side effects.

May 08 '24 22:05 gm42

I have added another commit for the second approach, destroying the transaction explicitly without relying on on GC. NOTE: the finalizer is passed as transaction.destroy instead of (*transaction).destroy, I think it's equivalent because only how destroy is declared (pointer receiver) matters.

It would make a difference if the destroy method would change any field of the object, but it does not.

May 09 '24 05:05 gm42

Result of foundationdb-pr-clang-ide on Linux CentOS 7

Commit ID: d313f28f97de8d8aa9faf4f44e4740e1ce9a572d
Duration 0:23:09
Result: :white_check_mark: SUCCEEDED
Error: N/A
Build Log terminal output (available for 30 days)
Build Workspace zip file of the working directory (available for 30 days)

May 13 '24 22:05 foundationdb-ci

Result of foundationdb-pr-clang on Linux CentOS 7

Commit ID: d313f28f97de8d8aa9faf4f44e4740e1ce9a572d
Duration 0:33:37
Result: :x: FAILED
Error: Error while executing command: ctest -j ${NPROC} --no-compress-output -T test --output-on-failure. Reason: exit status 8
Build Log terminal output (available for 30 days)
Build Workspace zip file of the working directory (available for 30 days)

May 13 '24 22:05 foundationdb-ci

Result of foundationdb-pr-cluster-tests on Linux CentOS 7

Commit ID: d313f28f97de8d8aa9faf4f44e4740e1ce9a572d
Duration 0:43:46
Result: :x: FAILED
Error: Error while executing command: docker build --label "org.foundationdb.version=${FDB_VERSION}" --label "org.foundationdb.build_date=${BUILD_DATE}" --label "org.foundationdb.commit=${COMMIT_SHA}" --progress plain --build-arg FDB_VERSION="${FDB_VERSION}" --build-arg FDB_LIBRARY_VERSIONS="${FDB_VERSION}" --build-arg FDB_WEBSITE="${FDB_WEBSITE}" --tag foundationdb/ycsb:${FDB_VERSION}-${COMMIT_SHA}-debug --file Dockerfile.eks --target ycsb .. Reason: exit status 1
Build Log terminal output (available for 30 days)
Build Workspace zip file of the working directory (available for 30 days)
Cluster Test Logs zip file of the test logs (available for 30 days)

May 13 '24 22:05 foundationdb-ci

Result of foundationdb-pr-macos on macOS Ventura 13.x

Commit ID: d313f28f97de8d8aa9faf4f44e4740e1ce9a572d
Duration 0:46:17
Result: :white_check_mark: SUCCEEDED
Error: N/A
Build Log terminal output (available for 30 days)
Build Workspace zip file of the working directory (available for 30 days)

May 13 '24 22:05 foundationdb-ci

Result of foundationdb-pr on Linux CentOS 7

Commit ID: d313f28f97de8d8aa9faf4f44e4740e1ce9a572d
Duration 0:54:13
Result: :x: FAILED
Error: Error while executing command: ctest -j ${NPROC} --no-compress-output -T test --output-on-failure. Reason: exit status 8
Build Log terminal output (available for 30 days)
Build Workspace zip file of the working directory (available for 30 days)

May 13 '24 23:05 foundationdb-ci

Result of foundationdb-pr-clang-ide on Linux CentOS 7

Commit ID: d313f28f97de8d8aa9faf4f44e4740e1ce9a572d
Duration 0:21:17
Result: :white_check_mark: SUCCEEDED
Error: N/A
Build Log terminal output (available for 30 days)
Build Workspace zip file of the working directory (available for 30 days)

May 14 '24 07:05 foundationdb-ci

Result of foundationdb-pr-clang on Linux CentOS 7

Commit ID: d313f28f97de8d8aa9faf4f44e4740e1ce9a572d
Duration 0:32:55
Result: :x: FAILED
Error: Error while executing command: ctest -j ${NPROC} --no-compress-output -T test --output-on-failure. Reason: exit status 8
Build Log terminal output (available for 30 days)
Build Workspace zip file of the working directory (available for 30 days)

May 14 '24 07:05 foundationdb-ci

Result of foundationdb-pr-macos-m1 on macOS Ventura 13.x

Commit ID: d313f28f97de8d8aa9faf4f44e4740e1ce9a572d
Duration 0:34:58
Result: :white_check_mark: SUCCEEDED
Error: N/A
Build Log terminal output (available for 30 days)
Build Workspace zip file of the working directory (available for 30 days)

May 14 '24 08:05 foundationdb-ci

Result of foundationdb-pr on Linux CentOS 7

Commit ID: d313f28f97de8d8aa9faf4f44e4740e1ce9a572d
Duration 0:42:28
Result: :x: FAILED
Error: Error while executing command: ctest -j ${NPROC} --no-compress-output -T test --output-on-failure. Reason: exit status 8
Build Log terminal output (available for 30 days)
Build Workspace zip file of the working directory (available for 30 days)

May 14 '24 08:05 foundationdb-ci

Result of foundationdb-pr-cluster-tests on Linux CentOS 7

Commit ID: d313f28f97de8d8aa9faf4f44e4740e1ce9a572d
Duration 0:43:56
Result: :x: FAILED
Error: Error while executing command: docker build --label "org.foundationdb.version=${FDB_VERSION}" --label "org.foundationdb.build_date=${BUILD_DATE}" --label "org.foundationdb.commit=${COMMIT_SHA}" --progress plain --build-arg FDB_VERSION="${FDB_VERSION}" --build-arg FDB_LIBRARY_VERSIONS="${FDB_VERSION}" --build-arg FDB_WEBSITE="${FDB_WEBSITE}" --tag foundationdb/ycsb:${FDB_VERSION}-${COMMIT_SHA}-debug --file Dockerfile.eks --target ycsb .. Reason: exit status 1
Build Log terminal output (available for 30 days)
Build Workspace zip file of the working directory (available for 30 days)
Cluster Test Logs zip file of the test logs (available for 30 days)

May 14 '24 08:05 foundationdb-ci

Result of foundationdb-pr-macos on macOS Ventura 13.x

Commit ID: d313f28f97de8d8aa9faf4f44e4740e1ce9a572d
Duration 0:47:15
Result: :white_check_mark: SUCCEEDED
Error: N/A
Build Log terminal output (available for 30 days)
Build Workspace zip file of the working directory (available for 30 days)

May 14 '24 08:05 foundationdb-ci

Result of foundationdb-pr on Linux CentOS 7

Commit ID: d313f28

Duration 0:42:28

Result: ❌ FAILED

Error: Error while executing command: ctest -j ${NPROC} --no-compress-output -T test --output-on-failure. Reason: exit status 8

Build Log terminal output (available for 30 days)

Build Workspace zip file of the working directory (available for 30 days)

Seems like the tests are not passing:

52/70 Test  #5: single_process_external_client_fdbcli_tests .........................   Passed   54.73 sec
      Start 69: fdb/tuple_go_test
53/70 Test #68: fdb_go_test .........................................................***Failed    3.98 sec
Database created
setOne called with:  fdb.Database
fatal error: runtime.SetFinalizer: cannot pass *fdb.transaction to finalizer func()

goroutine 19 gp=0xc0001028c0 m=0 mp=0x6704c0 [running]:
runtime.throw({0xc000160000?, 0x55ecd8?})
	/usr/local/go/src/runtime/panic.go:1023 +0x5c fp=0xc000075c80 sp=0xc000075c50 pc=0x43b6fc
runtime.SetFinalizer({0x546b60, 0xc00011e570}, {0x528780, 0xc000118330})
	/usr/local/go/src/runtime/mfinal.go:475 +0x525 fp=0xc000075d58 sp=0xc000075c80 pc=0x41caa5
github.com/apple/foundationdb/bindings/go/src/fdb.Database.CreateTransaction({{0x0?, 0x0?}, 0x88?, 0xc0001280a8?})
	/codebuild/output/src4263330600/src/github.com/apple/foundationdb/build_output/bindings/go/src/github.com/apple/foundationdb/bindings/go/src/fdb/database.go:99 +0x7c fp=0xc000075d90 sp=0xc000075d58 pc=0x5102bc
github.com/apple/foundationdb/bindings/go/src/fdb.Database.Transact({{0x0?, 0x411085?}, 0x20?, 0xc0001280a8?}, 0xc00015a040)
	/codebuild/output/src4263330600/src/github.com/apple/foundationdb/build_output/bindings/go/src/github.com/apple/foundationdb/bindings/go/src/fdb/database.go:189 +0x26 fp=0xc000075de8 sp=0xc000075d90 pc=0x510646
github.com/apple/foundationdb/bindings/go/src/fdb.(*Database).Transact(0x58cda0?, 0xc00011a050?)
	<autogenerated>:1 +0x39 fp=0xc000075e20 sp=0xc000075de8 pc=0x514dd9
github.com/apple/foundationdb/bindings/go/src/fdb_test.TestVersionstamp.func1({0x58d268, 0xc00015a020}, {0xc0001280b0, 0x3, 0x3})
	/codebuild/output/src4263330600/src/github.com/apple/foundationdb/build_output/bindings/go/src/github.com/apple/foundationdb/bindings/go/src/fdb/fdb_test.go:66 +0xef fp=0xc000075e90 sp=0xc000075e20 pc=0x51824f
github.com/apple/foundationdb/bindings/go/src/fdb_test.TestVersionstamp(0xc0001484e0)
	/codebuild/output/src4263330600/src/github.com/apple/foundationdb/build_output/bindings/go/src/github.com/apple/foundationdb/bindings/go/src/fdb/fdb_test.go:89 +0xd3 fp=0xc000075f70 sp=0xc000075e90 pc=0x515773
testing.tRunner(0xc0001484e0, 0x563f60)
	/usr/local/go/src/testing/testing.go:1689 +0xfb fp=0xc000075fc0 sp=0xc000075f70 pc=0x4cfc3b
testing.(*T).Run.gowrap1()
	/usr/local/go/src/testing/testing.go:1742 +0x25 fp=0xc000075fe0 sp=0xc000075fc0 pc=0x4d0c65
runtime.goexit({})
	/usr/local/go/src/runtime/asm_amd64.s:1695 +0x1 fp=0xc000075fe8 sp=0xc000075fe0 pc=0x472b81
created by testing.(*T).Run in goroutine 1
	/usr/local/go/src/testing/testing.go:1742 +0x390

goroutine 1 gp=0xc0000081c0 m=nil [chan receive]:
runtime.gopark(0xc0001205b0?, 0xc000120638?, 0xb0?, 0x5?, 0x537500?)
	/usr/local/go/src/runtime/proc.go:402 +0xce fp=0xc0000749c0 sp=0xc0000749a0 pc=0x43e74e
runtime.chanrecv(0xc000154070, 0xc000074aa7, 0x1)
	/usr/local/go/src/runtime/chan.go:583 +0x3bf fp=0xc000074a38 sp=0xc0000749c0 pc=0x4099ff
runtime.chanrecv1(0x66f820?, 0x52a460?)
	/usr/local/go/src/runtime/chan.go:442 +0x12 fp=0xc000074a60 sp=0xc000074a38 pc=0x409632
testing.(*T).Run(0xc000148340, {0x5597eb?, 0x0?}, 0x563f60)
	/usr/local/go/src/testing/testing.go:1750 +0x3ab fp=0xc000074b20 sp=0xc000074a60 pc=0x4d0b0b
testing.runTests.func1(0xc000148340)
	/usr/local/go/src/testing/testing.go:2161 +0x37 fp=0xc000074b60 sp=0xc000074b20 pc=0x4d2bf7
testing.tRunner(0xc000148340, 0xc000074c70)
	/usr/local/go/src/testing/testing.go:1689 +0xfb fp=0xc000074bb0 sp=0xc000074b60 pc=0x4cfc3b
testing.runTests(0xc00011c0c0, {0x657ce0, 0x4, 0x4}, {0x1?, 0x4b844e?, 0x0?})
	/usr/local/go/src/testing/testing.go:2159 +0x445 fp=0xc000074ca0 sp=0xc000074bb0 pc=0x4d2ae5
testing.(*M).Run(0xc0001220a0)
	/usr/local/go/src/testing/testing.go:2027 +0x68b fp=0xc000074ed0 sp=0xc000074ca0 pc=0x4d14eb
main.main()
	_testmain.go:69 +0x16c fp=0xc000074f50 sp=0xc000074ed0 pc=0x5187cc
runtime.main()
	/usr/local/go/src/runtime/proc.go:271 +0x29d fp=0xc000074fe0 sp=0xc000074f50 pc=0x43e2fd
runtime.goexit({})
	/usr/local/go/src/runtime/asm_amd64.s:1695 +0x1 fp=0xc000074fe8 sp=0xc000074fe0 pc=0x472b81

goroutine 2 gp=0xc000008c40 m=nil [force gc (idle)]:
runtime.gopark(0x0?, 0x0?, 0x0?, 0x0?, 0x0?)
	/usr/local/go/src/runtime/proc.go:402 +0xce fp=0xc000064fa8 sp=0xc000064f88 pc=0x43e74e
runtime.goparkunlock(...)
	/usr/local/go/src/runtime/proc.go:408
runtime.forcegchelper()
	/usr/local/go/src/runtime/proc.go:326 +0xb3 fp=0xc000064fe0 sp=0xc000064fa8 pc=0x43e5b3
runtime.goexit({})
	/usr/local/go/src/runtime/asm_amd64.s:1695 +0x1 fp=0xc000064fe8 sp=0xc000064fe0 pc=0x472b81
created by runtime.init.6 in goroutine 1
	/usr/local/go/src/runtime/proc.go:314 +0x1a

goroutine 3 gp=0xc000009180 m=nil [GC sweep wait]:
runtime.gopark(0x0?, 0x0?, 0x0?, 0x0?, 0x0?)
	/usr/local/go/src/runtime/proc.go:402 +0xce fp=0xc000065780 sp=0xc000065760 pc=0x43e74e
runtime.goparkunlock(...)
	/usr/local/go/src/runtime/proc.go:408
runtime.bgsweep(0xc00002c1c0)
	/usr/local/go/src/runtime/mgcsweep.go:278 +0x94 fp=0xc0000657c8 sp=0xc000065780 pc=0x428894
runtime.gcenable.gowrap1()
	/usr/local/go/src/runtime/mgc.go:203 +0x25 fp=0xc0000657e0 sp=0xc0000657c8 pc=0x41d1e5
runtime.goexit({})
	/usr/local/go/src/runtime/asm_amd64.s:1695 +0x1 fp=0xc0000657e8 sp=0xc0000657e0 pc=0x472b81
created by runtime.gcenable in goroutine 1
	/usr/local/go/src/runtime/mgc.go:203 +0x66

goroutine 4 gp=0xc000009340 m=nil [GC scavenge wait]:
runtime.gopark(0xc00002c1c0?, 0x58aef0?, 0x1?, 0x0?, 0xc000009340?)
	/usr/local/go/src/runtime/proc.go:402 +0xce fp=0xc000065f78 sp=0xc000065f58 pc=0x43e74e
runtime.goparkunlock(...)
	/usr/local/go/src/runtime/proc.go:408
runtime.(*scavengerState).park(0x66fb40)
	/usr/local/go/src/runtime/mgcscavenge.go:425 +0x49 fp=0xc000065fa8 sp=0xc000065f78 pc=0x426289
runtime.bgscavenge(0xc00002c1c0)
	/usr/local/go/src/runtime/mgcscavenge.go:653 +0x3c fp=0xc000065fc8 sp=0xc000065fa8 pc=0x42681c
runtime.gcenable.gowrap2()
	/usr/local/go/src/runtime/mgc.go:204 +0x25 fp=0xc000065fe0 sp=0xc000065fc8 pc=0x41d185
runtime.goexit({})
	/usr/local/go/src/runtime/asm_amd64.s:1695 +0x1 fp=0xc000065fe8 sp=0xc000065fe0 pc=0x472b81
created by runtime.gcenable in goroutine 1
	/usr/local/go/src/runtime/mgc.go:204 +0xa5

goroutine 18 gp=0xc000102700 m=nil [finalizer wait]:
runtime.gopark(0xc000064648?, 0x411085?, 0xa8?, 0x1?, 0xc0000081c0?)
	/usr/local/go/src/runtime/proc.go:402 +0xce fp=0xc000064620 sp=0xc000064600 pc=0x43e74e
runtime.runfinq()
	/usr/local/go/src/runtime/mfinal.go:194 +0x107 fp=0xc0000647e0 sp=0xc000064620 pc=0x41c1a7
runtime.goexit({})
	/usr/local/go/src/runtime/asm_amd64.s:1695 +0x1 fp=0xc0000647e8 sp=0xc0000647e0 pc=0x472b81
created by runtime.createfing in goroutine 1
	/usr/local/go/src/runtime/mfinal.go:164 +0x3d

goroutine 20 gp=0xc000102a80 m=nil [runnable]:
github.com/apple/foundationdb/bindings/go/src/fdb.startNetwork.func1()
	/codebuild/output/src4263330600/src/github.com/apple/foundationdb/build_output/bindings/go/src/github.com/apple/foundationdb/bindings/go/src/fdb/fdb.go:219 fp=0xc000060fe0 sp=0xc000060fd8 pc=0x514640
runtime.goexit({})
	/usr/local/go/src/runtime/asm_amd64.s:1695 +0x1 fp=0xc000060fe8 sp=0xc000060fe0 pc=0x472b81
created by github.com/apple/foundationdb/bindings/go/src/fdb.startNetwork in goroutine 19
	/codebuild/output/src4263330600/src/github.com/apple/foundationdb/build_output/bindings/go/src/github.com/apple/foundationdb/bindings/go/src/fdb/fdb.go:219 +0x148