Re: pgbench - allow to create partitioned tables
От | Amit Kapila |
---|---|
Тема | Re: pgbench - allow to create partitioned tables |
Дата | |
Msg-id | CAA4eK1KeZT+6ZhGd61=EcDUnJH7_6mFqC77ppgJiC_DjDUs4JA@mail.gmail.com обсуждение исходный текст |
Ответ на | Re: pgbench - allow to create partitioned tables (Fabien COELHO <coelho@cri.ensmp.fr>) |
Ответы |
Re: pgbench - allow to create partitioned tables
(Fabien COELHO <coelho@cri.ensmp.fr>)
|
Список | 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
В списке pgsql-hackers по дате отправления:
Следующее
От: Soumyadeep ChakrabortyДата:
Сообщение: Re: Don't codegen deform code for virtual tuples in expr eval forscan fetch