On Sat, Sep 28, 2019 at 11:41 AM Fabien COELHO <coe...@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