Feature issue#49

[onderkalaci]

This pull request includes commits which add dependency control for pg_shard extension and distributed tables.

Fixes #49

Code Review Tasks

• Rebase this branch onto develop (i.e. git fetch; git rebase -i origin/develop) or merge develop into this branch
• Remove DROP TABLE logic and all functions/tests used by that logic
• Tell me what happens when you run DROP EXTENSION pg_shard against a database where pg_shard isn't yet created (i.e. does your block run, or is it skipped?)
• Don't do if (booleanVar == true), just do if (booleanVar)
• Always use the bounded version of string/memory functions (strncmp, never strcmp)
• Pull up DistributedTablesExist so it's not called in a loop
• Don't check a condition just to set a boolean flag to true in an if block. Just set the boolean flag to the condition's value

[jasonmp85]

The nesting gets pretty deep here. Can you move the drop logic into its own function?

[jasonmp85]

Left some comments, mostly minor for now. Still want to think overnight about this approach, as it's quite a bit of code for something that's conceptually pretty simple. Will add more comments tomorrow morning.

[onderkalaci]

@jasonmp85 Well, in the beginning of our discussions, we thought that we should write C codes for this purpose. If we are going to implement it with C (either event trigger or utility hook), we need to write these codes (at least I couldn't find any other way).

So, we may consider writing a SQL event trigger.

Or, I am not sure, but if we use referential integrity constraints during metadata table creation, would the code become smaller? For instance, deleting a row from partition column propagates to shard and shard_placements.

I haven't addressed your feedback, I think it is better to decide high level design first.

[onderkalaci]

@jasonmp85 we have chatted with @sumedhpathak and thought that we could divide the issue into two. One is for DROP EXTENSION and the other is DROP TABLE. In this way, we can merge DROP EXTENSION for v1.1, which is simpler and requires small code changes.
So, you can also consider this option for the issue.

[jasonmp85]

OK, the more I think about it, the more I think @sumedhpathak and you are on to something with the "let's split this in two" idea…

• The foreign key concept might have some merit and was precisely what I was thinking when considering whether this could be done with a higher level (less LoC) implementation
• Prevent deletion of metadata when still in use #49 really only discusses removing the extension, anyhow
• Much of the code here is boilerplate stuff to remove rows. If we just address the extension dropping, that goes away

@onderkalaci Can you rework this to just handle dropping the extension and we'll open an issue for later about dropping tables?

[jasonmp85]

OK, I left all my feedback. Can you remove the DROP TABLE logic and address feedback? Adding my checklist…

[jasonmp85]

I think the idiom within PostgreSQL is to cast this before calling the function so the function signature can be more specific.

[jasonmp85]

@onderkalaci I hope this isn't too presumptuous of me, but I'm making some changes here before merging:

• The "if there exists" construction is pretty formal, so I rewrote the comments that used it
• A lot of the comments seemed pretty verbose, so I abbreviated most, leaving block comments only for things that obviously required special explanation
• I removed any mention of "only handling DROP EXTENSION", which won't make sense unless the reader knows we removed the code to handle DROP TABLE
• When I viewed your method in my editor, it had many levels of nesting, requiring long function calls to split parameters across many lines (up to five!). I'll explain the fix to this below
• I renamed the PgShardProcessUtilityDropStatement function to be very clear about its purpose and changed its parameter to a DropStmt pointer

The only change in functionality I made is this: the original implementation would have printed a NOTICE if the user specified CASCADE even if no distributed tables existed. This would be misleading. Now I test early whether any distributed tables exist and just exit early it not.

Reducing Indentation Levels

If we have a construct like this:

void
SomeFunction(Params *param)
{
    Variable variable = param->field;
    /* ... */

    if (variable == VALUE)
    {
        Variable other = variable * 2;
        /* ... */
    }
    /* no more code in this function after if statement */
}

We can remove a level of nesting by doing this instead:

void
SomeFunction(Params *param)
{
    Variable variable = param->field;
    Variable other = -1;
    /* ... */

    if (variable != VALUE)
    {
        return;
    }

    other = variable * 2;
    /* code that used to be inside if block */
}

I'd say use this judiciously, because it can become a tax on the reader, but if you find yourself with five levels of nesting and you're inside some if block that could be replaced with a short-circuit (either a call to ereport(ERROR, ...) or a return), consider it.

[jasonmp85]

Oh yeah, and I changed the ERROR to use the precise language used by PostgreSQL when you try to drop a table that's dependent on another table.

A final note: You had errmsg("some message: %s", POUND_DEFINED_STRING_CONSTANT); in a couple of places. You can just let the compiler concatenate these strings for you: errmsg("some message: " POUND_DEFINED_STRING_CONSTANT);