The great connection cleanup

[jberkus]

I'd like to rationalize how we track, store, and use postgres database connections. Currently the methods of passing connection information is quite haphazard, resulting in strange and unpredictable issues with hba and passwords. It also makes it really hard to follow the code, or to make simple fixes like issue #61.

I need to get agreement from all contributors on the way to address this, because otherwise we're headed for merge conflict hell, since touching connections and connection-passing will touch quite a bit of code.

Here is my thinking on how we should do this:

• all connection information should be stored and passed as postgresql URIs.
• we should only be using two types of connections to databases, which should have sensible defaults.

The two types of connections are:

1. connections from the patroni daemon to the local PostgreSQL, which should default to a local socket connection as the supplied superuser.
2. connections from one patroni node to another as the replication user, which connection information is supplied by the key in the DCS.

All other connections should go through the API; there is no reason why a patroni node should need to connect directly to another node for any purpose other than replication (and replication support).

This would also have some other, follow-on changes:

• the URIs we are using will need to be fixed to be valid PostgreSQL URIs. particularly, the "database" key needs to go (proper key is "dbname").
• the replication user will need perms for setting up replication slots, etc.

So ... feedback/changes/etc.?

[alexeyklyukin]

I think we should accept both name=value and URI format when getting connection strings from the user.

As for the internal format, it makes sense to store all connection informations as dicts. This simplifies checking whether two conninfos represent the same connection, as well as modifying them.

As for the primary_conninfo in recovery.conf, we don't have a choice: the URI format was only introduced in 9.5, which is yet to be released. It makes sense to use the name=value connection strings when we output the value somewhere (i.e. to the recovery.conf, pg_rewind connection string, etc).

As for the usage of two types of connections, it won't work: pg_rewind requires a superuser connection to the master host, so you need it as well (there is a discussion to patch pg_rewind to use only replication connection, but this is not going to happen in 9.5).

The idea to go through the API for everything except the local, the replication and pg_rewind connection looks reasonable to me (aren't we doing it already?)

Note that we might be slow on providing feedback this week: the team is on PGConf.EU conference until Monday...

[jberkus]

Ah, ok, so there's three types of connections then. I haven't really looked at the pg_rewind code; I hadn't planned on using pg_rewind short of 9.5.

I'd forgotten about that limitation with recovery.conf. So, then we should make the canonical type dictionaries? With a single canonical function for creating connection strings?

If we're not going to use URIs, then wouldn't it make more sense to store the connection info as JSON in the DCS?

[a1exsh]

On Thu, Oct 29, 2015 at 6:58 PM, Josh Berkus notifications@github.com
wrote:

Ah, ok, so there's three types of connections then. I haven't really
looked at the pg_rewind code; I hadn't planned on using pg_rewind short of
9.5.

I'd forgotten about that limitation with recovery.conf. So, then we should
make the canonical type dictionaries? With a single canonical function for
creating connection strings?

Actually I wonder why this wasn't back-patched to 9.2... Maybe we should
raise a point on the hackers list. Original discussion:
http://www.postgresql.org/message-id/5474AEC9.2070109@vmware.com

Alex

[jberkus]

Are we sure it isn't supported? I'll test.

[a1exsh]

On Thu, Oct 29, 2015 at 9:37 PM, Josh Berkus notifications@github.com
wrote:

Are we sure it isn't supported? I'll test.

I am pretty sure it isn't, because of this:
https://github.com/postgres/postgres/blob/REL9_4_STABLE/src/backend/replication/libpqwalreceiver/libpqwalreceiver.c#L90

Appending that to an URI doesn't do the trick.

[feikesteenbergen]

three types of connections then

The third can be generated from the key value pairs, as you can specify the connection string that way:

$ pg_rewind -D /tmp/ --source-server="dbname=postgres host=localhost user=dummy"

could not connect to server: FATAL:  role "dummy" does not exist

[lasomethingsomething]

@jberkus @feikesteenbergen @a1exsh @alexeyklyukin @CyberDem0n Any agreement/compromise we can come to here so we can adopt/formalize any necessary standards + close out the issue?

[jberkus]

Well, because we didn't do this 4 months ago, it's going to be much harder now ...