Re: Should we add xid_current() or a int8->xid cast?
On Tue, Apr 7, 2020 at 12:14 PM Thomas Munro wrote: > On Sun, Apr 5, 2020 at 11:31 AM Thomas Munro wrote: > > On Sun, Apr 5, 2020 at 10:34 AM Mark Dilger > > wrote: > > > 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. > > I'll look into proposing that for PG14. One reason I like that idea > is that system view names like backend_xid could potentially retain > their names while switching to xid8 type, (maybe?) breaking fewer > queries and avoiding ugly names, on the theory that _xid doesn't > specify whether it's xid4 or an xid8. Here's a patch that renames xid to xid4, but I realised that we lack the technology to create a suitable backwards compat alias. The bigint/int8 keyword trick doesn't work here, because it would break existing queries using xid as, for example, a function argument name. Perhaps we could invent a new kind of type that is a simple alias for another type, and is entirely replaced by the base type early in processing, so that you can do type aliases without bigint-style keywords. Perhaps all of this is not worth the churn just for a neatnick project. From fb6af225121e667f77c16f257d58dcbf56ee Mon Sep 17 00:00:00 2001 From: Thomas Munro Date: Sat, 18 Apr 2020 09:09:07 +1200 Subject: [PATCH] Rename xid to xid4. Now that we have SQL type xid8, rename SQL type xid to xid4 for consistency. This makes the width of the type clearer, and avoids creating the impression that xid is the 'real' transaction ID type, rather than being a narrower version that lacks upper bits. XXX Experiment only, not for commit. To actually do this, we'd probably want to figure out how to provide aliases like SQL xid and C XIDOID for backwards compatibility. It's not done in this commit, to demonstrate that nothing in the tree depends on the old name. XXX Documentation changes needed. --- src/backend/access/hash/hashvalidate.c | 2 +- src/backend/access/transam/commit_ts.c | 2 +- src/backend/access/transam/multixact.c | 2 +- src/backend/access/transam/twophase.c | 2 +- src/backend/bootstrap/bootstrap.c | 4 +- src/backend/catalog/Catalog.pm | 2 +- src/backend/catalog/genbki.pl | 4 +- src/backend/catalog/heap.c | 4 +- src/backend/catalog/system_views.sql | 8 +-- src/backend/utils/adt/lockfuncs.c | 2 +- src/backend/utils/adt/xid.c| 20 +++--- src/backend/utils/misc/pg_controldata.c| 14 ++-- src/fe_utils/print.c | 2 +- src/include/catalog/pg_amop.dat| 6 +- src/include/catalog/pg_amproc.dat | 8 +-- src/include/catalog/pg_cast.dat| 4 +- src/include/catalog/pg_opclass.dat | 4 +- src/include/catalog/pg_operator.dat| 18 +++--- src/include/catalog/pg_opfamily.dat| 2 +- src/include/catalog/pg_proc.dat| 66 +-- src/include/catalog/pg_type.dat| 6 +- src/interfaces/ecpg/ecpglib/execute.c | 2 +- src/test/regress/expected/alter_table.out | 8 +-- src/test/regress/expected/groupingsets.out | 2 +- src/test/regress/expected/opr_sanity.out | 8 +-- src/test/regress/expected/update.out | 4 +- src/test/regress/expected/xid.out | 74 +++--- src/test/regress/sql/alter_table.sql | 8 +-- src/test/regress/sql/groupingsets.sql | 2 +- src/test/regress/sql/update.sql| 4 +- src/test/regress/sql/xid.sql | 34 +- 31 files changed, 164 insertions(+), 164 deletions(-) diff --git a/src/backend/access/hash/hashvalidate.c b/src/backend/access/hash/hashvalidate.c index b3d1367fec..a38fa344cb 100644 --- a/src/backend/access/hash/hashvalidate.c +++ b/src/backend/access/hash/hashvalidate.c @@ -315,7 +315,7 @@ check_hash_func_signature(Oid funcid, int16 amprocnum, Oid argtype) */ if ((funcid == F_HASHINT4 || funcid == F_HASHINT4EXTENDED) && (argtype == DATEOID || - argtype == XIDOID || argtype == CIDOID)) + argtype == XID4OID || argtype == CIDOID)) /* okay, allowed use of hashint4() */ ; else if ((funcid == F_HASHINT8 || funcid == F_HASHINT8EXTENDED) && (argtype == XID8OID)) diff --git a/src/backend/access/transam/commit_ts.c b/src/backend/access/transam/commit_ts.c index 630df672cc..a89d962a20 100644 --- a/src/backend/access/transam/commit_ts.c +++ b/src/backend/access/transam/commit_ts.c @@ -436,7 +436,7 @@ pg_last_committed_xact(PG_FUNCTION_ARGS) */
Re: Should we add xid_current() or a int8->xid cast?
On 2020-Apr-17, Andres Freund wrote: > Yes? But that type doesn't exist in isolation. Having yet another > significantly different representation of 64bit xids (as plain 64 bit > integers, and as some 32/32 epoch/xid split) would make an already > confusing situation even more complex. On the contrary -- I think it would clarify a confusing situation. -- Álvaro Herrerahttps://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Re: Should we add xid_current() or a int8->xid cast?
Hi, On 2020-04-17 14:07:07 -0400, Robert Haas wrote: > On Fri, Apr 17, 2020 at 1:45 PM Andres Freund wrote: > > You seem to be entirely disregarding my actual point, namely that > > txid_current(), as well as some other txid_* functions, have returned > > 64bit xids for many many years. txid_current() is the only function to > > get the current xid in a reasonable way. I don't understand how a > > proposal to add a 32/32 bit representation *in addition* to the existing > > 32 and 64bit representations is going to improve the situation. Nor do I > > see changing txid_current()'s return format as something we're going to > > go for. > > > > I did not argue against a function to turn 64bit xids into epoch/32bit > > xid or such. > > I thought we were talking about how the new xid8 type ought to behave. Yes? But that type doesn't exist in isolation. Having yet another significantly different representation of 64bit xids (as plain 64 bit integers, and as some 32/32 epoch/xid split) would make an already confusing situation even more complex. Greetings, Andres Freund
Re: Should we add xid_current() or a int8->xid cast?
On Fri, Apr 17, 2020 at 1:45 PM Andres Freund wrote: > You seem to be entirely disregarding my actual point, namely that > txid_current(), as well as some other txid_* functions, have returned > 64bit xids for many many years. txid_current() is the only function to > get the current xid in a reasonable way. I don't understand how a > proposal to add a 32/32 bit representation *in addition* to the existing > 32 and 64bit representations is going to improve the situation. Nor do I > see changing txid_current()'s return format as something we're going to > go for. > > I did not argue against a function to turn 64bit xids into epoch/32bit > xid or such. I thought we were talking about how the new xid8 type ought to behave. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Re: Should we add xid_current() or a int8->xid cast?
Hi, On 2020-04-17 13:33:53 -0400, Robert Haas wrote: > On Thu, Apr 2, 2020 at 5:13 PM Andres Freund wrote: > > Given that txid_current() "always" has been a plain 64 bit integer, and > > the various txid_* functions always have returned 64 bit integers, I > > really don't think arguing for some 32bit/32bit situation now makes > > sense. > > I'm not sure what the best thing to do is here, but the reality is > that there are many places where 32-bit XIDs are going to be showing > up for years to come. With the format printed as a raw 64-bit > quantity, people troubleshooting stuff are going to spend a lot of > time figuring what x%2^32 is. And I can't do that in my head. So I > think saying that the proposal does not makes sense is a gross > overstatement. It may not be what we want to do. But it definitely > would make sense. You seem to be entirely disregarding my actual point, namely that txid_current(), as well as some other txid_* functions, have returned 64bit xids for many many years. txid_current() is the only function to get the current xid in a reasonable way. I don't understand how a proposal to add a 32/32 bit representation *in addition* to the existing 32 and 64bit representations is going to improve the situation. Nor do I see changing txid_current()'s return format as something we're going to go for. I did not argue against a function to turn 64bit xids into epoch/32bit xid or such. ? Greetings, Andres Freund
Re: Should we add xid_current() or a int8->xid cast?
On Thu, Apr 2, 2020 at 5:13 PM Andres Freund wrote: > Given that txid_current() "always" has been a plain 64 bit integer, and > the various txid_* functions always have returned 64 bit integers, I > really don't think arguing for some 32bit/32bit situation now makes > sense. I'm not sure what the best thing to do is here, but the reality is that there are many places where 32-bit XIDs are going to be showing up for years to come. With the format printed as a raw 64-bit quantity, people troubleshooting stuff are going to spend a lot of time figuring what x%2^32 is. And I can't do that in my head. So I think saying that the proposal does not makes sense is a gross overstatement. It may not be what we want to do. But it definitely would make sense. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Re: Should we add xid_current() or a int8->xid cast?
On Fri, Apr 17, 2020 at 3:44 AM Mark Dilger wrote: > Hmm, I should have spoken sooner... > > src/backend/replication/walsender.c:static bool > TransactionIdInRecentPast(TransactionId xid, uint32 epoch); > src/backend/utils/adt/xid8funcs.c:TransactionIdInRecentPast(FullTransactionId > fxid, TransactionId *extracted_xid) > > I don't care much for having two different functions with the same name and > related semantics but different argument types. Maybe that's not ideal, but it's not because of this patch. Those functions are from 5737c12df05 and 857ee8e391f.
Re: Should we add xid_current() or a int8->xid cast?
> On Apr 6, 2020, at 5:14 PM, Thomas Munro wrote: > > On Sun, Apr 5, 2020 at 11:31 AM Thomas Munro wrote: >> On Sun, Apr 5, 2020 at 10:34 AM Mark Dilger >> wrote: >>> 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. > > I'll look into proposing that for PG14. One reason I like that idea > is that system view names like backend_xid could potentially retain > their names while switching to xid8 type, (maybe?) breaking fewer > queries and avoiding ugly names, on the theory that _xid doesn't > specify whether it's xid4 or an xid8. > pg_current_xact_id() pg_current_xact_id_if_assigned() pg_xact_status(xid8) > 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) > >>> As such, I'm ±0 for the change. >> >> I'll let this sit for another day and see if some more reactions appear. > > Hearing no objections, pushed. Happy to reconsider these names before > release if someone finds a problem with this scheme. Hmm, I should have spoken sooner... src/backend/replication/walsender.c:static bool TransactionIdInRecentPast(TransactionId xid, uint32 epoch); src/backend/utils/adt/xid8funcs.c:TransactionIdInRecentPast(FullTransactionId fxid, TransactionId *extracted_xid) I don't care much for having two different functions with the same name and related semantics but different argument types. — Mark Dilger EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Re: Should we add xid_current() or a int8->xid cast?
On Sun, Apr 5, 2020 at 11:31 AM Thomas Munro wrote: > On Sun, Apr 5, 2020 at 10:34 AM Mark Dilger > wrote: > > 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. I'll look into proposing that for PG14. One reason I like that idea is that system view names like backend_xid could potentially retain their names while switching to xid8 type, (maybe?) breaking fewer queries and avoiding ugly names, on the theory that _xid doesn't specify whether it's xid4 or an xid8. > > > pg_current_xact_id() > > > pg_current_xact_id_if_assigned() > > > pg_xact_status(xid8) > > > 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) > > As such, I'm ±0 for the change. > > I'll let this sit for another day and see if some more reactions appear. Hearing no objections, pushed. Happy to reconsider these names before release if someone finds a problem with this scheme.
Re: Should we add xid_current() or a int8->xid cast?
On Sun, Apr 5, 2020 at 10:34 AM Mark Dilger wrote: > > On Apr 4, 2020, at 7:11 AM, Thomas Munro 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.
Re: Should we add xid_current() or a int8->xid cast?
> On Apr 4, 2020, at 7:11 AM, Thomas Munro wrote: > > On Sat, Apr 4, 2020 at 4:45 AM Mark Dilger > wrote: >> FYI, (not the responsibility of this patch), we never quite define what the >> abbreviation "xip" stands for. If "Active xid8s at the time of the >> snapshot." were rewritten as "In progress xid8s at the time of the >> snapshot", it might be slightly easier for the reader to figure out that >> "xip" = "Xid8s In Progress". As it stands, nothing in the docs seems to >> explain the abbrevation. See doc/src/sgml/func.sgml > > You're right. Done. Thanks! > 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. > 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 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. 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. As such, I'm ±0 for the change. — Mark Dilger EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Re: Should we add xid_current() or a int8->xid cast?
> On Apr 2, 2020, at 7:39 PM, Thomas Munro wrote: > > These apply cleanly, build and pass check-world on mac, and the documentation and regression test changes surrounding txid look good to me. FYI, (not the responsibility of this patch), we never quite define what the abbreviation "xip" stands for. If "Active xid8s at the time of the snapshot." were rewritten as "In progress xid8s at the time of the snapshot", it might be slightly easier for the reader to figure out that "xip" = "Xid8s In Progress". As it stands, nothing in the docs seems to explain the abbrevation. See doc/src/sgml/func.sgml — Mark Dilger EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Re: Should we add xid_current() or a int8->xid cast?
Hi, On 2020-04-02 14:26:41 -0700, Mark Dilger wrote: > Since Thomas's patch is really just focused on transitioning from txid > -> xid8, I think this conversation is a bit beyond scope for this > patch, except that "xid8" sounds an awful lot like the new type that > all user facing xid output will transition to. Maybe I'm wrong about > that. Several at least. For me it'd e.g. make no sense to change pageinspect etc. > Are we going to change the definition of the "xid" type to 8 bytes? > That sounds dangerous, from a compatibility standpoint. No, I can't see that happening. > On balance, I'd rather have xid8in and xid8out work just as Thomas has > it. I'm not asking for any change there. But I'm curious if the > whole community is on the same page regarding where this is all > heading. > > I'm contemplating the statement that "the goal should be to reduce > awareness of the 32bitness of normal xids from as many places as > possible", which I support, and what that means for the eventual > signatures of functions like pg_stat_get_activity, including: Maybe. Aiming to do things like this all-at-once just makes it less likely for anything to ever happen. > but otherwise there would be a transition period where some have been > reworked to return xid8 but others not, and users during that > transition period might be happier with Alvaro's suggestion of > treating epoch/xid as two fields in xid8in and xid8out. -countless I can only restate my point that we've had 8 byte xids exposed for many years. We've had very few epoch/xid values exposed. I think it'd be insane to now start to expose that more widely. It's just about impossible for normal users to compare xids. Once one wrapped around, it's just too hard/mindbending. Yes, an accompanying epoch makes it easier, but it still can be quite confusing. Greetings, Andres Freund
Re: Should we add xid_current() or a int8->xid cast?
> On Apr 2, 2020, at 2:13 PM, Andres Freund wrote: > > Hi, > > On 2020-04-02 11:47:32 -0700, Mark Dilger wrote: >> I agree with transitioning to 64-bit xids with 32 bit xid/epoch pairs >> as an internal implementation and storage detail only, but we still >> have user facing views that don't treat it that way. > > Note that epochs are not really a thing internally anymore. The xid > counter is a FullTransactionId. > > >> pg_stat_get_activity still returns backend_xid and backend_xmin as >> 32-bit, not 64-bit. Should this function change to be consistent? I'm >> curious what the user experience will be during the transitional period >> where some user facing xids are 64 bit and others (perhaps the same xids >> but viewed elsewhere) will be 32 bit. That might make it difficult for >> users to match them up. > > I think we probably should switch them over at some point, but I would > strongly advise against coupling that with Thomas' patch. That patch > doesn't make the current situation around 32bit / 64bit any worse, as > far as I can tell. I agree with that. > Given that txid_current() "always" has been a plain 64 bit integer, and > the various txid_* functions always have returned 64 bit integers, I > really don't think arguing for some 32bit/32bit situation now makes > sense. Yeah, I'm not arguing for that, though I can see how my email might have been ambiguous on that point. Since Thomas's patch is really just focused on transitioning from txid -> xid8, I think this conversation is a bit beyond scope for this patch, except that "xid8" sounds an awful lot like the new type that all user facing xid output will transition to. Maybe I'm wrong about that. Are we going to change the definition of the "xid" type to 8 bytes? That sounds dangerous, from a compatibility standpoint. On balance, I'd rather have xid8in and xid8out work just as Thomas has it. I'm not asking for any change there. But I'm curious if the whole community is on the same page regarding where this is all heading. I'm contemplating the statement that "the goal should be to reduce awareness of the 32bitness of normal xids from as many places as possible", which I support, and what that means for the eventual signatures of functions like pg_stat_get_activity, including: (..., backend_xid XID, backend_xminxid XID, ...) pg_stat_get_activity(pid INTEGER) (..., transactionid XID, ...) pg_lock_status() (transaction XID, ...) pg_prepared_xact() timestamptz pg_xact_commit_timestamp(XID) (xid XID, ...) pg_last_committed_xact() (..., xmin XID, catalog_xmin XID, ...) pg_get_replication_slots() ... more that I'm too lazy to copy-and-paste just now ... I would expect that, eventually, these would be upgraded to xid8. If that happened seemlessly in one release, then there would be no problem with some functions returning 4-byte xids and some returning 8-byte xids, but otherwise there would be a transition period where some have been reworked to return xid8 but others not, and users during that transition period might be happier with Alvaro's suggestion of treating epoch/xid as two fields in xid8in and xid8out. I'm also doubtful that these functions would be "upgraded". It seems far more likely that alternate versions, perhaps named something with /xid8/ in them, would exist side-by-side with the originals. So I'm really just wondering where others on this list think all this is heading, and if there are any arguments brewing about that which could be avoided by making assumptions clear right up front. — Mark Dilger EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Re: Should we add xid_current() or a int8->xid cast?
Hi, On 2020-04-02 11:47:32 -0700, Mark Dilger wrote: > I agree with transitioning to 64-bit xids with 32 bit xid/epoch pairs > as an internal implementation and storage detail only, but we still > have user facing views that don't treat it that way. Note that epochs are not really a thing internally anymore. The xid counter is a FullTransactionId. > pg_stat_get_activity still returns backend_xid and backend_xmin as > 32-bit, not 64-bit. Should this function change to be consistent? I'm > curious what the user experience will be during the transitional period > where some user facing xids are 64 bit and others (perhaps the same xids > but viewed elsewhere) will be 32 bit. That might make it difficult for > users to match them up. I think we probably should switch them over at some point, but I would strongly advise against coupling that with Thomas' patch. That patch doesn't make the current situation around 32bit / 64bit any worse, as far as I can tell. Given that txid_current() "always" has been a plain 64 bit integer, and the various txid_* functions always have returned 64 bit integers, I really don't think arguing for some 32bit/32bit situation now makes sense. Greetings, Andres Freund
Re: Should we add xid_current() or a int8->xid cast?
On Thu, Apr 2, 2020 at 2:47 PM Mark Dilger wrote: > > > > > On Apr 2, 2020, at 11:01 AM, Andres Freund wrote: > > > >> > >> Hmm, for some reason I had it in my head that we would make these use an > >> "epoch/val" output format rather than raw uint64 values. > > > > Why would we do that? IMO the goal should be to reduce awareness of the > > 32bitness of normal xids from as many places as possible, and treat them > > as an internal space optimization. > > I agree with transitioning to 64-bit xids with 32 bit xid/epoch pairs as an > internal implementation and storage detail only, but we still have user > facing views that don't treat it that way. pg_stat_get_activity still > returns backend_xid and backend_xmin as 32-bit, not 64-bit. Should this > function change to be consistent? I'm curious what the user experience will > be during the transitional period where some user facing xids are 64 bit and > others (perhaps the same xids but viewed elsewhere) will be 32 bit. That > might make it difficult for users to match them up. Agreed. The "benefit" (at least in the short term) of using the epoch/value style is that it makes (visual, at least) comparison with other (32-bit) xid values easier. I'm not sure if that's worth it, or if it's worth making a change depend on changing all of those views too. James
Re: Should we add xid_current() or a int8->xid cast?
Andres Freund writes: > On 2020-04-02 14:33:18 -0300, Alvaro Herrera wrote: >> Hmm, for some reason I had it in my head that we would make these use an >> "epoch/val" output format rather than raw uint64 values. > Why would we do that? IMO the goal should be to reduce awareness of the > 32bitness of normal xids from as many places as possible, and treat them > as an internal space optimization. If they're just int64s then you don't need special functions to do things like finding the min or max in a column of them. regards, tom lane
Re: Should we add xid_current() or a int8->xid cast?
> On Apr 2, 2020, at 11:01 AM, Andres Freund wrote: > >> >> Hmm, for some reason I had it in my head that we would make these use an >> "epoch/val" output format rather than raw uint64 values. > > Why would we do that? IMO the goal should be to reduce awareness of the > 32bitness of normal xids from as many places as possible, and treat them > as an internal space optimization. I agree with transitioning to 64-bit xids with 32 bit xid/epoch pairs as an internal implementation and storage detail only, but we still have user facing views that don't treat it that way. pg_stat_get_activity still returns backend_xid and backend_xmin as 32-bit, not 64-bit. Should this function change to be consistent? I'm curious what the user experience will be during the transitional period where some user facing xids are 64 bit and others (perhaps the same xids but viewed elsewhere) will be 32 bit. That might make it difficult for users to match them up. — Mark Dilger EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Re: Should we add xid_current() or a int8->xid cast?
Hi, On 2020-04-02 14:33:18 -0300, Alvaro Herrera wrote: > On 2020-Apr-02, Thomas Munro wrote: > > > On Sat, Mar 21, 2020 at 11:14 AM Thomas Munro > > wrote: > > > * updated OIDs to avoid collisions > > > * added btequalimage to btree/xid8_ops > > > > Here's the version I'm planning to commit tomorrow, if no one objects. > > Changes: > > > > * txid.c renamed to xid8funcs.c > > * remaining traces of "txid" replaced various internal identifiers > > * s/backwards compatible/backward compatible/ in funcs.sgml (en_GB -> en_US) > > Hmm, for some reason I had it in my head that we would make these use an > "epoch/val" output format rather than raw uint64 values. Why would we do that? IMO the goal should be to reduce awareness of the 32bitness of normal xids from as many places as possible, and treat them as an internal space optimization. Greetings, Andres Freund
Re: Should we add xid_current() or a int8->xid cast?
On 2020-Apr-02, Thomas Munro wrote: > On Sat, Mar 21, 2020 at 11:14 AM Thomas Munro wrote: > > * updated OIDs to avoid collisions > > * added btequalimage to btree/xid8_ops > > Here's the version I'm planning to commit tomorrow, if no one objects. > Changes: > > * txid.c renamed to xid8funcs.c > * remaining traces of "txid" replaced various internal identifiers > * s/backwards compatible/backward compatible/ in funcs.sgml (en_GB -> en_US) Hmm, for some reason I had it in my head that we would make these use an "epoch/val" output format rather than raw uint64 values. Are we really going to do it this way? Myself I can convert values easily enough, but I'm not sure this is user-friendly. (If somebody were to tell me that LSNs are going to be straight uint64 values, I would not be happy.) Or maybe it's the other way around -- this is fine for everyone except me -- and we should never expose the epoch as a separate quantity. -- Álvaro Herrerahttps://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Re: Should we add xid_current() or a int8->xid cast?
> On Apr 2, 2020, at 9:06 AM, Mark Dilger wrote:
>
> ("xid8_current" is not exercised by name anywhere in the regression suite,
> that I can see.)
I spoke too soon. That's exercised in the new xid.sql test file. It didn't
show up in my 'git diff', because it's new. Sorry about that.
—
Mark Dilger
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
Re: Should we add xid_current() or a int8->xid cast?
> On Apr 1, 2020, at 8:21 PM, Thomas Munro wrote:
>
> On Sat, Mar 21, 2020 at 11:14 AM Thomas Munro wrote:
>> * updated OIDs to avoid collisions
>> * added btequalimage to btree/xid8_ops
>
> Here's the version I'm planning to commit tomorrow, if no one objects.
> Changes:
>
> * txid.c renamed to xid8funcs.c
> * remaining traces of "txid" replaced various internal identifiers
> * s/backwards compatible/backward compatible/ in funcs.sgml (en_GB -> en_US)
>
Hi Thomas, Thanks for working on this.
I'm taking a quick look at your patches. It's not a big deal, and certainly
not a show stopper if you want to go ahead with the commit, but you've left
some mentions of "txid_current" that might better be modified to use the new
name "xid8_current". At least one mention of "txid_current" is needed to check
that the old name still works, but leaving this many in the regression test
suite may lead other developers to follow that lead and use txid_current() in
newly developed code. ("xid8_current" is not exercised by name anywhere in the
regression suite, that I can see.)
> contrib/test_decoding/expected/ddl.out:SELECT txid_current() != 0; -- so no
> fixed xid apears in the outfile
> contrib/test_decoding/expected/decoding_in_xact.out:SELECT txid_current() = 0;
> contrib/test_decoding/expected/decoding_in_xact.out:SELECT txid_current() = 0;
> contrib/test_decoding/expected/decoding_in_xact.out:SELECT txid_current() = 0;
> contrib/test_decoding/expected/oldest_xmin.out:step s0_getxid: SELECT
> txid_current() IS NULL;
> contrib/test_decoding/expected/ondisk_startup.out:step s2txid: SELECT
> txid_current() IS NULL;
> contrib/test_decoding/expected/ondisk_startup.out:step s3txid: SELECT
> txid_current() IS NULL;
> contrib/test_decoding/expected/ondisk_startup.out:step s2txid: SELECT
> txid_current() IS NULL;
> contrib/test_decoding/expected/snapshot_transfer.out:step s0_log_assignment:
> SELECT txid_current() IS NULL;
> contrib/test_decoding/expected/snapshot_transfer.out:step s0_log_assignment:
> SELECT txid_current() IS NULL;
> contrib/test_decoding/specs/oldest_xmin.spec:step "s0_getxid" { SELECT
> txid_current() IS NULL; }
> contrib/test_decoding/specs/ondisk_startup.spec:step "s2txid" { SELECT
> txid_current() IS NULL; }
> contrib/test_decoding/specs/ondisk_startup.spec:step "s3txid" { SELECT
> txid_current() IS NULL; }
> contrib/test_decoding/specs/snapshot_transfer.spec:step "s0_log_assignment" {
> SELECT txid_current() IS NULL; }
> contrib/test_decoding/sql/ddl.sql:SELECT txid_current() != 0; -- so no fixed
> xid apears in the outfile
> contrib/test_decoding/sql/decoding_in_xact.sql:SELECT txid_current() = 0;
> contrib/test_decoding/sql/decoding_in_xact.sql:SELECT txid_current() = 0;
> contrib/test_decoding/sql/decoding_in_xact.sql:SELECT txid_current() = 0;
>
> src/test/modules/commit_ts/t/004_restart.pl:SELECT txid_current();
> src/test/modules/commit_ts/t/004_restart.pl:EXECUTE 'SELECT
> txid_current()';
> src/test/modules/commit_ts/t/004_restart.pl:SELECT txid_current();
> src/test/recovery/t/003_recovery_targets.pl:"SELECT pg_current_wal_lsn(),
> txid_current();");
> src/test/recovery/t/011_crash_recovery.pl:SELECT txid_current();
> src/test/recovery/t/011_crash_recovery.pl:cmp_ok($node->safe_psql('postgres',
> 'SELECT txid_current()'),
> src/test/regress/expected/alter_table.out:where transactionid =
> txid_current()::integer)
> src/test/regress/expected/alter_table.out:where transactionid =
> txid_current()::integer)
> src/test/regress/expected/hs_standby_functions.out:select txid_current();
> src/test/regress/expected/hs_standby_functions.out:ERROR: cannot execute
> txid_current() during recovery
> src/test/regress/expected/hs_standby_functions.out:select
> length(txid_current_snapshot()::text) >= 4;
> src/test/regress/expected/txid.out:select txid_current() >=
> txid_snapshot_xmin(txid_current_snapshot());
> src/test/regress/expected/txid.out:select
> txid_visible_in_snapshot(txid_current(), txid_current_snapshot());
> src/test/regress/expected/txid.out:-- test txid_current_if_assigned
> src/test/regress/expected/txid.out:SELECT txid_current_if_assigned() IS NULL;
> src/test/regress/expected/txid.out:SELECT txid_current() \gset
> src/test/regress/expected/txid.out:SELECT txid_current_if_assigned() IS NOT
> DISTINCT FROM BIGINT :'txid_current';
> src/test/regress/expected/txid.out:SELECT txid_current() AS committed \gset
> src/test/regress/expected/txid.out:SELECT txid_current() AS rolledback \gset
> src/test/regress/expected/txid.out:SELECT txid_current() AS inprogress \gset
> src/test/regress/expected/update.out:CREATE FUNCTION xid_current() RETURNS
> xid LANGUAGE SQL AS $$SELECT (txid_current() % ((1::int8<<32)))::text::xid;$$;
> src/test/regress/sql/alter_table.sql:where transactionid =
> txid_current()::integer)
> src/test/regress/sql/alter_table.sql:where transactionid =
> txid_current()::integer)
Re: Should we add xid_current() or a int8->xid cast?
On Sat, Mar 21, 2020 at 11:14 AM Thomas Munro wrote:
> * updated OIDs to avoid collisions
> * added btequalimage to btree/xid8_ops
Here's the version I'm planning to commit tomorrow, if no one objects. Changes:
* txid.c renamed to xid8funcs.c
* remaining traces of "txid" replaced various internal identifiers
* s/backwards compatible/backward compatible/ in funcs.sgml (en_GB -> en_US)
From 308b094306f587a6736963983162290ac2808ac7 Mon Sep 17 00:00:00 2001
From: Thomas Munro
Date: Thu, 2 Apr 2020 15:00:23 +1300
Subject: [PATCH v8 1/2] Add SQL type xid8 to expose FullTransactionId to
users.
Similar to xid, but 64 bits wide. This new type is suitable for use in
various system views and administration functions.
Reviewed-by: Fujii Masao
Reviewed-by: Takao Fujii
Reviewed-by: Yoshikazu Imai
Reviewed-by: Mark Dilger
Discussion: https://postgr.es/m/20190725000636.666m5mad25wfbrri%40alap3.anarazel.de
---
doc/src/sgml/datatype.sgml | 7 ++
src/backend/access/hash/hashvalidate.c | 3 +
src/backend/utils/adt/xid.c | 116 +++
src/fe_utils/print.c | 1 +
src/include/access/transam.h | 14 +++
src/include/catalog/pg_amop.dat | 22
src/include/catalog/pg_amproc.dat| 8 ++
src/include/catalog/pg_cast.dat | 4 +
src/include/catalog/pg_opclass.dat | 4 +
src/include/catalog/pg_operator.dat | 25 +
src/include/catalog/pg_opfamily.dat | 4 +
src/include/catalog/pg_proc.dat | 36 ++
src/include/catalog/pg_type.dat | 4 +
src/include/utils/xid8.h | 22
src/test/regress/expected/opr_sanity.out | 7 ++
src/test/regress/expected/xid.out| 136 +++
src/test/regress/parallel_schedule | 2 +-
src/test/regress/serial_schedule | 1 +
src/test/regress/sql/xid.sql | 48
19 files changed, 463 insertions(+), 1 deletion(-)
create mode 100644 src/include/utils/xid8.h
create mode 100644 src/test/regress/expected/xid.out
create mode 100644 src/test/regress/sql/xid.sql
diff --git a/doc/src/sgml/datatype.sgml b/doc/src/sgml/datatype.sgml
index 03971822c2..89f3a7c119 100644
--- a/doc/src/sgml/datatype.sgml
+++ b/doc/src/sgml/datatype.sgml
@@ -4516,6 +4516,10 @@ INSERT INTO mytable VALUES(-1); -- fails
regtype
+
+xid8
+
+
cid
@@ -4719,6 +4723,9 @@ SELECT * FROM pg_attribute
Another identifier type used by the system is xid, or transaction
(abbreviated xact) identifier. This is the data type of the system columns
xmin and xmax. Transaction identifiers are 32-bit quantities.
+In some contexts, a 64-bit variant xid8 is used. Unlike
+xid values, xid8 values increase strictly
+monotonically and cannot be reused in the lifetime of a database cluster.
diff --git a/src/backend/access/hash/hashvalidate.c b/src/backend/access/hash/hashvalidate.c
index 7b08ed5354..b3d1367fec 100644
--- a/src/backend/access/hash/hashvalidate.c
+++ b/src/backend/access/hash/hashvalidate.c
@@ -317,6 +317,9 @@ check_hash_func_signature(Oid funcid, int16 amprocnum, Oid argtype)
(argtype == DATEOID ||
argtype == XIDOID || argtype == CIDOID))
/* okay, allowed use of hashint4() */ ;
+ else if ((funcid == F_HASHINT8 || funcid == F_HASHINT8EXTENDED) &&
+ (argtype == XID8OID))
+ /* okay, allowed use of hashint8() */ ;
else if ((funcid == F_TIMESTAMP_HASH ||
funcid == F_TIMESTAMP_HASH_EXTENDED) &&
argtype == TIMESTAMPTZOID)
diff --git a/src/backend/utils/adt/xid.c b/src/backend/utils/adt/xid.c
index db6fc9dd6b..20389aff1d 100644
--- a/src/backend/utils/adt/xid.c
+++ b/src/backend/utils/adt/xid.c
@@ -21,6 +21,7 @@
#include "access/xact.h"
#include "libpq/pqformat.h"
#include "utils/builtins.h"
+#include "utils/xid8.h"
#define PG_GETARG_TRANSACTIONID(n) DatumGetTransactionId(PG_GETARG_DATUM(n))
#define PG_RETURN_TRANSACTIONID(x) return TransactionIdGetDatum(x)
@@ -147,6 +148,121 @@ xidComparator(const void *arg1, const void *arg2)
return 0;
}
+Datum
+xid8toxid(PG_FUNCTION_ARGS)
+{
+ FullTransactionId fxid = PG_GETARG_FULLTRANSACTIONID(0);
+
+ PG_RETURN_TRANSACTIONID(XidFromFullTransactionId(fxid));
+}
+
+Datum
+xid8in(PG_FUNCTION_ARGS)
+{
+ char *str = PG_GETARG_CSTRING(0);
+
+ PG_RETURN_FULLTRANSACTIONID(FullTransactionIdFromU64(pg_strtouint64(str, NULL, 0)));
+}
+
+Datum
+xid8out(PG_FUNCTION_ARGS)
+{
+ FullTransactionId fxid = PG_GETARG_FULLTRANSACTIONID(0);
+ char *result = (char *) palloc(21);
+
+ snprintf(result, 21, UINT64_FORMAT, U64FromFullTransactionId(fxid));
+ PG_RETURN_CSTRING(result);
+}
+
+Datum
+xid8recv(PG_FUNCTION_ARGS)
+{
+ StringInfo buf = (StringInfo) PG_GETARG_POINTER(0);
+ uint64 value;
+
+ value = (uint64) pq_getmsgint64(buf);
+ PG_RETURN_FULLTRANSACTIONID(FullTransactionIdFromU64(value));
+}
+
+Datum
+xid8send(PG_FUNCTION_ARGS)
+{
+ FullTransactionId arg1 =
Re: Should we add xid_current() or a int8->xid cast?
On Fri, Feb 14, 2020 at 6:31 PM Thomas Munro wrote: > On Wed, Jan 29, 2020 at 12:43 AM Fujii Masao > wrote: > > /* > > - * do a TransactionId -> txid conversion for an XID near the given epoch > > + * Do a TransactionId -> fxid conversion for an XID that is known to > > precede > > + * the given 'next_fxid'. > >*/ > > -static txid > > -convert_xid(TransactionId xid, const TxidEpoch *state) > > +static FullTransactionId > > +convert_xid(TransactionId xid, FullTransactionId next_fxid) > > > > As the comment suggests, this function assumes that "xid" must > > precede "next_fxid". But there is no check for the assumption. > > Isn't it better to add, e.g., an assertion checking that? > > Or convert_xid() should handle the case where "xid" follows > > "next_fxid" like the orignal convert_xid() does. That is, don't > > apply the following change. > > > > - if (xid > state->last_xid && > > - TransactionIdPrecedes(xid, state->last_xid)) > > + if (xid > next_xid) > > epoch--; > > - else if (xid < state->last_xid && > > -TransactionIdFollows(xid, state->last_xid)) > > - epoch++; I don't think it is reachable. I have renamed the function to widen_snapshot_xid() and rewritten the comments to explain the logic. The other changes in this version: * updated OIDs to avoid collisions * added btequalimage to btree/xid8_ops From 7fc4387dae4395df111ac64aa28c66aebbb04dbb Mon Sep 17 00:00:00 2001 From: Thomas Munro Date: Tue, 28 Jan 2020 16:36:36 +1300 Subject: [PATCH v7 1/2] Add SQL type xid8 to expose FullTransactionId to users. Similar to xid, but 64 bits wide. This new type is suitable for use in various system views and administration functions. Reviewed-by: Fujii Masao Reviewed-by: Takao Fujii Reviewed-by: Yoshikazu Imai Reviewed-by: Mark Dilger Discussion: https://postgr.es/m/https://www.postgresql.org/message-id/20190725000636.666m5mad25wfbrri%40alap3.anarazel.de --- doc/src/sgml/datatype.sgml | 7 ++ src/backend/access/hash/hashvalidate.c | 3 + src/backend/utils/adt/xid.c | 116 +++ src/fe_utils/print.c | 1 + src/include/access/transam.h | 14 +++ src/include/catalog/pg_amop.dat | 22 src/include/catalog/pg_amproc.dat| 8 ++ src/include/catalog/pg_cast.dat | 4 + src/include/catalog/pg_opclass.dat | 4 + src/include/catalog/pg_operator.dat | 25 + src/include/catalog/pg_opfamily.dat | 4 + src/include/catalog/pg_proc.dat | 36 ++ src/include/catalog/pg_type.dat | 4 + src/include/utils/xid8.h | 22 src/test/regress/expected/opr_sanity.out | 7 ++ src/test/regress/expected/xid.out| 136 +++ src/test/regress/parallel_schedule | 2 +- src/test/regress/serial_schedule | 1 + src/test/regress/sql/xid.sql | 48 19 files changed, 463 insertions(+), 1 deletion(-) create mode 100644 src/include/utils/xid8.h create mode 100644 src/test/regress/expected/xid.out create mode 100644 src/test/regress/sql/xid.sql diff --git a/doc/src/sgml/datatype.sgml b/doc/src/sgml/datatype.sgml index 03971822c2..89f3a7c119 100644 --- a/doc/src/sgml/datatype.sgml +++ b/doc/src/sgml/datatype.sgml @@ -4516,6 +4516,10 @@ INSERT INTO mytable VALUES(-1); -- fails regtype + +xid8 + + cid @@ -4719,6 +4723,9 @@ SELECT * FROM pg_attribute Another identifier type used by the system is xid, or transaction (abbreviated xact) identifier. This is the data type of the system columns xmin and xmax. Transaction identifiers are 32-bit quantities. +In some contexts, a 64-bit variant xid8 is used. Unlike +xid values, xid8 values increase strictly +monotonically and cannot be reused in the lifetime of a database cluster. diff --git a/src/backend/access/hash/hashvalidate.c b/src/backend/access/hash/hashvalidate.c index 6346e65865..2f2858021c 100644 --- a/src/backend/access/hash/hashvalidate.c +++ b/src/backend/access/hash/hashvalidate.c @@ -313,6 +313,9 @@ check_hash_func_signature(Oid funcid, int16 amprocnum, Oid argtype) (argtype == DATEOID || argtype == XIDOID || argtype == CIDOID)) /* okay, allowed use of hashint4() */ ; + else if ((funcid == F_HASHINT8 || funcid == F_HASHINT8EXTENDED) && + (argtype == XID8OID)) + /* okay, allowed use of hashint8() */ ; else if ((funcid == F_TIMESTAMP_HASH || funcid == F_TIMESTAMP_HASH_EXTENDED) && argtype == TIMESTAMPTZOID) diff --git a/src/backend/utils/adt/xid.c b/src/backend/utils/adt/xid.c index db6fc9dd6b..20389aff1d 100644 --- a/src/backend/utils/adt/xid.c +++ b/src/backend/utils/adt/xid.c @@ -21,6 +21,7 @@ #include "access/xact.h" #include "libpq/pqformat.h" #include "utils/builtins.h" +#include "utils/xid8.h" #define
Re: Should we add xid_current() or a int8->xid cast?
Hello Fujii-san,
Thanks for your review!
On Wed, Jan 29, 2020 at 12:43 AM Fujii Masao
wrote:
> Isn't it better to add also xid8lt, xid8gt, xid8le, and xid8ge?
xid doesn't have these operators, probably to avoid confusion about
wraparound. But you're right, we should add them for xid8, especially
since the xid_snapshot documentation mentions such comparisons (the
type used by the old functions was int8, so that worked). Done. I
also added the extra catalogue nuts and bolts required to use xid8 in
btree indexes and merge joins.
To test the operators, I added a new regression test for xid and xid8
types. While doing that, I tried to add some range checks to validate
input, but I discovered that it's a bit tricky to do so portably with
strtoul(). I suspect '0x1'::xid already gives different
results on Windows and Unix today, and if better input validation is
desired, I think it should be tackled outside this project.
While working on those tests, I realised that we probably wanted two
sets of tests:
1. txid.sql: The existing tests that show that the old txid_XXX()
functions continue to work correctly (with the only user-visible
difference being that in their error messages they sometimes mention
names including xid8). This test will be dropped when the txid_XXX()
functions are dropped.
2. xid.sql: A new set of tests that show that the new xid8_XXX()
functions work correctly.
To verify that the old tests and the new tests are exactly the same
except for typenames and some casts, use:
diff -u src/test/regress/expected/txid.out src/test/regress/expected/xid.out
> xid8 and xid8_snapshot should be documented in datatype.sgml like
> txid_snapshot is?
Done.
> logicaldecoding.sgml and monitoring.sgml still referred to txid_xxx.
> They should be updated so that new xid8_xxx is used?
Done.
> In func.sgml, the table "Snapshot Components" is described still based
> on txid. It should be updated so that it uses xid8, instead?
Done.
> +# xid_ops
> +{ amopfamily => 'hash/xid8_ops', amoplefttype => 'xid8', amoprighttype =>
> 'xid8',
> + amopstrategy => '1', amopopr => '=(xid8,xid8)', amopmethod => 'hash' },
>
> "xid_ops" in the comment should be "xid8_ops".
Fixed.
> +{ oid => '9558',
> + proname => 'xid8neq', proleakproof => 't', prorettype => 'bool',
> + proargtypes => 'xid8 xid8', prosrc => 'xid8neq' },
>
> Basically the names of not-equal functions for most data types are
> something like "xxxne" not "xxxneq". So IMO it's better to use the name
> "xid8ne" instead of "xid8neq" here.
Huh. You are right, but the existing function xidneq is an exception.
It's not clear which one to follow. I will take your advice and use
xid8ne. We could potentially change xidneq to xidne too, but it's
user-visible.
> /*
> - * do a TransactionId -> txid conversion for an XID near the given epoch
> + * Do a TransactionId -> fxid conversion for an XID that is known to precede
> + * the given 'next_fxid'.
>*/
> -static txid
> -convert_xid(TransactionId xid, const TxidEpoch *state)
> +static FullTransactionId
> +convert_xid(TransactionId xid, FullTransactionId next_fxid)
>
> As the comment suggests, this function assumes that "xid" must
> precede "next_fxid". But there is no check for the assumption.
> Isn't it better to add, e.g., an assertion checking that?
> Or convert_xid() should handle the case where "xid" follows
> "next_fxid" like the orignal convert_xid() does. That is, don't
> apply the following change.
>
> - if (xid > state->last_xid &&
> - TransactionIdPrecedes(xid, state->last_xid))
> + if (xid > next_xid)
> epoch--;
> - else if (xid < state->last_xid &&
> -TransactionIdFollows(xid, state->last_xid))
> - epoch++;
I need to think about this part some more, but I wanted to share
responses to the rest of your review now. I'll return to this point
next week.
> > Does anyone want to object to the txid/xid8 type punning scheme or
> > long term txid-sunsetting plan?
>
> No. +1 to retire txid someday.
Cool. Let's do that in a couple of years.
0001-Add-SQL-type-xid8-to-expose-FullTransactionId-to--v6.patch
Description: Binary data
0002-Introduce-xid8-variants-of-the-txid_XXX-fmgr-func-v6.patch
Description: Binary data
Re: Should we add xid_current() or a int8->xid cast?
On 2020/01/28 14:05, Thomas Munro wrote:
On Tue, Dec 3, 2019 at 6:56 AM Mark Dilger wrote:
On 11/30/19 5:14 PM, Thomas Munro wrote:
On Sun, Dec 1, 2019 at 12:22 PM Mark Dilger wrote:
These two patches (v3) no longer apply cleanly. Could you please
rebase?
Hi Mark,
Thanks. Here's v4.
Thanks, Thomas.
The new patches apply cleanly and pass 'installcheck'.
I rebased, fixed the "xid_snapshot_xip" problem spotted by Takao Fujii
that I had missed earlier, updated a couple of error messages to refer
to the new names (even when using the old functions) and ran
check-world and some simple manual tests on an -m32 build just to be
paranoid. Here are the versions of these patches I'd like to commit.
Thanks for the patches! Here are my minor comments.
Isn't it better to add also xid8lt, xid8gt, xid8le, and xid8ge?
xid8 and xid8_snapshot should be documented in datatype.sgml like
txid_snapshot is?
logicaldecoding.sgml and monitoring.sgml still referred to txid_xxx.
They should be updated so that new xid8_xxx is used?
In func.sgml, the table "Snapshot Components" is described still based
on txid. It should be updated so that it uses xid8, instead?
+# xid_ops
+{ amopfamily => 'hash/xid8_ops', amoplefttype => 'xid8', amoprighttype =>
'xid8',
+ amopstrategy => '1', amopopr => '=(xid8,xid8)', amopmethod => 'hash' },
"xid_ops" in the comment should be "xid8_ops".
+{ oid => '9558',
+ proname => 'xid8neq', proleakproof => 't', prorettype => 'bool',
+ proargtypes => 'xid8 xid8', prosrc => 'xid8neq' },
Basically the names of not-equal functions for most data types are
something like "xxxne" not "xxxneq". So IMO it's better to use the name
"xid8ne" instead of "xid8neq" here.
/*
- * do a TransactionId -> txid conversion for an XID near the given epoch
+ * Do a TransactionId -> fxid conversion for an XID that is known to precede
+ * the given 'next_fxid'.
*/
-static txid
-convert_xid(TransactionId xid, const TxidEpoch *state)
+static FullTransactionId
+convert_xid(TransactionId xid, FullTransactionId next_fxid)
As the comment suggests, this function assumes that "xid" must
precede "next_fxid". But there is no check for the assumption.
Isn't it better to add, e.g., an assertion checking that?
Or convert_xid() should handle the case where "xid" follows
"next_fxid" like the orignal convert_xid() does. That is, don't
apply the following change.
- if (xid > state->last_xid &&
- TransactionIdPrecedes(xid, state->last_xid))
+ if (xid > next_xid)
epoch--;
- else if (xid < state->last_xid &&
-TransactionIdFollows(xid, state->last_xid))
- epoch++;
Does anyone want to object to the txid/xid8 type punning scheme or
long term txid-sunsetting plan?
No. +1 to retire txid someday.
Regards,
--
Fujii Masao
NTT DATA CORPORATION
Advanced Platform Technology Group
Research and Development Headquarters
Re: Should we add xid_current() or a int8->xid cast?
On Tue, Dec 3, 2019 at 6:56 AM Mark Dilger wrote: > On 11/30/19 5:14 PM, Thomas Munro wrote: > > On Sun, Dec 1, 2019 at 12:22 PM Mark Dilger wrote: > >> These two patches (v3) no longer apply cleanly. Could you please > >> rebase? > > > > Hi Mark, > > Thanks. Here's v4. > > Thanks, Thomas. > > The new patches apply cleanly and pass 'installcheck'. I rebased, fixed the "xid_snapshot_xip" problem spotted by Takao Fujii that I had missed earlier, updated a couple of error messages to refer to the new names (even when using the old functions) and ran check-world and some simple manual tests on an -m32 build just to be paranoid. Here are the versions of these patches I'd like to commit. Does anyone want to object to the txid/xid8 type punning scheme or long term txid-sunsetting plan? 0001-Add-SQL-type-xid8-to-expose-FullTransactionId-to--v5.patch Description: Binary data 0002-Introduce-xid8-variants-of-the-txid_XXX-fmgr-func-v5.patch Description: Binary data
Re: Should we add xid_current() or a int8->xid cast?
On 11/30/19 5:14 PM, Thomas Munro wrote: On Sun, Dec 1, 2019 at 12:22 PM Mark Dilger wrote: These two patches (v3) no longer apply cleanly. Could you please rebase? Hi Mark, Thanks. Here's v4. Thanks, Thomas. The new patches apply cleanly and pass 'installcheck'. -- Mark Dilger
Re: Should we add xid_current() or a int8->xid cast?
On Sun, Dec 1, 2019 at 12:22 PM Mark Dilger wrote: > These two patches (v3) no longer apply cleanly. Could you please > rebase? Hi Mark, Thanks. Here's v4. 0001-Add-SQL-type-xid8-to-expose-FullTransactionId-to--v4.patch Description: Binary data 0002-Introduce-xid8-variants-of-the-txid_XXX-fmgr-func-v4.patch Description: Binary data
Re: Should we add xid_current() or a int8->xid cast?
On 11/3/19 2:43 PM, Thomas Munro wrote:
On Tue, Oct 29, 2019 at 5:23 PM btfujiitkp wrote:
Thomas Munro writes:
On Sun, Sep 1, 2019 at 5:04 PM Thomas Munro
wrote:
Adding to CF.
Rebased. An OID clashed so re-roll the dice. Also spotted a typo.
I have some questions in this code.
Thanks for looking at the patch.
First,
"FullTransactionIdPrecedes(xmax, val)" is not equal to "val >= xmax" of
the previous code. "FullTransactionIdPrecedes(xmax, val)" expresses
"val > xmax". Is it all right?
@@ -384,15 +324,17 @@ parse_snapshot(const char *str)
while (*str != '\0')
{
/* read next value */
- val = str2txid(str, );
+ val = FullTransactionIdFromU64(pg_strtouint64(str, , 10));
str = endp;
/* require the input to be in order */
- if (val < xmin || val >= xmax || val < last_val)
+ if (FullTransactionIdPrecedes(val, xmin) ||
+ FullTransactionIdPrecedes(xmax, val) ||
+ FullTransactionIdPrecedes(val, last_val))
In addition to it, as to current TransactionId(not FullTransactionId)
comparison, when we express ">=" of TransactionId, we use
"TransactionIdFollowsOrEquals". this method is referred by some methods.
On the other hand, FullTransactionIdFollowsOrEquals has not implemented
yet. So, how about implementing this method?
Good idea. I added the missing variants:
+#define FullTransactionIdPrecedesOrEquals(a, b) ((a).value <= (b).value)
+#define FullTransactionIdFollows(a, b) ((a).value > (b).value)
+#define FullTransactionIdFollowsOrEquals(a, b) ((a).value >= (b).value)
Second,
About naming rule, "8" of xid8 means 8 bytes, but "8" has different
meaning in each situation. For example, int8 of PostgreSQL means 8
bytes, int8 of C language means 8 bits. If 64 is used, it just means 64
bits. how about xid64()?
In C, the typenames use bits, by happy coincidence similar to the C99
stdint.h typenames (int32_t etc) that we should perhaps eventually
switch to.
In SQL, the types have names based on the number of bytes: int2, int4,
int8, float4, float8, not conforming to any standard but established
over 3 decades ago and also understood by a few other SQL systems.
That's unfortunate, but I can't see that ever changing. I thought
that it would make most sense for the SQL type to be called xid8,
though admittedly it doesn't quite fit the pattern because xid is not
called xid4. There is another example a bit like that: macaddr (6
bytes) and macaccdr8 (8 bytes). As for the C type, we use
TransactionId and FullTransactionId (rather than, say, xid32 and
xid64).
In the attached I also took Tom's advice and used unused_oids script
to pick random OIDs >= 8000 for all new objects (ignoring nearby
comments about the range of OIDs used in different sections of the
file).
These two patches (v3) no longer apply cleanly. Could you please
rebase?
--
Mark Dilger
Re: Should we add xid_current() or a int8->xid cast?
On Wed, Nov 20, 2019 at 5:43 PM imai.yoshik...@fujitsu.com wrote: > Is there any agreement we can throw the wraparound problem away if we adopt > FullTransactionId? Here is one argument for why 64 bits ought to be enough: we use 64 bit LSNs for the WAL, and it usually takes more than one byte of WAL to consume a transaction. If you write about 500MB of WAL per second, your system will break in about a thousand years due to LSN wraparound, that is, assuming the earth hasn't been destroyed to make way for a hyperspace bypass, but either way you will probably still have some spare full transaction IDs. That's fun to think about, but unfortunately it's not easy to figure out how to retrofit FullTransactionId into enough places to make wraparounds go away in the traditional heap. It's a goal of at least a couple of ongoing new AM projects to not have that problem, and I figured it was a good idea to lay down very basic facilities for that, trivial as they might be, and see where else they can be useful...
RE: Should we add xid_current() or a int8->xid cast?
Hi Thomas,
Please let me ask something about wraparound problem.
+static FullTransactionId
+convert_xid(TransactionId xid, FullTransactionId next_fxid)
{
- uint64 epoch;
+ TransactionId next_xid = XidFromFullTransactionId(next_fxid);
+ uint32 epoch = EpochFromFullTransactionId(next_fxid);
...
- /* xid can be on either side when near wrap-around */
- epoch = (uint64) state->epoch;
- if (xid > state->last_xid &&
- TransactionIdPrecedes(xid, state->last_xid))
+ if (xid > next_xid)
epoch--;
- else if (xid < state->last_xid &&
-TransactionIdFollows(xid, state->last_xid))
- epoch++;
- return (epoch << 32) | xid;
+ return FullTransactionIdFromEpochAndXid(epoch, xid);
ISTM codes for wraparound are deleted. Is that correct?
I couldn't have read all related threads about using FullTransactionId but
does using FullTransactionId avoid wraparound problem?
If we consider below conditions, we can say it's difficult to see wraparound
with current disk like SSD (2GB/s) or memory DDR4 (34GB/s), but if we can use
more high-spec hardware like HBM3 (2048GB/s), we can see wraparound. Or do
I say silly things?
* 10 year system ( < 2^4 )
* 1 year = 31536000 ( = 60 * 60 * 24 * 365) secs ( < 2^25 )
* 2^35 ( = 2^64 / 2^4 / 2^25) transactions we can use in each seconds
* we can write at (2^5 * 2^30 * n) bytes/sec = (32 * n) GB/sec if we use 'n'
bytes for each transactions.
Is there any agreement we can throw the wraparound problem away if we adopt
FullTransactionId?
Thanks
--
Yoshikazu Imai
Re: Should we add xid_current() or a int8->xid cast?
On Tue, Oct 29, 2019 at 5:23 PM btfujiitkp
wrote:
> Thomas Munro writes:
>> On Sun, Sep 1, 2019 at 5:04 PM Thomas Munro
>> wrote:
>>> Adding to CF.
>
>> Rebased. An OID clashed so re-roll the dice. Also spotted a typo.
>
I have some questions in this code.
Thanks for looking at the patch.
First,
"FullTransactionIdPrecedes(xmax, val)" is not equal to "val >= xmax"
of
the previous code. "FullTransactionIdPrecedes(xmax, val)" expresses
"val > xmax". Is it all right?
@@ -384,15 +324,17 @@ parse_snapshot(const char *str)
while (*str != '\0')
{
/* read next value */
- val = str2txid(str, );
+ val = FullTransactionIdFromU64(pg_strtouint64(str,
, 10));
str = endp;
/* require the input to be in order */
- if (val < xmin || val >= xmax || val < last_val)
+ if (FullTransactionIdPrecedes(val, xmin) ||
+ FullTransactionIdPrecedes(xmax, val) ||
+ FullTransactionIdPrecedes(val, last_val))
In addition to it, as to current TransactionId(not FullTransactionId)
comparison, when we express ">=" of TransactionId, we use
"TransactionIdFollowsOrEquals". this method is referred by some
methods.
On the other hand, FullTransactionIdFollowsOrEquals has not
implemented
yet. So, how about implementing this method?
Good idea. I added the missing variants:
+#define FullTransactionIdPrecedesOrEquals(a, b) ((a).value <=
(b).value)
+#define FullTransactionIdFollows(a, b) ((a).value > (b).value)
+#define FullTransactionIdFollowsOrEquals(a, b) ((a).value >=
(b).value)
Thank you for your patch.
It looks good.
Second,
About naming rule, "8" of xid8 means 8 bytes, but "8" has different
meaning in each situation. For example, int8 of PostgreSQL means 8
bytes, int8 of C language means 8 bits. If 64 is used, it just means
64
bits. how about xid64()?
In C, the typenames use bits, by happy coincidence similar to the C99
stdint.h typenames (int32_t etc) that we should perhaps eventually
switch to.
In SQL, the types have names based on the number of bytes: int2, int4,
int8, float4, float8, not conforming to any standard but established
over 3 decades ago and also understood by a few other SQL systems.
That's unfortunate, but I can't see that ever changing. I thought
that it would make most sense for the SQL type to be called xid8,
though admittedly it doesn't quite fit the pattern because xid is not
called xid4. There is another example a bit like that: macaddr (6
bytes) and macaccdr8 (8 bytes). As for the C type, we use
TransactionId and FullTransactionId (rather than, say, xid32 and
xid64).
That makes sense.
Anyway,
In the pg_proc.dat, "xid_snapshot_xip" should be "xid8_snapshot_xip".
And some parts of 0002 patch are rejected when I patch 0002 after
patching 0001.
regards
Re: Should we add xid_current() or a int8->xid cast?
On Tue, Oct 29, 2019 at 5:23 PM btfujiitkp wrote:
> > Thomas Munro writes:
> >> On Sun, Sep 1, 2019 at 5:04 PM Thomas Munro
> >> wrote:
> >>> Adding to CF.
> >
> >> Rebased. An OID clashed so re-roll the dice. Also spotted a typo.
> >
>
> I have some questions in this code.
Thanks for looking at the patch.
> First,
> "FullTransactionIdPrecedes(xmax, val)" is not equal to "val >= xmax" of
> the previous code. "FullTransactionIdPrecedes(xmax, val)" expresses
> "val > xmax". Is it all right?
>
> @@ -384,15 +324,17 @@ parse_snapshot(const char *str)
> while (*str != '\0')
> {
> /* read next value */
> - val = str2txid(str, );
> + val = FullTransactionIdFromU64(pg_strtouint64(str, ,
> 10));
> str = endp;
>
> /* require the input to be in order */
> - if (val < xmin || val >= xmax || val < last_val)
> + if (FullTransactionIdPrecedes(val, xmin) ||
> + FullTransactionIdPrecedes(xmax, val) ||
> + FullTransactionIdPrecedes(val, last_val))
>
> In addition to it, as to current TransactionId(not FullTransactionId)
> comparison, when we express ">=" of TransactionId, we use
> "TransactionIdFollowsOrEquals". this method is referred by some methods.
> On the other hand, FullTransactionIdFollowsOrEquals has not implemented
> yet. So, how about implementing this method?
Good idea. I added the missing variants:
+#define FullTransactionIdPrecedesOrEquals(a, b) ((a).value <= (b).value)
+#define FullTransactionIdFollows(a, b) ((a).value > (b).value)
+#define FullTransactionIdFollowsOrEquals(a, b) ((a).value >= (b).value)
> Second,
> About naming rule, "8" of xid8 means 8 bytes, but "8" has different
> meaning in each situation. For example, int8 of PostgreSQL means 8
> bytes, int8 of C language means 8 bits. If 64 is used, it just means 64
> bits. how about xid64()?
In C, the typenames use bits, by happy coincidence similar to the C99
stdint.h typenames (int32_t etc) that we should perhaps eventually
switch to.
In SQL, the types have names based on the number of bytes: int2, int4,
int8, float4, float8, not conforming to any standard but established
over 3 decades ago and also understood by a few other SQL systems.
That's unfortunate, but I can't see that ever changing. I thought
that it would make most sense for the SQL type to be called xid8,
though admittedly it doesn't quite fit the pattern because xid is not
called xid4. There is another example a bit like that: macaddr (6
bytes) and macaccdr8 (8 bytes). As for the C type, we use
TransactionId and FullTransactionId (rather than, say, xid32 and
xid64).
In the attached I also took Tom's advice and used unused_oids script
to pick random OIDs >= 8000 for all new objects (ignoring nearby
comments about the range of OIDs used in different sections of the
file).
0001-Add-SQL-type-xid8-to-expose-FullTransactionId-to--v3.patch
Description: Binary data
0002-Introduce-xid8-variants-of-the-txid_XXX-fmgr-func-v3.patch
Description: Binary data
Re: Should we add xid_current() or a int8->xid cast?
Thomas Munro writes:
On Sun, Sep 1, 2019 at 5:04 PM Thomas Munro
wrote:
Adding to CF.
Rebased. An OID clashed so re-roll the dice. Also spotted a typo.
I have some questions in this code.
First,
"FullTransactionIdPrecedes(xmax, val)" is not equal to "val >= xmax" of
the previous code. "FullTransactionIdPrecedes(xmax, val)" expresses
"val > xmax". Is it all right?
@@ -384,15 +324,17 @@ parse_snapshot(const char *str)
while (*str != '\0')
{
/* read next value */
- val = str2txid(str, );
+ val = FullTransactionIdFromU64(pg_strtouint64(str, , 10));
str = endp;
/* require the input to be in order */
- if (val < xmin || val >= xmax || val < last_val)
+ if (FullTransactionIdPrecedes(val, xmin) ||
+ FullTransactionIdPrecedes(xmax, val) ||
+ FullTransactionIdPrecedes(val, last_val))
In addition to it, as to current TransactionId(not FullTransactionId)
comparison, when we express ">=" of TransactionId, we use
"TransactionIdFollowsOrEquals". this method is referred by some methods.
On the other hand, FullTransactionIdFollowsOrEquals has not implemented
yet. So, how about implementing this method?
Second,
About naming rule, "8" of xid8 means 8 bytes, but "8" has different
meaning in each situation. For example, int8 of PostgreSQL means 8
bytes, int8 of C language means 8 bits. If 64 is used, it just means 64
bits. how about xid64()?
regards,
Takao Fujii
Re: Should we add xid_current() or a int8->xid cast?
Thomas Munro writes: > On Sun, Sep 1, 2019 at 5:04 PM Thomas Munro wrote: >> Adding to CF. > Rebased. An OID clashed so re-roll the dice. Also spotted a typo. FWIW, I'd move *all* the OIDs added by this patch up to >= 8000. I don't feel a strong need to fill in the gaps in the low-numbered OIDs, and people who do try that are likely to hit problems of the sort you just did. regards, tom lane
Re: Should we add xid_current() or a int8->xid cast?
On Sun, Sep 1, 2019 at 5:04 PM Thomas Munro wrote: > Adding to CF. Rebased. An OID clashed so re-roll the dice. Also spotted a typo. -- Thomas Munro https://enterprisedb.com 0001-Add-SQL-type-xid8-to-expose-FullTransactionId-to--v2.patch Description: Binary data 0002-Introduce-xid8-variants-of-the-txid_XXX-fmgr-func-v2.patch Description: Binary data
Re: Should we add xid_current() or a int8->xid cast?
On Fri, Aug 2, 2019 at 10:42 PM Thomas Munro wrote: > On Thu, Jul 25, 2019 at 1:11 PM Thomas Munro wrote: > > On Thu, Jul 25, 2019 at 12:42 PM Tom Lane wrote: > > > Andres Freund writes: > > > > On 2019-07-24 20:34:39 -0400, Tom Lane wrote: > > > >> Yeah, I would absolutely NOT recommend that you open that can of worms > > > >> right now. We have looked at adding unsigned integer types in the past > > > >> and it looked like a mess. > > > > > > > I assume Thomas was thinking more of another bespoke type like xid, just > > > > wider. There's some notational advantage in not being able to > > > > immediately do math etc on xids. > > > > > > Well, we could invent an xid8 type if we want, just don't try to make > > > it part of the numeric hierarchy (as indeed xid isn't). > > > > Yeah, I meant an xid64/xid8/fxid/pg_something/... type that isn't a > > kind of number. I thought about how to deal with the transition to xid8 for the txid_XXX() family of functions. The best idea I've come up with so far is to create a parallel xid8_XXX() family of functions, and declare the bigint-based functions to be deprecated, and threaten to drop them from a future release. The C code for the two families can be the same (it's a bit of a dirty trick, but only until the txid_XXX() variants go away). Here's a PoC patch demonstrating that. Not tested much, yet, probably needs some more work, but I wanted to see if others thought the idea was terrible first. I wonder if there is a better way to share hash functions than the hack in check_hash_func_signature(), which I had to extend to cover xid8. Adding to CF. -- Thomas Munro https://enterprisedb.com 0001-Add-SQL-type-xid8-to-expose-FullTransactionId-to-use.patch Description: Binary data 0002-Introduce-xid8-variants-of-the-txid_XXX-fmgr-functio.patch Description: Binary data
Re: Should we add xid_current() or a int8->xid cast?
On Thu, Jul 25, 2019 at 1:11 PM Thomas Munro wrote: > On Thu, Jul 25, 2019 at 12:42 PM Tom Lane wrote: > > Andres Freund writes: > > > On 2019-07-24 20:34:39 -0400, Tom Lane wrote: > > >> Yeah, I would absolutely NOT recommend that you open that can of worms > > >> right now. We have looked at adding unsigned integer types in the past > > >> and it looked like a mess. > > > > > I assume Thomas was thinking more of another bespoke type like xid, just > > > wider. There's some notational advantage in not being able to > > > immediately do math etc on xids. > > > > Well, we could invent an xid8 type if we want, just don't try to make > > it part of the numeric hierarchy (as indeed xid isn't). > > Yeah, I meant an xid64/xid8/fxid/pg_something/... type that isn't a > kind of number. I played around with an xid8 type over here (not tested much yet, in particular not tested on 32 bit box): https://www.postgresql.org/message-id/CA%2BhUKGKbQtX8E5TEdcZaYhTxqLqrvcpN1Vjb7eCu2bz5EACZbw%40mail.gmail.com -- Thomas Munro https://enterprisedb.com
Re: Should we add xid_current() or a int8->xid cast?
On Thu, Jul 25, 2019 at 12:42 PM Tom Lane wrote: > Andres Freund writes: > > On 2019-07-24 20:34:39 -0400, Tom Lane wrote: > >> Yeah, I would absolutely NOT recommend that you open that can of worms > >> right now. We have looked at adding unsigned integer types in the past > >> and it looked like a mess. > > > I assume Thomas was thinking more of another bespoke type like xid, just > > wider. There's some notational advantage in not being able to > > immediately do math etc on xids. > > Well, we could invent an xid8 type if we want, just don't try to make > it part of the numeric hierarchy (as indeed xid isn't). Yeah, I meant an xid64/xid8/fxid/pg_something/... type that isn't a kind of number. -- Thomas Munro https://enterprisedb.com
Re: Should we add xid_current() or a int8->xid cast?
Andres Freund writes: > On 2019-07-24 20:34:39 -0400, Tom Lane wrote: >> Yeah, I would absolutely NOT recommend that you open that can of worms >> right now. We have looked at adding unsigned integer types in the past >> and it looked like a mess. > I assume Thomas was thinking more of another bespoke type like xid, just > wider. There's some notational advantage in not being able to > immediately do math etc on xids. Well, we could invent an xid8 type if we want, just don't try to make it part of the numeric hierarchy (as indeed xid isn't). regards, tom lane
Re: Should we add xid_current() or a int8->xid cast?
Hi, On 2019-07-24 20:34:39 -0400, Tom Lane wrote: > Yeah, I would absolutely NOT recommend that you open that can of worms > right now. We have looked at adding unsigned integer types in the past > and it looked like a mess. I assume Thomas was thinking more of another bespoke type like xid, just wider. There's some notational advantage in not being able to immediately do math etc on xids. - Andres
Re: Should we add xid_current() or a int8->xid cast?
Andres Freund writes: > On 2019-07-25 12:20:58 +1200, Thomas Munro wrote: >> On Thu, Jul 25, 2019 at 12:06 PM Andres Freund wrote: >>> Seems easiest to just add xid_current(), or add a cast from int8 to xid >>> (probably explicit?) that handles the wraparound logic correctly? >> Yeah, I was wondering about that. int8 isn't really the right type, >> since FullTransactionId is unsigned. > For now that doesn't seem that big an impediment... Yeah, I would absolutely NOT recommend that you open that can of worms right now. We have looked at adding unsigned integer types in the past and it looked like a mess. I think an explicit cast is a reasonable thing to add, though. regards, tom lane
Re: Should we add xid_current() or a int8->xid cast?
Hi, On 2019-07-25 12:20:58 +1200, Thomas Munro wrote: > On Thu, Jul 25, 2019 at 12:06 PM Andres Freund wrote: > > we have txid_current(), which returns an int8. But there's no convenient > > way to convert that to type 'xid'. Which is fairly inconvenient, given > > that we expose xids in various places. > > > > My current need for this was just a regression test to make sure that > > system columns (xmin/xmax in particular) don't get broken again for ON > > CONFLICT. But I've needed this before in other scenarios - e.g. age(xid) > > can be useful to figure out how old a transaction is, but age() doesn't > > work with txid_current()'s return value. > > > > Seems easiest to just add xid_current(), or add a cast from int8 to xid > > (probably explicit?) that handles the wraparound logic correctly? > > Yeah, I was wondering about that. int8 isn't really the right type, > since FullTransactionId is unsigned. For now that doesn't seem that big an impediment... > If we had a SQL type for 64 bit xids, it should be convertible to xid, > and the reverse conversion should require a more complicated dance. > Of course we can't casually change txid_current() without annoying > people who are using it, so perhaps if we invent a new SQL type we > should also make a new function that returns it. Possibly we could add a fullxid or xid8, xid64, pg_xid64, ... type, and have an implicit cast to int8? Greetings, Andres Freund
Re: Should we add xid_current() or a int8->xid cast?
On Thu, Jul 25, 2019 at 12:06 PM Andres Freund wrote: > we have txid_current(), which returns an int8. But there's no convenient > way to convert that to type 'xid'. Which is fairly inconvenient, given > that we expose xids in various places. > > My current need for this was just a regression test to make sure that > system columns (xmin/xmax in particular) don't get broken again for ON > CONFLICT. But I've needed this before in other scenarios - e.g. age(xid) > can be useful to figure out how old a transaction is, but age() doesn't > work with txid_current()'s return value. > > Seems easiest to just add xid_current(), or add a cast from int8 to xid > (probably explicit?) that handles the wraparound logic correctly? Yeah, I was wondering about that. int8 isn't really the right type, since FullTransactionId is unsigned. If we had a SQL type for 64 bit xids, it should be convertible to xid, and the reverse conversion should require a more complicated dance. Of course we can't casually change txid_current() without annoying people who are using it, so perhaps if we invent a new SQL type we should also make a new function that returns it. -- Thomas Munro https://enterprisedb.com
Should we add xid_current() or a int8->xid cast?
Hi, we have txid_current(), which returns an int8. But there's no convenient way to convert that to type 'xid'. Which is fairly inconvenient, given that we expose xids in various places. My current need for this was just a regression test to make sure that system columns (xmin/xmax in particular) don't get broken again for ON CONFLICT. But I've needed this before in other scenarios - e.g. age(xid) can be useful to figure out how old a transaction is, but age() doesn't work with txid_current()'s return value. Seems easiest to just add xid_current(), or add a cast from int8 to xid (probably explicit?) that handles the wraparound logic correctly? Greetings, Andres Freund
