public inbox for [email protected]  

help / color / mirror / Atom feed

From: Amit Kapila <[email protected]>
To: Andres Freund <[email protected]>
Cc: Alvaro Herrera <[email protected]>
Cc: Chao Li <[email protected]>
Cc: [email protected]
Cc: Michael Paquier <[email protected]>
Subject: Re: pgstat include expansion
Date: Tue, 17 Feb 2026 10:15:07 +0530
Message-ID: <CAA4eK1JoZAfVkEi3W1z_p72At1s89W7Nu4h4PNyqNbuF7DsJLQ@mail.gmail.com> (raw)
In-Reply-To: <xdnnfrmte2xoddap7kjfyho5jx6ukjo4jzbj7grtu7d6vxnsci@ghjyaealg22u>
References: <CAA4eK1+ZOXJotnk+d4mxHFP_i2SEWGTdyJgkQadOCvMgEcPGVg@mail.gmail.com>
	<[email protected]>
	<xdnnfrmte2xoddap7kjfyho5jx6ukjo4jzbj7grtu7d6vxnsci@ghjyaealg22u>

On Mon, Feb 16, 2026 at 11:20 PM Andres Freund <[email protected]> wrote:
>
> I think we should simply not pass the worker type. The only reason the worker
> type passed is that that is used as a proxy for what kind of error occurred:
>
>
> /*
>  * Report a subscription error.
>  */
> void
> pgstat_report_subscription_error(Oid subid, int wtype)
> {
>     PgStat_EntryRef *entry_ref;
>     PgStat_BackendSubEntry *pending;
>
>     entry_ref = pgstat_prep_pending_entry(PGSTAT_KIND_SUBSCRIPTION,
>                                           InvalidOid, subid, NULL);
>     pending = entry_ref->pending;
>
>     switch ((LogicalRepWorkerType) wtype)
>     {
>         case WORKERTYPE_APPLY:
>             pending->apply_error_count++;
>             break;
>
>         case WORKERTYPE_SEQUENCESYNC:
>             pending->sync_seq_error_count++;
>             break;
>
>         case WORKERTYPE_TABLESYNC:
>             pending->sync_table_error_count++;
>             break;
>
>         default:
>             /* Should never happen. */
>             Assert(0);
>             break;
>     }
> }
>
>
> It doesn't seem like the right thing to have pgstat_subscription.c translate
> the worker type to the concrete error this way. Afaict each of the callsites
> of pgstat_report_subscription_error() actually knows what kind of error its
> reporting, but just uses the worker type to do so.
>
> I'd either change the signature to have one argument for each of the error
> types, i.e.
>     pgstat_report_subscription_error(int subid,
>                                      bool apply_error,
>                                      bool sequencesync_error,
>                                      bool_tablesync_error);
>
> or split the function into three, and have
>     pgstat_report_subscription_{apply,sequence,tablesync}(int subid);
>

Good idea. +1 for the second approach to split the function. We can
name them as pgstat_report_subscription_apply_error(int subid),
pgstat_report_subscription_sequence_error(int subid),
pgstat_report_subscription_tablesync_error(int subid).

With Regards,
Amit Kapila.

view thread (4+ messages)

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: pgstat include expansion
  In-Reply-To: <CAA4eK1JoZAfVkEi3W1z_p72At1s89W7Nu4h4PNyqNbuF7DsJLQ@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