Обсуждение: Re: [BUGS] BUG #5487: dblink failed with 63 bytes connection names

"Takahiro Itagaki" <itagaki.takahiro@oss.ntt.co.jp> wrote:

> Bug reference:      5487
> Logged by:          Takahiro Itagaki
> Email address:      itagaki.takahiro@oss.ntt.co.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.takahiro@oss.ntt.co.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.linnakangas@enterprisedb.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.linnakangas@enterprisedb.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.linnakangas@enterprisedb.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