Hi, Pavel.
Thank you for the review.
On Fri, Apr 26, 2024 at 4:33 PM Pavel Borisov <pashkin.elfe@gmail.com> wrote:
> I've looked at the patchset:
>
> 0001 Look good.
> 0002 Also right with docs modification proposed by Justin.
Modified as proposed by Justin. The documentation for the way new
partitions are created is now in separate patch.
> 0003:
> Looks like unused code
> 5268 datum = cmpval ? list_nth(spec->lowerdatums, abs(cmpval) - 1) : NULL;
> overridden by
> 5278 datum = list_nth(spec->upperdatums, abs(cmpval) - 1);
> and
> 5290 datum = list_nth(spec->upperdatums, abs(cmpval) - 1);
>
> Otherwise - good.
Fixed, thanks.
> 0004:
> I suggest also getting rid of thee-noun compound words like: salesperson_name. Maybe salesperson -> clerk? Or maybe
usethe same terms like in pgbench: branches, tellers, accounts, balance.
Thank you, but I'd like to prefer keeping these modifications simple.
It's just regression tests, we don't need to have perfect naming here.
My intention is to fix just obvious errors.
> 0005: Good
> 0006: Patch is right
> In comments:
> + New partitions will have the same table access method,
> + same column names and types as the partitioned table to which they belong.
> (I'd suggest to remove second "same")
Documentation is modified per proposal by Justin. Thus double "same"
is already gone.
> Tests are passed. I suppose that it's better to add similar tests for SPLIT/MERGE PARTITION(S) to those covering
ATTACH/DETACHPARTITION (e.g.: subscription/t/013_partition.pl and regression tests)
The revised patchset is attached. I'm going to push it if there are
no objections.
Thank you for your suggestions about adding tests similar to
subscription/t/013_partition.pl. I will work on this after pushing
this patchset.
------
Regards,
Alexander Korotkov
Supabase