supabase / wrappers

Postgres Foreign Data Wrapper development framework in Rust.
http://fdw.dev
Apache License 2.0
541 stars 52 forks source link

Queries made to stripe foreign table returns error after being queried a few times #56

Closed ntoombs19 closed 1 year ago

ntoombs19 commented 1 year ago

Bug report

Describe the bug

When querying a view table with fields from a local table and a foreign table created with the stripe FDW, the client returns the following error after an arbitrary number of requests (usually three):

{
  code: 'HV00J',
  details: 'Wrappers',
  hint: null,
  message: 'required option "object" is not specified'
}

To Reproduce

Steps to reproduce the behavior, please provide code snippets or a repository:

  1. Setup a stripe account with test users
  2. Create a local PostgreSQL DB using Supabase CLI
  3. Add a local table that contains a field referencing a stripe customer ID
    CREATE TABLE public.users
    (
    id       uuid NOT NULL,
    customer text
    );
  4. Add the stripe server and stripe wrapper
    
    CREATE EXTENSION IF NOT EXISTS wrappers;
    CREATE FOREIGN DATA WRAPPER stripe_wrapper
    HANDLER stripe_fdw_handler
    VALIDATOR stripe_fdw_validator;

CREATE SERVER stripe_server FOREIGN DATA WRAPPER stripe_wrapper OPTIONS ( api_key '' );

5. Create the subscriptions foreign table

CREATE FOREIGN TABLE public.subscriptions ( id TEXT, customer TEXT, currency TEXT, current_period_start TIMESTAMP, current_period_end TIMESTAMP, attrs JSONB ) SERVER stripe_server OPTIONS ( object 'subscriptions' );

6. Create a view table that combines fields from the users table and the subscriptions table

CREATE VIEW public.vw_users_with_stripe_subscription AS SELECT u.id, s.customer, s.attrs FROM public.users u LEFT JOIN public.subscriptions s ON s.customer = u.customer;

7. Update the tables so that the view table will contain a record of the the user WITH the stripe data as well.
8. Write a query using the supabase client to return that record

supabase.from('vw_users_with_stripe_subscription') .select( 'id, customer, attrs', ).eq('id', '').single(),



## Expected behavior
The record containing the users subscription information, customer id, and user id should be returned each time the request is made.

## Screenshots

[Video capture](https://share.cleanshot.com/PXQTszpr)

## System information

- OS: macOS
- Browser: Chrome
- Version of supabase-js: 1.36.7
- Version of Node.js: v16.14.2

## Additional context

The issue occurs after some arbitrary number of successful queries (usually three) of the view table. Eventually, the error message `'required option "object" is not specified'` is received. I've also tried to make the query using an rpc function but it resulted in the same error message after some requests. Once the error is returned, it will be returned every subsequent request. If I drop the wrapper, the server, the view table, and recreate all three on the DB, the error will not return until some arbitrary number of requests are made.

The error appears to come from this function: https://github.com/supabase/wrappers/blob/main/supabase-wrappers/src/utils.rs#L276
ntoombs19 commented 1 year ago

I've confirmed that this error occurs even without querying stripe foreign table from a view table or from within a function.

burmecia commented 1 year ago

Thanks for reporting this issue, but I cannot reproduce it, can you try with latest version of supabase-js (v2.7.1) to check if this error still exists?

Marviel commented 1 year ago

@burmecia I'm also having the same issue with "@supabase/supabase-js": "^2.11.0"

Jesse-Bakker commented 1 year ago

I was fascinated by this bug, so I decided to dive into it. I could not reproduce it using psql, but with a simple python script, I reliably got a SEGFAULT at the seventh query to a foreign table. Diving in with a debugger, I noticed that on the seventh query, postgres skipped the planning phase of the query, reusing the query plan from the sixth execution.

This explains the SEGFAULTs I was getting and the errors that were previously reported:

A fresh query goes through the query planning stage, and in get_foreign_rel_size, all the parameters for the query are stored in a freshly allocated FdwState. A pointer to this state is then copied into the ForeignScanState in begin_foreign_scan.

In end_scan, the FdwState is cleared and subsequently dropped. This makes the pointer in the query plan dangle, but generally, the query plan will be dropped as well. However, when the query is prepared, PG will, after five executions, plan the query one more time, but generalized (without specific parameter values) and cache the query plan. That is where the bug manifests itself: on the seventh execution, the query plan from the sixth execution is reused, with a dangling pointer to FdwState. In the case of the reported bug, this manifests itself just as a cleared opts Hashmap (it is cleared in end_foreign_scan), but in my reproduction, the HashMap got in an inconsistent state and segfaulted.

The easiest way to solve this would be to move the logic that is currently in get_foreign_rel_size to begin_foreign_scan. Slightly more efficient would be to split FdwState into a part that stays the same between scans with the same plan and a part that varies between queries and only move the variable part to begin_foreign_scan.

burmecia commented 1 year ago

Thanks @Jesse-Bakker , you're right. The PostgREST uses prepared statement by default, which skips parsing query and directly call begin_foreign_scan, then tries to 'deserialise' a dangling FdwState pointer thus caused segfault.

The reason we're doing this way is we want to 'memorise' some parsed query info, such as order by and limit, and expose it to user during begin_foreign_scan. In this prepared statement case, that info cannot be retrieved solely during begin_foreign_scan.

A simple and quick workaround is to set db-prepared-statements = false in PostgREST config file, which will turn off prepared statements but this is not recommended by PostgREST. On Supabase platform, you can run alter role authenticator set pgrst.db_prepared_statements to false; to disable it.

A better solution will be re-writing 'serialiser' and 'deserialiser' for FdwState, currently it is just a pointer<->integer trick, those will put the saved state in server's memory so it won't loss and can be restored when using prepared statements.

Marviel commented 1 year ago

excited to see the fix! thanks @burmecia 🎉