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
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?
> 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
> 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
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
> See patch below on top of yours, it fixes some brainos:
Thanks, those look like good changes.
In response to
pgsql-hackers by date
|Next:||From: Dimitri Fontaine||Date: 2010-01-16 12:55:12|
|Subject: Re: Streaming replication and non-blocking I/O|
|Previous:||From: Pavel Stehule||Date: 2010-01-16 12:26:30|
|Subject: Re: review: More frame options in window functions|