BUG #16743: psql doesn't show whole expression in stored column

PG Bug reporting form <noreply@postgresql.org> writes:

when generated column expression length is larger then some value - the rest
of expression is cut in \d[+] output:

Yeah, this is an intentional and very ancient behavior:

appendPQExpBufferStr(&buf,
",\n (SELECT substring(pg_catalog.pg_get_expr(d.adbin, d.adrelid, true) for 128)"
"\n FROM pg_catalog.pg_attrdef d"

Maybe we should decide that completeness is more important than keeping
the line to some arbitrary width. But it's operating as designed.

regards, tom lane

On Tue, Nov 24, 2020 at 10:46:57AM -0500, Tom Lane wrote:

PG Bug reporting form <noreply@postgresql.org> writes:

when generated column expression length is larger then some value - the rest
of expression is cut in \d[+] output:

Yeah, this is an intentional and very ancient behavior:

appendPQExpBufferStr(&buf,
",\n (SELECT substring(pg_catalog.pg_get_expr(d.adbin, d.adrelid, true) for 128)"
"\n FROM pg_catalog.pg_attrdef d"

Maybe we should decide that completeness is more important than keeping
the line to some arbitrary width. But it's operating as designed.

I think I am fine with the current behavior.

If you run psql with -E, you can see the queries it generates:

SELECT a.attname,
pg_catalog.format_type(a.atttypid, a.atttypmod),
--> (SELECT substring(pg_catalog.pg_get_expr(d.adbin, d.adrelid, true) for 128)
--------- -------
FROM pg_catalog.pg_attrdef d
WHERE d.adrelid = a.attrelid AND d.adnum = a.attnum AND a.atthasdef),
a.attnotnull,
(SELECT c.collname FROM pg_catalog.pg_collation c, pg_catalog.pg_type t
WHERE c.oid = a.attcollation AND t.oid = a.atttypid AND a.attcollation <> t.typcollation) AS attcollation,
a.attidentity,
a.attgenerated
FROM pg_catalog.pg_attribute a
WHERE a.attrelid = '16385' AND a.attnum > 0 AND NOT a.attisdropped
ORDER BY a.attnum;

--
Bruce Momjian <bruce@momjian.us> https://momjian.us
EnterpriseDB https://enterprisedb.com

The usefulness of a cup is in its emptiness, Bruce Lee

Bruce Momjian <bruce@momjian.us> writes:

On Tue, Nov 24, 2020 at 10:46:57AM -0500, Tom Lane wrote:

Maybe we should decide that completeness is more important than keeping
the line to some arbitrary width. But it's operating as designed.

I think I am fine with the current behavior.

It's less great in the context of a GENERATED column. As things stand,
the psql-side code wraps the truncated result into some more syntax:

else if (generated[0] == ATTRIBUTE_GENERATED_STORED)
default_str = psprintf("generated always as (%s) stored", PQgetvalue(res, i, attrdef_col));
else
/* (note: above we cut off the 'default' string at 128) */
default_str = PQgetvalue(res, i, attrdef_col);

which seems distinctly more confusing than just truncating the result.
So maybe there's justification here for revisiting that ancient decision.

One could also imagine not appending ") stored" if the attrdef value
appears to have been truncated --- although encoding-conversion effects
would make it hard to tell that for sure. Perhaps we'd be better off
to move the truncation logic to the psql side?

Another thing here, which is undeniably a bug, is that that psprintf
result is promptly leaked.

regards, tom lane

Yes ist little bit confusing with truncate only part and leave stored. Maybe
substring to first 125 and append at least '...' or some switch in psql '\
pset cut_row_length on' or something similar.

postgres@pgdist:test=# SELECT a.attname,

  pg_catalog.format_type(a.atttypid, a.atttypmod),

  (SELECT substring(pg_catalog.pg_get_expr(d.adbin, d.adrelid, true) for
125)||'...'

   FROM pg_catalog.pg_attrdef d

   WHERE d.adrelid = a.attrelid AND d.adnum = a.attnum AND a.atthasdef),

  a.attnotnull,

  (SELECT c.collname FROM pg_catalog.pg_collation c, pg_catalog.pg_type t

   WHERE c.oid = a.attcollation AND t.oid = a.atttypid AND a.attcollation <>
t.typcollation) AS attcollation,

  a.attidentity,

  a.attgenerated

FROM pg_catalog.pg_attribute a

WHERE a.attrelid = '896567' AND a.attnum > 0 AND NOT a.attisdropped

ORDER BY a.attnum;

-[ RECORD 5 ]+--------------------------------------------------------------
-------------------------------------------------------------------

attname      | ts_vector

format_type  | tsvector

?column?     | ((setweight(to_tsvector('simple'::regconfig, COALESCE(a, ''::
text)), 'A'::"char") || setweight(to_tsvector('simple'::regconfi...

attnotnull   | f

attcollation | <NULL>

attidentity  | 

attgenerated | s

I am using pager, so for me fixed row/column lenght isn't priority.  

regards, David T.

---------- Původní e-mail ----------
Od: Tom Lane <tgl@sss.pgh.pa.us>
Komu: Bruce Momjian <bruce@momjian.us>
Datum: 24. 11. 2020 17:26:07
Předmět: Re: BUG #16743: psql doesn't show whole expression in stored column

"Bruce Momjian <bruce@momjian.us> writes:

On Tue, Nov 24, 2020 at 10:46:57AM -0500, Tom Lane wrote:

Maybe we should decide that completeness is more important than keeping
the line to some arbitrary width. But it's operating as designed.

I think I am fine with the current behavior.

It's less great in the context of a GENERATED column. As things stand,
the psql-side code wraps the truncated result into some more syntax:

else if (generated[0] == ATTRIBUTE_GENERATED_STORED)
default_str = psprintf("generated always as (%s) stored", PQgetvalue(res, i,
attrdef_col));
else
/* (note: above we cut off the 'default' string at 128) */
default_str = PQgetvalue(res, i, attrdef_col);

which seems distinctly more confusing than just truncating the result.
So maybe there's justification here for revisiting that ancient decision.

One could also imagine not appending ") stored" if the attrdef value
appears to have been truncated --- although encoding-conversion effects
would make it hard to tell that for sure. Perhaps we'd be better off
to move the truncation logic to the psql side?

Another thing here, which is undeniably a bug, is that that psprintf
result is promptly leaked.

regards, tom lane
"

On 2020-11-24 16:46, Tom Lane wrote:

PG Bug reporting form <noreply@postgresql.org> writes:

when generated column expression length is larger then some value - the rest
of expression is cut in \d[+] output:

Yeah, this is an intentional and very ancient behavior:

appendPQExpBufferStr(&buf,
",\n (SELECT substring(pg_catalog.pg_get_expr(d.adbin, d.adrelid, true) for 128)"
"\n FROM pg_catalog.pg_attrdef d"

Maybe we should decide that completeness is more important than keeping
the line to some arbitrary width. But it's operating as designed.

I think we should get rid of the truncating. Otherwise, there is no way
to actually get the full information, is there? (Other than pg_dump or
manual catalog queries.)

On Wed, Nov 25, 2020 at 10:19 AM Peter Eisentraut <
peter.eisentraut@enterprisedb.com> wrote:

On 2020-11-24 16:46, Tom Lane wrote:

PG Bug reporting form <noreply@postgresql.org> writes:

when generated column expression length is larger then some value - the

rest

of expression is cut in \d[+] output:

Yeah, this is an intentional and very ancient behavior:

appendPQExpBufferStr(&buf,
",\n (SELECT

substring(pg_catalog.pg_get_expr(d.adbin, d.adrelid, true) for 128)"

"\n FROM pg_catalog.pg_attrdef d"

Maybe we should decide that completeness is more important than keeping
the line to some arbitrary width. But it's operating as designed.

I think we should get rid of the truncating. Otherwise, there is no way
to actually get the full information, is there? (Other than pg_dump or
manual catalog queries.)

Either that or we have to make it clear that the output is truncated:
otherwise it can be mighty confusing.

--
Alex

Peter Eisentraut <peter.eisentraut@enterprisedb.com> writes:

On 2020-11-24 16:46, Tom Lane wrote:

Maybe we should decide that completeness is more important than keeping
the line to some arbitrary width. But it's operating as designed.

I think we should get rid of the truncating. Otherwise, there is no way
to actually get the full information, is there? (Other than pg_dump or
manual catalog queries.)

That'd be okay with me. It's always seemed a little odd that we do that
for attrdefs but not anything else.

A bit of checking with git blame says that the habit came in with
the very first version of describe.c, in

commit a45195a191eec367a4f305bb71ab541d17a3b9f9
Author: Bruce Momjian <bruce@momjian.us>
Date: Thu Nov 4 21:56:02 1999 +0000

Major psql overhaul by Peter Eisentraut.

which has

+    /* Info */
+    cells[i*cols + 2] = xmalloc(128 + 128); /* I'm cutting off the default string at 128 */
+    cells[i*cols + 2][0] = '\0';
+    if (strcmp(PQgetvalue(res, i, 4), "t") == 0)
+        strcat(cells[i*cols + 2], "not null");
+    if (strcmp(PQgetvalue(res, i, 5), "t") == 0) {
+        /* handle "default" here */
+        strcpy(descbuf, "SELECT substring(d.adsrc for 128) FROM pg_attrdef d, pg_class c\n"
+                    "WHERE c.relname = '");

So that looks very much like the truncation was an expedient thing
to do to work with a fixed-size result buffer, rather than something
that was chosen to improve the user experience.

regards, tom lane

I wrote:

Peter Eisentraut <peter.eisentraut@enterprisedb.com> writes:

I think we should get rid of the truncating. Otherwise, there is no way
to actually get the full information, is there? (Other than pg_dump or
manual catalog queries.)

That'd be okay with me. It's always seemed a little odd that we do that
for attrdefs but not anything else.

Here's a proposed patch for that.

After looking a bit closer, I saw that the memory leak I was worried
about before is not real, because the code passes mustfree = true to
printTableAddCell. However, I still don't like it one bit, because you
need some undocumented and fragile assumptions about the relationship
between attidentity and attgenerated to conclude that we won't instead
have a false free() attempt on something that mustn't be free'd. So I
think we should adjust the code to track mustfree explicitly, as done
below.

Should we back-patch this? I think that the truncation behavior became
significantly more of a problem with the addition of the GENERATED
feature; before that it was clearer what was going on. So I'm mildly
inclined to back-patch to v12 where that came in.

regards, tom lane

On Wed, Nov 25, 2020 at 11:15:39AM -0500, Tom Lane wrote:

I wrote:

Peter Eisentraut <peter.eisentraut@enterprisedb.com> writes:

I think we should get rid of the truncating. Otherwise, there is no way
to actually get the full information, is there? (Other than pg_dump or
manual catalog queries.)

That'd be okay with me. It's always seemed a little odd that we do that
for attrdefs but not anything else.

Here's a proposed patch for that.

After looking a bit closer, I saw that the memory leak I was worried
about before is not real, because the code passes mustfree = true to
printTableAddCell. However, I still don't like it one bit, because you
need some undocumented and fragile assumptions about the relationship
between attidentity and attgenerated to conclude that we won't instead
have a false free() attempt on something that mustn't be free'd. So I
think we should adjust the code to track mustfree explicitly, as done
below.

Should we back-patch this? I think that the truncation behavior became
significantly more of a problem with the addition of the GENERATED
feature; before that it was clearer what was going on. So I'm mildly
inclined to back-patch to v12 where that came in.

Works for me.

--
Bruce Momjian <bruce@momjian.us> https://momjian.us
EnterpriseDB https://enterprisedb.com

The usefulness of a cup is in its emptiness, Bruce Lee

Bruce Momjian <bruce@momjian.us> writes:

On Wed, Nov 25, 2020 at 11:15:39AM -0500, Tom Lane wrote:

Should we back-patch this? I think that the truncation behavior became
significantly more of a problem with the addition of the GENERATED
feature; before that it was clearer what was going on. So I'm mildly
inclined to back-patch to v12 where that came in.

Works for me.

Done that way.

regards, tom lane