On Tue, Oct 29, 2019 at 5:23 PM btfujiitkp <btfujiitkp@oss.nttdata.com> wrote:
> > Thomas Munro <thomas.munro@gmail.com> writes:
> >> On Sun, Sep 1, 2019 at 5:04 PM Thomas Munro <thomas.munro@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)
> 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).
