Re: shared-memory based stats collector

At Wed, 10 Mar 2021 21:47:51 +0900, Fujii Masao <masao(dot)fujii(at)oss(dot)nttdata(dot)com> wrote in
> Attached is the updated version of the 0003 patch. Barring any
> objection,
> I will commit this patch.
>
>
> -#include "storage/latch.h"
> -#include "storage/proc.h"
>
> I removed these because they are no longer necessary.

Mmm. Sorry for the garbage.

> <literal>logical replication worker</literal>,
> <literal>parallel worker</literal>, <literal>background
> writer</literal>,
> <literal>client backend</literal>, <literal>checkpointer</literal>,
> + <literal>archiver</literal>,
> <literal>startup</literal>, <literal>walreceiver</literal>,
> <literal>walsender</literal> and <literal>walwriter</literal>.
>
> In the document about pg_stat_activity, possible values in
> backend_type
> column are all listed. I added "archiver" into the list.
>
> BTW, those values were originally listed in alphabetical order,
> but ISTM that they were gotten out of order at a certain moment.
> So they should be listed in alphabetical order again. This should
> be implemented as a separate patch.

Thanks for adding it.

They are also mildly sorted by function or characteristics. I'm not
sure which is better, but anyway they should be the order based on a
clear criteria.

> -PgArchData *PgArch = NULL;
> +static PgArchData *PgArch = NULL;
>
> I marked PgArchData as static because it's used only in pgarch.c.

Right.

> - ShmemInitStruct("Archiver ", PgArchShmemSize(), &found);
> + ShmemInitStruct("Archiver Data", PgArchShmemSize(), &found);
>
> I found that the original shmem name ended with unnecessary space
> character.
> I replaced it with "Archiver Data".

Oops. The trailing space is where I stopped writing the string and try
to find a better word, then in the meanwhile, my mind have been
attracted to something else and left. I don't object to "Archiver
Data". Thanks for completing it.

> In reaper(), I moved the code block for archiver to the original
> location.

Agreed.

> I ran pgindent for the files that the patch modifies.

Yeah, I forgot to add the new struct into typedefs.list. I
intentionally omitted clearing newly-acquired shared memory but it
doesn't matter to do that.

So, I'm fine with it. Thanks for taking this.

regards.

--
Kyotaro Horiguchi
NTT Open Source Software Center