Thread

Commits

Improve psql's \d display of foreign key constraints
- 1af25ca0c2d9 12.0 landed
pg_partition_ancestors
- b96f6b19487f 12.0 landed

psql display of foreign keys

Alvaro Herrera <alvherre@2ndquadrant.com> — 2018-12-04T14:38:34Z

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

Re: psql display of foreign keys

Tom Lane <tgl@sss.pgh.pa.us> — 2018-12-04T15:00:00Z

Alvaro Herrera <alvherre@2ndquadrant.com> 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

Re: psql display of foreign keys

David Fetter <david@fetter.org> — 2018-12-04T15:23:30Z

On Tue, Dec 04, 2018 at 10:00:00AM -0500, Tom Lane wrote:
> Alvaro Herrera <alvherre@2ndquadrant.com> 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

Re: psql display of foreign keys

Alvaro Herrera <alvherre@2ndquadrant.com> — 2018-12-04T15:46:18Z

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

Re: psql display of foreign keys

Alvaro Herrera <alvherre@2ndquadrant.com> — 2018-12-04T18:41:59Z

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

Re: psql display of foreign keys

Alvaro Herrera <alvherre@2ndquadrant.com> — 2018-12-04T18:48:56Z

On 2018-Dec-04, Alvaro Herrera wrote:

> v2 attached.

Oops.

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

Re: psql display of foreign keys

Michael Paquier <michael@paquier.xyz> — 2018-12-05T01:16:33Z

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

Re: psql display of foreign keys

Alvaro Herrera <alvherre@2ndquadrant.com> — 2018-12-05T16:09:54Z

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

Re: psql display of foreign keys

Alvaro Herrera <alvherre@2ndquadrant.com> — 2018-12-05T16:30:39Z

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

Re: psql display of foreign keys

Alvaro Herrera <alvherre@2ndquadrant.com> — 2018-12-05T17:56:04Z

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

Re: psql display of foreign keys

Robert Haas <robertmhaas@gmail.com> — 2018-12-06T16:23:38Z

On Wed, Dec 5, 2018 at 11:30 AM Alvaro Herrera <alvherre@2ndquadrant.com> 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

Re: psql display of foreign keys

Alvaro Herrera <alvherre@2ndquadrant.com> — 2018-12-06T16:26:30Z

On 2018-Dec-06, Robert Haas wrote:

> On Wed, Dec 5, 2018 at 11:30 AM Alvaro Herrera <alvherre@2ndquadrant.com> 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

Re: psql display of foreign keys

Michael Paquier <michael@paquier.xyz> — 2018-12-06T22:56:36Z

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

Re: psql display of foreign keys

Peter Eisentraut <peter.eisentraut@2ndquadrant.com> — 2019-01-02T20:38:40Z

On 06/12/2018 23:56, Michael Paquier wrote:
> 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.

I'm setting this to "Waiting on Author", awaiting a new version based on
pg_partition_root() once that one is done.

-- 
Peter Eisentraut              http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Re: psql display of foreign keys

Michael Paquier <michael@paquier.xyz> — 2019-02-04T01:55:38Z

On Wed, Jan 02, 2019 at 09:38:40PM +0100, Peter Eisentraut wrote:
> I'm setting this to "Waiting on Author", awaiting a new version based on
> pg_partition_root() once that one is done.

pg_partition_root() has not made it to the finish line yet, still it
would have been nice to see a rebase, and the patch has been waiting
for input for 4 weeks now.  So I am marking it as returned with
feedback.
--
Michael

Re: psql display of foreign keys

Alvaro Herrera <alvherre@2ndquadrant.com> — 2019-02-26T22:27:57Z

On 2019-Feb-04, Michael Paquier wrote:

> pg_partition_root() has not made it to the finish line yet, still it
> would have been nice to see a rebase, and the patch has been waiting
> for input for 4 weeks now.  So I am marking it as returned with
> feedback.

Thanks for committing pg_partition_root ... but it turns out to be
useless for this purpose.  It turns out that we need to obtain the list
of *ancestors* of the table being displayed, which pg_partition_tree
does not easily give you.  So I ended up adding yet another auxiliary
function, pg_partition_ancestors, which in its current formulation
returns just a lits of OIDs of ancestor tables.  This seems generally
useful, and can be used in conjunction with pg_partition_tree():

alvherre=# select t.* from pg_partition_tree(pg_partition_root('pk11')) t
           join pg_partition_ancestors('pk11', true) a on (t.relid = a.relid);
 relid | parentrelid | isleaf | level 
-------+-------------+--------+-------
 pk    |             | f      |     0
 pk1   | pk          | f      |     1
 pk11  | pk1         | t      |     2
(3 filas)

(A small tweak is to change the return type from OID to regclass.
Docbook additions missing also.)

Anyway, given this function, it's possible to fix the psql display to be
as I showed previously.  Patches attached.

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

Re: psql display of foreign keys

Michael Paquier <michael@paquier.xyz> — 2019-02-27T06:24:01Z

On Tue, Feb 26, 2019 at 07:27:57PM -0300, Alvaro Herrera wrote:
> Thanks for committing pg_partition_root ... but it turns out to be
> useless for this purpose.

Well, what's done is done.  The thing is useful by itself in my
opinion.

> It turns out that we need to obtain the list
> of *ancestors* of the table being displayed, which pg_partition_tree
> does not easily give you.  So I ended up adding yet another auxiliary
> function, pg_partition_ancestors, which in its current formulation
> returns just a lits of OIDs of ancestor tables.  This seems generally
> useful, and can be used in conjunction with pg_partition_tree():
> 
> alvherre=# select t.* from pg_partition_tree(pg_partition_root('pk11')) t
>            join pg_partition_ancestors('pk11', true) a on (t.relid = a.relid);
>  relid | parentrelid | isleaf | level 
> -------+-------------+--------+-------
>  pk    |             | f      |     0
>  pk1   | pk          | f      |     1
>  pk11  | pk1         | t      |     2
> (3 filas)
> 
> (A small tweak is to change the return type from OID to regclass.
> Docbook additions missing also.)

In the second patch, pg_partition_ancestors always sets include_self
to true.  What's the use case you have in mind to set it to false?  In
the other existing functions we always include the argument itself, so
we may want to keep things consistent.

I think that you should make the function return a record of regclass
elements instead of OIDs to be consistent.  This could save casts for
the callers.

Adding the self-member at the beginning of the record set is more
consistent with the order of the results returned by
get_partition_ancestors().

It would be nice to add some tests in partition_info.sql for tables
and indexes (both work).

> Anyway, given this function, it's possible to fix the psql display to be
> as I showed previously.  Patches attached.

+    "  FROM pg_constraint, pg_partition_ancestors('%s', 't')\n"
+    " WHERE confrelid = relid AND contype = 'f' AND conparentid = 0\n"
A JOIN would have been cleaner in my opinion, but feel free with the
style you think is more adapted.

For the meaning of using pg_partition_ancestors, I see...  Not only do
you want to show the foreign keys defined in the top-most parent, but
also these defined in intermediate layers.  That makes sense.  Using
only pg_partition_root would have been enough to show FKs in the
top-most parent, but the intermediate ones would be missed (using only
pg_partition_root() would miss the FKs fk_partitioned_fk_5_a_fkey1 and
fk_partitioned_fk_5_a_fkey when doing "\d fk_partitioned_fk_5_1" based
on the test set).
--
Michael

Re: psql display of foreign keys

Alvaro Herrera <alvherre@2ndquadrant.com> — 2019-02-27T18:37:23Z

On 2019-Feb-27, Michael Paquier wrote:

> On Tue, Feb 26, 2019 at 07:27:57PM -0300, Alvaro Herrera wrote:
> > Thanks for committing pg_partition_root ... but it turns out to be
> > useless for this purpose.
> 
> Well, what's done is done.  The thing is useful by itself in my
> opinion.

Eh, of course -- note that the psql query I added does use
pg_partition_root, it's just that it is not useful *all by itself*.

> In the second patch, pg_partition_ancestors always sets include_self
> to true.  What's the use case you have in mind to set it to false?  In
> the other existing functions we always include the argument itself, so
> we may want to keep things consistent.

Hmm, true.

> I think that you should make the function return a record of regclass
> elements instead of OIDs to be consistent.  This could save casts for
> the callers.

Yeah, done.

> Adding the self-member at the beginning of the record set is more
> consistent with the order of the results returned by
> get_partition_ancestors().

You're right, done.

> It would be nice to add some tests in partition_info.sql for tables
> and indexes (both work).

Well.  I tried this scenario
create table t1 (a int);
create table t11 () inherits (t1);
create table t2 (b int);
create table t111() inherits (t1, t2);

and the result I get from my new function is not good:
alvherre=# select * from pg_partition_ancestors('t111');
 relid 
-------
 t111
 t1
(2 filas)

it should have listed t2 too, but it doesn't.  Since these functions
aren't supposed to work on legacy inheritance anyway, I think the right
action is to return the empty set.  In the current version I just do
what pg_partition_tree does, but I think we should adjust that behavior.
I'll start a new thread about that.

> For the meaning of using pg_partition_ancestors, I see...  Not only do
> you want to show the foreign keys defined in the top-most parent, but
> also these defined in intermediate layers.  That makes sense.  Using
> only pg_partition_root would have been enough to show FKs in the
> top-most parent, but the intermediate ones would be missed (using only
> pg_partition_root() would miss the FKs fk_partitioned_fk_5_a_fkey1 and
> fk_partitioned_fk_5_a_fkey when doing "\d fk_partitioned_fk_5_1" based
> on the test set).

Exactly -- that's the whole point.  We need to list all FKs that are
applicable to the partition, indicating which relation is the one where
the FK generates, and without polluting the output with countless
"internal" pg_constraint rows.  The output psql presents for the PK-side
relation when it's partitioned, with my patch to support that, is quite
ugly when there are many partitions.

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

Re: psql display of foreign keys

Michael Paquier <michael@paquier.xyz> — 2019-02-28T03:25:27Z

On Wed, Feb 27, 2019 at 03:37:23PM -0300, Alvaro Herrera wrote:
> It should have listed t2 too, but it doesn't.  Since these functions
> aren't supposed to work on legacy inheritance anyway, I think the right
> action is to return the empty set.  In the current version I just do
> what pg_partition_tree does, but I think we should adjust that behavior.
> I'll start a new thread about that.

Yes, that's not good.  The internal wrapper for ancestors should be
reworked.  The results of pg_partition_tree are what I would expect
them to be though?  Taking your example, t111 gets listed if listing
the trees from t1 or t2.  This seems natural to me.  I am wondering
the amount of work that it would take to actually have the function
return both relations in this case..

> +pg_partition_ancestors(PG_FUNCTION_ARGS)
> +{
> +	Oid			relid = PG_GETARG_OID(0);
> +	FuncCallContext *funcctx;
> +	ListCell  **next;
> +
> +	if (!check_rel_can_be_partition(relid))
> +		PG_RETURN_NULL();

Not returning an empty set here? ;)

I would have added tests with pg_partition_ancestors(NULL) and
pg_partition_ancestors(0) for consistency with the rest.

Except that and the ancestor tracking for inheritance, the shape of
the patch looks good to me.
--
Michael

Re: psql display of foreign keys

Alvaro Herrera <alvherre@2ndquadrant.com> — 2019-03-04T19:41:00Z

On 2019-Feb-28, Michael Paquier wrote:

> On Wed, Feb 27, 2019 at 03:37:23PM -0300, Alvaro Herrera wrote:

> > +pg_partition_ancestors(PG_FUNCTION_ARGS)
> > +{
> > +	Oid			relid = PG_GETARG_OID(0);
> > +	FuncCallContext *funcctx;
> > +	ListCell  **next;
> > +
> > +	if (!check_rel_can_be_partition(relid))
> > +		PG_RETURN_NULL();
> 
> Not returning an empty set here? ;)

Yeah, I adapted to what was there then, but in the original coding I had
the SRF_RETURN_DONE that you committed for pg_partition_tree.

> I would have added tests with pg_partition_ancestors(NULL) and
> pg_partition_ancestors(0) for consistency with the rest.

Done.

> Except that and the ancestor tracking for inheritance, the shape of
> the patch looks good to me.

Thanks for reviewing!  I have pushed with your proposed changes.

Here's the patch I'm really interested about :-)

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

Re: psql display of foreign keys

Amit Langote <langote_amit_f8@lab.ntt.co.jp> — 2019-03-05T02:01:33Z

On 2019/03/05 4:41, Alvaro Herrera wrote:
> Here's the patch I'm really interested about :-)

Thanks for the updated patch.  I applied it and rebased the
foreign-keys-referencing-partitioned-tables patch on top.  Here's
something I think you may have missed:

-- partitioned primary key table
create table p (a int primary key) partition by list (a);
create table p1 partition of p for values in (1) partition by list (a);
create table p11 partition of p1 for values in (1);

-- regular primary key table
create table pk (a int primary key);

-- another partitioned table to define FK on
create table q (a int) partition by list (a);
create table q1 partition of q for values in (1) partition by list (a);
create table q11 partition of q1 for values in (1);

-- FK on q referencing p
alter table q add foreign key (a) references p;

-- seems OK

\d p
           Partitioned table "public.p"
 Column │  Type   │ Collation │ Nullable │ Default
────────┼─────────┼───────────┼──────────┼─────────
 a      │ integer │           │ not null │
Partition key: LIST (a)
Indexes:
    "p_pkey" PRIMARY KEY, btree (a)
Referenced by:
    TABLE "q" CONSTRAINT "q_a_fkey" FOREIGN KEY (a) REFERENCES p(a)
Number of partitions: 1 (Use \d+ to list them.)

\d p1
           Partitioned table "public.p1"
 Column │  Type   │ Collation │ Nullable │ Default
────────┼─────────┼───────────┼──────────┼─────────
 a      │ integer │           │ not null │
Partition of: p FOR VALUES IN (1)
Partition key: LIST (a)
Indexes:
    "p1_pkey" PRIMARY KEY, btree (a)
Referenced by:
    TABLE "q" CONSTRAINT "q_a_fkey" FOREIGN KEY (a) REFERENCES p(a)
Number of partitions: 1 (Use \d+ to list them.)

\d p11
                Table "public.p11"
 Column │  Type   │ Collation │ Nullable │ Default
────────┼─────────┼───────────┼──────────┼─────────
 a      │ integer │           │ not null │
Partition of: p1 FOR VALUES IN (1)
Indexes:
    "p11_pkey" PRIMARY KEY, btree (a)
Referenced by:
    TABLE "q" CONSTRAINT "q_a_fkey" FOREIGN KEY (a) REFERENCES p(a)

-- change the FK to reference regular table
alter table q drop constraint q_a_fkey ;
alter table q add foreign key (a) references pk;

-- not OK?
\d pk
                 Table "public.pk"
 Column │  Type   │ Collation │ Nullable │ Default
────────┼─────────┼───────────┼──────────┼─────────
 a      │ integer │           │ not null │
Indexes:
    "pk_pkey" PRIMARY KEY, btree (a)
Referenced by:
    TABLE "q" CONSTRAINT "q_a_fkey" FOREIGN KEY (a) REFERENCES pk(a)
    TABLE "q1" CONSTRAINT "q_a_fkey" FOREIGN KEY (a) REFERENCES pk(a)
    TABLE "q11" CONSTRAINT "q_a_fkey" FOREIGN KEY (a) REFERENCES pk(a)

Shouldn't the above only list the constraint on q as follows?

Referenced by:
    TABLE "q" CONSTRAINT "q_a_fkey" FOREIGN KEY (a) REFERENCES pk(a)


Maybe:

@@ -2488,7 +2488,8 @@ describeOneTableDetails(const char *schemaname,
                                   "SELECT conname,
conrelid::pg_catalog.regclass,\n"
                                   "
pg_catalog.pg_get_constraintdef(c.oid, true) as condef\n"
                                   "FROM pg_catalog.pg_constraint c\n"
-                                  "WHERE c.confrelid = '%s' AND c.contype
= 'f' ORDER BY 1;",
+                                  "WHERE c.confrelid = '%s' AND c.contype
= 'f' AND conparentid = 0\n"
+                                  "ORDER BY conname;",

Thanks,
Amit

Re: psql display of foreign keys

Alvaro Herrera <alvherre@2ndquadrant.com> — 2019-03-22T22:15:57Z

On 2019-Mar-05, Amit Langote wrote:

> On 2019/03/05 4:41, Alvaro Herrera wrote:
> > Here's the patch I'm really interested about :-)
> 
> Thanks for the updated patch.  I applied it and rebased the
> foreign-keys-referencing-partitioned-tables patch on top.  Here's
> something I think you may have missed:

I missed that indeed!  Thanks for noticing.  Here's an updated and
rebased version of this patch.

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

Re: psql display of foreign keys

Alvaro Herrera <alvherre@2ndquadrant.com> — 2019-03-23T02:30:31Z

On 2019-Mar-22, Alvaro Herrera wrote:

> On 2019-Mar-05, Amit Langote wrote:
> 
> > On 2019/03/05 4:41, Alvaro Herrera wrote:
> > > Here's the patch I'm really interested about :-)
> > 
> > Thanks for the updated patch.  I applied it and rebased the
> > foreign-keys-referencing-partitioned-tables patch on top.  Here's
> > something I think you may have missed:
> 
> I missed that indeed!  Thanks for noticing.  Here's an updated and
> rebased version of this patch.

I forgot to "git add" the new changes to the expected file.  Here's v8
with that fixed.

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

Re: psql display of foreign keys

Peter Eisentraut <peter.eisentraut@2ndquadrant.com> — 2019-03-25T11:04:40Z

On 2019-03-23 03:30, Alvaro Herrera wrote:
>>> Thanks for the updated patch.  I applied it and rebased the
>>> foreign-keys-referencing-partitioned-tables patch on top.  Here's
>>> something I think you may have missed:
>>
>> I missed that indeed!  Thanks for noticing.  Here's an updated and
>> rebased version of this patch.
> 
> I forgot to "git add" the new changes to the expected file.  Here's v8
> with that fixed.

Looks OK in general.

relispartition was added in PG10, so the conditional in
describeOneTableDetails() seems wrong.

In the older branches of that same function, I'd prefer writing

    false AS relispartition

for clarity.

Some of the other queries could also use some column aliases, like

    conrelid = '%s'::pg_catalog.regclass AS isroot (?)

or

    pg_catalog.pg_get_constraintdef(oid, true) AS condef

(as in the other branch).

A test case for the incoming foreign key display would be nice, as that
was the original argument for the patch.

-- 
Peter Eisentraut              http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Re: psql display of foreign keys

Alvaro Herrera <alvherre@2ndquadrant.com> — 2019-03-25T22:38:52Z

v9 attached; this one's final AFAICT.

On 2019-Mar-25, Peter Eisentraut wrote:

> relispartition was added in PG10, so the conditional in
> describeOneTableDetails() seems wrong.

Hmm, yeah, we weren't using it anyway (since we can only use the new
display with pg_partition_ancestors which is new in pg12), but I guess
you have a point that this could confuse somebody in the future.

> In the older branches of that same function, I'd prefer writing
> 
>     false AS relispartition
> 
> for clarity.

Yeah, some previous commits in that area have added "false" flags here
and there without adding aliases.  We should fix those sometime.  And
also the new "amname" output column is conditional on the version number
and changes column numbering for any column that appears afterwards ...
that one definitely deserves a "NULL as amname" in the older branches.

I changed some code to use PQfnumber() the way pg_dump does it; that
code's support for back-branch compatibility is much more battle-tested
than psql's and I trust that to be more maintainable.  In fact, my
motivation for doing it that way is that I found psql's way to be
confusing.

> Some of the other queries could also use some column aliases, like
> 
>     conrelid = '%s'::pg_catalog.regclass AS isroot (?)
> 
> or
> 
>     pg_catalog.pg_get_constraintdef(oid, true) AS condef
> 
> (as in the other branch).

Agreed, added.

> A test case for the incoming foreign key display would be nice, as that
> was the original argument for the patch.

Agreed, added.

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

Re: psql display of foreign keys

Alvaro Herrera <alvherre@2ndquadrant.com> — 2019-03-26T01:57:29Z

On 2019-Mar-25, Alvaro Herrera wrote:

> v9 attached; this one's final AFAICT.

Actually, I propose this fixup.  It doesn't change the current output,
but of course it affects how this works with my patch in
https://postgr.es/m/20190321215420.GA22766@alvherre.pgsql
The v9 patch does not show anything for the partitions of the referenced
partitioned table; with this one it shows like this

-- verify psql behaves sanely
\d droppk
       Partitioned table "regress_fk.droppk"
 Column |  Type   | Collation | Nullable | Default 
--------+---------+-----------+----------+---------
 a      | integer |           | not null | 
Partition key: RANGE (a)
Indexes:
    "droppk_pkey" PRIMARY KEY, btree (a)
Referenced by:
    TABLE "dropfk" CONSTRAINT "dropfk_a_fkey" FOREIGN KEY (a) REFERENCES droppk(a)
Number of partitions: 3 (Use \d+ to list them.)

\d droppk21
            Table "regress_fk.droppk21"
 Column |  Type   | Collation | Nullable | Default 
--------+---------+-----------+----------+---------
 a      | integer |           | not null | 
Partition of: droppk2 FOR VALUES FROM (1000) TO (1400)
Indexes:
    "droppk21_pkey" PRIMARY KEY, btree (a)
Referenced by:
    TABLE "dropfk" CONSTRAINT "dropfk_a_fkey" FOREIGN KEY (a) REFERENCES droppk(a)

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

Re: psql display of foreign keys

Amit Langote <langote_amit_f8@lab.ntt.co.jp> — 2019-03-26T01:58:06Z

Hi,

On 2019/03/26 7:38, Alvaro Herrera wrote:
> v9 attached; this one's final AFAICT.

Agreed.

Thanks,
Amit

Re: psql display of foreign keys

Alvaro Herrera <alvherre@2ndquadrant.com> — 2019-03-26T02:42:53Z

Patch tester didn't like that one bit.  Here's v10 with the fixup
applied.

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

Re: psql display of foreign keys

Peter Eisentraut <peter.eisentraut@2ndquadrant.com> — 2019-03-26T09:30:39Z

On 2019-03-26 03:42, Alvaro Herrera wrote:
> Patch tester didn't like that one bit.  Here's v10 with the fixup
> applied.

Looks good to me.

-- 
Peter Eisentraut              http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Re: psql display of foreign keys

Alvaro Herrera <alvherre@2ndquadrant.com> — 2019-03-26T14:35:50Z

On 2019-Mar-26, Peter Eisentraut wrote:

> On 2019-03-26 03:42, Alvaro Herrera wrote:
> > Patch tester didn't like that one bit.  Here's v10 with the fixup
> > applied.
> 
> Looks good to me.

Thanks!

I ran "make installcheck-parallel" using this psql version on all
supported branches plus 9.2.  There were some expected failures, such as
some changed table titles, "FDW options" changed to "FDW Options", and
things like that; but AFAICT other than those everything works as
expected.  (I did find a couple of bugs this way.)

Now pushed.

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