On Thu, Sep 2, 2021 at 5:41 PM houzj.fnst@fujitsu.com
<houzj.fnst@fujitsu.com> wrote:
>
> From Mon, Aug 30, 2021 3:07 PM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
> > I've attached rebased patches. 0004 patch is not the scope of this patch. It's
> > borrowed from another thread[1] to fix the assertion failure for newly added
> > tests. Please review them.
>
> Hi,
>
> I reviewed the v12-0001 patch, here are some comments:
Thank you for the comments!
>
> 1)
> --- a/src/backend/utils/error/elog.c
> +++ b/src/backend/utils/error/elog.c
> @@ -1441,7 +1441,6 @@ getinternalerrposition(void)
> return edata->internalpos;
> }
>
> -
>
> It seems a miss change in elog.c
Fixed.
>
> 2)
>
> + TupleDescInitEntry(tupdesc, (AttrNumber) 10, "stats_reset",
> + TIMESTAMPTZOID, -1, 0);
>
> The document doesn't mention the column "stats_reset".
Added.
> 3)
>
> +typedef struct PgStat_StatSubErrEntry
> +{
> + Oid subrelid; /* InvalidOid if the apply worker, otherwise
> + * the table sync worker. hash table key. */
>
> From the comments of subrelid, I think one subscription only have one apply
> worker error entry, right ? If so, I was thinking can we move the the apply
> error entry to PgStat_StatSubEntry. In that approach, we don't need to build a
> inner hash table when there are no table sync error entry.
I wanted to avoid having unnecessary error entry fields when there is
no apply worker error but there is a table sync worker error. But
after more thoughts, the apply worker is likely to raise an error than
table sync workers. So it might be better to have both
PgStat_StatSubErrEntry for the apply worker error and hash table for
table sync workers errors in PgStat_StatSubEntry.
>
> 4)
> Is it possible to add some testcases to test the subscription error entry delete ?
Do you mean the tests checking if subscription error entry is deleted
after DROP SUBSCRIPTION?
Those comments are incorporated into local branches. I'll submit the
updated patches after incorporating other comments.
Regards,
--
Masahiko Sawada
EDB: https://www.enterprisedb.com/