On Tue, Mar 21, 2023 at 7:03 AM Peter Smith <smithpb2250@gmail.com> wrote: > > > ====== > src/test/subscription/t/014_binary.pl > > 4. > # ----------------------------------------------------- > # Test mismatched column types with/without binary mode > # ----------------------------------------------------- > > # Test syncing tables with mismatching column types > $node_publisher->safe_psql( > 'postgres', qq( > CREATE TABLE public.test_mismatching_types ( > a bigint PRIMARY KEY > ); > INSERT INTO public.test_mismatching_types (a) > VALUES (1), (2); > )); > > # Check the subscriber log from now on. > $offset = -s $node_subscriber->logfile; > > # Ensure the subscription is enabled. disable_on_error is still true, > # so the subscription can be disabled due to missing realtion until > # the test_mismatching_types table is created. > $node_subscriber->safe_psql( > 'postgres', qq( > CREATE TABLE public.test_mismatching_types ( > a int PRIMARY KEY > ); > ALTER SUBSCRIPTION tsub ENABLE; > ALTER SUBSCRIPTION tsub REFRESH PUBLICATION; > )); > > ~~ > > I found the "Ensure the subscription is enabled..." comment and the > necessity for enabling the subscription to be confusing. > > Can't some complications all be eliminated just by creating the table > on the subscribe side first? >
Hmm, that would make this test inconsistent with other tests and probably difficult to understand and extend. I don't like to say this but I think introducing disable_on_error has introduced more complexities in the patch due to the requirement of enabling subscription again and again. I feel it would be better without using disable_on_error option in these tests.
While this change would make the test inconsistent, I think it also would make more confusing.
Explaining the issue explicitly with a comment seems better to me than the trick of changing order of table creation just for some test cases.
But I'm also ok with removing the use of disable_on_error if that's what you agree on.
