Обсуждение: Dropped and generated columns might cause wrong data on subs when REPLICA IDENTITY FULL

On Sun, Mar 12, 2023 4:00 AM Önder Kalacı <onderkalaci@gmail.com> 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

On Mon, Mar 13, 2023 at 6:26 PM Önder Kalacı <onderkalaci@gmail.com> wrote:
>
> 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.

On Thu, Mar 16, 2023 5:23 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
> 
> On Mon, Mar 13, 2023 at 6:26 PM Önder Kalacı <onderkalaci@gmail.com>
> wrote:
> >
> > 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?
> 

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

On Thu, Mar 16, 2023 at 9:33 PM Önder Kalacı <onderkalaci@gmail.com> wrote:
>
> Hi Amit, Shi Yu,
>
> > 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'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?
>

Works for me.

> >  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?
>

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,
> I can work on the followup.
>

Thanks for working on it.

--
With Regards,
Amit Kapila.

Amit Kapila <amit.kapila16@gmail.com> writes:
> On Thu, Mar 16, 2023 at 9:33 PM Önder Kalacı <onderkalaci@gmail.com> wrote:
>>> and backpatch the fix for dropped column to PG10.

> 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.

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

            regards, tom lane

On Fri, Mar 17, 2023 at 5:41 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>
> Amit Kapila <amit.kapila16@gmail.com> writes:
> > On Thu, Mar 16, 2023 at 9:33 PM Önder Kalacı <onderkalaci@gmail.com> wrote:
> >>> and backpatch the fix for dropped column to PG10.
>
> > 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.
>
> Don't worry about v10 --- that's out of support and shouldn't
> get patched for this.
>

Okay, thanks for reminding me.

--
With Regards,
Amit Kapila.

On Friday, March 17, 2023 3:38 PM Önder Kalacı <onderkalaci@gmail.com> wrote:
> 
> Hi Amit, all
>  
> 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.
> 
> 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. 
> 

Thanks for updating the patch.

I couldn't apply v2-0001-Ignore-dropped-columns-HEAD-REL_15-REL_14-REL_13.patch
cleanly in v13 and v14. It looks the patch needs some changes in these versions.

```
Checking patch src/backend/executor/execReplication.c...
Hunk #1 succeeded at 243 (offset -46 lines).
Hunk #2 succeeded at 263 (offset -46 lines).
Checking patch src/test/subscription/t/100_bugs.pl...
error: while searching for:
$node_publisher->stop('fast');
$node_subscriber->stop('fast');

done_testing();

error: patch failed: src/test/subscription/t/100_bugs.pl:373
Applied patch src/backend/executor/execReplication.c cleanly.
Applying patch src/test/subscription/t/100_bugs.pl with 1 reject...
Rejected hunk #1.
```

Besides, I tried v2-0001-Ignore-dropped-columns-REL_12-REL_11.patch in v12. The
test failed and here's some information.

```
Can't locate object method "new" via package "PostgreSQL::Test::Cluster" (perhaps you forgot to load
"PostgreSQL::Test::Cluster"?)at t/100_bugs.pl line 74.
 
# Looks like your test exited with 2 just after 1.
```

+my $node_publisher_d_cols = PostgreSQL::Test::Cluster->new('node_publisher_d_cols');

It seems this usage is not supported in v12 and we should use get_new_node()
like other test cases.

Regards,
Shi Yu

On Fri, Mar 17, 2023 11:29 PM Önder Kalacı <onderkalaci@gmail.com> wrote:
> 
> Thanks for sharing. Fixed
> 
> 
> This time I was able to run all the tests with all the patches applied.
> 
> Again, the generated column fix also has some minor differences
> per version. So, overall we have 6 patches with very minor 
> differences :) 

Thanks for updating the patches. It seems you forgot to attach the patches of
dropped columns for HEAD and pg15, I think they are the same as v2.

On HEAD, we can re-use clusters in other test cases, which can save some time.
(see fccaf259f22f4a)

In the patches for pg12 and pg11, I am not sure why not add the test at end of
the file 100_bugs.pl. I think it would be better to be consistent with other
versions.

The attached patches modify these two points. Besides, I made some minor
changes, ran pgindent and pgperltidy. These are patches for dropped columns,
because I think this would be submitted first, and we can discuss the fix for
generated columns later.

Regards,
Shi Yu

On Mon, Mar 20, 2023 at 2:58 PM Önder Kalacı <onderkalaci@gmail.com> wrote:
>
>
> I applied all patches for all the versions, and re-run the subscription tests,
> all looks good to me.
>

LGTM. I'll push this tomorrow unless there are more comments.

--
With Regards,
Amit Kapila.

On Mon, Mar 20, 2023 at 6:28 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
>
> On Mon, Mar 20, 2023 at 2:58 PM Önder Kalacı <onderkalaci@gmail.com> wrote:
> >
> >
> > I applied all patches for all the versions, and re-run the subscription tests,
> > all looks good to me.
> >
>
> LGTM. I'll push this tomorrow unless there are more comments.
>

Pushed.

--
With Regards,
Amit Kapila.

On Tue, Mar 21, 2023 4:51 PM Önder Kalacı <onderkalaci@gmail.com> wrote:
> 
> Hi Amit, Shi Yu
> 
> Now attaching the similar patches for generated columns.
> 

Thanks for your patches. Here are some comments.

1.
 $node_publisher->safe_psql(
     'postgres', qq(
         ALTER TABLE dropped_cols DROP COLUMN b_drop;
+        ALTER TABLE generated_cols DROP COLUMN b_gen;
 ));
 $node_subscriber->safe_psql(
     'postgres', qq(
         ALTER TABLE dropped_cols DROP COLUMN b_drop;
+        ALTER TABLE generated_cols DROP COLUMN b_gen;
 ));

I think we want to test generated columns, so we don't need to drop columns.
Otherwise the generated column problem can't be detected.

2. 
# The bug was that when the REPLICA IDENTITY FULL is used with dropped columns,
# we fail to apply updates and deletes

Maybe we should mention generated columns in comment of the test.

3.
I ran pgindent and it modified some lines. Maybe we can improve the patch
as the following.

@@ -292,8 +292,8 @@ tuples_equal(TupleTableSlot *slot1, TupleTableSlot *slot2,
         att = TupleDescAttr(slot1->tts_tupleDescriptor, attrnum);
 
         /*
-         * Ignore dropped and generated columns as the publisher
-         * doesn't send those
+         * Ignore dropped and generated columns as the publisher doesn't send
+         * those
          */
         if (att->attisdropped || att->attgenerated)
             continue;

Regards,
Shi Yu

On Tuesday, March 21, 2023 8:03 PM Önder Kalacı <onderkalaci@gmail.com> wrote:
> 
> Attached patches again.
> 

Thanks for updating the patch.

@@ -408,15 +412,18 @@ $node_subscriber->wait_for_subscription_sync;
 $node_publisher->safe_psql(
     'postgres', qq(
         ALTER TABLE dropped_cols DROP COLUMN b_drop;
+        ALTER TABLE generated_cols DROP COLUMN c_drop;
 ));
 $node_subscriber->safe_psql(
     'postgres', qq(
         ALTER TABLE dropped_cols DROP COLUMN b_drop;
+        ALTER TABLE generated_cols DROP COLUMN c_drop;
 ));

Is there any reasons why we drop column here? Dropped column case has been
tested on table dropped_cols. The generated column problem can be detected
without dropping columns on my machine.

Regards,
Shi Yu

On Wed, Mar 22, 2023 2:53 PM Önder Kalacı <onderkalaci@gmail.com> wrote:
> 
> We don't really need to, if you check the first patch, we don't have DROP for generated case. I mostly
> wanted to make the test a little more interesting, but it also seems to be a little confusing.
> 
> Now attaching v2 where we do not drop the columns. I don't have strong preference on
> which patch to proceed with, mostly wanted to attach this version to progress faster (in case
> you/Amit considers this one better).
> 

Thanks for updating the patches.
The v2 patch LGTM.

Regards,
Shi Yu

On Wed, Mar 22, 2023 at 1:39 PM shiy.fnst@fujitsu.com
<shiy.fnst@fujitsu.com> wrote:
>
> On Wed, Mar 22, 2023 2:53 PM Önder Kalacı <onderkalaci@gmail.com> wrote:
> >
> > We don't really need to, if you check the first patch, we don't have DROP for generated case. I mostly
> > wanted to make the test a little more interesting, but it also seems to be a little confusing.
> >
> > Now attaching v2 where we do not drop the columns. I don't have strong preference on
> > which patch to proceed with, mostly wanted to attach this version to progress faster (in case
> > you/Amit considers this one better).
> >
>
> Thanks for updating the patches.
> The v2 patch LGTM.
>

Pushed.

--
With Regards,
Amit Kapila.