Re: [PATCH] postgres_fdw connection caching - cause remote sessions linger till the local session exit

On Sat, Jan 2, 2021 at 10:53 AM Bharath Rupireddy
<bharath.rupireddyforpostgres@gmail.com> wrote:
> Thanks for taking a look at the patches.
>
> On Fri, Jan 1, 2021 at 9:35 PM Zhihong Yu <zyu@yugabyte.com> wrote:
> > Happy new year.
> >
> > +       appendStringInfo(&buf, "(%s, %s)", server->servername,
> > +                        entry->invalidated ? "false" : "true");
> >
> > Is it better to use 'invalidated' than 'false' in the string ?
>
> This point was earlier discussed in [1] and [2], but the agreement was
> on having true/false [2] because of a simple reason specified in [1],
> that is when some users have foreign server names as invalid or valid,
> then the output is difficult to interpret which one is what. With
> having true/false, it's easier. IMO, let's keep the true/false as is,
> since it's also suggested in [2].
>
> [1] - https://www.postgresql.org/message-id/CALj2ACUv%3DArQXs0U9PM3YXKCeSzJ1KxRokDY0g_0aGy--kDScA%40mail.gmail.com
> [2] - https://www.postgresql.org/message-id/6da38393-6ae5-4d87-2690-11c932123403%40oss.nttdata.com
>
> > For the first if block of postgres_fdw_disconnect():
> >
> > +        * Check if the connection associated with the given foreign server is
> > +        * in use i.e. entry->xact_depth > 0. Since we can not close it, so
> > +        * error out.
> > +        */
> > +       if (is_in_use)
> > +           ereport(WARNING,
> >
> > since is_in_use is only set in the if (server) block, I think the above warning can be moved into that block.
>
> Modified that a bit. Since we error out when no server object is
> found, then no need of keeping the code in else part. We could save on
> some indentation
>
> +        if (!server)
> +            ereport(ERROR,
> +                    (errcode(ERRCODE_CONNECTION_DOES_NOT_EXIST),
> +                     errmsg("foreign server \"%s\" does not exist",
> servername)));
> +
> +        hashvalue = GetSysCacheHashValue1(FOREIGNSERVEROID,
> +                                          ObjectIdGetDatum(server->serverid));
> +        result = disconnect_cached_connections(hashvalue, false, &is_in_use);
> +
> +        /*
> +         * Check if the connection associated with the given foreign server is
> +         * in use i.e. entry->xact_depth > 0. Since we can not close it, so
> +         * error out.
> +         */
> +        if (is_in_use)
> +            ereport(WARNING,
> +                    (errmsg("cannot close the connection because it
> is still in use")));
>
> Attaching v6 patch set. Please have a look.

I'm sorry for the mess. I missed adding the new files into the v6-0001
patch. Please ignore the v6 patch set and consder the v7 patch set for
further review. Note that 0002 and 0003 patches have no difference
from v5 patch set.

With Regards,
Bharath Rupireddy.
EnterpriseDB: http://www.enterprisedb.com