Thread: Proposal: move column defaults into pg_attribute along with attacl

I had a thought while looking over the column-level privileges patch
that Stephen Frost is working on.  To wit, that the only reason that
column default expressions are stored in a separate catalog pg_attrdef
is the historical assumption in some parts of the code that pg_attribute
rows are fixed-width and all-not-null.  This assumption is destroyed
by adding an attacl column, and so the patch has already done the
legwork to get rid of the assumption.  Given that, it would be a lot
cleaner and more efficient to get rid of pg_attrdef and store column
default expressions in a new, possibly-null column
pg_attribute.attdefault.

For backwards compatibility for clients, we could create a system view
replacing pg_attrdef, but the backend wouldn't use it any more.  Also,
although the atthasdef column is redundant with checking for non-null
attdefault, we should keep it: not only for compatibility, but because
it would be accessible in the fixed-width subset of pg_attribute rows
that will be kept in tuple descriptors, and so it could save a syscache
lookup in some places.

If that idea seems sane to people, what I would like to do is grab the
parts of the column-level privileges patch that relate to allowing
pg_attribute to contain null columns, and apply a patch that gets rid of
pg_attrdef and adds two columns attdefault and attacl to pg_attribute.
For the moment attacl would remain unused and always null.  This would
eliminate a fair amount of the nuisance diffs that Stephen is currently
carrying and allow him to focus on the mechanics of doing something
useful with per-column ACLs.  Adding both columns at the same time
should eliminate most of the merge problem he'd otherwise have from
needing to touch pg_attribute.h and pg_class.h again.

A possible objection to this plan is that if the column-level privileges
patch doesn't get in, then we're left with a useless column in
pg_attribute.  But an always-null column doesn't cost much of anything,
and we know that sooner or later we will support per-column ACLs:
they are clearly useful as well as required by spec.  So any
inefficiency would only be transient anyway.

Thoughts, objections?
        regards, tom lane

Tom Lane wrote:

> 
> A possible objection to this plan is that if the column-level privileges
> patch doesn't get in, then we're left with a useless column in
> pg_attribute.  But an always-null column doesn't cost much of anything,
> and we know that sooner or later we will support per-column ACLs:
> they are clearly useful as well as required by spec.  So any
> inefficiency would only be transient anyway.

Right. I don't see this objection holding much water as column privs are 
something that many in the community would like to see. If Stephen's 
patch doesn't get in, it is likely it would (or a derivative there of) 
within the 8.5 release cycle. If anything it just provides a stepping 
stone. I see nothing wrong with that.

> 
> Thoughts, objections?
> 

+1

Sincerely,

Joshua D. Drake

"Joshua D. Drake" <jd@commandprompt.com> writes:
> Tom Lane wrote:
>> A possible objection to this plan is that if the column-level privileges
>> patch doesn't get in, then we're left with a useless column in
>> pg_attribute.  But an always-null column doesn't cost much of anything,
>> and we know that sooner or later we will support per-column ACLs:
>> they are clearly useful as well as required by spec.  So any
>> inefficiency would only be transient anyway.

> Right. I don't see this objection holding much water as column privs are 
> something that many in the community would like to see. If Stephen's 
> patch doesn't get in, it is likely it would (or a derivative there of) 
> within the 8.5 release cycle. If anything it just provides a stepping 
> stone. I see nothing wrong with that.

Yah.  However, I started to look at doing this and immediately hit a
stumbling block: we need a representation in pg_depend for a column's
default expression (as distinct from the column itself).  Currently
this consists of classid = OID of pg_attrdef, objid = OID of the
default's row in pg_attrdef; both of which would disappear if we
get rid of pg_attrdef as an actual catalog.

I can think of a way around that: represent a default expression using
classid = OID of pg_attribute, objid = OID of table, objsubid = column
attnum.  This is distinct from the column itself, which is represented
with classid = OID of pg_class.  It seems pretty ugly and potentially
confusing though.  Also there would be a compatibility issue for clients
that examine pg_depend.  Is it ugly enough to scuttle the whole concept
of merging pg_attrdef into pg_attribute?
        regards, tom lane

pgadmin has some umm, interesting queries over pg_depends. It sounds
like this change could complicate those. I doubt it's an
insurmountable problem of course.

On 9/21/08, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> "Joshua D. Drake" <jd@commandprompt.com> writes:
>> Tom Lane wrote:
>>> A possible objection to this plan is that if the column-level privileges
>>> patch doesn't get in, then we're left with a useless column in
>>> pg_attribute.  But an always-null column doesn't cost much of anything,
>>> and we know that sooner or later we will support per-column ACLs:
>>> they are clearly useful as well as required by spec.  So any
>>> inefficiency would only be transient anyway.
>
>> Right. I don't see this objection holding much water as column privs are
>> something that many in the community would like to see. If Stephen's
>> patch doesn't get in, it is likely it would (or a derivative there of)
>> within the 8.5 release cycle. If anything it just provides a stepping
>> stone. I see nothing wrong with that.
>
> Yah.  However, I started to look at doing this and immediately hit a
> stumbling block: we need a representation in pg_depend for a column's
> default expression (as distinct from the column itself).  Currently
> this consists of classid = OID of pg_attrdef, objid = OID of the
> default's row in pg_attrdef; both of which would disappear if we
> get rid of pg_attrdef as an actual catalog.
>
> I can think of a way around that: represent a default expression using
> classid = OID of pg_attribute, objid = OID of table, objsubid = column
> attnum.  This is distinct from the column itself, which is represented
> with classid = OID of pg_class.  It seems pretty ugly and potentially
> confusing though.  Also there would be a compatibility issue for clients
> that examine pg_depend.  Is it ugly enough to scuttle the whole concept
> of merging pg_attrdef into pg_attribute?
>
>             regards, tom lane
>
> --
> Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
> To make changes to your subscription:
> http://www.postgresql.org/mailpref/pgsql-hackers
>


-- 
Dave Page
EnterpriseDB UK:   http://www.enterprisedb.com

dpage@pgadmin.org writes:
> pgadmin has some umm, interesting queries over pg_depends. It sounds
> like this change could complicate those. I doubt it's an
> insurmountable problem of course.

Yeah.  But the only real point of the change is cleanliness, and if it's
injecting ugliness into clients then that argument loses a lot of its
force.  I looked at pg_dump a bit and it definitely would get uglier
--- not really more complicated, but defaults would get handled a bit
differently from everything else.  Seems like a fertile source of bugs.
So maybe we'd better forget the idea :-(
        regards, tom lane

"Alex Hunsaker" <badalex@gmail.com> writes:
> Hrm, I thought if anything we wanted to put them in pg_constraints (at
> least inherited ones).  Now maybe I have defaults confused with NOT
> NULLs... But don't we want to be able to give defaults names and and
> such?

No, I think you're thinking of NOT NULL.  There's no reason to have
names for defaults that I can see.
        regards, tom lane

On Sun, Sep 21, 2008 at 11:09 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> A possible objection to this plan is that if the column-level privileges
> patch doesn't get in, then we're left with a useless column in
> pg_attribute.  But an always-null column doesn't cost much of anything,
> and we know that sooner or later we will support per-column ACLs:
> they are clearly useful as well as required by spec.  So any
> inefficiency would only be transient anyway.
>
> Thoughts, objections?

Hrm, I thought if anything we wanted to put them in pg_constraints (at
least inherited ones).  Now maybe I have defaults confused with NOT
NULLs... But don't we want to be able to give defaults names and and
such?

* Tom Lane (tgl@sss.pgh.pa.us) wrote:
> I can think of a way around that: represent a default expression using
> classid = OID of pg_attribute, objid = OID of table, objsubid = column
> attnum.  This is distinct from the column itself, which is represented
> with classid = OID of pg_class.  It seems pretty ugly and potentially
> confusing though.  Also there would be a compatibility issue for clients
> that examine pg_depend.  Is it ugly enough to scuttle the whole concept
> of merging pg_attrdef into pg_attribute?

Being able to handle a setup like that (though in pg_shdepend) might be
necessary anyway, depending on the approach we're happiest with for
column-level acl dependencies.  Right now I've avoided it by just going
through all of the columns and the table level grantors/grantees and
listing them all when updating the dependencies.  That keeps the
dependency system simple but complicates other things for it.  I posed a
question previously about how people felt and don't recall there being
any response as yet.

Certainly, if we move to objid=table OID, objsubid=column attnum in
pg_shdepend, it strikes me that we should do the same in pg_depend where
appropriate, otherwise it'll just be a confusing inconsistancy.

Honestly, I really disliked the code which assumed pg_attribute had no
NULLable/toastable columns and used what seemed like pretty gruesome
hacks to create pg_attribute structures.  I'd also really like to get
away from things which approached pg_attribute in that way, such as
pg_attrdef.  If we were to accept the pg_attrdef approach, why aren't we
doing a pg_attracl table instead of adding a column to pg_attribute?
Thanks,
    Stephen

Stephen Frost <sfrost@snowman.net> writes:
> Honestly, I really disliked the code which assumed pg_attribute had no
> NULLable/toastable columns and used what seemed like pretty gruesome
> hacks to create pg_attribute structures.

Agreed, but that seems orthogonal to the point here, which is that a
column's default expression is a distinct object for dependency purposes
and so it needs its own ID.  An OID in the pg_attrdef catalog works
nicely for that; the alternatives I've thought of seem like kluges.

> If we were to accept the pg_attrdef approach, why aren't we
> doing a pg_attracl table instead of adding a column to pg_attribute?

That's actually not an unreasonable question.  If you were to do that
then you could attach OIDs to the attribute ACLs, which might be a nicer
representation in pg_shdepend than you were thinking of using.
        regards, tom lane

* Tom Lane (tgl@sss.pgh.pa.us) wrote:
> Stephen Frost <sfrost@snowman.net> writes:
> > If we were to accept the pg_attrdef approach, why aren't we
> > doing a pg_attracl table instead of adding a column to pg_attribute?
>
> That's actually not an unreasonable question.  If you were to do that
> then you could attach OIDs to the attribute ACLs, which might be a nicer
> representation in pg_shdepend than you were thinking of using.

What bugs me about this is that it comes across as poor database design-
both of these really are attributes of a column.  We're creating
seperate tables for each so we can induce a cleaner ID for them, which
just isn't the right approach imv.  This would also be another table to
go deal with when a column is removed, and a less-than-obvious place to
look for this information from the user's perspective.  It's also the
case that the items in these tables and the columns they're attached to
really are one-to-one, there's no many-to-one or one-to-many
relationship between them..

At the end of the day, this approach feels like more of a kludge to me
to keep the dependency system simple rather than making the dependency
system support the real-world system layout, which is that columns don't
have their own IDs.  Maybe we could approach this another way- what
about creating a new table which is pg_attrcolids that has both
pg_attrdef and pg_attracl rolled into it?  Then at least we're accepting
that we need a distinct ID for columns, but keeping them in one specific
place?  Is there a reason we would need a seperate ID for each?

It also strikes me to wonder about possible future support for
re-ordering columns, though I don't immediately see a way to use this as
a step towards supporting that.
Thanks,
    Stephen

Hi,

Tom Lane wrote:
> Yah.  However, I started to look at doing this and immediately hit a
> stumbling block: we need a representation in pg_depend for a column's
> default expression (as distinct from the column itself).

Just to understand the issue here: what's the reason for having an OID 
for the default value and possible another one for a ACLs, but none for 
the attribute itself?

Why don't we just have a unique OID for pg_attribute (i.e. drop the 
BKI_WITHOUT_OIDS of pg_attribute) and merge in the default values and ACLs?

Regards

Markus Wanner

Markus Wanner <markus@bluegap.ch> writes:
> Just to understand the issue here: what's the reason for having an OID 
> for the default value and possible another one for a ACLs, but none for 
> the attribute itself?

Well, as far as the dependency system goes this way is more convenient.
If pg_attribute entries had their own OIDs it would be fairly hard
to implement DROP TABLE except with an intermediate step of dropping
each of the columns one by one, because you'd pretty much have to have
explicit pg_depend entries linking each column to its table, and that
behavior is what you'd get from the dependency traversal.

So that's why we didn't add OIDs (back) to pg_attribute when we invented
the dependency system.

Default values would need their own OIDs, or at least some distinct
representation in pg_depend, in any case.
        regards, tom lane

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Hi,

Tom Lane wrote:
> Well, as far as the dependency system goes this way is more convenient.
> If pg_attribute entries had their own OIDs it would be fairly hard
> to implement DROP TABLE except with an intermediate step of dropping
> each of the columns one by one, because you'd pretty much have to have
> explicit pg_depend entries linking each column to its table, and that
> behavior is what you'd get from the dependency traversal.

So, we do not want attributes to be dependent on the relation, because
that complicates DROP TABLE. On the other hand, we want defaults (and
possibly ACLs) to be dependent, so that the dependency system cleans
them up when dropping the table. It that correct?

ISTM that we should at least combine defaults and ACLs then, as proposed
by Stephen.

Regards

Markus Wanner

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.9 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org

iEUEARECAAYFAkjY7K8ACgkQsPwMloDjyo/UGACeI2YA2bAV+NAt3NXNCP641NXP
phAAmPuQRUxkNRQOsVwQAKLNlayuPg4=
=dwXa
-----END PGP SIGNATURE-----

Markus Wanner <markus@bluegap.ch> writes:
> ISTM that we should at least combine defaults and ACLs then, as proposed
> by Stephen.

Huh?  Maybe I missed something, but I didn't think that was suggested
anywhere.
        regards, tom lane

Tom,

* Tom Lane (tgl@sss.pgh.pa.us) wrote:
> Markus Wanner <markus@bluegap.ch> writes:
> > ISTM that we should at least combine defaults and ACLs then, as proposed
> > by Stephen.
>
> Huh?  Maybe I missed something, but I didn't think that was suggested
> anywhere.

I had suggested a single table, with an OID, which would house anything
that needed a seperate OID for columns (defaults and ACLs currently) in
20080922024129.GD16005@tamriel.snowman.net.  It's not a completely
thought-through solution, just something that struck me as a more
general way of handling these situations (assuming we have more in the
future and don't want to give each one its own table).  If putting them
together implies we have to complicate things to add some way to
seperate them then it might not be worth it.  Having a seperate table
for each means we can use the table's OID to seperate them though.  I
still dislike this possible continued growth of the catalogs.
Thanks,
    Stephen

Stephen Frost <sfrost@snowman.net> writes:
>> Huh?  Maybe I missed something, but I didn't think that was suggested
>> anywhere.

> I had suggested a single table, with an OID, which would house anything
> that needed a seperate OID for columns (defaults and ACLs currently) in
> 20080922024129.GD16005@tamriel.snowman.net.

[ squint... ]  But the default needs its *own* OID, because it is a
distinct entity for dependency purposes.  I think you're just confusing
two separate issues there.  If we did drop the "object/subobject" model
and just give attributes their own OIDs, we'd still need to give a
separate OID to each default; but ACLs wouldn't have their own OIDs.

The DROP issue I was complaining about could certainly be solved with
some uglification of the dependency-chasing code, so as far as the
backend is concerned it might be about a wash.  But there is enough
client-side code out there that roots around in pg_depend for
information we don't store any other way that I'm pretty hesitant to
change the pg_depend representation now.  I think adding a subobject
column to pg_shdepend is probably the best answer --- we only didn't
do that to start with because we thought it wasn't needed.
        regards, tom lane

Tom Lane wrote:

> I think adding a subobject column to pg_shdepend is probably the best
> answer --- we only didn't do that to start with because we thought it
> wasn't needed.

Yep.  I did consider adding it, but there was no use for it at the time
so I just left it out.  It's not like it's very difficult to add it at
this point.

-- 
Alvaro Herrera                                http://www.CommandPrompt.com/
The PostgreSQL Company - Command Prompt, Inc.

Alvaro Herrera wrote:
> Tom Lane wrote:
> 
>> I think adding a subobject column to pg_shdepend is probably the best
>> answer --- we only didn't do that to start with because we thought it
>> wasn't needed.
> 
> Yep.  I did consider adding it, but there was no use for it at the time
> so I just left it out.  It's not like it's very difficult to add it at
> this point.

Sorry, I lost track here and would like to know the recommended course
of action for ACLs. I see these issues:
* make pg_attribute store VARLENA and/or NOT NULL fields* dependency tracking of defaults (and ACLs)* where to place
ACLs(pg_attribute, merge with pg_attrdef or yet
 
another pg_attracl table)

So far I understood that merging pg_attrdef into pg_attribute is
unwanted due to complexity at DROP TABLE.

What does the subobject column for pg_shdepend buy us?

Clarification appreciated.

Regards

Markus Wanner

Markus,

* Markus Wanner (markus.wanner@programmfabrik.de) wrote:
> What does the subobject column for pg_shdepend buy us?

Tracking column-level ACL dependencies rather than having those
dependencies only be at the table-level.  This complicates
pg_shdepend some, but simplifies the dependency handling in the
ACL area and in handling table/column drops.

I'm still not a fan of having column-level deps handled
differently between pg_shdepend and pg_depend, but that's not
something which has to be addressed directly by the column-level
privs patch.  Perhaps once it's done I'll do a proof-of-concept
for removing pg_attdef.
Thanks,
    Stephen

Hi,

Stephen Frost wrote:
> * Markus Wanner (markus.wanner@programmfabrik.de) wrote:
>> What does the subobject column for pg_shdepend buy us?
> 
> Tracking column-level ACL dependencies rather than having those
> dependencies only be at the table-level.  This complicates
> pg_shdepend some, but simplifies the dependency handling in the
> ACL area and in handling table/column drops.

With a separate table? Or as part of pg_attribute (which can handle NULL
and VARLENA attributes now?)

Regards

Markus Wanner

* Markus Wanner (markus@bluegap.ch) wrote:
> Stephen Frost wrote:
> > * Markus Wanner (markus.wanner@programmfabrik.de) wrote:
> >> What does the subobject column for pg_shdepend buy us?
> >
> > Tracking column-level ACL dependencies rather than having those
> > dependencies only be at the table-level.  This complicates
> > pg_shdepend some, but simplifies the dependency handling in the
> > ACL area and in handling table/column drops.
>
> With a separate table? Or as part of pg_attribute (which can handle NULL
> and VARLENA attributes now?)

As part of pg_attribute..  Having a seperate table would be an
alternative to adding a column to pg_shdepend.
Stephen

Hi,

Stephen Frost wrote:
> As part of pg_attribute..  Having a seperate table would be an
> alternative to adding a column to pg_shdepend.

Aha. Hm...

I thought tracking dependencies between tables and attributes
complicates DROP TABLE? Why doesn't that concern apply here?

And why do we keep the attributes defaults in their own table with their
own OID, instead of merging them into pg_attributes? (Or put another way
around: why do these need their own dependency tracking, while the ACLs
don't?)

Or do we just want to keep the column-level privileges patch simple here
and deferring other work to another patch?

Regards

Markus Wanner

Markus Wanner <markus.wanner@programmfabrik.de> writes:
> And why do we keep the attributes defaults in their own table with their
> own OID, instead of merging them into pg_attributes?

That has already been explained multiple times in this thread, but: the
default expression is a separate entity from the attribute itself, so
there needs to be some different representation for it in pg_depend.
Otherwise we couldn't handle the concept that dropping some entity
(like a function) forces discarding of the default, not the whole
column the default is attached to.

Now admittedly giving it its own OID and classid = pg_attrdef is
probably not the only way to do that.  But merging it into the
pg_attribute row leaves no obvious way to do it within the
object identity representation that's been chosen for pg_depend.

> (Or put another way
> around: why do these need their own dependency tracking, while the ACLs
> don't?)

pg_shdepend is already designed to track ACLs: an ACL dependency says
that "there's some privilege that this role has been granted on this
object".  So as long as you can identify the object you're okay, you
don't need a separate identity for the ACL.

> Or do we just want to keep the column-level privileges patch simple here
> and deferring other work to another patch?

Stephen was arm-waving about getting rid of pg_attrdef, but trying to
hold the column privileges patch hostage to that would be a serious
error.  It's an independent problem, so it ought to be addressed in
a separate patch; and it has no clear solution so it's not entirely
obvious that it can or should be done at all.
        regards, tom lane

Hi,

thank you for your patience in explaining. Rest assured that I've read
the relevant messages multiple times.

Tom Lane wrote:
> the
> default expression is a separate entity from the attribute itself,

That was the point I didn't understand...

> ..
> Otherwise we couldn't handle the concept that dropping some entity
> (like a function) forces discarding of the default, not the whole
> column the default is attached to.

..and that finally provided the missing piece for my puzzle: it's not
the dependency between the attribute and its default which matters here.
But the possible dependencies of the default (and not the attribute
itself) on other entities.

> Now admittedly giving it its own OID and classid = pg_attrdef is
> probably not the only way to do that.  But merging it into the
> pg_attribute row leaves no obvious way to do it within the
> object identity representation that's been chosen for pg_depend.

Understood, makes sense.

> pg_shdepend is already designed to track ACLs: an ACL dependency says
> that "there's some privilege that this role has been granted on this
> object".  So as long as you can identify the object you're okay, you
> don't need a separate identity for the ACL.

Sure.

> Stephen was arm-waving about getting rid of pg_attrdef, but trying to
> hold the column privileges patch hostage to that would be a serious
> error.  It's an independent problem, so it ought to be addressed in
> a separate patch; and it has no clear solution so it's not entirely
> obvious that it can or should be done at all.

Agreed.

Regards

Markus Wanner