Horton

Thread

Commits

  1. Ignore generated columns during apply of update/delete.

  2. Ignore dropped columns during apply of update/delete.

  1. Dropped and generated columns might cause wrong data on subs when REPLICA IDENTITY FULL

    Önder Kalacı <onderkalaci@gmail.com> — 2023-03-11T19:59:37Z

    Hi all,

(cc'ed Amit as he has the context)

While working on [1], I realized that on HEAD there is a problem with the
$subject.  Here is the relevant discussion on the thread [2]. Quoting my
own notes on that thread below;

I realized that the dropped columns also get into the tuples_equal()
> function. And,
> the remote sends NULL to for the dropped columns(i.e., remoteslot), but
> index_getnext_slot() (or table_scan_getnextslot) indeed fills the dropped
> columns on the outslot. So, the dropped columns are not NULL in the outslot


Amit also suggested checking generated columns, which indeed has the same
problem.

Here are the steps to repro the problem with dropped columns:

- pub
CREATE TABLE test (drop_1 jsonb, x int, drop_2 numeric, y text, drop_3
timestamptz);
ALTER TABLE test REPLICA IDENTITY FULL;
INSERT INTO test SELECT NULL, i, i, (i)::text, now() FROM
generate_series(0,1)i;
CREATE PUBLICATION pub FOR ALL TABLES;

-- sub
CREATE TABLE test (drop_1 jsonb, x int, drop_2 numeric, y text, drop_3
timestamptz);
CREATE SUBSCRIPTION sub CONNECTION 'host=localhost port=5432
dbname=postgres' PUBLICATION pub;

-- show that before dropping the columns, the data in the source and
-- target are deleted properly
DELETE FROM test WHERE x = 0;

-- both on the source and target
SELECT count(*) FROM test WHERE x = 0;
┌───────┐
│ count │
├───────┤
│     0 │
└───────┘
(1 row)

-- drop columns on both the the source
ALTER TABLE test DROP COLUMN drop_1;
ALTER TABLE test DROP COLUMN drop_2;
ALTER TABLE test DROP COLUMN drop_3;

-- drop columns on both the the target
ALTER TABLE test DROP COLUMN drop_1;
ALTER TABLE test DROP COLUMN drop_2;
ALTER TABLE test DROP COLUMN drop_3;

-- on the target
ALTER SUBSCRIPTION sub REFRESH PUBLICATION;

-- after dropping the columns
DELETE FROM test WHERE x = 1;

-- source
SELECT count(*) FROM test WHERE x = 1;
┌───────┐
│ count │
├───────┤
│     0 │
└───────┘
(1 row)


**-- target, OOPS wrong result!!!!**SELECT count(*) FROM test WHERE x = 1;
┌───────┐
│ count │
├───────┤
│     1 │
└───────┘
(1 row)



Attaching a patch that could possibly solve the problem.

Thanks,
Onder KALACI


[1]
https://www.postgresql.org/message-id/flat/CACawEhUN%3D%2BvjY0%2B4q416-rAYx6pw-nZMHQYsJZCftf9MjoPN3w%40mail.gmail.com#2f7fa76f9e4496e3b52a9be6736e5b43
[2]
https://www.postgresql.org/message-id/CACawEhUu6S8E4Oo7%2Bs5iaq%3DyLRZJb6uOZeEQSGJj-7NVkDzSaw%40mail.gmail.com

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

    Yu Shi (Fujitsu) <shiy.fnst@fujitsu.com> — 2023-03-13T10:53:03Z

    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

  3. Re: Dropped and generated columns might cause wrong data on subs when REPLICA IDENTITY FULL

    Önder Kalacı <onderkalaci@gmail.com> — 2023-03-13T12:56:28Z

    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

  4. Re: Dropped and generated columns might cause wrong data on subs when REPLICA IDENTITY FULL

    Amit Kapila <amit.kapila16@gmail.com> — 2023-03-16T09:23:19Z

    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.

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

    Yu Shi (Fujitsu) <shiy.fnst@fujitsu.com> — 2023-03-16T10:38:25Z

    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

  6. Re: Dropped and generated columns might cause wrong data on subs when REPLICA IDENTITY FULL

    Önder Kalacı <onderkalaci@gmail.com> — 2023-03-16T16:03:25Z

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

  7. Re: Dropped and generated columns might cause wrong data on subs when REPLICA IDENTITY FULL

    Amit Kapila <amit.kapila16@gmail.com> — 2023-03-17T00:08:50Z

    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.

  8. Re: Dropped and generated columns might cause wrong data on subs when REPLICA IDENTITY FULL

    Tom Lane <tgl@sss.pgh.pa.us> — 2023-03-17T00:11:37Z

    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

  9. Re: Dropped and generated columns might cause wrong data on subs when REPLICA IDENTITY FULL

    Amit Kapila <amit.kapila16@gmail.com> — 2023-03-17T00:14:15Z

    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.

  10. Re: Dropped and generated columns might cause wrong data on subs when REPLICA IDENTITY FULL

    Önder Kalacı <onderkalaci@gmail.com> — 2023-03-17T07:38:02Z

    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

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

    Yu Shi (Fujitsu) <shiy.fnst@fujitsu.com> — 2023-03-17T08:45:29Z

    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

  12. Re: Dropped and generated columns might cause wrong data on subs when REPLICA IDENTITY FULL

    Önder Kalacı <onderkalaci@gmail.com> — 2023-03-17T15:28:46Z

    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

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

    Yu Shi (Fujitsu) <shiy.fnst@fujitsu.com> — 2023-03-20T07:18:22Z

    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

  14. Re: Dropped and generated columns might cause wrong data on subs when REPLICA IDENTITY FULL

    Önder Kalacı <onderkalaci@gmail.com> — 2023-03-20T09:28:06Z

    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

  15. Re: Dropped and generated columns might cause wrong data on subs when REPLICA IDENTITY FULL

    Amit Kapila <amit.kapila16@gmail.com> — 2023-03-20T12:58:19Z

    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.

  16. Re: Dropped and generated columns might cause wrong data on subs when REPLICA IDENTITY FULL

    Amit Kapila <amit.kapila16@gmail.com> — 2023-03-21T06:07:05Z

    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.

  17. Re: Dropped and generated columns might cause wrong data on subs when REPLICA IDENTITY FULL

    Önder Kalacı <onderkalaci@gmail.com> — 2023-03-21T08:51:20Z

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

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

    Yu Shi (Fujitsu) <shiy.fnst@fujitsu.com> — 2023-03-21T10:07:11Z

    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

  19. Re: Dropped and generated columns might cause wrong data on subs when REPLICA IDENTITY FULL

    Önder Kalacı <onderkalaci@gmail.com> — 2023-03-21T12:02:58Z

    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

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

    Yu Shi (Fujitsu) <shiy.fnst@fujitsu.com> — 2023-03-22T02:13:13Z

    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

  21. Re: Dropped and generated columns might cause wrong data on subs when REPLICA IDENTITY FULL

    Önder Kalacı <onderkalaci@gmail.com> — 2023-03-22T06:53:04Z

    Hi Shi Yu,


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

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

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

    Yu Shi (Fujitsu) <shiy.fnst@fujitsu.com> — 2023-03-22T08:08:22Z

    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

  23. Re: Dropped and generated columns might cause wrong data on subs when REPLICA IDENTITY FULL

    Amit Kapila <amit.kapila16@gmail.com> — 2023-03-23T13:45:48Z

    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.