Thread: Use T_IntList for uint32

Currently pg_list.h doesn't have a variant for uint32 list (like
T_UIntList), is there a reason other than that that we don't need it
till now? I see that one can use T_OidList instead (as Oid is uint32)
but I am not sure if that is a good idea to say use it for maintaining
a list of TransactionIds. We need to use such a variant of T_UIntlist
at one place in the patch for the "streaming of in-progress
transactions" [1].

The current use case is as below:

typedef struct RelationSyncEntry
{
..
List    *streamed_txns; /* streamed toplevel transactions with this
* schema */

/*
 * We expect relatively small number of streamed transactions.
 */
static bool
get_schema_sent_in_streamed_txn(RelationSyncEntry *entry, TransactionId xid)
{
..
foreach (lc, entry->streamed_txns)
{
if (xid == lfirst_int(lc))
return true;
}
..
}

/*
 * Add the xid in the rel sync entry for which we have already sent the schema
 * of the relation.
 */
static void
set_schema_sent_in_streamed_txn(RelationSyncEntry *entry, TransactionId xid)
{
..
entry->streamed_txns = lappend_int(entry->streamed_txns, xid);
..
}

Now, as far as I can see there is no problem in using T_IntList in
such usage because we are not going to fetch stored unsigned value as
a signed value, so the comparison in get_schema_sent_in_streamed_txn
should work well. However, still, I thought it would be better if
there is a built-in T_UIntList.

Thoughts?

[1] - https://www.postgresql.org/message-id/CAFiTN-u_4uvGjAPO536m-YsR%2Bk9J-%3Dwqx2K9CtrFOHcJPa7Szg%40mail.gmail.com

-- 
With Regards,
Amit Kapila.

On Mon, Aug 31, 2020 at 4:59 PM Amit Kapila <amit.kapila16@gmail.com> wrote:

> Now, as far as I can see there is no problem in using T_IntList in
> such usage because we are not going to fetch stored unsigned value as
> a signed value, so the comparison in get_schema_sent_in_streamed_txn
> should work well. However, still, I thought it would be better if
> there is a built-in T_UIntList.
>
> Thoughts?

May be we should have separate list APIs for XID like OID, in case we
change underlying datatype of XID in future (unlikely but we have had
discussion about 64bit XIDs in the past). Apart from that it helps us
track code which deals with XID lists.


-- 
Best Wishes,
Ashutosh Bapat

Amit Kapila <amit.kapila16@gmail.com> writes:
> Currently pg_list.h doesn't have a variant for uint32 list (like
> T_UIntList), is there a reason other than that that we don't need it
> till now?

I'm not in favor of adding another list variant code just for that;
the overhead is nonzero, and the gain negligible.  (I think the reason
we have OID lists is the idea that someday we'd want to make OID 64-bit.
A list type defined as "UInt" would offer no such future-proofing.)

The code you quote probably ought to be casting the result of lfirst_int
to uint32, but I see no reason to work harder.

            regards, tom lane

On Mon, Aug 31, 2020 at 6:58 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>
> Amit Kapila <amit.kapila16@gmail.com> writes:
> > Currently pg_list.h doesn't have a variant for uint32 list (like
> > T_UIntList), is there a reason other than that that we don't need it
> > till now?
>
> I'm not in favor of adding another list variant code just for that;
> the overhead is nonzero, and the gain negligible.  (I think the reason
> we have OID lists is the idea that someday we'd want to make OID 64-bit.
> A list type defined as "UInt" would offer no such future-proofing.)
>

I agree with this and I also don't want to add more code for this
unless it is really required. Having said that, the case for Xids is
similar to Oids where someday we might want it to be 64-bit but we can
leave it for another day.

> The code you quote probably ought to be casting the result of lfirst_int
> to uint32, but I see no reason to work harder.
>

Sounds reasonable, will use this for now.

-- 
With Regards,
Amit Kapila.

On Mon, Aug 31, 2020 at 5:44 PM Ashutosh Bapat
<ashutosh.bapat.oss@gmail.com> wrote:
>
> On Mon, Aug 31, 2020 at 4:59 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
>
> > Now, as far as I can see there is no problem in using T_IntList in
> > such usage because we are not going to fetch stored unsigned value as
> > a signed value, so the comparison in get_schema_sent_in_streamed_txn
> > should work well. However, still, I thought it would be better if
> > there is a built-in T_UIntList.
> >
> > Thoughts?
>
> May be we should have separate list APIs for XID like OID, in case we
> change underlying datatype of XID in future (unlikely but we have had
> discussion about 64bit XIDs in the past). Apart from that it helps us
> track code which deals with XID lists.
>

This is a valid point but I think for now I will go with Tom's
suggestion as the demand for this seems low at this stage. I don't
want to introduce a new set of APIs just for one use case especially
when we can work with existing APIs.

-- 
With Regards,
Amit Kapila.

Amit Kapila <amit.kapila16@gmail.com> writes:
> On Mon, Aug 31, 2020 at 5:44 PM Ashutosh Bapat
> <ashutosh.bapat.oss@gmail.com> wrote:
>> May be we should have separate list APIs for XID like OID, in case we
>> change underlying datatype of XID in future (unlikely but we have had
>> discussion about 64bit XIDs in the past). Apart from that it helps us
>> track code which deals with XID lists.

> This is a valid point but I think for now I will go with Tom's
> suggestion as the demand for this seems low at this stage. I don't
> want to introduce a new set of APIs just for one use case especially
> when we can work with existing APIs.

I've occasionally wondered about having exactly two physical List
implementations, one for 32-bit payloads and one for 64-bit payloads, and
then putting a trivial macros-or-static-inlines layer in front of that
that uses casts to supply variants for pointers, signed ints, unsigned
ints, etc etc.  There hasn't yet been enough reason to pursue doing it
though.

            regards, tom lane

On Tue, Sep 01, 2020 at 01:27:15AM -0400, Tom Lane wrote:
> I've occasionally wondered about having exactly two physical List
> implementations, one for 32-bit payloads and one for 64-bit payloads, and
> then putting a trivial macros-or-static-inlines layer in front of that
> that uses casts to supply variants for pointers, signed ints, unsigned
> ints, etc etc.  There hasn't yet been enough reason to pursue doing it
> though.

FWIW, moving pg_list.h & co to have just two list implementations, based
on say bits32 and a new bits64 is something I was thinking about while
reading this thread.
--
Michael