On Tue, Oct 29, 2019 at 5:23 PM btfujiitkp <btfujii...@oss.nttdata.com>
wrote:
> Thomas Munro <thomas.mu...@gmail.com> writes:
>> On Sun, Sep 1, 2019 at 5:04 PM Thomas Munro <thomas.mu...@gmail.com>
>> 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, &endp);
+ val = FullTransactionIdFromU64(pg_strtouint64(str,
&endp, 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
