public inbox for [email protected]
help / color / mirror / Atom feedFrom: Peter Smith <[email protected]>
To: Masahiko Sawada <[email protected]>
Cc: Amit Kapila <[email protected]>
Cc: Andres Freund <[email protected]>
Cc: David G. Johnston <[email protected]>
Cc: pgsql-hackers <[email protected]>
Subject: Re: Design of pg_stat_subscription_workers vs pgstats
Date: Wed, 23 Feb 2022 17:20:58 +1100
Message-ID: <CAHut+Pt28WdVt44CMf+fE8HeWj6ciHtwJMrVbWMvLepe8_aUsQ@mail.gmail.com> (raw)
In-Reply-To: <CAHut+PtH-uN5rbGRh-=kCd8xvQYDf_JCcjLcVjW3OXGz6T+xCw@mail.gmail.com>
References: <[email protected]>
<CAA4eK1Jwn5hhgHJWZ9n+wYyQEk+brfR1zkuqLjh=waji8RYWAw@mail.gmail.com>
<CAD21AoAqEt=TbUDY96Uhf9pNuh-NDR_3-CByd+xR=tFEq==1NA@mail.gmail.com>
<CAD21AoA-MVZcMA1iYwbtvTCOHx1SwG8amQUTT8j2ssh2zuc6OQ@mail.gmail.com>
<[email protected]>
<CAD21AoDTO3bJvrc39Tv3VCeyUzZpimXnJuRhHQhKyjOWorHeOA@mail.gmail.com>
<[email protected]>
<CAD21AoA_r+Wc-zJZdy4Lii3oOvvdGcLwrY=2Y4y63HueYSd6Eg@mail.gmail.com>
<[email protected]>
<CAA4eK1+j00Pc+p=AfJLXh-a+OJ-a0UD4adMr2tRkQvKG_LggHA@mail.gmail.com>
<[email protected]>
<CAA4eK1J3dhfMRNQ6Q585nFr1vmXaTXVW9qv4hmUxLwDwJp-9RQ@mail.gmail.com>
<CAD21AoAtrgBEUU08VvfzRnGS+=9rS=MEq1cTLaZ1OT51xX34dQ@mail.gmail.com>
<CAHut+PtH-uN5rbGRh-=kCd8xvQYDf_JCcjLcVjW3OXGz6T+xCw@mail.gmail.com>
Below are my review comments just for the v1 patch test code.
======
1. "table sync error" versus "tablesync error"
+# Wait for the table sync error to be reported.
+$node_subscriber->poll_query_until(
+ 'postgres',
+ qq[
+SELECT apply_error_count = 0 AND sync_error_count > 0
+FROM pg_stat_subscription_activity
+WHERE subname = 'tap_sub'
+]) or die "Timed out while waiting for table sync error";
+
+# Truncate test_tab1 so that table sync can continue.
+$node_subscriber->safe_psql('postgres', "TRUNCATE test_tab1;");
+
# Wait for initial table sync for test_tab1 to finish.
IMO all these "table sync" should be changed to "tablesync", because a
table "sync error" sounds like something completely different to a
"tablesync error".
SUGGESTIONS
- "Wait for the table sync error to be reported." --> "Wait for the
tablesync error to be reported."
- "Timed out while waiting for table sync error" --> "Timed out while
waiting for tablesync error"
- "Truncate test_tab1 so that table sync can continue." --> "Truncate
test_tab1 so that tablesync worker can fun to completion."
- "Wait for initial table sync for test_tab1 to finish." --> "Wait for
the tablesync worker of test_tab1 to finish."
~~~
2. Unnecessary INSERT VALUES (2)?
(I think this is a duplicate of what [Osumi] #6 reported)
+# Insert more data to test_tab1 on the subscriber and then on the
publisher, raising an
+# error on the subscriber due to violation of the unique constraint
on test_tab1.
+$node_subscriber->safe_psql('postgres', "INSERT INTO test_tab1 VALUES (2)");
Why does the test do INSERT data (2)? There is already data (1) from
the tablesync which will cause an apply worker PK violation when
another VALUES (1) is published.
Note, the test comment also needs to change...
~~~
3. Wait for the apply worker error
+# Wait for the apply error to be reported.
+$node_subscriber->poll_query_until(
+ 'postgres',
+ qq[
+SELECT apply_error_count > 0 AND sync_error_count > 0
+FROM pg_stat_subscription_activity
+WHERE subname = 'tap_sub'
+]) or die "Timed out while waiting for apply error";
This test is only for apply worker errors. So why is the test SQL
checking "AND sync_error_count > 0"?
(This is similar to what [Tang] #2 reported, but I think she was
referring to the other tablesync test)
~~~
4. Wrong worker?
(looks like a duplicate of what [Osumi] #5 already)
+
+# Truncate test_tab1 so that table sync can continue.
+$node_subscriber->safe_psql('postgres', "TRUNCATE test_tab1;");
$node_subscriber->stop('fast');
$node_publisher->stop('fast');
Cut/paste error? Aren't you doing TRUNCATE here so the apply worker
can continue; not the tablesync worker (which already completed)
"Truncate test_tab1 so that table sync can continue." --> "Truncate
test_tab1 so that the apply worker can continue."
------
[Osumi] https://www.postgresql.org/message-id/CAD21AoBRt%3DcyKsZP83rcMkHnT498gHH0TEP34fZBrGCxT-Ahwg%40mail.g...
[Tang] https://www.postgresql.org/message-id/TYCPR01MB612840D018FEBD38268CC83BFB3C9%40TYCPR01MB6128.jpnprd0...
Kind Regards,
Peter Smith.
Fujitsu Australia
view thread (40+ messages) latest in thread
reply
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: [email protected]
Cc: [email protected], [email protected], [email protected], [email protected], [email protected]
Subject: Re: Design of pg_stat_subscription_workers vs pgstats
In-Reply-To: <CAHut+Pt28WdVt44CMf+fE8HeWj6ciHtwJMrVbWMvLepe8_aUsQ@mail.gmail.com>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox