Subject: RE: Dropped and generated columns might cause wrong data on subs when REPLICA IDENTITY FULL

From: "[email-masked]" <[email-masked]>
Date: Mon, 13 Mar 2023 10:53:03 +0000

On Sun, Mar 12, 2023 4:00 AM Önder Kalacı <[email-masked]> wrote:
> 
> Attaching a patch that could possibly solve the problem. 
> 

Thanks for your patch. I tried it and it worked well.
Here are some minor comments.

1.
@@ -243,6 +243,17 @@ tuples_equal(TupleTableSlot *slot1, TupleTableSlot *slot2,
 		Form_pg_attribute att;
 		TypeCacheEntry *typentry;
 
+
+		Form_pg_attribute attr = TupleDescAttr(slot1->tts_tupleDescriptor, attrnum);
+

I think we can use "att" instead of a new variable. They have the same value.

2. 
+# The bug was that when when the REPLICA IDENTITY FULL is used with dropped

There is an extra "when".

Regards,
Shi Yu

---

From: Önder Kalacı <[email-masked]>
Date: Mon, 13 Mar 2023 15:56:28 +0300

Hi Shi Yu,

> +               Form_pg_attribute attr =
> TupleDescAttr(slot1->tts_tupleDescriptor, attrnum);
> +
>

ah, of course :)

> There is an extra "when".
Fixed, thanks

Attaching v2

---

From: Amit Kapila <[email-masked]>
Date: Thu, 16 Mar 2023 14:53:19 +0530

> Attaching v2

Can we change the comment to: "Ignore dropped and generated columns as
the publisher doesn't send those."? After your change, att =
TupleDescAttr(slot1->tts_tupleDescriptor, attrnum); is done twice in
the same function.

In test cases, let's change the comment to: "The bug was that when the
REPLICA IDENTITY FULL is used with dropped or generated columns, we
fail to apply updates and deletes.". Also, I think we don't need to
provide the email link as anyway commit message will have a link to
the discussion.

Did you check this in the back branches?

-- 
With Regards,
Amit Kapila.

---

From: "[email-masked]" <[email-masked]>
Date: Thu, 16 Mar 2023 10:38:25 +0000

> Did you check this in the back branches?

I tried to reproduce this bug in backbranch.

Generated column is introduced in PG12, and I reproduced generated column problem
in PG12~PG15.

For dropped column problem, I reproduced it in PG10~PG15. (Logical replication
was introduced in PG10)

So I think we should backpatch the fix for generated column to PG12, and
backpatch the fix for dropped column to PG10.

Regards,
Shi Yu

---

From: Önder Kalacı <[email-masked]>
Date: Thu, 16 Mar 2023 19:03:25 +0300

Hi Amit, Shi Yu,

in PG12~PG15.
was introduced in PG10)

So, I'm planning to split the changes into two commits. The first one fixes
for dropped columns, and the second one adds generated columns check/test.

Is that the right approach for such a case?

>  and backpatch the fix for dropped column to PG10.

Still, even the first commit fails to apply cleanly to PG12 (and below).

What is the procedure here? Should I be creating multiple patches per
version?
Or does the committer prefer to handle the conflicts? Depending on your
reply,
I can work on the followup.

I'm still attaching the dropped column patch for reference.

Thanks,
Onder

[email-masked] <[email-masked]>, 16 Mar 2023 Per, 13:38

---

From: Amit Kapila <[email-masked]>
Date: Fri, 17 Mar 2023 05:38:50 +0530

> Is that the right approach for such a case?

Works for me.

> What is the procedure here? Should I be creating multiple patches per version?
>

You can first submit the fix for dropped columns with patches till
v10. Once that is committed, then you can send the patches for
generated columns.

> Or does the committer prefer to handle the conflicts? Depending on your reply,

Thanks for working on it.

-- 
With Regards,
Amit Kapila.

---

From: Tom Lane <[email-masked]>
Date: Thu, 16 Mar 2023 20:11:37 -0400

> v10. Once that is committed, then you can send the patches for

Don't worry about v10 --- that's out of support and shouldn't
get patched for this.

			regards, tom lane

---

From: Önder Kalacı <[email-masked]>
Date: Fri, 17 Mar 2023 10:38:02 +0300

Hi Amit, all

> v10. Once that is committed, then you can send the patches for
Alright, attaching 2 patches for dropped columns, the names of the files
shows which
versions the patch can be applied to:
v2-0001-Ignore-dropped-columns-HEAD-REL_15-REL_14-REL_13.patch
v2-0001-Ignore-dropped-columns-REL_12-REL_11.patch

And, then on top of that, you can apply the patch for generated columns on
all applicable
versions (HEAD, 15, 14, 13 and 12). It applies cleanly.  The name of the
file
is: v2-0001-Ignore-generated-columns.patch

But unfortunately I couldn't test the patch with PG 12 and below. I'm
getting some
unrelated compile errors and Postgrees CI is not available on
these versions . I'll try
to fix that, but I thought it would still be good to share the patches as
you might
already have the environment to run the tests.

> get patched for this.

Given this information, I skipped the v10 patch.

Thanks,
Onder KALACI