Thread: Re: CREATE SUBSCRIPTION - add missing test case

On Thu, 15 Aug 2024 at 12:55, Peter Smith <smithpb2250@gmail.com> wrote:
>
> Hi Hackers,
>
> While reviewing another logical replication thread [1], I found an
> ERROR scenario that seems to be untested.
>
> TEST CASE: Attempt CREATE SUBSCRIPTION where the subscriber table is
> missing some expected column(s).
>
> Attached is a patch to add the missing test for this error message.

I agree currently there is no test to hit this code. I'm not sure if
this is the correct location for the test, should it be included in
the 008_diff_schema.pl file? Additionally, the commenting style for
this test appears quite different from the others. Could we use a
commenting style consistent with the earlier tests?

Regards,
Vignesh

On Tue, 20 Aug 2024 at 08:21, Peter Smith <smithpb2250@gmail.com> wrote:
>
> On Fri, Aug 16, 2024 at 2:15 PM vignesh C <vignesh21@gmail.com> wrote:
> >
>
> Thanks for the review.
>
> >
> > I agree currently there is no test to hit this code. I'm not sure if
> > this is the correct location for the test, should it be included in
> > the 008_diff_schema.pl file?
>
> Yes, that is a better home for this test. Done as suggested in
> attached patch v2.

Thanks, this looks good to me.

Regards,
Vignesh

On Fri, Aug 16, 2024 at 9:45 AM vignesh C <vignesh21@gmail.com> wrote:
>
> On Thu, 15 Aug 2024 at 12:55, Peter Smith <smithpb2250@gmail.com> wrote:
> >
> > Hi Hackers,
> >
> > While reviewing another logical replication thread [1], I found an
> > ERROR scenario that seems to be untested.
> >
> > TEST CASE: Attempt CREATE SUBSCRIPTION where the subscriber table is
> > missing some expected column(s).
> >
> > Attached is a patch to add the missing test for this error message.
>
> I agree currently there is no test to hit this code.
>

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.

--
With Regards,
Amit Kapila.

On Wed, Aug 21, 2024 at 8:48 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
>
> On Fri, Aug 16, 2024 at 9:45 AM vignesh C <vignesh21@gmail.com> wrote:
> >
> > On Thu, 15 Aug 2024 at 12:55, Peter Smith <smithpb2250@gmail.com> wrote:
> > >
> > > Hi Hackers,
> > >
> > > While reviewing another logical replication thread [1], I found an
> > > ERROR scenario that seems to be untested.
> > >
> > > TEST CASE: Attempt CREATE SUBSCRIPTION where the subscriber table is
> > > missing some expected column(s).
> > >
> > > Attached is a patch to add the missing test for this error message.
> >
> > I agree currently there is no test to hit this code.
> >
>
> 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.

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.

~~~

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...

======
[1] https://www.postgresql.org/message-id/flat/B80D17B2-2C8E-4C7D-87F2-E5B4BE3C069E%40gmail.com
[2] https://www.postgresql.org/docs/devel/regress-run.html#REGRESS-ADDITIONAL

Kind Regards,
Peter Smith.
Fujitsu Australia

On Thu, Aug 22, 2024 at 8:54 AM Peter Smith <smithpb2250@gmail.com> wrote:
>
> On Wed, Aug 21, 2024 at 8:48 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
> >
> > On Fri, Aug 16, 2024 at 9:45 AM vignesh C <vignesh21@gmail.com> wrote:
> > >
> > > On Thu, 15 Aug 2024 at 12:55, Peter Smith <smithpb2250@gmail.com> wrote:
> > > >
> > > > Hi Hackers,
> > > >
> > > > While reviewing another logical replication thread [1], I found an
> > > > ERROR scenario that seems to be untested.
> > > >
> > > > TEST CASE: Attempt CREATE SUBSCRIPTION where the subscriber table is
> > > > missing some expected column(s).
> > > >
> > > > Attached is a patch to add the missing test for this error message.
> > >
> > > I agree currently there is no test to hit this code.
> > >
> >
> > 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.
>
> 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.
>
> ~~~
>
> 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';
}

======
[1] https://github.com/postgres/postgres/blob/master/src/test/recovery/t/027_stream_regress.pl
[2] https://github.com/postgres/postgres/blob/master/src/interfaces/libpq/t/004_load_balance_dns.pl

Kind Regards,
Peter Smith.
Fujitsu Australia

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