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.

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

Add ability to specify / disable temp directory #832

Closed
mihaitodor opened this issue Jun 23, 2023 · 11 comments
Closed

Add ability to specify / disable temp directory #832

mihaitodor opened this issue Jun 23, 2023 · 11 comments
Assignees
Labels
enhancement The issue is a request for improvement or a new feature

Comments

@mihaitodor
Copy link

What is the current behavior?

Currently, users can specify the location where temp files are created via the Go-specific env var TMPDIR. The driver needs to write temp files as part of its workflow.

What is the desired behavior?

We’d like the ability to specify the temp directory used by this driver via a dedicated mechanism, such as the SNOWFLAKE_TMPDIR env var instead of relying on Go’s general purpose TMPDIR env var. Additionally, it would be great to be able to disable it completely, even if that means disabling some functionality.

How would this improve gosnowflake?

This would allow people to use the driver when the local file system is readonly.

References, Other Background

This is a follow-up to #700.

What is your Snowflake account identifier, if any?

@sfc-gh-dszmolka
Copy link
Contributor

thank you so much @mihaitodor for creating this follow-up issue; now I can update this properly once there's any new progress on the improvement part

@sfc-gh-dszmolka sfc-gh-dszmolka added enhancement The issue is a request for improvement or a new feature status-in_progress Issue is worked on by the driver team labels Jun 23, 2023
@sfc-gh-dszmolka
Copy link
Contributor

#874 aims to implement this functionality.

@sfc-gh-dszmolka sfc-gh-dszmolka added status-pr_pending_merge A PR is made and is under review and removed status-in_progress Issue is worked on by the driver team labels Aug 5, 2023
@sfc-gh-dszmolka
Copy link
Contributor

PR has been merged and will be part of the next release, which is expected towards the end of August

@sfc-gh-dszmolka sfc-gh-dszmolka added status-fixed_awaiting_release The issue has been fixed, its PR merged, and now awaiting the next release cycle of the connector. and removed status-pr_pending_merge A PR is made and is under review labels Aug 11, 2023
@mihaitodor
Copy link
Author

@sfc-gh-dszmolka Awesome, thank you! Looking at the changes, I'm not sure if this also adds the ability to disable the usage of temp files. Is this something that you're planning to implement in the future?

Also, I think it might be worth documenting this tempdir functionality in https://github.com/snowflakedb/gosnowflake/blob/master/doc.go so people can learn about it via the package docs: https://pkg.go.dev/github.com/snowflakedb/gosnowflake

@sfc-gh-dszmolka
Copy link
Contributor

we'll definitely document it, it's in the plans, please bear with us @mihaitodor :) until it is 'officially' documented, I see it can be used with TmpDirPath, alongside with the usual configuration options, like:

			cfg: &Config{
				User:       "u",
				Password:   "p",
				Account:    "a.b.c",
				TmpDirPath: "/tmp",
			},

Regarding the other question about completely disabling usage of temp files, reading back the original request, it seems it was twofold:

  • functionality to enable the user to specify temp dir path (implemented this PR)
  • consider disabling temp dir usage completely, even at the cost of loss of driver functionality

The latter is not part of this PR and I would imagine it is maybe not even straightforward. I need to check with the rest of the team.

@sfc-gh-dszmolka
Copy link
Contributor

got some update on this one:

  1. docs: SNOW-845282: Add docs about tmpDirPath #884 for the newly implemented feature (ability to specify the temp dir)
  2. the driver team has considered the other request too (disabling temp dir usage completely, even at the cost of loss of driver functionality) and for now we decided not to move forward with this request, as it seems to be a non-generic use-case of the gosnowflake connector. Of course this can be re-evaluated any time if more users and customers are asking for it, we will reconsider it then.

Speaking of which, as with any feature request with any Snowflake connector: if you (or the future readers, who would need the ability of disabling the usage of temp directory completely) are already a Snowflake customer, please do speak to your Sales representative and let them know how important this feature would be to you. They have access to certain other aspects of your account, which often can help the product teams to prioritize the requests differently.

Of course we'll still love to hear from the open-source community even if not a Snowflake customer, so please Future Readers: if you have a use-case for this feature (disabling temp dir usage completely, even at the cost of loss of driver functionality), let us know.

@mihaitodor
Copy link
Author

Thank you for the update @sfc-gh-dszmolka! Regarding the disabling of the temp directory, I'd be curious to learn for which use case(s) it's beneficial to write temp files. I see from the updated docs that it's used for "encryption and compression", but I can't imagine a good reason to persist that data to disk. I haven't read all the code to see what it does, but I'm genuinely curious, because, for my use case at least, I'm using db.ExecContext(gosnowflake.WithFileStream(ctx, bytes.NewReader(data)), "some_path"), where data is an in-memory buffer and simply streaming that data through an encryptor and a compressor while pushing it to your storage shouldn't require a temp file, but I'm probably missing some crucial context.

Additionally, there are use cases where people just want to do some small CRUD operations on tables without using a PUT statement. In that case, would temp files still get created? I guess it gets tricky when it falls back to an implicit stage like I mentioned in #540, but this isn't documented behaviour...

@sfc-gh-pfus
Copy link
Collaborator

Hello @mihaitodor ! Your questions made me to dig deeper. I believe that around PUT/GET support we can do some improvements, apart of giving up temp files, we can also move to more stream-based approach. It would require less memory. We have Q3 already planned, but we will consider to include it into our scope for the next quarters.

@sfc-gh-dszmolka
Copy link
Contributor

i also raised #887 so the effort for the second request could be publicly tracked, once it's scheduled.
we'll use this 832 to track the first request (user-manageable temp directory) which will be released in this month's release.

@mihaitodor
Copy link
Author

Thank you @sfc-gh-pfus and @sfc-gh-dszmolka! I appreciate that!

@sfc-gh-dszmolka
Copy link
Contributor

feature handled in this Issue

functionality to enable the user to specify temp dir path (implemented this PR)

just got released with version 1.6.24, so closing this one. The other request is tracked separately.

@sfc-gh-dszmolka sfc-gh-dszmolka removed the status-fixed_awaiting_release The issue has been fixed, its PR merged, and now awaiting the next release cycle of the connector. label Aug 22, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement The issue is a request for improvement or a new feature
Projects
None yet
Development

No branches or pull requests

4 participants