Re: [PATCH] use separate PartitionedRelOptions structure to storepartitioned table options - Mailing list pgsql-hackers

Hi Nikolay,

Sorry for the late reply.

On Fri, Oct 11, 2019 at 7:38 PM Nikolay Shaplov <dhyan@nataraj.su> wrote:
> В Thu, 10 Oct 2019 15:50:05 +0900
> Amit Langote <amitlangote09@gmail.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.

Sure, they're all relations in the abstract, but we've got to
distinguish different kinds in the code somehow.

Anyway, I just want the code to be consistent with what we've already
got, especially, considering that we might need similar function for
partitioned "indexes" in the future.

> > 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.

Yeah, I have never run into this code before, but this might need
revisiting, if only to be consistent with the table counterpart.

> 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...

Sure, this seems like a topic for another thread.

I looked atthe v2 patch and noticed a typo:

+ *     Binary representation of relation options for rtitioned tables.

s/rtitioned/partitioned/g

Other than that, looks good.

Thanks,
Amit