Обсуждение: BUG #16743: psql doesn't show whole expression in stored column

The following bug has been logged on the website:

Bug reference:      16743
Logged by:          David Turon
Email address:      turon.david@seznam.cz
PostgreSQL version: 12.5
Operating system:   CentOS 7
Description:

Good morning,

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

CREATE TABLE test(
    a text,
    b text,
    c text,
    d text,    
    ts_vector tsvector GENERATED ALWAYS AS (
            setweight(to_tsvector('simple', COALESCE(a, '')), 'A') || 
            setweight(to_tsvector('simple', COALESCE(b, '')), 'B') ||
            setweight(to_tsvector('simple', COALESCE(c, '')), 'C') ||
            setweight(to_tsvector('simple', COALESCE(d, '')), 'D')) STORED
);

postgres@pgdist:test=# \d test 

               Tabulka "public.test"
  Sloupec  |   Typ    | Collation | Nullable |
                                           Implicitně
                                                   

-----------+----------+-----------+----------+---------------------------------------------------------------------------------------------------------------------------------------------------------------
 a         | text     |           |          | 
 b         | text     |           |          | 
 c         | text     |           |          | 
 d         | text     |           |          | 
 ts_vector | tsvector |           |          | generated always as
(((setweight(to_tsvector('simple'::regconfig, COALESCE(a, ''::text)),
'A'::"char") || setweight(to_tsvector('simple'::regconfig, ) stored

psql (12.5) server/client

pager is off, only way to show whole expresion is use pg_dump i think. Maybe
it was the intention and isn't bug. Thanks. 

Best regards
David T.

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. ---------- 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.)

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

diff --git a/src/bin/psql/describe.c b/src/bin/psql/describe.c
index 07d640021c..14150d05a9 100644
--- a/src/bin/psql/describe.c
+++ b/src/bin/psql/describe.c
@@ -1842,7 +1842,7 @@ describeOneTableDetails(const char *schemaname,
     {
         /* use "pretty" mode for expression to avoid excessive parentheses */
         appendPQExpBufferStr(&buf,
-                             ",\n  (SELECT substring(pg_catalog.pg_get_expr(d.adbin, d.adrelid, true) for 128)"
+                             ",\n  (SELECT pg_catalog.pg_get_expr(d.adbin, d.adrelid, true)"
                              "\n   FROM pg_catalog.pg_attrdef d"
                              "\n   WHERE d.adrelid = a.attrelid AND d.adnum = a.attnum AND a.atthasdef)"
                              ",\n  a.attnotnull");
@@ -2045,7 +2045,8 @@ describeOneTableDetails(const char *schemaname,
         {
             char       *identity;
             char       *generated;
-            char       *default_str = "";
+            char       *default_str;
+            bool        mustfree = false;

             printTableAddCell(&cont, PQgetvalue(res, i, attcoll_col), false, false);

@@ -2061,12 +2062,15 @@ describeOneTableDetails(const char *schemaname,
             else if (identity[0] == ATTRIBUTE_IDENTITY_BY_DEFAULT)
                 default_str = "generated by default as identity";
             else if (generated[0] == ATTRIBUTE_GENERATED_STORED)
-                default_str = psprintf("generated always as (%s) stored", PQgetvalue(res, i, attrdef_col));
+            {
+                default_str = psprintf("generated always as (%s) stored",
+                                       PQgetvalue(res, i, attrdef_col));
+                mustfree = true;
+            }
             else
-                /* (note: above we cut off the 'default' string at 128) */
                 default_str = PQgetvalue(res, i, attrdef_col);

-            printTableAddCell(&cont, default_str, false, generated[0] ? true : false);
+            printTableAddCell(&cont, default_str, false, mustfree);
         }

         /* Info for index columns */

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