Hi,
On 2018-11-20 11:25:22 -0500, Stephen Frost wrote:
> Greetings,
>
> * Robert Haas ([email protected]) wrote:
> > On Sun, Oct 14, 2018 at 6:35 PM Tom Lane <[email protected]> wrote:
> > > Andres Freund <[email protected]> writes:
> > > > Does anybody have engineering / architecture level comments about this
> > > > proposal?
> > >
> > > FWIW, I'm -1 on making OIDs be not-magic for SELECT purposes. Yeah, it's
> > > a wart we wouldn't have if we designed the system today, but the wart is
> > > thirty years old. I think changing that will break so many catalog
> > > queries that we'll have the villagers on the doorstep. Most of the other
> > > things you're suggesting here could be done easily without making that
> > > change.
> > >
> > > Possibly we could make them not-magic from the storage standpoint (ie
> > > they're regular columns) but have a pg_attribute flag that says not
> > > to include them in "SELECT *" expansion.
> >
> > I think such a flag would be a good idea; it seems to have other uses.
> > As Andres also noted, Kevin was quite interested in having a
> > hidden-by-default COUNT column to assist with materialized view
> > maintenance, and presumably this could also be used for that purpose.
>
> Yeah, I like the idea of having this column too, it'd also be useful for
> people who are trying to use column-level privileges.
FWIW, leaving grammar bikeshedding aside, I don't think this is
particularly hard. There certainly are a few corner cases I've not
touched here, but the attached implements the basic features for hidden
columns.
postgres[14805][1]=# CREATE TABLE blarg(id serial, data text, annotation text);
postgres[14805][1]=# INSERT INTO blarg (data, annotation) VALUES ('42', 'this
is THE question');
postgres[14805][1]=# SELECT * FROM blarg;
┌────┬──────┬──────────────────────┐
│ id │ data │ annotation │
├────┼──────┼──────────────────────┤
│ 1 │ 42 │ this is THE question │
└────┴──────┴──────────────────────┘
(1 row)
postgres[14805][1]=# SELECT s.* FROM (SELECT * FROM blarg) s;
┌────┬──────┬──────────────────────┐
│ id │ data │ annotation │
├────┼──────┼──────────────────────┤
│ 1 │ 42 │ this is THE question │
└────┴──────┴──────────────────────┘
(1 row)
postgres[14805][1]=# SELECT s.* FROM (SELECT blarg FROM blarg) s;
┌───────────────────────────────┐
│ blarg │
├───────────────────────────────┤
│ (1,42,"this is THE question") │
└───────────────────────────────┘
(1 row)
postgres[14805][1]=# SELECT s.annotation FROM (SELECT * FROM blarg) s;
┌──────────────────────┐
│ annotation │
├──────────────────────┤
│ this is THE question │
└──────────────────────┘
(1 row)
postgres[14805][1]=# SELECT (s.blarg).annotation FROM (SELECT blarg FROM blarg)
s;
┌──────────────────────┐
│ annotation │
├──────────────────────┤
│ this is THE question │
└──────────────────────┘
(1 row)
-- update column to be hidden
postgres[14805][1]=# UPDATE pg_attribute SET attishidden = true WHERE attrelid
= 'blarg'::regclass AND attname = 'annotation';
postgres[14805][1]=# SELECT * FROM blarg;
┌────┬──────┐
│ id │ data │
├────┼──────┤
│ 1 │ 42 │
└────┴──────┘
(1 row)
postgres[14805][1]=# SELECT s.* FROM (SELECT * FROM blarg) s;
┌────┬──────┐
│ id │ data │
├────┼──────┤
│ 1 │ 42 │
└────┴──────┘
(1 row)
postgres[14805][1]=# SELECT s.blarg FROM (SELECT blarg FROM blarg) s;
┌───────────────────────────────┐
│ blarg │
├───────────────────────────────┤
│ (1,42,"this is THE question") │
└───────────────────────────────┘
(1 row)
postgres[14805][1]=# SELECT s.annotation FROM (SELECT * FROM blarg) s;
ERROR: 42703: column s.annotation does not exist
LINE 1: SELECT s.annotation FROM (SELECT * FROM blarg) s;
^
LOCATION: errorMissingColumn, parse_relation.c:3319
postgres[14805][1]=# SELECT (s.blarg).annotation FROM (SELECT blarg FROM blarg)
s;
┌──────────────────────┐
│ annotation │
├──────────────────────┤
│ this is THE question │
└──────────────────────┘
(1 row)
postgres[14805][1]=# SELECT (s.blarg).* FROM (SELECT blarg FROM blarg) s;
┌────┬──────┐
│ id │ data │
├────┼──────┤
│ 1 │ 42 │
└────┴──────┘
(1 row)
It's debatable if a wholerow-var select (without *, i.e the s.blarg case
above)) ought to include the hidden column, but to me that intuitively
makes sense. Otherwise you couldn't select it after a cast and such.
Greetings,
Andres Freund
diff --git a/src/backend/access/common/tupdesc.c b/src/backend/access/common/tupdesc.c
index 5354a04639b..78934c11de1 100644
--- a/src/backend/access/common/tupdesc.c
+++ b/src/backend/access/common/tupdesc.c
@@ -461,6 +461,8 @@ equalTupleDescs(TupleDesc tupdesc1, TupleDesc tupdesc2)
return false;
if (attr1->attislocal != attr2->attislocal)
return false;
+ if (attr1->attishidden != attr2->attishidden)
+ return false;
if (attr1->attinhcount != attr2->attinhcount)
return false;
if (attr1->attcollation != attr2->attcollation)
@@ -641,6 +643,7 @@ TupleDescInitEntry(TupleDesc desc,
att->attidentity = '\0';
att->attisdropped = false;
att->attislocal = true;
+ att->attishidden = false;
att->attinhcount = 0;
/* attacl, attoptions and attfdwoptions are not present in tupledescs */
@@ -700,6 +703,7 @@ TupleDescInitBuiltinEntry(TupleDesc desc,
att->attidentity = '\0';
att->attisdropped = false;
att->attislocal = true;
+ att->attishidden = false;
att->attinhcount = 0;
/* attacl, attoptions and attfdwoptions are not present in tupledescs */
@@ -791,6 +795,7 @@ BuildDescForRelation(List *schema)
int32 atttypmod;
Oid attcollation;
int attdim;
+ bool attishidden;
/*
* allocate a new tuple descriptor
@@ -843,6 +848,7 @@ BuildDescForRelation(List *schema)
att->attnotnull = entry->is_not_null;
has_not_null |= entry->is_not_null;
att->attislocal = entry->is_local;
+ att->attishidden = false;
att->attinhcount = entry->inhcount;
}
diff --git a/src/backend/bootstrap/bootstrap.c b/src/backend/bootstrap/bootstrap.c
index 8e2cf7df562..acad35de08c 100644
--- a/src/backend/bootstrap/bootstrap.c
+++ b/src/backend/bootstrap/bootstrap.c
@@ -748,6 +748,7 @@ DefineAttr(char *name, char *type, int attnum, int nullness)
attrtypes[attnum]->attcacheoff = -1;
attrtypes[attnum]->atttypmod = -1;
attrtypes[attnum]->attislocal = true;
+ attrtypes[attnum]->attishidden = false;
if (nullness == BOOTCOL_NULL_FORCE_NOT_NULL)
{
diff --git a/src/backend/parser/parse_relation.c b/src/backend/parser/parse_relation.c
index 378cbcbf79e..3c6875e0e0d 100644
--- a/src/backend/parser/parse_relation.c
+++ b/src/backend/parser/parse_relation.c
@@ -2596,6 +2596,13 @@ expandTupleDesc(TupleDesc tupdesc, Alias *eref, int count, int offset,
continue;
}
+ /*
+ * Skip over hidden fields, unless reproducing a closely matching
+ * entry (FIXME: check callers whether that's sensible).
+ */
+ if (attr->attishidden && !include_dropped)
+ continue;
+
if (colnames)
{
char *label;
diff --git a/src/backend/parser/parse_target.c b/src/backend/parser/parse_target.c
index b8702d914dc..0e27cd6b100 100644
--- a/src/backend/parser/parse_target.c
+++ b/src/backend/parser/parse_target.c
@@ -1434,7 +1434,7 @@ ExpandRowReference(ParseState *pstate, Node *expr,
Form_pg_attribute att = TupleDescAttr(tupleDesc, i);
FieldSelect *fselect;
- if (att->attisdropped)
+ if (att->attisdropped || att->attishidden)
continue;
fselect = makeNode(FieldSelect);
diff --git a/src/include/catalog/pg_attribute.h b/src/include/catalog/pg_attribute.h
index 1bac59bf26e..dbad2ebf5be 100644
--- a/src/include/catalog/pg_attribute.h
+++ b/src/include/catalog/pg_attribute.h
@@ -154,6 +154,9 @@ CATALOG(pg_attribute,1249,AttributeRelationId) BKI_BOOTSTRAP BKI_ROWTYPE_OID(75,
*/
bool attislocal BKI_DEFAULT(t);
+ /* column included in SELECT * expansion */
+ bool attishidden BKI_DEFAULT(f);
+
/* Number of times inherited from direct parent relation(s) */
int32 attinhcount BKI_DEFAULT(0);
diff --git a/src/include/catalog/pg_class.dat b/src/include/catalog/pg_class.dat
index 5a884a852b5..e6d5e2cdda9 100644
--- a/src/include/catalog/pg_class.dat
+++ b/src/include/catalog/pg_class.dat
@@ -36,7 +36,7 @@
reloftype => '0', relowner => 'PGUID', relam => '0', relfilenode => '0',
reltablespace => '0', relpages => '0', reltuples => '0', relallvisible => '0',
reltoastrelid => '0', relhasindex => 'f', relisshared => 'f',
- relpersistence => 'p', relkind => 'r', relnatts => '24', relchecks => '0',
+ relpersistence => 'p', relkind => 'r', relnatts => '25', relchecks => '0',
relhasrules => 'f', relhastriggers => 'f', relhassubclass => 'f',
relrowsecurity => 'f', relforcerowsecurity => 'f', relispopulated => 't',
relreplident => 'n', relispartition => 'f', relrewrite => '0',
