On Sun, Apr 5, 2020 at 10:34 AM Mark Dilger <mark.dil...@enterprisedb.com> wrote: > > On Apr 4, 2020, at 7:11 AM, Thomas Munro <thomas.mu...@gmail.com> wrote: > > However, I am getting cold feet about the new function names. The > > existing naming structure made sense when all this stuff originated in > > a contrib module with "txid_" as a prefix all over the place, but now > > that 64 bit IDs are a core concept, I wonder if we shouldn't aim for > > something that looks a little more like core functionality and doesn't > > have those "xid8_" warts in the names. > > The "xid8_" warts are partly motivated by having a type named "xid8", which > is a bit of a wart in itself.
Just a thought for the future, not sure if it's a good one: would it seem less warty in years to come if we introduced xid4 as an alias for xid, and preferred the name xid4? Then it wouldn't look so much like xid is the "real" transaction ID type and xid8 is some kind of freaky extended version; instead it would look like xid4 and xid8 are narrow and wide transaction IDs, and xid is just a historical name for xid4. > > Here's what I now propose: > > > > Transaction ID functions, using names that fit with others (cf > > pg_xact_commit_timestamp()): > > > > pg_current_xact_id() > > pg_current_xact_id_if_assigned() > > pg_xact_status(xid8) > > > > Snapshot functions (cf pg_export_snapshot()): > > > > pg_current_snapshot() > > pg_snapshot_xmin(pg_snapshot) > > pg_snapshot_xmax(pg_snapshot) > > pg_snapshot_xip(pg_snapshot) > > pg_visible_in_snapshot(xid8, pg_snapshot) > > I like some aspects of this, but not others. Function > pg_stat_get_activity(), which gets exposed through view pg_stat_activity > exposes both "backend_xid" and "backend_xmin" as (32-bit) xid. Your new > function names are not sufficiently distinct from these older names for users > to easily remember the difference: > > select pg_snapshot_xmax(st.snap) > from snapshot_test st, pg_stat_activity sa > where pg_snapshot_xmin(st.snap) = sa.backend_xmin; > ERROR: operator does not exist: xid8 = xid > > SELECT * FROM pg_stat_activity s WHERE backend_xid = pg_current_xact_id(); > ERROR: operator does not exist: xid = xid8 It's quite tempting to go and widen pg_stat_activity etc... but in any case I'm sure it'll happen for PG14. > SELECT pg_xact_status(backend_xmin), * FROM pg_stat_activity; > ERROR: function pg_xact_status(xid) does not exist > > It's not the end of the world, and users can figure out to put a cast on > those, but it's kind of ugly. That particular one can't really be fixed with a cast, either before or after this patch (I mean, if you add the right casts you can get the query to run with this function or its txid ancestor, but it'll only give the right answers during epoch 0 so I would call this friction a good case of the type system doing its job during the transition). > It was cleaner before v10, when "backend_xid = xid8_current()" clearly had a > xid vs. xid8 mismatch. On the other hand, if the xid columns in > pg_stat_activity and elsewhere eventually get upgraded to xid8 fields, then > the new naming convention in v10 will be cleaner. Yeah. Well, my cold feet with the v9 names came from thinking about how all this is going to look in a couple of years as xid8 flows into more administration interfaces. It seems inevitable that there will be some friction along the way, but it seems like a nice goal to have wider values everywhere possible from functions and views with non-warty names, and use cast to get narrow values if needed for some reason. > As such, I'm ±0 for the change. I'll let this sit for another day and see if some more reactions appear.
