Add pg_lsn postgres data type #4499

arpad-m · 2025-02-19T17:43:20Z

A pg_lsn encodes an offset into the Postgres Write Ahead Log (WAL). It is a native type of Postgres.

Ten0

Thanks a lot for your contribution!

I have no strong opinion on whether this should live in Diesel (I'd say I'm 80% sure that others will say yes, but unless I'm mistaken this could alternatively be implemented in any given schema crate and I doubt many people use this, and this seems to add non trivial code) so I'll let others voice their opinions there.

That being said If we do indeed merge this I think I notice a few things that we might want to address before merging.

Thanks again! 🤗

Ten0 · 2025-02-21T22:38:31Z

diesel/src/pg/types/mod.rs

+    /// [`pg_lsn`]: https://www.postgresql.org/docs/current/datatype-pg-lsn.html
+    #[cfg(feature = "postgres_backend")]
+    #[derive(Debug, Clone, Copy, Default, QueryId, SqlType)]
+    #[diesel(postgres_type(name = "pg_lsn"))]


doesn't it have a constant oid, like most other types?
If so then we should prefer using that so as to avoid doing unnecessary dynamic type OID lookups.

indeed it has a constant oid, will make it use that one instead.

Ten0 · 2025-02-21T22:47:18Z

diesel/src/pg/types/pg_lsn.rs

+        let val = value.as_bytes().read_u64::<NetworkEndian>().map_err(|_| {
+            Box::<dyn Error + Send + Sync>::from("invalid lsn format: input isn't 8 bytes.")


unless I'm mistaken read_u64 will succeed as long as input length is >= 8 bytes, but we might want to check that there is nothing left as well, to minimize the odds of mistakes:

diesel/diesel/src/pg/types/integers.rs

Lines 83 to 95 in 828e51f

if bytes.len() < 8 {

return emit_size_error(

"Received less than 8 bytes while decoding an i64. \

Was an Integer expression accidentally marked as BigInt?",

);

}

if bytes.len() > 8 {

return emit_size_error(

"Received more than 8 bytes while decoding an i64. \

Was an expression of a different type expression accidentally marked as BigInt?"

);

}

Ten0 · 2025-02-21T22:55:53Z

diesel_cli/src/infer_schema_internals/data_structures.rs

+            if last.ident == "PgLsn" {
+                "pg_lsn".to_string()
+            } else {
+                last.ident
+                    .to_string()
+                    .split("_")
+                    .collect::<Vec<_>>()
+                    .join(" ")
+            }


It's unclear to me how this is consistent with what was previously written: previous writing seems to take something that's snake and turn it into separate words, while this seems to take something that's PascalCase, and turn it into snake, and I don't understand where these are mixed and this is a legitimate special case.
Could you please clarify this for me?
(Disclaimer: I have not investigated this in detail.)

Without this change, it would take PgLsn and turn it into PGLSN. For the othe types like TimestampTz this is not a problem: TIMESTAMPTZ is accepted by postgres. But PGLSN isn't. The underscore is required: either pg_lsn or PG_LSN, or some other casing.

I tried calling the type Pg_Lsn, but that would not just be against Rust's naming guidelines (it would trigger lints), it also created issues with other parts of the diesel tooling which want to turn pg_lsn into PgLsn.

Given that there seem to be no other types with an underscore, I figured the easiest fix is to just special case it in this piece of code. It allows us to use PgLsn for the type name without trouble.

Ok, did you figure out what the previous code was intending to do then ? I don't see a reason why it replacing underscores with spaces was something it wanted to do before though. (It might be linked to the fact that in Diesel 1.4 SQL type names inferred by the CLI might contain underscores, IIRC... But maybe that isn't relevant anymore)

I don't see a reason why it replacing underscores with spaces was something it wanted to do before though.

Don't really know but I didn't want to create spacebar heater situations where this change broke it for people.

In any case, I don't think we should call the type Pg_Lsn. It would then require changes in other code that turns pg_lsn into PgLsn (to make it turn it to Pg_Lsn). But your call in the end.

In any case, I don't think we should call the type Pg_Lsn. It would then require changes in other code that turns pg_lsn into PgLsn (to make it turn it to Pg_Lsn). But your call in the end.

Yeah no I agree that PgLsn is the correct name but I'm weary of making a change here without understanding why the code that was here before was here, because I don't see how these are consistent, and I'm wondering if the previous code even makes sense anymore now.

Don't really know

I'll have to investigate this further then.

pg_lsn is a bit of an outlier that it has an actual underscore in the type name. E.g. we don't want it to turn TimestampTz into timestamp_tz, because postgres doesn't use an underscore in that type. And this is the case for the other types that use camel case as well.

yeah but if the other types are supposed to use upper camel (pascal) here why is the previous code looking for underscores? is what I'm wondering.
Because if it's not doing anything, then there's a chance that this entire code should just be pascal to snake instead of special casing PgLsn.

A generic pascal to snake would fail because it would turn stuff like TimestampTz into timestamp_tz. pg_lsn is the only diesel-supported type with an underscore, so I feel like hardcoding this makes sense. If more types with an underscore is added, one can turn the single case into a list.

As far as I can tell now (as person that have written that old code 🤦 ) there was no case where this was required. I checked our type defs and did not found any even hidden type def that contains an _ so I'm not sure why that splitting was there is the first place. Given that I would suggest to just remove the splitting, but keeping the special casing for pg_lsn. If it turns out to be important, we well certainly learn about that in the future from a bug report. After all that part of the code is not 100% mission critical as it is only used to generate migrations in the first place.

I've removed the _ to code as per your request. I don't care in either direction about that part. When I changed the code originally I didn't know whether it was important or not so I erred on the side of not touching it.

weiznich

Thanks for contribution. This looks fine to me beside that minor remark about the commented out code.

weiznich · 2025-02-28T10:08:26Z

diesel_cli/src/infer_schema_internals/data_structures.rs

+            if last.ident == "PgLsn" {
+                "pg_lsn".to_string()
+            } else {
+                last.ident
+                    .to_string()
+                    .split("_")
+                    .collect::<Vec<_>>()
+                    .join(" ")
+            }


As far as I can tell now (as person that have written that old code 🤦 ) there was no case where this was required. I checked our type defs and did not found any even hidden type def that contains an _ so I'm not sure why that splitting was there is the first place. Given that I would suggest to just remove the splitting, but keeping the special casing for pg_lsn. If it turns out to be important, we well certainly learn about that in the future from a bug report. After all that part of the code is not 100% mission critical as it is only used to generate migrations in the first place.

weiznich · 2025-02-28T10:09:08Z

diesel_tests/tests/schema/mod.rs

+    /*diesel::sql_query("SET lc_messages TO 'en_US.UTF-8'")
+    .execute(&mut conn)
+    .unwrap();*/


Is there any particular reason to comment out this code?

arpad-m mentioned this pull request Feb 19, 2025

Store timeline locations on storage controller (schema, insert, timeline creation and passthrough to the majority) neondatabase/neon#9011

Open

19 tasks

Ten0 requested changes Feb 21, 2025

View reviewed changes

weiznich reviewed Feb 28, 2025

View reviewed changes

arpad-m added 6 commits February 28, 2025 16:51

Add pg_lsn data type

627f33f

add pg_lsn tests

7cd65ed

fixup! add pg_lsn tests

1705fb2

Add round trip test

abecc27

review comments

1cdf7d3

review comments: don't replace _ with ' ' any more

a13ccb3

arpad-m force-pushed the add_pg_lsn branch from 3cdd495 to a13ccb3 Compare February 28, 2025 15:52

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add pg_lsn postgres data type #4499

Add pg_lsn postgres data type #4499

arpad-m commented Feb 19, 2025

Ten0 left a comment •

edited

Loading

Ten0 Feb 21, 2025

arpad-m Feb 22, 2025

Ten0 Feb 21, 2025

Ten0 Feb 21, 2025

arpad-m Feb 22, 2025

Ten0 Feb 22, 2025

arpad-m Feb 24, 2025

Ten0 Feb 24, 2025

arpad-m Feb 24, 2025

Ten0 Feb 24, 2025 •

edited

Loading

arpad-m Feb 24, 2025

weiznich Feb 28, 2025

arpad-m Feb 28, 2025 •

edited

Loading

weiznich left a comment

weiznich Feb 28, 2025

weiznich Feb 28, 2025

		let val = value.as_bytes().read_u64::<NetworkEndian>().map_err(\|_\| {
		Box::<dyn Error + Send + Sync>::from("invalid lsn format: input isn't 8 bytes.")

	if bytes.len() < 8 {
	return emit_size_error(
	"Received less than 8 bytes while decoding an i64. \
	Was an Integer expression accidentally marked as BigInt?",
	);
	}

	if bytes.len() > 8 {
	return emit_size_error(
	"Received more than 8 bytes while decoding an i64. \
	Was an expression of a different type expression accidentally marked as BigInt?"
	);
	}

Add pg_lsn postgres data type #4499

Are you sure you want to change the base?

Add pg_lsn postgres data type #4499

Conversation

arpad-m commented Feb 19, 2025

Ten0 left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Ten0 Feb 24, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arpad-m Feb 28, 2025 • edited Loading

Choose a reason for hiding this comment

weiznich left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Ten0 left a comment •

edited

Loading

Ten0 Feb 24, 2025 •

edited

Loading

arpad-m Feb 28, 2025 •

edited

Loading