Add an option to the PostgreSQL helper to not reset sequences #38
Conversation
There was a problem hiding this comment.
Hi @dgsb,
Can you explain a bit more about your use case?
Resetting the sequences will prevent errors on your tests, and shouldn't have any side effects.
| @@ -16,6 +16,10 @@ type PostgreSQL struct { | |||
| // which requires SUPERUSER privileges. | |||
| UseAlterConstraint bool | |||
|
|
|||
| // DontResetSequences prevents the reset of the databases | |||
There was a problem hiding this comment.
| // DontResetSequences prevents the reset of the databases | |
| // SkipResetSequences prevents the reset of the databases |
postgresql.go
Outdated
| @@ -16,6 +16,10 @@ type PostgreSQL struct { | |||
| // which requires SUPERUSER privileges. | |||
| UseAlterConstraint bool | |||
|
|
|||
| // DontResetSequences prevents the reset of the databases | |||
| // sequences after load fixtures time | |||
| DontResetSequences bool | |||
There was a problem hiding this comment.
| DontResetSequences bool | |
| SkipResetSequences bool |
There was a problem hiding this comment.
I choose this negative logic so that the default value keep a compatible backward behaviour. But in the end its your call.
postgresql.go
Outdated
| @@ -217,6 +221,9 @@ func (h *PostgreSQL) makeConstraintsDeferrable(db *sql.DB, loadFn loadFunction) | |||
| func (h *PostgreSQL) disableReferentialIntegrity(db *sql.DB, loadFn loadFunction) (err error) { | |||
| // ensure sequences being reset after load | |||
| defer func() { | |||
| if h.DontResetSequences { | |||
There was a problem hiding this comment.
We should actually avoid deferring this whole function if sequences resetting is being skipped:
if !h.SkipResetSequences {
defer func() {
// ...
}()
}There was a problem hiding this comment.
see my previous comment. I think its better to keep the behaviour backward compatible.
There was a problem hiding this comment.
@dgsb Sorry, there was a typo in the snippet above. It should be:
if !h.SkipResetSequences {Instead of:
if h.SkipResetSequences {The value is still negated (and backwards compatible). I just suggested renaming it from Dont to Skip
There was a problem hiding this comment.
ok, I read to fast your answer
andreynering
changed the title
|
CI should be fixed if you update with master |
|
@andreynering I will resync with master in order to have a working CI. |
|
@dgsb Sorry for taking long to respond. Since it doesn't hurt to allow skipping reseting of sequences, I'll accept this patch. But the truth is that this problem shouldn't happen. If you have more info about your issue, let me know. |
|
@andreynering I will try to setup a minimalistic reproduction tests. I've opened #39 to not forget to identify the root cause of the problem. |
We do not always want to reset the sequence especially then they are used as primary key in some table.