Removing the need for `Listener`

[steve-chavez]

Problem

The Listener causes several problems:

• intermittent postgres core bug: LISTEN channel stops working due to postgres bug #3147
• Listener doesn't work on read replicas: Event triggers don't work on read replicas #3414 (which forces use of a primary with a hackish implementation)
• Proxies timeout the listener: Computed Relations slow at first request or after idle #3313 (reply in thread)
• gives problems on certain kubernetes deployments: Listener - connection being intermittently dropped when using Postgrest with PGCloudnative backed database.  #3778
• Weird bugs with GSSAPI: prevent GSSAPI error between Listener and pool #3573 (this caused a revert for GSSAPI authorization to the database #2815)

While it's possible postgres itself improves in some of the above areas, a listen channel is still an extra long lived connection, which adds maintenance burden for the user. And I'm thinking that maybe we can do the schema cache reload in another way..

Solution

The pre_config is a function that just sets some GUCs. What if we set a schema cache version there as:

create or replace function postgrest.pre_config()
returns void as $$
  select
      set_config('pgrst.db_schemas', 'schema1, schema2', true)
    , set_config('pgrst.scache_version', 1, true);
$$ language sql;

An event trigger then can redefine this function and increase the pgrst.scache_version each time a db object changes.

Then during transaction variable setup, we always execute the pre_config, which as a constant cost so it shouldn't affect performance that much:

select
  -- usual set_configs...
  postgrest.pre_config(..);

Then during execution of the main query, we read the scache version:

WITH pgrst_source AS (...)
SELECT
    coalesce(json_agg(_postgrest_t), '[]') AS body,
    nullif(current_setting('response.headers', true), '') AS response_headers,
    nullif(current_setting('response.status', true), '') AS response_status,
    nullif(current_setting('pgrst.scache_version', true), '') AS scache_version,
FROM ( SELECT * FROM pgrst_source ) _postgrest_t;

And cache that version, if it doesn't change we just serve the request as usual. If the version changes, we reload the schema cache.

[steve-chavez]

The pre_config is a function that just sets some GUCs.

Maybe this warrants the introduction of a new function too, a pgrst.schema_cache(), which has the sole purpose of returning a new version. It could be a table too, but maybe it's easier to just redefine a function than having a table with a single row which updates all the time?

[wolfgangwalther]

This probably needs a separate function, or at least a way to return the value separate from the main query. That's because if the schema changes it can change in a way that breaks every main query. And if the main query breaks, we won't observe the new value and would never refresh the cache.

I'm trying to think about implications when changing the location of pre_config itself... and realized we actually allow in-database configuration for db-pre-config? How exactly is that supposed to work? This would mean that on the first run, the full config would not be available, yet, but only after reloading? Or would the queries be executed twice, after the value for db-pre-config change?

I think we should keep things simple and just forbid in-database configuration of db-pre-config - otherwise this proposal will likely become very brittle.

---

In any case, we need pipelining for this to not have a bad effect on performance, because - as I wrote above - we can't return the value in the main query.

[steve-chavez]

I think we should keep things simple and just forbid in-database configuration of db-pre-config - otherwise this proposal will likely become very brittle.

Yeah, that's weird, it's listed on:

postgrest/src/PostgREST/Config/Database.hs

Lines 44 to 50 in 638d20d

	-- | In-db settings names
	dbSettingsNames :: [Text]
	dbSettingsNames =
	(prefix <>) <$>
	["db_aggregates_enabled"
	,"db_anon_role"
	,"db_pre_config"

I agree on removing it.

That's because if the schema changes it can change in a way that breaks every main query. And if the main query breaks, we won't observe the new value and would never refresh the cache.

How can that happen realistically? All our main queries should always be valid under any schema change. If any query is invalid then that is a bug that should be fixed, and that's a whole separate issue.

In any case, we need pipelining for this to not have a bad effect on performance, because - as I wrote above - we can't return the value in the main query.

Not keen on adding one more SQL statement for making this to work and per the above I don't think we need it, so pipelining should not be a strict requirement for this.

[wolfgangwalther]

How can that happen realistically? All our main queries should always be valid under any schema change. If any query is invalid then that is a bug that should be fixed, and that's a whole separate issue.

If the schema cache is outdated, all queries might point to a non-existing schema after the schema update.

[steve-chavez]

If the schema cache is outdated, all queries might point to a non-existing schema after the schema update.

We can detect cases of invalid queries (given that we know they're the cause of an outdated schema cache), and we can use those too as a chance to reload the schema cache.

[steve-chavez]

So I guess the full logic should be:

• execute the tx variables query -- with the inclusion of the scache version function
• execute the main query, read the scache version -- check if version matches or reload
  • if the query fails due to invalid db objects, check the version again and reload
    • @wolfgangwalther I think you proposed something similar before, but can't find the reference

With this we don't need to add an additional statement (which would be bad really, even with pipelining)

[steve-chavez]

An event trigger then can redefine this function and increase the pgrst.scache_version each time a db object changes.

Leaving here a sample event trigger that proves the above works:

create or replace function schema_cache_version() returns int
as 'select 1'
language sql;

create or replace function pgrst_ddl_watch()
returns event_trigger
language plpgsql
as $$
declare
  cmd record;
  ver int;
  func_schema text := 'public';
  func_name text := 'schema_cache_version';
begin
  for cmd in
    select *
    from pg_event_trigger_ddl_commands()
  loop
    if cmd.object_type = 'function'
       and split_part(cmd.object_identity, '(', 1) = format('%i.%i', func_schema, func_name)
    then
      continue; -- don't execute the trigger again when creating the scache function
    end if;

    select schema_cache_version() into ver;
    execute format($_$
      create or replace function %i.%i() returns int
      as 'select %s::int'
      language sql
    $_$, func_schema, func_name, ver + 1);
  end loop;
end;
$$;

create event trigger pgrst_watch
  on ddl_command_end
  execute procedure pgrst_ddl_watch();

select schema_cache_version();
 schema_cache_version 
----------------------
                    1
(1 row)

create table foo();
CREATE TABLE

select schema_cache_version();
 schema_cache_version 
----------------------
                    2
(1 row)

[wolfgangwalther]

if the query fails due to invalid db objects, check the version again and reload

It would be great if this would not require the version check, so that the whole version thing is optional.

We could build this logic (reload after error) independently at first, while we still keep the listener. We'd have to see whether we can make this work well on its own already, aka whether we can avoid useless reloads.

[mkleczek]

To be honest I find this proposal a little bit too hackish for my taste. You want to create a side channel in main query to retrieve some non-related (to the query) configuration information.

The alternative would be to use logical decoding and pg_logical_emit_message instead of LISTEN/NOTIFY?

[wolfgangwalther]

The alternative would be to use logical decoding and pg_logical_emit_message instead of LISTEN/NOTIFY?

I highly doubt that setting up logical decoding would achieve the goal: Making this easier to set up and less brittle.

[mkleczek]

The alternative would be to use logical decoding and pg_logical_emit_message instead of LISTEN/NOTIFY?

I highly doubt that setting up logical decoding would achieve the goal: Making this easier to set up and less brittle.

Yeah...
But it could fix issues of LISTEN/NOTIFY not being reliable and not working on read replicas.

[steve-chavez]

The alternative would be to use logical decoding and pg_logical_emit_message instead of LISTEN/NOTIFY?
I highly doubt that setting up logical decoding would achieve the goal: Making this easier to set up and less brittle.

I've seen anything WAL (and logical decoding plugins) present recurring maintenance problems. Cases where the schema cache queries take too long thanks to an abandoned replication slot that causes catalog bloat, once the replication slot is manually dropped all goes back to normal. Overall I see it as a bigger can of worms than the current LISTEN one.

You want to create a side channel in main query to retrieve some non-related (to the query) configuration information.

Note that we already do something similar to get the HTTP headers and status:

postgrest/src/PostgREST/Query/Statements.hs

Lines 109 to 110 in f01d5bf

	responseHeadersF <> " AS response_headers, " <>
	responseStatusF <> " AS response_status, " <>

And this on every main query.

To be honest I find this proposal a little bit too hackish for my taste.

It is hackish, but our query for getting views' relationships is much more hackish and has been working fine for many years now!

postgrest/src/PostgREST/SchemaCache.hs

Lines 848 to 854 in f01d5bf

	-- | Returns all the views' primary keys and foreign keys dependencies
	allViewsKeyDependencies :: Bool -> SQL.Statement AppConfig [ViewKeyDependency]
	allViewsKeyDependencies =
	SQL.Statement sql params decodeViewKeyDeps
	-- query explanation at:
	-- * rationale: https://gist.github.com/wolfgangwalther/5425d64e7b0d20aad71f6f68474d9f19
	-- * json transformation: https://gist.github.com/wolfgangwalther/3a8939da680c24ad767e93ad2c183089

[mkleczek]

The alternative would be to use logical decoding and pg_logical_emit_message instead of LISTEN/NOTIFY?
I highly doubt that setting up logical decoding would achieve the goal: Making this easier to set up and less brittle.

I've seen anything WAL (and logical decoding plugins) present recurring maintenance problems. Cases where the schema cache queries take too long thanks to an abandoned replication slot that causes catalog bloat, once the replication slot is manually dropped all goes back to normal. Overall I see it as a bigger can of worms than the current LISTEN one.

My experience is the opposite: we are happily using temporary replication slots (see pg_create_logical_replication_slot) to avoid catalog/WAL bloat and slot maintenance burden.

Anyway - that was just a suggestion.