Re: attoptions

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