Re: making relfilenodes 56 bits

On Fri, Jul 29, 2022 at 6:26 PM Ashutosh Sharma <ashu.coek88@gmail.com>
wrote:

> On Thu, Jul 28, 2022 at 5:02 PM Dilip Kumar <dilipbalaut@gmail.com> wrote:
>
> +/* ----------
> + * RelFileNumber zero is InvalidRelFileNumber.
> + *
> + * For the system tables (OID < FirstNormalObjectId) the initial storage
>
> Above comment says that RelFileNumber zero is invalid which is technically
> correct because we don't have any relation file in disk with zero number.
> But the point is that if someone reads below definition of
> CHECK_RELFILENUMBER_RANGE he/she might get confused because as per this
> definition relfilenumber zero is valid.
>

Please ignore the above comment shared in my previous email.  It is a
little over-thinking on my part that generated this comment in my mind.
Sorry for that. Here are the other comments I have:

+/* First we have to remove them from the extension */
+ALTER EXTENSION pg_buffercache DROP VIEW pg_buffercache;
+ALTER EXTENSION pg_buffercache DROP FUNCTION pg_buffercache_pages();
+
+/* Then we can drop them */
+DROP VIEW pg_buffercache;
+DROP FUNCTION pg_buffercache_pages();
+
+/* Now redefine */
+CREATE OR REPLACE FUNCTION pg_buffercache_pages()
+RETURNS SETOF RECORD
+AS 'MODULE_PATHNAME', 'pg_buffercache_pages_v1_4'
+LANGUAGE C PARALLEL SAFE;
+
+CREATE OR REPLACE VIEW pg_buffercache AS
+   SELECT P.* FROM pg_buffercache_pages() AS P
+   (bufferid integer, relfilenode int8, reltablespace oid, reldatabase oid,
+    relforknumber int2, relblocknumber int8, isdirty bool, usagecount int2,
+    pinning_backends int4);

As we are dropping the function and view I think it would be good if we
*don't* use the "OR REPLACE" keyword when re-defining them.

==

+                   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")));

I think it would be good to recommend users to upgrade to the latest
version instead of just saying upgrade the pg_buffercache using ALTER
EXTENSION ....

==

--- a/contrib/pg_walinspect/sql/pg_walinspect.sql
+++ b/contrib/pg_walinspect/sql/pg_walinspect.sql
@@ -39,10 +39,10 @@ SELECT COUNT(*) >= 0 AS ok FROM
pg_get_wal_stats_till_end_of_wal(:'wal_lsn1');
 -- Test for filtering out WAL records of a particular table
 -- ===================================================================

-SELECT oid AS sample_tbl_oid FROM pg_class WHERE relname = 'sample_tbl'
\gset
+SELECT relfilenode AS sample_tbl_relfilenode FROM pg_class WHERE relname =
'sample_tbl' \gset

Is this change required? The original query is just trying to fetch table
oid not relfilenode and AFAIK we haven't changed anything in table oid.

==

+#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)
+

I think we can shift this macro to some header file and reuse it at several
places.

==


+    * Generate a new relfilenumber.  We cannot reuse the old relfilenumber
+    * because of the possibility that that relation will be moved back to
the

that that relation -> that relation

--
With Regards,
Ashutosh Sharma.