TCL_ARRAYS code in libpgtcl is pretty seriously broken

From: Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>
To: pgsql-hackers(at)postgreSQL(dot)org, pgsql-interfaces(at)postgreSQL(dot)org
Subject: TCL_ARRAYS code in libpgtcl is pretty seriously broken
Date: 1998-10-04 22:24:16
Message-ID: 19449.907539856@sss.pgh.pa.us
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-hackers pgsql-interfaces

There is some code in libpgtcl that purports to convert Postgres array
data values into Tcl lists. Is anyone prepared to argue that that code
does something useful in its present state? I can name half a dozen
bugs in it without breathing hard:

1. Blithely assumes that any data value beginning with '{' and ending
with '}' must represent an array value. Should have some more robust
way of discovering whether a column is array type. (In fairness, this
might require a FE/BE protocol change, unless arrayness can be
determined from the tuple descriptors provided by the backend, ie,
field type OID, size, and attmod. Anybody know a way to do that?)

2. Applies a translation that converts all backslash escape sequences
defined for C string constants into their equivalent single characters.
Since neither the backend nor Tcl generate anything close to C-string
escapes, the point of this is difficult to determine. It does however
result in unexpected output, eg disappearing backslashes.

3. Applies said translation even when processing a non-array data value.

4. Doesn't actually manage to produce a valid Tcl list, if the data
contains anything Tcl considers a special character. What it *should*
be doing is quoting, not de-quoting.

5. Fails to modify \\, \{, and \} (thus quite unintentionally doing
almost the right thing...) when these sequences appear inside an array
value, because "they will be unescaped by Tcl in Tcl_AppendElement".
But in fact Tcl_AppendElement is not invoked on the results of this
code.

6. Modifies the string returned by libpq *in place*. This would be a
const-ness violation if we had been more careful about declaring things
const. More importantly, it means that re-examining the same tuple of
the PGresult will yield a different result. Not cool.

7. The TCL_ARRAYS code is only invoked in the "-assign" variant of
the pgtcl pg_result statement, not in any of the other paths that allow
tuple values to be examined. This is presumably an oversight, not
the intended behavior.

8. Does not cope with MULTIBYTE strings. (But I don't think Tcl does
either, so it's not clear that this can be called a bug.)

I am strongly inclined to rip this code out, because it is responsible
for several behaviors that were correctly called bugs when backslash-
handling was discussed on pgsql-interfaces back in August. If we don't
rip it out, it needs a complete rewrite.

Unless there is a bulletproof solution to problem #1 (how to tell
whether a field's data type is array), I do not think it is appropriate
for the basic pg_result code to be applying any such transform. Perhaps
it would be reasonable to invent a separate string-formatting function,
say "pg_arraytolist", that would perform the conversion. It would then
be the application writer's responsibility to know which fields were
arrays and apply the conversion if he wanted it.

Comments?

regards, tom lane

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Thomas G. Lockhart 1998-10-04 22:58:41 Re: pg_dump new -n flag
Previous Message Stupor Genius 1998-10-04 20:12:37 RE: [HACKERS] plpgsql is not ready for prime time

Browse pgsql-interfaces by date

  From Date Subject
Next Message Thomas G. Lockhart 1998-10-04 23:06:40 Re: [INTERFACES] Re: Just some unfinished stuff.
Previous Message Gerald Gryschuk 1998-10-04 20:59:56 Re: [INTERFACES] Re: Just some unfinished stuff.