Re: pgbench - allow to create partitioned tables - Mailing list pgsql-hackers

On Sat, Sep 21, 2019 at 12:26 AM Fabien COELHO <coelho@cri.ensmp.fr> wrote:
>
> >> I would not bother to create a patch for so small an improvement. This
> >> makes sense in passing because the created function makes it very easy,
> >> but otherwise I'll just drop it.
> >
> > I would prefer to drop for now.
>
> Attached v13 does that, I added a comment instead. I do not think that it
> is an improvement.
>
> > + else
> > + {
> > + fprintf(stderr, "unexpected partition method: \"%s\"\n", ps);
> > + exit(1);
> > + }
> >
> > If we can't catch that earlier, then it might be better to have some
> > version-specific checks rather than such obscure code which is
> > difficult to understand for others.
>
> Hmmm. The code simply checks for the current partitioning and fails if the
> result is unknown, which I understood was what you asked, the previous
> version was just ignoring the result.
>

Yes, this code is correct.  I am not sure if you understood the point,
so let me try again. I am bothered about below code in the patch:
+ /* only print partitioning information if some partitioning was detected */
+ if (partition_method != PART_NONE)

This is the only place now where we check 'whether there are any
partitions' differently.  I am suggesting to make this similar to
other checks (if (partitions > 0)).

> The likelyhood of postgres dropping support for range or hash partitions
> seems unlikely.
>
> This issue rather be raised if an older partition-enabled pgbench is run
> against a newer postgres which adds a new partition method. But then I
> cannot guess when a new partition method will be added, so I cannot put a
> guard with a version about something in the future. Possibly, if no new
> method is ever added, the code will never be triggered.
>

Sure, even in that case your older version of pgbench will be able to
detect by below code:
+ else
+ {
+ fprintf(stderr, "unexpected partition method: \"%s\"\n", ps);
+ exit(1);
+ }

>
> > * improve the comments around query to fetch partitions
>
> What? How?
>
> There are already quite a few comments compared to the length of the
> query.
>

Hmm, you have just written what each part of the query is doing which
I think one can identify if we write some general comment as I have in
the patch to explain the overall intent.  Even if we write what each
part of the statement is doing, the comment explaining overall intent
is required.  I personally don't like writing a comment for each
sub-part of the query as that makes reading the query difficult.  See
the patch sent by me in my previous email.

> > * improve the comments in the patch and make the code look like nearby
> > code
>
> This requirement is to fuzzy. I re-read the changes, and both code and
> comments look okay to me.
>

I have done that in some of the cases in the patch attached by me in
my last email.  Have you looked at those changes?  Try to make those
changes in the next version unless you see something wrong is written
in comments.

> > * pgindent the patch
>
> Done.
>
> > I think we should try to add some note or comment that why we only
> > choose to partition pgbench_accounts table when the user has given
> > --partitions option.
>
> Added as a comment on the initPartition function.
>

I am not sure if something like that is required in the docs, but we
can leave it for now.

-- 
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com