Re: pgbench - allow to create partitioned tables

On Wed, Sep 18, 2019 at 12:19 AM Fabien COELHO <coelho(at)cri(dot)ensmp(dot)fr> wrote:
>
>
> Attached v9:
>
> - remove the PART_UNKNOWN and use partitions = -1 to tell
> that there is an error, and partitions >= 1 to print info
> - use search_path to find at most one pgbench_accounts
> It still uses left join because I still think that it is appropriate.
> I added a lateral to avoid repeating the array_position call
> to manage the search_path, and use explicit pg_catalog everywhere.

It would be good if you can add some more comments to explain the
intent of query.

Few more comments:
*
else
+ {
+ /* PQntupes(res) == 1: normal case, extract the partition status */
+ char *ps = PQgetvalue(res, 0, 1);
+
+ if (ps == NULL)
+ partition_method = PART_NONE;

When can we expect ps as NULL? If this is not a valid case, then
probably and Assert would be better.

*
+ else if (PQntuples(res) == 0)
+ {
+ /* no pgbench_accounts found, builtin script should fail later */
+ partition_method = PART_NONE;
+ partitions = -1;
+ }

If we don't find pgbench_accounts, let's give error here itself rather
than later unless you have a valid case in mind.

*
+
+ /*
+ * Partition information. Assume no partitioning on any failure, so as
+ * to avoid failing on an older version.
+ */
..
+ if (PQresultStatus(res) != PGRES_TUPLES_OK)
+ {
+ /* probably an older version, coldly assume no partitioning */
+ partition_method = PART_NONE;
+ partitions = 0;
+ }

So, here we are silently absorbing the error when pgbench is executed
against older server version which doesn't support partitioning. If
that is the case, then I think if user gives --partitions for the old
server version, it will also give an error? It is not clear in
documentation whether we support or not using pgbench with older
server versions. I guess it didn't matter, but with this feature, it
can matter. Do we need to document this?

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