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: support pg13 #134

Merged
merged 13 commits into from
Oct 8, 2024
Merged

feat: support pg13 #134

merged 13 commits into from
Oct 8, 2024

Conversation

pert5432
Copy link
Contributor

@pert5432 pert5432 commented Sep 25, 2024

Ticket(s) Closed

What

Support for Postgres 13

How

Manually fetch the current schema if needed in auto_create_schema_hook. This is needed because the pointer to the schema name is null if the schema isn't explicitly specified in the CREATE FOREIGN TABLE statement.

Tests

All existing tests pass

@philippemnoel
Copy link
Collaborator

The error is strange -- wrappers should support PG13-onwards: https://github.com/paradedb/wrappers

@philippemnoel
Copy link
Collaborator

philippemnoel commented Sep 28, 2024

Seems like it's good after @Weijun-H's PG17 support PR re: wrappers! Probably only need to enable PG13 in any other test and add it to the README and fix the tests andwe should be all set :)

@philippemnoel
Copy link
Collaborator

If you rebase, there's a workflow file called check-pg_analytics-schema-upgrade.yml, which I've commented out since it requires PG13. We can reenable it as part of this PR once it is done

@pert5432
Copy link
Contributor Author

Postgres segfaults when running

CREATE FOREIGN DATA WRAPPER parquet_wrapper HANDLER parquet_fdw_handler VALIDATOR parquet_fdw_validator;
	        CREATE SERVER parquet_server FOREIGN DATA WRAPPER parquet_wrapper;
	        CREATE FOREIGN TABLE MyTable (boolean_col boolean,int8_col smallint,int16_col smallint,int32_col integer,int64_col bigint,uint8_col smallint,uint16_col integer,uint32_col bigint,uint64_col numeric(20),float32_col real,float64_col double precision,date32_col date,date64_col date,binary_col bytea,large_binary_col bytea,utf8_col text,large_utf8_col text) SERVER parquet_server OPTIONS (files '/tmp/.tmpSNcAfR/test_arrow_types.parquet'); 

logs: https://github.com/paradedb/pg_analytics/actions/runs/11092399046/job/30817389181?pr=134#step:12:67

I get the same segfault locally as well. Has this been experienced before? I'll look into debugging it later but should this be reported as a Postgres bug?

@philippemnoel
Copy link
Collaborator

Postgres segfaults when running

CREATE FOREIGN DATA WRAPPER parquet_wrapper HANDLER parquet_fdw_handler VALIDATOR parquet_fdw_validator;
	        CREATE SERVER parquet_server FOREIGN DATA WRAPPER parquet_wrapper;
	        CREATE FOREIGN TABLE MyTable (boolean_col boolean,int8_col smallint,int16_col smallint,int32_col integer,int64_col bigint,uint8_col smallint,uint16_col integer,uint32_col bigint,uint64_col numeric(20),float32_col real,float64_col double precision,date32_col date,date64_col date,binary_col bytea,large_binary_col bytea,utf8_col text,large_utf8_col text) SERVER parquet_server OPTIONS (files '/tmp/.tmpSNcAfR/test_arrow_types.parquet'); 

logs: https://github.com/paradedb/pg_analytics/actions/runs/11092399046/job/30817389181?pr=134#step:12:67

I get the same segfault locally as well. Has this been experienced before? I'll look into debugging it later but should this be reported as a Postgres bug?

I would suspect this is an issue with wrappers more than with Postgres. Perhaps we need to make edits to paradedb/wrappers? The segfault is only on PG13 right?

@pert5432
Copy link
Contributor Author

Postgres segfaults when running

CREATE FOREIGN DATA WRAPPER parquet_wrapper HANDLER parquet_fdw_handler VALIDATOR parquet_fdw_validator;
	        CREATE SERVER parquet_server FOREIGN DATA WRAPPER parquet_wrapper;
	        CREATE FOREIGN TABLE MyTable (boolean_col boolean,int8_col smallint,int16_col smallint,int32_col integer,int64_col bigint,uint8_col smallint,uint16_col integer,uint32_col bigint,uint64_col numeric(20),float32_col real,float64_col double precision,date32_col date,date64_col date,binary_col bytea,large_binary_col bytea,utf8_col text,large_utf8_col text) SERVER parquet_server OPTIONS (files '/tmp/.tmpSNcAfR/test_arrow_types.parquet'); 

logs: https://github.com/paradedb/pg_analytics/actions/runs/11092399046/job/30817389181?pr=134#step:12:67
I get the same segfault locally as well. Has this been experienced before? I'll look into debugging it later but should this be reported as a Postgres bug?

I would suspect this is an issue with wrappers more than with Postgres. Perhaps we need to make edits to paradedb/wrappers? The segfault is only on PG13 right?

I ran the tests locally on pg16 and they all ran fine, I haven't tried other versions than that

@philippemnoel
Copy link
Collaborator

Postgres segfaults when running

CREATE FOREIGN DATA WRAPPER parquet_wrapper HANDLER parquet_fdw_handler VALIDATOR parquet_fdw_validator;
	        CREATE SERVER parquet_server FOREIGN DATA WRAPPER parquet_wrapper;
	        CREATE FOREIGN TABLE MyTable (boolean_col boolean,int8_col smallint,int16_col smallint,int32_col integer,int64_col bigint,uint8_col smallint,uint16_col integer,uint32_col bigint,uint64_col numeric(20),float32_col real,float64_col double precision,date32_col date,date64_col date,binary_col bytea,large_binary_col bytea,utf8_col text,large_utf8_col text) SERVER parquet_server OPTIONS (files '/tmp/.tmpSNcAfR/test_arrow_types.parquet'); 

logs: https://github.com/paradedb/pg_analytics/actions/runs/11092399046/job/30817389181?pr=134#step:12:67
I get the same segfault locally as well. Has this been experienced before? I'll look into debugging it later but should this be reported as a Postgres bug?

I would suspect this is an issue with wrappers more than with Postgres. Perhaps we need to make edits to paradedb/wrappers? The segfault is only on PG13 right?

I ran the tests locally on pg16 and they all ran fine, I haven't tried other versions than that

Yeah. Then this isn't a Postgres bug I'd suspect it's a bug in our code. Two options come to mind:

  • Support for PG13 is incomplete in supabase/wrappers. This is unlikely IMO, I remember them telling me they added it.
  • We need to register the FDW differently in our code for PG13, which is perhaps more likely

@philippemnoel
Copy link
Collaborator

Postgres segfaults when running

CREATE FOREIGN DATA WRAPPER parquet_wrapper HANDLER parquet_fdw_handler VALIDATOR parquet_fdw_validator;
	        CREATE SERVER parquet_server FOREIGN DATA WRAPPER parquet_wrapper;
	        CREATE FOREIGN TABLE MyTable (boolean_col boolean,int8_col smallint,int16_col smallint,int32_col integer,int64_col bigint,uint8_col smallint,uint16_col integer,uint32_col bigint,uint64_col numeric(20),float32_col real,float64_col double precision,date32_col date,date64_col date,binary_col bytea,large_binary_col bytea,utf8_col text,large_utf8_col text) SERVER parquet_server OPTIONS (files '/tmp/.tmpSNcAfR/test_arrow_types.parquet'); 

logs: https://github.com/paradedb/pg_analytics/actions/runs/11092399046/job/30817389181?pr=134#step:12:67
I get the same segfault locally as well. Has this been experienced before? I'll look into debugging it later but should this be reported as a Postgres bug?

I would suspect this is an issue with wrappers more than with Postgres. Perhaps we need to make edits to paradedb/wrappers? The segfault is only on PG13 right?

I ran the tests locally on pg16 and they all ran fine, I haven't tried other versions than that

Yeah. Then this isn't a Postgres bug I'd suspect it's a bug in our code. Two options come to mind:

  • Support for PG13 is incomplete in supabase/wrappers. This is unlikely IMO, I remember them telling me they added it.
  • We need to register the FDW differently in our code for PG13, which is perhaps more likely

Actually... https://fdw.dev/guides/limitations/.

@philippemnoel
Copy link
Collaborator

philippemnoel commented Sep 29, 2024

Postgres segfaults when running

CREATE FOREIGN DATA WRAPPER parquet_wrapper HANDLER parquet_fdw_handler VALIDATOR parquet_fdw_validator;
	        CREATE SERVER parquet_server FOREIGN DATA WRAPPER parquet_wrapper;
	        CREATE FOREIGN TABLE MyTable (boolean_col boolean,int8_col smallint,int16_col smallint,int32_col integer,int64_col bigint,uint8_col smallint,uint16_col integer,uint32_col bigint,uint64_col numeric(20),float32_col real,float64_col double precision,date32_col date,date64_col date,binary_col bytea,large_binary_col bytea,utf8_col text,large_utf8_col text) SERVER parquet_server OPTIONS (files '/tmp/.tmpSNcAfR/test_arrow_types.parquet'); 

logs: https://github.com/paradedb/pg_analytics/actions/runs/11092399046/job/30817389181?pr=134#step:12:67
I get the same segfault locally as well. Has this been experienced before? I'll look into debugging it later but should this be reported as a Postgres bug?

I would suspect this is an issue with wrappers more than with Postgres. Perhaps we need to make edits to paradedb/wrappers? The segfault is only on PG13 right?

I ran the tests locally on pg16 and they all ran fine, I haven't tried other versions than that

Yeah. Then this isn't a Postgres bug I'd suspect it's a bug in our code. Two options come to mind:

  • Support for PG13 is incomplete in supabase/wrappers. This is unlikely IMO, I remember them telling me they added it.
  • We need to register the FDW differently in our code for PG13, which is perhaps more likely

Actually... https://fdw.dev/guides/limitations/.

Actually 2: supabase/wrappers#313 They did merge PG13 support, but perhaps just didn't update the documentation

Perhaps you can use the PR here to see what might be missing from our code to support PG13.

@pert5432
Copy link
Contributor Author

pert5432 commented Oct 3, 2024

UPDATE: I did some debugging with lldb and figured out it segfaults when running CREATE FOREIGN TABLE and subsequently running our event trigger. The segfault happens on line 96 in src/fdw/trigger.rs In particular (*relation).schemaname is a null pointer in here: let schema_name = CStr::from_ptr((*relation).schemaname).to_str()?;

It works as intended when specifying the schema name when creating the foreign table (CREATE FOREIGN TABLE public.table won't segfault). When I specify the schema in test runs they pass.

I'll have a look at fetching the schema for cases when its not supplied.

@philippemnoel
Copy link
Collaborator

UPDATE: I did some debugging with lldb and figured out it segfaults when running CREATE FOREIGN TABLE and subsequently running our event trigger. The segfault happens on line 96 in src/fdw/trigger.rs In particular (*relation).schemaname is a null pointer in here: let schema_name = CStr::from_ptr((*relation).schemaname).to_str()?;

It works as intended when specifying the schema name when creating the foreign table (CREATE FOREIGN TABLE public.table won't segfault). When I specify the schema in test runs they pass.

I'll have a look at fetching the schema for cases when its not supplied.

Great find! We may need to #cfg a specific PG version for some function

@pert5432
Copy link
Contributor Author

pert5432 commented Oct 6, 2024

Actually read for review 🥳

Copy link
Collaborator

@philippemnoel philippemnoel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

!! Could you also add PG13 to the publish workflow, and reactivate the check-schema-upgrade workflow now that we support PG13?

@pert5432
Copy link
Contributor Author

pert5432 commented Oct 7, 2024

!! Could you also add PG13 to the publish workflow, and reactivate the check-schema-upgrade workflow now that we support PG13?

Done!

src/fdw/trigger.rs Outdated Show resolved Hide resolved
Copy link
Collaborator

@philippemnoel philippemnoel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We also have a few places in the documentation in the README we might want to highlight that PG13 is now supported/we support all official PGDG versions.

Thank you for doing this, this is huge!

@philippemnoel philippemnoel merged commit 5b14368 into paradedb:dev Oct 8, 2024
15 checks passed
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.

Add PG13 support
2 participants