Proposal: refactor policy config metadata (description -> labels map, timestamps in JSONB -> table columns)

[jakedoublev]

Proposal

Part 1: timestamps

1. We add two new columns created_at and updated_at to the table schema for tables: attributes (FKA attribute_definitions), resource_mappings, subject_mappings, attribute_values, key_access_servers.
2. We use default NOW() in each table schema to set the created_at column on creation.
3. We use a trigger to replace the updated_at timestamp column in each table whenever a row is updated.
4. The Metadata proto schema still includes both timestamps contained within, but the column value of table[id].metadata JSON no longer includes the timestamps set by the services & their SQL queries since they are now individual columns on the row set at the DB level.

Part 2: description

1. The Metadata proto schema no longer contains description as a separate field. (code example)
2. Docs/UXs (i.e. the in-flight CLI tool) direct users to place any description-like state within the metadata labels.

Background

We hold a JSONB selection of metadata in many of our tables within the refactored policy config schema. It stands to reason that we could split those off into a separate metadata table with a FK relationship from the table to its metadata, but that introduces the need for a new join on every single read of a table row, which has performance impact. This metadata exists to provide a mechanism for a platform administrator to store a set of labels for an attribute with internal meaning (hence the generic map[string]string label type) and to track creation and updation timestamps.

Simplicity and dynamicity are the goals. This proposal will move some SQL query logic into the schema so it's native Postgres and not the server assigning timestamps (which has plenty of benefits on its own), and it will simplify the proto Message schema so there is one less field to support and marshal/unmarshal in and out of JSON.

Scope: There is known desire for audit capabilities on config data accesses and changes, but those needs are not the immediate concern as they are likely far more contextual.

Scope: There is uncertainty around timestamps when there exists in a FQN the tree-like relationship from (pseudocode) namespace -> attribute[] -> attribute[each id].values[] -> optional value[id].members[]. We should track timestamps on each table's rows, but if an end user is looking for the last time an attribute was updated, does that really mean "an aspect of its values was changed" or "an attribute definition was changed" since an attribute has little meaning without an associated value? The answer to that question is out of scope, but is something to consider directionally with any decision.

[dmihalcik-virtru]

Looking at cloud scale databases, aurora (AWS) does seem to support triggers but spanner (GCP) does not. I've always kind of held on to the idea we could use either, but I'm fine with that being a requirement if we see a real improvement in guarantees/experience

[jakedoublev]

Looking at cloud scale databases, aurora (AWS) does seem to support triggers but spanner (GCP) does not. I've always kind of held on to the idea we could use either, but I'm fine with that being a requirement if we see a real improvement in guarantees/experience

I'll add more context around triggers and support to this thread. I do think, though, that even if we rule out triggers and the services still set the updated_at column on update, it would still be an improvement to setting it within the metadata JSONB where it currently is.

[jakedoublev]

Looks like we have this cloud matrix for triggers support:

Cloud	PostgreSQL deployment	Docs	Supported
AWS	RDS	docs	✅
AWS	Aurora	docs	✅
GCP	Cloud SQL	docs	(✅ /?) plpgsql extension
GCP	Spanner	docs	❌
Azure	Azure DB for Postgres	docs	(❌/?) Most docs encouraging stitching together multiple cloud services when answering about triggers (Azure Event Grid, Azure Functions), so seems unlikely native triggers are supported, but would need further investigation for finality

[jrschumacher]

@dmihalcik-virtru the bigger question is outside postgres what kind of DBs can we support. @damorris25 mentioned that we should keep MSSQL in the back of our mind. It is understood that there will be work to support a driver approach, but we just want to make sure we don't box ourselves in.

[jakedoublev]

@dmihalcik-virtru the bigger question is outside postgres what kind of DBs can we support. @damorris25 mentioned that we should keep MSSQL in the back of our mind. It is understood that there will be work to support a driver approach, but we just want to make sure we don't box ourselves in.

This makes sense. We can separate the questions into:

1. is the requirement Postgres with solely cloud-agnostic features, or
2. is the requirement to support x, y, postgres, ...z databases

I think we can try to keep our implementations as portable as possible and meet the new requirements as customers present them with a driver abstraction as suggested in #70

[jrschumacher]

This article has an interesting performance analysis between Triggers and Rules with bulk inserts:

While rules are very powerful, they are also tricky to get right, particularly when data modification is involved. It is usually recommended to use the more modern triggers, and there have been attempts to deprecate them. But since rules are used to implement views, they are probable here to stay.

[jakedoublev]

In our case, we're concerned with one row in a table being updated, not a bulk update, so I would not expect even a row level trigger to experience performance impact that is significant enough to feel. I think it would look something like this, where the triggers are defined per table.

CREATE  FUNCTION update_updated_at()
RETURNS TRIGGER AS $$
BEGIN
    NEW.updated_at = now();
    RETURN NEW;
END;
$$ language 'plpgsql';

CREATE TRIGGER update_attribute_values
    BEFORE UPDATE
    ON
        attribute_values
    FOR EACH ROW
EXECUTE PROCEDURE update_updated_at();

CREATE TRIGGER update_subject_mappings
    BEFORE UPDATE
    ON
        subject_mappings
    FOR EACH ROW
EXECUTE PROCEDURE update_updated_at();

We might be able to DRY this up since we're using the same function across multiple tables, but I think that's the gist of what it would look like.

We could also definitely do performance testing if the need arises.