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
<masao(dot)fujii(at)oss(dot)nttdata(dot)com> 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 '0x100000000'::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.