pert5432 commented 1 month ago

Ticket(s) Closed

Closes https://github.com/paradedb/pg_analytics/issues/97

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 commented 4 weeks ago

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

philippemnoel commented 4 weeks ago

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 commented 4 weeks ago

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 commented 4 weeks ago

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 commented 4 weeks ago

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 commented 4 weeks ago

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 commented 4 weeks ago

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 commented 4 weeks ago

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 commented 4 weeks ago

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: https://github.com/supabase/wrappers/pull/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 commented 3 weeks ago

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 commented 3 weeks ago

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 commented 3 weeks ago

Actually read for review 🥳

pert5432 commented 2 weeks ago

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

Done!

paradedb / pg_analytics

feat: support pg13 #134

Ticket(s) Closed

What

How

Tests