Re: BUG #5487: dblink failed with 63 bytes connection names

The following bug has been logged online:

Bug reference: 5487
Logged by: Takahiro Itagaki
Email address: itagaki(dot)takahiro(at)oss(dot)ntt(dot)co(dot)jp
PostgreSQL version: 9.0beta1
Operating system: Linux
Description: dblink failed with 63 bytes connection names
Details:

Contib/dblink module seems to have a bug in handling
connection names in NAMEDATALEN-1 bytes.

It cannot use exiting connections with 63 bytes name
in some cases. For example, we cannot disconnect
such connections. Also, we can reconnect with the
same name and will have two connections with the name.

=# SELECT dblink_connect(repeat('1234567890', 6) || 'ABC',
'host=localhost');
dblink_connect
----------------
OK
(1 row)

=# SELECT dblink_get_connections();
dblink_get_connections
-------------------------------------------------------------------
{123456789012345678901234567890123456789012345678901234567890ABC}
(1 row)

=# SELECT dblink_disconnect(repeat('1234567890', 6) || 'ABC');
ERROR: connection
"123456789012345678901234567890123456789012345678901234567890ABC" not
available

"Takahiro Itagaki" <itagaki(dot)takahiro(at)oss(dot)ntt(dot)co(dot)jp> wrote:

> Bug reference: 5487
> Logged by: Takahiro Itagaki
> Email address: itagaki(dot)takahiro(at)oss(dot)ntt(dot)co(dot)jp
> Description: dblink failed with 63 bytes connection names
> Details:
>
> Contib/dblink module seems to have a bug in handling
> connection names in NAMEDATALEN-1 bytes.

Here is a patch to fix the bug. I think it comes from wrong usage
of snprintf(NAMEDATALEN - 1). It just copies 62 bytes + \0.

In addition, it should be safe to use pg_mbcliplen() to truncate
extra bytes in connection names because we might return invalid
text when a multibyte character is at 62 or 63 bytes.

Note that the fix should be ported to previous versions, too.

> It cannot use exiting connections with 63 bytes name
> in some cases. For example, we cannot disconnect
> such connections. Also, we can reconnect with the
> same name and will have two connections with the name.
>
> =# SELECT dblink_connect(repeat('1234567890', 6) || 'ABC',
> 'host=localhost');
> dblink_connect
> ----------------
> OK
> (1 row)
>
> =# SELECT dblink_get_connections();
> dblink_get_connections
> -------------------------------------------------------------------
> {123456789012345678901234567890123456789012345678901234567890ABC}
> (1 row)
>
> =# SELECT dblink_disconnect(repeat('1234567890', 6) || 'ABC');
> ERROR: connection
> "123456789012345678901234567890123456789012345678901234567890ABC" not
> available

Regards,
---
Takahiro Itagaki
NTT Open Source Software Center

On 01/06/10 05:55, Takahiro Itagaki wrote:
> "Takahiro Itagaki"<itagaki(dot)takahiro(at)oss(dot)ntt(dot)co(dot)jp> wrote:
>>
>> Contib/dblink module seems to have a bug in handling
>> connection names in NAMEDATALEN-1 bytes.
>
> Here is a patch to fix the bug. I think it comes from wrong usage
> of snprintf(NAMEDATALEN - 1). It just copies 62 bytes + \0.
>
> In addition, it should be safe to use pg_mbcliplen() to truncate
> extra bytes in connection names because we might return invalid
> text when a multibyte character is at 62 or 63 bytes.

Hmm, seems that dblink should call truncate_identifier() for the
truncation, to be consistent with truncation of table names etc.

I also spotted this in dblink.c:

> /* first gather the server connstr options */
> if (strlen(servername) < NAMEDATALEN)
> foreign_server = GetForeignServerByName(servername, true);

I think that's wrong. We normally consistently truncate identifiers at
creation and at use, so that if you create an object with a very long
name and it's truncated, you can still refer to it with the untruncated
name because all such references are truncated too.

--
Heikki Linnakangas
EnterpriseDB http://www.enterprisedb.com

Heikki Linnakangas <heikki(dot)linnakangas(at)enterprisedb(dot)com> wrote:

> Hmm, seems that dblink should call truncate_identifier() for the
> truncation, to be consistent with truncation of table names etc.

Hmmm, we need the same routine with truncate_identifier(), but we hard
to use the function because it modifies the input buffer directly.
Since all of the name strings in dblink is const char *, I added
a bit modified version of the function as truncate_identifier_copy()
in the attached v2 patch.

> I also spotted this in dblink.c:
>
> > /* first gather the server connstr options */
> > if (strlen(servername) < NAMEDATALEN)
> > foreign_server = GetForeignServerByName(servername, true);
>
> I think that's wrong. We normally consistently truncate identifiers at
> creation and at use, so that if you create an object with a very long
> name and it's truncated, you can still refer to it with the untruncated
> name because all such references are truncated too.

Absolutely. I re-use the added function for the fix.

Regards,
---
Takahiro Itagaki
NTT Open Source Software Center

On 02/06/10 09:46, Takahiro Itagaki wrote:
>
> Heikki Linnakangas<heikki(dot)linnakangas(at)enterprisedb(dot)com> wrote:
>
>> Hmm, seems that dblink should call truncate_identifier() for the
>> truncation, to be consistent with truncation of table names etc.
>
> Hmmm, we need the same routine with truncate_identifier(), but we hard
> to use the function because it modifies the input buffer directly.
> Since all of the name strings in dblink is const char *, I added
> a bit modified version of the function as truncate_identifier_copy()
> in the attached v2 patch.

Well, looking at the callers, most if not all of them seem to actually
pass a palloc'd copy, allocated by text_to_cstring().

Should we throw a NOTICE like vanilla truncate_identifier() does?

I feel it would be better to just call truncate_identifier() than roll
your own. Assuming we want the same semantics in dblink, we'll otherwise
have to remember to update truncate_identifier_copy() with any changes
to truncate_identifier().

--
Heikki Linnakangas
EnterpriseDB http://www.enterprisedb.com

Heikki Linnakangas <heikki(dot)linnakangas(at)enterprisedb(dot)com> wrote:

> Well, looking at the callers, most if not all of them seem to actually
> pass a palloc'd copy, allocated by text_to_cstring().
>
> Should we throw a NOTICE like vanilla truncate_identifier() does?
>
> I feel it would be better to just call truncate_identifier() than roll
> your own. Assuming we want the same semantics in dblink, we'll otherwise
> have to remember to update truncate_identifier_copy() with any changes
> to truncate_identifier().

That makes sense. Now I use truncate_identifier(warn=true) for the fix.

Regards,
---
Takahiro Itagaki
NTT Open Source Software Center