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

feat(compute): Add pg_duckdb extension v0.2.0 #10350

Open
wants to merge 12 commits into
base: main
Choose a base branch
from

Conversation

ololobus
Copy link
Member

@ololobus ololobus commented Jan 10, 2025

We want to host pg_duckdb on Neon.

Use cases

  • faster OLAP queries
  • access to datelake files (e.g. parquet) on S3 buckets from Neon PostgreSQL

Because neon does not provide superuser role to neon customers we need to grant some additional permissions to neon_superuser:

GRANT ALL ON FUNCTION duckdb.cache(TEXT, TEXT) TO neon_superuser;
GRANT ALL ON FUNCTION duckdb.install_extension(TEXT) TO neon_superuser;
GRANT ALL ON FUNCTION duckdb.raw_query(TEXT) TO neon_superuser;
GRANT ALL ON FUNCTION duckdb.cache_info() TO neon_superuser;
GRANT ALL ON FUNCTION duckdb.cache_delete(cache_key TEXT) TO neon_superuser;
GRANT ALL ON PROCEDURE duckdb.recycle_ddb() 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.

@ololobus ololobus force-pushed the alexk/add-pg_duckdb branch from 7fe60c4 to 4056242 Compare January 10, 2025 18:15
Copy link

github-actions bot commented Jan 10, 2025

7403 tests run: 7016 passed, 0 failed, 387 skipped (full report)


Flaky tests (5)

Postgres 17

Postgres 15

Code coverage* (full report)

  • functions: 33.5% (8500 of 25347 functions)
  • lines: 49.3% (71488 of 144978 lines)

* collected from Rust tests only


The comment gets automatically updated with the latest test results
fa2657d at 2025-01-25T18:00:37.190Z :recycle:

@ololobus ololobus marked this pull request as ready for review January 13, 2025 13:02
@ololobus ololobus requested a review from a team as a code owner January 13, 2025 13:02
Copy link
Member

@tristan957 tristan957 left a 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.

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 && \
Copy link
Member Author

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)

Copy link
Contributor

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants