Re: making relfilenodes 56 bits

On Wed, Jul 27, 2022 at 3:27 PM vignesh C <vignesh21@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?

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

-- 
Regards,
Dilip Kumar
EnterpriseDB: http://www.enterprisedb.com