On Mon, Jul 29, 2024 at 12:57 PM Peter Smith <smithpb2250@gmail.com> wrote:
>
> Thanks for the patch updates.
>
> Here are my review comments for v21-0001.
>
> I think this patch is mostly OK now except there are still some
> comments about the TAP test.
>
> ======
> Commit Message
>
> 0.
> Using Create Subscription:
> CREATE SUBSCRIPTION sub2_gen_to_gen CONNECTION '$publisher_connstr' PUBLICATION
> pub1 WITH (include_generated_columns = true, copy_data = false)"
>
> If you are going to give an example, I think a gen-to-nogen example
> would be a better choice. That's because the gen-to-gen (as you have
> here) is not going to replicate anything due to the subscriber-side
> column being generated.
>
> ======
> src/test/subscription/t/011_generated.pl
>
> 1.
> +$node_subscriber2->safe_psql('postgres',
> + "CREATE TABLE tab1 (a int PRIMARY KEY, b int GENERATED ALWAYS AS (a
> * 22) STORED, c int)"
> +);
>
> The subscriber2 node was intended only for all the tables where we
> need include_generated_columns to be true. Mostly that is the
> combination tests. (tab_gen_to_nogen, tab_nogen_to_gen, etc) OTOH,
> table 'tab1' already existed. I don't think we need to bother
> subscribing to tab1 from subscriber2 because every combination is
> already covered by the combination tests. Let's leave this one alone.
>
>
> ~~~
>
> 2.
> Huh? Where is the "tab_nogen_to_gen" combination test that I sent to
> you off-list?
>
> ~~~
>
> 3.
> +$node_subscriber->safe_psql('postgres',
> + "CREATE TABLE tab_order (c int GENERATED ALWAYS AS (a * 22) STORED,
> a int, b int)"
> +);
>
> Maybe you can test 'tab_order' on both subscription nodes but I think
> it is overkill. IMO it is enough to test it on subscription2.
>
> ~~~
>
> 4.
> +$node_subscriber->safe_psql('postgres',
> + "CREATE TABLE tab_alter (a int, b int, c int GENERATED ALWAYS AS (a
> * 22) STORED)"
> +);
>
> Ditto above. Maybe you can test 'tab_order' on both subscription nodes
> but I think it is overkill. IMO it is enough to test it on
> subscription2.
>
> ~~~
>
> 5.
> Don't forget to add initial data for the missing nogen_to_gen table/test.
>
> ~~~
>
> 6.
> $node_publisher->safe_psql('postgres',
> - "CREATE PUBLICATION pub1 FOR ALL TABLES");
> + "CREATE PUBLICATION pub1 FOR TABLE tab1, tab_gen_to_gen,
> tab_gen_to_nogen, tab_gen_to_missing, tab_missing_to_gen, tab_order");
> +
> $node_subscriber->safe_psql('postgres',
> "CREATE SUBSCRIPTION sub1 CONNECTION '$publisher_connstr' PUBLICATION pub1"
> );
>
> It is not a bad idea to reduce the number of publications as you have
> done, but IMO jamming all the tables into 1 publication is too much
> because it makes it less understandable instead of simpler.
>
> How about this:
> - leave the 'pub1' just for 'tab1'.
> - have a 'pub_combo' for publication all the gen_to_nogen,
> nogen_to_gen etc combination tests.
> - and a 'pub_misc' for any other misc tables like tab_order.
>
> ~~~
>
> 7.
> +#####################
> # Wait for initial sync of all subscriptions
> +#####################
>
> I think you should write a note here that you have deliberately set
> copy_data = false because COPY and include_generated_columns are not
> allowed at the same time for patch 0001. And that is why all expected
> results on subscriber2 will be empty. Also, say this limitation will
> be changed in patch 0002.
>
> ~~~
>
> (I didn't yet check 011_generated.pl file results beyond this point...
> I'll wait for v22-0001 to review further)
The attached Patches contain all the suggested changes.
v22-0001 - Addressed the comments.
v22-0002 - Rebased the Patch.
v22-0003 - Rebased the Patch.
v22-0004 - Rebased the Patch.
Thanks and Regards,
Shubham Khanna.
