You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@cloudberry.apache.org by "yjhjstz (via GitHub)" <gi...@apache.org> on 2026/09/04 17:25:11 UTC
[PR] interconnect: prune cursor IC history table by local xid, not distributed xid [cloudberry]
yjhjstz opened a new pull request, #1964:
URL: https://github.com/apache/cloudberry/pull/1964
SetupUDPIFCInterconnect_Internal() records one CursorICHistoryEntry per interconnect instance on the QD and prunes old entries only when the statement runs in a different transaction than the previous one, using the distributed xid as the transaction identity. Distributed xids are assigned lazily (only for transactions that write or are dispatched two-phase), so autocommit read-only statements always see InvalidDistributedTransactionId on both sides of the comparison. The table is therefore never pruned on read-only sessions and grows by one 48-byte entry per Motion-containing statement for the life of the backend (2.8 MB after 60000 statements, plus the heap fragmentation the steady stream of small long-lived allocations induces).
Use MyProc->lxid instead: it is assigned to every top-level transaction, including read-only ones, and stays constant within a transaction block, so entries belonging to cursors that are still open in the current transaction remain protected exactly as before.
Verified on a single long-lived session running the queries from #1947: UdpInterconnectMemContext grew 0 -> 2814 KB over 60000 statements before the change and stays at 12 KB after it; RssAnon went 12.7 -> 24.6 MB before and is flat at 14.4 MB after. Pruning still does not happen inside an explicit transaction block or while a cursor is open, and FETCH from an open cursor after 300 intervening statements works.
Fixes #1947
### What does this PR do?
<!-- Brief overview of the changes, including any major features or fixes -->
### Type of Change
- [ ] Bug fix (non-breaking change)
- [ ] New feature (non-breaking change)
- [ ] Breaking change (fix or feature with breaking changes)
- [ ] Documentation update
### Breaking Changes
<!-- Remove if not applicable. If yes, explain impact and migration path -->
### Test Plan
<!-- How did you test these changes? -->
- [ ] Unit tests added/updated
- [ ] Integration tests added/updated
- [ ] Passed `make installcheck`
- [ ] Passed `make -C src/test installcheck-cbdb-parallel`
### Impact
<!-- Remove sections that don't apply -->
**Performance:**
<!-- Any performance implications? -->
**User-facing changes:**
<!-- Any changes visible to users? -->
**Dependencies:**
<!-- New dependencies or version changes? -->
### Checklist
- [ ] Followed [contribution guide](https://cloudberry.apache.org/contribute/code)
- [ ] Added/updated documentation
- [ ] Reviewed code for security implications
- [ ] This PR contains AI-assisted code generation
- [ ] Requested review from [cloudberry committers](https://github.com/orgs/apache/teams/cloudberry-committers)
### Additional Context
<!-- Any other information that would help reviewers? Remove if none -->
### CI Skip Instructions
<!--
To skip CI builds, add the appropriate CI skip identifier to your PR title.
The identifier must:
- Be in square brackets []
- Include the word "ci" and either "skip" or "no"
- Only use for documentation-only changes or when absolutely necessary
-->
---
<!-- Join our community:
- Mailing list: [dev@cloudberry.apache.org](https://lists.apache.org/list.html?dev@cloudberry.apache.org) (subscribe: dev-subscribe@cloudberry.apache.org)
- Discussions: https://github.com/apache/cloudberry/discussions -->
--
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
Re: [PR] interconnect: prune cursor IC history table by local xid, not distributed xid [cloudberry]
Posted by "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
Re: [PR] interconnect: prune cursor IC history table by local xid, not distributed xid [cloudberry]
Posted by "yjhjstz (via GitHub) via commits" <co...@cloudberry.apache.org>.
yjhjstz merged PR #1964:
URL: https://github.com/apache/cloudberry/pull/1964
--
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
Re: [PR] interconnect: prune cursor IC history table by local xid, not distributed xid [cloudberry]
Posted by "yjhjstz (via GitHub)" <gi...@apache.org>.
yjhjstz commented on PR #1964:
URL: https://github.com/apache/cloudberry/pull/1964#issuecomment-5573005068
> LGTM,
>
> Changing the pruning rule must not destroy is the history entry of an interconnect instance belonging to a still-open cursor.
>
> And this is true:
>
> * Non-holdable cursors can only exist inside an explicit transaction block (commands/portalcmds.c:77-78, RequireTransactionBlock), where lxid is constant → no prune.
> * WITH HOLD cursors are drained into a tuplestore at commit (PersistHoldablePortal, portalcmds.c:424) and the executor is shut down, so no live interconnect crosses the boundary.
>
> Also here we fixed other bug with cursors:
>
> BEGIN; DECLARE c CURSOR FOR ; -- dxid still Invalid FETCH 1 FROM c; INSERT ...; -- setupDtxTransaction() assigns gxid N Do something with ; -- Invalid != N → prune fires, c still open, got race here
>
> Maybe we should add here regression tests, but I'm not sure if they are valid for our case, we could add it and be sure that covered the issue. So in overall LGTM
I tested cursor case, It works fine. But hard to add to regression test.
--
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