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

Hi Shi Yu,

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.

ah, of course :)

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

There is an extra "when".

Fixed, thanks

Attaching v2

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

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?

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

shiy.fnst@fujitsu.com <shiy.fnst@fujitsu.com>, 16 Mar 2023 Per, 13:38
tarihinde şunu yazdı:

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.

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.

Don't worry about v10 --- that's out of support and shouldn't

get patched for this.

Given this information, I skipped the v10 patch.

Thanks,
Onder KALACI

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

Hi Shi Yu,

Thanks for the review, really appreciate it!

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

Hmm, interesting, it behaves differently on Macos and linux. Now attaching
new patches that should apply. Can you please try?

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.

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,
Onder

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

Hi Shi Yu, all

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.

Yes, it seems I forgot. And, yes they were the same as v2.

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

Thanks for noting.

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.

I applied the same patch that I created for HEAD, and then the "patch"
command
created this version. Given that we are creating new patches per version, I
think
you are right, we should put them at the end.

The attached patches modify these two points. Besides, I made some minor
changes, ran pgindent and pgperltidy.

oh, I didn't know about pgperltidy. And, thanks a lot for making changes and
making it ready for the committer.

These are patches for dropped columns,
because I think this would be submitted first, and we can discuss the fix
for
generated columns later.

Makes sense, even now we have 5 different patches, lets work on generated
columns
when this is fixed.

I applied all patches for all the versions, and re-run the subscription
tests,
all looks good to me.

Thanks,
Onder KALACI

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.

Hi Amit, Shi Yu

Now attaching the similar patches for generated columns.

Thanks,
Onder KALACI

Amit Kapila <amit.kapila16@gmail.com>, 21 Mar 2023 Sal, 09:07 tarihinde
şunu yazdı:

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

Hi Shi Yu,

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.

Ow, what a mistake. Now changed (and ensured that without the patch
the test fails).

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.

makes sense

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;

fixed

Attached patches again.

Thanks,
Onder KALACI

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