Re: CREATE SUBSCRIPTION - add missing test case

On Sun, Dec 8, 2024 at 10:57 AM Tomas Vondra <tomas(at)vondra(dot)me> wrote:
>
> On 8/22/24 05:21, Peter Smith wrote:
> > ...
> >>>
> >>> I also don't see a test for this error condition. However, it is not
> >>> clear to me how important is it to cover this error code path. This
> >>> code has existed for a long time and I didn't notice any bugs related
> >>> to this. There is a possibility that in the future we might break
> >>> something because of a lack of this test but not sure if we want to
> >>> cover every code path via tests as each additional test also has some
> >>> cost. OTOH, If others think it is important or a good idea to have
> >>> this test then I don't have any objection to the same.
> >>
> >> Yes, AFAIK there were no bugs related to this; The test was proposed
> >> to prevent accidental future bugs.
> >>
>
> Not sure if absence of prior bug reports is a good data point to decide
> which tests are useful. It seems worth checking we keep reporting the
> error, even if it seems unlikely we'd break that.
>
> >> BACKGROUND
> >>
> >> Another pending feature thread (replication of generated columns) [1]
> >> required many test combinations to confirm all the different expected
> >> results which are otherwise easily accidentally broken without
> >> noticing. This *current* thread test shares one of the same error
> >> messages, which is how it was discovered missing in the first place.
> >>
> >> ~~~
> >>
>
> Right.
>
> >> PROPOSAL
> >>
> >> I think this is not the first time a logical replication test has been
> >> questioned due mostly to concern about creeping "costs".
> >>
> >> How about we create a new test file and put test cases like this one
> >> into it, guarded by code like the below using PG_TEST_EXTRA [2]?
> >>
> >> Doing it this way we can have better code coverage and higher
> >> confidence when we want it, but zero test cost overheads when we don't
> >> want it.
> >>
> >> e.g.
> >>
> >> src/test/subscription/t/101_extra.pl:
> >>
> >> if (!$ENV{PG_TEST_EXTRA} || $ENV{PG_TEST_EXTRA} !~ /\bsubscription\b/)
> >> {
> >> plan skip_all =>
> >> 'Due to execution costs these tests are skipped unless subscription
> >> is enabled in PG_TEST_EXTRA';
> >> }
> >>
> >> # Add tests here...
> >>
> >
> > To help strengthen the above proposal, here are a couple of examples
> > where TAP tests already use this strategy to avoid tests for various
> > reasons.
> >
> > [1] Avoids some test because of cost
> > # WAL consistency checking is resource intensive so require opt-in with the
> > # PG_TEST_EXTRA environment variable.
> > if ( $ENV{PG_TEST_EXTRA}
> > && $ENV{PG_TEST_EXTRA} =~ m/\bwal_consistency_checking\b/)
> > {
> > $node_primary->append_conf('postgresql.conf',
> > 'wal_consistency_checking = all');
> > }
> >
> > [2] Avoids some tests because of safety
> > if (!$ENV{PG_TEST_EXTRA} || $ENV{PG_TEST_EXTRA} !~ /\bload_balance\b/)
> > {
> > plan skip_all =>
> > 'Potentially unsafe test load_balance not enabled in PG_TEST_EXTRA';
> > }
> >
>
> Yes, there are cases with logical replication where reproducing may be
> expensive (in terms of data amounts, time, ...) but I don't think that's
> the case here - this test is trivial/cheap.
>
> But I believe the "costs" mentioned by Amit are more about having to
> maintain the tests etc. rather than execution costs. In which case
> having a flag does exactly nothing - we'd still have to maintain it.
>
> I propose we simply add the test to 008_diff_schema.pl, per v2. I see no
> reason to invent something more here.
>

Hi Tomas,

Since you think patch v2 is already OK as-is, I have changed the
commitfest entry [1] for this to RfC.

======
[1] https://commitfest.postgresql.org/51/5190/

Kind Regards,
Peter Smith.
Fujitsu Australia