From: Tom Lane <tgl@sss.pgh.pa.us>
To: pgsql-hackers@lists.postgresql.org
cc: Andres Freund <andres@anarazel.de>
Subject: Re: pg_subscription.subslotname is wrongly marked NOT NULL
In-reply-to: <34984.1595124581@sss.pgh.pa.us>
References: <4118109.1595096139@sss.pgh.pa.us>
 <34984.1595124581@sss.pgh.pa.us>
Comments: In-reply-to Tom Lane <tgl@sss.pgh.pa.us>
	message dated "Sat, 18 Jul 2020 22:09:41 -0400"
MIME-Version: 1.0
Content-Type: multipart/mixed; boundary="----- =_aaaaaaaaaa0"
Content-ID: <229368.1595191298.0@sss.pgh.pa.us>
Date: Sun, 19 Jul 2020 16:42:25 -0400
Message-ID: <229396.1595191345@sss.pgh.pa.us>
Precedence: bulk

------- =_aaaaaaaaaa0
Content-Type: text/plain; charset="us-ascii"
Content-ID: <229368.1595191298.1@sss.pgh.pa.us>

I wrote:
> (2) In pre-v13 branches, hack the JIT tuple deconstruction code
> to be specifically aware that it can't trust attnotnull for
> pg_subscription.subslotname.  Yeah, it's ugly, but at least it's
> not ugly going forwards.

Concretely, as attached for v12.

			regards, tom lane


------- =_aaaaaaaaaa0
Content-Type: text/x-diff; name="work-around-bogus-attnotnull-v12.patch";
	charset="us-ascii"
Content-ID: <229368.1595191298.2@sss.pgh.pa.us>
Content-Description: work-around-bogus-attnotnull-v12.patch
Content-Transfer-Encoding: quoted-printable

diff --git a/src/backend/jit/llvm/llvmjit_deform.c b/src/backend/jit/llvm/=
llvmjit_deform.c
index 835aea83e9..4b2144e1a7 100644
--- a/src/backend/jit/llvm/llvmjit_deform.c
+++ b/src/backend/jit/llvm/llvmjit_deform.c
@@ -22,11 +22,24 @@
 =

 #include "access/htup_details.h"
 #include "access/tupdesc_details.h"
+#include "catalog/pg_subscription.h"
 #include "executor/tuptable.h"
 #include "jit/llvmjit.h"
 #include "jit/llvmjit_emit.h"
 =

 =

+/*
+ * Through an embarrassing oversight, pre-v13 installations may have
+ * pg_subscription.subslotname marked as attnotnull, which it should not =
be.
+ * To avoid possible crashes, use this macro instead of testing attnotnul=
l
+ * directly.
+ */
+#define ATTNOTNULL(att) \
+	((att)->attnotnull && \
+	 ((att)->attrelid !=3D SubscriptionRelationId || \
+	  (att)->attnum !=3D Anum_pg_subscription_subslotname))
+
+
 /*
  * Create a function that deforms a tuple of type desc up to natts column=
s.
  */
@@ -121,7 +134,7 @@ slot_compile_deform(LLVMJitContext *context, TupleDesc=
 desc,
 		 * combination of attisdropped && attnotnull combination shouldn't
 		 * exist.
 		 */
-		if (att->attnotnull &&
+		if (ATTNOTNULL(att) &&
 			!att->atthasmissing &&
 			!att->attisdropped)
 			guaranteed_column_number =3D attnum;
@@ -419,7 +432,7 @@ slot_compile_deform(LLVMJitContext *context, TupleDesc=
 desc,
 		 * into account, because if they're present the heaptuple's natts
 		 * would have indicated that a slot_getmissingattrs() is needed.
 		 */
-		if (!att->attnotnull)
+		if (!ATTNOTNULL(att))
 		{
 			LLVMBasicBlockRef b_ifnotnull;
 			LLVMBasicBlockRef b_ifnull;
@@ -586,6 +599,9 @@ slot_compile_deform(LLVMJitContext *context, TupleDesc=
 desc,
 		 * to generate better code. Without that LLVM can't figure out that
 		 * the offset might be constant due to the jumps for previously
 		 * decoded columns.
+		 *
+		 * Note: these two tests on attnotnull don't need the ATTNOTNULL hack,
+		 * because they are harmless on pg_subscription anyway.
 		 */
 		if (attguaranteedalign)
 		{
diff --git a/src/test/regress/expected/subscription.out b/src/test/regress=
/expected/subscription.out
index e7add9d2b8..3ba1e5dcdd 100644
--- a/src/test/regress/expected/subscription.out
+++ b/src/test/regress/expected/subscription.out
@@ -147,6 +147,13 @@ DROP SUBSCRIPTION regress_testsub;
 ERROR:  DROP SUBSCRIPTION cannot run inside a transaction block
 COMMIT;
 ALTER SUBSCRIPTION regress_testsub SET (slot_name =3D NONE);
+\dRs+
+                                                      List of subscriptio=
ns
+      Name       |           Owner            | Enabled |     Publication=
     | Synchronous commit |           Conninfo           =

+-----------------+----------------------------+---------+----------------=
-----+--------------------+------------------------------
+ regress_testsub | regress_subscription_user2 | f       | {testpub2,testp=
ub3} | local              | dbname=3Dregress_doesnotexist2
+(1 row)
+
 -- now it works
 BEGIN;
 DROP SUBSCRIPTION regress_testsub;
diff --git a/src/test/regress/sql/subscription.sql b/src/test/regress/sql/=
subscription.sql
index 9e234ab8b3..1bc58887f7 100644
--- a/src/test/regress/sql/subscription.sql
+++ b/src/test/regress/sql/subscription.sql
@@ -109,6 +109,8 @@ COMMIT;
 =

 ALTER SUBSCRIPTION regress_testsub SET (slot_name =3D NONE);
 =

+\dRs+
+
 -- now it works
 BEGIN;
 DROP SUBSCRIPTION regress_testsub;

------- =_aaaaaaaaaa0--