Re: Proposal: Recent mutated table tracking in memory

Hi Nadav,

> Hi Tatsuo,
>
> Looks good to me thanks!
>
> Please go ahead with your review. waiting to hear back from you.

Here are the code review results.

diff --git a/doc/src/sgml/loadbalance.sgml b/doc/src/sgml/loadbalance.sgml
index 9e1e7b39b..7384ce81a 100644
--- a/doc/src/sgml/loadbalance.sgml
+++ b/doc/src/sgml/loadbalance.sgml
:
+ <sect2 id="runtime-config-table-mutation-map">
+ <title>Table Mutation Map Configuration (Lagless Replica Reads)</title>

"(Lagless Replica Reads)" sounds like an advertisement to me. It
should be removed.

+ <para>
+ These parameters configure the track table mutation feature, which is activated by setting
+ <xref linkend="guc-disable-load-balance-on-write"> to <literal>dml_adaptive_global</literal>.
+ The feature tracks recently written tables to prevent stale reads from replica nodes during
+ replication lag, implementing the "lagless" architecture pattern for distributed systems
+ with read replicas.

I think the feature does not guarantee "lagless" anytime, in all cases.

+ <para>
+ This feature requires time-based replication delay monitoring. This can be provided by either
+ <xref linkend="guc-replication-delay-source-cmd"> (external command mode) or by setting
+ <xref linkend="guc-delay-threshold-by-time"> (which uses <literal>pg_stat_replication.replay_lag</literal>
+ from PostgreSQL 10+). At least one of these must be configured for the TTL calculation to work.

If one of these is not set, what happens? Error? Need to describe it.

+ </para>
+
+ <warning>
+ <para>
+ Enabling <literal>dml_adaptive_global</literal> increases shared memory consumption. With default settings,
+ the feature requires approximately 6.4 MB of shared memory (0.1 MB for table tracking + 6.3 MB for query cache).

"query cache" should be "query parse cache".

+ Memory usage scales with configuration parameters:
+ </para>
+ <itemizedlist>
+ <listitem>
+ <para>
+ Table tracking: <literal>track_table_mutation_table_size * 40 bytes</literal> (default: 2048 * 40 = ~80 KB)
+ </para>
+ </listitem>
+ <listitem>
+ <para>
+ Query cache: <literal>track_table_mutation_query_parse_cache_size * 640 bytes</literal> (default: 10000 * 640 = ~6.3 MB)

"query cache" should be "query parse cache".

+ <title>Limitations</title>

I think number of tables tacked in a SELECT is limited to 8. It should
be mentioned.

diff --git a/src/context/pool_query_context.c b/src/context/pool_query_context.c
index a056ac596..0190d3673 100644
--- a/src/context/pool_query_context.c
+++ b/src/context/pool_query_context.c
@@ -1828,15 +1829,23 @@ is_in_list(char *name, List *list)
static bool
is_select_object_in_temp_write_list(Node *node, void *context)
{
- if (node == NULL || pool_config->disable_load_balance_on_write != DLBOW_DML_ADAPTIVE)
+ if (node == NULL ||
+ !DLBOW_IS_DML_ADAPTIVE(
+ pool_config->disable_load_balance_on_write))

You don't need to split the line.

+ is_adaptive = DLBOW_IS_DML_ADAPTIVE(
+ pool_config->disable_load_balance_on_write);

You don't need to split the line.

- if (pool_config->disable_load_balance_on_write == DLBOW_DML_ADAPTIVE && session_context->is_in_transaction)
+ if (is_adaptive &&
+ session_context->is_in_transaction)
{
ereport(DEBUG1,
(errmsg("is_select_object_in_temp_write_list: \"%s\", found relation \"%s\"", (char *) context, rgv->relname)));
This line is too long. Please split.

@@ -1880,7 +1889,13 @@ static char *get_associated_object_from_dml_adaptive_relations
void
check_object_relationship_list(char *name, bool is_func_name)
{
- if (pool_config->disable_load_balance_on_write == DLBOW_DML_ADAPTIVE && pool_config->parsed_dml_adaptive_object_relationship_list)
+ bool is_adaptive;
+
+ is_adaptive = DLBOW_IS_DML_ADAPTIVE(
+ pool_config->disable_load_balance_on_write);

I wrote in the commit message:

modifications are only detected in the same transaction). Note,
however, you cannot use dml_adaptive_object_relationship_list to track
dependency among table and other objects.

In my understanding the feature does not use
dml_adaptive_object_relationship_list. If this is correct, why
check_object_relationship_list() is called here in case
dml_adaptive_global? If the feature uses
dml_adaptive_object_relationship_list, test cases should be included.

diff --git a/src/utils/pool_track_table_mutation.c b/src/utils/pool_track_table_mutation.c
new file mode 100644
index 000000000..9be46b28f
--- /dev/null
+++ b/src/utils/pool_track_table_mutation.c

It seems following functions are not used anywhere. I wonder if this
feature actually use "query parse cache".

pool_track_table_mutation_get_cached_parse
pool_track_table_mutation_cache_parse
pool_track_table_mutation_normalize_and_hash

Besides the code review, I mutated one of regression tests to check
whether the feature co exists with in the existing memory query cache
feature. After attached patch applied, I ran 006.memqcache and got the
following result.

cd src/test/regression
./regress.sh 006
creating pgpool-II temporary installation ...
moving pgpool_setup to temporary installation path ...
moving watchdog_setup to temporary installation path ...
using pgpool-II at /home/t-ishii/work/Pgpool-II/current/pgpool2/src/test/regression/temp/installed
*************************
REGRESSION MODE : install
Pgpool-II version : pgpool-II version 4.8devel (mitsukakeboshi)
Pgpool-II install path : /home/t-ishii/work/Pgpool-II/current/pgpool2/src/test/regression/temp/installed
PostgreSQL bin : /usr/local/pgsql/bin
PostgreSQL Major version : 18
pgbench : /usr/local/pgsql/bin/pgbench
PostgreSQL jdbc : /usr/local/pgsql/share/postgresql-9.2-1003.jdbc4.jar
*************************
testing 006.memqcache...failed.
out of 1 ok:0 failed:1 timeout:0

log/006.memqcache shows:

../expected.txt result.txt differ: char 1, line 1

So I checked the test script and found the error was generated by a
Java program test.

java jdbctest > result.txt 2>&1
cmp ../expected.txt result.txt
if [ $? != 0 ];then
./shutdownall
exit 1
fi

In jdbctest.java:

/*
* Cache test in an explicit transaction
*/
conn.setAutoCommit(false);
// execute DML. This should prevent SELECTs from using query cache in the transaction.
sql = "UPDATE t1 SET i = 2;";
pst = conn.createStatement();
pst.executeUpdate(sql);
pst.close();
// should not use the cache and should return "2", rather than "1"
prest = conn.prepareStatement("SELECT * FROM t1");
rs = prest.executeQuery();

The expected file (expected.txt) has "2" but the result file
(testdir/result.txt) was "1". This is the reason why the test
failed. I wonder if there's something wrong with the feature when the
query cache is enabled. Can you look into this?

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