Support for logical streaming replication#1271
Conversation
| params.push("dbname='" + this.database + "'"); | ||
| } | ||
| if(this.replication) { | ||
| params.push("replication='" + (this.database === true ? "true" : this.replication) + "'"); |
There was a problem hiding this comment.
"Replication" can have "database", "true" and "false" values.
This is checking this.database === true, though – not this.replication === true. Is that really intended? I’m not sure why dbname='true' (not replication='true') would have any special meaning beyond a database named “true”.
There was a problem hiding this comment.
Oh, sorry. I understand. You are right.
I'll change that code
There was a problem hiding this comment.
I changed this issue in last commit. Check please :)
| params.push("dbname='" + this.database + "'"); | ||
| } | ||
| if(this.replication) { | ||
| params.push("replication='" + (this.replication === true ? "true" : this.replication) + "'"); |
There was a problem hiding this comment.
Since true stringifies to "true", this can be "replication='" + this.replication + "'".
There was a problem hiding this comment.
I push the commit that accepted your advice :)
| data.application_name = appName; | ||
| } | ||
| if (params.replication) { | ||
| data.replication = params.replication === true ? 'true' : params.replication; |
There was a problem hiding this comment.
Similarly: '' + params.replication
charmander
left a comment
charmander
left a comment
There was a problem hiding this comment.
Some tests might be requested, but these changes look right to me.
|
Hi @brianc , |
|
@kibae I'm excited for you to get your module out into the wild!! Don't forget to add it to the wiki! I'll go ahead & push a minor version bump right now with the changes in this PR. |
|
published @ 6.2.0! |
|
@brianc I changed dependencies of pg to 6.2.0 npm ERR! notarget No compatible version found: pg@6.2.0 |
|
@brainc It looks like 6.1.5 in npm yet. Can you check it? |
|
@brianc @charmander Please publish to npm 6.2.0~ |
3 participants
Hello, @brianc @charmander
Please check the codes associated with this request. #1254
After merging, I will create a new project for logical replication.
Thanks