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