Re: Retry Cached Remote Connections for postgres_fdw in case remote backend gets killed/goes away

On Mon, Oct 5, 2020 at 9:45 AM Fujii Masao <masao(dot)fujii(at)oss(dot)nttdata(dot)com> wrote:
>
> > 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?
>
> I was thinking that elog() basically should be used to report this
> debug message, instead, but you used ereport() because maybe
> you'd like to add detail message about connection error. Is this
> understanding right? elog() uses errmsg_internal().
>

Yes that's correct.

>
> So if ereport() is used as an aternative of elog() for some reasons,
> IMO errmsg_internal() should be used. Thought?
>

Yes, this is apt for our case.

>
> OTOH, the messages you mentioned are not debug ones,
> so basically ereport() and errmsg() should be used, I think.
>

In connection.c file, yes they are of ERROR type. Looks like it's not
a standard to use errmsg_internal for DEBUG messages that require no
translation with ereport

(errmsg("wrote block details for %d blocks", num_blocks)));
(errmsg("MultiXact member stop limit is now %u based on MultiXact %u
(errmsg("oldest MultiXactId member is at offset %u",

However, there are few other places, where errmsg_internal is used for
DEBUG purposes.

(errmsg_internal("finished verifying presence of "
(errmsg_internal("%s(%d) name: %s; blockState:

Having said that, IMHO it's better to keep the way it is currently in
the code base.

>
> > Thanks. Attaching v10 patch. Please have a look.
>
> Thanks for updating the patch! I will mark the patch as ready for committer in CF.
> Barring any objections, I will commit that.
>

Thanks a lot for the review comments.

> >>
> >>> 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.
>
> Sounds good!
> ISTM that he function should at least check the target process is PostgreSQL one.
>

Thanks. I will take care of this point.

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