Re: [PATCH] use separate PartitionedRelOptions structure to store partitioned table options

В Thu, 10 Oct 2019 15:50:05 +0900
Amit Langote <amitlangote09(at)gmail(dot)com> пишет:

> > I think it is bad idea to suggest option adder to ad it to
> > StdRdOption, we already have a big mess there. Better if he add it
> > to an new empty structure.
>
> I tend to agree that this improves readability of the reloptions code
> a bit.
>
> Some comments on the patch:
>
> How about naming the function partitioned_table_reloptions() instead
> of partitioned_reloptions()?

I have my doubts about using word table here... In relational model
there are no such concept as "table", it uses concept "relation". And
in postgres there were no tables, there were only relations. Heap
relation, toast relation, all kind of index relations... I do not
understand when and why word "table" appeared when we speak about some
virtual relation that is made of several real heap relation. That is
why I am not using the word table here...

But since you are the author of partition table code, and this code is
already accepted in the core, you should know better. So I will change
it the way you say.

> + switch ((int)relkind)
>
> Needs a space between (int) and relkind, but I don't think the (int)
> cast is really necessary. I don't see it in other places.
Oh. Yeh. This is my mistake... I had some strange compilation
problems, and this is a remnant of my efforts to find the cause of
it, I've forgot to clean...
Thanks!

> Speaking of partitioned relations vs tables, I see that you didn't
> touch partitioned indexes (RELKIND_PARTITIONED_INDEX relations). Is
> that because we leave option handling to the index AMs?

Because for partitioned indexes the code says "Use same options
original index does"

bytea *
extractRelOptions(HeapTuple tuple, TupleDesc tupdesc,
amoptions_function amoptions)
{
............
switch (classForm->relkind)
{
case RELKIND_RELATION:
case RELKIND_TOASTVALUE:
case RELKIND_MATVIEW:
options = heap_reloptions(classForm->relkind, datum, false);
break;
case RELKIND_PARTITIONED_TABLE:
options = partitioned_table_reloptions(datum, false);
break;
case RELKIND_VIEW:
options = view_reloptions(datum, false);
break;
case RELKIND_INDEX:
case RELKIND_PARTITIONED_INDEX:
options = index_reloptions(amoptions, datum, false);
break;

Here you see the function accepts amoptions method that know how to
parse options for a particular index, and passes it to index_reloptions
functions. For both indexes and partitioned indexes it is taken from AM
"method" amoptions. So options uses exactly the same code and same
options both for indexes and for partitioned indexes.

I do not know if it is correct from global point of view, but from the
point of view of reloptions engine, it does not require any attention:
change index options and get partitioned_index options for free...

Actually I would expect some problems there, because sooner or later,
somebody would want to set custom fillfactor to partitioned table, or
set some custom autovacuum options for it. But I would prefer to think
about it when I am done with reloption engine rewriting... Working in
both direction will cause more trouble then get benefits...