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

Feature issue#12 #35

Closed onderkalaci closed 9 years ago

onderkalaci commented 9 years ago

Shards backed by foreign tables marked correctly in the metadata.

Tested with:

  1. Postgres 9.4
    • foreign table - SELECT/INSERT
    • normal table (RELKIND_RELATION) - SELECT/INSERT
  2. Citus 3.0.1
    • foreign table - SELECT problematic - sent mail to @jasonmp85. But I don't think it is related to this fix, because when I test with undoing the fix, we still have the problem.
    • foreign table - INSERT OK
    • normal table (RELKIND_RELATION) - select/insert

closes #12

Review tasks:

jasonmp85 commented 9 years ago

I made two comments in the source that need to be addressed. Also: run the tests (make installcheck) and add a new one to verify that 'f' is inserted where we expect it.

jasonmp85 commented 9 years ago

I've added a checklist to the PR description: after you address all items, we can :shipit:

jasonmp85 commented 9 years ago

If you need guidance on how to add a test, check out 1247452e3d89497d1d54b59b7d6735d71d6652f8, in which I added a test for the bug when you try to distribute an index.

coveralls commented 9 years ago

Coverage Status

Coverage increased (+0.01%) when pulling b659a6559d7695c93eca0c6b06fcff6b94377819 on feature-issue#12 into 46bcf2c0b1a5250a286e5dd6b0e9fa42e275725f on develop.

onderkalaci commented 9 years ago

@jasonmp85 Since it is the first time I wrote regression tests, it would be nice if you can check it in detail. I had to change expected/modifications.out.tmpl", is that required or useless? Is there a way not to do that?

jasonmp85 commented 9 years ago

If you look into the Makefile, you'll see that the tmpl files are used to generate the sql and out files for certain tests. This is because certain tests' output will contain the port number of the PostgreSQL instance being used and since make installcheck just checks against the current installation, we have no way of predicting what that will be. So if a tmpl file exists, it should be what you modify; otherwise, just modify the sql and out files as necessary.

jasonmp85 commented 9 years ago

This looks good! Merging.