Arvados

After lots of trial & error I finally saw what was my misunderstanding about the postgres-formula. The update is super simple.

Updates at 78668c47f - branch 20926-installer-pg-client-version

• Adds DATABASE_POSTGRESQL_VERSION configuration on local.params with default set to '12'
• Uses DATABASE_POSTGRESQL_VERSION in the postgresql.sls pillar.
• Adds postgres.client state to the controller so that it gets the matching version postgresql-client package installed.

Did manual tests with database and controller roles on separate nodes using version 15 and confirmed that all got installed correctly.

Lucas Di Pentima wrote in #note-5:

• Adds DATABASE_POSTGRESQL_VERSION configuration on local.params with default set to '12'

My two cents from an ops perspective: I don't think it makes sense for this setting to have a default value. It's necessarily going to depend on the deployment environment (does the installer install the PostgreSQL server or use an existing one?) and distribution. Having a default seems like a gotcha for when you forget to set it. It seems safer to just force people to set it.

Brett Smith wrote in #note-7:

Lucas Di Pentima wrote in #note-5:

• Adds DATABASE_POSTGRESQL_VERSION configuration on local.params with default set to '12'

My two cents from an ops perspective: I don't think it makes sense for this setting to have a default value. It's necessarily going to depend on the deployment environment (does the installer install the PostgreSQL server or use an existing one?) and distribution. Having a default seems like a gotcha for when you forget to set it. It seems safer to just force people to set it.

Yes, you make a good point here. I was just keeping the previous status quo of having the v12 as the default, with the addition of making it configurable instead of "hardcoded" on the salt's pillar.

The installer installs v12 on the node with the database role. IMO in that case, having a functional default is better than requiring the admin to make that decision. WDYT about making a check to set v12 as default when the database role is being used, and ask the admin to set it otherwise?

Brett Smith wrote in #note-7:

The installer installs v12 on the node with the database role. IMO in that case, having a functional default is better than requiring the admin to make that decision. WDYT about making a check to set v12 as default when the database role is being used, and ask the admin to set it otherwise?

Setting a default only if you use the database role sounds good to me. I guess my only question then is, why v12? I'm under the impression we're using that just because it's the version on the release of Debian/Ubuntu we install most. But if we can install different versions, why not default to the latest?

Brett Smith wrote in #note-9:

I guess my only question then is, why v12? I'm under the impression we're using that just because it's the version on the release of Debian/Ubuntu we install most. But if we can install different versions, why not default to the latest?

Not sure if there're incompatibilities or other issues with newer PG versions, I tend to agree to use the newest but I would like to double check with others.

Updates at 5e1a45ef8

• Requires manually setting PG version when not using the 'database' role.

I left version 12 for now, but whenever we want to change it, it's now set on common.sh.

Updates at 4e527029e
Test run: test-provision-debian11: #498

• Sets PG v15 as a default

Updates at 284a699
Test run: test-provision-debian11: #500

• Sets default PG version on single host case too.

Not sure if there're incompatibilities or other issues with newer PG versions, I tend to agree to use the newest but I would like to double check with others.

I mean, I thought part of the motivation for this branch was because we have a user deployed with PostgreSQL 15 in RDS? Plus for whatever it's worth I can run the test suite against PostgreSQL 15 on my Debian 12 system. So just for ticket posterity, my understanding is we have some reasons to believe it works, and no reasons to believe it doesn't.

Just one minor suggestion on the states manipulation, to narrow the regexp to avoid accidental false matches, I would suggest writing it as:

grep -q "^ +- postgres\.client$" ${STATES_TOP} || echo "    - postgres.client" >> ${STATES_TOP}

• I'm guessing you removed the front anchor to be more flexible about leading space, this is hopefully a good balance between that and strictness.

• Escaping the . avoids matching "postgres_client" or whatever.

(It would be super cool if this grep || sed recipe was a repeatable function, but you definitely don't need to go that far for this branch.)

Thanks.