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

From Fabien COELHO
Subject Re: pgbench - allow to create partitioned tables
Date
Msg-id alpine.DEB.2.21.1909201626110.3955@lancre
Whole thread Raw
In response to Re: pgbench - allow to create partitioned tables  (Amit Kapila <amit.kapila16@gmail.com>)
Responses Re: pgbench - allow to create partitioned tables
List pgsql-hackers
Hello Amit,

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

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.

> I have made a few modifications in the attached patch.
> * move the create partitions related code into a separate function.

Why not. Not sure it is an improvement.

> * 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

I switched two instances of >= 1 to > 0, which had 1 instance before.

> * when we don't found pgbench_accounts table, error out instead of continuing

I do not think that it is a a good idea, but I did it anyway to move 
things forward.

> * ensure append_fillfactor doesn't assume that it has to append
> fillfactor and removed fillfactor < 100 check from it.

Done, which is too bad.

> * improve the comments around query to fetch partitions

What? How?

There are already quite a few comments compared to the length of the 
query.

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

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

-- 
Fabien.
Attachment

pgsql-hackers by date:

Previous
From: Robert Haas
Date:
Subject: Re: backup manifests
Next
From: David Fetter
Date:
Subject: Re: Efficient output for integer types