Thread: Executing inet_server_addr/port() in parallel workers

Hi,

While investigating a customer issue it's turned out that if a
parallel worker executes inet_server_addr() and inet_server_port() the
results are always null because MyProcPort is not set in parallel
workers. We can reproduce it in all supported versions higher than
9.6. Here is an example:

postgres=# select inet_server_addr(), inet_server_port();
 inet_server_addr | inet_server_port
------------------+------------------
 172.254.99.88    |             5432
(1 row)

postgres=# select inet_client_addr(), inet_client_port();
 inet_client_addr | inet_client_port
------------------+------------------
 172.254.99.109   |            31383
(1 row)

postgres=# set force_parallel_mode to on;
SET
postgres=# select inet_server_addr(), inet_server_port();
 inet_server_addr | inet_server_port
------------------+------------------
                  |
(1 row)

postgres=# select inet_client_addr(), inet_client_port();
 inet_client_addr | inet_client_port
------------------+------------------
 172.254.99.109   |            31383
(1 row)

To fix this issue, I think there are two options:

1. Inherits the local address information from postmaster or the
leader to parallel workers.
2. Have both inet_server_addr() and inet_server_port() parallel restricted.

Since auxiliary processes including parallel workers don't communicate
directly to the client, this information is basically not necessary.
Also, the option #1 needs code change to pass the information, which
brings complexity and is not good in terms of back-patching. So I'd
prefer the option #2. I've attached the patch doing the option #2.

Regards,

-- 
Masahiko Sawada
EnterpriseDB:  https://www.enterprisedb.com/

Masahiko Sawada <sawada.mshk@gmail.com> writes:
> While investigating a customer issue it's turned out that if a
> parallel worker executes inet_server_addr() and inet_server_port() the
> results are always null because MyProcPort is not set in parallel
> workers.

Check.

> To fix this issue, I think there are two options:
> 1. Inherits the local address information from postmaster or the
> leader to parallel workers.
> 2. Have both inet_server_addr() and inet_server_port() parallel restricted.

> Since auxiliary processes including parallel workers don't communicate
> directly to the client, this information is basically not necessary.
> Also, the option #1 needs code change to pass the information, which
> brings complexity and is not good in terms of back-patching. So I'd
> prefer the option #2. I've attached the patch doing the option #2.

Um, but #2 is surely not back-patchable.  Changing initial catalog
contents in a back branch is a big PITA, so we generally wouldn't
even consider it except for critical security problems.  Which this
is not.

Having said that, I'm fine with the idea of just marking these
parallel-restricted in HEAD and ignoring the problem in the back
branches.

            regards, tom lane

BTW, just for the archives' sake: inet_client_addr() and
inet_client_port() also access MyProcPort, so they'd also have
this issue, except that they're already marked parallel-restricted.
So somebody just missed the server equivalents when marking the
parallel safety of built-in functions.

contrib/sslinfo is also full of uses of MyProcPort, but all its
functions are properly marked parallel-restricted.

I did not see any other risky-looking uses of MyProcPort in a
quick grep.

            regards, tom lane

I wrote:
> Having said that, I'm fine with the idea of just marking these
> parallel-restricted in HEAD and ignoring the problem in the back
> branches.

Hearing no complaints, done that way.

            regards, tom lane

On Thu, Jan 14, 2021 at 6:24 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>
> I wrote:
> > Having said that, I'm fine with the idea of just marking these
> > parallel-restricted in HEAD and ignoring the problem in the back
> > branches.
>
> Hearing no complaints, done that way.

Although I thought that backpatching would be helpful for users who
start to use the supported PostgreSQL, I agree not to backpatching
since the likelihood that parallel workers execute these functions is
low.

Thank you for committing the patch!

Regards,

--
Masahiko Sawada
EnterpriseDB:  https://www.enterprisedb.com/