Switch to SPI for metadata functions #110
Conversation
| /* insert the shard metadata row along with its min/max values */ | ||
| minHashTokenText = IntegerToText(shardMinHashToken); | ||
| maxHashTokenText = IntegerToText(shardMaxHashToken); | ||
| shardId = CreateShardRow(distributedTableId, shardStorageType, minHashTokenText, |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Discussed at length over the past few weeks. Sticking with this new approach. (done)
jasonmp85
force-pushed
the
feature-metadata_uses_spi
branch
2 times, most recently
from
c7ec962
to
128424a
Compare
| @@ -41,6 +41,12 @@ CREATE SCHEMA pgs_distribution_metadata | |||
| CREATE SEQUENCE shard_id_sequence MINVALUE 10000 NO CYCLE | |||
There was a problem hiding this comment.
So I understand, in the SQL file if we detect CitusDB, we'll modify the view to insert using CitusDB's sequences (instead of these) right? I wanted to make sure we aren't potentially creating collisions here.
There was a problem hiding this comment.
Yeah; I'm in the process of rebasing just the schema changes from #103 on top of this branch so it will be much more clear what the behavior will be.
There obviously could be collisions when upgrading from PostgreSQL + pg_shard to CitusDB, but that's a pretty obscure edge case, IMO.
Easiest to start with, with one caveat: the SELECT statements needed to implement these functions with SPI themselves trigger pg_shard code! So in IsDistributedTable we need to short-circuit if a target table is in the pgs_distribution_metadata schema, otherwise we end up with infinite recursion. Another fun thing is that SPI runs everything within a special memory context that it manages. It provides an SPI_palloc function to reach out to the top-level context, but that only helps replace direct palloc calls, not calls to system functions that themselves use palloc. It seems to me that manually switching to another context is the cleanest way around this "feature" of SPI.
These were pretty straightforward. A test had an arbitrary failure due to ordering changing, but otherwise no problems. I did have to change the shard creation method to create the shard _before_ the placements due to foreign key constraint enforcement, but because the creation is contained within a transaction anyways, there is no difference.
The need to use partition type and column information to interpret min and max values for a shard means we would need to have annoying nesting of the SPI calls. Instead, I kept things simpler by just JOIN-ing in the values I need so they'd be in the HeapTuple.
Will make Citus integration cleaner.
Needed to add an explicit UPDATE call for this to be feasible, but otherwise identical to the change for Shard.
No longer needed. Getting away from referencing anything except views in the C code for flexibility's sake.
Clean up comments.
Unit tests FTW.
Used for primary keys, might as well say so.
Decided an Assert is more appropriate here.
Simplifies this call.
These lines were unexercised by our tests.
Getting coverage up.
Several metadata functions were missing unit tests, so I added them.
We have the code here, might as well test it.
Bad whitespace. Bad.
The codebase doesn't use this functionality, so I see little point in maintaining it.
Per code-review feedback.
Since the recursion arises only when a query references the partition table, it's clearer to check only for that case.
Per code review.
Now the target list indexes and queries live side-by-side.
jasonmp85
force-pushed
the
feature-metadata_uses_spi
branch
from
128424a
to
b22f13e
Compare
Switch to SPI for metadata functions cr: @sumedhpathak
In #103, we discovered that using low-level operations is kind of unwieldy when the code needs to switch between different backing stores. By using SPI, we can overlay a
VIEWon the CitusDB metadata catalogs and have everything "just work".This change prepares for that by switching to SPI for our metadata functions. The functions will now respect any
VIEWorTRIGGERwe need to adapt to CitusDB's underlying representation.