On Wed, Jun 1, 2022 at 11:58 AM Dmitry Koval <d.ko...@postgrespro.ru> wrote:
> Hi, > > 1) > > For attachPartTable, the parameter wqueue is missing from comment. > > The parameters of CloneRowTriggersToPartition are called parent and > partition. > > I think it is better to name the parameters to attachPartTable in a > similar manner. > > > > For struct SplitPartContext, SplitPartitionContext would be better name. > > > > + /* Store partition contect into list. */ > > contect -> context > > Thanks, changed. > > 2) > > For transformPartitionCmdForMerge(), nested loop is used to detect > duplicate names. > > If the number of partitions in partcmd->partlist, we should utilize map > to speed up the check. > > I'm not sure what we should utilize map in this case because chance that > number of merging partitions exceed dozens is low. > Is there a function example that uses a map for such a small number of > elements? > > 3) > > For check_parent_values_in_new_partitions(): > > > > + if (!find_value_in_new_partitions(&key->partsupfunc[0], > > + key->partcollation, parts, > nparts, datum, false)) > > + found = false; > > > > It seems we can break out of the loop when found is false. > > We have implicit "break" in "for" construction: > > + for (i = 0; i < boundinfo->ndatums && found; i++) > > I'll change it to explicit "break;" to avoid confusion. > > > Attached patch with the changes described above. > -- > With best regards, > Dmitry Koval > > Postgres Professional: http://postgrespro.com Hi, Thanks for your response. w.r.t. #2, I think using nested loop is fine for now. If, when this feature is merged, some user comes up with long merge list, we can revisit this topic. Cheers
