postgres_fdw: possible overlooked NAMEDATALEN comparison in stats import
Hi,
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.
Best regards,
--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/
Attachments:
remattrmap_cmp.diffapplication/octet-stream; name=remattrmap_cmp.diff; x-unix-mode=0644Download+1-1
Hi Chao,
On Thu, Jun 11, 2026 at 5:51 PM Chao Li <li.evan.chao@gmail.com> 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
On Thu, Jun 11, 2026 at 7:59 PM Etsuro Fujita <etsuro.fujita@gmail.com> wrote:
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.
As I couldn't find any, I pushed it.
Best regards,
Etsuro Fujita
On Jun 14, 2026, at 15:22, Etsuro Fujita <etsuro.fujita@gmail.com> wrote:
On Thu, Jun 11, 2026 at 7:59 PM Etsuro Fujita <etsuro.fujita@gmail.com> wrote:
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.As I couldn't find any, I pushed it.
Best regards,
Etsuro Fujita
Thanks a lot for pushing.
Best regards,
--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/