Thread: Re: Collect statistics about conflicts in logical replication

On Tue, Aug 27, 2024 at 3:21 PM Zhijie Hou (Fujitsu)
<houzj.fnst@fujitsu.com> wrote:
>
> On Tuesday, August 27, 2024 10:59 AM Peter Smith <smithpb2250@gmail.com> wrote:
> >
> > ~~~
> >
> > 3.
> > +# Enable track_commit_timestamp to detect origin-differ conflicts in
> > +logical # replication. Reduce wal_retrieve_retry_interval to 1ms to
> > +accelerate the # restart of the logical replication worker after encountering a
> > conflict.
> > +$node_subscriber->append_conf(
> > + 'postgresql.conf', q{
> > +track_commit_timestamp = on
> > +wal_retrieve_retry_interval = 1ms
> > +});
> >
> > Later, after CDR resolvers are implemented, it might be good to revisit these
> > conflict test cases and re-write them to use some conflict resolvers like 'skip'.
> > Then the subscriber won't give ERRORs and restart apply workers all the time
> > behind the scenes, so you won't need the above configuration for accelerating
> > the worker restarts. In other words, running these tests might be more efficient
> > if you can avoid restarting workers all the time.
> >
> > I suggest putting an XXX comment here as a reminder that these tests should
> > be revisited to make use of conflict resolvers in the future.
>
> I think it would be too early to mention the resolution implementation detail
> in the comments considering that the resolution is still not RFC. Also, I think
> reducing wal_retrieve_retry_interval is a reasonable way to speed up the test
> in this case because the test is not letting the worker to restart all the time, the
> error causes the restart will be resolved immediately after the stats check. So, I
> think adding XXX is not very appropriate.
>
> Other comments look good to me.
> I slightly adjusted few words and merged the changes. Thanks !
>
> Here is V3 patch.
>

Thanks for the patch. Just thinking out loud, since we have names like
'apply_error_count', 'sync_error_count' which tells that they are
actually error-count, will it be better to have something similar in
conflict-count cases, like 'insert_exists_conflict_count',
'delete_missing_conflict_count' and so on. Thoughts?

I noticed that now we do mention this (as I suggested earlier):
+ Note that any conflict resulting in an apply error will be counted
in both apply_error_count and the corresponding conflict count.

But we do not mention clearly which ones are conflict-counts. As an
example, we have this:

+ insert_exists_count bigint:
+ Number of times a row insertion violated a NOT DEFERRABLE unique
constraint during the application of changes

It does not mention that it is a conflict count. So we need to either
change names or mention clearly against each that it is a conflict
count.

thanks
sHveta

On Wed, Aug 28, 2024 at 11:43 AM shveta malik <shveta.malik@gmail.com> wrote:
>
> Thanks for the patch. Just thinking out loud, since we have names like
> 'apply_error_count', 'sync_error_count' which tells that they are
> actually error-count, will it be better to have something similar in
> conflict-count cases, like 'insert_exists_conflict_count',
> 'delete_missing_conflict_count' and so on. Thoughts?
>

It would be better to have conflict in the names but OTOH it will make
the names a bit longer. The other alternatives could be (a)
insert_exists_confl_count, etc. (b) confl_insert_exists_count, etc.
(c) confl_insert_exists, etc. These are based on the column names in
the existing view pg_stat_database_conflicts [1]. The (c) looks better
than other options but it will make the conflict-related columns
different from error-related columns.

Yet another option is to have a different view like
pg_stat_subscription_conflicts but that sounds like going too far.

[1] - https://www.postgresql.org/docs/devel/monitoring-stats.html#MONITORING-PG-STAT-DATABASE-CONFLICTS-VIEW

--
With Regards,
Amit Kapila.

On Wed, Aug 28, 2024 at 9:19 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
>
> On Wed, Aug 28, 2024 at 11:43 AM shveta malik <shveta.malik@gmail.com> wrote:
> >
> > Thanks for the patch. Just thinking out loud, since we have names like
> > 'apply_error_count', 'sync_error_count' which tells that they are
> > actually error-count, will it be better to have something similar in
> > conflict-count cases, like 'insert_exists_conflict_count',
> > 'delete_missing_conflict_count' and so on. Thoughts?
> >
>
> It would be better to have conflict in the names but OTOH it will make
> the names a bit longer. The other alternatives could be (a)
> insert_exists_confl_count, etc. (b) confl_insert_exists_count, etc.
> (c) confl_insert_exists, etc. These are based on the column names in
> the existing view pg_stat_database_conflicts [1]. The (c) looks better
> than other options but it will make the conflict-related columns
> different from error-related columns.
>
> Yet another option is to have a different view like
> pg_stat_subscription_conflicts but that sounds like going too far.
>
> [1] - https://www.postgresql.org/docs/devel/monitoring-stats.html#MONITORING-PG-STAT-DATABASE-CONFLICTS-VIEW

Option (c) looked good to me.

Removing the suffix "_count" is OK. For example, try searching all of
Chapter 27 ("The Cumulative Statistics System") [1] for columns
described as "Number of ..." and you will find that a "_count" suffix
is used only rarely.

Adding the prefix "confl_" is OK. As mentioned, there is a precedent
for this. See "pg_stat_database_conflicts" [2].

Mixing column names where some have and some do not have "_count"
suffixes may not be ideal, but I see no problem because there are
precedents for that too. E.g. see "pg_stat_replication_slots" [3], and
"pg_stat_all_tables" [4].

======
[1] https://www.postgresql.org/docs/devel/monitoring-stats.html
[2] https://www.postgresql.org/docs/devel/monitoring-stats.html#MONITORING-PG-STAT-DATABASE-CONFLICTS-VIEW
[3] https://www.postgresql.org/docs/devel/monitoring-stats.html#MONITORING-PG-STAT-REPLICATION-SLOTS-VIEW
[4] https://www.postgresql.org/docs/devel/monitoring-stats.html#MONITORING-PG-STAT-ALL-TABLES-VIEW

Kind Regards,
Peter Smith.
Fujitsu Australia

On Thu, Aug 29, 2024 at 4:59 AM Peter Smith <smithpb2250@gmail.com> wrote:
>
> On Wed, Aug 28, 2024 at 9:19 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
> >
> > On Wed, Aug 28, 2024 at 11:43 AM shveta malik <shveta.malik@gmail.com> wrote:
> > >
> > > Thanks for the patch. Just thinking out loud, since we have names like
> > > 'apply_error_count', 'sync_error_count' which tells that they are
> > > actually error-count, will it be better to have something similar in
> > > conflict-count cases, like 'insert_exists_conflict_count',
> > > 'delete_missing_conflict_count' and so on. Thoughts?
> > >
> >
> > It would be better to have conflict in the names but OTOH it will make
> > the names a bit longer. The other alternatives could be (a)
> > insert_exists_confl_count, etc. (b) confl_insert_exists_count, etc.
> > (c) confl_insert_exists, etc. These are based on the column names in
> > the existing view pg_stat_database_conflicts [1]. The (c) looks better
> > than other options but it will make the conflict-related columns
> > different from error-related columns.
> >
> > Yet another option is to have a different view like
> > pg_stat_subscription_conflicts but that sounds like going too far.

Yes, I think we are good with pg_stat_subscription_stats for the time being.

> >
> > [1] - https://www.postgresql.org/docs/devel/monitoring-stats.html#MONITORING-PG-STAT-DATABASE-CONFLICTS-VIEW
>
> Option (c) looked good to me.

+1 for option c. it should be okay to not have '_count' in the name.

> Removing the suffix "_count" is OK. For example, try searching all of
> Chapter 27 ("The Cumulative Statistics System") [1] for columns
> described as "Number of ..." and you will find that a "_count" suffix
> is used only rarely.
>
> Adding the prefix "confl_" is OK. As mentioned, there is a precedent
> for this. See "pg_stat_database_conflicts" [2].
>
> Mixing column names where some have and some do not have "_count"
> suffixes may not be ideal, but I see no problem because there are
> precedents for that too. E.g. see "pg_stat_replication_slots" [3], and
> "pg_stat_all_tables" [4].
>
> ======
> [1] https://www.postgresql.org/docs/devel/monitoring-stats.html
> [2] https://www.postgresql.org/docs/devel/monitoring-stats.html#MONITORING-PG-STAT-DATABASE-CONFLICTS-VIEW
> [3] https://www.postgresql.org/docs/devel/monitoring-stats.html#MONITORING-PG-STAT-REPLICATION-SLOTS-VIEW
> [4] https://www.postgresql.org/docs/devel/monitoring-stats.html#MONITORING-PG-STAT-ALL-TABLES-VIEW
>
> Kind Regards,
> Peter Smith.
> Fujitsu Australia

On Thu, Aug 29, 2024 at 11:06 AM Zhijie Hou (Fujitsu)
<houzj.fnst@fujitsu.com> wrote:
>
>
> Agreed. Here is new version patch which change the names as suggested. I also
> rebased the patch based on another renaming commit 640178c9.
>

Thanks for the patch. Few minor things:

1)
conflict.h:
 * This enum is used in statistics collection (see
 * PgStat_StatSubEntry::conflict_count) as well, therefore, when adding new
 * values or reordering existing ones, ensure to review and potentially adjust
 * the corresponding statistics collection codes.

--We shall mention PgStat_BackendSubEntry as well.

026_stats.pl:
2)
# Now that the table is empty, the
# update conflict (update_existing) ERRORs will stop happening.

--Shall it be update_exists instead of update_existing here:

3)
This is an existing comment above insert_exists conflict capture:
# Wait for the apply error to be reported.

--Shall we change to:
# Wait for the subscriber to report apply error and insert_exists conflict.

thanks
Shveta

Hi Hou-San. Here are my review comments for v4-0001.

======

1. Add links in the docs

IMO it would be good for all these confl_* descriptions (in
doc/src/sgml/monitoring.sgml) to include links back to where each of
those conflict types was defined [1].

Indeed, when links are included to the original conflict type
information then I think you should remove mentioning
"track_commit_timestamp":
+       counted only when the
+       <link linkend="guc-track-commit-timestamp"><varname>track_commit_timestamp</varname></link>
+       option is enabled on the subscriber.

It should be obvious that you cannot count a conflict if the conflict
does not happen, but I don't think we should scatter/duplicate those
rules in different places saying when certain conflicts can/can't
happen -- we should just link everywhere back to the original
description for those rules.

~~~

2. Arrange all the counts into an intuitive/natural order

There is an intuitive/natural ordering for these counts. For example,
the 'confl_*' count fields are in the order insert -> update ->
delete, which LGTM.

Meanwhile, the 'apply_error_count' and the 'sync_error_count' are not
in a good order.

IMO it makes more sense if everything is ordered as:
'sync_error_count', then 'apply_error_count', then all the 'confl_*'
counts.

This comment applies to lots of places, e.g.:
- docs (doc/src/sgml/monitoring.sgml)
- function pg_stat_get_subscription_stats (pg_proc.dat)
- view pg_stat_subscription_stats (src/backend/catalog/system_views.sql)
- TAP test SELECTs (test/subscription/t/026_stats.pl)

As all those places are already impacted by this patch, I think it
would be good if (in passing) we (if possible) also swapped the
sync/apply counts so everything  is ordered intuitively top-to-bottom
or left-to-right.

======
[1] https://www.postgresql.org/docs/devel/logical-replication-conflicts.html#LOGICAL-REPLICATION-CONFLICTS

Kind Regards,
Peter Smith.
Fujitsu Australia

On Fri, Aug 30, 2024 at 9:40 AM shveta malik <shveta.malik@gmail.com> wrote:
>
> On Thu, Aug 29, 2024 at 11:06 AM Zhijie Hou (Fujitsu)
> <houzj.fnst@fujitsu.com> wrote:
> >
> >
> > Agreed. Here is new version patch which change the names as suggested. I also
> > rebased the patch based on another renaming commit 640178c9.
> >
>
> Thanks for the patch. Few minor things:
>
> 1)
> conflict.h:
>  * This enum is used in statistics collection (see
>  * PgStat_StatSubEntry::conflict_count) as well, therefore, when adding new
>  * values or reordering existing ones, ensure to review and potentially adjust
>  * the corresponding statistics collection codes.
>
> --We shall mention PgStat_BackendSubEntry as well.
>
> 026_stats.pl:
> 2)
> # Now that the table is empty, the
> # update conflict (update_existing) ERRORs will stop happening.
>
> --Shall it be update_exists instead of update_existing here:
>
> 3)
> This is an existing comment above insert_exists conflict capture:
> # Wait for the apply error to be reported.
>
> --Shall we change to:
> # Wait for the subscriber to report apply error and insert_exists conflict.
>

1) I have tested the patch, works well.
2) Verified headers inclusions, all good
3) All my comments (very old ones when the patch was initially posted)
are now addressed.

So apart from the comments I posted in [1],  I have no more comments.

[1]: https://www.postgresql.org/message-id/CAJpy0uAZpzustNOMBhxBctHHWbBA%3DsnTAYsLpoWZg%2BcqegmD-A%40mail.gmail.com

thanks
Shveta

On Fri, Aug 30, 2024 at 10:53 AM Peter Smith <smithpb2250@gmail.com> wrote:
>
> Hi Hou-San. Here are my review comments for v4-0001.
>
> ======
>
> 1. Add links in the docs
>
> IMO it would be good for all these confl_* descriptions (in
> doc/src/sgml/monitoring.sgml) to include links back to where each of
> those conflict types was defined [1].
>
> Indeed, when links are included to the original conflict type
> information then I think you should remove mentioning
> "track_commit_timestamp":
> +       counted only when the
> +       <link linkend="guc-track-commit-timestamp"><varname>track_commit_timestamp</varname></link>
> +       option is enabled on the subscriber.
>
> It should be obvious that you cannot count a conflict if the conflict
> does not happen, but I don't think we should scatter/duplicate those
> rules in different places saying when certain conflicts can/can't
> happen -- we should just link everywhere back to the original
> description for those rules.

+1, will make the doc better.

> ~~~
>
> 2. Arrange all the counts into an intuitive/natural order
>
> There is an intuitive/natural ordering for these counts. For example,
> the 'confl_*' count fields are in the order insert -> update ->
> delete, which LGTM.
>
> Meanwhile, the 'apply_error_count' and the 'sync_error_count' are not
> in a good order.
>
> IMO it makes more sense if everything is ordered as:
> 'sync_error_count', then 'apply_error_count', then all the 'confl_*'
> counts.
>
> This comment applies to lots of places, e.g.:
> - docs (doc/src/sgml/monitoring.sgml)
> - function pg_stat_get_subscription_stats (pg_proc.dat)
> - view pg_stat_subscription_stats (src/backend/catalog/system_views.sql)
> - TAP test SELECTs (test/subscription/t/026_stats.pl)
>
> As all those places are already impacted by this patch, I think it
> would be good if (in passing) we (if possible) also swapped the
> sync/apply counts so everything  is ordered intuitively top-to-bottom
> or left-to-right.

Not sure about this though. It does not seem to belong to the current patch.

thanks
Shveta

On Fri, Aug 30, 2024 at 12:15 PM Zhijie Hou (Fujitsu)
<houzj.fnst@fujitsu.com> wrote:
>
>
> Here is V5 patch which addressed above and Shveta's[1] comments.
>

The patch looks good to me.

thanks
Shveta

On Fri, Aug 30, 2024 at 6:36 PM shveta malik <shveta.malik@gmail.com> wrote:
>
> On Fri, Aug 30, 2024 at 12:15 PM Zhijie Hou (Fujitsu)
> <houzj.fnst@fujitsu.com> wrote:
> >
> >
> > Here is V5 patch which addressed above and Shveta's[1] comments.
> >
>
> The patch looks good to me.
>

Patch v5 LGTM.

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

On Fri, Aug 30, 2024 at 4:24 PM shveta malik <shveta.malik@gmail.com> wrote:
>
> On Fri, Aug 30, 2024 at 10:53 AM Peter Smith <smithpb2250@gmail.com> wrote:
> >
...
> > 2. Arrange all the counts into an intuitive/natural order
> >
> > There is an intuitive/natural ordering for these counts. For example,
> > the 'confl_*' count fields are in the order insert -> update ->
> > delete, which LGTM.
> >
> > Meanwhile, the 'apply_error_count' and the 'sync_error_count' are not
> > in a good order.
> >
> > IMO it makes more sense if everything is ordered as:
> > 'sync_error_count', then 'apply_error_count', then all the 'confl_*'
> > counts.
> >
> > This comment applies to lots of places, e.g.:
> > - docs (doc/src/sgml/monitoring.sgml)
> > - function pg_stat_get_subscription_stats (pg_proc.dat)
> > - view pg_stat_subscription_stats (src/backend/catalog/system_views.sql)
> > - TAP test SELECTs (test/subscription/t/026_stats.pl)
> >
> > As all those places are already impacted by this patch, I think it
> > would be good if (in passing) we (if possible) also swapped the
> > sync/apply counts so everything  is ordered intuitively top-to-bottom
> > or left-to-right.
>
> Not sure about this though. It does not seem to belong to the current patch.
>

Fair enough. But, besides being inappropriate to include in the
current patch, do you think the suggestion to reorder them made sense?
If it has some merit, then I will propose it again as a separate
thread.

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

On Mon, Sep 2, 2024 at 4:20 AM Peter Smith <smithpb2250@gmail.com> wrote:
>
> On Fri, Aug 30, 2024 at 4:24 PM shveta malik <shveta.malik@gmail.com> wrote:
> >
> > On Fri, Aug 30, 2024 at 10:53 AM Peter Smith <smithpb2250@gmail.com> wrote:
> > >
> ...
> > > 2. Arrange all the counts into an intuitive/natural order
> > >
> > > There is an intuitive/natural ordering for these counts. For example,
> > > the 'confl_*' count fields are in the order insert -> update ->
> > > delete, which LGTM.
> > >
> > > Meanwhile, the 'apply_error_count' and the 'sync_error_count' are not
> > > in a good order.
> > >
> > > IMO it makes more sense if everything is ordered as:
> > > 'sync_error_count', then 'apply_error_count', then all the 'confl_*'
> > > counts.
> > >
> > > This comment applies to lots of places, e.g.:
> > > - docs (doc/src/sgml/monitoring.sgml)
> > > - function pg_stat_get_subscription_stats (pg_proc.dat)
> > > - view pg_stat_subscription_stats (src/backend/catalog/system_views.sql)
> > > - TAP test SELECTs (test/subscription/t/026_stats.pl)
> > >
> > > As all those places are already impacted by this patch, I think it
> > > would be good if (in passing) we (if possible) also swapped the
> > > sync/apply counts so everything  is ordered intuitively top-to-bottom
> > > or left-to-right.
> >
> > Not sure about this though. It does not seem to belong to the current patch.
> >
>
> Fair enough. But, besides being inappropriate to include in the
> current patch, do you think the suggestion to reorder them made sense?
> If it has some merit, then I will propose it again as a separate
> thread.
>

 Yes, I think it makes sense. With respect to internal code, it might
be still okay as is, but when it comes to pg_stat_subscription_stats,
I think it is better if user finds it in below order:
 subid | subname | sync_error_count | apply_error_count | confl_*

 rather than the existing one:
 subid | subname | apply_error_count | sync_error_count | confl_*

thanks
Shveta

On Mon, Sep 2, 2024 at 1:28 PM shveta malik <shveta.malik@gmail.com> wrote:
>
> On Mon, Sep 2, 2024 at 4:20 AM Peter Smith <smithpb2250@gmail.com> wrote:
> >
> > On Fri, Aug 30, 2024 at 4:24 PM shveta malik <shveta.malik@gmail.com> wrote:
> > >
> > > On Fri, Aug 30, 2024 at 10:53 AM Peter Smith <smithpb2250@gmail.com> wrote:
> > > >
> > ...
> > > > 2. Arrange all the counts into an intuitive/natural order
> > > >
> > > > There is an intuitive/natural ordering for these counts. For example,
> > > > the 'confl_*' count fields are in the order insert -> update ->
> > > > delete, which LGTM.
> > > >
> > > > Meanwhile, the 'apply_error_count' and the 'sync_error_count' are not
> > > > in a good order.
> > > >
> > > > IMO it makes more sense if everything is ordered as:
> > > > 'sync_error_count', then 'apply_error_count', then all the 'confl_*'
> > > > counts.
> > > >
> > > > This comment applies to lots of places, e.g.:
> > > > - docs (doc/src/sgml/monitoring.sgml)
> > > > - function pg_stat_get_subscription_stats (pg_proc.dat)
> > > > - view pg_stat_subscription_stats (src/backend/catalog/system_views.sql)
> > > > - TAP test SELECTs (test/subscription/t/026_stats.pl)
> > > >
> > > > As all those places are already impacted by this patch, I think it
> > > > would be good if (in passing) we (if possible) also swapped the
> > > > sync/apply counts so everything  is ordered intuitively top-to-bottom
> > > > or left-to-right.
> > >
> > > Not sure about this though. It does not seem to belong to the current patch.
> > >
> >
> > Fair enough. But, besides being inappropriate to include in the
> > current patch, do you think the suggestion to reorder them made sense?
> > If it has some merit, then I will propose it again as a separate
> > thread.
> >
>
>  Yes, I think it makes sense. With respect to internal code, it might
> be still okay as is, but when it comes to pg_stat_subscription_stats,
> I think it is better if user finds it in below order:
>  subid | subname | sync_error_count | apply_error_count | confl_*
>
>  rather than the existing one:
>  subid | subname | apply_error_count | sync_error_count | confl_*
>

Hi Shveta, Thanks. FYI - I created a new thread for this here [1].

======
[1] https://www.postgresql.org/message-id/CAHut+PvbOw90wgGF4aV1HyYtX=6pjWc+pn8_fep7L=aLXwXkqg@mail.gmail.com

Kind Regards,
Peter Smith.
Fujitsu Australia

On Fri, Aug 30, 2024 at 12:15 PM Zhijie Hou (Fujitsu)
<houzj.fnst@fujitsu.com> wrote:
>
> Here is V5 patch which addressed above and Shveta's[1] comments.
>

Testing the stats for all types of conflicts is not required for this
patch especially because they increase the timings by 3-4s. We can add
tests for one or two types of conflicts.

*
(see
+ * PgStat_StatSubEntry::conflict_count and PgStat_StatSubEntry::conflict_count)

There is a typo in the above comment.

--
With Regards,
Amit Kapila.

On Tue, Sep 3, 2024 at 9:23 PM Zhijie Hou (Fujitsu)
<houzj.fnst@fujitsu.com> wrote:
>
> On Tuesday, September 3, 2024 7:12 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
> >
> > Testing the stats for all types of conflicts is not required for this patch
> > especially because they increase the timings by 3-4s. We can add tests for one
> > or two types of conflicts.
> >
...
>
> Thanks for the comments. I have addressed the comments and adjusted the tests.
> In the V6 patch, Only insert_exists and delete_missing are tested.
>
> I confirmed that it only increased the testing time by 1 second on my machine.
>
> Best Regards,
> Hou zj

It seems a pity to throw away perfectly good test cases just because
they increase how long the suite takes to run.

This seems like yet another example of where we could have made good
use of the 'PG_TEST_EXTRA' environment variable. I have been trying to
propose adding "subscription" support for this in another thread [1].
By using this variable to make some tests conditional, we could have
the best of both worlds. e.g.
- retain all tests, but
- by default, only run a subset of those tests (to keep default test
execution time low).

I hope that if the idea to use PG_TEST_EXTRA for "subscription" tests
gets accepted then later we can revisit this, and put all the removed
extra test cases back in again.

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

Kind Regards,
Peter Smith.
Fujitsu Australia

On Wed, Sep 4, 2024 at 9:17 AM Peter Smith <smithpb2250@gmail.com> wrote:
>
> On Tue, Sep 3, 2024 at 9:23 PM Zhijie Hou (Fujitsu)
> <houzj.fnst@fujitsu.com> wrote:
> >
> > I confirmed that it only increased the testing time by 1 second on my machine.
> >
>
> It seems a pity to throw away perfectly good test cases just because
> they increase how long the suite takes to run.
>

We can take every possible test, but I worry about the time they
consume without adding much value and the maintenance burden. I feel
like core-code we should pay attention to tests as well and don't try
to jam all the possible tests testing mostly similar stuff. Each time
before committing or otherwise verifying the patch, we run make
check-world, so don't want that time to go enormously high. Having
said that, I don't want the added functionality shouldn't be tested
properly and I try my best to achieve that.

> This seems like yet another example of where we could have made good
> use of the 'PG_TEST_EXTRA' environment variable. I have been trying to
> propose adding "subscription" support for this in another thread [1].
> By using this variable to make some tests conditional, we could have
> the best of both worlds. e.g.
> - retain all tests, but
> - by default, only run a subset of those tests (to keep default test
> execution time low).
>
> I hope that if the idea to use PG_TEST_EXTRA for "subscription" tests
> gets accepted then later we can revisit this, and put all the removed
> extra test cases back in again.
>

I am not convinced that tests that are less useful than others or are
increasing the time are good to be kept under PG_TEST_EXTRA but if
more people advocate such an approach then it is worth considering.

--
With Regards,
Amit Kapila.