From: | Bharath Rupireddy <bharath(dot)rupireddyforpostgres(at)gmail(dot)com> |
---|---|
To: | Zhihong Yu <zyu(at)yugabyte(dot)com> |
Cc: | Alexey Kondratov <a(dot)kondratov(at)postgrespro(dot)ru>, "Hou, Zhijie" <houzj(dot)fnst(at)cn(dot)fujitsu(dot)com>, PostgreSQL Hackers <pgsql-hackers(at)lists(dot)postgresql(dot)org>, Fujii Masao <masao(dot)fujii(at)oss(dot)nttdata(dot)com> |
Subject: | Re: [PATCH] postgres_fdw connection caching - cause remote sessions linger till the local session exit |
Date: | 2021-01-02 05:23:40 |
Message-ID: | CALj2ACVq+8N4fgWu6fR7+6MgtLWoVJj2a=vzJvsoUfwGxn_BcQ@mail.gmail.com |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
Thanks for taking a look at the patches.
On Fri, Jan 1, 2021 at 9:35 PM Zhihong Yu <zyu(at)yugabyte(dot)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.
With Regards,
Bharath Rupireddy.
EnterpriseDB: http://www.enterprisedb.com
Attachment | Content-Type | Size |
---|---|---|
v6-0001-postgres_fdw-function-to-discard-cached-connectio.patch | application/octet-stream | 40.6 KB |
v6-0002-postgres_fdw-add-keep_connections-GUC-to-not-cach.patch | application/octet-stream | 9.7 KB |
v6-0003-postgres_fdw-server-level-option-keep_connection-.patch | application/octet-stream | 11.7 KB |
From | Date | Subject | |
---|---|---|---|
Next Message | Michael Paquier | 2021-01-02 05:25:33 | Re: Moving other hex functions to /common |
Previous Message | Justin Pryzby | 2021-01-01 21:49:30 | Re: adding wait_start column to pg_locks |