Re: attoptions

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. :)