On Mon, Mar 14, 2022 at 2:05 AM Melanie Plageman
<melanieplage...@gmail.com> wrote:
>
> On Sat, Mar 12, 2022 at 3:15 PM Andres Freund <and...@anarazel.de> wrote:
> >
> > Hi,
> >
> > On 2022-03-12 08:28:35 +0530, Amit Kapila wrote:
> > > On Sat, Mar 12, 2022 at 2:14 AM Melanie Plageman
> > > <melanieplage...@gmail.com> wrote:
> > > >
> > > > So, I noticed that pg_stat_reset_subscription_stats() wasn't working
> > > > properly, and, upon further investigation, I'm not sure the view
> > > > pg_stat_subscription_stats is being properly populated.
> > > >
> > >
> > > I have tried the below scenario based on this:
> > > Step:1 Create some data that generates conflicts and lead to apply
> > > failures and then check in the view:
> >
> > I think the problem is present when there was *no* conflict
> > previously. Because nothing populates the stats entry without an error, the
> > reset doesn't have anything to set the stats_reset field in, which then
> > means
> > that the stats_reset field is NULL even though stats have been reset.
>
> Yes, this is what I meant. stats_reset is not initialized and without
> any conflict happening to populate the stats, after resetting the stats,
> the field still does not get populated. I think this is a bit
> unexpected.
>
> psql (15devel)
> Type "help" for help.
>
> mplageman=# select * from pg_stat_subscription_stats ;
> subid | subname | apply_error_count | sync_error_count | stats_reset
> -------+---------+-------------------+------------------+-------------
> 16398 | mysub | 0 | 0 |
> (1 row)
>
> mplageman=# select pg_stat_reset_subscription_stats(16398);
> pg_stat_reset_subscription_stats
> ----------------------------------
>
> (1 row)
>
> mplageman=# select * from pg_stat_subscription_stats ;
> subid | subname | apply_error_count | sync_error_count | stats_reset
> -------+---------+-------------------+------------------+-------------
> 16398 | mysub | 0 | 0 |
> (1 row)
>
Looking at other statistics such as replication slots, shared stats,
and SLRU stats, it makes sense that resetting it populates the stats.
So we need to fix this issue.
However, I think the proposed fix has two problems; it can create an
entry for non-existing subscriptions if the user directly calls
function pg_stat_get_subscription_stats(), and stats_reset value is
not updated in the stats file as it is not done by the stats
collector.
An alternative solution would be to send the message for creating the
subscription at the end of CRAETE SUBSCRIPTION which basically
resolves them. A caveat is that if CREATE SUBSCRIPTION (that doesn't
involve replication slot creation) is rolled back, the first problem
still occurs. But it should not practically matter as a similar thing
is possible via existing table-related functions for dropped tables.
Also, we normally don't know the OID of subscription that is rolled
back. I've attached a patch for that.
Regards,
--
Masahiko Sawada
EDB: https://www.enterprisedb.com/
diff --git a/src/backend/commands/subscriptioncmds.c b/src/backend/commands/subscriptioncmds.c
index 3922658bbc..e3a878e7cc 100644
--- a/src/backend/commands/subscriptioncmds.c
+++ b/src/backend/commands/subscriptioncmds.c
@@ -611,6 +611,14 @@ CreateSubscription(ParseState *pstate, CreateSubscriptionStmt *stmt,
InvokeObjectPostCreateHook(SubscriptionRelationId, subid, 0);
+ /*
+ * Send a message for creating the subscription to the stats collector.
+ * CREATE SUBSCRIPTION that doesn't involve replication slot creation
+ * might be rolled back but these stats won't be shown on the
+ * pg_stat_subscription_stats view and removed later by (auto)vacuum.
+ */
+ pgstat_report_subscription_create(subid);
+
return myself;
}
diff --git a/src/backend/postmaster/pgstat.c b/src/backend/postmaster/pgstat.c
index 53ddd930e6..7a6bad0032 100644
--- a/src/backend/postmaster/pgstat.c
+++ b/src/backend/postmaster/pgstat.c
@@ -380,7 +380,7 @@ static void pgstat_recv_connect(PgStat_MsgConnect *msg, int len);
static void pgstat_recv_disconnect(PgStat_MsgDisconnect *msg, int len);
static void pgstat_recv_replslot(PgStat_MsgReplSlot *msg, int len);
static void pgstat_recv_tempfile(PgStat_MsgTempFile *msg, int len);
-static void pgstat_recv_subscription_drop(PgStat_MsgSubscriptionDrop *msg, int len);
+static void pgstat_recv_subscription(PgStat_MsgSubscription *msg, int len);
static void pgstat_recv_subscription_error(PgStat_MsgSubscriptionError *msg, int len);
/* ------------------------------------------------------------
@@ -1945,6 +1945,23 @@ pgstat_report_subscription_error(Oid subid, bool is_apply_error)
pgstat_send(&msg, sizeof(PgStat_MsgSubscriptionError));
}
+/* ----------
+ * pgstat_report_subscription_create() -
+ *
+ * Tell the collector about creating the subscription.
+ * ----------
+ */
+void
+pgstat_report_subscription_create(Oid subid)
+{
+ PgStat_MsgSubscription msg;
+
+ pgstat_setheader(&msg.m_hdr, PGSTAT_MTYPE_SUBSCRIPTION);
+ msg.m_subid = subid;
+ msg.m_create = true;
+ pgstat_send(&msg, sizeof(PgStat_MsgSubscription));
+}
+
/* ----------
* pgstat_report_subscription_drop() -
*
@@ -1954,11 +1971,12 @@ pgstat_report_subscription_error(Oid subid, bool is_apply_error)
void
pgstat_report_subscription_drop(Oid subid)
{
- PgStat_MsgSubscriptionDrop msg;
+ PgStat_MsgSubscription msg;
- pgstat_setheader(&msg.m_hdr, PGSTAT_MTYPE_SUBSCRIPTIONDROP);
+ pgstat_setheader(&msg.m_hdr, PGSTAT_MTYPE_SUBSCRIPTION);
msg.m_subid = subid;
- pgstat_send(&msg, sizeof(PgStat_MsgSubscriptionDrop));
+ msg.m_create = false;
+ pgstat_send(&msg, sizeof(PgStat_MsgSubscription));
}
/* ----------
@@ -3679,8 +3697,8 @@ PgstatCollectorMain(int argc, char *argv[])
pgstat_recv_disconnect(&msg.msg_disconnect, len);
break;
- case PGSTAT_MTYPE_SUBSCRIPTIONDROP:
- pgstat_recv_subscription_drop(&msg.msg_subscriptiondrop, len);
+ case PGSTAT_MTYPE_SUBSCRIPTION:
+ pgstat_recv_subscription(&msg.msg_subscription, len);
break;
case PGSTAT_MTYPE_SUBSCRIPTIONERROR:
@@ -6053,21 +6071,26 @@ pgstat_recv_funcpurge(PgStat_MsgFuncpurge *msg, int len)
}
/* ----------
- * pgstat_recv_subscription_drop() -
+ * pgstat_recv_subscription() -
*
- * Process a SUBSCRIPTIONDROP message.
+ * Process a SUBSCRIPTION message.
* ----------
*/
static void
-pgstat_recv_subscription_drop(PgStat_MsgSubscriptionDrop *msg, int len)
+pgstat_recv_subscription(PgStat_MsgSubscription *msg, int len)
{
- /* Return if we don't have replication subscription statistics */
- if (subscriptionStatHash == NULL)
- return;
+ if (msg->m_create)
+ pgstat_get_subscription_entry(msg->m_subid, true);
+ else
+ {
+ /* Return if we don't have replication subscription statistics */
+ if (subscriptionStatHash == NULL)
+ return;
- /* Remove from hashtable if present; we don't care if it's not */
- (void) hash_search(subscriptionStatHash, (void *) &(msg->m_subid),
- HASH_REMOVE, NULL);
+ /* Remove from hashtable if present; we don't care if it's not */
+ (void) hash_search(subscriptionStatHash, (void *) &(msg->m_subid),
+ HASH_REMOVE, NULL);
+ }
}
/* ----------
diff --git a/src/include/pgstat.h b/src/include/pgstat.h
index be2f7e2bcc..d897952b56 100644
--- a/src/include/pgstat.h
+++ b/src/include/pgstat.h
@@ -85,7 +85,7 @@ typedef enum StatMsgType
PGSTAT_MTYPE_REPLSLOT,
PGSTAT_MTYPE_CONNECT,
PGSTAT_MTYPE_DISCONNECT,
- PGSTAT_MTYPE_SUBSCRIPTIONDROP,
+ PGSTAT_MTYPE_SUBSCRIPTION,
PGSTAT_MTYPE_SUBSCRIPTIONERROR,
} StatMsgType;
@@ -554,15 +554,17 @@ typedef struct PgStat_MsgReplSlot
} PgStat_MsgReplSlot;
/* ----------
- * PgStat_MsgSubscriptionDrop Sent by the backend and autovacuum to tell the
- * collector about the dead subscription.
+ * PgStat_MsgSubscription Sent by the backend and autovacuum to tell the
+ * collector about creating/dropping the subscription.
* ----------
*/
-typedef struct PgStat_MsgSubscriptionDrop
+typedef struct PgStat_MsgSubscription
{
PgStat_MsgHdr m_hdr;
Oid m_subid;
-} PgStat_MsgSubscriptionDrop;
+ bool m_create; /* true if creating the subscription and false
+ * if dropping the subscription. */
+} PgStat_MsgSubscription;
/* ----------
* PgStat_MsgSubscriptionError Sent by the apply worker or the table sync
@@ -755,8 +757,8 @@ typedef union PgStat_Msg
PgStat_MsgReplSlot msg_replslot;
PgStat_MsgConnect msg_connect;
PgStat_MsgDisconnect msg_disconnect;
+ PgStat_MsgSubscription msg_subscription;
PgStat_MsgSubscriptionError msg_subscriptionerror;
- PgStat_MsgSubscriptionDrop msg_subscriptiondrop;
} PgStat_Msg;
@@ -1093,8 +1095,9 @@ extern void pgstat_report_checksum_failure(void);
extern void pgstat_report_replslot(const PgStat_StatReplSlotEntry *repSlotStat);
extern void pgstat_report_replslot_create(const char *slotname);
extern void pgstat_report_replslot_drop(const char *slotname);
-extern void pgstat_report_subscription_error(Oid subid, bool is_apply_error);
+extern void pgstat_report_subscription_create(Oid subid);
extern void pgstat_report_subscription_drop(Oid subid);
+extern void pgstat_report_subscription_error(Oid subid, bool is_apply_error);
extern void pgstat_initialize(void);
diff --git a/src/tools/pgindent/typedefs.list b/src/tools/pgindent/typedefs.list
index eaf3e7a8d4..9cbef71895 100644
--- a/src/tools/pgindent/typedefs.list
+++ b/src/tools/pgindent/typedefs.list
@@ -1945,7 +1945,7 @@ PgStat_MsgResetsinglecounter
PgStat_MsgResetslrucounter
PgStat_MsgResetsubcounter
PgStat_MsgSLRU
-PgStat_MsgSubscriptionDrop
+PgStat_MsgSubscription
PgStat_MsgSubscriptionError
PgStat_MsgTabpurge
PgStat_MsgTabstat
