[pgpool-hackers: 3934] Re: [pgpool-committers: 7824] pgpool: Refactor "SHOW pool_pool_pools" and pcp_proc_info.

[Tatsuo Ishii]

I just want to explain more about this commit.

To modify output data of pcp_proc_info command, following 4 functions
at least are needed to be modified. Moreover, each function must have
knowledge about the data structure (POOL_REPORT_POOLS) related to the
data: this means that we need to repeat apply similar modifications to
each function, which is not only boring but counter productive.

inform_process_info (pcp_worker.c)	/* collect information and send to client */
process_process_info_response (pcp.c)	/* receive information from pcp_server and store into buffer */
pcp_process_info (pcp.c)	/* send request to pcp server */
output_procinfo_result (pcp_frontend_client.c)	/* print data */

So what I did was:

1) Modify POOL_REPORT_POOLS so that it only contains string
members. This eliminates the necessity of converting binary to string
(from pcp server to client) and string to binary (in pcp client).

2) With 2, the only necessary knowledge about the POOL_REPORT_POOLS
is, number of struct members and relative offset from the beginning of
the struct. It's capsuled in a function pool_report_pools_offsets().

3) Once the struct is filled with data by calling get_pools() in
inform_process_info(), the data can be passed rest of the functions
without any knowledge about the struct.

4) Example of coding style change is here (inform_process_info).

Before:
			snprintf(proc_pid, sizeof(proc_pid), "%d", pools[i].pool_pid);
			snprintf(proc_start_time, sizeof(proc_start_time), "%ld", pools[i].start_time);
			snprintf(proc_create_time, sizeof(proc_create_time), "%ld", pools[i].create_time);
			snprintf(majorversion, sizeof(majorversion), "%d", pools[i].pool_majorversion);
			snprintf(minorversion, sizeof(minorversion), "%d", pools[i].pool_minorversion);
			snprintf(pool_counter, sizeof(pool_counter), "%d", pools[i].pool_counter);
			snprintf(backend_id, sizeof(backend_pid), "%d", pools[i].backend_id);
			snprintf(backend_pid, sizeof(backend_pid), "%d", pools[i].pool_backendpid);
			snprintf(connected, sizeof(connected), "%d", pools[i].pool_connected);

			pcp_write(frontend, "p", 1);
			wsize = htonl(sizeof(code) +
						  strlen(proc_pid) + 1 +
						  strlen(pools[i].database) + 1 +
						  strlen(pools[i].username) + 1 +
						  strlen(proc_start_time) + 1 +
						  strlen(proc_create_time) + 1 +
						  strlen(majorversion) + 1 +
						  strlen(minorversion) + 1 +
						  strlen(pool_counter) + 1 +
						  strlen(backend_id) + 1 +
						  strlen(backend_pid) + 1 +
						  strlen(connected) + 1 +
						  sizeof(int));
			pcp_write(frontend, &wsize, sizeof(int));
			pcp_write(frontend, code, sizeof(code));
			pcp_write(frontend, proc_pid, strlen(proc_pid) + 1);
			pcp_write(frontend, pools[i].database, strlen(pools[i].database) + 1);
			pcp_write(frontend, pools[i].username, strlen(pools[i].username) + 1);
			pcp_write(frontend, proc_start_time, strlen(proc_start_time) + 1);
			pcp_write(frontend, proc_create_time, strlen(proc_create_time) + 1);
			pcp_write(frontend, majorversion, strlen(majorversion) + 1);
			pcp_write(frontend, minorversion, strlen(minorversion) + 1);
			pcp_write(frontend, pool_counter, strlen(pool_counter) + 1);
			pcp_write(frontend, backend_id, strlen(backend_id) + 1);
			pcp_write(frontend, backend_pid, strlen(backend_pid) + 1);
			pcp_write(frontend, connected, strlen(connected) + 1);
			do_pcp_flush(frontend);

After:
			pcp_write(frontend, "p", 1);

			wsize = 0;
			for (j = 0; j < n; j++)
			{
				wsize += strlen((char *)&pools[i] + offsets[j]) + 1;
			}
			wsize += sizeof(code) + sizeof(int);
			wsize = htonl(wsize);

			/* send packet length to frontend */
			pcp_write(frontend, &wsize, sizeof(int));
			/* send "this is a record" to frontend */
			pcp_write(frontend, code, sizeof(code));

			/* send each process info data to frontend */
			for (j = 0; j < n; j++)
			{
				pcp_write(frontend, (char *)&pools[i] + offsets[j], strlen((char *)&pools[i] + offsets[j]) + 1);
			}

			do_pcp_flush(frontend);

Another area of refactoring is, the format string for printf() in output_procinfo_result().

Before:

		frmt = "Database     : %s\n"
			"Username     : %s\n"
			"Start time   : %s\n"
			"Creation time: %s\n"
			"Major        : %d\n"
			"Minor        : %d\n"
			"Counter      : %d\n"
			"Backend PID  : %d\n"
			"Connected    : %d\n"
			"PID          : %d\n"
			"Backend ID   : %d\n";

After:
	const char *titles[] = {
		"Database", "Username", "Start time", "Creation time", 
		"Major", "Minor", "Counter", "Backend PID",
		"Connected", "PID", "Backend ID"
	};

	format = format_titles(titles, types, sizeof(titles)/sizeof(char *));

Format_titles() automatically calculates the necessary padding spaces for given titles.

If we want to add a new field to the output of pcp_proc_info,
functions we need to modify are:

- pool_getnodes()
- pool_report_pools_offsets()
- output_procinfo_result()

> 7 files changed, 173 insertions(+), 320 deletions(-)

With all the refactoring, 320 - 173 = 147 lines are removed. I
believe these give small contributions for making Pgpool-II hackers
life easier:-)

Best regards,
--
Tatsuo Ishii
SRA OSS, Inc. Japan
English: http://www.sraoss.co.jp/index_en.php
Japanese:http://www.sraoss.co.jp

From: Tatsuo Ishii <ishii at sraoss.co.jp>
Subject: [pgpool-committers: 7824] pgpool: Refactor "SHOW pool_pool_pools" and pcp_proc_info.
Date: Wed, 16 Jun 2021 13:04:27 +0000
Message-ID: <E1ltVDX-0001bG-5e at gothos.postgresql.org>

> Refactor "SHOW pool_pool_pools" and pcp_proc_info.
> 
> These commands had many duplicated codes and did not use
> infrastructures to make the code shorter. Now they are fixed and it is
> much easier to add new data.
> 
> Branch
> ------
> master
> 
> Details
> -------
> https://git.postgresql.org/gitweb?p=pgpool2.git;a=commitdiff;h=c7690453aed0a5c3b177e38e4f0aa09b56aff1ee
> 
> Modified Files
> --------------
> src/include/pcp/libpcp_ext.h               |  21 ++--
> src/include/utils/pool_process_reporting.h |   4 +-
> src/libs/pcp/pcp.c                         |  94 ++++----------
> src/pcp_con/pcp_worker.c                   |  79 +++++-------
> src/tools/pcp/pcp_frontend_client.c        |  69 +++++-----
> src/utils/pool_health_check_stats.c        |  30 ++++-
> src/utils/pool_process_reporting.c         | 196 +++++++----------------------
> 7 files changed, 173 insertions(+), 320 deletions(-)
>