Re: pgbench - allow to create partitioned tables

On Fri, Sep 20, 2019 at 10:29 AM Fabien COELHO <coelho(at)cri(dot)ensmp(dot)fr> wrote:
>
> >> The behavior is not actually changed, but I had to move fillfactor away
> >> because it cannot be declared on partitioned tables, it must be declared
> >> on partitions only. Once there is a function to handle that it is pretty
> >> easy to add the test.
> >>
> >> I can remove it but franckly there are only benefits: the default is now
> >> tested by pgbench, the create query is smaller, and it would work with
> >> older versions of pg, which does not matter but is good on principle.
> >
> > I am not saying that it is a bad check on its own, rather it might be
> > good, but let's not do any unrelated change as that will delay the
> > main patch. Once, we are done with the main patch, you can propose
> > these as improvements.
>
> 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.

> >> The user could do a -i with a version of pgbench and bench with another
> >> one. I do that often while developing…
> >
> > I am not following what you want to say here especially ("pgbench and
> > bench with another one"). Can you explain with some example?
>
> While developing, I often run pgbench under development client against an
> already created set of tables on an already created cluster, and usually
> the server side on my laptop is the last major release from pgdg (ie 11.5)
> while the pgbench I'm testing is from sources (ie 12dev). If I type
> "pgbench" I run 11.5, and in the sources "./pgbench" runs the dev version.
>

Hmm, I think some such thing is possible when you are running pgbench
of lower version with tables initialized by some higher version of
pgbench. Because higher version pgbench must be a superset of lower
version unless we drop support for one of the partitioning method. I
think even if there is some unknown partition method, it should be
detected much earlier rather than reaching the stage of printing the
results like after the query for partitions in below code.

+ 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.

I have made a few modifications in the attached patch.
* move the create partitions related code into a separate function.
* make the check related to number of partitions consistent i.e check
partitions > 0 apart from where we print which I also want to change
but let us first discuss one of the above points
* when we don't found pgbench_accounts table, error out instead of continuing
* ensure append_fillfactor doesn't assume that it has to append
fillfactor and removed fillfactor < 100 check from it.
* improve the comments around query to fetch partitions
* improve the comments in the patch and make the code look like nearby code
* pgindent the patch

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.

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