Re: CREATE SUBSCRIPTION - add missing test case

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.

regards

--
Tomas Vondra