Adds postgres implementation for tree_storage
#1343
Conversation
vishalkuo
force-pushed
the
vishal/tree-storage
branch
from
02ed8ae to
9cc4bd3
Compare
vishalkuo
force-pushed
the
vishal/tree-storage
branch
from
9cc4bd3 to
ec81b66
Compare
vishalkuo
force-pushed
the
vishal/tree-storage
branch
from
ec81b66 to
8fb8dee
Compare
Codecov Report
@@ Coverage Diff @@
## master #1343 +/- ##
==========================================
- Coverage 60.17% 59.25% -0.92%
==========================================
Files 129 129
Lines 10380 10568 +188
==========================================
+ Hits 6246 6262 +16
- Misses 3428 3600 +172
Partials 706 706
Continue to review full report at Codecov.
|
| return err | ||
| } | ||
| } | ||
| t.closed = true |
There was a problem hiding this comment.
Should the reads/writes to closed not be covered by a mutex?
There was a problem hiding this comment.
Looks like it's the same in the mysql version, @Martin2112 do you have thoughts?
The cloudspanner implementation by comparison does hold mu in Close()
| @@ -12,43 +12,37 @@ | |||
| // See the License for the specific language governing permissions and | |||
| // limitations under the License. | |||
|
|
|||
| package postgres_test | |||
| package postgres | |||
There was a problem hiding this comment.
Is it not possible to do black box testing of this? (just curious)
There was a problem hiding this comment.
Admin storage is actually currently blackbox tested; however I wanted to bring everything under the postgres package because
A) that seems to be the pattern the rest of the codebase follows
B) It allows us to use a unified TestMain function.
Happy to change it back though, I don't have strong feelings about this
|
I'm alright with going on with this PR and then building on top with |
vishalkuo
force-pushed
the
vishal/tree-storage
branch
from
77d4644 to
ee08162
Compare
|
Polite bump |
|
@RJPercival you happy? |
| } | ||
|
|
||
| // statementSkeleton contains the structure of a query to create. | ||
| type statementSkeleton struct { |
There was a problem hiding this comment.
I assume this gets used a lot more in future PRs, thereby justifying its existence? It looks rather complicated for the two uses that appear in this PR. All of this string substitution into SQL and use of variables as printf format strings makes me a little uneasy!
There was a problem hiding this comment.
If this implementation follows the MySQL impl - it won't get used elsewhere. I'm not sure if there's a better way to handle this but I'm open to suggestions
There was a problem hiding this comment.
Well if @AlCutter is happy with it, I don't want to block you any longer. I'll try and find the time to have a think about whether there's a better way of going about this and come back to it.
There was a problem hiding this comment.
Thanks Rob, I just created #1369 to track this.
This PR adds a partial implementation for the
ReadOnlyTreeTXin postgres.This is mostly a copy from mySQL's tree_storage.go file. The only major difference is the
expandPlaceholderSQLmethod which needs a little more attention due to formatting requiring numeric placeholders for pg instead of?in mySQL.I'm not sure how to test this PR without starting work on the
log_storageimplementation and I was worried at that point it'd be really long and difficult to review. If we're fine merging a NOOP PR in so that it's in a manageable size, I can add the rest in subsequent PRs with tests.Alternatively, I can try to break this PR down into smaller ones that tests functions solely in tree storage OR create a much larger PR with tests via log storage.
I have no strong preference to either option, I'd just love to get some feedback on how to proceed.
Issue: #1298