Re: Should we add xid_current() or a int8->xid cast?

On Sun, Apr 5, 2020 at 10:34 AM Mark Dilger
<mark(dot)dilger(at)enterprisedb(dot)com> wrote:
> > On Apr 4, 2020, at 7:11 AM, Thomas Munro <thomas(dot)munro(at)gmail(dot)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.