Re: making relfilenodes 56 bits

On Wed, Jul 27, 2022 at 6:02 PM Dilip Kumar <dilipbalaut(at)gmail(dot)com> wrote:
>
> On Wed, Jul 27, 2022 at 3:27 PM vignesh C <vignesh21(at)gmail(dot)com> wrote:
> >
>
> > 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 */
>
> Thanks for the review I have fixed these except,
> > 9) should we change elog to ereport to use the New-style error reporting API
> I think this is internal error so if we use ereport we need to give
> error code and all and I think for internal that is not necessary?

Ok, Sounds reasonable.

> > 8) I felt this include is not required:
> it is using access API so we do need <unistd.h>

Ok, It worked for me because I had not used the ASSERT enabled flag
while compilation.

Regards,
Vignesh