Re: Add SPLIT PARTITION/MERGE PARTITIONS commands

On Wed, Jun 1, 2022 at 11:58 AM Dmitry Koval <d(dot)koval(at)postgrespro(dot)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