On Fri, Nov 5, 2021 at 12:57 AM vignesh C <vignes...@gmail.com> wrote: > > On Fri, Oct 29, 2021 at 10:55 AM Masahiko Sawada <sawada.m...@gmail.com> > wrote: > > > > On Thu, Oct 28, 2021 at 7:40 PM Amit Kapila <amit.kapil...@gmail.com> wrote: > > > > > > On Thu, Oct 28, 2021 at 10:36 AM Masahiko Sawada <sawada.m...@gmail.com> > > > wrote: > > > > > > > > On Wed, Oct 27, 2021 at 7:02 PM Amit Kapila <amit.kapil...@gmail.com> > > > > wrote: > > > > > > > > > > On Thu, Oct 21, 2021 at 10:29 AM Masahiko Sawada > > > > > <sawada.m...@gmail.com> wrote: > > > > > > > > > > > > > > > > > > I've attached updated patches. > > > > > > > > Thank you for the comments! > > > > > > > > > > > > > > Few comments: > > > > > ============== > > > > > 1. Is the patch cleaning tablesync error entries except via vacuum? If > > > > > not, can't we send a message to remove tablesync errors once tablesync > > > > > is successful (say when we reset skip_xid or when tablesync is > > > > > finished) or when we drop subscription? I think the same applies to > > > > > apply worker. I think we may want to track it in some way whether an > > > > > error has occurred before sending the message but relying completely > > > > > on a vacuum might be the recipe of bloat. I think in the case of a > > > > > drop subscription we can simply send the message as that is not a > > > > > frequent operation. I might be missing something here because in the > > > > > tests after drop subscription you are expecting the entries from the > > > > > view to get cleared > > > > > > > > Yes, I think we can have tablesync worker send a message to drop stats > > > > once tablesync is successful. But if we do that also when dropping a > > > > subscription, I think we need to do that only the transaction is > > > > committed since we can drop a subscription that doesn't have a > > > > replication slot and rollback the transaction. Probably we can send > > > > the message only when the subscritpion does have a replication slot. > > > > > > > > > > Right. And probably for apply worker after updating skip xid. > > > > I'm not sure it's better to drop apply worker stats after resetting > > skip xid (i.g., after skipping the transaction). Since the view is a > > cumulative view and has last_error_time, I thought we can have the > > apply worker stats until the subscription gets dropped. Since the > > error reporting message could get lost, no entry in the view doesn’t > > mean the worker doesn’t face an issue. > > > > > > > > > In other cases, we can remember the subscriptions being dropped and > > > > send the message to drop the statistics of them after committing the > > > > transaction but I’m not sure it’s worth having it. > > > > > > > > > > Yeah, let's not go to that extent. I think in most cases subscriptions > > > will have corresponding slots. > > > > Agreed. > > > > > > > > FWIW, we completely > > > > rely on pg_stat_vacuum_stats() for cleaning up the dead tables and > > > > functions. And we don't expect there are many subscriptions on the > > > > database. > > > > > > > > > > True, but we do send it for the database, so let's do it for the cases > > > you explained in the first paragraph. > > > > Agreed. > > > > I've attached a new version patch. Since the syntax of skipping > > transaction id is under the discussion I've attached only the error > > reporting patch for now. > > Thanks for the updated patch, few comments: > 1) This check and return can be moved above CreateTemplateTupleDesc so > that the tuple descriptor need not be created if there is no worker > statistics > + BlessTupleDesc(tupdesc); > + > + /* Get subscription worker stats */ > + wentry = pgstat_fetch_subworker(subid, subrelid); > + > + /* Return NULL if there is no worker statistics */ > + if (wentry == NULL) > + PG_RETURN_NULL(); > + > + /* Initialise values and NULL flags arrays */ > + MemSet(values, 0, sizeof(values)); > + MemSet(nulls, 0, sizeof(nulls)); > > 2) "NULL for the main apply worker" is mentioned as "null for the main > apply worker" in case of pg_stat_subscription view, we can mention it > similarly. > + <para> > + OID of the relation that the worker is synchronizing; NULL for the > + main apply worker > + </para></entry> > > 3) Variable assignment can be done during declaration and this the > assignment can be removed > + i = 0; > + /* subid */ > + values[i++] = ObjectIdGetDatum(subid); > > 4) I noticed that the worker error is still present when queried from > pg_stat_subscription_workers even after conflict is resolved in the > subscriber and the worker proceeds with applying the other > transactions, should this be documented somewhere? > > 5) This needs to be aligned, the columns in select have used TAB, we > should align it using spaces. > +CREATE VIEW pg_stat_subscription_workers AS > + SELECT > + w.subid, > + s.subname, > + w.subrelid, > + w.relid, > + w.command, > + w.xid, > + w.error_count, > + w.error_message, > + w.last_error_time, > + w.stats_reset >
Thank you for the comments! These comments are incorporated into the latest (v20) patch I just submitted[1]. Regards, [1] https://www.postgresql.org/message-id/CAD21AoAT42mhcqeB1jPfRL1%2BEUHbZk8MMY_fBgsyZvJeKNpG%2Bw%40mail.gmail.com -- Masahiko Sawada EDB: https://www.enterprisedb.com/
