Thread: pg_constraint.conincluding is useless

pg_constraint.conincluding is useless

From
Alvaro Herrera
Date:
Hi

Already mentioned this in 
https://postgr.es/m/20180831205020.nxhw6ypysgshjtnl@alvherre.pgsql

While trying to add support for foreign keys to partitioned tables, I
noticed that commit 8224de4f42cc ("Indexes with INCLUDE columns and
their support in B-tree") added a column to pg_constraint that appears
to be there only to enable ruleutils.c to print out the list of columns
in a PRIMARY KEY or UNIQUE constraint that uses included columns.
However, this is pretty easy to obtain from pg_index.conkey instead, so
I claim that that column is useless.  In fact, here's a patch to remove
it.

This requires a catversion bump, for which it may seem a bit late;
however I think it's better to release pg11 without a useless catalog
column only to remove it in pg12 ...

Thoughts?

-- 
Álvaro Herrera

Attachment

Re: pg_constraint.conincluding is useless

From
Tom Lane
Date:
Alvaro Herrera <alvherre@2ndquadrant.com> writes:
> This requires a catversion bump, for which it may seem a bit late;
> however I think it's better to release pg11 without a useless catalog
> column only to remove it in pg12 ...

Catversion bumps during beta are routine.  If we had put out rc1
I'd say it was too late, but we have not.

If we do do a bump for beta4, I'd be strongly tempted to address the
lack of a unique index for pg_constraint as well, cf
https://www.postgresql.org/message-id/10110.1535907645@sss.pgh.pa.us

            regards, tom lane


Re: pg_constraint.conincluding is useless

From
Michael Paquier
Date:
On Sun, Sep 02, 2018 at 01:27:25PM -0400, Tom Lane wrote:
> Alvaro Herrera <alvherre@2ndquadrant.com> writes:
> > This requires a catversion bump, for which it may seem a bit late;
> > however I think it's better to release pg11 without a useless catalog
> > column only to remove it in pg12 ...
>
> Catversion bumps during beta are routine.  If we had put out rc1
> I'd say it was too late, but we have not.

At the same time Covering indexes are a new thing, so if the timing
allows, let's move on with having a cleaner catalog layer from the
start, that's less compatibility breakages to justify later on.
Hopefully.

> If we do do a bump for beta4, I'd be strongly tempted to address the
> lack of a unique index for pg_constraint as well, cf
> https://www.postgresql.org/message-id/10110.1535907645@sss.pgh.pa.us

Yeah...  I looked at the thread.
--
Michael

Attachment

Re: pg_constraint.conincluding is useless

From
Stephen Frost
Date:
Greetings,

* Tom Lane (tgl@sss.pgh.pa.us) wrote:
> Alvaro Herrera <alvherre@2ndquadrant.com> writes:
> > This requires a catversion bump, for which it may seem a bit late;
> > however I think it's better to release pg11 without a useless catalog
> > column only to remove it in pg12 ...

Yeah, I really don't think we want to have another catalog column that
we end up wanting to remove later on, if we can avoid it..

> Catversion bumps during beta are routine.  If we had put out rc1
> I'd say it was too late, but we have not.

I agree that rc1 would be too late.  On the flip side, I don't think
we should really consider catversion bumps during beta to be 'routine'.

> If we do do a bump for beta4, I'd be strongly tempted to address the
> lack of a unique index for pg_constraint as well, cf
> https://www.postgresql.org/message-id/10110.1535907645@sss.pgh.pa.us

All that said, given that we've got two pretty good reasons for a
catversion bump, and one is to remove a useless column before it ever
gets in a release, I'm +1 for making both of these changes.

Thanks!

Stephen

Attachment

Re: pg_constraint.conincluding is useless

From
Bruce Momjian
Date:
On Sun, Sep  2, 2018 at 01:27:25PM -0400, Tom Lane wrote:
> Alvaro Herrera <alvherre@2ndquadrant.com> writes:
> > This requires a catversion bump, for which it may seem a bit late;
> > however I think it's better to release pg11 without a useless catalog
> > column only to remove it in pg12 ...
> 
> Catversion bumps during beta are routine.  If we had put out rc1
> I'd say it was too late, but we have not.
> 
> If we do do a bump for beta4, I'd be strongly tempted to address the
> lack of a unique index for pg_constraint as well, cf
> https://www.postgresql.org/message-id/10110.1535907645@sss.pgh.pa.us

Uh, if we add a unique index later, wouldn't that potentially cause
future restores to fail?  Seems we better add it now.

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

+ As you are, so once was I.  As I am, so you will be. +
+                      Ancient Roman grave inscription +


Re: pg_constraint.conincluding is useless

From
Alvaro Herrera
Date:
On 2018-Sep-07, Bruce Momjian wrote:

> On Sun, Sep  2, 2018 at 01:27:25PM -0400, Tom Lane wrote:
> > Alvaro Herrera <alvherre@2ndquadrant.com> writes:
> > > This requires a catversion bump, for which it may seem a bit late;
> > > however I think it's better to release pg11 without a useless catalog
> > > column only to remove it in pg12 ...
> > 
> > Catversion bumps during beta are routine.  If we had put out rc1
> > I'd say it was too late, but we have not.
> > 
> > If we do do a bump for beta4, I'd be strongly tempted to address the
> > lack of a unique index for pg_constraint as well, cf
> > https://www.postgresql.org/message-id/10110.1535907645@sss.pgh.pa.us
> 
> Uh, if we add a unique index later, wouldn't that potentially cause
> future restores to fail?  Seems we better add it now.

Committed on Sep 4th as 17b7c302b5fc.

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


Re: pg_constraint.conincluding is useless

From
Tom Lane
Date:
Bruce Momjian <bruce@momjian.us> writes:
> On Sun, Sep  2, 2018 at 01:27:25PM -0400, Tom Lane wrote:
>> If we do do a bump for beta4, I'd be strongly tempted to address the
>> lack of a unique index for pg_constraint as well, cf
>> https://www.postgresql.org/message-id/10110.1535907645@sss.pgh.pa.us

> Uh, if we add a unique index later, wouldn't that potentially cause
> future restores to fail?  Seems we better add it now.

Yup, done already.

            regards, tom lane