psql display of foreign keys

classic Classic list List threaded Threaded
13 messages Options
Reply | Threaded
Open this post in threaded view
|

psql display of foreign keys

Alvaro Herrera-9
When \d a table referenced by a foreign key on a partitioned table, you
currently get this:

             Table "public.referenced"
 Column |  Type   | Collation | Nullable | Default
--------+---------+-----------+----------+---------
 a      | integer |           | not null |
Indexes:
    "referenced_pkey" PRIMARY KEY, btree (a)
Referenced by:
    TABLE "hashp96_39" CONSTRAINT "hashp_a_fkey" FOREIGN KEY (a) REFERENCES referenced(a)
    TABLE "hashp96_38" CONSTRAINT "hashp_a_fkey" FOREIGN KEY (a) REFERENCES referenced(a)
    TABLE "hashp96_37" CONSTRAINT "hashp_a_fkey" FOREIGN KEY (a) REFERENCES referenced(a)
    TABLE "hashp96_36" CONSTRAINT "hashp_a_fkey" FOREIGN KEY (a) REFERENCES referenced(a)
   (thousands more)

This is not very useful.  I propose that we change it so that it only
displays the one on the partitioned table on which the constraint was
defined:
             Table "public.referenced"
 Column │  Type   │ Collation │ Nullable │ Default
────────┼─────────┼───────────┼──────────┼─────────
 a      │ integer │           │ not null │
Indexes:
    "referenced_pkey" PRIMARY KEY, btree (a)
Referenced by:
    TABLE "hashp" CONSTRAINT "hashp_a_fkey" FOREIGN KEY (a) REFERENCES referenced(a)
    TABLE "hashp" CONSTRAINT "hashp_b_fkey" FOREIGN KEY (b) REFERENCES referenced(a)
    TABLE "parted" CONSTRAINT "parted_a_fkey" FOREIGN KEY (a) REFERENCES referenced(a)

Which results in the actually useful info.

Also, when describing one of the partitions, I propose we add a "TABLE
foo" prefix to the constraint line, so that it indicates on which
ancestor table the constraint was defined.  So instead of this:

\d parted1
              Table "public.parted1"
 Column |  Type   | Collation | Nullable | Default
--------+---------+-----------+----------+---------
 a      | integer |           | not null |
Partition of: parted FOR VALUES FROM (0) TO (1)
Foreign-key constraints:
    "parted_a_fkey" FOREIGN KEY (a) REFERENCES referenced(a)

we get this:

\d parted1
              Table "public.parted1"
 Column │  Type   │ Collation │ Nullable │ Default
────────┼─────────┼───────────┼──────────┼─────────
 a      │ integer │           │ not null │
Partition of: parted FOR VALUES FROM (0) TO (1)
Foreign-key constraints:
    TABLE "parted" CONSTRAINT "parted_a_fkey" FOREIGN KEY (a) REFERENCES referenced(a)

In some cases (such as in the regression tests that change in this
commit) the constraint name is different in the parent than the
partition, and it is more useful to display the parent's constraint name
rather than the partition's.


My first instinct is to change this in psql for Postgres 11, unless
there's much opposition to that.

Patch attached.


PS -- it surprises me that we've got this far without an index on
pg_constraint.confrelid.

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

0001-Fix-psql-display-of-FKs-on-partitioned-tables.patch (10K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: psql display of foreign keys

Tom Lane-2
Alvaro Herrera <[hidden email]> writes:
> This is not very useful.  I propose that we change it so that it only
> displays the one on the partitioned table on which the constraint was
> defined:

OK goal, but ...

> Patch attached.

... this patch breaks the expectation set at the top of describe.c:

 * Support for the various \d ("describe") commands.  Note that the current
 * expectation is that all functions in this file will succeed when working
 * with servers of versions 7.4 and up.  It's okay to omit irrelevant
 * information for an old server, but not to fail outright.

Do you really need WITH RECURSIVE for this?  If so, I'd suggest
applying it only when relkind == RELKIND_PARTITIONED_TABLE, so
that the case doesn't happen in servers too old to have WITH.
That's probably a win performance-wise anyway, as I have no doubt
that the performance of this query is awful compared to what it
replaces, so we don't really want to use it if we don't have to.

                        regards, tom lane

Reply | Threaded
Open this post in threaded view
|

Re: psql display of foreign keys

David Fetter
On Tue, Dec 04, 2018 at 10:00:00AM -0500, Tom Lane wrote:

> Alvaro Herrera <[hidden email]> writes:
> > This is not very useful.  I propose that we change it so that it only
> > displays the one on the partitioned table on which the constraint was
> > defined:
>
> OK goal, but ...
>
> > Patch attached.
>
> ... this patch breaks the expectation set at the top of describe.c:
>
>  * Support for the various \d ("describe") commands.  Note that the current
>  * expectation is that all functions in this file will succeed when working
>  * with servers of versions 7.4 and up.  It's okay to omit irrelevant
>  * information for an old server, but not to fail outright.
>
> Do you really need WITH RECURSIVE for this?  If so, I'd suggest
> applying it only when relkind == RELKIND_PARTITIONED_TABLE, so
> that the case doesn't happen in servers too old to have WITH.

Makes sense.

> That's probably a win performance-wise anyway, as I have no doubt
> that the performance of this query is awful compared to what it
> replaces, so we don't really want to use it if we don't have to.

Do you have cases where we should be measuring performance dips?
Also, is there something about  about indexes involved in this query
or WITH RECURSIVE itself that's pessimizing performance, generally?

Best,
David.
--
David Fetter <david(at)fetter(dot)org> http://fetter.org/
Phone: +1 415 235 3778

Remember to vote!
Consider donating to Postgres: http://www.postgresql.org/about/donate

Reply | Threaded
Open this post in threaded view
|

Re: psql display of foreign keys

Alvaro Herrera-9
On 2018-Dec-04, David Fetter wrote:

> On Tue, Dec 04, 2018 at 10:00:00AM -0500, Tom Lane wrote:

> > That's probably a win performance-wise anyway, as I have no doubt
> > that the performance of this query is awful compared to what it
> > replaces, so we don't really want to use it if we don't have to.

Sure thing.

Fixed the easy one.  On to the other one ...

> Do you have cases where we should be measuring performance dips?
> Also, is there something about  about indexes involved in this query
> or WITH RECURSIVE itself that's pessimizing performance, generally?

Note that there are two queries being changed in this patch, one for
each side of any foreign key.  They start with either a lookup on
conrelid or confrelid; only one of those columns has an index (so
priming the CTE is a little slow for the confrelid one, if your
pg_constraint is bloated).  But after that the CTE iterates on the OID
column, which is indexed, so it should be quick enough.

This is the conrelid plan:
 Sort  (cost=1605.38..1605.39 rows=1 width=101)
   Sort Key: ((constraints.conrelid = '311099'::oid)) DESC, constraints.conname
   CTE constraints
     ->  Recursive Union  (cost=0.29..1600.82 rows=202 width=76)
           ->  Index Scan using pg_constraint_conrelid_contypid_conname_index on pg_constraint  (cost=0.29..11.77 rows=2 width=76)
                 Index Cond: (conrelid = '311099'::oid)
                 Filter: (contype = 'f'::"char")
           ->  Nested Loop  (cost=0.29..158.50 rows=20 width=76)
                 ->  WorkTable Scan on constraints constraints_1  (cost=0.00..0.40 rows=20 width=4)
                 ->  Index Scan using pg_constraint_oid_index on pg_constraint pc  (cost=0.29..7.90 rows=1 width=76)
                       Index Cond: (oid = constraints_1.parent)
   ->  CTE Scan on constraints  (cost=0.00..4.55 rows=1 width=101)
         Filter: (parent = '0'::oid)

This is the confrelid plan:
 Sort  (cost=1793.40..1793.40 rows=1 width=100)
   Sort Key: constraints.conname
   CTE constraints
     ->  Recursive Union  (cost=0.00..1791.11 rows=101 width=80)
           ->  Seq Scan on pg_constraint  (cost=0.00..956.59 rows=1 width=80)
                 Filter: ((contype = 'f'::"char") AND (confrelid = '311099'::oid))
           ->  Nested Loop  (cost=0.29..83.25 rows=10 width=80)
                 ->  WorkTable Scan on constraints constraints_1  (cost=0.00..0.20 rows=10 width=4)
                 ->  Index Scan using pg_constraint_oid_index on pg_constraint pc  (cost=0.29..8.30 rows=1 width=80)
                       Index Cond: (oid = constraints_1.parent)
   ->  CTE Scan on constraints  (cost=0.00..2.27 rows=1 width=100)
         Filter: (parent = '0'::oid)

Of course, the original queries did the same thing (lookup via unindexed
confrelid) and nobody has complained about that yet.  Then again, the
difference between a query taking 0.1 ms (the original query on
conrelid, without recursive CTE) and one that takes 6ms (recursive one
on confrelid) is not noticeable to humans anyway; it's not like this is
a hot path.

In any case, if anyone can think of another method to obtain the topmost
constraint of a hierarchy involving the current table (not involving a
recursive CTE, or maybe with a better one), I'm all ears.

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

Reply | Threaded
Open this post in threaded view
|

Re: psql display of foreign keys

Alvaro Herrera-9
In reply to this post by Tom Lane-2
On 2018-Dec-04, Tom Lane wrote:

> ... this patch breaks the expectation set at the top of describe.c:
>
>  * Support for the various \d ("describe") commands.  Note that the current
>  * expectation is that all functions in this file will succeed when working
>  * with servers of versions 7.4 and up.  It's okay to omit irrelevant
>  * information for an old server, but not to fail outright.

Fixed in the attached.

> Do you really need WITH RECURSIVE for this?

I don't see any other way, but I'm open to ideas.

> If so, I'd suggest applying it only when relkind ==
> RELKIND_PARTITIONED_TABLE, so that the case doesn't happen in servers
> too old to have WITH.  That's probably a win performance-wise anyway,
> as I have no doubt that the performance of this query is awful
> compared to what it replaces, so we don't really want to use it if we
> don't have to.

The query to display foreign keys on the current table can continue to
use the old, fast version when the table is not a partition (I had to
add the relispartition column to another query for this to work).  But I
cannot use the old version for the query that searches for FKs
referencing the current table, because the table for which
partitionedness matters is the other one.  (The WITH version is only
used for servers that can have foreign keys on partitioned tables, viz.
11).

I spent a few minutes trying to think of a way of determining which
query to use at SQL-execution time -- two CTEs, one of which would be
short-circuited ... but I couldn't figure out how.  I also tried to use
the new pg_partition_tree() function, but it's useless for this purpose
because it roots at its argument table, and there doesn't seem to be a
convenient way to obtain the topmost ancestor.

v2 attached.

Many thanks for reviewing.

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

Reply | Threaded
Open this post in threaded view
|

Re: psql display of foreign keys

Alvaro Herrera-9
On 2018-Dec-04, Alvaro Herrera wrote:

> v2 attached.

Oops.

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

v2-0001-Fix-psql-display-of-FKs-on-partitioned-tables.patch (16K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: psql display of foreign keys

Michael Paquier-2
In reply to this post by Alvaro Herrera-9
On Tue, Dec 04, 2018 at 03:41:59PM -0300, Alvaro Herrera wrote:
> I spent a few minutes trying to think of a way of determining which
> query to use at SQL-execution time -- two CTEs, one of which would be
> short-circuited ... but I couldn't figure out how.  I also tried to use
> the new pg_partition_tree() function, but it's useless for this purpose
> because it roots at its argument table, and there doesn't seem to be a
> convenient way to obtain the topmost ancestor.

This has been mentioned on the thread where pg_partition_tree has been
discussed:
https://www.postgresql.org/message-id/6baeb45a-6222-6b88-342d-37fc7d3cf89a%40lab.ntt.co.jp

It got shaved from the final patch for simplicity as we had enough
issues to deal with first.  Adding a pg_partition_root or a new column
in pg_partition_tree makes sense.  My guts are telling me that a
separate function is more instinctive to use.
--
Michael

signature.asc (849 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: psql display of foreign keys

Alvaro Herrera-9
On 2018-Dec-05, Michael Paquier wrote:

> This has been mentioned on the thread where pg_partition_tree has been
> discussed:
> https://www.postgresql.org/message-id/6baeb45a-6222-6b88-342d-37fc7d3cf89a%40lab.ntt.co.jp
>
> It got shaved from the final patch for simplicity as we had enough
> issues to deal with first.  Adding a pg_partition_root or a new column
> in pg_partition_tree makes sense.  My guts are telling me that a
> separate function is more instinctive to use.

I agree with your guts ... you can combine them (the functions, not the
guts) to obtain the full view of the partition hierarchy just by
applying pg_partition_root() to the argument of pg_partition_tree.

I think with pg_partition_root we can rewrite the FK queries to avoid
WITH RECURSIVE with pg12 servers, but of course with a pg11 server we'll
have to keep using WITH RECURSIVE.

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

Reply | Threaded
Open this post in threaded view
|

Re: psql display of foreign keys

Alvaro Herrera-9
In reply to this post by Alvaro Herrera-9
I added this patch to commitfest in order to get more opinions,
particularly on whether to backpatch this.  I might commit sooner than
that if others care to comment.

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

Reply | Threaded
Open this post in threaded view
|

Re: psql display of foreign keys

Alvaro Herrera-9
In reply to this post by Alvaro Herrera-9
On 2018-Dec-04, Alvaro Herrera wrote:

> On 2018-Dec-04, Alvaro Herrera wrote:
>
> > v2 attached.
>
> Oops.

One more oops: The version I posted was for pg11, and does not apply to
master.  This version applies to master.

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

v2-master-0001-Fix-psql-display-of-FKs-on-partitioned-tables.patch (16K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: psql display of foreign keys

Robert Haas
In reply to this post by Alvaro Herrera-9
On Wed, Dec 5, 2018 at 11:30 AM Alvaro Herrera <[hidden email]> wrote:
> I added this patch to commitfest in order to get more opinions,
> particularly on whether to backpatch this.  I might commit sooner than
> that if others care to comment.

I don't think this is a bug fix, and therefore I oppose back-patching it.

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

Reply | Threaded
Open this post in threaded view
|

Re: psql display of foreign keys

Alvaro Herrera-9
On 2018-Dec-06, Robert Haas wrote:

> On Wed, Dec 5, 2018 at 11:30 AM Alvaro Herrera <[hidden email]> wrote:
> > I added this patch to commitfest in order to get more opinions,
> > particularly on whether to backpatch this.  I might commit sooner than
> > that if others care to comment.
>
> I don't think this is a bug fix, and therefore I oppose back-patching it.

OK.

That means I can just get pg_partition_root() done first, and try to
write the queries using that instead of WITH RECURSIVE.

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

Reply | Threaded
Open this post in threaded view
|

Re: psql display of foreign keys

Michael Paquier-2
On Thu, Dec 06, 2018 at 01:26:30PM -0300, Alvaro Herrera wrote:
> That means I can just get pg_partition_root() done first, and try to
> write the queries using that instead of WITH RECURSIVE.

FWIW, I was just writing a patch about this one, so I was going to spawn
a new thread today :)

Let's definitely avoid WITH RECURSIVE if we can.
--
Michael

signature.asc (849 bytes) Download Attachment