Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Switch to SPI for metadata functions #110

Merged
merged 21 commits into from
May 22, 2015
Merged

Switch to SPI for metadata functions #110

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
21 commits
Select commit Hold shift + click to select a range
063a7d2
SPI-ify Partition metadata functions
jasonmp85 May 6, 2015
6a1868a
SPI-ify Placement metadata functions
jasonmp85 May 7, 2015
48e2b64
SPI-ify Shard metadata functions
jasonmp85 May 7, 2015
1825e72
Remove use of NextSequenceId for Shard
jasonmp85 May 7, 2015
ef7294d
Remove use of NextSequenceId for ShardPlacement
jasonmp85 May 7, 2015
764669f
Remove NextSequenceId and its callers
jasonmp85 May 7, 2015
08c6632
Documentation fixes
jasonmp85 May 8, 2015
73d449c
Add test for placement update function
jasonmp85 May 8, 2015
54283da
Add ALTER statement to specify sequence owners
jasonmp85 May 8, 2015
bb55332
Clean up style, conditionals
jasonmp85 May 8, 2015
7b58b23
Use SPI_exec for no-arg calls
jasonmp85 May 8, 2015
8d53d73
Test rejection of system column as partition
jasonmp85 May 8, 2015
f223f68
Add tests for metadata delete/update errors
jasonmp85 May 8, 2015
ce5f2cb
Add missing unit tests
jasonmp85 May 8, 2015
4c6e189
Add test for range-partitioned shards
jasonmp85 May 8, 2015
e9a4f9a
Simple style fixes
jasonmp85 May 8, 2015
c6149bd
Remove NULL value handling in CreateShardRow
jasonmp85 May 8, 2015
4e09d61
Add comments about SPI's MemoryContext
jasonmp85 May 21, 2015
54cedf2
Narrow recursion base-case in IsDistributedTable
jasonmp85 May 21, 2015
87580e2
Fix minor comment typo
jasonmp85 May 21, 2015
b22f13e
Switch to macros for more complicated queries
jasonmp85 May 22, 2015
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
28 changes: 11 additions & 17 deletions create_shards.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -236,14 +236,11 @@ master_create_worker_shards(PG_FUNCTION_ARGS)

for (shardIndex = 0; shardIndex < shardCount; shardIndex++)
{
uint64 shardId = NextSequenceId(SHARD_ID_SEQUENCE_NAME);
int64 shardId = -1;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I initially thought we had shardId's defined as unsigned everywhere, but that doesn't seem to be the case. Do you know why NextSequenceId was returning uint64's, but shardId's were int64's everywhere? Do we have other places with this inconsistency?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some of the CitusDB-inspired code uses unsigned integer types even though all of PostgreSQL's built-in integer types are signed. We kept the code identical before to have it remain identical to CitusDB where possible, but this has been bothering me for some time now, so I've been changing unsigned types to signed as I touch any of the affected lines.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not changing in this review, but I will go through and eradicate unsigned integers from pg_shard in another branch. (done)

int32 placementCount = 0;
uint32 placementIndex = 0;
uint32 roundRobinNodeIndex = shardIndex % workerNodeCount;

List *extendedDDLCommands = ExtendedDDLCommandList(distributedTableId, shardId,
ddlCommandList);

/* initialize the hash token space for this shard */
text *minHashTokenText = NULL;
text *maxHashTokenText = NULL;
Expand All @@ -256,6 +253,15 @@ master_create_worker_shards(PG_FUNCTION_ARGS)
shardMaxHashToken = INT_MAX;
}

/* insert the shard metadata row along with its min/max values */
minHashTokenText = IntegerToText(shardMinHashToken);
maxHashTokenText = IntegerToText(shardMaxHashToken);
shardId = CreateShardRow(distributedTableId, shardStorageType, minHashTokenText,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We seem to have switched to associating the id column with its sequence, with an auto-increment. Any reason why?
This currently switches the order of creation of shard and shard placement. We initially created all the placements first, and then created the shard.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We also error out if we were unable to create placements. Thus creating the shard row after that was considered safe. Will the shard row we've created get rolled back if we error out on placement creation? (See line 296 for the error)

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We seem to have switched to associating the id column with its sequence, with an auto-increment. Any reason why?

By using a default value, the C code can remain ignorant of the names of sequences used by CitusDB or pg_shard. This means the only place that knowledge lives will be in the SQL install script: the C code will not have any CitusDB/pg_shard detection code for metadata functionality.

We also error out if we were unable to create placements. Thus creating the shard row after that was considered safe. Will the shard row we've created get rolled back if we error out on placement creation? (See line 296 for the error)

Both the old and the new approaches are wrapped in a transaction. They aren't actually any different. We never "creat[ed] the shard row after that was considered safe"… we had an atomic transaction that resulted in other readers seeing the shard once placement rows showing up at once.

The practical reason for switching the order is because SPI enforces the foreign key constraint, so the old order is impossible (I even tried using a DEFERRED constraint, but I think each SPI command runs in its own subtransaction, so that failed as well).

FWIW I tested failures at each point of creation, both with the old and new implementation, and they exhibit identical functionality. I also tested visibility of modifications, and they were and are atomic.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Discussed at length over the past few weeks. Sticking with this new approach. (done)

maxHashTokenText);

List *extendedDDLCommands = ExtendedDDLCommandList(distributedTableId, shardId,
ddlCommandList);

for (placementIndex = 0; placementIndex < placementAttemptCount; placementIndex++)
{
int32 candidateNodeIndex =
Expand All @@ -269,13 +275,7 @@ master_create_worker_shards(PG_FUNCTION_ARGS)
extendedDDLCommands);
if (created)
{
uint64 shardPlacementId = 0;
ShardState shardState = STATE_FINALIZED;


shardPlacementId = NextSequenceId(SHARD_PLACEMENT_ID_SEQUENCE_NAME);
InsertShardPlacementRow(shardPlacementId, shardId, shardState,
nodeName, nodePort);
CreateShardPlacementRow(shardId, STATE_FINALIZED, nodeName, nodePort);
placementCount++;
}
else
Expand All @@ -298,12 +298,6 @@ master_create_worker_shards(PG_FUNCTION_ARGS)
"requested replication factor of %d.",
placementCount, replicationFactor)));
}

/* insert the shard metadata row along with its min/max values */
minHashTokenText = IntegerToText(shardMinHashToken);
maxHashTokenText = IntegerToText(shardMaxHashToken);
InsertShardRow(distributedTableId, shardId, shardStorageType,
minHashTokenText, maxHashTokenText);
}

if (QueryCancelPending)
Expand Down
Loading