From: Tom Lane <tgl@sss.pgh.pa.us>
To: hisahiro@freemind.co.jp
cc: pgsql-bugs@lists.postgresql.org
Subject: Re: BUG #17823: Generated columns not always updated correctly
In-reply-to: <17823-b64909cf7d63de84@postgresql.org>
References: <17823-b64909cf7d63de84@postgresql.org>
Comments: In-reply-to PG Bug reporting form <noreply@postgresql.org>
	message dated "Mon, 06 Mar 2023 07:26:42 +0000"
MIME-Version: 1.0
Content-Type: multipart/mixed; boundary="----- =_aaaaaaaaaa0"
Content-ID: <3402867.1678120442.0@sss.pgh.pa.us>
Date: Mon, 06 Mar 2023 11:35:29 -0500
Message-ID: <3402993.1678120529@sss.pgh.pa.us>
Archived-At: 
 <https://www.postgresql.org/message-id/3402993.1678120529%40sss.pgh.pa.us>
Precedence: bulk

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

PG Bug reporting form <noreply@postgresql.org> writes:
> I found that the generated columns are sometimes not updated.

Yeah.  Looking into nodeModifyTable.c, we miss re-doing
ExecComputeStoredGenerated when looping back after an EPQ update
(which is what this case is).  I see that we also fail to redo that
after a cross-partition move, which is a bug since 8bf6ec3ba.

The attached seems to be enough to fix it, but I want to also devise
an isolation test for these cases ...

			regards, tom lane


------- =_aaaaaaaaaa0
Content-Type: text/x-diff; name="fix-bug-17823-wip.patch";
	charset="us-ascii"
Content-ID: <3402867.1678120442.2@sss.pgh.pa.us>
Content-Description: fix-bug-17823-wip.patch
Content-Transfer-Encoding: quoted-printable

diff --git a/src/backend/executor/nodeModifyTable.c b/src/backend/executor=
/nodeModifyTable.c
index 6f0543af83..6d9c0b0806 100644
--- a/src/backend/executor/nodeModifyTable.c
+++ b/src/backend/executor/nodeModifyTable.c
@@ -1910,9 +1910,10 @@ ExecUpdatePrologue(ModifyTableContext *context, Res=
ultRelInfo *resultRelInfo,
 }
 =

 /*
- * ExecUpdatePrepareSlot -- subroutine for ExecUpdate
+ * ExecUpdatePrepareSlot -- subroutine for ExecUpdateAct
  *
  * Apply the final modifications to the tuple slot before the update.
+ * (This is split out because we also need it in the foreign-table code p=
ath.)
  */
 static void
 ExecUpdatePrepareSlot(ResultRelInfo *resultRelInfo,
@@ -1962,13 +1963,14 @@ ExecUpdateAct(ModifyTableContext *context, ResultR=
elInfo *resultRelInfo,
 	updateCxt->crossPartUpdate =3D false;
 =

 	/*
-	 * If we generate a new candidate tuple after EvalPlanQual testing, we
-	 * must loop back here and recheck any RLS policies and constraints. (We
-	 * don't need to redo triggers, however.  If there are any BEFORE trigge=
rs
-	 * then trigger.c will have done table_tuple_lock to lock the correct
-	 * tuple, so there's no need to do them again.)
+	 * If we move the tuple to a new partition, we loop back here to recompu=
te
+	 * GENERATED values (which are allowed to be different across partitions=
)
+	 * and recheck any RLS policies and constraints.  We do not fire any
+	 * BEFORE triggers of the new partition, however.
 	 */
 lreplace:
+	/* Fill in GENERATEd columns */
+	ExecUpdatePrepareSlot(resultRelInfo, slot, estate);
 =

 	/* ensure slot is independent, consider e.g. EPQ */
 	ExecMaterializeSlot(slot);
@@ -2268,6 +2270,7 @@ ExecUpdate(ModifyTableContext *context, ResultRelInf=
o *resultRelInfo,
 	}
 	else if (resultRelInfo->ri_FdwRoutine)
 	{
+		/* Fill in GENERATEd columns */
 		ExecUpdatePrepareSlot(resultRelInfo, slot, estate);
 =

 		/*
@@ -2290,9 +2293,13 @@ ExecUpdate(ModifyTableContext *context, ResultRelIn=
fo *resultRelInfo,
 	}
 	else
 	{
-		/* Fill in the slot appropriately */
-		ExecUpdatePrepareSlot(resultRelInfo, slot, estate);
-
+		/*
+		 * If we generate a new candidate tuple after EvalPlanQual testing, we
+		 * must loop back here to try again.  (We don't need to redo triggers,
+		 * however.  If there are any BEFORE triggers then trigger.c will have
+		 * done table_tuple_lock to lock the correct tuple, so there's no need
+		 * to do them again.)
+		 */
 redo_act:
 		result =3D ExecUpdateAct(context, resultRelInfo, tupleid, oldtuple, slo=
t,
 							   canSetTag, &updateCxt);
@@ -2876,7 +2883,6 @@ lmerge_matched:
 					result =3D TM_Ok;
 					break;
 				}
-				ExecUpdatePrepareSlot(resultRelInfo, newslot, context->estate);
 				result =3D ExecUpdateAct(context, resultRelInfo, tupleid, NULL,
 									   newslot, false, &updateCxt);
 				if (result =3D=3D TM_Ok && updateCxt.updated)

------- =_aaaaaaaaaa0--