From: | Bharath Rupireddy <bharath(dot)rupireddyforpostgres(at)gmail(dot)com> |
---|---|
To: | Fujii Masao <masao(dot)fujii(at)oss(dot)nttdata(dot)com> |
Cc: | PostgreSQL-development <pgsql-hackers(at)postgresql(dot)org> |
Subject: | Re: Retry Cached Remote Connections for postgres_fdw in case remote backend gets killed/goes away |
Date: | 2020-10-03 11:40:26 |
Message-ID: | CALj2ACUFyeEtjqXrShMbiRuNmKfMxRs8Fg2YMJ0Y9AJGgJ5ahw@mail.gmail.com |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
On Fri, Oct 2, 2020 at 11:30 PM Fujii Masao <masao(dot)fujii(at)oss(dot)nttdata(dot)com> wrote:
>
> > Attaching v8 patch, please review it.. Both make check and make
> > check-world passes on v8.
>
> Thanks for updating the patch! It basically looks good to me.
> I tweaked the patch as follows.
>
> + if (!entry->conn ||
> + PQstatus(entry->conn) != CONNECTION_BAD ||
>
> With the above change, if entry->conn is NULL, an error is thrown and no new
> connection is reestablished. But why? IMO it's more natural to reestablish
> new connection in that case. So I removed "!entry->conn" from the above
> condition.
>
Yeah, that makes sense.
>
> + ereport(DEBUG3,
> + (errmsg("could not start remote transaction on connection %p",
> + entry->conn)),
>
> I replaced errmsg() with errmsg_internal() because the translation of
> this debug message is not necessary.
>
I'm okay with this as we don't have any specific strings that need
translation in the debug message. But, should we also try to have
errmsg_internal in a few other places in connection.c?
errmsg("could not obtain message string for remote error"),
errmsg("cannot PREPARE a transaction that has
operated on postgres_fdw foreign tables")));
errmsg("password is required"),
I see the errmsg() with plain texts in other places in the code base
as well. Is it that we look at the error message and if it is a plain
text(without database objects or table data), we decide to have no
translation? Or is there any other policy?
>
> +SELECT pg_terminate_backend(pid) FROM pg_stat_activity WHERE
> +backend_type = 'client backend' AND application_name = 'fdw_retry_check';
> +CALL wait_for_backend_termination();
>
> Since we always use pg_terminate_backend() and wait_for_backend_termination()
> together, I merged them into one function.
>
> I simplied the comments on the regression test.
>
+1. I slightly adjusted comments in connection.c and ran pg_indent to
keep them upt 80 char limit.
>
> Attached is the updated version of the patch. If this patch is ok,
> I'd like to mark it as ready for committer.
>
Thanks. Attaching v10 patch. Please have a look.
>
> > I have another question not related to this patch: though we have
> > wait_pid() function, we are not able to use it like
> > pg_terminate_backend() in other modules, wouldn't be nice if we can
> > make it generic under the name pg_wait_pid() and usable across all pg
> > modules?
>
> I thought that, too. But I could not come up with good idea for *real* use case
> of that function. At least that's useful for the regression test, though.
> Anyway, IMO it's worth proposing that and hearing more opinions about that
> from other hackers.
>
Yes it will be useful for testing when coupled with
pg_terminate_backend(). I will post the idea in a separate thread soon
for more thoughts.
With Regards,
Bharath Rupireddy.
EnterpriseDB: http://www.enterprisedb.com
Attachment | Content-Type | Size |
---|---|---|
v10-Retry-Cached-Remote-Connections-For-postgres_fdw.patch | application/x-patch | 7.8 KB |
From | Date | Subject | |
---|---|---|---|
Next Message | Petru Ghita | 2020-10-03 12:13:01 | RE: POC: contrib/unaccent as IMMUTABLE |
Previous Message | Amit Langote | 2020-10-03 11:26:19 | Re: a misbehavior of partition row movement (?) |