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

Provide access to Postgres replication #2308

Open
wants to merge 5 commits into
base: dev
from

Conversation

@SeanFarrow
Copy link

commented Jan 30, 2019

Scoped out an API to manage and fluently build replication slots.

Copy link
Member

left a comment

Some questions related to naming and enum/generics design choices. Overall, an interesting proposal!

/// Ensure the <see cref="ReplicationSlotInfo"/> built by this <see cref="ReplicationSlotInfoBuilder"/> indicates the built replication slot is available for the current session only.
/// </summary>
/// <returns>A <see cref="ReplicationSlotInfoBuilder"/> for further configuration.</returns>
public ReplicationSlotInfoBuilder SlotIsTemporary()

This comment has been minimized.

Copy link
@austindrenski

austindrenski Jan 30, 2019

Member

For these two, why not name as Is* rather than SlotIs*?

/// Ensure the <see cref="ReplicationSlotInfo"/> built by this <see cref="ReplicationSlotInfoBuilder"/> indicates the built replication slot uses physical replication.
/// </summary>
/// <returns>A <see cref="PhysicalReplicationSlotInfoBuilder"/> for further configuration.</returns>
public PhysicalReplicationSlotInfoBuilder UsesPhysicalReplication()

This comment has been minimized.

Copy link
@austindrenski

austindrenski Jan 30, 2019

Member

What about EnsurePhysical() and EnsureLogical()?

/// <summary>
/// Used to build a <see cref="ReplicationSlotInfo"/> object using a fluent API.
/// </summary>
public class ReplicationSlotInfoBuilder

This comment has been minimized.

Copy link
@austindrenski

austindrenski Jan 30, 2019

Member

Should this be abstract?

/// A replication slot creator.
/// </summary>
/// <typeparam name="TSlotType">The replication slot type to use for creation.</typeparam>
public interface IReplicationSlotCreator<TSlotType> where TSlotType: ReplicationSlotInfo

This comment has been minimized.

Copy link
@austindrenski

austindrenski Jan 30, 2019

Member

Does the interface need to be in a nested namespace?

@@ -0,0 +1,9 @@
namespace Npgsql.Replication.Slots.Common

This comment has been minimized.

Copy link
@austindrenski

austindrenski Jan 30, 2019

Member

Does anything live in the .Slots namespace? If not, could .Common be merged upward?

/// <param name="slotName">The name of the slot for wich to retrieve information.</param>
/// <param name="slotType">The <see cref="ReplicationSlotType"/> on which to filter.</param>
/// <returns></returns>
public ReplicationSlotInfo GetSlotByName(string slotName, ReplicationSlotType slotType)

This comment has been minimized.

Copy link
@austindrenski

austindrenski Jan 30, 2019

Member

Why not just GetByName()?

/// </summary>
/// <param name="slotToCreate">Information pertaining to the replication slot to create.</param>
///<exception cref="ArgumentNullException">Thrown when the <paramref name="slotToCreate"/> is <c>Null</c></exception>
public void CreateSlot(ReplicationSlotInfo slotToCreate)

This comment has been minimized.

Copy link
@austindrenski

austindrenski Jan 30, 2019

Member

Why not just Create()?

/// <summary>
/// Enum describing the replication slot types.
/// </summary>
public enum ReplicationSlotType

This comment has been minimized.

Copy link
@austindrenski

austindrenski Jan 30, 2019

Member

Wouldn't this just be described by the type hierarchy? Any benefit to the added type?

/// </summary>
/// <param name="slotType">The <see cref="ReplicationSlotType"/> on which to filter.</param>
/// <returns>An <see cref="IReadOnlyCollection{ReplicationSlotInfo}"/> containing details of all available replication slots of the <paramref name="slotType"/> type.</returns>
public IReadOnlyCollection<ReplicationSlotInfo> GetSlots(ReplicationSlotType slotType)

This comment has been minimized.

Copy link
@austindrenski

austindrenski Jan 30, 2019

Member

Why not implement this with properties?

/// <param name="slotName">The name of the slot for which to check existence.</param>
/// <param name="slotType">The <see cref="ReplicationSlotType"/> on which to filter.</param>
/// <returns><c>True</c> if a slot of the specified <see cref="ReplicationSlotType"/> and with the specified <paramref name="slotName"/> exists, <c>False</c> otherwise.</returns>
public bool Exists(string slotName, ReplicationSlotType slotType)

This comment has been minimized.

Copy link
@austindrenski

austindrenski Jan 30, 2019

Member

If you can forgo the enum, this could become a generic method constrained over the base type.

@SeanFarrow

This comment has been minimized.

Copy link
Author

commented Jan 30, 2019

@austindrenski
I’ll change the two IsSlotXX method names on the ReplicationInfoBuilder class.
I agree Ensure* makes more sense.
I don’t see how it can be abstract, given that it needs to be enstantiated.
Not necessarily, but I’ve done this for code organisation, also, there will be implementations of this interface, so thought this made sense.
Happy to merge Common upwards.
Will rename all CRUD methods on the ReplicationSlotManager.
Will rename the CreateSlot method.
Will rename the GetSlotByName method.
There is a benefit to the ReplicationSlotType enum, it allows you to ask for all slots or slots of a particular type.
We could and have a LogicalSlots and a PhysicalSlots property, it's down to preference I guess.
I'm not sure what you mean by making the Exists method generic in the base type.

@austindrenski

This comment has been minimized.

Copy link
Member

commented Jan 30, 2019

@SeanFarrow For clarity of discussion, could you please respond to each comment via the GitHub web interface?

@SeanFarrow

This comment has been minimized.

Copy link
Author

commented Jan 30, 2019

@austindrenski,

Not really, I'm totally blind and it doesn't work as well as it should with access technology, sorry!

@austindrenski

This comment has been minimized.

Copy link
Member

commented Jan 30, 2019

@SeanFarrow Oh, my apologies, that makes complete sense! I'll take a closer look at your responses and respond in a bit.

@roji

This comment has been minimized.

Copy link
Member

commented Feb 8, 2019

@SeanFarrow thanks for submitting this and sorry it took me so long to take a look.

At its present state, this PR looks like its modeling the CREATE_REPLICATION_SLOT command of the streaming replication protocol (along with DROP_REPLICATION_SLOT, and there seems to also be an Exists() method in your API but I can't see a corresponding command in the protocol).

Looking at the protocol and its commands, I'm not sure a fluent design with a type hierarchy for representing physical vs. logical is necessary here. First, based on the protocol docs, the decision on logical vs. physical replication is made at connection-time - it doesn't seem possible for the same connection to do both, nor to switch a connection from one mode to the other. As such, I'd expect us to have two distinct NpgsqlPhysicalReplicationConnection/NpgsqlLogicalReplicationConnection types, each allowing only what is possible in that mode. In your current proposal, it seems like it would be possible to attempt to create a logical replication slot on a physical connection and vice versa, triggering a runtime error - it would be better design to prevent this at compile-time instead.

Once we have these two types, a simple CreateReplicationSlot() method with the appropriate parameters seems sufficient, rather than a full-blown builder architecture with fluent APIs - simply to keep things light. In general, I'd expect the replication connection types to simply wrap the PostgreSQL protocol commands with corresponding .NET methods, which look and behave as closely as possible to the their protocol equivalents.

Finally, there's the matter of what these replication connection types share with each other (depending on the extent to which physical and logical replication protocols overlap), and what these types share with the regular NpgsqlConnection (e.g. socket connection, authentication...). It's totally fine that you didn't start there - but we'll have to tackle that sooner or later.

@SeanFarrow

This comment has been minimized.

Copy link
Author

commented Feb 8, 2019

@roji

This comment has been minimized.

Copy link
Member

commented Feb 8, 2019

The reasoning behind having an exists method was to allow the user to check whether a slot exists before performing any operation that may use it.

I understand, it's just that I don't see any command in the protocol that would do that... Am I missing it?

If we switch to separate connection types (which I have no problem with), do we want interfaces, my feeling is yes, so that things can be mocked, should people so desire. Also, do we want the ability to create a physical/logical replication connection from an existing connection, using extension methods?

This question has come up from time to time (albeit quite rarely) with regards to Npgsql types (e.g. NpgsqlConnection). If we do this, we should probably tackle it for all of Npgsql and not just for replication, so let's leave it out of scope for now.

I’ll be working on this next week, so should have updated code for you on Tuesday, at least at an API level.

Great! I'm looking forward to reviewing your work.

@SeanFarrow

This comment has been minimized.

Copy link
Author

commented Feb 8, 2019

@roji

This comment has been minimized.

Copy link
Member

commented Feb 9, 2019

Your not missing it, there’s nothing directly in the protocol, replication slots are made available via a view.

Oh, I see what you mean - you were planning on querying the view for the user. It's definitely doable, I'm just more in a mindset where we expose PostgreSQL functionality to the user, rather than provided added services of our own on top of it. In other words, the user can easily query the view themselves and our added value here is very limited (if any). It seems better for all our methods here to directly map to protocol commands - it keeps everything very clean and consistent.

But everything is up for discussion. I still suggest that we start with the base functionality (protocol commands), and then we can always discuss adding stuff on top of that.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.