Обсуждение: make \d pg_toast.foo show its indices

It's deliberate that \dt doesn't show toast tables.
\d shows them, but doesn't show their indices.

It seems to me that their indices should be shown, without having to think and
know to query pg_index.

postgres=# \d pg_toast.pg_toast_2600
TOAST table "pg_toast.pg_toast_2600"
   Column   |  Type   
------------+---------
 chunk_id   | oid
 chunk_seq  | integer
 chunk_data | bytea
Indexes:
    "pg_toast_2600_index" PRIMARY KEY, btree (chunk_id, chunk_seq)

Justin

On Mon, 22 Apr 2019 at 17:49, Justin Pryzby <pryzby@telsasoft.com> wrote:
>
> It's deliberate that \dt doesn't show toast tables.
> \d shows them, but doesn't show their indices.
>
> It seems to me that their indices should be shown, without having to think and
> know to query pg_index.
>
> postgres=# \d pg_toast.pg_toast_2600
> TOAST table "pg_toast.pg_toast_2600"
>    Column   |  Type
> ------------+---------
>  chunk_id   | oid
>  chunk_seq  | integer
>  chunk_data | bytea
> Indexes:
>     "pg_toast_2600_index" PRIMARY KEY, btree (chunk_id, chunk_seq)
>
+1.


-- 
Regards,
Rafia Sabih

On Fri, May 03, 2019 at 02:55:47PM +0200, Rafia Sabih wrote:
> On Mon, 22 Apr 2019 at 17:49, Justin Pryzby <pryzby@telsasoft.com> wrote:
> >
> > It's deliberate that \dt doesn't show toast tables.
> > \d shows them, but doesn't show their indices.
> >
> > It seems to me that their indices should be shown, without having to think and
> > know to query pg_index.
> >
> > postgres=# \d pg_toast.pg_toast_2600
> > TOAST table "pg_toast.pg_toast_2600"
> >    Column   |  Type
> > ------------+---------
> >  chunk_id   | oid
> >  chunk_seq  | integer
> >  chunk_data | bytea
> > Indexes:
> >     "pg_toast_2600_index" PRIMARY KEY, btree (chunk_id, chunk_seq)
>
> +1.

Thanks - what about also showing the associated non-toast table ?

postgres=# \d pg_toast.pg_toast_2620
TOAST table "pg_toast.pg_toast_2620"
   Column   |  Type
------------+---------
 chunk_id   | oid
 chunk_seq  | integer
 chunk_data | bytea
FOR TABLE: "pg_catalog.pg_trigger"
Indexes:
    "pg_toast_2620_index" PRIMARY KEY, btree (chunk_id, chunk_seq)

That could be displayed differently, perhaps in the header, but I think this is
more consistent with other display.

Justin

On Fri, 3 May 2019 at 16:27, Justin Pryzby <pryzby@telsasoft.com> wrote:
>
> On Fri, May 03, 2019 at 02:55:47PM +0200, Rafia Sabih wrote:
> > On Mon, 22 Apr 2019 at 17:49, Justin Pryzby <pryzby@telsasoft.com> wrote:
> > >
> > > It's deliberate that \dt doesn't show toast tables.
> > > \d shows them, but doesn't show their indices.
> > >
> > > It seems to me that their indices should be shown, without having to think and
> > > know to query pg_index.
> > >
> > > postgres=# \d pg_toast.pg_toast_2600
> > > TOAST table "pg_toast.pg_toast_2600"
> > >    Column   |  Type
> > > ------------+---------
> > >  chunk_id   | oid
> > >  chunk_seq  | integer
> > >  chunk_data | bytea
> > > Indexes:
> > >     "pg_toast_2600_index" PRIMARY KEY, btree (chunk_id, chunk_seq)
> >
> > +1.
>
> Thanks - what about also showing the associated non-toast table ?
>
IMHO, what makes more sense is to show the name of associated toast
table in the \dt+ of the normal table.


-- 
Regards,
Rafia Sabih

Rafia Sabih <rafia.pghackers@gmail.com> writes:
> On Fri, 3 May 2019 at 16:27, Justin Pryzby <pryzby@telsasoft.com> wrote:
>> Thanks - what about also showing the associated non-toast table ?

> IMHO, what makes more sense is to show the name of associated toast
> table in the \dt+ of the normal table.

I'm not for that: it's useless information in at least 99.44% of cases.

Possibly it is useful in the other direction as Justin suggests.
Not sure though --- generally, if you're looking at a specific
toast table, you already know which table is its parent.  But
maybe confirmation is a good thing.

That seems off-topic for this thread though.  I agree with the
stated premise that \d on a toast table should show all the same
information \d on a regular table would.

            regards, tom lane

On Mon, May 06, 2019 at 09:13:52AM +0200, Rafia Sabih wrote:
> On Fri, 3 May 2019 at 16:27, Justin Pryzby <pryzby@telsasoft.com> wrote:
> >
> > On Fri, May 03, 2019 at 02:55:47PM +0200, Rafia Sabih wrote:
> > > On Mon, 22 Apr 2019 at 17:49, Justin Pryzby <pryzby@telsasoft.com> wrote:
> > > >
> > > > It's deliberate that \dt doesn't show toast tables.
> > > > \d shows them, but doesn't show their indices.
> > > >
> > > > It seems to me that their indices should be shown, without having to think and
> > > > know to query pg_index.
> > > >
> > > > postgres=# \d pg_toast.pg_toast_2600
> > > > TOAST table "pg_toast.pg_toast_2600"
> > > >    Column   |  Type
> > > > ------------+---------
> > > >  chunk_id   | oid
> > > >  chunk_seq  | integer
> > > >  chunk_data | bytea
> > > > Indexes:
> > > >     "pg_toast_2600_index" PRIMARY KEY, btree (chunk_id, chunk_seq)
> > >
> > > +1.
> >
> > Thanks - what about also showing the associated non-toast table ?
> >
> IMHO, what makes more sense is to show the name of associated toast
> table in the \dt+ of the normal table.

Perhaps ... but TOAST is an implementation detail, and I think it should rarely
be important to know the toast table for a given table.

I think it's more useful to go the other way (at least), to answer questions
when pg_toast.* table shows up in a query like these:

 - SELECT relpages, relname FROM pg_class ORDER BY 1 DESC;
 - SELECT COUNT(1), relname FROM pg_class c JOIN pg_buffercache b ON b.relfilenode=c.oid GROUP BY 2 ORDER BY 1 DESC
LIMIT9;
 

Justin

Hi,

On 2019-05-06 11:58:18 -0400, Tom Lane wrote:
> Not sure though --- generally, if you're looking at a specific
> toast table, you already know which table is its parent.  But
> maybe confirmation is a good thing.

I'm not convinced by that. I've certainly many a time wrote queries
against pg_class to figure out which relation a toast table belongs
to. E.g. after looking at the largest relations in the system, looking
at pg_stat_*_tables, after seeing an error in the logs, etc.


> That seems off-topic for this thread though.  I agree with the
> stated premise that \d on a toast table should show all the same
> information \d on a regular table would.

+1

Greetings,

Andres Freund

On 2019-May-06, Justin Pryzby wrote:

> Perhaps ... but TOAST is an implementation detail, and I think it should rarely
> be important to know the toast table for a given table.

I'm with Andres -- while it's admittedly a rare need, it is a real one.

Sometimes I wish for \d++ which would display internal details too obscure
to show in the regular \d+, such as the toast table name.

-- 
Álvaro Herrera                https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

On Mon, May 6, 2019 at 12:26 PM Andres Freund <andres@anarazel.de> wrote:
> I'm not convinced by that. I've certainly many a time wrote queries
> against pg_class to figure out which relation a toast table belongs
> to. E.g. after looking at the largest relations in the system, looking
> at pg_stat_*_tables, after seeing an error in the logs, etc.

+1.  I think it would be great for \d on the TOAST table to show this
information.

> > That seems off-topic for this thread though.  I agree with the
> > stated premise that \d on a toast table should show all the same
> > information \d on a regular table would.
>
> +1

That premise seems like a good one, too.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Greetings,

* Tom Lane (tgl@sss.pgh.pa.us) wrote:
> Rafia Sabih <rafia.pghackers@gmail.com> writes:
> > On Fri, 3 May 2019 at 16:27, Justin Pryzby <pryzby@telsasoft.com> wrote:
> >> Thanks - what about also showing the associated non-toast table ?
>
> > IMHO, what makes more sense is to show the name of associated toast
> > table in the \dt+ of the normal table.
>
> I'm not for that: it's useless information in at least 99.44% of cases.

I don't think I'd put it in \dt+, but the toast table is still
pg_toast.pg_toast_{relOid}, right?  What about showing the OID of the
table in the \d output, eg:

=> \d comments
           Table "public.comments" (50788)
 Column  |  Type   | Collation | Nullable |   Default

etc?

> Possibly it is useful in the other direction as Justin suggests.
> Not sure though --- generally, if you're looking at a specific
> toast table, you already know which table is its parent.  But
> maybe confirmation is a good thing.

As mentioned elsewhere, there are certainly times when you don't know
that info and if you're looking at the definition of a TOAST table,
which isn't terribly complex, it seems like a good idea to go ahead and
include the table it's the TOAST table for.

> That seems off-topic for this thread though.  I agree with the
> stated premise that \d on a toast table should show all the same
> information \d on a regular table would.

+1

Thanks!

Stephen

Stephen Frost <sfrost@snowman.net> writes:
> * Tom Lane (tgl@sss.pgh.pa.us) wrote:
>> Rafia Sabih <rafia.pghackers@gmail.com> writes:
>>> IMHO, what makes more sense is to show the name of associated toast
>>> table in the \dt+ of the normal table.

>> I'm not for that: it's useless information in at least 99.44% of cases.

> I don't think I'd put it in \dt+, but the toast table is still
> pg_toast.pg_toast_{relOid}, right?  What about showing the OID of the
> table in the \d output, eg:
> => \d comments
>            Table "public.comments" (50788)

Not unless you want to break every regression test that uses \d.
Instability of the output is also a reason not to show the
toast table's name in the parent's \d[+].

>> Possibly it is useful in the other direction as Justin suggests.
>> Not sure though --- generally, if you're looking at a specific
>> toast table, you already know which table is its parent.  But
>> maybe confirmation is a good thing.

> As mentioned elsewhere, there are certainly times when you don't know
> that info and if you're looking at the definition of a TOAST table,
> which isn't terribly complex, it seems like a good idea to go ahead and
> include the table it's the TOAST table for.

I'm not against putting that info into the result of \d on the toast
table.

            regards, tom lane

Greetings,

* Tom Lane (tgl@sss.pgh.pa.us) wrote:
> Stephen Frost <sfrost@snowman.net> writes:
> > * Tom Lane (tgl@sss.pgh.pa.us) wrote:
> >> Rafia Sabih <rafia.pghackers@gmail.com> writes:
> >>> IMHO, what makes more sense is to show the name of associated toast
> >>> table in the \dt+ of the normal table.
>
> >> I'm not for that: it's useless information in at least 99.44% of cases.
>
> > I don't think I'd put it in \dt+, but the toast table is still
> > pg_toast.pg_toast_{relOid}, right?  What about showing the OID of the
> > table in the \d output, eg:
> > => \d comments
> >            Table "public.comments" (50788)
>
> Not unless you want to break every regression test that uses \d.
> Instability of the output is also a reason not to show the
> toast table's name in the parent's \d[+].

So we need a way to turn it off.  That doesn't seem like it'd be hard to
implement and the information is certainly quite useful.

Thanks,

Stephen

On Tue, May 7, 2019 at 11:30 AM Stephen Frost <sfrost@snowman.net> wrote:
> > Not unless you want to break every regression test that uses \d.
> > Instability of the output is also a reason not to show the
> > toast table's name in the parent's \d[+].
>
> So we need a way to turn it off.  That doesn't seem like it'd be hard to
> implement and the information is certainly quite useful.

Ugh.  It's not really worth it if we have to go to such lengths.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Greetings,

* Robert Haas (robertmhaas@gmail.com) wrote:
> On Tue, May 7, 2019 at 11:30 AM Stephen Frost <sfrost@snowman.net> wrote:
> > > Not unless you want to break every regression test that uses \d.
> > > Instability of the output is also a reason not to show the
> > > toast table's name in the parent's \d[+].
> >
> > So we need a way to turn it off.  That doesn't seem like it'd be hard to
> > implement and the information is certainly quite useful.
>
> Ugh.  It's not really worth it if we have to go to such lengths.

I don't think I agree..  We've gone to pretty great lengths to have
things that can be turned on and off for explain because they're useful
to have but not something that's predictible in the regression tests.
This doesn't strike me as all that different (indeed, if anything it
seems like it should be less of an issue since it's entirely client
side...).

Having our test framework deny us useful features just strikes me as
bizarre.

Thanks,

Stephen

Stephen Frost <sfrost@snowman.net> writes:
> Having our test framework deny us useful features just strikes me as
> bizarre.

This is presuming that it's useful, which is debatable IMO.
I think most people will find it useless noise almost all of the time.

            regards, tom lane

Greetings,

* Tom Lane (tgl@sss.pgh.pa.us) wrote:
> Stephen Frost <sfrost@snowman.net> writes:
> > Having our test framework deny us useful features just strikes me as
> > bizarre.
>
> This is presuming that it's useful, which is debatable IMO.
> I think most people will find it useless noise almost all of the time.

Alright, maybe I'm not the best representation of our user base, but I
sure type 'select oid,* from pg_class where relname = ...' with some
regularity, mostly to get the oid to then go do something else.  Having
the relfilenode would be nice too, now that I think about it, and
reltuples.  There's ways to get *nearly* everything that's in pg_class
and friends out of various \d incantations, but not quite everything,
which seems unfortunate.

In any case, I can understand an argument that the code it requires is
too much to maintain for a relatively minor feature (though it hardly
seems like it would be...) or that it would be confusing or unhelpful to
users (aka "noise") much of the time, so I'll leave it to others to
comment on if they think any of these ideas be a useful addition or not.

I just don't think we should be voting down a feature because it'd take
a bit of extra effort to make our regression tests work with it, which
is all I was intending to get at here.

Thanks!

Stephen

On Tue, May 7, 2019 at 6:03 PM Stephen Frost <sfrost@snowman.net> wrote:
> Alright, maybe I'm not the best representation of our user base, but I
> sure type 'select oid,* from pg_class where relname = ...' with some
> regularity, mostly to get the oid to then go do something else.  Having
> the relfilenode would be nice too, now that I think about it, and
> reltuples.  There's ways to get *nearly* everything that's in pg_class
> and friends out of various \d incantations, but not quite everything,
> which seems unfortunate.
>
> In any case, I can understand an argument that the code it requires is
> too much to maintain for a relatively minor feature (though it hardly
> seems like it would be...) or that it would be confusing or unhelpful to
> users (aka "noise") much of the time, so I'll leave it to others to
> comment on if they think any of these ideas be a useful addition or not.
>
> I just don't think we should be voting down a feature because it'd take
> a bit of extra effort to make our regression tests work with it, which
> is all I was intending to get at here.

I think it's unjustifiable to show this in \d output.  But maybe in
\d+ output it could be justified, or perhaps in the \d++ which I seem
to recall Alvaro proposing someplace recently.

I think if we're going to show it, it should be on its own line, with
a clear label, not just in the table header as you proposed.
Otherwise, people won't know what it is.

I suppose the work we'd need to make it work with the regression tests
is no worse than the hide_tableam crock which Andres recently added.
That is certainly a crock, but I can testify that it's a very useful
crock for zheap development.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Robert Haas <robertmhaas@gmail.com> writes:
> I think it's unjustifiable to show this in \d output.  But maybe in
> \d+ output it could be justified, or perhaps in the \d++ which I seem
> to recall Alvaro proposing someplace recently.

Yeah, if we're going to do that (show a table's toast table) I would
want to bury it in \d++ or some other not-currently-used notation.

            regards, tom lane

I'm continuing this thread with an additional change to slash dee for
partitioned indexes.

postgres=# \d ttz_i_idx
 Partitioned index "public.ttz_i_idx"
 Column |  Type   | Key? | Definition 
--------+---------+------+------------
 i      | integer | yes  | i
btree, for table "public.ttz"
Number of partitions: 2 (Use \d+ to list them.)

postgres=# \d+ ttz_i_idx
             Partitioned index "public.ttz_i_idx"
 Column |  Type   | Key? | Definition | Storage | Stats target 
--------+---------+------+------------+---------+--------------
 i      | integer | yes  | i          | plain   | 
btree, for table "public.ttz"
Partitions: ttz1_i_idx,
            ttz2_i_idx, PARTITIONED

Showing the list of index partitions is probably not frequently useful, but
consider the case of non-default names, for example due to truncation.

I didn't update regression output; note that this patch also, by chance, causes
tablespace of partitioned indexes to be output, which I think is good and an
oversight that it isn't currently shown.

I added CF entry and including previous two patches for CFBOT purposes.

Recap: Tom, Andreas, Robert, Stephen and I agree that \d toast should show the
main table.  Rafia and Alvaro think that \d on the main table should (also?)
show its toast.

Justin

My previous patch missed a 1-line hunk, so resending.

There are 3 independent patches associated to one thread and one CF entry.


*** About toast table v3:

Patch applies cleanly, compiles, works for me.

ISTM that the he query should be unambiguous: pg_catalog.pg_class instead 
of pg_class, add an alias (eg c), use c.FIELD to access an attribute. In 
its current form "pg_class" could resolve to another table depending on 
the search path.

C style is broken. On "if () {", brace must be on next line. On "1 != 
PQntuples(result)", I would exchange operands.

PQclear must be called on the main path.

If the table name contains a ", the result looks awkward:

     For table: "public.foo"bla"

I'm wondering whether some escaping should be done. Well, it is not done 
for other simular entries, so probably this is bad but okay:-)

There are no tests:-(


*** About toast index v3

Patch applies cleanly, compiles, works for me.

There are no tests:-(

*** About the next one, v4

Patch applies cleanly, compiles. Not sure how to test it.

"switch (*PQgetvalue(result, i, 2))": I understand that relkind is a must 
admit I do not like this style much, an intermediate variable would 
improve readability. Also, a simple if instead of a swich might be more 
appropriate, and be closer to the previous implementation.

There are no tests:-(

-- 
Fabien.

Sorry, I missed this until now.

On Sun, Jun 30, 2019 at 10:26:28AM +0200, Fabien COELHO wrote:
> *** About toast table v3:
> 
> Patch applies cleanly, compiles, works for me.
> 
> ISTM that the he query should be unambiguous: pg_catalog.pg_class instead of
> pg_class, add an alias (eg c), use c.FIELD to access an attribute. In its
> current form "pg_class" could resolve to another table depending on the
> search path.

Thanks for noticing, fixed.

> C style is broken. On "if () {", brace must be on next line. On "1 !=
> PQntuples(result)", I would exchange operands.
> 
> PQclear must be called on the main path.

Done

> There are no tests:-(

"show-childs" caused plenty of tests fail; actually..it looks like my previous
patch duplicated "tablespace" line for indices (and I managed to not notice the
original one, and claimed my patch fixed that omission, sigh).  I added test
that it shows its partitions, too.

It seems like an obviously good idea to add tests for \d toast; it's not clear
to me how to do run \d for a toast table, which is named after a user table's
OID...  (I tested that \gexec doesn't work for this).

So for now I used \d pg_toast.pg_toast_2619

> If the table name contains a ", the result looks awkward:
> 
>     For table: "public.foo"bla"
> 
> I'm wondering whether some escaping should be done. Well, it is not done for
> other simular entries, so probably this is bad but okay:-)

Leaving this for another commit-day.

Thanks for testing.

Justin

I realized that the test added to show-childs patch was listing partitioned
tables not indices..fixed.

Find attached updated patches which also work against old servers.

1) avoid ::regnamespace; 2) don't PQgetvalue() fields which don't exist and then crash.

> Find attached updated patches which also work against old servers.

I can't check that for sure.

* About toast table addition v7:

Patch applies cleanly, compiles, make check ok, no doc.

This addition show the main table of a toast table, which is useful.

Field relnamespace oid in pg_class appears with pg 7.3, maybe it would be 
appropriate to guard agains older versions, with "pset.sversion >= 70300". 
It seems that there are other unguarded instances in "describe.c", so 
maybe this is considered too old.

Test is ok.

* About toast index v7:

Patch applies cleanly on top of previous, compiles, make check ok, no doc.

This patch simply enables an existing query on toast tables so as to show 
corresponding indices.

Test is ok.

* About toast part v7.

Patch applies cleanly, compiles, make check ok, no doc.

It gives the partition info about an index as it is shown about a table, 
which is useful.

There are some changes in the query on older systems, which seem harmless.
The code is rather simplified because a special case is removed, which is 
a good thing.

Test is ok.

Marked as ready.

-- 
Fabien.

Fabien COELHO <coelho@cri.ensmp.fr> writes:
> Field relnamespace oid in pg_class appears with pg 7.3, maybe it would be 
> appropriate to guard agains older versions, with "pset.sversion >= 70300". 
> It seems that there are other unguarded instances in "describe.c", so 
> maybe this is considered too old.

Per the comment at the head of describe.c, we only expect it to work
back to 7.4.  I tested against a 7.4 server, the modified queries
seem fine.

> Marked as ready.

Pushed with minor fiddling with the toast-table code, and rather
more significant hacking on the partitioned-index code.  Notably,
0003 had broken output of Tablespace: footers for everything except
indexes.  It's possibly not Justin's fault that that wasn't noticed,
because we had no regression tests covering it :-(.  We do now.

            regards, tom lane

> Pushed with minor fiddling with the toast-table code, and rather
> more significant hacking on the partitioned-index code.  Notably,
> 0003 had broken output of Tablespace: footers for everything except
> indexes.

Argh, sorry for the review miss.

> It's possibly not Justin's fault that that wasn't noticed,
> because we had no regression tests covering it :-(.  We do now.

Thanks.

-- 
Fabien.