Prev: PG_MODULE_MAGIC checks and pg_migrator
Next: Streaming replication, loose ends
From: Alex Hunsaker on 15 Jan 2010 00:52
On Sun, Jan 10, 2010 at 12:27, Robert Haas <robertmhaas(a)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.

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;

/*

--
Sent via pgsql-hackers mailing list (pgsql-hackers(a)postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

From: Robert Haas on 16 Jan 2010 07:39
First, thanks for the review. Detailed comments/questions below.

On Fri, Jan 15, 2010 at 12:52 AM, Alex Hunsaker <badalex(a)gmail.com> wrote:
> On Sun, Jan 10, 2010 at 12:27, Robert Haas <robertmhaas(a)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?

> 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

--
Sent via pgsql-hackers mailing list (pgsql-hackers(a)postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

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

--
Sent via pgsql-hackers mailing list (pgsql-hackers(a)postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

From: Robert Haas on 19 Jan 2010 09:49
On Sun, Jan 17, 2010 at 9:57 PM, Alex Hunsaker <badalex(a)gmail.com> wrote:
>>>> ... 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?

It's clear now, but I don't think I agree. On balance, I'm inclined
to just rip out the special case permissions checks that
AT_SetDistinct uses and just use ATSimplePermissionsRelationOrIndex()
instead. That will mean that users can't use ALTER TABLE ... ALTER
COLUMN ... SET STATISTICS DISTINCT for system tables, but I don't
think that's much of a loss, and it certainly seems cleaner than
hoping that any additional attoptions we add in the future will be
things that we don't mind having applied to system tables.

There's a further design issue here in that the reloptions code
currently contemplates at most 31 types of objects. That makes sense
if the object types are things like "table" or "GIN index", but it's
not going to work if we get too fine-grained. The "right" way to make
n_distinct apply to both table columns and index columns and
n_distinct_inherited only to table columns is probably to define two
different reloption kinds, but that's burning up our supply of
available bits a little more quickly than I feel comfortable with. So
I'm inclined to just let n_distinct_inherited be applied either place,
and if you happen to apply it to an index column it just won't affect
anything. We might want to refactor the reloptions API in the future
to allow this to be handled better, but I don't think we need or want
to do that for 8.5.

Does that make sense?

....Robert

--
Sent via pgsql-hackers mailing list (pgsql-hackers(a)postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

From: Alex Hunsaker on 19 Jan 2010 10:51
On Tue, Jan 19, 2010 at 07:49, Robert Haas <robertmhaas(a)gmail.com> wrote:
>That will mean that users can't use ALTER TABLE ... ALTER
> COLUMN ... SET STATISTICS DISTINCT for system tables, but I don't
> think that's much of a loss, and it certainly seems cleaner than
> hoping that any additional attoptions we add in the future will be
> things that we don't mind having applied to system tables.

I assumed there was a good reason to apply them to system tables. But
I admit I did not follow the original SET STATISTICS DISTINCT patch. [
looks ] Oh ok seeing that you were the original patch author you
probably have a good idea about the above, so ill defer :)

> There's a further design issue here in that the reloptions code
> currently contemplates at most 31 types of objects.  That makes sense
> if the object types are things like "table" or "GIN index", but it's
> not going to work if we get too fine-grained.  The "right" way to make
> n_distinct apply to both table columns and index columns and
> n_distinct_inherited only to table columns is probably to define two
> different reloption kinds, but that's burning up our supply of
> available bits a little more quickly than I feel comfortable with.  So
> I'm inclined to just let n_distinct_inherited be applied either place,
> and if you happen to apply it to an index column it just won't affect
> anything.  We might want to refactor the reloptions API in the future
> to allow this to be handled better, but I don't think we need or want
> to do that for 8.5.

Agreed.

Although ISTM we might be able to extend it just using two bits:
RELOPT_KIND_SYSTEM
RELOPT_KIND_INHERIT
#define RELOPT_KIND_INDEX RELOPT_KIND_BTREE|RELOPT_KIND_GIST|...

Abuse ? Maybe. The hardest part I think would be setting those flags
appropriately.

But yes, lets keep it simple for now.

--
Sent via pgsql-hackers mailing list (pgsql-hackers(a)postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

 |  Next  |  Last
Pages: 1 2 3
Prev: PG_MODULE_MAGIC checks and pg_migrator
Next: Streaming replication, loose ends