On Mon, Oct 5, 2020 at 9:45 AM Fujii Masao <masao.fu...@oss.nttdata.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