On Sat, Nov 9, 2024 at 12:41 PM Dilip Kumar <dilipbal...@gmail.com> wrote:
>
> On Fri, Nov 8, 2024 at 8:36 PM Tom Lane <t...@sss.pgh.pa.us> wrote:
>>
>> Dilip Kumar <dilipbal...@gmail.com> writes:
>> > IIRC, In catalog we intentionally left it as Oid because RelFileNumber is
>> > an internal typedef bug, it is not an exposed datatype, so probably we can
>> > not use it in catalog.
>>
>> We could declare it as RelFileNumber so that that is what C code sees,
>> and teach Catalog.pm to translate that to OID in the emitted catalog
>> contents.
>
>
> Yeah that make sense and yes we can actually keep this change
>
>>
>> I think you'd have to do that to avoid a ton of false
>> positives when you make RelFileNumber into a struct, and we might as
>> well keep the result.
>
>
> Even after this, there were tons of false positives, whenever using any
> comparison operator on relfilenumbers[1] and there are tons of those, or
> using them in log messages with %u [2]. Anyway, I have gone through those
> manually and after ignoring all false positives here is what I got, PFA patch
> (odd 25 places where I have fixed usage of Oid instead of RelFileNumbers).
>
>
> [1]
> ../../../../src/include/storage/buf_internals.h:157:20: error: invalid
> operands to binary expression ('const RelFileNumber' (aka 'const struct
> RelFileNumber') and 'const RelFileNumber')
> (tag1->relNumber == tag2->relNumber)
>
> [2]
> fsmpage.c:277:47: warning: format specifies type 'unsigned int' but the
> argument has type 'RelFileNumber' (aka 'struct RelFileNumber') [-Wformat]
> blknum, rlocator.spcOid, rlocator.dbOid,
> rlocator.relNumber);
>
Other than the changes, I have made in patch in my previous email
there are a couple of more items we can consider for this change, but
I am not sure so listing down here
1. We are using PG_RETURN_OID() for returning the relfilenumber,
should we introduce something like PG_RETURN_RELFILENUMBER() ? e.g
pg_relation_filenode()
2. We are using PG_GETARG_OID() for getting the relfilenumber as an
external function argument, shall we introduce something like
PG_GETARG_RELFILENUMBER() ? e.g.
binary_upgrade_set_next_heap_relfilenode()
3. info.c:611:23: error: assigning to 'RelFileNumber' (aka 'struct
RelFileNumber') from incompatible type 'Oid' (aka 'unsigned int')
curr->relfilenumber = atooid(PQgetvalue(res, relnum,
i_relfilenumber));
4. Also using ObjectIdGetDatum() and DatumGetObjectId() for
relfilenumber so shall we introduce a new function i.e
RelFileNumberGetDatum() and DatumGetRelFileNumber()
--
Regards,
Dilip Kumar
EnterpriseDB: http://www.enterprisedb.com
