Thread: 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
Attachment
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
Attachment
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