Re: attoptions

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;

/*