Closed onderkalaci closed 9 years ago
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.
@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.
@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.
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…
@onderkalaci Can you rework this to just handle dropping the extension and we'll open an issue for later about dropping tables?
OK, I left all my feedback. Can you remove the DROP TABLE
logic and address feedback? Adding my checklist…
@onderkalaci I hope this isn't too presumptuous of me, but I'm making some changes here before merging:
DROP EXTENSION
", which won't make sense unless the reader knows we removed the code to handle DROP TABLE
PgShardProcessUtilityDropStatement
function to be very clear about its purpose and changed its parameter to a DropStmt
pointerThe 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.
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);
This pull request includes commits which add dependency control for pg_shard extension and distributed tables.
Fixes #49
Code Review Tasks
develop
(i.e.git fetch; git rebase -i origin/develop
) or merge develop into this branchDROP TABLE
logic and all functions/tests used by that logicDROP EXTENSION pg_shard
against a database wherepg_shard
isn't yet created (i.e. does your block run, or is it skipped?)if (booleanVar == true)
, just doif (booleanVar)
strncmp
, neverstrcmp
)DistributedTablesExist
so it's not called in a loop