Re: pgbench - allow to create partitioned tables

On Sat, Sep 28, 2019 at 11:41 AM Fabien COELHO <coelho@cri.ensmp.fr> wrote:
>
>
> Hello Amit,
>
> > I think we might also need to use pg_get_partkeydef along with
> > pg_partition_tree to fetch the partition method information.  However,
> > I think to find reloid of pgbench_accounts in the current search path,
> > we might need to use some part of query constructed by Fabien.
> >
> > Fabien, what do you think about Alvaro's suggestion?
>
> I think that the current straightforward SQL query is and works fine, and
> I find it pretty elegant. No doubt other solutions could be implemented to
> the same effect, with SQL or possibly through introspection functions.
>
> Incidentally, ISTM that "pg_partition_tree" appears in v12, while
> partitions exist in v11, so it would break uselessly backward
> compatibility of the feature which currently work with v11, which I do not
> find desirable.
>

Fair enough.  Alvaro, do let us know if you think this can be
simplified?  I think even if we find some better way to get that
information as compare to what Fabien has done here, we can change it
later without any impact.

> Attached v18:
>   - remove the test tablespace
>     I had to work around a strange issue around partitioned tables and
>     the default tablespace.

- if (tablespace != NULL)
+
+ if (tablespace != NULL && strcmp(tablespace, "pg_default") != 0)
  {

- if (index_tablespace != NULL)
+ if (index_tablespace != NULL && strcmp(index_tablespace, "pg_default") != 0)

I don't think such a workaround is a good idea for two reasons (a)
having check on the name ("pg_default") is not advisable, we should
get the tablespace oid and then check if it is same as
DEFAULTTABLESPACE_OID, (b) this will change something which was
previously allowed i.e. to append default tablespace name for the
non-partitioned tables.

I don't think we need any such check, rather if the user gives
default_tablespace with 'partitions' option, then let it fail with an
error "cannot specify default tablespace for partitioned relations".
If we do that then one of the modified pgbench tests will start
failing.  I think we have two options here:

(a) Don't test partitions with "all possible options" test and add a
comment on why we are not testing it there.
(b) Create a non-default tablespace to test partitions with "all
possible options" test as you have in your previous version.  Also,
add a comment explaining why in that test we are using non-default
tablespace.

I am leaning towards approach (b) unless you and or Alvaro feels (a)
is good for now or if you have some other idea.

If we want to go with option (b), I have small comment in your previous test:
+# tablespace for testing
+my $ts = $node->basedir . '/regress_pgbench_tap_1_ts_dir';
+mkdir $ts or die "cannot create directory $ts";
+my $ets = TestLib::perl2host($ts);
+# add needed escaping!
+$ets =~ s/'/''/;

I am not sure if we really need this quote skipping stuff.  Why can't
we write the test as below:

# tablespace for testing
my $basedir = $node->basedir;
my $ts = "$basedir/regress_pgbench_tap_1_ts_dir";
mkdir $ts or die "cannot create directory $ts";
$ts = TestLib::perl2host($ts);
$node->safe_psql('postgres',
        "CREATE TABLESPACE regress_pgbench_tap_1_ts LOCATION '$ets';"

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