Re: pgbench - allow to create partitioned tables

On Sat, Sep 14, 2019 at 6:35 PM Fabien COELHO <coelho@cri.ensmp.fr> wrote:
> >>>> I'm ensuring that there is always a one line answer, whether it is
> >>>> partitioned or not. Maybe the count(*) should be count(something in p) to
> >>>> get 0 instead of 1 on non partitioned tables, though, but this is hidden
> >>>> in the display anyway.
> >>>
> >>> Sure, but I feel the code will be simplified.  I see no reason for
> >>> using left join here.
> >>
> >> Without a left join, the query result is empty if there are no partitions,
> >> whereas there is one line with it. This fact simplifies managing the query
> >> result afterwards because we are always expecting 1 row in the "normal"
> >> case, whether partitioned or not.
> >
> > Why can't we change it as attached?
>
> I think that your version works, but I do not like much the condition for
> the normal case which is implicitely assumed. The solution I took has 3
> clear-cut cases: 1 error against a server without partition support,
> detect multiple pgbench_accounts table -- argh, and then the normal
> expected case, whether partitioned or not. Your solution has 4 cases
> because of the last implicit zero-row select that relies on default, which
> would need some explanations.
>

Why?  Here, we are fetching the partitioning information. If it
exists, then we remember that to display for later, otherwise, the
default should apply.

> > I find using left join to always get one row as an ugly way to
> > manipulate the results later.
>
> Hmmm. It is really a matter of taste. I do not share your distate for left
> join on principle.
>

Oh no, I am not generally against using left join, but here it appears
like using it without much need.  If nothing else, it will consume
more cycles to fetch one extra row when we can avoid it.

Irrespective of whether we use left join or not, I think the below
change from my patch is important.
- /* only print partitioning information if some partitioning was detected */
- if (partition_method != PART_NONE && partition_method != PART_UNKNOWN)
+ /* print partitioning information only if there exists any partition */
+ if (partitions > 0)

Basically, it would be good if we just rely on 'partitions' to decide
whether we have partitions or not.

> In the case at hand, I find that getting one row in all
> cases pretty elegant because there is just one code for handling them all.
>

Hmm, I would be fine if you can show some other place in code where
such a method is used or if someone else also shares your viewpoint.

>
> > What is the need of using regress_pgbench_tap_1_ts in this test?
>
> I wanted to check that tablespace options work appropriately with
> partition tables, as I changed the create table stuff significantly, and
> just using "pg_default" is kind of cheating.
>

I think your change will be tested if there is a '--tablespace'
option.  Even if you want to test win non-default tablespace, then
also, adding additional test would make more sense rather than
changing existing one which is testing a valid thing.  Also, there is
an existing way to create tablespace location in
"src/bin/pg_checksums/t/002_actions".  I think we can use the same.  I
don't find any problem with your way, but why having multiple ways of
doing same thing in code.  We need to test this on windows also once
as this involves some path creation which might vary, although I don't
think there should be any problem in that especially if we use
existing way.

> > I think we don't need to change existing tests unless required for the
> > new functionality.
>
> I do agree, but there was a motivation behind the addition.
>
> > *
> > - 'pgbench scale 1 initialization');
> > + 'pgbench scale 1 initialization with options');
> >
> > Similar to the above, it is not clear to me why we need to change this?
>
> Because I noticed that it had the same description as the previous one, so
> I made the test name distinct and more precise, while I was adding options
> on it.
>

Good observation, but better be done separately.  I think in general
the more unrelated changes are present in patch, the more time it
takes to review.

One more comment:
+typedef enum { PART_NONE, PART_RANGE, PART_HASH, PART_UNKNOWN }
+  partition_method_t;

See, if we can eliminate PART_UNKNOWN.  I don't see much use of same.
It is used at one place where we can set PART_NONE without much loss.
Having lesser invalid values makes code easier to follow.

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