On Mon, Sep 27, 2021 at 2:55 PM Amit Kapila <amit.kapil...@gmail.com> wrote:
>
> On Mon, Sep 27, 2021 at 11:02 AM Masahiko Sawada <sawada.m...@gmail.com>
> wrote:
> >
> > On Mon, Sep 27, 2021 at 12:50 PM Masahiko Sawada <sawada.m...@gmail.com>
> > wrote:
> > >
> > > On Mon, Sep 27, 2021 at 12:24 PM Amit Kapila <amit.kapil...@gmail.com>
> > > wrote:
> > > >
> > > > On Mon, Sep 27, 2021 at 6:21 AM Masahiko Sawada <sawada.m...@gmail.com>
> > > > wrote:
> > > > >
> > > > > On Sat, Sep 25, 2021 at 4:23 PM Amit Kapila <amit.kapil...@gmail.com>
> > > > > wrote:
> > > > > >
> > > > > > Sure, but each tablesync worker must have a separate relid. Why
> > > > > > can't
> > > > > > we have a single hash table for both apply and table sync workers
> > > > > > which are hashed by sub_id + rel_id? For apply worker, the rel_id
> > > > > > will
> > > > > > always be zero (InvalidOId) and tablesync workers will have a unique
> > > > > > OID for rel_id, so we should be able to uniquely identify each of
> > > > > > apply and table sync workers.
> > > > >
> > > > > What I imagined is to extend the subscription statistics, for
> > > > > instance, transaction stats[1]. By having a hash table for
> > > > > subscriptions, we can store those statistics into an entry of the hash
> > > > > table and we can think of subscription errors as also statistics of
> > > > > the subscription. So we can have another hash table for errors in an
> > > > > entry of the subscription hash table. For example, the subscription
> > > > > entry struct will be something like:
> > > > >
> > > > > typedef struct PgStat_StatSubEntry
> > > > > {
> > > > > Oid subid; /* hash key */
> > > > >
> > > > > HTAB *errors; /* apply and table sync errors */
> > > > >
> > > > > /* transaction stats of subscription */
> > > > > PgStat_Counter xact_commit;
> > > > > PgStat_Counter xact_commit_bytes;
> > > > > PgStat_Counter xact_error;
> > > > > PgStat_Counter xact_error_bytes;
> > > > > PgStat_Counter xact_abort;
> > > > > PgStat_Counter xact_abort_bytes;
> > > > > PgStat_Counter failure_count;
> > > > > } PgStat_StatSubEntry;
> > > > >
> > > >
> > > > I think these additional stats will be displayed via
> > > > pg_stat_subscription, right? If so, the current stats of that view are
> > > > all in-memory and are per LogicalRepWorker which means that for those
> > > > stats also we will have different entries for apply and table sync
> > > > worker. If this understanding is correct, won't it be better to
> > > > represent this as below?
> > >
> > > I was thinking that we have a different stats view for example
> > > pg_stat_subscription_xacts that has entries per subscription. But your
> > > idea seems better to me.
> >
> > I mean that showing statistics (including transaction statistics and
> > errors) per logical replication worker seems better to me, no matter
> > what view shows these statistics. I'll change the patch in that way.
> >
>I've attached updated patches that incorporate all comments I got so far. Please review them. Regards, -- Masahiko Sawada EDB: https://www.enterprisedb.com/
v15-0003-Add-skip_xid-option-to-ALTER-SUBSCRIPTION.patch
Description: Binary data
v15-0002-Add-RESET-command-to-ALTER-SUBSCRIPTION-command.patch
Description: Binary data
v15-0001-Add-a-subscription-errors-statistics-view-pg_sta.patch
Description: Binary data
