Skip to content
This repository has been archived by the owner on Jan 29, 2020. It is now read-only.

[WIP][RFC]sequences support #186

Closed
6 of 12 tasks
alextech opened this issue Nov 9, 2016 · 4 comments
Closed
6 of 12 tasks

[WIP][RFC]sequences support #186

alextech opened this issue Nov 9, 2016 · 4 comments

Comments

@alextech
Copy link
Contributor

alextech commented Nov 9, 2016

  • Explicit sequence with string or array identifier
  • Multiple sequences per table (would love ideas for improvement)
  • Implicit sequence with column name only for SERIAL pseudo-type
    • Generate based on PostgreSQL concatenation rule
    • Query database metadata if identifier ends up being longer than 63 bytes
  • Add SERIAL pseudo-type to column creation DDL to compliment idea of implicit sequence at DB creation/migrations level.
  • Update Insert and Update statements to filter out or generate values based on the changes
  • Consider making SequenceFeature independent from TableGateway since (1) it does not necesseirly tie to a specific table, (2) fix RowGateway limitation
  • Update Metadata TableGateway feature to recognize SERIAL columns and add SequenceFeature instances
  • Add MS SQL Server 2016 support
  • Stretch goal: add Oracle 12c implicit sequence support

No BC breaks.

Thought process history

While reviewing #172, helping with #162, and fresh out of a heavy database driven enterprise platform contract, noticed how deficient current sequence support is plus misleading function signatures. This is an important feature because, better than auto_increment in PostgreSQL, the only way to increment Oracle, and now Microsoft brought it in due to popular demand in SQL Server 2016 so I feel it needs detailed look.

Current situation is that:

  • TableGateway can only deal with a single sequence instance primarily for primary key usage, despite accepting unrestricted number of features.
  • As pointed out by @andrey-mokhov only supports explicit sequence naming. In some engines like Oracle, MSSQL2016 this is fine, but PostgreSQL supports auto-generated sequence names with SERIAL which is a problem.
  • Discovered at Primary keys (Sequence) not setting after save #191 that RowGateway is also incapable of handling sequences. Limitation is in fact that RowGateway does not link to any table gateway. Easy to add as an optional dependency by making constructor support TableGateway along with string and TableIdentifier OR add addition of features like other db classes, but hard in that INSERT operation on new record cannot be engine augnostic. May need to explicitly check for existence of that feature even if it breaks good OOP practice (or better suggestion).

What I have seen in the field:

Submitted PR #187 where I got around multiple features of same type problem. Intended usage would be (documented of course when settled one one)

$tableGateway = new TableGateway('table', $adapter, [new Sequence('pk_column', 'seq_1'), new Sequence('seq_2')]);
$tableGateway->nextSequenceId(); // no parameter aimed at 'seq_1' implying primary key
$tableGateway->nextSequenceId('seq_2'); // parameter specifying which one of the participating sequences are interested in

Why not always let developers manually handle Sequence objects without attaching them to table gateway? In large schemas, relationships set by architect can be difficult to keep in mind and I caused horrible bugs not being aware of what sequence participates in what business logic transaction. Using tablegateway to group them together helps a lot.

Now undecided on how to deal with preInsert/postInsert and dependence on gateway, how would I optionally mark one of several sequences that is used in TableGateway as responsible for PK? How would lastInsertValue be managed for multiple autoincremented columns?

alextech added a commit to alextech/zend-db that referenced this issue Nov 9, 2016
alextech added a commit to alextech/zend-db that referenced this issue Nov 9, 2016
@alextech alextech changed the title sequences support [WIP][RFC]sequences support Nov 20, 2016
alextech added a commit to alextech/zend-db that referenced this issue Dec 25, 2016
alextech added a commit to alextech/zend-db that referenced this issue Dec 25, 2016
alextech added a commit to alextech/zend-db that referenced this issue Dec 25, 2016
 Amendment to how SequenceFeatures are filtered from FeatureSet to match documentation. Since sequence names are not always known + more natural to interact with TableGateway using column names, filter function call by column names.
 Open to suggestion on how to improve this and filter somehow from FeatureSet *before* attempt to execute method on the feature (challenge to do so that does not break other feature types).

 Also, since SequenceFeature is more involved and requires long description, can consider moving to own docs page.
alextech added a commit to alextech/zend-db that referenced this issue Dec 25, 2016
alextech added a commit to alextech/zend-db that referenced this issue Dec 25, 2016
…er than 63 bytes for postgresql, query the actual name.
alextech added a commit to alextech/zend-db that referenced this issue Dec 25, 2016
alextech added a commit to alextech/zend-db that referenced this issue Dec 25, 2016
alextech added a commit to alextech/zend-db that referenced this issue Dec 25, 2016
alextech added a commit to alextech/zend-db that referenced this issue Dec 25, 2016
alextech added a commit to alextech/zend-db that referenced this issue Dec 25, 2016
alextech added a commit to alextech/zend-db that referenced this issue Dec 25, 2016
alextech added a commit to alextech/zend-db that referenced this issue Dec 25, 2016
@alextech
Copy link
Contributor Author

alextech commented Feb 5, 2017

Trying to consolidate conflicting sequence PRs reported over time in Zend DB. Stuck at deciding how to do platform specific code. Would like opinion of significantly more knowledgeable than I am individuals @froschdesign @Ocramius @weierophinney

Original code had switch/case statement per platform. Thats what bug reports focused on so I followed same pattern: https://github.com/zendframework/zend-db/pull/187/files#diff-85fe938f5a6f87443ad2578472eed9e0R166

But I just noticed there is duplication of that lastSequenceId() query in Connection class. Seems like a great benefit because do not need nasty switch($platform), and if anyone relied on similarly broken code in Connection too, all is fixed in one place.

https://github.com/zendframework/zend-db/blob/master/src/Adapter/Driver/Pgsql/Connection.php#L267

https://github.com/zendframework/zend-db/blob/master/src/Adapter/Driver/Pdo/Connection.php#L419

There are some concerns, however. What about nextSequenceId()? Connection interface does not have signature for it the way getLastGeneratedValue() is and rightfully so, I believe, should not be added. I could add nextSequenceId() to connection classes without adding to interface and assume dynamic typing will handle it, but that looks dirty. But code duplication looks dirty too, and so does inconsistency of doing switch/case for one, and call to adapter for the other method.

Another possible downside of using adapter connection, is getLastGeneratedValue() does not share same syntax for SqlServer 2016 where its one command for IDENTITY column but a different one for SEQUENCE object/column. (not a problem for postgresql or oracle up to 12c because sequence is the only way to do it)

Finally, how do I go about deducing implicit sequence names which too is a platform specific action? My getSequenceName() is awefully postgresql specific. I am not sure if decorator is possible to do for TableGateway features.

@alextech
Copy link
Contributor Author

alextech commented Feb 5, 2017

@alextech
Copy link
Contributor Author

@ezimuel while you are triaging issues, do you have a minute to give a hint about the last question, please?

@alextech
Copy link
Contributor Author

alextech commented Dec 7, 2017

Closing this in favor of keeping TODO and discussion in one place at linked PR.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

1 participant