ansible-collections / community.postgresql

Manage PostgreSQL with Ansible
https://galaxy.ansible.com/ui/repo/published/community/postgresql/
Other
97 stars 81 forks source link

Create postgresql_domain module #706

Open arcturustech opened 3 weeks ago

arcturustech commented 3 weeks ago
SUMMARY

This would be a new module to manage domains. Interface would be exactly analogous to postgresql.ext, postgresql.schema, or many others that manage create, drop and alter other major postgres objects

ISSUE TYPE
COMPONENT NAME

postgresql_domain

ADDITIONAL INFORMATION

There is currently no easy or obvious way to manage domains idempotently in ansible playbooks, the way there now is for tables, schemas, extensions, etc.

Domains are a puzzling exception to the wonderful way almost all other postgres objects can be managed in ansible. Without this, a developer will have to fall back on raw queries through the query module, and manage idempotence manually

    - name: Create a COUNT domain in the INVENTORY schema
      become_user: postgres
      community.postgresql.postgresql_domain:
        name: "COUNT"
        db: "WIDGET_INC"
        schema: "INVENTORY"
        base_type: "INT"
        check: "VALUE >= 0"
        default: 0
        cascade: false
        comment: "Domains are a great, underused tool for code readability and maintainability"
        state: present
Andersson007 commented 2 weeks ago

@arcturustech hello, thanks for opening the issue! Does a separate module bring any value compared to using just the postgresql_query module? Specifically, can it provide full idempotency? I know there are some modules (like postgresql_table, _idx, etc.) that don't work fully idempotently and it's partly my fault due to not being experienced at that time but imo there shouldn't be modules like that. I'm not a user so why I'm asking this because I don't know. For example, if we create a domain using the task from the description and later change any of the arguments values, will the module change the object to match the new state or it's impossible w/o destroying the object and re-creating it from scratch?

What do others think? Thanks

hunleyd commented 2 weeks ago

iirc, renaming a domain is the only altering you can do w/o dropping and recreating

Andersson007 commented 2 weeks ago

iirc, renaming a domain is the only altering you can do w/o dropping and recreating

@hunleyd thanks! in this case, i would suggest using postgresql_query instead of adding another module because those _table , _idx, etc modules is antipattern

arcturustech commented 2 weeks ago

iirc, renaming a domain is the only altering you can do w/o dropping and recreating

Actually the postgres ALTER DOMAIN statement (as of the current version 16) is pretty complete, as per the official documentation: https://www.postgresql.org/docs/current/sql-alterdomain.html

As you can see, pretty much every parameter of a DOMAIN can be altered without dropping and recreating. (If that were not true, I would see little point in using domains, but because it is true it makes them highly useful.)

hunleyd commented 2 weeks ago

I should have been clearer, you cannot change a constraint w/o dropping the existing constraint and adding the new one, and you cannot change the base type w/o dropping the domain itself. You can:

-    { SET DEFAULT expression | DROP DEFAULT }
-    { SET | DROP } NOT NULL
-    ADD domain_constraint [ NOT VALID ]
-    DROP CONSTRAINT [ IF EXISTS ] constraint_name [ RESTRICT | CASCADE ]
-    RENAME CONSTRAINT constraint_name TO new_constraint_name
-    VALIDATE CONSTRAINT constraint_name
-    OWNER TO { new_owner | CURRENT_ROLE | CURRENT_USER | SESSION_USER }
-    RENAME TO new_name
-    SET SCHEMA new_schema

which means of the proposed module args both:

-        base_type: "INT"
-        check: "VALUE >= 0"

cannot be changed, which to me seems to be a the 'base functionality' of the proposed module.

Having said that, I happily defer to others on the matter.

arcturustech commented 2 weeks ago

It's true that you cannot alter the base type of a domain. Presumably this is because doing so would make no sense, and it's not something a developer should ever want to do. A domain is by definition a subset of another datatype - the base datatype is fundamental to what the domain is and should not be changeable. So I understand why the postgres team did not provide for this and likely never would.

The constraints (note plural) really are the heart of what you need to be able to alter to make domains useful, as they define the subset. Those alterations take two basic forms - contracting the domain subset (by adding a constraint) and expanding the domain subset (by removing a constraint).

Initially, my example has one constraint (value >=0). If I later decide that no COUNT should be too large (restrict the subset), it's true I can't change constraint1 to (value >= 0 and value < 1000) but I can certainly use ALTER DOMAIN to add a second, independent constraint2 (value < 1000). If I then decide that 0 value COUNT is also unacceptable, I simply add constraint3 (value > 0). At that point constraint1 (value >= 0) is still applicable, just always overridden by constraint3. I could then remove constraint1, but I don't have to and probably wouldn't, as it will then still be there doing it's job if I ever remove constraint3. Meanwhile, once I realize that constraint2 was a bad idea, I can remove it independently (expand the subset)

The important thing is that I can massage the constraints (as a group) to provide any subset of the base datatype that I need without having to drop and re-create the domain (and worse yet every table that depends on that domain).

So does this count as having to drop and re-create the domain itself? That would seem to depend on the logic of why drop and re-create would be considered an anti-pattern and whether that logic still applies to this situation. That is something I don't fully understand, and at any rate you will have to decide for yourself.

hunleyd commented 2 weeks ago

Given the following:

    - name: Create a COUNT domain in the INVENTORY schema
      become_user: postgres
      community.postgresql.postgresql_domain:
        name: "COUNT"
        db: "WIDGET_INC"
        schema: "INVENTORY"
        base_type: "INT"
        check: "VALUE >= 0"
        default: 0
        cascade: false
        comment: "Domains are a great, underused tool for code readability and maintainability"
        state: present

[time passes]

    - name: Create a COUNT domain in the INVENTORY schema
      become_user: postgres
      community.postgresql.postgresql_domain:
        name: "COUNT"
        db: "WIDGET_INC"
        schema: "INVENTORY"
        base_type: "INT"
        check: "VALUE < 1000"
        default: 0
        cascade: false
        comment: "Domains are a great, underused tool for code readability and maintainability"
        state: present

how would the module know that this is an additional constraint and not a desire to replace the existing constraint?

arcturustech commented 2 weeks ago

That's a good question. It looks like the name of the constraint would have to be a required argument. Sorry, I didn't think of that in my initial proposal.

Another option might be to convert the check: argument from a string to a list, which might be more in line with the interface conventions of other modules

hunleyd commented 2 weeks ago

Making it a list would make sense. Do you have thoughts @Andersson007 ?

Andersson007 commented 1 week ago

hey folks, great discussion, thanks So, do i understand correctly that we can add and remove those checks? If yes, let's say this is an initial value

check: "VALUE >= 0"

then we changed it to

check: "VALUE < 1000"

can it be replaced in DB? i.e. the second one added, the first one removed? If yes, it sounds like an idempotent thing to me. @hunleyd you asked how the module determines - same as in, say, any user module: there'll probably be another argument called, say, append_check: true|false with a default value (i'd suggest false, i.e. replace). Passing a list SGTM

Could you folks please answer my questions

hunleyd commented 1 week ago

@Andersson007 we could just make check into checks and it always be a list?

checks: ['value >=0', 'value < 1000']

and every time the module runs it makes sure the list of checks exists as written?

Andersson007 commented 1 week ago

@Andersson007 we could just make check into checks and it always be a list?

checks: ['value >=0', 'value < 1000']

and every time the module runs it makes sure the list of checks exists as written?

@hunleyd we could also make it of the raw type which can accept a string or a list (any type basically but we ensure in the code that it's either list or str)

arcturustech commented 1 week ago

can it be replaced in DB? i.e. the second one added, the first one removed? If yes, it sounds like an idempotent thing to me.

Yes, that's true, and agreed I think that is totally idempotent

same as in, say, any user module: there'll probably be another argument called, say, append_check: true|false with a default value (i'd suggest false, i.e. replace). Passing a list SGTM

When I suggested a list earlier, I totally forgot that each constraint has a name - so actually maybe the even better suggestion for checks would be a dict checks: { "constraint1": "value >= 0", ... } And this interface I think would fully cover all possibilities that postgres allows, and also would not require the separate append_check argument

However -

make it of the raw type which can accept a string or a list (any type basically but we ensure in the code that it's either list or str

This is a great suggestion (if we substitute dict for list), it goes beyond the strictly necessary but would be definitely desirable from a convenience and usability standpoint. I suspect that most developers don't and would not want to actually make use of the multiple checks feature, simply provide one check as a simple string, and let it be replaced idempotently via drop and re-create the one constraint, and simply let postgres assign a default name.

Another option to do that might be to provide two arguments: checks (plural), which always takes a dict, for those who want to use the full capability of multiple check constraints, and check (singular) which always takes a string, for those who wish to just use a single constraint which can be idempotently changed by dropping and re-creating. Presumably only check or checks, but not both, should be specified in a single task

Further thoughts, anyone?

hunleyd commented 1 week ago

Another option to do that might be to provide two arguments: checks (plural), which always takes a dict, for those who want to use the full capability of multiple check constraints, and check (singular) which always takes a string, for those who wish to just use a single constraint which can be idempotently changed by dropping and re-creating. Presumably only check or checks, but not both, should be specified in a single task

i kinda like this actually

Andersson007 commented 1 week ago

Another option to do that might be to provide two arguments: checks (plural), which always takes a dict, for those who want to use the full capability of multiple check constraints, and check (singular) which always takes a string, for those who wish to just use a single constraint which can be idempotently changed by dropping and re-creating. Presumably only check or checks, but not both, should be specified in a single task

i kinda like this actually

could get better seen during development:)

@arcturustech would you like to try to create it yourself or anyone else can?

arcturustech commented 1 week ago

@arcturustech would you like to try to create it yourself or anyone else can?

As much as I would like to contribute, I don't have the time any time soon, so anyone else is most welcome and appreciated to take it on