Add event trigger resource management

[Fabianoshz]

I'm opening this as a draft in case anyone wants to give feedback or test the code.

This should allow us to manage event triggers in this provider, here's how the resource would look like:

resource "postgresql_event_trigger" "event_trigger" {
  name = "event_trigger_test"

  database = postgresql_database.database.name
  schema = postgresql_schema.schema.name

  function = postgresql_function.function.name
  on = "ddl_command_end"

  owner = "postgres"

  status = "enable"

  filter {
    variable = "TAG"
    values = [
      "CREATE TABLE",
      "ALTER TABLE",
    ]
  }
}

Closes #398

[Fabianoshz]

@cyrilgdn I've used this to add a few hundred event triggers where I work, our fork has a few more commits with some fixes of things that we found after opening the PR, I can push the fixes to this branch but it's not clear to me if you're interested in that, let me know either way.

[cyrilgdn]

Hi @Fabianoshz ,

Thanks for your work and for opening this PR,
I'm interested yes, I'm just lacking a bit of time to review it properly but will have a look as soon as possible 👍

[Fabianoshz]

Perfect, I take some time this week to push the other fixes we've added on our fork

[cyrilgdn]

Thanks again for you work, here are some comments. Basically:

• The ID used in setID needs to be based on database name / event name and not only event name
• The update function doesn't work well for some cases, I explained how to test it easily in the TestCase directly
• The Exists function can be removed
• A small bug in the read function for the owner
• and a few minor comments

let me know if you need help to fix that.

[cyrilgdn]

I think we cannot use simply the name for resource ID as it's possible to have 2 event trigger with the same name in different databases.

I tested with the default example of the doc:

postgres=# CREATE OR REPLACE FUNCTION abort_any_command()
  RETURNS event_trigger
 LANGUAGE plpgsql
  AS $$
BEGIN
  RAISE EXCEPTION 'command % is disabled', tg_tag;
END;
$$;

CREATE EVENT TRIGGER abort_ddl ON ddl_command_start
   EXECUTE FUNCTION abort_any_command();
CREATE FUNCTION
CREATE EVENT TRIGGER

postgres=# create database test;
CREATE DATABASE

postgres=# \c test
You are now connected to database "test" as user "postgres".

test=# CREATE OR REPLACE FUNCTION abort_any_command()
  RETURNS event_trigger
 LANGUAGE plpgsql
  AS $$
BEGIN
  RAISE EXCEPTION 'command % is disabled', tg_tag;
END;
$$;

CREATE EVENT TRIGGER abort_ddl ON ddl_command_start
   EXECUTE FUNCTION abort_any_command();
CREATE FUNCTION
CREATE EVENT TRIGGER

If someone tries to do that with Terraform, the second event trigger will remove the first one as they will share the same ID.

To fix that, you can, like for some other resources, compose an ID with dbname.eventname
(e.g.: https://github.com/cyrilgdn/terraform-provider-postgresql/blob/main/postgresql/resource_postgresql_schema.go#L155)

[cyrilgdn]

By the way that's why, in the getDBEventTriggerName function (that I guess you copied 😄 ), there these lines:

	if eventTriggerName == "" {
		parsed := strings.Split(d.Id(), ".")
		if len(parsed) != 2 {
			return "", "", fmt.Errorf("schema ID %s has not the expected format 'database.event_trigger': %v", d.Id(), parsed)
		}
		database = parsed[0]
		eventTriggerName = parsed[1]
	}

that expect to have an ID defined in this dbname.eventname form.

[cyrilgdn]

Not sure it's worth to create a variable if it's used only here.

Suggested change

	eventTriggerOn := d.Get(eventTriggerOnAttr).(string)
	fmt.Fprint(b, " ON ", eventTriggerOn)
	fmt.Fprint(b, " ON ", d.Get(eventTriggerOnAttr).(string))

[cyrilgdn]

Suggested change

	query := `SELECT evtname, evtevent, proname, nspname, evtenabled, evttags, usename ` +
	`FROM pg_catalog.pg_event_trigger ` +
	`JOIN pg_catalog.pg_user on pg_catalog.pg_event_trigger.evtowner = pg_catalog.pg_user.usesysid ` +
	query := `SELECT evtname, evtevent, proname, nspname, evtenabled, evttags, pg_get_userbyid(evtowner) ` +
	`FROM pg_catalog.pg_event_trigger ` +

It's simpler but on on top that you cannot use pg_user but should have used (without this function) pg_roles instead. Otherwise if you the owner to a superuser without LOGIN flag, your request will not return anything (as the role will not appear in pg_user)

[cyrilgdn]

Suggested change

Exists: PGResourceExistsFunc(resourcePostgreSQLEventTriggerExists),

Exists function is deprecated, if not defined it will call the Read function in which the SELECT will end in the case err == sql.ErrNoRows and will set an empty ID which means the function doesn't exist.

[cyrilgdn]

Suggested change

	func resourcePostgreSQLEventTriggerExists(db *DBConnection, d *schema.ResourceData) (bool, error) {
	database, eventTriggerName, err := getDBEventTriggerName(d, db.client.databaseName)
	if err != nil {
	return false, err
	}
	// Check if the database exists
	exists, err := dbExists(db, database)
	if err != nil || !exists {
	return false, err
	}
	txn, err := startTransaction(db.client, database)
	if err != nil {
	return false, err
	}
	defer deferredRollback(txn)
	err = txn.QueryRow("SELECT evtname FROM pg_event_trigger WHERE evtname=$1", eventTriggerName).Scan(&eventTriggerName)
	switch {
	case err == sql.ErrNoRows:
	return false, nil
	case err != nil:
	return false, fmt.Errorf("error reading schema: %w", err)
	}
	return true, nil
	}

cf comment above on Exists

[cyrilgdn]

Should the eventTriggerFunction be pq.QuoteIdentifier like the schema name?

[cyrilgdn]

Does it work for enable_replica and enable_always ?
it seems to generate ALTER EVENT TRIGGER name enable_replica where the documentation says:

ALTER EVENT TRIGGER name ENABLE [ REPLICA | ALWAYS ]

[cyrilgdn]

I actually checked with your test by adding an extra step like:

            {
                Config: fmt.Sprintf(testAccPostgreSQLEventTriggerConfig, dbName, schemas[0], "enable"),
                Check: resource.ComposeTestCheckFunc(
                    testAccCheckPostgresqlEventTriggerExists("postgresql_event_trigger.event_trigger", dbName),
                ),
            },
            {
                Config: fmt.Sprintf(testAccPostgreSQLEventTriggerConfig, dbName, schemas[0], "enable_always"),
                Check: resource.ComposeTestCheckFunc(
                    testAccCheckPostgresqlEventTriggerExists("postgresql_event_trigger.event_trigger", dbName),
                ),
            },

(and changing the conf definition to take %[3]s instead of "enabled" directly)
so it tries to alter the status of the event and it indeed fails with:

Error: pq: syntax error at or near "enable_always"

Full PG logs being:

ERROR:  syntax error at or near "enable_always" at character 34
STATEMENT:  ALTER EVENT TRIGGER "event_trigger_test" enable_always

[cyrilgdn]

I guess we should quote the owner too?

Suggested change

	fmt.Fprint(b, pq.QuoteIdentifier(eventTriggerName), " OWNER TO ", eventTriggerOwner)
	fmt.Fprint(b, pq.QuoteIdentifier(eventTriggerName), " OWNER TO ", pq.QuoteIdentifier(eventTriggerOwner))

[cyrilgdn]

You don't manage the renaming of an event properly, similar to the comment on status you can test easily by updating your test case with a new step that rename it, e.g.:

    {
        Config: fmt.Sprintf(testAccPostgreSQLEventTriggerConfig, dbName, schemas[0], "test_event"),
        Check: resource.ComposeTestCheckFunc(
            testAccCheckPostgresqlEventTriggerExists("postgresql_event_trigger.event_trigger", dbName),
        ),
    },
    {
        Config: fmt.Sprintf(testAccPostgreSQLEventTriggerConfig, dbName, schemas[0], "test_event_renamed"),
        Check: resource.ComposeTestCheckFunc(
            testAccCheckPostgresqlEventTriggerExists("postgresql_event_trigger.event_trigger", dbName),
        ),
    },

In this case, this test will fail with:

Error: pq: event trigger "test_event_renamed" does not exist

You can simply check here if the name changed with something like:

	if d.HasChange(eventTriggerNameAttr) {
		old, new := d.GetChange(eventTriggerNameAttr)
		if _, err := txn.Exec(
			fmt.Sprintf("ALTER EVENT TRIGGER %s RENAME TO %s", pq.QuoteIdentifier(old), pq.QuoteIdentifier(new)),
		); err != nil {
			return err
		}
	}

[cyrilgdn]

It's probably done in a similar way in other places but probably this could be simplified to

Suggested change

	b := bytes.NewBufferString("DROP EVENT TRIGGER ")
	fmt.Fprint(b, pq.QuoteIdentifier(eventTriggerName))
	sql := b.String()
	sql := fmt.Sprintf("DROP EVENT TRIGGER %s", pq.QuoteIdentifier(eventTriggerName))

As you prefer

[Fabianoshz]

@cyrilgdn thanks for throughout review, I will start working on fixing in the next few days