Patch for pl/tcl Tcl_ExternalToUtf and Tcl_UtfToExternal support

Is there a way to make this automatically enabled based on the TCL
version? Does that make sense?

This patch adds calls to Tcl_ExternalToUtf and Tcl_UtfToExternal
functions on parameters of SPI functions.

Without this calls it's imposiible to work with 8-bit text in pl/tcl
functions with tcl-8.3.

Patch assumes that database encoding and system encoding of Tcl is
equal.

Patch is agains current CVS sources. It adds new configure switch
--enable-pltcl-utf. Without this switch patch does nothing.

Content-Description:

[ Attachment, skipping... ]

---------------------------(end of broadcast)---------------------------
TIP 6: Have you searched our list archives?

http://www.postgresql.org/search.mpl

-- 
  Bruce Momjian                        |  http://candle.pha.pa.us
  pgman@candle.pha.pa.us               |  (610) 853-3000
  +  If your life is a hard drive,     |  830 Blythe Avenue
  +  Christ can be your backup.        |  Drexel Hill, Pennsylvania 19026

On Thu, 23 Aug 2001, Bruce Momjian wrote:

Is there a way to make this automatically enabled based on the TCL
version? Does that make sense?

This adds significant overhead to pl/tcl calls :((
so I'm not sure that anybody want this functionality enabled by default

Got it. Thanks.

On Thu, 23 Aug 2001, Bruce Momjian wrote:

Is there a way to make this automatically enabled based on the TCL
version? Does that make sense?

This adds significant overhead to pl/tcl calls :((
so I'm not sure that anybody want this functionality enabled by default

-- 
  Bruce Momjian                        |  http://candle.pha.pa.us
  pgman@candle.pha.pa.us               |  (610) 853-3000
  +  If your life is a hard drive,     |  830 Blythe Avenue
  +  Christ can be your backup.        |  Drexel Hill, Pennsylvania 19026

Vsevolod Lobko <seva@sevasoft.kiev.ua> writes:

This patch adds calls to Tcl_ExternalToUtf and Tcl_UtfToExternal
functions on parameters of SPI functions.

I hate to say it, but this is an amazingly ugly patch. Can't you
do it without so many #ifdefs and duplicating a lot of code? Think
of the next guy who has to look at/work on this code.

Possibly a macro that invokes Tcl_ExternalToUtfDString or does nothing
might help.

Patch assumes that database encoding and system encoding of Tcl is
equal.

Hmm, is that a tenable assumption? I don't know, I'm just asking.

It adds new configure switch
--enable-pltcl-utf. Without this switch patch does nothing.

This is not a good approach, since it relies on the user to know whether
he needs to do this or not. Seems like looking at the Tcl version
number would be more appropriate: if version >= 8.3 then make these
changes. You could do that in the code, without any configure patch,
using #if's on TCL_MAJOR_VERSION and TCL_MINOR_VERSION (see libpgtcl
for examples).

regards, tom lane

On Thu, 23 Aug 2001, Tom Lane wrote:

Vsevolod Lobko <seva@sevasoft.kiev.ua> writes:

This patch adds calls to Tcl_ExternalToUtf and Tcl_UtfToExternal
functions on parameters of SPI functions.

I hate to say it, but this is an amazingly ugly patch. Can't you
do it without so many #ifdefs and duplicating a lot of code? Think
of the next guy who has to look at/work on this code.

There are only two problems:
1) I need temporary Tcl_DString
2) I need to free it after use
So I can go with three macros (one for declaration, one for conversion,
and one for free) but I dont think it will be better

Possibly a macro that invokes Tcl_ExternalToUtfDString or does nothing
might help.

Patch assumes that database encoding and system encoding of Tcl is
equal.

Hmm, is that a tenable assumption? I don't know, I'm just asking.

Yes, because it does 8-bit to unicode conversion and must to know
codepage for 8-bit characters. Unfortunately charset names for tcl and postgres
does not match, so this demands additional field in charset tables or additional
table :((

It adds new configure switch
--enable-pltcl-utf. Without this switch patch does nothing.

This is not a good approach, since it relies on the user to know whether
he needs to do this or not. Seems like looking at the Tcl version
number would be more appropriate: if version >= 8.3 then make these
changes. You could do that in the code, without any configure patch,
using #if's on TCL_MAJOR_VERSION and TCL_MINOR_VERSION (see libpgtcl
for examples).

See previous reply, this patch adds significant overhead.

It adds new configure switch
--enable-pltcl-utf. Without this switch patch does nothing.

This is not a good approach, since it relies on the user to know whether
he needs to do this or not. Seems like looking at the Tcl version
number would be more appropriate: if version >= 8.3 then make these
changes. You could do that in the code, without any configure patch,
using #if's on TCL_MAJOR_VERSION and TCL_MINOR_VERSION (see libpgtcl
for examples).

I asked him this already and he said that there is extra overhead for
the conversion that many wouldn't want.

-- 
  Bruce Momjian                        |  http://candle.pha.pa.us
  pgman@candle.pha.pa.us               |  (610) 853-3000
  +  If your life is a hard drive,     |  830 Blythe Avenue
  +  Christ can be your backup.        |  Drexel Hill, Pennsylvania 19026

On Thu, 23 Aug 2001, Tom Lane wrote:

Vsevolod Lobko <seva@sevasoft.kiev.ua> writes:

This patch adds calls to Tcl_ExternalToUtf and Tcl_UtfToExternal
functions on parameters of SPI functions.

I hate to say it, but this is an amazingly ugly patch. Can't you
do it without so many #ifdefs and duplicating a lot of code? Think
of the next guy who has to look at/work on this code.

Possibly a macro that invokes Tcl_ExternalToUtfDString or does nothing
might help.

something like this?

#ifdef ENABLE_PLTCL_UTF
#warning bubu
# define UTF_BEGIN do { Tcl_DString _pltcl_ds_tmp;
# define UTF_END Tcl_DStringFree(&_pltcl_ds_tmp); } while (0);
# define UTF_U2E(x) (Tcl_UtfToExternalDString(NULL,(x),-1,\
&_pltcl_ds_tmp))
# define UTF_E2U(x) (Tcl_ExternalToUtfDString(NULL,(x),-1,\
&_pltcl_ds_tmp))
#else /* ENABLE_PLTCL_UTF */
# define UTF_BEGIN
# define UTF_END
# define UTF_U2E(x) (x)
# define UTF_E2U(x) (x)
#endif /* ENABLE_PLTCL_UTF */

and

if (part != NULL)
{
UTF_BEGIN
Tcl_DStringAppend(&unknown_src, UTF_E2U(part), -1);
UTF_END
pfree(part);
}

Is this looks better?

Vsevolod Lobko <seva@sevasoft.kiev.ua> writes:

#ifdef ENABLE_PLTCL_UTF
#warning bubu
# define UTF_BEGIN do { Tcl_DString _pltcl_ds_tmp;
# define UTF_END Tcl_DStringFree(&_pltcl_ds_tmp); } while (0);
# define UTF_U2E(x) (Tcl_UtfToExternalDString(NULL,(x),-1,\
&_pltcl_ds_tmp))
# define UTF_E2U(x) (Tcl_ExternalToUtfDString(NULL,(x),-1,\
&_pltcl_ds_tmp))
#else /* ENABLE_PLTCL_UTF */
# define UTF_BEGIN
# define UTF_END
# define UTF_U2E(x) (x)
# define UTF_E2U(x) (x)
#endif /* ENABLE_PLTCL_UTF */

and

if (part != NULL)
{
UTF_BEGIN
Tcl_DStringAppend(&unknown_src, UTF_E2U(part), -1);
UTF_END
pfree(part);
}

Is this looks better?

It does, but one small gripe: the lack of semicolons will probably cause
pg_indent to mess up the indentation. (I know emacs' autoindent mode
will not work nicely with it, either.) Please set up the macros so that
you write

UTF_BEGIN;
Tcl_DStringAppend(&unknown_src, UTF_E2U(part), -1);
UTF_END;

and then I'll be happy.

Your point about overhead is a good one, so I retract the gripe about
using a configure switch. But please include documentation patches to
describe the configure option in the administrator's guide (installation
section).

regards, tom lane

Vsevolod Lobko writes:

Is there a way to make this automatically enabled based on the TCL
version? Does that make sense?

This adds significant overhead to pl/tcl calls :((
so I'm not sure that anybody want this functionality enabled by default

It is my understanding that Tcl 8.3 will not work with 8 bit characters
without this functionality. That would simply be unacceptable. If users
want better performance they should use a different Tcl version, but we
should not override Tcl's design decisions unilaterally.

--
Peter Eisentraut peter_e@gmx.net http://funkturm.homeip.net/~peter

On Thu, 23 Aug 2001, Peter Eisentraut wrote:

Vsevolod Lobko writes:

Is there a way to make this automatically enabled based on the TCL
version? Does that make sense?

This adds significant overhead to pl/tcl calls :((
so I'm not sure that anybody want this functionality enabled by default

It is my understanding that Tcl 8.3 will not work with 8 bit characters
without this functionality. That would simply be unacceptable. If users

Really - yes.
I can make this automatic based on tcl version if performance penalty on
tcl>=8.3 is acceptable.

On Thu, 23 Aug 2001, Peter Eisentraut wrote:

Vsevolod Lobko writes:

Is there a way to make this automatically enabled based on the TCL
version? Does that make sense?

This adds significant overhead to pl/tcl calls :((
so I'm not sure that anybody want this functionality enabled by default

It is my understanding that Tcl 8.3 will not work with 8 bit characters
without this functionality. That would simply be unacceptable. If users

Really - yes.
I can make this automatic based on tcl version if performance penalty on
tcl>=8.3 is acceptable.

It is my understanding that the UTF penalty applies to all the TCL
versions >= 8.1 and that there is debate whether this is a good idea or
not. However, seeing as TCL has the penalty, I don't see a problem
with having the same penalty in our code.

-- 
  Bruce Momjian                        |  http://candle.pha.pa.us
  pgman@candle.pha.pa.us               |  (610) 853-3000
  +  If your life is a hard drive,     |  830 Blythe Avenue
  +  Christ can be your backup.        |  Drexel Hill, Pennsylvania 19026

On Thu, 23 Aug 2001, Tom Lane wrote:

Is this looks better?

It does, but one small gripe: the lack of semicolons will probably cause
pg_indent to mess up the indentation. (I know emacs' autoindent mode
will not work nicely with it, either.) Please set up the macros so that
you write

UTF_BEGIN;
Tcl_DStringAppend(&unknown_src, UTF_E2U(part), -1);
UTF_END;

and then I'll be happy.

Attached revised patch

Your point about overhead is a good one, so I retract the gripe about
using a configure switch. But please include documentation patches to
describe the configure option in the administrator's guide (installation
section).

This patch still uses configure switch for enabling feature.

For enabling based on tcl version we have 2 posibilites:
1) having feature enabled by default, but in pltcl.c check for tcl
version and disable it for old versions
2) enable or disable at configure time based on tcl version, but there
are problem - current configure don't checks for tcl version at all
and my configure skills not enought for adding this

Next version of patch.
Now with documentation update and disabling of UTF conversion for Tcl <=8.0

On Fri, 24 Aug 2001, Vsevolod Lobko wrote:

Your patch has been added to the PostgreSQL unapplied patches list at:

http://candle.pha.pa.us/cgi-bin/pgpatches

I will try to apply it within the next 48 hours.

Next version of patch.
Now with documentation update and disabling of UTF conversion for Tcl <=8.0

On Fri, 24 Aug 2001, Vsevolod Lobko wrote:

On Thu, 23 Aug 2001, Tom Lane wrote:

Is this looks better?

It does, but one small gripe: the lack of semicolons will probably cause
pg_indent to mess up the indentation. (I know emacs' autoindent mode
will not work nicely with it, either.) Please set up the macros so that
you write

UTF_BEGIN;
Tcl_DStringAppend(&unknown_src, UTF_E2U(part), -1);
UTF_END;

and then I'll be happy.

Attached revised patch

Your point about overhead is a good one, so I retract the gripe about
using a configure switch. But please include documentation patches to
describe the configure option in the administrator's guide (installation
section).

This patch still uses configure switch for enabling feature.

For enabling based on tcl version we have 2 posibilites:
1) having feature enabled by default, but in pltcl.c check for tcl
version and disable it for old versions
2) enable or disable at configure time based on tcl version, but there
are problem - current configure don't checks for tcl version at all
and my configure skills not enought for adding this

Content-Description:

[ Attachment, skipping... ]

---------------------------(end of broadcast)---------------------------
TIP 5: Have you checked our extensive FAQ?

http://www.postgresql.org/users-lounge/docs/faq.html

-- 
  Bruce Momjian                        |  http://candle.pha.pa.us
  pgman@candle.pha.pa.us               |  (610) 853-3000
  +  If your life is a hard drive,     |  830 Blythe Avenue
  +  Christ can be your backup.        |  Drexel Hill, Pennsylvania 19026

Vsevolod Lobko writes:

Next version of patch.
Now with documentation update and disabling of UTF conversion for Tcl <=8.0

You still have the configure option in there. Too many configure options
are no good. If Tcl decided to move to Unicode, we should follow. It's
their problem in the end.

--
Peter Eisentraut peter_e@gmx.net http://funkturm.homeip.net/~peter

Next version of patch.
Now with documentation update and disabling of UTF conversion for Tcl <=8.0

You still have the configure option in there. Too many configure options
are no good. If Tcl decided to move to Unicode, we should follow. It's
their problem in the end.

You missed the point Tom has given.

Your point about overhead is a good one, so I retract the gripe about
using a configure switch. But please include documentation patches to
describe the configure option in the administrator's guide (installation
section).

I agree with Tom here.
--
Tatsuo Ishii

Tatsuo Ishii <t-ishii@sra.co.jp> writes:

Peter wrote:

If Tcl decided to move to Unicode, we should follow.

Your point about overhead is a good one, so I retract the gripe about
using a configure switch. But please include documentation patches to
describe the configure option in the administrator's guide (installation
section).

I agree with Tom here.

But Peter's point is a strong one as well. Does anyone else have an
opinion about this?

regards, tom lane

Tatsuo Ishii <t-ishii@sra.co.jp> writes:

Peter wrote:

If Tcl decided to move to Unicode, we should follow.

Your point about overhead is a good one, so I retract the gripe about
using a configure switch. But please include documentation patches to
describe the configure option in the administrator's guide (installation
section).

I agree with Tom here.

But Peter's point is a strong one as well. Does anyone else have an
opinion about this?

But if the config switch was removed, Tcls prior 8.1 would not be
supported at all, no?
--
Tatsuo Ishii

Tatsuo Ishii <t-ishii@sra.co.jp> writes:

But if the config switch was removed, Tcls prior 8.1 would not be
supported at all, no?

No, Peter wasn't proposing removing #if's from the code. His thought
is that the #if's should be conditional only on Tcl version --- if you
have a Tcl version that does UTF, then you get the translation code.

Of course, there are some issues here about compiling against a
different set of Tcl headers than you later run against, but I think
you could get burnt by that anyway.

regards, tom lane