Implement the PostgreSQL Streaming Replication Protocol by Brar · Pull Request #3281 · npgsql/npgsql

Brar · 2020-10-29T01:31:32Z

This type was added in PostgreSQL 9.4 and we didn't handle it yet. While a type handler and a specific type isnt't strictly necessary for the implementation of replication in Npgsql it will probably be useful once people start using the replication feature and want to query their database regading replication related information like pg_replication_slots.

There was a very old ToDo-comment in the code because TimestampHandler and TimestampTzHandler were passing values through NpgsqlDateTime in order to convert them from/to the backend representation. For Replication we need to convert timestamps in a completely separate part of the codebase so we change it now and benefit from it multiple times.

Brar · 2020-10-29T17:00:04Z

@ Reviewers: Please don't bother investigating failing tests for now. I know about their flakiness and I expect to remove/disable most of them in the course of the review process.
For now they serve as documentation on how things are supposed to work and as regression tests to avoid accidentally breaking something when I integrate requested changes from your reviews.

Also I'll add one additional commit to stop the timers when cancelling (it was on my todo-list but somehow I forgot it last night).

roji

Here's a first partial round - I've gone through about half the files (I think), and it looks great. I've made lots of comments, but marked certain ones as IMPORTANT - anything without that can probably be ignored for now (up to you).

You should probably add yourself to CODEOWNERS for the appropriate paths 🤣

test/Npgsql.Tests/Replication/SafeReplicationTestBase.cs

test/Npgsql.Tests/Replication/CommonLogicalReplicationTests.cs

test/Npgsql.Tests/Replication/SafeReplicationTestBase.cs

test/Npgsql.Tests/Replication/PgOutputReplicationTests.cs

src/Npgsql/Replication/Logical/Protocol/NpgsqlLogicalReplicationConnectionExtensions.cs

…onConnectionExtensions.cs

If wal_sender_timeout is more than 3 seconds. We don't want this test to inexplicably hang for 60s (by default).

…e fail

src/Npgsql/Replication/NpgsqlReplicationConnection.cs

src/Npgsql/Replication/Logical/Protocol/NpgsqlLogicalReplicationConnectionExtensions.cs

src/Npgsql/Replication/Logical/Internal/NpgsqlLogicalReplicationConnectionExtensions.cs

src/Npgsql/Replication/NpgsqlReplicationConnection.cs

src/Npgsql/Replication/TestDecoding/NpgsqlTestDecodingPluginOptions.cs

src/Npgsql/Replication/Internal/LogicalReplicationConnectionExtensions.cs

This reverts commit 3be4e28.

Also just DRY a bit

roji · 2020-11-08T14:47:58Z

From my side, we're very close to merging this for 5.0. Some notes:

The main remaining thing is to get the tests to a stable green situation, one way or another (by identifying and fixing the issues or by skipping specific/problematic tests).
Some minor test cleanup comments have been scoped out to Minor replication test cleanup tasks #3302 so we can forget about them
There are some unresolved comments above which I think we should at least look at before releasing.
~~Use the regular user action mechanism for state management Brar/npgsql#11 still needs to be reviewed/merged.~~

Previous commit was a test cleanup

src/Npgsql/Replication/PgOutput/PgOutputReplicationOptions.cs

roji

Really, really great to finally see this go in - and I'm very happy with the end results. Thanks for all the great work @Brar.

jberzy · 2020-11-08T20:02:05Z

This is fantastic! Thanks @Brar and @roji

Emill · 2021-04-11T18:05:01Z

src/Npgsql/NpgsqlReadBuffer.cs

+            async ValueTask<string> ReadLong(string s)
+            {
+                var builder = new StringBuilder(s);
+                bool complete;
+                do
+                {
+                    await ReadMore(async, cancellationToken);
+                    complete = ReadFromBuffer(out s);
+                    builder.Append(s);
+                }
+                while (!complete);
+
+                return builder.ToString();
+            }


Maybe a bit late, but I think this code will fail if the split is in the middle of a unicode character that uses more than 1 byte to represent. Better Ensure the max length of a string is already present in the buffer (as it was previously), or create a temporary byte buffer with all the data which you then convert to a string in one go using the encoding.

You're technically right... However, note that this is for reading null-terminated strings, which AFAIK are only used for certain short ASCII control strings, rather than actual user data - so I doubt there's a real bug here. But you can open a new issue to at least have it tracked.

BTW if we do want to preemptively fix this, then rather than entirely buffering the strings them in a temporary buffer, we can use System.Text.Decoder to incrementally load more chunks a decode. Note that we haven't even done this optimization for actual user strings (which are probably much more important, see TextHandler.ReadLong).

At least in the replication protocol they are used for user data as well, for example table names, which can contain non-ascii characters.

I don't see why System.Text.Decoder would be anything better. It gives you some chars on every iteration. These chars are temporary anyway and must then be made into a string, after which the previously buffered chars are discarded. But generally a char[] takes up more memory than the corresponding byte[] in UTF8, so I think it's better to just buffer everything as bytes. The encoder can then convert these to a string in-place, with no extra temporary space.

You're right... IIRC that's why I didn't do it in TextHandler either.

Open an issue for us to fix this?

Brar added 3 commits October 28, 2020 18:01

Implement the PostgreSQL Streaming Replication Protocol

8cdacfa

Brar requested review from YohDeadfall and roji as code owners October 29, 2020 01:31

Disable feedback timers when cancelling

53e75ff

roji reviewed Oct 31, 2020

View reviewed changes

src/Npgsql/Replication/Logical/Protocol/NpgsqlLogicalReplicationConnectionExtensions.cs Outdated Show resolved Hide resolved

roji reviewed Oct 31, 2020

View reviewed changes

src/Npgsql/Replication/Logical/Protocol/NpgsqlLogicalReplicationConnectionExtensions.cs Outdated Show resolved Hide resolved

roji reviewed Oct 31, 2020

View reviewed changes

src/Npgsql/Replication/Logical/Protocol/NpgsqlLogicalReplicationConnectionExtensions.cs Outdated Show resolved Hide resolved

roji reviewed Oct 31, 2020

View reviewed changes

src/Npgsql/Replication/Logical/Protocol/NpgsqlLogicalReplicationConnectionExtensions.cs Outdated Show resolved Hide resolved

roji added 15 commits November 1, 2020 11:45

Remove [PublicAPI]

8e49da7

Update test/Npgsql.Tests/Replication/SafeReplicationTestBase.cs

8eb4de6

Update src/Npgsql/Replication/NpgsqlReplicationConnection.cs

f44caa7

Small fixes and nits

72f04b4

Simplify some test names

62e25cf

Tweak name setup in tests to use [CallMemberName] etc.

d31df95

DRY some PG version checks in tests

dbf3e36

Add some missing cancellation token checks

71d4e55

Update src/Npgsql/Replication/Logical/Internal/NpgsqlLogicalReplicati…

baf6c85

…onConnectionExtensions.cs

Remove some needed cancellation token checks

99a3e4f

Cleanup of LSN

4aae8c4

Make cancellation token mandatory for StartReplication

781ee73

Ignore ReplicationSurvivesPausesLongerThanWalSenderTimeout except on CI

c760050

If wal_sender_timeout is more than 3 seconds. We don't want this test to inexplicably hang for 60s (by default).

Add some more PG config checks for skipping tests that would otherwis…

1147c74

…e fail

Correct mess done in last commit

2399af0

roji reviewed Nov 1, 2020

View reviewed changes

roji added 2 commits November 1, 2020 20:09

Refactor StartReplication methods

1ae6d3b

Read from IAsyncEnumerable directly in tests

6848312

Brar commented Nov 6, 2020

View reviewed changes

src/Npgsql/Replication/TestDecoding/NpgsqlTestDecodingPluginOptions.cs Outdated Show resolved Hide resolved

roji and others added 6 commits November 6, 2020 17:17

Fix tiny namespace confusion

89f32f5

Fixup

873fc42

Remove Npgsql prefix everywhere

1908979

Add constructors with connection strings to connections

f53b581

Fixup

4c1da9f

Fix StringBuilder.AppendJoin() missing in .NET Standard 2.0

416d414

roji reviewed Nov 7, 2020

View reviewed changes

src/Npgsql/Replication/Internal/LogicalReplicationConnectionExtensions.cs Show resolved Hide resolved

roji added 8 commits November 8, 2020 09:13

And remove another unused thing

3be4e28

Revert "And remove another unused thing"

a3ed79e

This reverts commit 3be4e28.

Timer concurrency logic

c781f9d

Disable cancel-on-dispose for now

110ec52

Implement proper cancel-on-dispose

38d2238

Fix up some silliness

6dd1fc3

Provide more information when cancellation fails in tests

324fc8b

Also just DRY a bit

Work on logical plugin options

744e7b9

roji mentioned this pull request Nov 8, 2020

Minor replication test cleanup tasks #3302

Open

4 tasks

roji added 3 commits November 8, 2020 16:57

Rename OpenAsync to Open

cba3374

Actually rename OpenAsync to Open

405f0e2

Previous commit was a test cleanup

Use the regular user action mechanism for state management

3226d14

Brar commented Nov 8, 2020

View reviewed changes

src/Npgsql/Replication/PgOutput/PgOutputReplicationOptions.cs Show resolved Hide resolved

roji added 2 commits November 8, 2020 20:43

Make physical replication tests explicit for now

faf49e6

Skip empty transactions in tests

6afa695

roji approved these changes Nov 8, 2020

View reviewed changes

Add @Brar as replication code owner

43fbc68

Brar merged commit 5a3b020 into npgsql:main Nov 8, 2020

Emill reviewed Apr 11, 2021

View reviewed changes

Emill mentioned this pull request Apr 12, 2021

ReadNullTerminatedString might fail for non-ascii characters #3660

Closed

Conversation

Brar commented Oct 29, 2020

Uh oh!

Brar commented Oct 29, 2020

Uh oh!

roji left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

roji commented Nov 8, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

roji left a comment

Choose a reason for hiding this comment

Uh oh!

jberzy commented Nov 8, 2020

Uh oh!

Emill Apr 11, 2021

Choose a reason for hiding this comment

Uh oh!

roji Apr 12, 2021

Choose a reason for hiding this comment

Uh oh!

Emill Apr 12, 2021

Choose a reason for hiding this comment

Uh oh!

roji Apr 12, 2021

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

roji commented Nov 8, 2020 •

edited

Loading