Re: making relfilenodes 56 bits

On Tue, Jul 26, 2022 at 1:32 PM Dilip Kumar <dilipbalaut@gmail.com> wrote:
>
> On Thu, Jul 21, 2022 at 9:53 AM Thomas Munro <thomas.munro@gmail.com> wrote:
> >
> > On Wed, Jul 20, 2022 at 11:27 PM Dilip Kumar <dilipbalaut@gmail.com> wrote:
> > > [v10 patch set]
> >
> > Hi Dilip, I'm experimenting with these patches and will hopefully have
> > more to say soon, but I just wanted to point out that this builds with
> > warnings and failed on 3/4 of the CI OSes on cfbot's last run.  Maybe
> > there is the good kind of uninitialised data on Linux, and the bad
> > kind of uninitialised data on those other pesky systems?
>
> Here is the patch to fix the issue, basically, while asserting for the
> file existence it was not setting the relfilenumber in the
> relfilelocator before generating the path so it was just checking for
> the existence of the random path so it was asserting randomly.

Thanks for the updated patch, Few comments:
1) The format specifier should be changed from %u to INT64_FORMAT
autoprewarm.c -> apw_load_buffers
...............
if (fscanf(file, "%u,%u,%u,%u,%u\n", &blkinfo[i].database,
   &blkinfo[i].tablespace, &blkinfo[i].filenumber,
   &forknum, &blkinfo[i].blocknum) != 5)
...............

2) The format specifier should be changed from %u to INT64_FORMAT
autoprewarm.c -> apw_dump_now
...............
ret = fprintf(file, "%u,%u,%u,%u,%u\n",
  block_info_array[i].database,
  block_info_array[i].tablespace,
  block_info_array[i].filenumber,
  (uint32) block_info_array[i].forknum,
  block_info_array[i].blocknum);
...............

3) should the comment "entry point for old extension version" be on
top of pg_buffercache_pages, as the current version will use
pg_buffercache_pages_v1_4
+
+Datum
+pg_buffercache_pages(PG_FUNCTION_ARGS)
+{
+       return pg_buffercache_pages_internal(fcinfo, OIDOID);
+}
+
+/* entry point for old extension version */
+Datum
+pg_buffercache_pages_v1_4(PG_FUNCTION_ARGS)
+{
+       return pg_buffercache_pages_internal(fcinfo, INT8OID);
+}

4) we could use the new style or ereport by removing the brackets
around errcode:
+                               if (fctx->record[i].relfilenumber > OID_MAX)
+                                       ereport(ERROR,
+
(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+
errmsg("relfilenode" INT64_FORMAT " is too large to be represented as
an OID",
+
 fctx->record[i].relfilenumber),
+
errhint("Upgrade the extension using ALTER EXTENSION pg_buffercache
UPDATE")));

like:
ereport(ERROR,

errcode(ERRCODE_INVALID_PARAMETER_VALUE),

errmsg("relfilenode" INT64_FORMAT " is too large to be represented as
an OID",

fctx->record[i].relfilenumber),

errhint("Upgrade the extension using ALTER EXTENSION pg_buffercache
UPDATE"));

5) Similarly in the below code too:
+       /* check whether the relfilenumber is within a valid range */
+       if ((relfilenumber) < 0 || (relfilenumber) > MAX_RELFILENUMBER)
+               ereport(ERROR,
+                               (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+                                errmsg("relfilenumber " INT64_FORMAT
" is out of range",
+                                               (relfilenumber))));


6) Similarly in the below code too:
+#define CHECK_RELFILENUMBER_RANGE(relfilenumber)
         \
+do {
                                                         \
+       if ((relfilenumber) < 0 || (relfilenumber) > MAX_RELFILENUMBER) \
+               ereport(ERROR,
                                                 \
+
(errcode(ERRCODE_INVALID_PARAMETER_VALUE),                      \
+                                errmsg("relfilenumber " INT64_FORMAT
" is out of range",       \
+                                               (relfilenumber)))); \
+} while (0)
+


7) This error code looks similar to CHECK_RELFILENUMBER_RANGE, can
this macro be used here too:
pg_filenode_relation(PG_FUNCTION_ARGS)
 {
        Oid                     reltablespace = PG_GETARG_OID(0);
-       RelFileNumber relfilenumber = PG_GETARG_OID(1);
+       RelFileNumber relfilenumber = PG_GETARG_INT64(1);
        Oid                     heaprel;

+       /* check whether the relfilenumber is within a valid range */
+       if ((relfilenumber) < 0 || (relfilenumber) > MAX_RELFILENUMBER)
+               ereport(ERROR,
+                               (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+                                errmsg("relfilenumber " INT64_FORMAT
" is out of range",
+                                               (relfilenumber))));


8) I felt this include is not required:
diff --git a/src/backend/access/transam/varsup.c
b/src/backend/access/transam/varsup.c
index 849a7ce..a2f0d35 100644
--- a/src/backend/access/transam/varsup.c
+++ b/src/backend/access/transam/varsup.c
@@ -13,12 +13,16 @@

 #include "postgres.h"

+#include <unistd.h>
+
 #include "access/clog.h"
 #include "access/commit_ts.h"

9) should we change elog to ereport to use the New-style error reporting API
+       /* safety check, we should never get this far in a HS standby */
+       if (RecoveryInProgress())
+               elog(ERROR, "cannot assign RelFileNumber during recovery");
+
+       if (IsBinaryUpgrade)
+               elog(ERROR, "cannot assign RelFileNumber during binary
upgrade");

10) Here nextRelFileNumber is protected by RelFileNumberGenLock, the
comment stated OidGenLock. It should be slightly adjusted.
typedef struct VariableCacheData
{
/*
* These fields are protected by OidGenLock.
*/
Oid nextOid; /* next OID to assign */
uint32 oidCount; /* OIDs available before must do XLOG work */
RelFileNumber nextRelFileNumber; /* next relfilenumber to assign */
RelFileNumber loggedRelFileNumber; /* last logged relfilenumber */
XLogRecPtr loggedRelFileNumberRecPtr; /* xlog record pointer w.r.t.
* loggedRelFileNumber */

Regards,
Vignesh