Date: Mon, 25 Aug 2025 11:18:25 +0900 (JST)
Message-Id: <20250825.111825.1765418484481637087.ishii@postgresql.org>
To: nadav@tailorbrands.com
Cc: pgpool-hackers@lists.postgresql.org
Subject: Re: Proposal: recent access based routing for primary-replica
 setups
From: Tatsuo Ishii <ishii@postgresql.org>
In-Reply-To: 
 <CACeKOO000N8gUodkXt_1V=XQYx_utxXdyOxkzV7GYSPpdHnTfg@mail.gmail.com>
References: 
 <CACeKOO3KF9ZdhkvgM+znSaj6dkTyMR77xCebQk9iuTfVavkYbA@mail.gmail.com>
	<20250821.192332.768193620076940871.ishii@postgresql.org>
	<CACeKOO000N8gUodkXt_1V=XQYx_utxXdyOxkzV7GYSPpdHnTfg@mail.gmail.com>
Mime-Version: 1.0
Content-Type: Multipart/Mixed;
 boundary="--Next_Part(Mon_Aug_25_11_18_25_2025_086)--"
Content-Transfer-Encoding: 7bit
Archived-At: 
 <https://www.postgresql.org/message-id/20250825.111825.1765418484481637087.ishii%40postgresql.org>
Precedence: bulk

----Next_Part(Mon_Aug_25_11_18_25_2025_086)--
Content-Type: Text/Plain; charset=us-ascii
Content-Transfer-Encoding: 7bit

Hi Nadav,

Thank you for the patch!

I have one question. How do you provide a password (sr_check_password)
while executing replication_delay_source_cmd as sr_check_user? In my
understanding replication_delay_source_cmd is executed through su
command in your patch. In this case su command tries to read the
password from terminal. I don't see such a code in the patch.

BTW, I start to think that executing replication_delay_source_cmd as
sr_check_user might not be a good idea. sr_check_user is a database
user, not OS user. In PostgreSQL they are not necessarily the
same. Also doing su in pgpool process needs to be very carefully to
avoid vulnerability. Probably we just execute it as pgpool OS user?

Lastly when I apply the patches using git apply, there are some
trailing space errors.

$ git apply ~/external-lag-feature-implementation.patch 
/home/t-ishii/external-lag-feature-implementation.patch:314: trailing whitespace.
			
/home/t-ishii/external-lag-feature-implementation.patch:317: trailing whitespace.
			
/home/t-ishii/external-lag-feature-implementation.patch:318: trailing whitespace.
			cmd_len = strlen(escaped_cmd) + 
/home/t-ishii/external-lag-feature-implementation.patch:320: trailing whitespace.
			
/home/t-ishii/external-lag-feature-implementation.patch:322: trailing whitespace.
			snprintf(full_command, cmd_len, "su - %s -c '%s'", 
warning: squelched 4 whitespace errors
warning: 9 lines add whitespace errors.

$ git apply ~/external-lag-feature-tests.patch 
/home/t-ishii/external-lag-feature-tests.patch:87: trailing whitespace.
- test_parsing.sh: Unit test for parsing logic  
/home/t-ishii/external-lag-feature-tests.patch:440: trailing whitespace.
# Test 2: Float values  
warning: 2 lines add whitespace errors.

Also I have some compilation errors after patching the source
code. See attached compilation log.

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

----Next_Part(Mon_Aug_25_11_18_25_2025_086)--
Content-Type: Text/Plain; charset=us-ascii
Content-Transfer-Encoding: 7bit
Content-Disposition: inline; filename="compile.log"

Making all in src
make[1]: Entering directory '/home/t-ishii/work/Pgpool-II/current/pgpool2/src'
Making all in parser
make[2]: Entering directory '/home/t-ishii/work/Pgpool-II/current/pgpool2/src/parser'
make[2]: Nothing to be done for 'all'.
make[2]: Leaving directory '/home/t-ishii/work/Pgpool-II/current/pgpool2/src/parser'
Making all in libs
make[2]: Entering directory '/home/t-ishii/work/Pgpool-II/current/pgpool2/src/libs'
Making all in pcp
make[3]: Entering directory '/home/t-ishii/work/Pgpool-II/current/pgpool2/src/libs/pcp'
make[3]: Nothing to be done for 'all'.
make[3]: Leaving directory '/home/t-ishii/work/Pgpool-II/current/pgpool2/src/libs/pcp'
make[3]: Entering directory '/home/t-ishii/work/Pgpool-II/current/pgpool2/src/libs'
make[3]: Nothing to be done for 'all-am'.
make[3]: Leaving directory '/home/t-ishii/work/Pgpool-II/current/pgpool2/src/libs'
make[2]: Leaving directory '/home/t-ishii/work/Pgpool-II/current/pgpool2/src/libs'
Making all in watchdog
make[2]: Entering directory '/home/t-ishii/work/Pgpool-II/current/pgpool2/src/watchdog'
make[2]: Nothing to be done for 'all'.
make[2]: Leaving directory '/home/t-ishii/work/Pgpool-II/current/pgpool2/src/watchdog'
Making all in .
make[2]: Entering directory '/home/t-ishii/work/Pgpool-II/current/pgpool2/src'
gcc -DHAVE_CONFIG_H -DDEFAULT_CONFIGDIR=\"/usr/local/etc\" -DPGSQL_BIN_DIR=\"/usr/local/pgsql/bin\" -I. -I../src/include  -D_GNU_SOURCE -I /usr/local/pgsql/include   -g -O2 -Wall -Wmissing-prototypes -Wmissing-declarations -Wno-format-truncation -Wno-stringop-truncation -fno-strict-aliasing -c -o streaming_replication/pool_worker_child.o streaming_replication/pool_worker_child.c
streaming_replication/pool_worker_child.c: In function 'do_worker_child':
streaming_replication/pool_worker_child.c:266:5: warning: this 'else' clause does not guard... [-Wmisleading-indentation]
  266 |     else
      |     ^~~~
streaming_replication/pool_worker_child.c:270:6: note: ...this statement, but the latter is misleadingly indented as if it were guarded by the 'else'
  270 |      node_status = verify_backend_node_status(slots);
      |      ^~~~~~~~~~~
In file included from streaming_replication/pool_worker_child.c:56:
streaming_replication/pool_worker_child.c: In function 'check_replication_time_lag_with_cmd':
../src/include/utils/elog.h:397:25: warning: unused variable 'save_context_stack' [-Wunused-variable]
  397 |   ErrorContextCallback *save_context_stack = error_context_stack; \
      |                         ^~~~~~~~~~~~~~~~~~
streaming_replication/pool_worker_child.c:786:2: note: in expansion of macro 'PG_TRY'
  786 |  PG_TRY();
      |  ^~~~~~
../src/include/utils/elog.h:396:15: warning: unused variable 'save_exception_stack' [-Wunused-variable]
  396 |   sigjmp_buf *save_exception_stack = PG_exception_stack; \
      |               ^~~~~~~~~~~~~~~~~~~~
streaming_replication/pool_worker_child.c:786:2: note: in expansion of macro 'PG_TRY'
  786 |  PG_TRY();
      |  ^~~~~~
../src/include/utils/elog.h:405:3: error: expected 'while' before 'else'
  405 |   else \
      |   ^~~~
streaming_replication/pool_worker_child.c:961:2: note: in expansion of macro 'PG_CATCH'
  961 |  PG_CATCH();
      |  ^~~~~~~~
../src/include/utils/elog.h:412:24: error: 'save_exception_stack' undeclared (first use in this function); did you mean 'PG_exception_stack'?
  412 |   PG_exception_stack = save_exception_stack; \
      |                        ^~~~~~~~~~~~~~~~~~~~
streaming_replication/pool_worker_child.c:980:2: note: in expansion of macro 'PG_END_TRY'
  980 |  PG_END_TRY();
      |  ^~~~~~~~~~
../src/include/utils/elog.h:412:24: note: each undeclared identifier is reported only once for each function it appears in
  412 |   PG_exception_stack = save_exception_stack; \
      |                        ^~~~~~~~~~~~~~~~~~~~
streaming_replication/pool_worker_child.c:980:2: note: in expansion of macro 'PG_END_TRY'
  980 |  PG_END_TRY();
      |  ^~~~~~~~~~
../src/include/utils/elog.h:413:25: error: 'save_context_stack' undeclared (first use in this function); did you mean 'error_context_stack'?
  413 |   error_context_stack = save_context_stack; \
      |                         ^~~~~~~~~~~~~~~~~~
streaming_replication/pool_worker_child.c:980:2: note: in expansion of macro 'PG_END_TRY'
  980 |  PG_END_TRY();
      |  ^~~~~~~~~~
streaming_replication/pool_worker_child.c:743:9: warning: variable 'cmd_allocated' set but not used [-Wunused-but-set-variable]
  743 |  bool   cmd_allocated = false;
      |         ^~~~~~~~~~~~~
In file included from streaming_replication/pool_worker_child.c:56:
streaming_replication/pool_worker_child.c: At top level:
../src/include/utils/elog.h:414:4: error: expected identifier or '(' before 'while'
  414 |  } while (0)
      |    ^~~~~
streaming_replication/pool_worker_child.c:980:2: note: in expansion of macro 'PG_END_TRY'
  980 |  PG_END_TRY();
      |  ^~~~~~~~~~
streaming_replication/pool_worker_child.c:983:2: error: expected identifier or '(' before 'if'
  983 |  if (line)
      |  ^~
streaming_replication/pool_worker_child.c:985:2: error: expected identifier or '(' before 'if'
  985 |  if (escaped_cmd)
      |  ^~
streaming_replication/pool_worker_child.c:987:2: error: expected identifier or '(' before 'if'
  987 |  if (cmd_allocated && command)
      |  ^~
streaming_replication/pool_worker_child.c:990:2: warning: data definition has no type or storage class
  990 |  error_context_stack = callback.previous;
      |  ^~~~~~~~~~~~~~~~~~~
streaming_replication/pool_worker_child.c:990:2: warning: type defaults to 'int' in declaration of 'error_context_stack' [-Wimplicit-int]
streaming_replication/pool_worker_child.c:990:2: error: conflicting types for 'error_context_stack'
In file included from streaming_replication/pool_worker_child.c:56:
../src/include/utils/elog.h:360:42: note: previous declaration of 'error_context_stack' was here
  360 | extern PGDLLIMPORT ErrorContextCallback *error_context_stack;
      |                                          ^~~~~~~~~~~~~~~~~~~
streaming_replication/pool_worker_child.c:990:24: error: 'callback' undeclared here (not in a function); did you mean 'calloc'?
  990 |  error_context_stack = callback.previous;
      |                        ^~~~~~~~
      |                        calloc
streaming_replication/pool_worker_child.c:991:1: error: expected identifier or '(' before '}' token
  991 | }
      | ^
In file included from streaming_replication/pool_worker_child.c:56:
streaming_replication/pool_worker_child.c: In function 'get_query_result':
../src/include/utils/elog.h:397:46: warning: initialization of 'ErrorContextCallback *' {aka 'struct ErrorContextCallback *'} from 'int' makes pointer from integer without a cast [-Wint-conversion]
  397 |   ErrorContextCallback *save_context_stack = error_context_stack; \
      |                                              ^~~~~~~~~~~~~~~~~~~
streaming_replication/pool_worker_child.c:1099:2: note: in expansion of macro 'PG_TRY'
 1099 |  PG_TRY();
      |  ^~~~~~
../src/include/utils/elog.h:408:24: warning: assignment to 'int' from 'ErrorContextCallback *' {aka 'struct ErrorContextCallback *'} makes integer from pointer without a cast [-Wint-conversion]
  408 |    error_context_stack = save_context_stack
      |                        ^
streaming_replication/pool_worker_child.c:1103:2: note: in expansion of macro 'PG_CATCH'
 1103 |  PG_CATCH();
      |  ^~~~~~~~
../src/include/utils/elog.h:413:23: warning: assignment to 'int' from 'ErrorContextCallback *' {aka 'struct ErrorContextCallback *'} makes integer from pointer without a cast [-Wint-conversion]
  413 |   error_context_stack = save_context_stack; \
      |                       ^
streaming_replication/pool_worker_child.c:1112:2: note: in expansion of macro 'PG_END_TRY'
 1112 |  PG_END_TRY();
      |  ^~~~~~~~~~
make[2]: *** [Makefile:883: streaming_replication/pool_worker_child.o] Error 1
make[2]: Leaving directory '/home/t-ishii/work/Pgpool-II/current/pgpool2/src'
make[1]: *** [Makefile:949: all-recursive] Error 1
make[1]: Leaving directory '/home/t-ishii/work/Pgpool-II/current/pgpool2/src'
make: *** [Makefile:413: all-recursive] Error 1

----Next_Part(Mon_Aug_25_11_18_25_2025_086)----