pg_constraint.conincluding is useless
Hi
Already mentioned this in
/messages/by-id/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
Attachments:
0001-remove-conincluding.patchtext/plain; charset=us-asciiDownload
From 5e9c74c4dc59afb348b8b3ea5233b7cbf43c0616 Mon Sep 17 00:00:00 2001
From: Alvaro Herrera <alvherre@alvh.no-ip.org>
Date: Sun, 2 Sep 2018 13:35:46 -0300
Subject: [PATCH] remove conincluding
---
doc/src/sgml/catalogs.sgml | 8 ----
src/backend/catalog/pg_constraint.c | 21 ----------
src/backend/utils/adt/ruleutils.c | 57 +++++++++++++++++++++------
src/include/catalog/pg_constraint.h | 6 ---
src/test/regress/expected/index_including.out | 40 +++++++++----------
src/test/regress/sql/index_including.sql | 10 ++---
6 files changed, 70 insertions(+), 72 deletions(-)
diff --git a/doc/src/sgml/catalogs.sgml b/doc/src/sgml/catalogs.sgml
index 07e8b3325f..0179deea2e 100644
--- a/doc/src/sgml/catalogs.sgml
+++ b/doc/src/sgml/catalogs.sgml
@@ -2374,14 +2374,6 @@ SCRAM-SHA-256$<replaceable><iteration count></replaceable>:<replaceable>&l
</row>
<row>
- <entry><structfield>conincluding</structfield></entry>
- <entry><type>int2[]</type></entry>
- <entry><literal><link linkend="catalog-pg-attribute"><structname>pg_attribute</structname></link>.attnum</literal></entry>
- <entry>List of the non-constrained columns which are included into
- the same index as the constrained columns</entry>
- </row>
-
- <row>
<entry><structfield>confkey</structfield></entry>
<entry><type>int2[]</type></entry>
<entry><literal><link linkend="catalog-pg-attribute"><structname>pg_attribute</structname></link>.attnum</literal></entry>
diff --git a/src/backend/catalog/pg_constraint.c b/src/backend/catalog/pg_constraint.c
index 7a6d158f89..ea84441360 100644
--- a/src/backend/catalog/pg_constraint.c
+++ b/src/backend/catalog/pg_constraint.c
@@ -85,7 +85,6 @@ CreateConstraintEntry(const char *constraintName,
bool nulls[Natts_pg_constraint];
Datum values[Natts_pg_constraint];
ArrayType *conkeyArray;
- ArrayType *conincludingArray;
ArrayType *confkeyArray;
ArrayType *conpfeqopArray;
ArrayType *conppeqopArray;
@@ -116,21 +115,6 @@ CreateConstraintEntry(const char *constraintName,
else
conkeyArray = NULL;
- if (constraintNTotalKeys > constraintNKeys)
- {
- Datum *conincluding;
- int j = 0;
- int constraintNIncludedKeys = constraintNTotalKeys - constraintNKeys;
-
- conincluding = (Datum *) palloc(constraintNIncludedKeys * sizeof(Datum));
- for (i = constraintNKeys; i < constraintNTotalKeys; i++)
- conincluding[j++] = Int16GetDatum(constraintKey[i]);
- conincludingArray = construct_array(conincluding, constraintNIncludedKeys,
- INT2OID, 2, true, 's');
- }
- else
- conincludingArray = NULL;
-
if (foreignNKeys > 0)
{
Datum *fkdatums;
@@ -204,11 +188,6 @@ CreateConstraintEntry(const char *constraintName,
else
nulls[Anum_pg_constraint_conkey - 1] = true;
- if (conincludingArray)
- values[Anum_pg_constraint_conincluding - 1] = PointerGetDatum(conincludingArray);
- else
- nulls[Anum_pg_constraint_conincluding - 1] = true;
-
if (confkeyArray)
values[Anum_pg_constraint_confkey - 1] = PointerGetDatum(confkeyArray);
else
diff --git a/src/backend/utils/adt/ruleutils.c b/src/backend/utils/adt/ruleutils.c
index 03e9a28a63..bd0792e13e 100644
--- a/src/backend/utils/adt/ruleutils.c
+++ b/src/backend/utils/adt/ruleutils.c
@@ -315,7 +315,7 @@ static char *deparse_expression_pretty(Node *expr, List *dpcontext,
static char *pg_get_viewdef_worker(Oid viewoid,
int prettyFlags, int wrapColumn);
static char *pg_get_triggerdef_worker(Oid trigid, bool pretty);
-static void decompile_column_index_array(Datum column_index_array, Oid relId,
+static int decompile_column_index_array(Datum column_index_array, Oid relId,
StringInfo buf);
static char *pg_get_ruledef_worker(Oid ruleoid, int prettyFlags);
static char *pg_get_indexdef_worker(Oid indexrelid, int colno,
@@ -2055,6 +2055,8 @@ pg_get_constraintdef_worker(Oid constraintId, bool fullCommand,
Datum val;
bool isnull;
Oid indexId;
+ int keyatts;
+ HeapTuple indtup;
/* Start off the constraint definition */
if (conForm->contype == CONSTRAINT_PRIMARY)
@@ -2069,24 +2071,52 @@ pg_get_constraintdef_worker(Oid constraintId, bool fullCommand,
elog(ERROR, "null conkey for constraint %u",
constraintId);
- decompile_column_index_array(val, conForm->conrelid, &buf);
+ keyatts = decompile_column_index_array(val, conForm->conrelid, &buf);
appendStringInfoChar(&buf, ')');
- /* Fetch and build including column list */
- isnull = true;
- val = SysCacheGetAttr(CONSTROID, tup,
- Anum_pg_constraint_conincluding, &isnull);
- if (!isnull)
+ indexId = get_constraint_index(constraintId);
+
+ /* Build including column list (from pg_index.indkeys) */
+ indtup = SearchSysCache1(INDEXRELID, ObjectIdGetDatum(indexId));
+ if (!HeapTupleIsValid(indtup))
+ elog(ERROR, "cache lookup failed for index %u", indexId);
+ val = SysCacheGetAttr(INDEXRELID, indtup,
+ Anum_pg_index_indnatts, &isnull);
+ if (isnull)
+ elog(ERROR, "null indnatts for index %u", indexId);
+ if (DatumGetInt32(val) > keyatts)
{
+ Datum cols;
+ Datum *keys;
+ int nKeys;
+ int j;
+
appendStringInfoString(&buf, " INCLUDE (");
- decompile_column_index_array(val, conForm->conrelid, &buf);
+ cols = SysCacheGetAttr(INDEXRELID, indtup,
+ Anum_pg_index_indkey, &isnull);
+ if (isnull)
+ elog(ERROR, "null indkey for index %u", indexId);
+
+ deconstruct_array(DatumGetArrayTypeP(cols),
+ INT2OID, 2, true, 's',
+ &keys, NULL, &nKeys);
+
+ for (j = keyatts; j < nKeys; j++)
+ {
+ char *colName;
+
+ colName = get_attname(conForm->conrelid, DatumGetInt16(keys[j]), false);
+ if (j == keyatts)
+ appendStringInfoString(&buf, quote_identifier(colName));
+ else
+ appendStringInfo(&buf, ", %s", quote_identifier(colName));
+ }
appendStringInfoChar(&buf, ')');
}
-
- indexId = get_constraint_index(constraintId);
+ ReleaseSysCache(indtup);
/* XXX why do we only print these bits if fullCommand? */
if (fullCommand && OidIsValid(indexId))
@@ -2232,9 +2262,10 @@ pg_get_constraintdef_worker(Oid constraintId, bool fullCommand,
/*
* Convert an int16[] Datum into a comma-separated list of column names
- * for the indicated relation; append the list to buf.
+ * for the indicated relation; append the list to buf. Returns the number
+ * of keys.
*/
-static void
+static int
decompile_column_index_array(Datum column_index_array, Oid relId,
StringInfo buf)
{
@@ -2258,6 +2289,8 @@ decompile_column_index_array(Datum column_index_array, Oid relId,
else
appendStringInfo(buf, ", %s", quote_identifier(colName));
}
+
+ return nKeys;
}
diff --git a/src/include/catalog/pg_constraint.h b/src/include/catalog/pg_constraint.h
index 7c1c0e1db8..9d209c9d19 100644
--- a/src/include/catalog/pg_constraint.h
+++ b/src/include/catalog/pg_constraint.h
@@ -106,12 +106,6 @@ CATALOG(pg_constraint,2606,ConstraintRelationId)
int16 conkey[1];
/*
- * Columns of conrelid that the constraint does not apply to, but are
- * included into the same index as the key columns
- */
- int16 conincluding[1];
-
- /*
* If a foreign key, the referenced columns of confrelid
*/
int16 confkey[1];
diff --git a/src/test/regress/expected/index_including.out b/src/test/regress/expected/index_including.out
index 45a1c8d017..16b4be34de 100644
--- a/src/test/regress/expected/index_including.out
+++ b/src/test/regress/expected/index_including.out
@@ -94,10 +94,10 @@ SELECT indexrelid::regclass, indnatts, indnkeyatts, indisunique, indisprimary, i
covering | 4 | 2 | t | f | 1 2 3 4 | 1978 1978
(1 row)
-SELECT pg_get_constraintdef(oid), conname, conkey, conincluding FROM pg_constraint WHERE conrelid = 'tbl'::regclass::oid;
- pg_get_constraintdef | conname | conkey | conincluding
-----------------------------------+----------+--------+--------------
- UNIQUE (c1, c2) INCLUDE (c3, c4) | covering | {1,2} | {3,4}
+SELECT pg_get_constraintdef(oid), conname, conkey FROM pg_constraint WHERE conrelid = 'tbl'::regclass::oid;
+ pg_get_constraintdef | conname | conkey
+----------------------------------+----------+--------
+ UNIQUE (c1, c2) INCLUDE (c3, c4) | covering | {1,2}
(1 row)
-- ensure that constraint works
@@ -113,10 +113,10 @@ SELECT indexrelid::regclass, indnatts, indnkeyatts, indisunique, indisprimary, i
covering | 4 | 2 | t | t | 1 2 3 4 | 1978 1978
(1 row)
-SELECT pg_get_constraintdef(oid), conname, conkey, conincluding FROM pg_constraint WHERE conrelid = 'tbl'::regclass::oid;
- pg_get_constraintdef | conname | conkey | conincluding
----------------------------------------+----------+--------+--------------
- PRIMARY KEY (c1, c2) INCLUDE (c3, c4) | covering | {1,2} | {3,4}
+SELECT pg_get_constraintdef(oid), conname, conkey FROM pg_constraint WHERE conrelid = 'tbl'::regclass::oid;
+ pg_get_constraintdef | conname | conkey
+---------------------------------------+----------+--------
+ PRIMARY KEY (c1, c2) INCLUDE (c3, c4) | covering | {1,2}
(1 row)
-- ensure that constraint works
@@ -136,10 +136,10 @@ SELECT indexrelid::regclass, indnatts, indnkeyatts, indisunique, indisprimary, i
tbl_c1_c2_c3_c4_key | 4 | 2 | t | f | 1 2 3 4 | 1978 1978
(1 row)
-SELECT pg_get_constraintdef(oid), conname, conkey, conincluding FROM pg_constraint WHERE conrelid = 'tbl'::regclass::oid;
- pg_get_constraintdef | conname | conkey | conincluding
-----------------------------------+---------------------+--------+--------------
- UNIQUE (c1, c2) INCLUDE (c3, c4) | tbl_c1_c2_c3_c4_key | {1,2} | {3,4}
+SELECT pg_get_constraintdef(oid), conname, conkey FROM pg_constraint WHERE conrelid = 'tbl'::regclass::oid;
+ pg_get_constraintdef | conname | conkey
+----------------------------------+---------------------+--------
+ UNIQUE (c1, c2) INCLUDE (c3, c4) | tbl_c1_c2_c3_c4_key | {1,2}
(1 row)
-- ensure that constraint works
@@ -155,10 +155,10 @@ SELECT indexrelid::regclass, indnatts, indnkeyatts, indisunique, indisprimary, i
tbl_pkey | 4 | 2 | t | t | 1 2 3 4 | 1978 1978
(1 row)
-SELECT pg_get_constraintdef(oid), conname, conkey, conincluding FROM pg_constraint WHERE conrelid = 'tbl'::regclass::oid;
- pg_get_constraintdef | conname | conkey | conincluding
----------------------------------------+----------+--------+--------------
- PRIMARY KEY (c1, c2) INCLUDE (c3, c4) | tbl_pkey | {1,2} | {3,4}
+SELECT pg_get_constraintdef(oid), conname, conkey FROM pg_constraint WHERE conrelid = 'tbl'::regclass::oid;
+ pg_get_constraintdef | conname | conkey
+---------------------------------------+----------+--------
+ PRIMARY KEY (c1, c2) INCLUDE (c3, c4) | tbl_pkey | {1,2}
(1 row)
-- ensure that constraint works
@@ -178,10 +178,10 @@ SELECT indexrelid::regclass, indnatts, indnkeyatts, indisunique, indisprimary, i
tbl_c1_c3_c4_excl | 3 | 1 | f | f | 1 3 4 | 1978
(1 row)
-SELECT pg_get_constraintdef(oid), conname, conkey, conincluding FROM pg_constraint WHERE conrelid = 'tbl'::regclass::oid;
- pg_get_constraintdef | conname | conkey | conincluding
---------------------------------------------------+-------------------+--------+--------------
- EXCLUDE USING btree (c1 WITH =) INCLUDE (c3, c4) | tbl_c1_c3_c4_excl | {1} | {3,4}
+SELECT pg_get_constraintdef(oid), conname, conkey FROM pg_constraint WHERE conrelid = 'tbl'::regclass::oid;
+ pg_get_constraintdef | conname | conkey
+--------------------------------------------------+-------------------+--------
+ EXCLUDE USING btree (c1 WITH =) INCLUDE (c3, c4) | tbl_c1_c3_c4_excl | {1}
(1 row)
-- ensure that constraint works
diff --git a/src/test/regress/sql/index_including.sql b/src/test/regress/sql/index_including.sql
index ee9d9f6bae..ef5fd882f5 100644
--- a/src/test/regress/sql/index_including.sql
+++ b/src/test/regress/sql/index_including.sql
@@ -60,7 +60,7 @@ ALTER TABLE tbl_include_box_pk add PRIMARY KEY (c1, c2) INCLUDE (c3, c4);
CREATE TABLE tbl (c1 int,c2 int, c3 int, c4 box,
CONSTRAINT covering UNIQUE(c1,c2) INCLUDE(c3,c4));
SELECT indexrelid::regclass, indnatts, indnkeyatts, indisunique, indisprimary, indkey, indclass FROM pg_index WHERE indrelid = 'tbl'::regclass::oid;
-SELECT pg_get_constraintdef(oid), conname, conkey, conincluding FROM pg_constraint WHERE conrelid = 'tbl'::regclass::oid;
+SELECT pg_get_constraintdef(oid), conname, conkey FROM pg_constraint WHERE conrelid = 'tbl'::regclass::oid;
-- ensure that constraint works
INSERT INTO tbl SELECT 1, 2, 3*x, box('4,4,4,4') FROM generate_series(1,10) AS x;
DROP TABLE tbl;
@@ -68,7 +68,7 @@ DROP TABLE tbl;
CREATE TABLE tbl (c1 int,c2 int, c3 int, c4 box,
CONSTRAINT covering PRIMARY KEY(c1,c2) INCLUDE(c3,c4));
SELECT indexrelid::regclass, indnatts, indnkeyatts, indisunique, indisprimary, indkey, indclass FROM pg_index WHERE indrelid = 'tbl'::regclass::oid;
-SELECT pg_get_constraintdef(oid), conname, conkey, conincluding FROM pg_constraint WHERE conrelid = 'tbl'::regclass::oid;
+SELECT pg_get_constraintdef(oid), conname, conkey FROM pg_constraint WHERE conrelid = 'tbl'::regclass::oid;
-- ensure that constraint works
INSERT INTO tbl SELECT 1, 2, 3*x, box('4,4,4,4') FROM generate_series(1,10) AS x;
INSERT INTO tbl SELECT 1, NULL, 3*x, box('4,4,4,4') FROM generate_series(1,10) AS x;
@@ -78,7 +78,7 @@ DROP TABLE tbl;
CREATE TABLE tbl (c1 int,c2 int, c3 int, c4 box,
UNIQUE(c1,c2) INCLUDE(c3,c4));
SELECT indexrelid::regclass, indnatts, indnkeyatts, indisunique, indisprimary, indkey, indclass FROM pg_index WHERE indrelid = 'tbl'::regclass::oid;
-SELECT pg_get_constraintdef(oid), conname, conkey, conincluding FROM pg_constraint WHERE conrelid = 'tbl'::regclass::oid;
+SELECT pg_get_constraintdef(oid), conname, conkey FROM pg_constraint WHERE conrelid = 'tbl'::regclass::oid;
-- ensure that constraint works
INSERT INTO tbl SELECT 1, 2, 3*x, box('4,4,4,4') FROM generate_series(1,10) AS x;
DROP TABLE tbl;
@@ -86,7 +86,7 @@ DROP TABLE tbl;
CREATE TABLE tbl (c1 int,c2 int, c3 int, c4 box,
PRIMARY KEY(c1,c2) INCLUDE(c3,c4));
SELECT indexrelid::regclass, indnatts, indnkeyatts, indisunique, indisprimary, indkey, indclass FROM pg_index WHERE indrelid = 'tbl'::regclass::oid;
-SELECT pg_get_constraintdef(oid), conname, conkey, conincluding FROM pg_constraint WHERE conrelid = 'tbl'::regclass::oid;
+SELECT pg_get_constraintdef(oid), conname, conkey FROM pg_constraint WHERE conrelid = 'tbl'::regclass::oid;
-- ensure that constraint works
INSERT INTO tbl SELECT 1, 2, 3*x, box('4,4,4,4') FROM generate_series(1,10) AS x;
INSERT INTO tbl SELECT 1, NULL, 3*x, box('4,4,4,4') FROM generate_series(1,10) AS x;
@@ -96,7 +96,7 @@ DROP TABLE tbl;
CREATE TABLE tbl (c1 int,c2 int, c3 int, c4 box,
EXCLUDE USING btree (c1 WITH =) INCLUDE(c3,c4));
SELECT indexrelid::regclass, indnatts, indnkeyatts, indisunique, indisprimary, indkey, indclass FROM pg_index WHERE indrelid = 'tbl'::regclass::oid;
-SELECT pg_get_constraintdef(oid), conname, conkey, conincluding FROM pg_constraint WHERE conrelid = 'tbl'::regclass::oid;
+SELECT pg_get_constraintdef(oid), conname, conkey FROM pg_constraint WHERE conrelid = 'tbl'::regclass::oid;
-- ensure that constraint works
INSERT INTO tbl SELECT 1, 2, 3*x, box('4,4,4,4') FROM generate_series(1,10) AS x;
INSERT INTO tbl SELECT x, 2*x, NULL, NULL FROM generate_series(1,10) AS x;
--
2.11.0
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
/messages/by-id/10110.1535907645@sss.pgh.pa.us
regards, tom lane
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
/messages/by-id/10110.1535907645@sss.pgh.pa.us
Yeah... I looked at the thread.
--
Michael
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
/messages/by-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
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
/messages/by-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 +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
/messages/by-id/10110.1535907645@sss.pgh.pa.usUh, 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
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
/messages/by-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