Feature issue#16

[onderkalaci]

This pull request aims to prevent tables to be distributed with columns that cannot be used as distribution column.

fixes #16

Code Review Tasks

• [x] Remove get_attnum section and replace with call to ColumnNameToColumn
• [x] Rewrite GetSupportRoutine as SupportFunctionForColumn and make it take a Var *
• [x] Move check for default operator class into SupportFunctionForColumn
• [x] Update error codes, messages, and codes as specified
• [x] Reduce variable scope as much as possible
• [x] Fix SQL formatting issues in test

[coveralls]

Coverage increased (+0.0%) to 93.21% when pulling 20b17d685940c888f95564b5b0a3dfd15c217a6a on feature-issue#16 into 036e3a8969ce19d862b71680134e54d1134ef8ec on develop.

[jasonmp85]

Hey @onderkalaci — I know I said I'd email @sumedhpathak and @ozgune about breaking the error checking out into its own function, but what I hadn't realized is that master_create_distributed_table is all error checking! The GitHub view cut off right after the call to InsertPartitionRow, so I didn't see that we return immediately after.

So there is no argument to support breaking error handling into its own function here: it makes sense that master_create_distributed_table is all error handling code since the logic is so simple at this point (a single function call).

Which just leaves my point about the inequality operator checks. I'll see whether there are any other stylistic things that need addressing and add a checklist at the top of this review. After those changes I think we're good to merge.

[onderkalaci]

Hey @jasonmp85, @sumedhpathak

Below I summerize what is included in the updated pull request:

• A new function "GetSupportRoutine()" is added. Basically, aim of this function is to find the support routine wrt partition method.
• For instance, this function helps us to check if the partition column has a hash function if hash partitioning is used. Similarly, check if partition column has required comparison operator if range partitioning is used. This function is compatible with citus codes.
• Two functions changed from "static" to "extern" which are used in the pull request: 1) distribution_metadata.c @ColumnNameToColumn(Oid relationId, char *columnName); 2) pruneShardList.c @GetOperatorByType(Oid typeId, Oid accessMethodId, int16 strategyNumber);
• Two checks are added to prevent table to be distributed: 1) If partition column data type does not have a default operator class, error out (ie:json data type). 2) Depending on the partition type, check for the existence of support procedure. For details, see below bullet item.
• Added two tests to increase the test coverage. One for each item above. In order to add the test, I created a type, function, operator, operator class and operator family. The reason that I had to create them is that I could not find any internal data type that does not have support routine.
• One controversial issue in the code is that, I added check for support routine for range partitioning. However, that part of the code is not possible to be executed because we reject range partitioning before that code is executed. So, I added a comment saying that when we support range partitioning, we should add regression test. Should I delete or handle this case in another way? Or it is OK?
• One future issue that we need to do is to apply the composite type bug changes to pg_shard, which is on CR in citus currently. This will enable us to distribute on arrays and composite types.

[jasonmp85]

Ok, so after staring at this for a while, I think I know what I'd like to see happen…

In some code you haven't modified, there's a section like:

partitionColumnId = get_attnum(distributedTableId, partitionColumnName);
if (partitionColumnId == InvalidAttrNumber)
{
    ereport(ERROR, (errmsg("could not find column: %s", partitionColumnName)));
}

This error checking is duplicated by ColumnNameToColumn, which you call later. I think the above section should be removed entirely and replaced by the call to ColumnNameToColumn (step one).

Then, GetSupportRoutine should be renamed SupportFunctionForColumn and it should be modified to accept that Var * (step two).

If the type does not have a default operator class, SupportFunctionForColumn should call ereport itself with a suitable error message (step three).

I think this could help code flow quite a bit while cutting down on verbosity and duplication.

[jasonmp85]

Ok @onderkalaci — I've left my checklist at the top of this PR. I think the reorganization is enough that I'll want to check this out once more before a merge, but assuming it's mostly in shape by morning (MDT) I'll merge it myself if it looks good.

[onderkalaci]

@jasonmp85 Thanks for the review. I think that now code looks better than previous implementation. But I couldn't understand why coverage decreased even I added tests. Is it a problem?

[jasonmp85]

@onderkalaci By default, Coveralls will complain if coverage decreases. We discussed the coverage of your patch (and how it couldn't be complete until we support range partitioning) and I expected coverage to decrease slightly.

If you notice, the merge button is still present, and nothing can ever stop any of us from merging from the command line. So it's just a slight inconvenience designed to make sure you are aware you lowered coverage.

In this case, it's expected, so no biggie.

[jasonmp85]

I've tested this on the latest CitusDB, PostgreSQL 9.3, and PostgreSQL 9.4 on OS X. Travis'll take care of Linux. After it passes I'm gonna merge.