Re: making relfilenodes 56 bits

On Tue, Jul 26, 2022 at 1:32 PM Dilip Kumar <dilipbalaut(at)gmail(dot)com> wrote:
>
> On Thu, Jul 21, 2022 at 9:53 AM Thomas Munro <thomas(dot)munro(at)gmail(dot)com> wrote:
> >
> > On Wed, Jul 20, 2022 at 11:27 PM Dilip Kumar <dilipbalaut(at)gmail(dot)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