Date: Wed, 12 Feb 2020 12:23:37 -0600
From: Justin Pryzby <pryzby@telsasoft.com>
To: Alvaro Herrera <alvherre@2ndquadrant.com>
Cc: Amit Langote <amitlangote09@gmail.com>, pgsql-hackers@postgresql.org
Subject: assert pg_class.relnatts is consistent
Message-ID: <20200212182337.GZ1412@telsasoft.com>
MIME-Version: 1.0
Content-Type: multipart/mixed; boundary="TB36FDmn/VVEgNH/"
Content-Disposition: inline
User-Agent: Mutt/1.5.24 (2015-08-30)
Precedence: bulk


--TB36FDmn/VVEgNH/
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline

Forking this thread for two tangential patches which I think are more
worthwhile than the original topic's patch.
https://www.postgresql.org/message-id/20200207143935.GP403%40telsasoft.com

Is there a better place to implement assertion from 0002 ?

On Fri, Feb 07, 2020 at 08:39:35AM -0600, Justin Pryzby wrote:
> From 7eea0a17e495fe13379ffd589b551f2f145f5672 Mon Sep 17 00:00:00 2001
> From: Justin Pryzby <pryzbyj@telsasoft.com>
> Date: Thu, 6 Feb 2020 21:48:13 -0600
> Subject: [PATCH v1 1/3] Update comment obsolete since b9b8831a
> 
> ---
>  src/backend/commands/cluster.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/src/backend/commands/cluster.c b/src/backend/commands/cluster.c
> index e9d7a7f..3adcbeb 100644
> --- a/src/backend/commands/cluster.c
> +++ b/src/backend/commands/cluster.c
> @@ -1539,9 +1539,9 @@ get_tables_to_cluster(MemoryContext cluster_context)
>  
>  	/*
>  	 * Get all indexes that have indisclustered set and are owned by
> -	 * appropriate user. System relations or nailed-in relations cannot ever
> -	 * have indisclustered set, because CLUSTER will refuse to set it when
> -	 * called with one of them as argument.
> +	 * appropriate user. Shared relations cannot ever have indisclustered
> +	 * set, because CLUSTER will refuse to set it when called with one as
> +	 * an argument.
>  	 */
>  	indRelation = table_open(IndexRelationId, AccessShareLock);
>  	ScanKeyInit(&entry,
> -- 
> 2.7.4
> 

> From 4777be522a7aa8b8c77b13f765cbd02043438f2a Mon Sep 17 00:00:00 2001
> From: Justin Pryzby <pryzbyj@telsasoft.com>
> Date: Fri, 7 Feb 2020 08:12:50 -0600
> Subject: [PATCH v1 2/3] Give developer a helpful kick in the pants if they
>  change natts in one place but not another
> 
> ---
>  src/backend/bootstrap/bootstrap.c | 23 +++++++++++++++++++++++
>  1 file changed, 23 insertions(+)
> 
> diff --git a/src/backend/bootstrap/bootstrap.c b/src/backend/bootstrap/bootstrap.c
> index bfc629c..d5e1888 100644
> --- a/src/backend/bootstrap/bootstrap.c
> +++ b/src/backend/bootstrap/bootstrap.c
> @@ -25,7 +25,9 @@
>  #include "access/xlog_internal.h"
>  #include "bootstrap/bootstrap.h"
>  #include "catalog/index.h"
> +#include "catalog/pg_class.h"
>  #include "catalog/pg_collation.h"
> +#include "catalog/pg_proc.h"
>  #include "catalog/pg_type.h"
>  #include "common/link-canary.h"
>  #include "libpq/pqsignal.h"
> @@ -49,6 +51,7 @@
>  #include "utils/ps_status.h"
>  #include "utils/rel.h"
>  #include "utils/relmapper.h"
> +#include "utils/syscache.h"
>  
>  uint32		bootstrap_data_checksum_version = 0;	/* No checksum */
>  
> @@ -602,6 +605,26 @@ boot_openrel(char *relname)
>  	TableScanDesc scan;
>  	HeapTuple	tup;
>  
> +	/* Check that pg_class data is consistent now, rather than failing obscurely later */
> +	struct { Oid oid; int natts; }
> +		checknatts[] = {
> +		{RelationRelationId, Natts_pg_class,},
> +		{TypeRelationId, Natts_pg_type,},
> +		{AttributeRelationId, Natts_pg_attribute,},
> +		{ProcedureRelationId, Natts_pg_proc,},
> +	};
> +
> +	for (int i=0; i<lengthof(checknatts); ++i) {
> +		Form_pg_class	classForm;
> +		HeapTuple	tuple;
> +		tuple = SearchSysCache1(RELOID, ObjectIdGetDatum(checknatts[i].oid));
> +		if (!HeapTupleIsValid(tuple))
> +			elog(ERROR, "cache lookup failed for relation %u", checknatts[i].oid);
> +		classForm = (Form_pg_class) GETSTRUCT(tuple);
> +		Assert(checknatts[i].natts == classForm->relnatts);
> +		ReleaseSysCache(tuple);
> +	}
> +
>  	if (strlen(relname) >= NAMEDATALEN)
>  		relname[NAMEDATALEN - 1] = '\0';
>  
> -- 
> 2.7.4
> 

--TB36FDmn/VVEgNH/
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment;
 filename="v1-0002-Give-developer-a-helpful-kick-in-the-pants-if-the.patch"

From 4777be522a7aa8b8c77b13f765cbd02043438f2a Mon Sep 17 00:00:00 2001
From: Justin Pryzby <pryzbyj@telsasoft.com>
Date: Fri, 7 Feb 2020 08:12:50 -0600
Subject: [PATCH v1 2/3] Give developer a helpful kick in the pants if they
 change natts in one place but not another

---
 src/backend/bootstrap/bootstrap.c | 23 +++++++++++++++++++++++
 1 file changed, 23 insertions(+)

diff --git a/src/backend/bootstrap/bootstrap.c b/src/backend/bootstrap/bootstrap.c
index bfc629c..d5e1888 100644
--- a/src/backend/bootstrap/bootstrap.c
+++ b/src/backend/bootstrap/bootstrap.c
@@ -25,7 +25,9 @@
 #include "access/xlog_internal.h"
 #include "bootstrap/bootstrap.h"
 #include "catalog/index.h"
+#include "catalog/pg_class.h"
 #include "catalog/pg_collation.h"
+#include "catalog/pg_proc.h"
 #include "catalog/pg_type.h"
 #include "common/link-canary.h"
 #include "libpq/pqsignal.h"
@@ -49,6 +51,7 @@
 #include "utils/ps_status.h"
 #include "utils/rel.h"
 #include "utils/relmapper.h"
+#include "utils/syscache.h"
 
 uint32		bootstrap_data_checksum_version = 0;	/* No checksum */
 
@@ -602,6 +605,26 @@ boot_openrel(char *relname)
 	TableScanDesc scan;
 	HeapTuple	tup;
 
+	/* Check that pg_class data is consistent now, rather than failing obscurely later */
+	struct { Oid oid; int natts; }
+		checknatts[] = {
+		{RelationRelationId, Natts_pg_class,},
+		{TypeRelationId, Natts_pg_type,},
+		{AttributeRelationId, Natts_pg_attribute,},
+		{ProcedureRelationId, Natts_pg_proc,},
+	};
+
+	for (int i=0; i<lengthof(checknatts); ++i) {
+		Form_pg_class	classForm;
+		HeapTuple	tuple;
+		tuple = SearchSysCache1(RELOID, ObjectIdGetDatum(checknatts[i].oid));
+		if (!HeapTupleIsValid(tuple))
+			elog(ERROR, "cache lookup failed for relation %u", checknatts[i].oid);
+		classForm = (Form_pg_class) GETSTRUCT(tuple);
+		Assert(checknatts[i].natts == classForm->relnatts);
+		ReleaseSysCache(tuple);
+	}
+
 	if (strlen(relname) >= NAMEDATALEN)
 		relname[NAMEDATALEN - 1] = '\0';
 
-- 
2.7.4


--TB36FDmn/VVEgNH/--