On Thu, 8 Aug 2024 at 10:53, Shubham Khanna <khannashubham1...@gmail.com> wrote:
>
> On Wed, Aug 7, 2024 at 1:31 PM Peter Smith <smithpb2...@gmail.com> wrote:
> >
> > Hi Shubham,
> >
> > Here are my review comments for patch v24-0001
> >
> > I think the TAP tests have incorrect expected results for the nogen-to-gen
> > case.
> >
> > Whereas the HEAD code will cause "ERROR" for this test scenario, patch
> > 0001 does not. IMO the behaviour should be unchanged for this scenario
> > which has no generated column on the publisher side. So it seems this
> > is a bug in patch 0001.
> >
> > FYI, I have included "FIXME" comments in the attached top-up diff
> > patch to show which test cases I think are expecting wrong results.
> >
>
> Fixed all the comments. The attached Patch(v25-0001) contains all the changes.
Few comments:
1) Can we add one test with replica identity full to show that
generated column is included in case of update operation with
test_decoding.
2) At the end of the file generated_columns.sql a newline is missing:
+-- when 'include-generated-columns' = '0' the generated column 'b'
values will not be replicated
+INSERT INTO gencoltable (a) VALUES (7), (8), (9);
+SELECT data FROM pg_logical_slot_get_changes('regression_slot', NULL,
NULL, 'include-xids', '0', 'skip-empty-xacts', '1',
'include-generated-columns', '0');
+
+DROP TABLE gencoltable;
+
+SELECT 'stop' FROM pg_drop_replication_slot('regression_slot');
\ No newline at end of file
3)
3.a)This can be changed:
+-- when 'include-generated-columns' is not set the generated column
'b' values will be replicated
+INSERT INTO gencoltable (a) VALUES (1), (2), (3);
to:
-- By default, 'include-generated-columns' is enabled, so the values
for the generated column 'b' will be replicated even if it is not
explicitly specified.
3.b) This can be changed:
-- when 'include-generated-columns' = '1' the generated column 'b'
values will be replicated
to:
-- when 'include-generated-columns' is enabled, the values of the
generated column 'b' will be replicated.
3.c) This can be changed:
-- when 'include-generated-columns' = '0' the generated column 'b'
values will not be replicated
to:
-- when 'include-generated-columns' is disabled, the values of the
generated column 'b' will not be replicated.
4) I did not see any test for dump, can we add one test for this.
Regards,
Vignesh
