electric-sql / pglite

Lightweight WASM Postgres with real-time, reactive bindings.
https://pglite.dev
Apache License 2.0
7.95k stars 151 forks source link

Event triggers not firing #258

Closed jamesgpearce closed 2 weeks ago

jamesgpearce commented 2 weeks ago

This is a slightly weird one. Table-level triggers work great:

import {PGlite} from '@electric-sql/pglite';

const pglite = await PGlite.create();

await pglite.listen('messages', console.log);

await pglite.query(`
  CREATE TABLE tableA (id TEXT, value TEXT);
`);

await pglite.query(`
  CREATE OR REPLACE FUNCTION functionA() RETURNS trigger AS $$ 
  BEGIN
    PERFORM pg_notify('messages', 'tableA changed');
    RETURN NULL;
  END;
  $$ LANGUAGE plpgsql;
`);

await pglite.query(`
  CREATE OR REPLACE TRIGGER triggerA 
  AFTER INSERT OR UPDATE OR DELETE ON tableA 
  EXECUTE FUNCTION functionA();
`);

await pglite.query(`
  INSERT INTO tableA (id, value) VALUES ('foo', 'bar');
`);
// -> 'tableA changed'

pglite.unlisten('messages');

BUT event triggers (for detecting table creation and deletion) do not seem to:

import {PGlite} from '@electric-sql/pglite';

const pglite = await PGlite.create();

await pglite.listen('messages', console.log);

pglite.query(`
  CREATE OR REPLACE FUNCTION functionB() RETURNS event_trigger AS $$ 
  BEGIN 
    PERFORM pg_notify('messages','table created or dropped');
  END;
  $$ LANGUAGE plpgsql;
`);

pglite.query(`
  CREATE EVENT TRIGGER triggerB
  ON ddl_command_end
  EXECUTE FUNCTION functionB();
`);

pglite.query(`
  CREATE TABLE tableB (id TEXT, value TEXT);
`);
// -> Should say 'table created or dropped' but does not

pglite.query(`
  DROP TABLE tableB;
`);
// -> Should say 'table created or dropped' but does not

pglite.unlisten('messages');

In both cases it seems as though the functions and triggers are created (ie they are present in routines, triggers, and pg_event_trigger as I would expect). eg the event trigger looks like this:

[
  {
    oid: 12772,
    evtname: 'triggerb',
    evtevent: 'ddl_command_end',
    evtowner: 10,
    evtfoid: 12771,
    evtenabled: 'O',
    evttags: null
  }
]

But the function is not being called for DDL operations. The same thing works great with the postgres module to a 'real' database. Any ideas why PGLite might be different?

I need to keep track of tables appearing and disappearing in TinyBase 😜 Thanks!

samwillis commented 2 weeks ago

That's an odd one... my first guess is that it's related to single user mode, which pglite is built on top off. We had to make a couple of modification related to multi process and auth stuff to make it behave the same, but nothing jump out as obvious.

@jamesgpearce I'm off this coming week, but very keen to help you integrate with TinyBase!

@msfstef could you take a look at this this week and give James a hand? First thing I would test is single user mode on a standard Postgres and see if they trigger. I imagin all event triggers (rather than normal triggers) are affected, we will want them for some of the sync related things. You may need to dig into the PG source and try and trace the calls, lots of printf...

samwillis commented 2 weeks ago

@msfstef

Note that pg_notify won't work on normal Postgres under single user mode, that's one of the changes we made. Worth double checking the trigger on PGlite by having it insert into a table. It may be the call to notify that's failing, not the trigger itself.

jamesgpearce commented 2 weeks ago

Yep OK, obviously pg_notify works great in PGlite for regular data changes. But I suspect you're on the right lines: I'm no PGLite expert, but the event triggers in general seem to be some sort of admin-ish concept. Hopefully these two test cases are sufficient to help!

This is my last (minor) blocker! I am just trying to be rigorous about the edge case of people removing a table and then putting it back: in that circumstance the data level trigger has to be put back on the table. It hasn't stopped me from releasing PGLite support in v5.2.0-beta.4 😎

jamesgpearce commented 2 weeks ago

Probably relatedly, I don't get notifications for data changes made to an underlying IDB database from another client. eg if I have two browser tabs, each running PGLite, the changes don't appear until I reload.

What's your overall strategy/plan for this? I can happily introduce a polling technique for it in TinyBase but it would be awesome if somehow the notifications straddled clients.

Edit: the more I thought about this the more I realize it's unlikely - since I should think of this as a file (a la SQLite) rather than a server (a la traditional PG). Polling it is!

samwillis commented 2 weeks ago

PGlite is currently single user/connection, opening the same datadir from multiple tabs is undefined behaviour. We have the PGliteWorker that runs it in a worker with a leader election to share the single instance between tabs.

I have a few thoughts around how to make it work across multiple connections, but that's going to come later. We will need to replace the shared memory and signals/interrupts that Postgres uses.

msfstef commented 2 weeks ago

Event triggers are disabled in single-user mode (see postgres). If an erroneous event trigger disables the database so much that you can't even drop the trigger, restart in single-user mode and you'll be able to do that.

From the pg docs and confirmed in manual testing.

I suspect we should be able to enable them though: https://github.com/postgres/postgres/blob/dbe37f1adb9fd10dc273ccf50816895360bcbc15/src/backend/commands/event_trigger.c#L718-L743

msfstef commented 2 weeks ago

I've patched postgres to bypass the event triggers being disabled in single user mode and it seems to work fine!

Here's the PR https://github.com/electric-sql/pglite/pull/266 - things should work once we get this in, test suite covers the usage with pg_notify

jamesgpearce commented 2 weeks ago

wow, you guys are crushing this

msfstef commented 2 weeks ago

Closed the issue as the fix was merged, it's going to make its way into the next release! If you need it soon let me know I can make a patch release with just this change.