Re: CREATE SUBSCRIPTION - add missing test case - Mailing list pgsql-hackers

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