Detect and upgrade from macaddr to macaddr8

[austindrenski]

Background

While working on #407, I'm finding it difficult to send PhysicalAddress parameters to PostgreSQL functions that expect a macaddr8 because the default mapping is for macaddr.

Based on the docs, it looks like these types are intended to interoperate smoothly:

This type can accept both 6 and 8 byte length MAC addresses and stores them in 8 byte length format. MAC addresses given in 6 byte format will be stored in 8 byte length format with the 4th and 5th bytes set to FF and FE, respectively.

This would normally be handled by setting a different CLR type as the default mapping for macaddr8. However, since PhysicalAddress can accommodate the 8-byte width, I would like to avoid introducing another provider-specific type just for the new width.

Question

Is it be possible for MacaddrHandler to forward to Macaddr8Handler on write operations when a PhysicalAddress parameter has a byte length of 8?

Example

public class NetTestEntity
{
 // This is the default mapping.
 [Column(TypeName = "macaddr")]
 public PhysicalAddress Macaddr { get; set; }

 [Column(TypeName = "macaddr8")]
 public PhysicalAddress Macaddr8 { get; set; }
}

// Passes
[Fact]
public void PhysicalAddress_macaddr_LessThan_macaddr()
{
 using (NetContext context = Fixture.CreateContext())
 {
 PhysicalAddress macaddr = new PhysicalAddress(new byte[6]);

 NetTestEntity[] _ =
 context.NetTestEntities
 .Where(x => EF.Functions.LessThan(x.Macaddr, macaddr))
 .ToArray();

 AssertContainsSql("WHERE (x.\"Macaddr\" < @__macaddr_1) = TRUE");
 }
}

// Passes
[Fact]
public void PhysicalAddress_macaddr8_LessThan_macaddr8()
{
 using (NetContext context = Fixture.CreateContext())
 {
 NetTestEntity[] _ =
 context.NetTestEntities
 .Where(x => EF.Functions.LessThan(x.Macaddr8, x.Macaddr8))
 .ToArray();

 AssertContainsSql("WHERE (x.\"Macaddr8\" < x.\"Macaddr8\") = TRUE");
 }
}

// Fails:
//
// System.FormatException : MAC addresses must have length 6 in PostgreSQL
// at Npgsql.TypeHandlers
// .NetworkHandlers
// .MacaddrHandler
// .ValidateAndGetLength(PhysicalAddress value, NpgsqlParameter parameter)
[Fact]
public void PhysicalAddress_macaddr8_LessThan_macaddr8_parameter()
{
 using (NetContext context = Fixture.CreateContext())
 {
 PhysicalAddress macaddr8 = new PhysicalAddress(new byte[8]);

 NetTestEntity[] _ =
 context.NetTestEntities
 .Where(x => EF.Functions.LessThan(x.Macaddr8, macaddr8))
 .ToArray();

 AssertContainsSql("WHERE (x.\"Macaddr8\" < @__macaddr8_1) = TRUE");
 }
}

[roji]

Well, in the current type handler, the PostgreSQL type actually sent isn't controlled by the type handler, but rather by the type mapper, and we never make decisions on which PostgreSQL type to send based on the contents of the CLR type - only based on the CLR type itself.

Note that before 4.0.0, we did have a hack that checked DateTime.Kind, if it was UTC we sent the timestamp with time zone PostgreSQL type. For other DateTime.Kind we sent timestamp without time zone. This has been removed in #1940, so now decisions on which PostgreSQL type to send are purely based on the CLR type.

Can you elaborate a bit more on what difficulties you're encountering? PhysicalAddress is by default mapped to macaddr, but you can always specify NpgsqlDbType.Macaddr8 to explicitly select macaddr8 instead.

Finally, we do have the possibility of changing the default mapping of PhysicalAddress to macaddr8 - PostgreSQL implicitly converts macaddr8 to macaddr (when the value has 6 bytes), so this wouldn't break existing code. The only disadvantages seems to be that macaddr8 was only introduced in PostgreSQL 10, so users of older versions would have to always specify NpgsqlDbType.Macaddr to avoid the new type. It seems a bit soon to do this - although we can definitely do it at some point. Thoughts?

[roji]

On the EF Core side, explicitly setting the column's type to macaddr8 should make the provider set NpgsqlDbType properly, and everything should be fine. What exactly isn't working?

[austindrenski]

Apologies for the noise from the commit reference. I didn't realize GitHub would show a notification for every rebase + force push. Next time that PR is updated, I'll unlink the commit reference.

we never make decisions on which PostgreSQL type to send based on the contents of the CLR type - only based on the CLR type itself.

In the general, I agree with this approach.

Can you elaborate a bit more on what difficulties you're encountering? PhysicalAddress is by default mapped to macaddr, but you can always specify NpgsqlDbType.Macaddr8 to explicitly select macaddr8 instead.

The difficulty is when the default mapping is PhysicalAddress to macaddr, but the parameter I'm sending is logically a macaddr8 (e.g. not strictly 6 bytes).

// create a value intended as a macaddr8
PhysicalAddress macaddr8 = new PhysicalAddress(new byte[8]);

// try to use the parameter with a column that is explicitly mapped as macaddr8
NetTestEntity[] _ =
 context.NetTestEntities
 .Where(x => EF.Functions.LessThan(x.Macaddr8, macaddr8))
 .ToArray();

// throws:
// System.FormatException : MAC addresses must have length 6 in PostgreSQL
// at Npgsql.TypeHandlers
// .NetworkHandlers
// .MacaddrHandler
// .ValidateAndGetLength(PhysicalAddress value, NpgsqlParameter parameter)

On the EF Core side, explicitly setting the column's type to macaddr8 should make the provider set NpgsqlDbType properly, and everything should be fine. What exactly isn't working?

This works as you describe. Columns are mapped as expected, but sending an 8-byte PhysicalAddress while the default mapping is set to macaddr results in an exception being thrown by the type handler.

This seems to go against Npgsql's standard approach of delegating validation to PostgreSQL.

Finally, we do have the possibility of changing the default mapping of PhysicalAddress to macaddr8 - PostgreSQL implicitly converts macaddr8 to macaddr (when the value has 6 bytes), so this wouldn't break existing code. The only disadvantages seems to be that macaddr8 was only introduced in PostgreSQL 10, so users of older versions would have to always specify NpgsqlDbType.Macaddr to avoid the new type. It seems a bit soon to do this - although we can definitely do it at some point. Thoughts?

I think it would be reasonable to keep macaddr as the default mapping until PosgreSQL 11 is released. At that point macaddr8 will have been available for the duration of a major release and the burden on users of older versions seems more defensible.

[roji]

Sorry for not responding for a while, times are busy.

I understand the issue now: your problem is that your your 8-byte parameter (not the column) is being sent as macaddr, and so gets rejected by PostgreSQL. This is a more general issue that can happen for other types: EF Core needs to have a mechanism to allows explicitly specifying the store type of a value in an expression. I opened dotnet/efcore#11921 for this a while ago (superceded by the more general dotnet/efcore#4978).

At the EF Core level there's no way to make mappings depend on values, only on types. At the Npgsql ADO level I'm really reluctant to add back value-based mapping decisions because of a EF Core shortcoming that is likely to be fixed soon... And especially since this isn't likely to affect too many users. Note also that literal generation - putting the new PhysicalAddress(...) inside the expression - should work fine, since it's rendered into a string client-side.

Because of the above I suggest moving forward with your PR and leaving this issue as out of scope - let me know what you think.

This seems to go against Npgsql's standard approach of delegating validation to PostgreSQL.

What does this refer to exactly, I didn't understand...

I think it would be reasonable to keep macaddr as the default mapping until PosgreSQL 11 is released. At that point macaddr8 will have been available for the duration of a major release and the burden on users of older versions seems more defensible.

I totally agree.

[roji]

@austindrenski any comment on the above?

[austindrenski]

@roji Thanks for those issue references, interesting to see the related work!

Because of the above I suggest moving forward with your PR and leaving this issue as out of scope - let me know what you think.

That works for me.

What does this refer to exactly, I didn't understand...

That's my fault, I wasn't very clear. I was thinking about the length checks in MacaddrHandler:

// from MacaddrHandler.cs
public override int ValidateAndGetLength(PhysicalAddress value, NpgsqlParameter parameter)
 => value.GetAddressBytes().Length == 6
 ? 6
 : throw new FormatException("MAC addresses must have length 6 in PostgreSQL");


public override void Write(PhysicalAddress value, NpgsqlWriteBuffer buf, NpgsqlParameter parameter)
 => buf.WriteBytes(value.GetAddressBytes(), 0, 6);

If this code didn't check the length, would that allow PostgreSQL to choose to either reject the macaddr/macaddr8 as an invalid literal, or make an implicit cast when possible? Or would the network protocol take issue with that?

[roji]

This seems to go against Npgsql's standard approach of delegating validation to PostgreSQL.

What does this refer to exactly, I didn't understand...

That's my fault, I wasn't very clear. I was thinking about the length checks in MacaddrHandler:

Oh I understand, yeah, that's a legitimate point.

The ValidateAndGetLength() in general is more about getting the length (which needs to be sent up-front in the PostgreSQL protocol) and less about validation. In fact, in #1727 I investigated "one-pass" writing, which attempts to write parameter data in one pass, rather than the current two-pass system, which first calculates all lengths and does validation, and only then starts actually writing to the buffer. If one-pass were to be implemented, validation would no longer make sense, since by the time a parameter is found to be invalid, we may have already sent previous parameter data to PostgreSQL and cannot abort.

Note that even when we speak of validation, the general idea is to catch errors that would prevent us from serializing the value to the wire, rather than preventing us from sending bad data to PostgreSQL. One of the (pretty rare) examples for this is trying to send a string which contains characters that cannot be encoded in the current connection encoding; this isn't a matter of whether Npgsql or PostgreSQL validates, it's a matter of not being able to even send the data to PostgreSQL in the first place...

In any case, coming back to the subject at hand... MacaddrHandler (as opposed to Macaddr8Handler) sends parameters with datatype macaddr, which only accepts 6-byte addresses. It's true that we could relax ValidateAndGetLength() to let through 8-byte addresses, and they would then be rejected by PostgreSQL instead. IIRC this would always happen: when we speak of implicit casing, we're talking about PostgreSQL casting a valid macaddr into a valid macaddr8, but in this case we've sent an invalid macaddr (you may want to test this to confirm what I'm saying is correct).

[austindrenski]

but in this case we've sent an invalid macaddr

Ah. I think that's what I've been missing.

I've been thinking about this as a mashup of the simple and extended protocols, where everything is sent as plain text and inferred. But parameters in the extended protocol may be sent with an OID...

After looking at extended query protocol and the protocol message formats (relevant message formats included below), it seems like the only time this could potentially work would be if the macaddr8 were passed as a literal in the query string...or passed without specifying a parameter type.

Otherwise, if the protocol specifies a macaddr, then the backend ought to reject an 8-byte value...

Parse (F)
 Byte1('P')
 Identifies the message as a Parse command.

 Int32
 Length of message contents in bytes, including self.

 String
 The name of the destination prepared statement (an empty string selects the unnamed prepared statement).

 String
 The query string to be parsed.

 Int16
 The number of parameter data types specified (can be zero). Note that this is not an indication of the number of parameters that might appear in the query string, only the number that the frontend wants to prespecify types for.

 Then, for each parameter, there is the following:

 Int32
 Specifies the object ID of the parameter data type. Placing a zero here is equivalent to leaving the type unspecified.

Bind (F)
 Byte1('B')
 Identifies the message as a Bind command.

 Int32
 Length of message contents in bytes, including self.

 String
 The name of the destination portal (an empty string selects the unnamed portal).

 String
 The name of the source prepared statement (an empty string selects the unnamed prepared statement).

 Int16
 The number of parameter format codes that follow (denoted C below). This can be zero to indicate that there are no parameters or that the parameters all use the default format (text); or one, in which case the specified format code is applied to all parameters; or it can equal the actual number of parameters.

 Int16[C]
 The parameter format codes. Each must presently be zero (text) or one (binary).

 Int16
 The number of parameter values that follow (possibly zero). This must match the number of parameters needed by the query.

 Next, the following pair of fields appear for each parameter:

 Int32
 The length of the parameter value, in bytes (this count does not include itself). Can be zero. As a special case, -1 indicates a NULL parameter value. No value bytes follow in the NULL case.

 Byten
 The value of the parameter, in the format indicated by the associated format code. n is the above length.

 After the last parameter, the following fields appear:

 Int16
 The number of result-column format codes that follow (denoted R below). This can be zero to indicate that there are no result columns or that the result columns should all use the default format (text); or one, in which case the specified format code is applied to all result columns (if any); or it can equal the actual number of result columns of the query.

 Int16[R]
 The result-column format codes. Each must presently be zero (text) or one (binary).

---

It's true that we could relax ValidateAndGetLength() to let through 8-byte addresses, and they would then be rejected by PostgreSQL instead.

While it wouldn't help in the current discussion, I like this idea.

If only because it would make clear that the protocol was rejecting a value, not the driver. (Assuming, of course, that a sensible error message was propagated about a type and data length mismatch.)

---

I appreciate the discussion and your insight on this issue. I'll go ahead and close this since it sounds like the real fix depends on future work from the main EF Core project.