search

apple / foundationdb

FoundationDB - the open source, distributed, transactional key-value store
https://apple.github.io/foundationdb/
Apache License 2.0
14.19k stars 1.29k forks source link

Go binding: do not commit read-only transactions #11366

Closed gm42 closed 1 month ago

gm42 commented 2 months ago

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.

For Release-Branches

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

gm42 commented 2 months ago

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

foundationdb-ci commented 2 months ago

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

foundationdb-ci commented 2 months ago

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

foundationdb-ci commented 2 months ago

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

foundationdb-ci commented 2 months ago

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

foundationdb-ci commented 2 months ago

Result of foundationdb-pr on Linux CentOS 7

foundationdb-ci commented 2 months ago

Result of foundationdb-pr-clang on Linux CentOS 7

gm42 commented 2 months ago

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).

gm42 commented 1 month ago

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.

gm42 commented 1 month ago

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.

foundationdb-ci commented 1 month ago

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

foundationdb-ci commented 1 month ago

Result of foundationdb-pr-clang on Linux CentOS 7

foundationdb-ci commented 1 month ago

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

foundationdb-ci commented 1 month ago

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

foundationdb-ci commented 1 month ago

Result of foundationdb-pr on Linux CentOS 7

foundationdb-ci commented 1 month ago

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

foundationdb-ci commented 1 month ago

Result of foundationdb-pr-clang on Linux CentOS 7

foundationdb-ci commented 1 month ago

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

foundationdb-ci commented 1 month ago

Result of foundationdb-pr on Linux CentOS 7

foundationdb-ci commented 1 month ago

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

foundationdb-ci commented 1 month ago

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

johscheuer commented 1 month ago

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
foundationdb-ci commented 1 month ago

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

foundationdb-ci commented 1 month ago

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

foundationdb-ci commented 1 month ago

Result of foundationdb-pr-clang on Linux CentOS 7

foundationdb-ci commented 1 month ago

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

foundationdb-ci commented 1 month ago

Result of foundationdb-pr on Linux CentOS 7

gm42 commented 1 month ago

@johscheuer anything else to change here?

foundationdb-ci commented 1 month ago

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

foundationdb-ci commented 1 month ago

Result of foundationdb-pr-clang on Linux CentOS 7

foundationdb-ci commented 1 month ago

Result of foundationdb-pr on Linux CentOS 7

foundationdb-ci commented 1 month ago

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

jzhou77 commented 1 month ago

Correctness failures are caused by restarting tests and have been fixed in main.