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 <hornschnorter(at)gmail(dot)com> wrote:
>> On 11/30/19 5:14 PM, Thomas Munro wrote:
>>> On Sun, Dec 1, 2019 at 12:22 PM Mark Dilger <hornschnorter(at)gmail(dot)com> 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