feat(go/adbc/driver/snowflake): support parameter binding #1808

lidavidm · 2024-05-03T01:57:07Z

lidavidm · 2024-05-03T06:04:57Z

@zeroshade could you help figure out the ASAN error? Reducing SqlPrepareUpdateStream to this first part still seems to trigger it, so to me it seems like a GC issue again:

void StatementTest::TestSqlPrepareUpdateStream() {
  if (!quirks()->supports_bulk_ingest(ADBC_INGEST_OPTION_MODE_CREATE) ||
      !quirks()->supports_dynamic_parameter_binding()) {
    GTEST_SKIP();
  }

  ASSERT_THAT(AdbcStatementNew(&connection, &statement, &error), IsOkStatus(&error));
  ASSERT_THAT(quirks()->DropTable(&connection, "bulk_ingest", &error),
              IsOkStatus(&error));
  struct ArrowError na_error;

  const std::vector<SchemaField> fields = {{"ints", NANOARROW_TYPE_INT64}};

  // Create table
  {
    Handle<struct ArrowSchema> schema;
    Handle<struct ArrowArray> array;

    ASSERT_THAT(AdbcStatementSetOption(&statement, ADBC_INGEST_OPTION_TARGET_TABLE,
                                       "bulk_ingest", &error),
                IsOkStatus(&error));
    ASSERT_THAT(MakeSchema(&schema.value, fields), IsOkErrno());
    ASSERT_THAT((MakeBatch<int64_t>(&schema.value, &array.value, &na_error, {})),
                IsOkErrno(&na_error));
    ASSERT_THAT(AdbcStatementBind(&statement, &array.value, &schema.value, &error),
                IsOkStatus(&error));
    ASSERT_THAT(AdbcStatementExecuteQuery(&statement, nullptr, nullptr, &error),
                IsOkStatus(&error));
  }

Direct leak of 160 byte(s) in 2 object(s) allocated from:
    #0 0x7f50bac9561c in __interceptor_malloc ../../../../libsanitizer/asan/asan_malloc_linux.cpp:69
    #1 0x7f50b95c1339 in get_arr /home/lidavidm/go/pkg/mod/github.com/apache/arrow/go/v17@v17.0.0-20240430043840-e4f31462dbd6/arrow/cdata/cdata.go:31
    #2 0x7f50b95c14d3 in _cgo_1f0435ccdf41_Cfunc_get_arr /tmp/go-build/cgo-gcc-prolog:144
    #3 0x7f50b7bb64e4 in runtime.asmcgocall /usr/lib/go-1.21/src/runtime/asm_amd64.s:872

Indirect leak of 584 byte(s) in 4 object(s) allocated from:
    #0 0x7f50bac9561c in __interceptor_malloc ../../../../libsanitizer/asan/asan_malloc_linux.cpp:69
    #1 0x55763b631bab in PrivateArrowMalloc /home/lidavidm/Code/arrow-adbc/c/vendor/nanoarrow/nanoarrow.c:186

Indirect leak of 33 byte(s) in 2 object(s) allocated from:
    #0 0x7f50bac94df6 in __interceptor_realloc ../../../../libsanitizer/asan/asan_malloc_linux.cpp:85
    #1 0x55763b631bba in PrivateArrowRealloc /home/lidavidm/Code/arrow-adbc/c/vendor/nanoarrow/nanoarrow.c:188

SUMMARY: AddressSanitizer: 777 byte(s) leaked in 8 allocation(s).

zeroshade · 2024-05-03T16:27:19Z

I tracked down the cause of the leak, it's not a GC issue. I filed an upstream PR to fix the leak in the cdata package of Go Arrow which is linked above

go/adbc/driver/snowflake/binding.go

### What changes are included in this PR? If the `imp.alloc.bufCount` is 0, indicating we did not import any buffers from the provided C ArrowArray object, then we are free to not only call the release callback (which we already do) but also we need to free the temp ArrowArray we allocated to move the source to. This was uncovered by apache/arrow-adbc#1808 * GitHub Issue: #41534 Authored-by: Matt Topol <zotthewizard@gmail.com> Signed-off-by: David Li <li.davidm96@gmail.com>

lidavidm · 2024-05-04T00:15:11Z

Hmm.

=================================================================
==8722==ERROR: LeakSanitizer: detected memory leaks

Direct leak of 80 byte(s) in 1 object(s) allocated from:
    #0 0x7fa643a955f8 in __interceptor_malloc ../../../../libsanitizer/asan/asan_malloc_linux.cpp:69
    #1 0x7fa642104139 in get_arr /home/runner/go/pkg/mod/github.com/apache/arrow/go/v17@v17.0.0-20240503231747-7cd9c6fbd313/arrow/cdata/cdata.go:31
    #2 0x7fa6421042d3 in _cgo_7084d99dab99_Cfunc_get_arr /tmp/go-build/cgo-gcc-prolog:144
    #3 0x7fa6406f8884 in runtime.asmcgocall /opt/hostedtoolcache/go/1.21.8/x64/src/runtime/asm_amd64.s:872

Indirect leak of 584 byte(s) in 4 object(s) allocated from:
    #0 0x7fa643a955f8 in __interceptor_malloc ../../../../libsanitizer/asan/asan_malloc_linux.cpp:69
    #1 0x5586872db7b0 in PrivateArrowMalloc /home/runner/work/arrow-adbc/arrow-adbc/c/vendor/nanoarrow/nanoarrow.c:186

Indirect leak of 33 byte(s) in 2 object(s) allocated from:
    #0 0x7fa643a94dd2 in __interceptor_realloc ../../../../libsanitizer/asan/asan_malloc_linux.cpp:85
    #1 0x5586872db7bf in PrivateArrowRealloc /home/runner/work/arrow-adbc/arrow-adbc/c/vendor/nanoarrow/nanoarrow.c:188

SUMMARY: AddressSanitizer: 697 byte(s) leaked in 7 allocation(s).

zeroshade · 2024-05-04T00:22:44Z

Still?? Wtf?

Fixes #1144.

lidavidm · 2024-05-04T11:54:36Z

I think I tracked it down to a missing release in one case, let's see :P

lidavidm · 2024-05-06T06:57:32Z

@zeroshade can I just get a final look here once you're up?

zeroshade

LGTM just a couple nitpicks

zeroshade · 2024-05-06T14:35:06Z

go/adbc/driver/snowflake/binding.go

+		case *array.Boolean:
+			params[i].Value = sql.NullBool{
+				Bool:  column.Value(index),
+				Valid: column.IsValid(index),
+			}


Something for us to think about is that go1.22 added sql.Null[T] which might be useful to allow us to create a more generic implementation of this later on (since we officially still support go 1.21, we wouldn't be able to use it yet)

zeroshade · 2024-05-06T14:35:50Z

go/adbc/driver/snowflake/binding.go

+	// technically, snowflake can bind an array of values at once, but
+	// only for INSERT, so we can't take advantage of that without
+	// analyzing the query ourselves


I hate this, but nothing we can do right now i guess

go/adbc/driver/snowflake/binding.go

zeroshade · 2024-05-06T14:37:55Z

go/adbc/driver/snowflake/binding.go

+	params, err := r.NextParams()
+	if err != nil {
+		return nil, err
+	} else if params == nil {


no need for else since we're returning in the previous condition

zeroshade · 2024-05-06T14:38:32Z

go/adbc/driver/snowflake/binding.go

+		return nil, err
+	} else if params == nil {
+		// end-of-stream
+		return nil, nil


nil defines end-of-stream instead of using io.EOF?

zeroshade · 2024-05-06T14:41:15Z

go/adbc/driver/snowflake/binding.go

+	if r.currentBatch != nil {
+		r.currentBatch.Release()
+	}


we might still end up with a double release here because we don't set r.currentBatch to nil after calling stream.Next() and getting the end of the stream. we should probably just always set r.currentBatch = nil before we call stream.Next() to ensure we don't double release. You can test for excess releases by running with -tags assert which will turn on the debug asserts in Arrow

zeroshade · 2024-05-06T14:43:33Z

go/adbc/driver/snowflake/binding.go

+	if r.stream != nil {
+		r.stream.Release()
+	}


we should probably also set both r.currentBatch and r.stream to nil during release, just to prevent issues if release is called multiple times somehow

zeroshade

LGTM

…41535) ### What changes are included in this PR? If the `imp.alloc.bufCount` is 0, indicating we did not import any buffers from the provided C ArrowArray object, then we are free to not only call the release callback (which we already do) but also we need to free the temp ArrowArray we allocated to move the source to. This was uncovered by apache/arrow-adbc#1808 * GitHub Issue: apache#41534 Authored-by: Matt Topol <zotthewizard@gmail.com> Signed-off-by: David Li <li.davidm96@gmail.com>

### What changes are included in this PR? If the `imp.alloc.bufCount` is 0, indicating we did not import any buffers from the provided C ArrowArray object, then we are free to not only call the release callback (which we already do) but also we need to free the temp ArrowArray we allocated to move the source to. This was uncovered by apache/arrow-adbc#1808 * GitHub Issue: #41534 Authored-by: Matt Topol <zotthewizard@gmail.com> Signed-off-by: David Li <li.davidm96@gmail.com>

github-actions bot added this to the ADBC Libraries 1.0.0 milestone May 3, 2024

lidavidm force-pushed the gh-1144 branch from e1553dd to 47302ec Compare May 3, 2024 02:21

lidavidm force-pushed the gh-1144 branch from 47302ec to 200d0b1 Compare May 3, 2024 06:05

zeroshade mentioned this pull request May 3, 2024

GH-41534: [Go] Fix mem leak importing 0 length C Array apache/arrow#41535

Merged

zeroshade reviewed May 3, 2024

View reviewed changes

go/adbc/driver/snowflake/binding.go Show resolved Hide resolved

lidavidm force-pushed the gh-1144 branch from 200d0b1 to aeca9c9 Compare May 3, 2024 23:55

feat(go/adbc/driver/snowflake): support parameter binding

c134295

Fixes #1144.

lidavidm force-pushed the gh-1144 branch from aeca9c9 to c134295 Compare May 4, 2024 11:26

Fix

b78fed1

lidavidm marked this pull request as ready for review May 4, 2024 12:54

zeroshade approved these changes May 6, 2024

View reviewed changes

Fix

3bbe586

lidavidm requested a review from zeroshade May 7, 2024 08:12

zeroshade approved these changes May 7, 2024

View reviewed changes

lidavidm merged commit 8f0bdb3 into main May 7, 2024
92 of 96 checks passed

lidavidm deleted the gh-1144 branch May 7, 2024 14:11

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(go/adbc/driver/snowflake): support parameter binding #1808

feat(go/adbc/driver/snowflake): support parameter binding #1808

lidavidm commented May 3, 2024

lidavidm commented May 3, 2024

zeroshade commented May 3, 2024

lidavidm commented May 4, 2024

zeroshade commented May 4, 2024

lidavidm commented May 4, 2024

lidavidm commented May 6, 2024

zeroshade left a comment

zeroshade May 6, 2024

zeroshade May 6, 2024

zeroshade May 6, 2024

zeroshade May 6, 2024

zeroshade May 6, 2024

zeroshade May 6, 2024

zeroshade left a comment

feat(go/adbc/driver/snowflake): support parameter binding #1808

feat(go/adbc/driver/snowflake): support parameter binding #1808

Conversation

lidavidm commented May 3, 2024

lidavidm commented May 3, 2024

zeroshade commented May 3, 2024

lidavidm commented May 4, 2024

zeroshade commented May 4, 2024

lidavidm commented May 4, 2024

lidavidm commented May 6, 2024

zeroshade left a comment

Choose a reason for hiding this comment

zeroshade May 6, 2024

Choose a reason for hiding this comment

zeroshade May 6, 2024

Choose a reason for hiding this comment

zeroshade May 6, 2024

Choose a reason for hiding this comment

zeroshade May 6, 2024

Choose a reason for hiding this comment

zeroshade May 6, 2024

Choose a reason for hiding this comment

zeroshade May 6, 2024

Choose a reason for hiding this comment

zeroshade left a comment

Choose a reason for hiding this comment