Hi Chao, On Thu, Jun 11, 2026 at 5:51 PM Chao Li <[email protected]> wrote: > I’m testing "[28972b6fc] Add support for importing statistics from remote > servers". Functionally, so far so good. I just found a piece of suspicious > code that might be an oversight in the follow-up commit aa1f93a3387: > ``` > /* > * Compare two RemoteAttributeMappings for sorting. > */ > static int > remattrmap_cmp(const void *v1, const void *v2) > { > const RemoteAttributeMapping *r1 = v1; > const RemoteAttributeMapping *r2 = v2; > > return strncmp(r1->remote_attname, r2->remote_attname, NAMEDATALEN); > } > ``` > > This function compares remote attribute names only up to NAMEDATALEN. But > aa1f93a3387 changed remote_attname from a NAMEDATALEN-byte array to a char > pointer, and its commit message explicitly says "the remote column name in > particular could be longer than NAMEDATALEN - 1". In that case, > remattrmap_cmp() could treat two distinct remote column names as equal for > sorting, while match_attrmap() later compares the full strings. > > Also, aa1f93a3387’s commit message mentions match_attrmap(), and that > function uses strcmp(). So I guess remattrmap_cmp() should also use strcmp(). > > I don’t have a repro that triggers a bad result. I just want to report and > confirm whether that code should be changed. I made a local change as the > attached diff, and no test is broken with the change.
You are right; that's an oversight in the follow-up commit. The patch looks good to me, so I'll push it after confirming that there are no similar mistakes. Thanks for the report and patch! Best regards, Etsuro Fujita
