On Wed, Jul 27, 2022 at 6:02 PM Dilip Kumar <dilipbal...@gmail.com> wrote: > > On Wed, Jul 27, 2022 at 3:27 PM vignesh C <vignes...@gmail.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