citusdata / pg_shard

ATTENTION: pg_shard is superseded by Citus, its more powerful replacement
https://github.com/citusdata/citus
GNU Lesser General Public License v3.0
1.06k stars 63 forks source link

Bad table name on master_create_distributed_table() #36

Closed onderkalaci closed 9 years ago

onderkalaci commented 9 years ago

This fix handles when master_create_distributed_table is called with a an index as the first argument. More generally, it is called with a relation that is not ordinary table or foreign table.

closes #13

sumedhpathak commented 9 years ago

I added in some minor comments. More importantly, could you check to see why get_attnum doesn't catch this problem when I pass in an index name instead of a table name?

onderkalaci commented 9 years ago

@sumedhpathak for get_attnum called with index as table name, there are two possible scenarios:

  1. if index is created on the partition column provided to master_create_distributed_table, we get no errors.

    CREATE INDEX index_1 ON customer_reviews(customer_id);
    SELECT master_create_distributed_table('index_1', 'customer_id');
  2. if index is NOT created on the partition column provided to master_create_distributed_table, we get an error

    CREATE INDEX index_1 ON customer_reviews(customer_id);
    SELECT master_create_distributed_table('index_1', 'product_id');
    # ERROR:  could not find column: product_id

So, with this fix we are on the safe side, we do not allow master_create_distributed_table called with indexes. Also, error message and code become more informative.

jasonmp85 commented 9 years ago

@sumedhpathak is there any interest in moving this error-handling logic into ResolveRelationId? I consider ResolveRelationId analogous to regclassin, which follows its logic exactly (i.e. it will gladly return an Oid for an index, not just tables), so I don't think we want to do this, but just wanted to bring it up.

jasonmp85 commented 9 years ago

From what @onderkalaci is saying, it appears get_attnum is designed to return attribute numbers both for columns within a table and to return columns used within a (possibly multi-column) index. So when we call it, we want to make sure we're passing an Oid for a table and not an index.

jasonmp85 commented 9 years ago

When I talked to @sumedhpathak on Tuesday he said he had no blockers for shipping this. I have a couple of style things, but as I've got time to kill and this PR is getting a bit old, I'm going to make them myself and merge this (i.e. :shipit: from @sumedhpathak and me and I'm going to merge it for us to get it into develop and close the issue).