Support GCS #73

raghunandanbhat · 2025-01-07T18:01:49Z

fixes #38

raghunandanbhat · 2025-01-13T16:44:12Z

@zhousun please review when you get a chance. thanks!

zhousun · 2025-01-13T21:39:03Z

@raghunandanbhat thanks for the contribute. For delta-rs, I think it is possible to inline the secret as json instead of providing a path: https://docs.rs/object_store/latest/src/object_store/gcp/builder.rs.html#365

raghunandanbhat · 2025-01-14T11:37:07Z

delta-rs now uses the service account json instead of the file path.

I believe it is better to provide the path when creating/inserting a secret into mooncake.secrets, as it is easier than typing the entire JSON.

zhousun · 2025-01-15T05:31:22Z

If I understand correct, providing a path like this requires the files to be in server side. For example if you connect to a remote postgres, you cannot use a secret file in local path. That's why I believe having a way to direct type/copy the secret is important.

zhousun

In general lgtm, will approve once you resolve the other comment (service account json file won't work with server-client setup).
thx!

zhousun · 2025-01-15T17:34:13Z

README.md

+#### GCS Buckets
+- Add your GCS credentials
+```sql
+SELECT mooncake.create_secret('<name>', 'GCS', '<HMAC_key_id>', '<HMAC_secret>', '{"PATH":"path/to/service_account.json"}');
+```
+- Set default bucket after adding GCS credentials.
+```sql
+SET mooncake.default_bucket = 'gs://<gcs_bucket>';
+```
+>**Note**: delta-rs seems to accept only Service Account JSON credentials when creating delta tables in GCS.
+


nit: To keep readme more condense, we will move the bucket instruction to https://pgmooncake.com/docs/cloud-storage. :)

sure.
please make sure to update the sql query.

SELECT mooncake.create_secret('<name>', 'GCS', '<HMAC_key_id>', '<HMAC_secret>', '{"PATH":"path/to/service_account.json"}');

to

SELECT mooncake.create_secret('<name>', 'GCS', '<HMAC_key_id>', '<HMAC_secret>', '{"GCS_SECRET":"Service Account JSON"}');

cc @paurora17

raghunandanbhat · 2025-01-15T17:53:38Z

If I understand correct, providing a path like this requires the files to be in server side. For example if you connect to a remote postgres, you cannot use a secret file in local path. That's why I believe having a way to direct type/copy the secret is important.

what if we keep both options?

type/copy secrets for server-client setup
path based secrets for local setup

zhousun · 2025-01-15T19:55:45Z

If I understand correct, providing a path like this requires the files to be in server side. For example if you connect to a remote postgres, you cannot use a secret file in local path. That's why I believe having a way to direct type/copy the secret is important.

what if we keep both options?

type/copy secrets for server-client setup

path based secrets for local setup

Yep that would work

rust_extensions/delta/Cargo.toml

rust_extensions/delta/src/lib.rs

sql/pg_mooncake--0.1.0.sql

src/columnstore/columnstore_metadata.cpp

rust_extensions/delta/src/lib.rs

dpxcc · 2025-01-16T20:51:47Z

rust_extensions/delta/src/lib.rs

@@ -48,8 +55,10 @@ pub fn DeltaCreateTable(
    runtime.block_on(async {
        let mut storage_options: HashMap<String, String> =
            serde_json::from_str(options.to_str()?).expect("invalid options");
-        // Write directly to S3 without locking is safe since Mooncake is the only writer
-        storage_options.insert("AWS_S3_ALLOW_UNSAFE_RENAME".to_string(), "true".to_string());
+        if get_storage_type(path) == StorageType::S3 {


Thinking about this again, I think it might be better to simply return the type from ColumnstoreMetadata::SecretsSearchDeltaOptions() and pass it down here

yes, it is much simpler. we can return tuple<string, string> from ColumnstoreMetadata::SecretsSearchDeltaOptions() and pass them separately to DeltaCreateTable()

src/columnstore/columnstore_metadata.cpp

dpxcc · 2025-01-21T00:43:45Z

README.md


 > **Note**: On Neon, only cloud storage is supported. Neon users must bring their own S3 or R2 buckets or get a free S3 bucket by signing up at [s3.pgmooncake.com](https://s3.pgmooncake.com/). For cloud storage configuration instructions, see [Cloud Storage](https://pgmooncake.com/docs/cloud-storage). We are working to improve this experience.

+


nit: rm extra newline

dpxcc · 2025-01-21T00:44:22Z

rust_extensions/delta/src/lib.rs

@@ -48,8 +52,10 @@ pub fn DeltaCreateTable(
    runtime.block_on(async {
        let mut storage_options: HashMap<String, String> =
            serde_json::from_str(options.to_str()?).expect("invalid options");
-        // Write directly to S3 without locking is safe since Mooncake is the only writer
-        storage_options.insert("AWS_S3_ALLOW_UNSAFE_RENAME".to_string(), "true".to_string());
+        if storage_type.to_str()?.eq("S3") {


I suppose you can just storage_type.eq("S3")

dpxcc · 2025-01-21T00:44:42Z

sql/pg_mooncake--0.1.0.sql

@@ -275,14 +275,17 @@ RETURNS VOID
 LANGUAGE plpgsql
 AS $create_secret$
 DECLARE
-    allowed_keys TEXT[] := ARRAY['ENDPOINT', 'REGION', 'SCOPE', 'USE_SSL'];
+    s3_allowed_keys TEXT[] := ARRAY['ENDPOINT', 'REGION', 'SCOPE', 'USE_SSL'];
+    gcs_allowed_keys TEXT[] := ARRAY['SCOPE', 'GCS_SECRET', 'PATH'];


nit: sort alphabetically

dpxcc · 2025-01-21T00:45:52Z

sql/pg_mooncake--0.1.0.sql

    keys TEXT[];
    invalid_keys TEXT[];
    delta_endpoint TEXT;
+    gcs_service_account_json JSONB;
+    gcs_required_keys TEXT[] := ARRAY['type', 'project_id', 'private_key_id', 'private_key', 'client_email', 'client_id'];


Sort alphabetically
Should these also be uppercase?
We need to also add doc for these - I think your original doc doesn't include them

gcs_required_keys lists the keys in lowercase and in the same order as they appear in secrets JSON file. This makes it easy to spot which key is missing when error message is shown.

cc: @paurora17
For docs, you can use the below-

Google Cloud Storage

Add your GCS credentials by either providing the path to the service account JSON file:

SELECT mooncake.create_secret('<name>', 'GCS', '<HMAC_key_id>', '<HMAC_secret>', '{"PATH":"path/to/service_account.json"}');

Or by supplying the Service Account JSON string directly:

SELECT mooncake.create_secret('<name>', 'GCS', '<HMAC_key_id>', '<HMAC_secret>', '{"GCS_SECRET":"Service Account JSON string"}');

Note: When supplying secrets using a JSON string, ensure it includes the following keys, properly escaped: 'type', 'project_id', 'private_key_id', 'private_key', 'client_email', 'client_id'.

Set the default bucket after adding GCS credentials:

SET mooncake.default_bucket = 'gs://<gcs_bucket>';

Note: delta-rs seems to accept only Service Account JSON credentials when creating delta tables in GCS.

Thanks so much for the background! I'll revert the order of keys to your original order

@paurora17
When adding the docs, please clearly state that '{"PATH":"path/to/service_account.json"}' only works if service_account.json is located on the server side
By default, this won't work in Docker containers unless the file is mounted, and it definitely won't work on Neon
Should we consider omitting this option from the doc altogether?

dpxcc · 2025-01-21T00:46:44Z

src/columnstore/columnstore_metadata.cpp

-                longest_match = scope.length();
-            }
+        const string storage_type = TextDatumGetCString(values[1]);
+        if (!IsMatchingStorageType(storage_type, path)) {


You should just compute the type based on path outside of the loop

dpxcc · 2025-01-21T00:47:20Z

src/columnstore/columnstore_metadata.cpp

@@ -57,6 +57,20 @@ Oid Secrets() {
    return get_relname_relid("secrets", Mooncake());
 }

+static const std::vector<string> s3_prefixes = {"s3://", "s3a://", "s3n://"};


nit:

no need for static. you are already in anonymous namespace

std::vector -> vector

dpxcc · 2025-01-21T00:48:01Z

src/columnstore/columnstore_metadata.cpp

+static const std::vector<string> gcs_prefixes = {"gcs://", "gs://"};
+
+// Checks if a path matches the URL scheme for the given storage type (S3 or GCS)
+bool IsMatchingStorageType(const string &storage_type, const string &path) {


your variable names are not conforming with our code convention

dpxcc · 2025-01-21T00:48:18Z

src/columnstore/columnstore_metadata.cpp

+bool IsMatchingStorageType(const string &storage_type, const string &path) {
+    auto has_matching_prefix = [&path](const std::vector<string> &prefixes) {
+        return std::any_of(prefixes.begin(), prefixes.end(),
+                           [&path](string prefix) { return StringUtil::StartsWith(path, prefix); });


should use reference string &prefix

dpxcc · 2025-01-21T00:49:18Z

src/columnstore/columnstore_metadata.hpp

@@ -29,7 +29,7 @@ class ColumnstoreMetadata {
                                   const ColumnList *columns = nullptr);

    vector<string> SecretsGetDuckdbQueries();
-    string SecretsSearchDeltaOptions(const string &path);
+    std::tuple<string, string> SecretsSearchDeltaStorageConfig(const string &path);


nit: std::tuple<string /*type*/, string /*options*/>

dpxcc · 2025-01-21T00:49:41Z

src/lake/lake.cpp

    }

    void ChangeFile(Oid oid, string file_name, int64_t file_size, bool is_add_file) {
        if (cached_table_infos.count(oid) == 0) {
            ColumnstoreMetadata metadata(NULL /*snapshot*/);
            auto [path, timeline_id] = metadata.TablesSearch(oid);
-            cached_table_infos[oid] = {path, std::move(timeline_id), metadata.SecretsSearchDeltaOptions(path)};
+            auto [storage_type, options] = metadata.SecretsSearchDeltaStorageConfig(path);
+            cached_table_infos[oid] = {path, std::move(timeline_id), storage_type, options};


should use std::move() on storage_type and options

dpxcc · 2025-01-21T01:19:57Z

Sorry, but my force-push to gcs-support branch somehow triggered Github to close this PR accidentally..

We've cleaned up your implementation at 42392b7
(The cleanup addressed the comments above)
Let us know if you are okay with that

Also, I believe DuckDB's delta_scan() won't work with GCS right now due to duckdb/duckdb-delta#97

raghunandanbhat · 2025-01-21T03:52:12Z

We've cleaned up your implementation at 42392b7
(The cleanup addressed the comments above)
Let us know if you are okay with that

looks great!

dpxcc · 2025-01-21T05:51:22Z

Merged into the main branch. Thank you for your work! 🥮

dpxcc requested a review from zhousun January 7, 2025 19:09

dpxcc force-pushed the main branch 2 times, most recently from 2af4b2b to 6a655ea Compare January 10, 2025 04:05

raghunandanbhat force-pushed the gcs-support branch 3 times, most recently from 03ea259 to 6892f98 Compare January 13, 2025 16:33

raghunandanbhat marked this pull request as ready for review January 13, 2025 16:33

zhousun reviewed Jan 15, 2025

View reviewed changes

dpxcc reviewed Jan 15, 2025

View reviewed changes

dpxcc force-pushed the main branch from 64a3e22 to 6a655ea Compare January 15, 2025 22:11

raghunandanbhat requested a review from dpxcc January 16, 2025 16:29

raghunandanbhat force-pushed the gcs-support branch from db60953 to b4b9472 Compare January 16, 2025 16:47

dpxcc reviewed Jan 16, 2025

View reviewed changes

raghunandanbhat force-pushed the gcs-support branch from b4b9472 to 62a1f99 Compare January 17, 2025 06:36

raghunandanbhat requested a review from dpxcc January 17, 2025 06:36

dpxcc reviewed Jan 21, 2025

View reviewed changes

dpxcc closed this Jan 21, 2025

dpxcc force-pushed the gcs-support branch from 62a1f99 to 0d5083e Compare January 21, 2025 01:12

dpxcc pushed a commit that referenced this pull request Jan 21, 2025

Support GCS (#73)

f7ac054

dpxcc pushed a commit that referenced this pull request Jan 21, 2025

Support GCS (#73)

ea4aced

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support GCS #73

Support GCS #73

raghunandanbhat commented Jan 7, 2025

raghunandanbhat commented Jan 13, 2025

zhousun commented Jan 13, 2025

raghunandanbhat commented Jan 14, 2025

zhousun commented Jan 15, 2025

zhousun left a comment

zhousun Jan 15, 2025

raghunandanbhat Jan 15, 2025

zhousun Jan 15, 2025

raghunandanbhat commented Jan 15, 2025

zhousun commented Jan 15, 2025

dpxcc Jan 16, 2025

raghunandanbhat Jan 17, 2025

dpxcc Jan 21, 2025

dpxcc Jan 21, 2025

dpxcc Jan 21, 2025

dpxcc Jan 21, 2025

raghunandanbhat Jan 21, 2025

dpxcc Jan 21, 2025

dpxcc Jan 21, 2025

dpxcc Jan 21, 2025

dpxcc Jan 21, 2025

dpxcc Jan 21, 2025

dpxcc Jan 21, 2025

dpxcc Jan 21, 2025

dpxcc commented Jan 21, 2025 •

edited

Loading

raghunandanbhat commented Jan 21, 2025

dpxcc commented Jan 21, 2025


		> Note: On Neon, only cloud storage is supported. Neon users must bring their own S3 or R2 buckets or get a free S3 bucket by signing up at [s3.pgmooncake.com](https://s3.pgmooncake.com/). For cloud storage configuration instructions, see [Cloud Storage](https://pgmooncake.com/docs/cloud-storage). We are working to improve this experience.

Support GCS #73

Support GCS #73

Conversation

raghunandanbhat commented Jan 7, 2025

raghunandanbhat commented Jan 13, 2025

zhousun commented Jan 13, 2025

raghunandanbhat commented Jan 14, 2025

zhousun commented Jan 15, 2025

zhousun left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

raghunandanbhat commented Jan 15, 2025

zhousun commented Jan 15, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Google Cloud Storage

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dpxcc commented Jan 21, 2025 • edited Loading

raghunandanbhat commented Jan 21, 2025

dpxcc commented Jan 21, 2025

dpxcc commented Jan 21, 2025 •

edited

Loading