Re: making relfilenodes 56 bits

On Wed, Jul 20, 2022 at 4:57 PM Dilip Kumar <dilipbalaut(at)gmail(dot)com> wrote:
>
> On Mon, Jul 18, 2022 at 4:51 PM Dilip Kumar <dilipbalaut(at)gmail(dot)com> wrote:
> >
> > I was doing some more testing by setting the FirstNormalRelFileNumber
> > to a high value(more than 32 bits) I have noticed a couple of problems
> > there e.g. relpath is still using OIDCHARS macro which says max
> > relfilenumber file name can be only 10 character long which is no
> > longer true. So there we need to change this value to 20 and also
> > need to carefully rename the macros and other variable names used for
> > this purpose.
> >
> > Similarly there was some issue in macro in buf_internal.h while
> > fetching the relfilenumber. So I will relook into all those issues
> > and repost the patch soon.
>
> I have fixed these existing issues and there was also some issue in
> pg_dump.c which was creating problems in upgrading to the same version
> while using a higher range of the relfilenumber.
>
> There was also an issue where the user table from the old cluster's
> relfilenode could conflict with the system table of the new cluster.
> As a solution currently for system table object (while creating
> storage first time) we are keeping the low range of relfilenumber,
> basically we are using the same relfilenumber as OID so that during
> upgrade the normal user table from the old cluster will not conflict
> with the system tables in the new cluster. But with this solution
> Robert told me (in off list chat) a problem that in future if we want
> to make relfilenumber completely unique within a cluster by
> implementing the CREATEDB differently then we can not do that as we
> have created fixed relfilenodes for the system tables.
>
> I am not sure what exactly we can do to avoid that because even if we
> do something to avoid that in the new cluster the old cluster might
> be already using the non-unique relfilenode so after upgrading the new
> cluster will also get those non-unique relfilenode.

Thanks for the patch, my comments from the initial review:
1) Since we have changed the macros to inline functions, should we
change the function names similar to the other inline functions in the
same file like: ClearBufferTag, InitBufferTag & BufferTagsEqual:
-#define BUFFERTAGS_EQUAL(a,b) \
-( \
- RelFileLocatorEquals((a).rlocator, (b).rlocator) && \
- (a).blockNum == (b).blockNum && \
- (a).forkNum == (b).forkNum \
-)
+static inline void
+CLEAR_BUFFERTAG(BufferTag *tag)
+{
+ tag->rlocator.spcOid = InvalidOid;
+ tag->rlocator.dbOid = InvalidOid;
+ tag->rlocator.relNumber = InvalidRelFileNumber;
+ tag->forkNum = InvalidForkNumber;
+ tag->blockNum = InvalidBlockNumber;
+}

2) We could move this macros along with the other macros at the top of the file:
+/*
+ * The freeNext field is either the index of the next freelist entry,
+ * or one of these special values:
+ */
+#define FREENEXT_END_OF_LIST (-1)
+#define FREENEXT_NOT_IN_LIST (-2)

3) typo thn should be then:
+ * can raise it as necessary if we end up with more mapped relations. For
+ * now, we just pick a round number that is modestly larger thn the expected
+ * number of mappings.
+ */

4) There is one whitespace issue:
git am v10-0004-Widen-relfilenumber-from-32-bits-to-56-bits.patch
Applying: Widen relfilenumber from 32 bits to 56 bits
.git/rebase-apply/patch:1500: space before tab in indent.
(relfilenumber)))); \
warning: 1 line adds whitespace errors.

Regards,
Vignesh