On Fri, Sep 20, 2019 at 12:41 AM Fabien COELHO <coe...@cri.ensmp.fr> wrote: > > > Hello Amit, > > > [...] 'ps' itself won't be NULL in that case, the value it contains is > > NULL. I have debugged this case as well. 'ps' itself can be NULL only > > when you pass wrong column number or something like that to PQgetvalue. > > Argh, you are right! I mixed up C NULL and SQL NULL:-( > > >>> If we don't find pgbench_accounts, let's give error here itself rather > >>> than later unless you have a valid case in mind. > >> > >> I thought of it, but decided not to: Someone could add a builtin script > >> which does not use pgbench_accounts, or a parallel running script could > >> create a table dynamically, whatever, so I prefer the error to be raised > >> by the script itself, rather than deciding that it will fail before even > >> trying. > > > > I think this is not a possibility today and I don't know of the > > future. I don't think it is a good idea to add code which we can't > > reach today. You can probably add Assert if required. > > I added a fail on an unexpected partition method, i.e. not 'r' or 'h', > and an Assert of PQgetvalue returns NULL. > > I fixed the query so that it counts actual partitions, otherwise I was > getting one for a partitioned table without partitions attached, which > does not generate an error by the way. I just figured out that pgbench > does not check that UPDATE updates anything. Hmmm. > > > Hmm, why you need to change the fill factor behavior? If it is not > > specifically required for the functionality of this patch, then I > > suggest keeping that behavior as it is. > > 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. > >> added that pgbench does not know about, the failure mode will be that > >> nothing is printed rather than printing something strange like > >> "method none with 2 partitions". > > > > but how will that new partition method will be associated with a table > > created via pgbench? > > 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? -- With Regards, Amit Kapila. EnterpriseDB: http://www.enterprisedb.com