-
Notifications
You must be signed in to change notification settings - Fork 479
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
feat(compute): Add pg_duckdb extension v0.2.0 #10350
base: main
Are you sure you want to change the base?
Conversation
7fe60c4
to
4056242
Compare
7403 tests run: 7016 passed, 0 failed, 387 skipped (full report)Flaky tests (5)Postgres 17
Postgres 15
Code coverage* (full report)
* collected from Rust tests only The comment gets automatically updated with the latest test results
fa2657d at 2025-01-25T18:00:37.190Z :recycle: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Approved pending testing on staging.
…nly functions to neon_superuser
…bled_filesystems='LocalFileSystem'
make install -j $(getconf _NPROCESSORS_ONLN) && \ | ||
echo 'trusted = true' >> /usr/local/pgsql/share/extension/pg_duckdb.control && \ | ||
file=/usr/local/pgsql/share/extension/pg_duckdb--0.1.0--0.2.0.sql && \ | ||
echo 'GRANT ALL ON FUNCTION duckdb.cache(TEXT, TEXT) TO neon_superuser;' >> $file && \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder, is it better to keep these grants as a patch for https://github.com/duckdb/pg_duckdb/blob/v0.2.0/sql/pg_duckdb--0.1.0--0.2.0.sql to have everything in one place?
Either way, is there anything else that blocks us from adding this into image? @Bodobolero (thanks for your fixes)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if you set disabled_filesystems=‘LocalFileSystem’ which is necessary to fix the security hole S3 access still doesn’t work.
I will see if I find time today to continue working on the patch.
…ELECT duckdb.install_extension('iceberg') ;)
We want to host pg_duckdb on Neon.
Use cases
Because neon does not provide superuser role to neon customers we need to grant some additional permissions to neon_superuser:
We also added a patch for issue duckdb/duckdb#15734
The problem is that we do not want to allow the pg_duckdb access to the Neon compute local filesystem, however we still want to allow remote access to parquet files on S3 buckets.
So we want to set duckdb setting
disabled_filesystems="LocalFileSystem"
and still be able to query S3 buckets.The patch tries to enable this
We also want to be able to install duckdb supplied core extensions (like iceberg) and needed a further patch to allow extension installation access to the local filesystem, too.