ANXS / postgresql

Fairly full featured Ansible role for Postgresql.
http://anxs.io/
MIT License
849 stars 573 forks source link

Fix #445: PSQL tasks fails when postgresql_port != 5432 (default value) #448

Closed lrk closed 4 years ago

lrk commented 4 years ago

I added PGPORT environment variable for each task using psql client where it need to connect to the database. I also change the postgresql_port in tests to check that everything work fine.

lrk commented 4 years ago

@gclough if it is OK for you

gclough commented 4 years ago

@lrk , actually, looking at this further, should we remove that code altogether and encourage people to use the postgresql_database_extensions: section? That's how I create my extensions:

  postgresql_database_extensions:
    - db: postgres
      extensions:
        - adminpack
        - pgcrypto
        - pg_stat_statements
        - pg_buffercache
        - pg_prewarm
        - pgrowlocks
        - pgstattuple

    - db: repmgr
      extensions:
        - repmgr

This runs via this piece of code in the same file, which already has the port: "{{ postgresql_port }}" set:

- name: PostgreSQL | Add extensions to the databases
  become: yes
  become_user: "{{ postgresql_admin_user }}"
  postgresql_ext:
    db: "{{ item.0.db }}"
    login_user: "{{ postgresql_service_user }}"
    port: "{{ postgresql_port }}"
    name: "{{ item.1 }}"
  with_subelements:
    - "{{ postgresql_database_extensions }}"
    - extensions
  register: result

For backward compatibility, is there any way to add extensions to all databases in the list when each specific variable is set, so that the behaviour of the role doesn't change?

gclough commented 4 years ago

... actually, I'd tend toward merging the code with the --port= on the command line, and then adding a task to try and clean it up later. At least that way it would fix things that are broken, with the cleanup step being done later.

lrk commented 4 years ago

@lrk , actually, looking at this further, should we remove that code altogether and encourage people to use the postgresql_database_extensions: section? That's how I create my extensions:

  postgresql_database_extensions:
    - db: postgres
      extensions:
        - adminpack
        - pgcrypto
        - pg_stat_statements
        - pg_buffercache
        - pg_prewarm
        - pgrowlocks
        - pgstattuple

    - db: repmgr
      extensions:
        - repmgr

This runs via this piece of code in the same file, which already has the port: "{{ postgresql_port }}" set:

- name: PostgreSQL | Add extensions to the databases
  become: yes
  become_user: "{{ postgresql_admin_user }}"
  postgresql_ext:
    db: "{{ item.0.db }}"
    login_user: "{{ postgresql_service_user }}"
    port: "{{ postgresql_port }}"
    name: "{{ item.1 }}"
  with_subelements:
    - "{{ postgresql_database_extensions }}"
    - extensions
  register: result

For backward compatibility, is there any way to add extensions to all databases in the list when each specific variable is set, so that the behaviour of the role doesn't change?

There is a couple of postgresql modules in ansible 2.8 that could replace most of psql calls, but it depend on which version of ansible you want as a minimum requirement

gclough commented 4 years ago

I think the change you've done is good, and we can consider refactoring things in the future to take advantage of postgresql_ext, as this has been around since 1.9 I believe. It's used in other part of the role already, so we should avoid calls to psql where possible.

If there is a way to remove psql altogether, then we should consider it... but v2.8 is probably still too new to force everyone up to that version.