Allow messages to be registered from any SQL function about operation-hooks HOT 6 CLOSED

Both NOTIFY and the unlogged table approach have the issue that messages are "rolled back" if a transaction is rolled back:

NOTIFY interacts with SQL transactions in some important ways. Firstly, if a NOTIFY is executed inside a transaction, the notify events are not delivered until and unless the transaction is committed. This is appropriate, since if the transaction is aborted, all the commands within it have had no effect, including NOTIFY.
-- https://www.postgresql.org/docs/current/sql-notify.html

For informational messages, this means if an error occurs the user can only see the error and not the details leading up to the error. This may or may not be acceptable, but it's certainly a trade-off.

Regarding the RAISE NOTICE (or even RAISE DEBUG) option, things are a little more interesting:

Whether messages of a particular priority are reported to the client, written to the server log, or both is controlled by the log_min_messages and client_min_messages configuration variables. See Chapter 19 for more information.
-- https://www.postgresql.org/docs/current/plpgsql-errors-and-messages.html

So we can log different things to the server logs vs to the client. That means we could use a particularly verbose level like DEBUG, LOG, INFO or NOTICE:

Allowed levels are DEBUG, LOG, INFO, NOTICE, WARNING, and EXCEPTION, with EXCEPTION being the default.

with a particular error code like GOPHK (Graphile OPeration HooKs, though if you try and read it it sounds a little different... Maybe we should choose a better abbreviation). We can then capture these notices, interpret them in a particular way (e.g. JSON) and add to the messages system.

I think the unlogged table route should be avoided, it doesn't really have any advantages over LISTEN/NOTIFY, but it does have many disadvantages.

Irrespective of how we do this, I don't think it should be a goal for the messages raised by these "ambient" functions to be retrievable by other SQL code that can make decisions based on it. We could support basic deduplication, perhaps, but if something requires the current operation to stop and it cannot be done in an explicit "before" check then it should raise an exception.

Shall ponder more.

from operation-hooks.

Agreed with all of the above. Ponder away, as I'm sure that what you will come back with will be good. I also have a feeling that it will be used a lot.

from operation-hooks.

From chat with @demianuco, this pseudo-code is the kind of shape I'd expect to apply to a single field. Once someone gets this working and padded out and checked for various issues (like synchronisation issues?) we could look at adding it more generically to this plugin

const { makeWrapResolversPlugin, makeExtendSchemaPlugin, makePluginByCombiningPlugins, gql } = require('graphile-utils');

const WrapPlugin = makeWrapResolversPlugin({
  Mutation: {
    myMutation(resolver, parent, args, context, info) {
      const { pgClient } = context;
      const notices = [];
      const push = msg => notices.push(msg);
      pgClient.on('notice', push);
      try {
        const result = await resolver();
        return {
          ...result,
          notices,
        };
      } finally {
        pgClient.removeListener('notice', push);
      }
    }
  }
});

const ExtendPlugin = makeExtendSchemaPlugin({
  typeDefs: gql`
    extend type MyMutationPayload {
      messages: SOMETHING_HERE
    }
  `,
  resolvers: {
    MyMutationPayload: {
      messages(myMutationResult, args, context, info) {
        const { notices } = myMutationResult; // < Extracted from the return result of makeWrapResolvers above
        return someMangledVersionOf(notices);
      }
    }
  }
};

module.exports = makePluginByCombiningPlugins([WrapPlugin, ExtendPlugin]);

from operation-hooks.

I read a bit of the PostgreSQL docs, and scanned through the node-postgres code, and I believe that this is reliable and not vulnerable to race conditions. I sponsor @brianc the node-postgres maintainer so I've reached out to him to validate my assumptions but I think we can go ahead with the NOTICE route 🎉.

from operation-hooks.

Brian confirmed my suspicions 👍

from operation-hooks.

First punt at this implemented in #16; would love some eyes on it.

from operation-hooks.