Horton

Thread

Commits

  1. Add test case for when subscriber table is missing a column

  1. CREATE SUBSCRIPTION - add missing test case

    Peter Smith <smithpb2250@gmail.com> — 2024-08-15T07:25:16Z

    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.

======
[1] https://www.postgresql.org/message-id/CAHut%2BPt5FqV7J9GnnWFRNW_Z1KOMMAZXNTRcRNdtFrfMBz_GLA%40mail.gmail.com

Kind Regards,
Peter Smith.
Fujitsu Australia

  2. Re: CREATE SUBSCRIPTION - add missing test case

    vignesh C <vignesh21@gmail.com> — 2024-08-16T04:15:33Z

    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

  3. Re: CREATE SUBSCRIPTION - add missing test case

    Peter Smith <smithpb2250@gmail.com> — 2024-08-20T02:51:08Z

    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.

======
Kind Regards,
Peter Smith.
Fujitsu Australia

  4. Re: CREATE SUBSCRIPTION - add missing test case

    vignesh C <vignesh21@gmail.com> — 2024-08-20T12:19:54Z

    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

  5. Re: CREATE SUBSCRIPTION - add missing test case

    Amit Kapila <amit.kapila16@gmail.com> — 2024-08-21T10:48:03Z

    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.

  6. Re: CREATE SUBSCRIPTION - add missing test case

    Peter Smith <smithpb2250@gmail.com> — 2024-08-21T22:54:40Z

    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

  7. Re: CREATE SUBSCRIPTION - add missing test case

    Peter Smith <smithpb2250@gmail.com> — 2024-08-22T03:21:48Z

    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

  8. Re: CREATE SUBSCRIPTION - add missing test case

    Tomas Vondra <tomas@vondra.me> — 2024-12-07T23:57:32Z

    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

  9. Re: CREATE SUBSCRIPTION - add missing test case

    Peter Smith <smithpb2250@gmail.com> — 2025-01-10T03:11:19Z

    On Sun, Dec 8, 2024 at 10:57 AM Tomas Vondra <tomas@vondra.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

  10. Re: CREATE SUBSCRIPTION - add missing test case

    Heikki Linnakangas <hlinnaka@iki.fi> — 2025-03-24T10:20:17Z

    On 10/01/2025 05:11, Peter Smith wrote:
> On Sun, Dec 8, 2024 at 10:57 AM Tomas Vondra <tomas@vondra.me> wrote:
>> 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.

Committed, thanks!

It's always a balancing act between test coverage and how long the tests 
run. I agree this test is worth it, albeit with a thin margin.

-- 
Heikki Linnakangas
Neon (https://neon.tech)

  11. Re: CREATE SUBSCRIPTION - add missing test case

    Peter Smith <smithpb2250@gmail.com> — 2025-04-02T04:10:34Z

    Thanks for pushing.

======
Kind Regards,
Peter Smith.
Fujitsu Australia