add: configs as gucs for db-root-spec

[steve-chavez]

Fixes #3029.

[mkleczek]

This seems like unrelated edit.

[mkleczek]

I've read your comment here: #3029 (comment) and it looks to me that passing arguments as GUCs is wrong:

You are committing to an interface (ie. any change to GUCs will require PostgREST change and release) but this interface is informal and hidden. There is no gain in avoiding function signature here.

The right path would be to either:

• make the interface explicit and live with the fact that it requires maintenance
• pass full config as JSON to SQL implementation so that it can select what it needs and deal with possible PostgREST version compatibility issues by itself
• invent some kind of negotiation protocol to come up with configuration needed by SQL implementation (this one seems like an overkill and really just moves the problem around)
• make OpenAPI fully database driven (ie. not depending on any values from outside the database)

The last one seems the most principled and viable in the long run. It looks to me server-host server-port and openapi-server-proxy-uri are not too much of a problem because SQL can return responses with placeholders that can be replaced by PostgREST before returning the response (granted: the syntax of the placeholders is the interface in itself). Or - better yet - OpenAPI SQL implementation can determine the values from the request itself (ie. Host header, X-Forwarded-For or any other means) - that is not reliable in all cases.
The real problem is db-schemas though and we've been discussing it elsewhere already.

[steve-chavez]

You are committing to an interface (ie. any change to GUCs will require PostgREST change and release) but this interface is informal and hidden. There is no gain in avoiding function signature here.

You're right, agree.

make OpenAPI fully database driven (ie. not depending on any values from outside the database)
because SQL can return responses with placeholders that can be replaced by PostgREST before returning the response (granted: the syntax of the placeholders is the interface in itself)

Yeah the interface is not that clear and the json openapi can be big so I worry replacing the placeholders could be expensive.

pass full config as JSON to SQL implementation so that it can select what it needs and deal with possible PostgREST version compatibility issues by itself

I think this is the right move (nice balance between explicit interface and backwards compatibility), will do.

[steve-chavez]

I think this is the right move (nice balance between explicit interface and backwards compatibility), will do.

After implementing the new function interface (assuming a single json parameter), I've realized we shouldn't do it:

• Right now a create function root(json) returns "application/openapi+json" works as expected with unnamed single json parameter, with this change it wouldn't work anymore.
  • Granted this isn't that useful now on the root spec, but we should keep the interface consistent and flexible.
• It makes the callReadPlan/findProc more complex (and they already are) as we have to special case it for the case of db-root-spec, basically assuming a named/unnamed json parameter + a content type of application/json.
  • In the GUC approach, we don't have to touch the Plan module at all.
  • Also needs special handling to not cause a breaking change for current db-root-spec, we'd need to accept both empty parameter or json parameter.
• Special casing for db-root-spec breaks function overloading
• If we do a breaking change for the GUC names, that can be handled in a backwards compatible way for users. I've seen this done in the wild:

  create or replace function auth.uid() 
  returns uuid 
  language sql stable
  as $$
    select 
    coalesce(
      current_setting('request.jwt.claim.sub', true),
      (current_setting('request.jwt.claims', true)::jsonb ->> 'sub')
    )::uuid
  $$;

  • This back when pg14 forced us to change our custom GUCs (PG14: Custom parameter names must be of the form "identifier.identifier" and no dash(-) can be inside them #1857)

So overall the GUC approach looks much more stable for us and users. We just have to document it.

[mkleczek]

This seems like unrelated edit.

[mkleczek]

This test (and the fixture) seem to be a byproduct of the issue pointed out here https://github.com/PostgREST/postgrest/pull/5085/changes#r3654315052 - it is unclear if the GUCs are now a formal interface or not.

It should be replaced by an integration test that exercises integration of PostgREST with SQL OpenApi - that's the only way to confirm we are doing the right thing.

[steve-chavez]

It should be replaced by an integration test that exercises integration of PostgREST with SQL OpenApi

I assume you refer to https://github.com/PostgREST/postgrest-openapi/tree/main/sql. Testing that integration and deprecating the haskell openapi is the final step. But postgrest-openapi still has issues and it's not that motivating to finish them when core is known to be lacking. So I think we should keep the test simple otherwise we create a chicken and egg problem.