В Thu, 10 Oct 2019 15:50:05 +0900
Amit Langote <amitlangot...@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.
> + 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...
diff --git a/.gitignore b/.gitignore
index 794e35b..37331c2 100644
diff --git a/src/backend/access/common/reloptions.c b/src/backend/access/common/reloptions.c
index b5072c0..067441f 100644
--- a/src/backend/access/common/reloptions.c
+++ b/src/backend/access/common/reloptions.c
@@ -1099,9 +1099,11 @@ extractRelOptions(HeapTuple tuple, TupleDesc tupdesc,
case RELKIND_RELATION:
case RELKIND_TOASTVALUE:
case RELKIND_MATVIEW:
- case RELKIND_PARTITIONED_TABLE:
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;
@@ -1538,6 +1540,25 @@ default_reloptions(Datum reloptions, bool validate, relopt_kind kind)
}
/*
+ * Option parser for partitioned tables
+ */
+bytea *
+partitioned_table_reloptions(Datum reloptions, bool validate)
+{
+ int numoptions;
+
+ /*
+ * Since there are no options for patitioned tables for now, we just do
+ * validation to report incorrect option error and leave.
+ */
+
+ if (validate)
+ parseRelOptions(reloptions, validate, RELOPT_KIND_PARTITIONED,
+ &numoptions);
+ return NULL;
+}
+
+/*
* Option parser for views
*/
bytea *
@@ -1593,9 +1614,6 @@ heap_reloptions(char relkind, Datum reloptions, bool validate)
case RELKIND_RELATION:
case RELKIND_MATVIEW:
return default_reloptions(reloptions, validate, RELOPT_KIND_HEAP);
- case RELKIND_PARTITIONED_TABLE:
- return default_reloptions(reloptions, validate,
- RELOPT_KIND_PARTITIONED);
default:
/* other relkinds are not supported */
return NULL;
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index 05593f3..f44e865 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -720,10 +720,17 @@ DefineRelation(CreateStmt *stmt, char relkind, Oid ownerId,
reloptions = transformRelOptions((Datum) 0, stmt->options, NULL, validnsps,
true, false);
- if (relkind == RELKIND_VIEW)
- (void) view_reloptions(reloptions, true);
- else
- (void) heap_reloptions(relkind, reloptions, true);
+ switch (relkind)
+ {
+ case RELKIND_VIEW:
+ (void) view_reloptions(reloptions, true);
+ break;
+ case RELKIND_PARTITIONED_TABLE:
+ (void) partitioned_table_reloptions(reloptions, true);
+ break;
+ default:
+ (void) heap_reloptions(relkind, reloptions, true);
+ }
if (stmt->ofTypename)
{
@@ -12158,9 +12165,11 @@ ATExecSetRelOptions(Relation rel, List *defList, AlterTableType operation,
case RELKIND_RELATION:
case RELKIND_TOASTVALUE:
case RELKIND_MATVIEW:
- case RELKIND_PARTITIONED_TABLE:
(void) heap_reloptions(rel->rd_rel->relkind, newOptions, true);
break;
+ case RELKIND_PARTITIONED_TABLE:
+ (void) partitioned_table_reloptions(newOptions, true);
+ break;
case RELKIND_VIEW:
(void) view_reloptions(newOptions, true);
break;
diff --git a/src/include/access/reloptions.h b/src/include/access/reloptions.h
index 6bde209..f03f0b1 100644
--- a/src/include/access/reloptions.h
+++ b/src/include/access/reloptions.h
@@ -301,6 +301,7 @@ extern bytea *default_reloptions(Datum reloptions, bool validate,
relopt_kind kind);
extern bytea *heap_reloptions(char relkind, Datum reloptions, bool validate);
extern bytea *view_reloptions(Datum reloptions, bool validate);
+extern bytea *partitioned_table_reloptions(Datum reloptions, bool validate);
extern bytea *index_reloptions(amoptions_function amoptions, Datum reloptions,
bool validate);
extern bytea *attribute_reloptions(Datum reloptions, bool validate);
diff --git a/src/include/utils/rel.h b/src/include/utils/rel.h
index a5cf804..a9cf15c 100644
--- a/src/include/utils/rel.h
+++ b/src/include/utils/rel.h
@@ -278,6 +278,16 @@ typedef struct StdRdOptions
#define HEAP_DEFAULT_FILLFACTOR 100
/*
+ * PartitionedRelOptions
+ * Binary representation of relation options for rtitioned tables.
+ */
+typedef struct PartitionedRelOptions
+{
+ int32 vl_len_; /* varlena header (do not touch directly!) */
+ /* No options defined yet */
+} PartitionedRelOptions;
+
+/*
* RelationGetToastTupleTarget
* Returns the relation's toast_tuple_target. Note multiple eval of argument!
*/
