Re: [HACKERS] Adding support for Default partition in partitioning

Поиск
Список
Период
Сортировка
От Jeevan Ladhe
Тема Re: [HACKERS] Adding support for Default partition in partitioning
Дата
Msg-id CAOgcT0Nmb3ufvtCKnnP5-vAQwrjWUEXDmb3mt7U0akTBdOu4Lw@mail.gmail.com
обсуждение исходный текст
Ответ на Re: [HACKERS] Adding support for Default partition in partitioning  (Robert Haas <robertmhaas@gmail.com>)
Ответы Re: [HACKERS] Adding support for Default partition in partitioning  (Robert Haas <robertmhaas@gmail.com>)
Список pgsql-hackers
Hi Robert,


> 0005:
> Extend default partitioning support to allow addition of new partitions.

+       if (spec->is_default)
+       {
+               /* Default partition cannot be added if there already
exists one. */
+               if (partdesc->nparts > 0 &&
partition_bound_has_default(boundinfo))
+               {
+                       with = boundinfo->default_index;
+                       ereport(ERROR,
+
(errcode(ERRCODE_INVALID_OBJECT_DEFINITION),
+                                        errmsg("partition \"%s\"
conflicts with existing default partition \"%s\"",
+                                                       relname,
get_rel_name(partdesc->oids[with])),
+                                        parser_errposition(pstate,
spec->location)));
+               }
+
+               return;
+       }

I generally think it's good to structure the code so as to minimize
the indentation level.  In this case, if you did if (partdesc->nparts
== 0 || !partition_bound_has_default(boundinfo)) return; first, then
the rest of it could be one level less indented.  Also, perhaps it
would be clearer to test boundinfo == NULL rather than
partdesc->nparts == 0, assuming they are equivalent.

I think even with this change there will be one level of indentation
needed for throwing the error, as the error is to be thrown only if
there exists a default partition.
 
 
-        * We must also lock the default partition, for the same
reasons explained
-        * in heap_drop_with_catalog().
+        * We must lock the default partition, for the same reasons explained in
+        * DefineRelation().

I don't really see the point of this change.  Whichever earlier patch
adds this code could include or omit the word "also" as appropriate,
and then this patch wouldn't need to change it.


Actually the change is made because if the difference in the function name.
I will remove ‘also’ from the first patch itself.
 
> 0007:
> This patch introduces code to check if the scanning of default partition
> child
> can be skipped if it's constraints are proven.

If I understand correctly, this is actually a completely separate
feature not intrinsically related to default partitioning.

I don't see this as a new feature, since scanning the default partition
will be introduced by this series of patches only, and rather than a
feature this can be classified as a completeness of default skip
validation logic. Your thoughts?

Regards,
Jeevan Ladhe

В списке pgsql-hackers по дате отправления:

Предыдущее
От: Mark Rofail
Дата:
Сообщение: Re: [HACKERS] GSoC 2017: Foreign Key Arrays
Следующее
От: Chris Travers
Дата:
Сообщение: [HACKERS] Orphaned files in base/[oid]