How about making it a deny list instead? The only settings we need to ensure are not modified are the role and the search_path because of impersonation + schema isolation. I can make a quick fix for this.

Have an allowlist of explicitly named parameters instead of just setting all of them.

This will be bad for maintenance because we can't list all extensions settings. For example plan_filter.statement_cost_limit (ref). If new extensions come or if the extension changes the setting name, we would have to patch PostgREST

Prefix those settings with pgrst.xxx or tx.xxx - and then set it without that prefix on the transaction level.

This would be bad for DX. Specially since statement_timeout is standard. Increases users cognitive load plus some support burden.

I think the deny list for search_path and role should be enough.

Prefix those settings with pgrst.xxx or tx.xxx - and then set it without that prefix on the transaction level.

Btw that won't work either because those pgrst.xxx require SUPERUSER. Unless we require pg 16 and do GRANT SET ON PARAMETER on the placeholder, which is still messy and weird behavior that could break later on.

How about making it a deny list instead? The only settings we need to ensure are not modified are the role and the search_path because of impersonation + schema isolation. I can make a quick fix for this.

We can't tell what kind of settings a user might have on their function, which they don't want to have hoisted. We should never just do "all of them" - this is just going to lead to problems.

Have an allowlist of explicitly named parameters instead of just setting all of them.

This will be bad for maintenance because we can't list all extensions settings. For example plan_filter.statement_cost_limit (ref). If new extensions come or if the extension changes the setting name, we would have to patch PostgREST

Then we need a config option to set those settings, that should be hoisted.

The config option can default to some basic PG-builtin settings.

I think the deny list for search_path and role should be enough.

Certainly not. As I said above, I have other cases except those two. For example it broke for me, because I had some timezone settings on some RPCs. Those would affect how the incoming dates were interpreted - but would not affect the transformation of the date to JSON when returning the result. Now, with this change, suddenly my whole output changes. This is a case that can be worked around, might actually just be a question of adjusting my snapshots.

But there are very likely other cases out there that we will just break silently - and it was very hard for me to debug this. It will be even harder for other users, who are not that familiar with postgrest. We 100% need an allowlist approach.

I see your point. Too bad we can't have this working transparently.

So how about a db-hoisted-tx-settings. For now I think the default will be:

• statement_timeout
• plan_filter.statement_cost_limit
• default_transaction_isolation
• ...

@taimoorzaeem Could you take this one? It's pretty much a follow up of #3142

It seems to me like the maximum length of a config is 52 characters. Please confirm if that is true.

I don't know any such limit, I assume there's something else going wrong.

@steve-chavez @wolfgangwalther @laurenceisla

Assigning list to config in test_io.py raises this type error. How do I assign the list to the config to test it?

def test_multiple_func_settings(defaultenv):
        "check multiple function settings are applied"
    
        env = {
            **defaultenv,
            "PGRST_DB_HOISTED_TX_SETTINGS": ["work_mem", "statement_timeout"],
        }
    
>       with run(env=env) as postgrest:
.
.
.
filename = ['work_mem', 'statement_timeout']

>   ???
E   TypeError: expected str, bytes or os.PathLike object, not list

<frozen os>:812: TypeError

How do I assign the list to the config to test it?

Using a string doesn't work?

        env = {
            **defaultenv,
            "PGRST_DB_HOISTED_TX_SETTINGS": "work_mem,statement_timeout",
        }