From: Antonin Houska <ah@cybertec.at>
To: Konstantin Knizhnik <knizhnik@garret.ru>
cc: Daniel Gustafsson <daniel@yesql.se>,
    Konstantin Knizhnik <k.knizhnik@postgrespro.ru>,
    Michael Paquier <michael@paquier.xyz>,
    David Steele <david@pgmasters.net>,
    "ideriha.takeshi@fujitsu.com" <ideriha.takeshi@fujitsu.com>,
    Jaime Casanova <jaime.casanova@2ndquadrant.com>,
    Tomas Vondra <tomas.vondra@2ndquadrant.com>,
    Ryan Lambert <ryan@rustprooflabs.com>,
    Thomas Munro <thomas.munro@gmail.com>,
    PostgreSQL Hackers <pgsql-hackers@postgresql.org>,
    Bruce Momjian <bruce@momjian.us>, Dimitri Fontaine <dim@tapoueh.org>,
    Alvaro Herrera <alvherre@2ndquadrant.com>
Subject: Re: Built-in connection pooler
In-reply-to: <f2d9757e-e78f-fa44-a8fe-b85a06c74326@garret.ru>
References: 
 <OSAPR01MB1985068ABB08A85828999460EA770@OSAPR01MB1985.jpnprd01.prod.outlook.com>
 <a79b0687-c94d-8e0e-835b-85782c7b68ec@postgrespro.ru>
 <OSAPR01MB1985554A600901FF0B8D125FEA710@OSAPR01MB1985.jpnprd01.prod.outlook.com>
 <b5ff3ff2-b75c-c518-f75e-a472fa5292cc@postgrespro.ru>
 <3edbc4d2-ebe8-d73f-a3f4-e043bc504f17@pgmasters.net>
 <ad6a2c97-2410-1778-fade-20502368fcbb@postgrespro.ru>
 <F56E550A-62C4-4F72-B763-A0496DF23B3D@yesql.se>
 <f16e8099-75ab-ea23-8678-0b78ca25a2d0@postgrespro.ru>
 <A6A82977-D032-4DA4-885A-BE993F6F45C3@yesql.se>
 <e42c8d25-9a1c-a835-c422-0dc56e5775a6@postgrespro.ru>
 <20200917050711.GK2873@paquier.xyz>
 <0fd2ee44-e75b-8d14-4192-641aa07d6c89@postgrespro.ru>
 <FB6148A2-3BE2-4860-B4B8-5726ED1EBB42@yesql.se>
 <f2d9757e-e78f-fa44-a8fe-b85a06c74326@garret.ru>
Comments: In-reply-to Konstantin Knizhnik <knizhnik@garret.ru>
   message dated "Sun, 21 Mar 2021 21:32:28 +0300."
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-ID: <1914.1619604896.1@antos>
Content-Transfer-Encoding: quoted-printable
Date: Wed, 28 Apr 2021 12:14:56 +0200
Message-ID: <1915.1619604896@antos>
Archived-At: <https://www.postgresql.org/message-id/1915.1619604896%40antos>
Precedence: bulk

Konstantin Knizhnik <knizhnik@garret.ru> wrote:

> People asked me to resubmit built-in connection pooler patch to commitfe=
st.
> Rebased version of connection pooler is attached.

I've reviewd the patch but haven't read the entire thread thoroughly. I ho=
pe
that I don't duplicate many comments posted earlier by others.

(Please note that the patch does not apply to the current master, I had to
reset the head of my repository to the appropriate commit.)


Documentation / user interface
------------------------------

* session_pool_size (config.sgml)

I wonder if

    "The default value is 10, so up to 10 backends will serve each databas=
e,"

should rather be

    "The default value is 10, so up to 10 backends will serve each databas=
e/user combination."

However, when I read the code, I think that each proxy counts the size of =
the
pool separately, so the total number of backends used for particular
database/user combination seems to be

    session_pool_size * connection_proxies

Since the feature uses shared memory statistics anyway, shouldn't it only
count the total number of backends per database/user? It would need some
locking, but the actual pools (hash tables) could still be local to the pr=
oxy
processes.

* connection_proxies

(I've noticed that Ryan Lambert questioned this variable upthread.)

I think this variable makes the configuration less straightforward from th=
e
user perspective. Cannot the server launch additional proxies dynamically,=
 as
needed, e.g. based on the shared memory statistics that the patch introduc=
es?
I see that postmaster would have to send the sockets in a different way. H=
ow
about adding a "proxy launcher" process that would take care of the schedu=
ling
and launching new proxies?

* multitenant_proxy

I thought the purpose of this setting is to reduce the number of backends
needed, but could not find evidence in the code. In particular,
client_attach() always retrieves the backend from the appropriate pool, an=
d
backend_reschedule() does so as well. Thus the role of both client and bac=
kend
should always match. What piece of information do I miss?

* typo (2 occurrences in config.sgml)

  "stanalone" -> "standalone"


Design / coding
---------------

* proxy.c:backend_start() does not change the value of the "host" paramete=
r to
  the socket directory, so I assume the proxy connects to the backend via =
TCP
  protocol. I think the unix socket should be preferred for this connectio=
n if
  the platform has it, however:

* is libpq necessary for the proxy to connect to backend at all? Maybe
  postgres.c:ReadCommand() can be adjusted so that the backend can communi=
cate
  with the proxy just via the plain socket.

  I don't like the idea of server components communicating via libpq (do w=
e
  need anything else of the libpq connection than the socket?) as such, bu=
t
  especially these includes in proxy.c look weird:

  #include "../interfaces/libpq/libpq-fe.h"
  #include "../interfaces/libpq/libpq-int.h"

* How does the proxy recognize connections to the walsender? I haven't tes=
ted
  that, but it's obvious that these connections should not be proxied.

* ConnectionProxyState is in shared memory, so access to its fields should=
 be
  synchronized.

* StartConnectionProxies() is only called from PostmasterMain(), so I'm no=
t
  sure the proxies get restarted after crash. Perhaps PostmasterStateMachi=
ne()
  needs to call it too after calling StartupDataBase().

* Why do you need the Channel.magic integer field? Wouldn't a boolean fiel=
d
  "active" be sufficient?

  ** In proxy_loop(), I've noticed tests (chan->magic =3D=3D ACTIVE_CHANNE=
L_MAGIC)
     tests inside the branch

     else if (chan->magic =3D=3D ACTIVE_CHANNEL_MAGIC)

     Since neither channel_write() nor channel_read() seem to change the
     value, I think those tests are not necessary.

* Comment lines are often too long.

* pgindent should be applied to the patch at some point.


I can spend more time reviewing the patch during the next CF.

-- =

Antonin Houska
Web: https://www.cybertec-postgresql.com