On Mon, Sep 23, 2019 at 11:58 AM Fabien COELHO <coelho@cri.ensmp.fr> wrote:
>
>
> Hello Amit,
>
> > It is better for a user to write a custom script for such cases.
>
> I kind-of agree, but IMHO this is not for pgbench to decide what is better
> for the user and to fail on a script that would not fail.
>
> > Because after that "select-only" or "simple-update" script doesn't
> > make any sense. [...].
>
> What make sense in a benchmarking context may not be what you think. For
> instance, AFAICR, I already removed benevolent but misplaced guards which
> were preventing running scripts without queries: if one wants to look at
> pgbench overheads because they are warry that it may be too high, they
> really need to be allowed to run such scripts.
>
> This not for us to decide, and as I already said they do if you want to
> test no-op overheads. Moreover the problem pre-exists: if the user deletes
> the contents of pgbench_accounts these scripts are no-op, and we do not
> complain. The no partition attached is just a particular case.
>
> > Having said that, I see your point and don't mind allowing such cases
> > until we don't have to write special checks in the code to support such
> > cases.
>
> Indeed, it is also simpler to not care about such issues in the code.
>
If you agree with this, then why haven't you changed below check in patch:
+ if (partition_method != PART_NONE)
+ printf("partition method: %s\npartitions: %d\n",
+ PARTITION_METHOD[partition_method], partitions);
This is exactly the thing bothering me. It won't be easy for others
to understand why this check for partitioning information is different
from other checks. For you or me, it might be okay as we have
discussed this case, but it won't be apparent to others. This doesn't
buy us much, so it is better to keep this code consistent with other
places that check for partitions.
> > [...] Now, we can have a detailed comment in printResults to explain why
> > we have a different check there as compare to other code paths or change
> > other code paths to have a similar check as printResults, but I am not
> > convinced of any of those options.
>
> Yep. ISTM that the current version is reasonable.
>
> > [...] I am talking about the call to append_fillfactor in
> > createPartitions() function. See, in my version, there are some
> > comments.
>
> Ok, I understand that you want a comment. Patch v15 does that.
>
Thanks!
--
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com