Re: bgw_type (was Re: Why does logical replication launcher set application_name?)

> On 31 Aug 2017, at 21:49, Peter Eisentraut <peter(dot)eisentraut(at)2ndquadrant(dot)com> wrote:
>
> On 5/30/17 23:10, Peter Eisentraut wrote:
>> Here is a proposed solution that splits bgw_name into bgw_type and
>> bgw_name_extra. bgw_type shows up in pg_stat_activity.backend_type.
>> Uses of application_name are removed, because they are no longer
>> necessary to identity the process type.
>
> Updated patch incorporating the feedback. I have kept bgw_name as it
> was and just added bgw_type completely independently.

The patch applies with minor fuzz, compiles without introduced warnings and
work the way it says on the tin. The utility of the proposed functionality is
a clear win so +1 on getting that in.

> One open question is how to treat a missing (empty) bgw_type. I
> currently fill in bgw_name as a fallback. We could also treat it as an
> error or a warning as a transition measure.

Warnings tend to stick around forever in my experience. An error would work as
a transition measure, but it seems a hammer too far.

Falling back on bgw_name solves there being data with the risk that some
workers will have a wonky type displayed, and those are the ones that of course
will never get updated. Either going with NULL as Michael suggested elsewhere
in this thread (my preference as well) or a default string along the lines of
“Unknown type” would probably carry more incentive to update.

A few random comments on the patch:

Regarding the following hunk:

+ process listing, for example. <structfield>bgw_name</structfield> on the
+ other hand can contain additional information about the specific process.
+ (Typically, the string for <structfield>bgw_name</structfield> will contain
+ the string for <structfield>bgw_type</structfield> somehow, but that is not
+ strictly required.)

This reads a bit too (oddly) detailed to me, I would’ve phrased it as something
along the lines of:

"Typically, the string for bgw_name will contain the type somehow, but that
is not strictly required.”

I find omitting “background worker” in the following hunk is leaving out
valuable information for bgworkers with badly configured types, but it’s
definitely a matter of taste rather than a straight-up patch critique:

- errmsg("terminating background worker \"%s\" due to administrator command",
- MyBgworkerEntry->bgw_name)));
+ errmsg("terminating %s due to administrator command",
+ MyBgworkerEntry->bgw_type)));

Updating the status to “Ready for committer” with the discussion around missing
bgw_type left as a decision point for a committer.

cheers ./daniel