Re: Pgoutput not capturing the generated columns

Hi, Here are my review comments for patch v22-0001

All comments now are only for the TAP test.

======
src/test/subscription/t/011_generated.pl

1. I added all new code for the missing combination test case
"gen-to-missing". See nitpicks diff.
- create a separate publication for this "tab_gen_to_missing" table
because the test gives subscription errors.
- for the initial data
- for the replicated data

~~~

2. I added sub1 and sub2 subscriptions for every combo test
(previously some were absent). See nitpicks diff.

~~~

3. There was a missing test case for nogen-to-gen combination, and
after experimenting with this I am getting a bit suspicious,

Currently, it seems that if a COPY is attempted then the error would
be like this:
2024-08-01 17:16:45.110 AEST [18942] ERROR: column "b" is a generated column
2024-08-01 17:16:45.110 AEST [18942] DETAIL: Generated columns cannot
be used in COPY.

OTOH, if a COPY is not attempted (e.g. copy_data = false) then patch
0001 allows replication to happen. And the generated value of the
subscriber "b" takes precedence.

I have included these tests in the nitpicks diff of patch 0001.

Those results weren't exactly what I was expecting. That is why it is
so important to include *every* test combination in these TAP tests --
because unless we know how it works today, we won't know if we are
accidentally breaking the current behaviour with the other (0002,
0003) patches.

Please experiment in patches 0001 and 0002 using tab_nogen_to_gen more
to make sure the (new?) patch errors make sense and don't overstep by
giving ERRORs when they should not.

~~~~

Also, many other smaller issues/changes were done:

~~~

Creating tables:

nitpick - rearranged to keep all combo test SQLs in a consistent order
throughout this file
1/ gen-to-gen
2/ gen-to-nogen
3/ gen-to-missing
4/ missing-to-gen
5/ nogen-to-gen

nitpick - fixed the wrong comment for CREATE TABLE tab_nogen_to_gen.

nitpick - tweaked some CREATE TABLE comments for consistency.

nitpick - in the v22 patch many of the generated col 'b' use different
computations for every test. It makes it unnecessarily difficult to
read/review the expected results. So, I've made them all the same. Now
computation is "a * 2" on the publisher side, and "a * 22" on the
subscriber side.

~~~

Creating Publications and Subscriptions:

nitpick - added comment for all the CREATE PUBLICATION

nitpick - added comment for all the CREATE SUBSCRIPTION

nitpick - I moved the note about copy_data = false to where all the
node_subscriber2 subscriptions are created. Also, don't explicitly
refer to "patch 000" in the comment, because that will not make any
sense after getting pushed.

nitpick - I changed many subscriber names to consistently use "sub1"
or "sub2" within the name (this is the visual cue of which
node_subscriber<n> they are on). e.g.
/regress_sub_combo2/regress_sub2_combo/

~~~

Initial Sync tests:

nitpick - not sure if it is possible to do the initial data tests for
"nogen_to_gen" in the normal place. For now, it is just replaced by a
comment.
NOTE - Maybe this should be refactored later to put all the initial
data checks in one place. I'll think about this point more in the next
review.

~~~

nitpick - Changed cleanup I drop subscriptions before publications.

nitpick - remove the unnecessary blank line at the end.

======

Please see the attached diffs patch (apply it atop patch 0001) which
includes all the nipick changes mentioned above.

~~

BTW, For a quicker turnaround and less churning please consider just
posting the v23-0001 by itself instead of waiting to rebase all the
subsequent patches. When 0001 settles down some more then rebase the
others.

~~

Also, please run the indentation tool over this code ASAP.

======
Kind Regards,
Peter Smith.
Fujitsu Australia