On Sat, Jan 16, 2010 at 05:39, Robert Haas <robertmhaas@gmail.com> wrote:
> First, thanks for the review. Detailed comments/questions below.
>
> On Fri, Jan 15, 2010 at 12:52 AM, Alex Hunsaker <badalex@gmail.com> wrote:
>> On Sun, Jan 10, 2010 at 12:27, Robert Haas <robertmhaas@gmail.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. :)