Skip to content
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Streaming Replication - 1st] Introduce new methods and getter to PostgreSQLConnection #51

Merged
merged 6 commits into from
Sep 9, 2022

Conversation

osaxma
Copy link
Contributor

@osaxma osaxma commented Sep 9, 2022

This PR adds functionality that's necessary for Streaming Replication.

The following will be the additions to PostgreSQLConnection:

  • querySimple method

    • Similar to execute, but it'll return whatever data the server gives.
    • This is necessary since in streaming replication mode, the extended query protocol cannot be used. On the other hand, execute does not return any data. querySimple was introduced to avoid breaking the existing API and to allow querying the database in Replication Mode which is necessary (i.e. executing IDENTIFY_SYSTEM; and retrieving the system info data)
  • addMessage method

    • I assumed since the socket is already publicly exposed, there's no harm of adding this method.
  • Stream<ServerMessage> messages

    • Since socket is a single listener stream, there's no way to listen to server messages after opening the connection.
    • The implementation is similar to notifications except that it'll pass all messages to any listeners.
    • In combination with addMessage, the user can take over the communication with the server.
    • This is necessary to respond to server messages in replication mode as the connection will always be in a busy state (i.e. after running START_REPLICATION)

p.s. I also added .vscode in .gitignore.

This is similar to execute but it'll return any
results received from the server
Gives the ability for the user to send `ClientMessage`s
to the server
Gives users the ability to listen to all server messages
@osaxma osaxma changed the title [Streaming Replication - 1st] Introduce necessary components [Streaming Replication - 1st] Introduce new methods and getter to PostgreSQLConnection Sep 9, 2022
lib/src/connection.dart Show resolved Hide resolved
lib/src/connection.dart Show resolved Hide resolved
///
/// This method is similar to [execute] except that it'll return all the data.
/// That is, when the query result contains data, the method will return a
/// [PostgreSQLResult]. If the query does not return data, then the affected row
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm wondering: wouldn't it be better to extend PostgreSQLResult with affectedRowCount? That way we don't need to introduce yet another query type, and still preserve the full information.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The main thing that I tried to avoid here is changing the return type of execute from int to PostgreSQLResult as that would be a breaking change.

If you think it's fine to change the return type of execute, I could that.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, changing execute would be breaking, and I would leave that to a future breaking change release. However, extending query's PostgreSQLResult return value would be non-breaking. Would it work?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I haven't thought about it this way... I think it'd and it'd be cleaner.

Tis means that the query function will have a new optional parameters called useSimpleQuery which internally means "use the simple query protocol instead of the extended one".. I'll give it a try now.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm genuinely clueless about this part of the code, but couldn't we use the extended protocol as is used in query?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, PostgreSQL does not allow using the Extended Query Protocol when the connection is established for a Streaming Replication Protocol.

When using query during a replication mode, the database will actually throw the following error:

ERROR. ERROR. 08P01. extended query protocol not supported in a replication connection. postgres.c. 4749. forbidden_in_wal_sender. 

which is how I figured this part out...

So the only option is to use the Simple Query Protocol which is the one used by execute.

Do you think changing the query definition to the following is fine:

  Future<PostgreSQLResult> query(String fmtString,
      {Map<String, dynamic>? substitutionValues,
      bool? allowReuse,
      int? timeoutInSeconds,
      bool? useSimpleQueryProtocol});

I'll expand the docs for useSimpleQueryProtocol. Also, I think spelling out useSimpleQueryProtocol is better than simpleQuery (so no one think simple query is faster).

Please let me know what you think.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the explanation!
I think the query with the bool? useSimpleQueryProtocol would seem the best compromise for now. Let's go with that! Thank you!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated. I created an _execute method that returns PostgreSQLResult which can replace execute in the future (I added a TODO comments for that).

@isoos isoos merged commit d877ca9 into isoos:master Sep 9, 2022
@osaxma osaxma deleted the replication_split_1 branch September 10, 2022 09:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants