- Notifications
You must be signed in to change notification settings - Fork35
Replication slots#41
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.
By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
codecov-io commentedMar 13, 2018 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
Codecov Report
@@ Coverage Diff @@## master #41 +/- ##==========================================+ Coverage 97.02% 97.27% +0.24%========================================== Files 16 16 Lines 1245 1356 +111 ==========================================+ Hits 1208 1319 +111 Misses 37 37
Continue to review full report at Codecov.
|
testgres/node.py Outdated
| ).format(slot_name) | ||
| self.execute(query=query, | ||
| dbname=dbname or default_dbname(), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
You don't have to explicitly usedefault_dbname() and such sinceexecute() method is able to handleNone args properly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Agree
testgres/node.py Outdated
| u"log_statement = {}\n" | ||
| u"listen_addresses = '{}'\n" | ||
| u"port = {}\n" | ||
| u"max_replication_slots = {}\n".format(log_statement, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
IMHOmax_replication_slots should be placed underallow_streaming=True, next tomax_wal_senders etc.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Agree as well
testgres/node.py Outdated
| @@ -856,7 +863,24 @@ def backup(self, **kwargs): | |||
| return NodeBackup(node=self, **kwargs) | |||
| def replicate(self, name=None, **kwargs): | |||
| def create_replication_slot(self, slot_name, dbname=None, username=None): | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Should we allow user to choose his ownslot_name? If not, it's better to inline this method.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
If we don't allow this, then it could contradict with logical replication interface (#42) which allows user to choose the subscriber name (which implicitly creates logical replication slot with the same name) voluntarily.
Speaking of inlining, I can callcreate_replication_slot() fromreplicate() ifslot_name != None so that user won't need to call it explicitly. It would be more convenient for user.
funbringerMar 20, 2018 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
@zilder You're right, of course. Maybe we'll add a default name generator later.
Speaking of inlining, I can call create_replication_slot() from replicate() if slot_name != None
Sure, that would be nice. It's better to check if slot exists, though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
It's better to check if slot exists, though.
True
Please make sure that code won't be rewritten by |
No description provided.