Re: [PR] interconnect: prune cursor IC history table by local xid, not distributed xid [cloudberry]

["Alena0704 (via GitHub)" <gi...@apache.org>]

Alena0704 commented on PR #1964:
URL: https://github.com/apache/cloudberry/pull/1964#issuecomment-6315469218

   LGTM. I'd only suggest adding a TAP regression test like the one below.
   
   The test runs read-only autocommit queries through one persistent connection and verifies that UDP interconnect history is pruned and memory usage remains bounded. It also checks that pruning does not occur within a transaction with an open cursor, and that `WITH HOLD` cursors remain usable after commit and subsequent pruning.
   
   Across 10 local runs per version, the test consistently failed before the fix and passed all 14 checks afterward. 
   
   <details>
   <summary>TAP test prototype</summary>
   
   ```perl
   use strict;
   use warnings;
   use IPC::Run qw(run timeout);
   use Test::More;
   
   my $connstr = $ENV{CBDB_TEST_CONNSTR};
   plan skip_all => 'set CBDB_TEST_CONNSTR to a test MPP coordinator'
     unless defined $connstr && length $connstr;
   
   # A single psql process keeps the same QD backend. With -f and AUTOCOMMIT,
   # each generated SELECT is a separate transaction. DO or repeated safe_psql()
   # calls would respectively use one transaction or different backend sessions.
   my $sql = <<'SQL';
   \set AUTOCOMMIT on
   SET client_min_messages = warning;
   SET optimizer = off;
   SET statement_timeout = '30s';
   SELECT current_setting('gp_interconnect_type') = 'udpifc'
      AND current_setting('gp_role') = 'dispatch' AS suitable
   \gset
   \if :suitable
   \else
     \echo Test requires a udpifc MPP coordinator
     \quit 3
   \endif
   SELECT 'PID_START|' || pg_backend_pid();
   CREATE TEMP TABLE ic_history_probe (i int) DISTRIBUTED BY (i);
   INSERT INTO ic_history_probe SELECT generate_series(1, 10);
   EXPLAIN (COSTS OFF) SELECT count(*) FROM ic_history_probe;
   
   -- Warm up connections and reach the history table's normal high-water mark.
   SELECT 'SELECT count(*) FROM ic_history_probe;'
   FROM generate_series(1, 300)
   \gexec
   SELECT 'USED_BEFORE|' || used_bytes
   FROM pg_backend_memory_contexts WHERE name = 'UdpInterconnectMemContext';
   
   -- No writes between measurements: the old distributed xid stays invalid.
   SET gp_log_interconnect = debug;
   SET client_min_messages = debug1;
   \warn IC_AUTOCOMMIT_BEGIN
   SELECT 'SELECT count(*) FROM ic_history_probe;'
   FROM generate_series(1, 1024)
   \gexec
   \warn IC_AUTOCOMMIT_END
   SET client_min_messages = warning;
   SELECT 'USED_AFTER|' || used_bytes
   FROM pg_backend_memory_contexts WHERE name = 'UdpInterconnectMemContext';
   
   -- Enough rows to leave a cursor with an active interconnect after FETCH 1.
   CREATE TEMP TABLE ic_history_cursor_data (i int) DISTRIBUTED BY (i);
   INSERT INTO ic_history_cursor_data SELECT generate_series(1, 100000);
   BEGIN;
   DECLARE c CURSOR FOR SELECT i FROM ic_history_cursor_data ORDER BY i;
   \echo CURSOR_FIRST
   FETCH 1 FROM c;
   SET client_min_messages = debug1;
   \warn IC_CURSOR_BEGIN
   SELECT 'SELECT count(*) FROM ic_history_probe;'
   FROM generate_series(1, 300)
   \gexec
   SAVEPOINT s;
   SELECT count(*) FROM ic_history_probe;
   ROLLBACK TO s;
   INSERT INTO ic_history_probe VALUES (11);
   SELECT count(*) FROM ic_history_probe;
   \echo CURSOR_NEXT
   FETCH 1 FROM c;
   \warn IC_CURSOR_END
   SET client_min_messages = warning;
   ROLLBACK;
   
   -- A holdable cursor must survive pruning in later transactions.
   BEGIN;
   DECLARE h CURSOR WITH HOLD FOR
     SELECT i FROM ic_history_cursor_data ORDER BY i;
   FETCH 1 FROM h;
   COMMIT;
   SET client_min_messages = debug1;
   \warn IC_HOLD_BEGIN
   SELECT 'SELECT count(*) FROM ic_history_probe;'
   FROM generate_series(1, 300)
   \gexec
   \warn IC_HOLD_END
   SET client_min_messages = warning;
   \echo HOLD_NEXT
   FETCH 1 FROM h;
   CLOSE h;
   SELECT 'PID_END|' || pg_backend_pid();
   SQL
   
   my ($stdout, $stderr) = ('', '');
   my $success = run [ 'psql', '-XAtq', '-v', 'ON_ERROR_STOP=1',
     '-d', $connstr, '-f', '-' ], '<', \$sql, '>', \$stdout, '2>', \$stderr,
     timeout(120);
   ok($success, 'all SQL completed without errors');
   unless ($success)
   {
     diag(substr($stdout, -4000), substr($stderr, -12000));
     done_testing();
     exit 1;
   }
   
   like($stdout, qr/Gather Motion/, 'probe actually uses interconnect');
   my ($pid_start) = $stdout =~ /^PID_START\|(\d+)$/m;
   my ($pid_end) = $stdout =~ /^PID_END\|(\d+)$/m;
   ok(defined($pid_start) && defined($pid_end) && $pid_start == $pid_end,
     'all checks used the same backend');
   
   my ($before) = $stdout =~ /^USED_BEFORE\|(\d+)$/m;
   my ($after) = $stdout =~ /^USED_AFTER\|(\d+)$/m;
   ok(defined($before) && defined($after), 'UDP memory context is present');
   SKIP:
   {
     skip 'missing memory measurements', 1
       unless defined($before) && defined($after);
     # History retains at most 257 entries after pruning. Allow 16 KiB of
     # growth instead of depending on exact allocator overhead or segment count.
     # The unfixed code retains another 1024 entries (about 48 KiB on 64-bit).
     cmp_ok($after - $before, '<', 16 * 1024,
       'autocommit read-only statements do not accumulate IC history');
     diag("UDP context: before=$before, after=$after, growth=" . ($after - $before));
   }
   
   my $prune_message = qr/prune cursor history table \(count \d+\), icid \d+/;
   for my $phase (qw(AUTOCOMMIT CURSOR HOLD))
   {
     my ($messages) = $stderr =~ /^IC_${phase}_BEGIN\r?\n(.*?)^IC_${phase}_END\r?$/ms;
     ok(defined($messages), "$phase phase captured");
     SKIP:
     {
       skip 'missing phase markers', 1 unless defined($messages);
       if ($phase eq 'CURSOR')
       {
         ok($messages !~ $prune_message,
           'no pruning within a transaction, including savepoint and write');
       }
       else
       {
         ok(scalar($messages =~ $prune_message), "$phase actually prunes history");
       }
     }
   }
   like($stdout, qr/^CURSOR_FIRST\r?\n1\r?$/m, 'cursor starts at row 1');
   like($stdout, qr/^CURSOR_NEXT\r?\n2\r?$/m, 'open cursor continues at row 2');
   like($stdout, qr/^HOLD_NEXT\r?\n2\r?$/m, 'WITH HOLD continues after commit and pruning');
   done_testing();
   ```
   
   </details>


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@cloudberry.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@cloudberry.apache.org
For additional commands, e-mail: commits-help@cloudberry.apache.org