Re: attoptions

PFA a revised attoptions patch. This may still be a little rough
around the edges, but I wanted to get it out there for feedback. This
would replace ALTER TABLE ... SET STATISTICS DISTINCT.

I am not very happy with ATPrepSetOptions(). I basically just
retained the logic from ATPrepSetDistinct(), but it doesn't really
make sense in this context. The idea that we want to support
attdistinct for system tables and index columns was based on a very
specific understanding of what that was going to do; for attoptions,
well, it might make sense for the options that we have now, but it
might not make sense for the next thing we want to add, and there's
not going to be any easy fix for that. Even as it stands, the
n_distinct_inherited option is supported for both table columns and
index columns, but it only actually does anything for table columns.

One possibility is to just give up on supporting attoptions for system
table columns and index columns and support them only for regular
table columns. That will lose the ability to override n_distinct for
expression indexes and system tables, but how much do we really care?

Another option is to just jettison this whole concept as a bad idea
and leave it the way it is.

There might be other reasonable options, too. Thoughts?

...Robert

On Sun, Jan 10, 2010 at 12:27, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
> I am not very happy with ATPrepSetOptions().  I basically just
> retained the logic from ATPrepSetDistinct(), but it doesn't really
> make sense in this context.  The idea that we want to support
> attdistinct for system tables and index columns was based on a very
> specific understanding of what that was going to do; for attoptions,
> well, it might make sense for the options that we have now, but it
> might not make sense for the next thing we want to add, and there's
> not going to be any easy fix for that.  Even as it stands, the
> n_distinct_inherited option is supported for both table columns and
> index columns, but it only actually does anything for table columns.

I say just do it in AT(Prep|Exec)SetOptions. We could extend struct
relopt_gen... but that seems overkill and hard to do without knowing
what else might be in attoptions. IMHO at this point its ok not to
worry about it util we have something we actually care about
restricting.

Comments on the patch below. Minus those Im happy with it.

in tablecmds.c:~3682 (ATExecAddColumn)
seems to be either missing a comment or missing the handling of
attoptions all together?

Any thoughts on how its now a float8 vs float4? Its nice how it
matches n_distinct in pg_stats now.

pg_dump.c:
You do '' AS attoptions in a few places, should that be NULL? Not
that it really matters in pg_dump...

I tested all the things you would expect (pg_dump included). The only
perhaps interesting thing is when creating or adding an inherited
table it does not pick up the parents attopts I think its debatable
if it should, but it seems kind of strange that given alter table
parent will give the child tables the appropriate attopts (of course
ONLY works as you expect)

My favorite error of the day :) :
ERROR: value -2 out of bounds for option "n_distinct_inherited"
DETAIL: Valid values are between "-1.000000" and
"179769313486231570814527423731704356798070567525844996598917476803157260780028538760589558632766878171540458953514382464234321326889464182768467546703537516986049910576551282076245490090389328944075868508455133942304583236903222948165808559332123348274797826204144723168738177180919299881250404026184124858368.000000".

See patch below on top of yours, it fixes some brainos:
*** a/src/backend/catalog/heap.c
--- b/src/backend/catalog/heap.c
***************
*** 161,167 **** static FormData_pg_attribute a6 = {
static FormData_pg_attribute a7 = {
0, {"tableoid"}, OIDOID, 0, sizeof(Oid),
TableOidAttributeNumber, 0, -1, -1,
! true, 'p', 'i', true, false, false, true, 0, {0}
};

static const Form_pg_attribute SysAtt[] = {&a1, &a2, &a3, &a4, &a5, &a6, &a7};
--- 161,167 ----
static FormData_pg_attribute a7 = {
0, {"tableoid"}, OIDOID, 0, sizeof(Oid),
TableOidAttributeNumber, 0, -1, -1,
! true, 'p', 'i', true, false, false, true, 0, {0}, {0}
};

static const Form_pg_attribute SysAtt[] = {&a1, &a2, &a3, &a4, &a5, &a6, &a7};
*** a/src/backend/commands/tablecmds.c
--- b/src/backend/commands/tablecmds.c
***************
*** 4218,4223 **** ATExecSetOptions(Relation rel, const char *colName,
Node *options,
--- 4218,4224 ----
newOptions = transformRelOptions(isnull ? (Datum) 0 : datum,
(List *) options, NULL, NULL, false,
isReset);
+ /* Validate new options */
(void) attribute_reloptions(newOptions, true);

/* Build new tuple. */
*** a/src/include/catalog/pg_attribute.h
--- b/src/include/catalog/pg_attribute.h
***************
*** 152,158 **** CATALOG(pg_attribute,1249) BKI_BOOTSTRAP
BKI_WITHOUT_OIDS BKI_ROWTYPE_OID(75) BK
aclitem attacl[1];

/* Column-level options */
! aclitem attoptions[1];
} FormData_pg_attribute;

/*
--- 152,158 ----
aclitem attacl[1];

/* Column-level options */
! text attoptions[1];
} FormData_pg_attribute;

/*

First, thanks for the review. Detailed comments/questions below.

On Fri, Jan 15, 2010 at 12:52 AM, Alex Hunsaker <badalex(at)gmail(dot)com> wrote:
> On Sun, Jan 10, 2010 at 12:27, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
>> I am not very happy with ATPrepSetOptions().  I basically just
>> retained the logic from ATPrepSetDistinct(), but it doesn't really
>> make sense in this context.  The idea that we want to support
>> attdistinct for system tables and index columns was based on a very
>> specific understanding of what that was going to do; for attoptions,
>> well, it might make sense for the options that we have now, but it
>> might not make sense for the next thing we want to add, and there's
>> not going to be any easy fix for that.  Even as it stands, the
>> n_distinct_inherited option is supported for both table columns and
>> index columns, but it only actually does anything for table columns.
>
> I say just do it in AT(Prep|Exec)SetOptions.  We could extend struct
> relopt_gen... but that seems overkill and hard to do without knowing
> what else might be in attoptions.  IMHO at this point its ok not to
> worry about it util we have something we actually care about
> restricting.

I'm sorry - do what in AT(Prep|Exec)SetOptions?

> Comments on the patch below.  Minus those Im happy with it.
>
> in tablecmds.c:~3682 (ATExecAddColumn)
>    seems to be either missing a comment or missing the handling of
> attoptions all together?

Comment.

> Any thoughts on how its now a float8 vs float4? Its nice how it
> matches n_distinct in pg_stats now.

Well, the original reason for using float4 was to avoid bloating
TupleDescs. That doesn't matter with this design, so might as well
splurge.

> pg_dump.c:
>    You do '' AS attoptions in a few places, should that be NULL? Not
> that it really matters in pg_dump...

I like it the way it is, YMMV.

> I tested all the things you would expect (pg_dump included).  The only
> perhaps interesting thing is when creating or adding an inherited
> table it does not pick up the parents attopts  I think its debatable
> if it should, but it seems kind of strange that given alter table
> parent will give the child tables the appropriate attopts (of course
> ONLY works as you expect)

I don't think it should - it's fairly nonsensical for the current
options, at least.

> My favorite error of the day :) :
> ERROR:  value -2 out of bounds for option "n_distinct_inherited"
> DETAIL:  Valid values are between "-1.000000" and
> "179769313486231570814527423731704356798070567525844996598917476803157260780028538760589558632766878171540458953514382464234321326889464182768467546703537516986049910576551282076245490090389328944075868508455133942304583236903222948165808559332123348274797826204144723168738177180919299881250404026184124858368.000000".

Yeah, I don't like that message very much, but I don't want to
reinvent the wheel just for this patch, so I think we're stuck with it
for now.

> See patch below on top of yours, it fixes some brainos:

Thanks, those look like good changes.

...Robert

On Sat, Jan 16, 2010 at 05:39, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
> First, thanks for the review.  Detailed comments/questions below.
>
> On Fri, Jan 15, 2010 at 12:52 AM, Alex Hunsaker <badalex(at)gmail(dot)com> wrote:
>> On Sun, Jan 10, 2010 at 12:27, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
>>> I am not very happy with ATPrepSetOptions().  I basically just
>>> retained the logic from ATPrepSetDistinct(), but it doesn't really
>>> make sense in this context.  The idea that we want to support
>>> attdistinct for system tables and index columns was based on a very
>>> specific understanding of what that was going to do; for attoptions,
>>> well, it might make sense for the options that we have now, but it
>>> might not make sense for the next thing we want to add, and there's
>>> not going to be any easy fix for that.  Even as it stands, the
>>> n_distinct_inherited option is supported for both table columns and
>>> index columns, but it only actually does anything for table columns.
>>
>> I say just do it in AT(Prep|Exec)SetOptions.  We could extend struct
>> relopt_gen... but that seems overkill and hard to do without knowing
>> what else might be in attoptions.  IMHO at this point its ok not to
>> worry about it util we have something we actually care about
>> restricting.
>
> I'm sorry - do what in AT(Prep|Exec)SetOptions?

Hrm lemme re-quote and try to slim it down a bit:

>>> ... The idea that we want to support
>>> attdistinct for system tables and index columns was based on a very
>>> specific understanding of what that was going to do; for attoptions,
>>> well, it might make sense for the options that we have now, but it
>>> might not make sense for the next thing we want to add, and there's
>>> not going to be any easy fix for that.

Basically I was agreeing and saying when we add something new lets
worry about it then. Clearer?

>> ... The only
>> perhaps interesting thing is when creating or adding an inherited
>> table it does not pick up the parents attopts ...
>
> I don't think it should - it's fairly nonsensical for the current
> options, at least.

No argument here. :)

On Sun, Jan 17, 2010 at 9:57 PM, Alex Hunsaker <badalex(at)gmail(dot)com> wrote:
>>>> ... The idea that we want to support
>>>> attdistinct for system tables and index columns was based on a very
>>>> specific understanding of what that was going to do; for attoptions,
>>>> well, it might make sense for the options that we have now, but it
>>>> might not make sense for the next thing we want to add, and there's
>>>> not going to be any easy fix for that.
>
> Basically I was agreeing and saying when we add something new lets
> worry about it then.  Clearer?

It's clear now, but I don't think I agree. On balance, I'm inclined
to just rip out the special case permissions checks that
AT_SetDistinct uses and just use ATSimplePermissionsRelationOrIndex()
instead. That will mean that users can't use ALTER TABLE ... ALTER
COLUMN ... SET STATISTICS DISTINCT for system tables, but I don't
think that's much of a loss, and it certainly seems cleaner than
hoping that any additional attoptions we add in the future will be
things that we don't mind having applied to system tables.

There's a further design issue here in that the reloptions code
currently contemplates at most 31 types of objects. That makes sense
if the object types are things like "table" or "GIN index", but it's
not going to work if we get too fine-grained. The "right" way to make
n_distinct apply to both table columns and index columns and
n_distinct_inherited only to table columns is probably to define two
different reloption kinds, but that's burning up our supply of
available bits a little more quickly than I feel comfortable with. So
I'm inclined to just let n_distinct_inherited be applied either place,
and if you happen to apply it to an index column it just won't affect
anything. We might want to refactor the reloptions API in the future
to allow this to be handled better, but I don't think we need or want
to do that for 8.5.

Does that make sense?

...Robert

On Tue, Jan 19, 2010 at 07:49, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
>That will mean that users can't use ALTER TABLE ... ALTER
> COLUMN ... SET STATISTICS DISTINCT for system tables, but I don't
> think that's much of a loss, and it certainly seems cleaner than
> hoping that any additional attoptions we add in the future will be
> things that we don't mind having applied to system tables.

I assumed there was a good reason to apply them to system tables. But
I admit I did not follow the original SET STATISTICS DISTINCT patch. [
looks ] Oh ok seeing that you were the original patch author you
probably have a good idea about the above, so ill defer :)

> There's a further design issue here in that the reloptions code
> currently contemplates at most 31 types of objects.  That makes sense
> if the object types are things like "table" or "GIN index", but it's
> not going to work if we get too fine-grained.  The "right" way to make
> n_distinct apply to both table columns and index columns and
> n_distinct_inherited only to table columns is probably to define two
> different reloption kinds, but that's burning up our supply of
> available bits a little more quickly than I feel comfortable with.  So
> I'm inclined to just let n_distinct_inherited be applied either place,
> and if you happen to apply it to an index column it just won't affect
> anything.  We might want to refactor the reloptions API in the future
> to allow this to be handled better, but I don't think we need or want
> to do that for 8.5.

Agreed.

Although ISTM we might be able to extend it just using two bits:
RELOPT_KIND_SYSTEM
RELOPT_KIND_INHERIT
#define RELOPT_KIND_INDEX RELOPT_KIND_BTREE|RELOPT_KIND_GIST|...

Abuse ? Maybe. The hardest part I think would be setting those flags
appropriately.

But yes, lets keep it simple for now.

On Fri, Jan 15, 2010 at 12:52 AM, Alex Hunsaker <badalex(at)gmail(dot)com> wrote:
> ***************
> *** 152,158 **** CATALOG(pg_attribute,1249) BKI_BOOTSTRAP
> BKI_WITHOUT_OIDS BKI_ROWTYPE_OID(75) BK
>        aclitem         attacl[1];
>
>        /* Column-level options */
> !       aclitem         attoptions[1];
>  } FormData_pg_attribute;
>
>  /*
> --- 152,158 ----
>        aclitem         attacl[1];
>
>        /* Column-level options */
> !       text            attoptions[1];
>  } FormData_pg_attribute;
>
>  /*

Unfortunately this change (which is obviously correct and necessary)
breaks the build on src/backend/catalog/heap.c with:

heap.c:122: error: missing braces around initializer
heap.c:122: error: (near initialization for ‘a1.attoptions[0]’)

...repeated of the 7 hard-coded descriptors. Sadly I'm not quite sure
what to use instead. I can't find any examples of static initializers
for a varlena (which text is). However, I think that it doesn't
actually matter how that gets initialized, because I think only the
fixed-size portion is ever examined, so perhaps I can just leave off
the attoptions and attacl initializers altogether.

Whatever we decide about this, genbki.pl also needs the same treatment.

...Robert

On Tue, Jan 19, 2010 at 13:06, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
> On Fri, Jan 15, 2010 at 12:52 AM, Alex Hunsaker <badalex(at)gmail(dot)com> wrote:
>> ***************
>> *** 152,158 **** CATALOG(pg_attribute,1249) BKI_BOOTSTRAP
>> BKI_WITHOUT_OIDS BKI_ROWTYPE_OID(75) BK
>>        aclitem         attacl[1];
>>
>>        /* Column-level options */
>> !       aclitem         attoptions[1];
>>  } FormData_pg_attribute;
>>
>>  /*
>> --- 152,158 ----
>>        aclitem         attacl[1];
>>
>>        /* Column-level options */
>> !       text            attoptions[1];
>>  } FormData_pg_attribute;
>>
>>  /*
>
> Unfortunately this change (which is obviously correct and necessary)
> breaks the build on src/backend/catalog/heap.c with:
>
> heap.c:122: error: missing braces around initializer
> heap.c:122: error: (near initialization for ‘a1.attoptions[0]’)

Huh. I must have not done an --enable-depend build :( Even so after
a make clean I just get warnings...
This is with gcc version 4.4.2 20091208 (prerelease) (GCC)

Well for grins I tried changing it to the obvious {{{0}, {0}}} as the
places that seem to use it mark it as "null" anyway (see heap.c
InsertPgAttributeTupe ~525).

But then relcache.c gets more warnings:
relcache.c:87: warning: missing braces around initialize
relcache.c:87: warning: (near initialization for
‘Desc_pg_class[0].attoptions[0]’)
relcache.c:88: warning: missing braces around initialize
...

Which comes from genbki.pl. Seems we are stuck with:
1) the non portable
$row->{attoptions} = q|.attoptions = { {0}, {0} }|;

2) just dont initialize as nothing seems need it (*note* I have not
looked very hard)
$row->{attoptions} = q||;

Thoughts? Id rather not have this useful patch fall on the floor
because of a stupid limitation like this :)

On Tue, Jan 19, 2010 at 23:02, Alex Hunsaker <badalex(at)gmail(dot)com> wrote:
> On Tue, Jan 19, 2010 at 13:06, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
>> On Fri, Jan 15, 2010 at 12:52 AM, Alex Hunsaker <badalex(at)gmail(dot)com> wrote:
>>> !       text            attoptions[1];

>> Unfortunately this change (which is obviously correct and necessary)
>> breaks the build on src/backend/catalog/heap.c with:

> 2) just dont initialize as nothing seems need it (*note* I have not
> looked very hard)
> $row->{attoptions}  = q||;

This should be OK because it will be zeroed anyway because its in the
.bss right?

On Tue, Jan 19, 2010 at 10:51 AM, Alex Hunsaker <badalex(at)gmail(dot)com> wrote:
> But yes, lets keep it simple for now.

OK. Updated patch attached. Changes:

- Incorporate your previous review patch.
- Omit attacl and attoptions from hardcoded relation descriptor
initializers so the whole thing still builds.- Use
ATSimplePermissionsRelationOrIndex instead of custom permissions
logic.
- Remove recursion, per further thought about a comment in your
original review - I agree that the recursive behavior is weird.
- Remove a stray reference to SET STATISTICS DISTINCT in the documentation.
- Bug fix: this wasn't working at all for index-expression columns.

...Robert

On Wed, Jan 20, 2010 at 19:51, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
> On Tue, Jan 19, 2010 at 10:51 AM, Alex Hunsaker <badalex(at)gmail(dot)com> wrote:
>> But yes, lets keep it simple for now.
>
> OK.  Updated patch attached.  Changes:
>
> - Incorporate your previous review patch.
> - Omit attacl and attoptions from hardcoded relation descriptor
> initializers so the whole thing still builds.

Seems to me a comment about the above might be nice. Something like
/* Things after here are should always be default null */ in
pg_attribute.h ?

Other than the below it looks good to me.

*** a/src/backend/commands/tablecmds.c
--- b/src/backend/commands/tablecmds.c
***************
*** 2426,2437 **** ATPrepCmd(List **wqueue, Relation rel, AlterTableCmd *cmd,
case AT_SetOptions: /* ALTER COLUMN SET ( options ) */
case AT_ResetOptions: /* ALTER COLUMN RESET ( options ) */
ATSimplePermissionsRelationOrIndex(rel);
! ATSimpleRecursion(wqueue, rel, cmd, recurse);
pass = AT_PASS_COL_ATTRS;
break;
case AT_SetStorage: /* ALTER COLUMN SET STORAGE */
ATSimplePermissions(rel, false);
! /* This command never recurses */
/* No command-specific prep needed */
pass = AT_PASS_COL_ATTRS;
break;
--- 2426,2437 ----
case AT_SetOptions: /* ALTER COLUMN SET ( options ) */
case AT_ResetOptions: /* ALTER COLUMN RESET ( options ) */
ATSimplePermissionsRelationOrIndex(rel);
! /* This command never recurses */
pass = AT_PASS_COL_ATTRS;
break;
case AT_SetStorage: /* ALTER COLUMN SET STORAGE */
ATSimplePermissions(rel, false);
! ATSimpleRecursion(wqueue, rel, cmd, recurse);
/* No command-specific prep needed */
pass = AT_PASS_COL_ATTRS;
break;

On Thu, Jan 21, 2010 at 12:57 AM, Alex Hunsaker <badalex(at)gmail(dot)com> wrote:
> Seems to me a comment about the above might be nice.  Something like
> /* Things after here are should always be default null */ in
> pg_attribute.h ?

Well... that wouldn't actually be a correct summary, so no. The point
is that variable-length fields are not used in tuple descriptors.
I'll add a comment about that.

> Other than the below it looks good to me.

Oh, dear. OK, I'll fix that.

...Robert

On Thu, Jan 21, 2010 at 07:30, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
> On Thu, Jan 21, 2010 at 12:57 AM, Alex Hunsaker <badalex(at)gmail(dot)com> wrote:
>> Seems to me a comment about the above might be nice.  Something like
>> /* Things after here are should always be default null */ in
>> pg_attribute.h ?
>
> Well... that wouldn't actually be a correct summary, so no.  The point
> is that variable-length fields are not used in tuple descriptors.
> I'll add a comment about that.

Yes that sounds much better, I was struggling to find the words. What
I was trying to express was something along the lines of you cant have
a non null value after these because you cant statically initialize
them in genbki.

On Thu, Jan 21, 2010 at 10:24 AM, Alex Hunsaker <badalex(at)gmail(dot)com> wrote:
> On Thu, Jan 21, 2010 at 07:30, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
>> On Thu, Jan 21, 2010 at 12:57 AM, Alex Hunsaker <badalex(at)gmail(dot)com> wrote:
>>> Seems to me a comment about the above might be nice.  Something like
>>> /* Things after here are should always be default null */ in
>>> pg_attribute.h ?
>>
>> Well... that wouldn't actually be a correct summary, so no.  The point
>> is that variable-length fields are not used in tuple descriptors.
>> I'll add a comment about that.
>
> Yes that sounds much better, I was struggling to find the words.  What
> I was trying to express was something along the lines of you cant have
> a non null value after these because you cant statically initialize
> them in genbki.

Committed. Thanks for the very thorough review.

...Robert