Prev: is_absolute_path incorrect on Windows
Next: Streaming Replication: Checkpoint_segment and wal_keep_segmentson standby
From: Takahiro Itagaki on 31 May 2010 22:55

"Takahiro Itagaki" <itagaki.takahiro(a)oss.ntt.co.jp> wrote:

> Bug reference: 5487
> Logged by: Takahiro Itagaki
> Email address: itagaki.takahiro(a)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

From: Heikki Linnakangas on 1 Jun 2010 05:00
On 01/06/10 05:55, Takahiro Itagaki wrote:
> "Takahiro Itagaki"<itagaki.takahiro(a)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

--
Sent via pgsql-hackers mailing list (pgsql-hackers(a)postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

From: Takahiro Itagaki on 2 Jun 2010 02:46

Heikki Linnakangas <heikki.linnakangas(a)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

From: Takahiro Itagaki on 2 Jun 2010 21:07

Heikki Linnakangas <heikki.linnakangas(a)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

 | 
Pages: 1
Prev: is_absolute_path incorrect on Windows
Next: Streaming Replication: Checkpoint_segment and wal_keep_segmentson standby