my-ship-it commented on issue #1862:
URL: https://github.com/apache/cloudberry/issues/1862#issuecomment-5065699804
Hi @nix-oss, thank you so much for this excellent report! 🙌 The root cause
analysis, the reproduction steps, and the proposed fix are all spot-on — this
is one of the nicest bug reports we've received. I've verified it against the
current `main` branch and can confirm everything you described.
**A bit of history to back you up:** I traced this with `git log -L`, and
the inverted logic came in with commit `0080be29610` ("Simplify RG cpuset
seperated by coordinator/segment (#14637)", Dec 2022), which refactored the
original `strtok`-based implementation. The pre-refactor code actually had it
right:
```c
/* Get result cpuset by gprole, on master or segment */
if (Gp_role == GP_ROLE_EXECUTE && arraycpuset[1] != NULL)
splitcpuset = arraycpuset[1]; /* segment gets the second part */
else
splitcpuset = arraycpuset[0]; /* coordinator gets the first part */
```
The two branches simply got swapped during the rewrite. Funnily enough, the
variable names still tell the true story: the dispatcher branch returns `scpu`
(segment cpu) and the segment branch builds `mcpu` (master cpu) — so your
reading of the intent is exactly right. Upstream GPDB 7 picked up the same code
from the same PR, so it inherits this bug too.
**One thing worth adding to your impact analysis:** on homogeneous clusters
the bug doesn't just go unnoticed — the DDL succeeds and the affinity is
silently applied to the wrong roles (coordinator gets the segment cores and
vice versa). So the fix helps everyone, not only heterogeneous deployments.
Your heterogeneous setup is what finally made it visible — great catch!
**A small suggestion if you send a PR:** it might be worth making both
branches return a palloc'd copy, rather than returning `first + 1` (a pointer
into the caller's string) on one path. There's a call site in `InitResGroups`
(`resgroup.c`, the fallback-core path) that does `snprintf(cpuset2,
MaxCpuSetLength, ...)` on the returned pointer, which writes through into the
caller's `caps.cpuset` buffer. That hazard exists both before and after the
swap, so it'd be lovely to clean it up in the same patch. A small unit test on
`getCpuSetByRole()` would also help guard against re-regression, since
single-host test setups can't tell the two roles' cpusets apart.
Please do feel encouraged to open that PR — you've already done the hard
part, and we'd be glad to review it. Thanks again for the careful investigation
and for making Cloudberry better! 🚀
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
