Re: WIP: Covering + unique indexes

Anastasia, et al,

This is a review of including_columns_9.7_v5.patch.

I looked through the commit fest list and this patch was interesting and I wanted to try it.

I have used include columns on Microsoft SQL Server; DB2 for Linux, Unix, Windows; and DB2 for z/OS.

After reading the e-mail discussions, there are still points that I am not clear on.

Given "create table foo (a int, b int, c int, d int)" and "create unique index foo_a_b on foo (a, b) including (c)".

index only? heap tuple needed?
select a, b, c from foo where a = 1 yes no
select a, b, d from foo where a = 1 no yes
select a, b from foo where a = 1 and c = 1 ? ?

It was clear from the discussion that the index scan/access method would not resolve the "c = 1" predicate but it was not clear if "c = 1" could be resolved without accessing the heap tuple.

Are included columns counted against the 32 column and 2712 byte index limits? I did not see either explicitly mentioned in the discussion or the documentation. I only ask because in SQL Server the limits are different for include columns.

1. syntax - on 2016-08-14, Andrey Borodin wrote "I think MS SQL syntax INCLUDE instead of INCLUDING would be better". I would go further than that. This feature is already supported by 2 of the top 5 SQL databases and they both use INCLUDE. Using different syntax because of an internal implementation detail seems short sighted.

2. The patch does not seem to apply cleanly anymore.

> git checkout master
Already on 'master'
> git pull
From http://git.postgresql.org/git/postgresql
d49cc58..06f5fd2 master -> origin/master
Updating d49cc58..06f5fd2
Fast-forward
src/include/port.h | 2 +-
src/port/dirmod.c | 4 ++--
2 files changed, 3 insertions(+), 3 deletions(-)
> patch -pl < including_columns_9.7_v5.patch
patching file contrib/dblink/dblink.c
...
patching file src/backend/parser/gram.y
...
Hunk #2 FAILED at 375.
...
1 out of 12 hunks FAILED -- saving rejects to file src/backend/parser/gram.y.rej
...
patching file src/bin/pg_dump/pg_dump.c
...
Hunk #8 FAILED at 6399.
Hunk #9 FAILED at 6426.
...
2 out of 13 hunks FAILED -- saving rejects to file src/bin/pg_dump/pg_dump.c.rej
...

3. After "fixing" compilation errors (best guess based on similar change in other location), "make check" failed.

> make check
...
parallel group (3 tests): index_including create_view create_index
create_index ... ok
create_view ... ok
index_including ... FAILED
...

Looking at regression.diffs, it looks like the output format of \d tbl changed (lines 288,300) from the expected "Column | Type | Modifiers" to "Column | Type | Collation | Nullable | Default".

4. documentation - minor items (these are not actual diffs)
create_index.sgml
- [ INCLUDING ( <replaceable class="parameter">column_name</replaceable> )]
+ [ INCLUDING ( <replaceable class="parameter">column_name</replaceable> [, ...] )]

An optional <literal>INCLUDING</> clause allows a list of columns to be
- specified which will be included in the index, in the non-key portion of the index.
+ specified which will be included in the non-key portion of the index.

The whole paragraph on INCLUDING does not include many of the points raised in the feature discussions.

create_table.sgml (for both UNIQUE and PRIMARY KEY constraints) (similar change could be made to nbtree/README)
- Optional clause <literal>INCLUDING</literal> allows to add into the index
- a portion of columns on which the uniqueness is not enforced upon.
- Note, that althogh constraint is not enforced on included columns, it still
- depends on them. Consequently, some operations on these columns (e.g. <literal>DROP COLUMN</literal>)
- can cause cascade constraint and index deletion.

+ An optional <literal>INCLUDING</literal> clause allows a list of columns
+ to be specified which will be included in the non-key portion of the index.
+ Although uniqueness is not enforced on the included columns, the constraint
+ still depends on them. Consequently, some operations on the included columns
+ (e.g. <literal>DROP COLUMN</literal>) can cause cascading constraint and index deletion.

indexcmds.c
- * are key columns, and which are just part of the INCLUDING list by check
+ * are key columns, and which are just part of the INCLUDING list by checking

ruleutils.c
- * meaningless there, so do not include them into the message.
+ * meaningless there, so do not include them in the message.

pg_dump.c (does "if (fout->remoteVersion >= 90600)" also need to change?)
- * In 9.6 we add INCLUDING columns functionality
- * that requires new fields to be added.
+ * In 10.0 we added INCLUDING columns functionality
+ * that required new fields to be added.

5. coding
parse_utilcmd.c
@@ -1334,6 +1334,38 @@ ...
The loop is handling included columns separately.
The loop adds the collation name for each included column if it is not the default.

Q: Given that the create index/create constraint syntax does not allow a collation to be specified for included columns, how can you ever have a non-default collation?

@@ -1776,6 +1816,7 @@
The comment here says "NOTE that exclusion constraints don't support included nonkey attributes". However, the paragraph on INCLUDING in create_index.sgml says "It's the same for the other constraints (PRIMARY KEY and EXCLUDE)".