Skip to content

feat: reject unsupported foreign table column types at DDL time - #640

Open
sanskar-soni-9 wants to merge 1 commit into
supabase:mainfrom
sanskar-soni-9:feat/reject-unsupported-column-type
Open

feat: reject unsupported foreign table column types at DDL time#640
sanskar-soni-9 wants to merge 1 commit into
supabase:mainfrom
sanskar-soni-9:feat/reject-unsupported-column-type

Conversation

@sanskar-soni-9

Copy link
Copy Markdown
Contributor

What kind of change does this PR introduce?

Feature

What is the current behavior?

Closes #346

Foreign table column types aren't validated against what Cell can actually represent, so an unsupported type fails late and indirectly:

  • On read, the Cell's Datum is written into the output slot with no type check, so an incompatible type like point yields garbage or crashes rather than erroring.
  • On write, an unrecognized rowid_column OID makes Cell::from_polymorphic_datum return None, so DELETE/UPDATE silently no-op.

Either way the user finds out far from the CREATE FOREIGN TABLE statement that caused it.

What is the new behavior?

A check_supported_column_types event trigger fires on ddl_command_end and rejects the offending statement immediately:

ERROR:  foreign table "my_table" has columns with unsupported data types: "loc" (type point)
HINT:   supported column types are: boolean, "char", smallint, integer, bigint, real,
        double precision, numeric, text, character varying, ... arrays of these, and
        domains over any of these
  • Scoped to this extension. The check resolves the table's FDW handler and confirms via pg_depend that it belongs to the wrappers extension, so postgres_fdw, file_fdw and friends are untouched — verified against a live file_fdw table with an unsupported column.
  • Domains resolve to their base type, so CREATE DOMAIN my_text AS text is accepted. All offending columns are reported in a single error rather than one at a time.
  • varchar/bpchar are now genuinely supported. from_polymorphic_datum previously matched only text, so a varchar rowid_column silently failed to parse. This PR adds the missing match arms — a latent bug fix riding along with the validation. Happy to split it into its own PR if you'd rather review it separately.

Additional context

On the approach. I weighed an event trigger against object_access_hook / ProcessUtility_hook and went with the event trigger: it's the documented, version-stable interface for DDL interception, whereas those hooks are internal API with no cross-version guarantee and would require the extension to own global hook chaining. Flagging this explicitly since it's the main design decision in the PR — happy to rework it if you'd prefer a hook-based approach.

Known limits.

  1. Validation is against the union of what Cell supports, not per-FDW. A type Cell handles but a particular FDW doesn't will still fail at query time; per-FDW type declarations would be a natural follow-up.
  2. The trigger is registered for the ALTER TABLE tag as well as ALTER FOREIGN TABLE, because Postgres tags ALTER TABLE <foreign_table> ADD COLUMN as the former. It therefore fires on every ALTER TABLE and filters via the pg_depend lookup above — one indexed catalog query on a path that isn't hot.

Implementation notes. Registration reuses the build-time versioned-library-name pattern already used for s3vec, guarded with EXCEPTION WHEN duplicate_object so ALTER EXTENSION wrappers UPDATE stays idempotent. New #[pg_test] coverage uses a small self-contained test FDW rather than a feature-gated one, so it runs under CI's default native_fdws invocation and not just locally. No new dependencies and no changes to the ForeignDataWrapper trait; the only change to existing runtime behavior is the varchar/bpchar fix noted above.

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.

Feedback on unsupported types

1 participant