Horton

Re: Skipping logical replication transactions on subscriber side

Masahiko Sawada <sawada.mshk@gmail.com>

View thread View on postgr.es
From: Masahiko Sawada <sawada.mshk@gmail.com>
To: "houzj.fnst@fujitsu.com" <houzj.fnst@fujitsu.com>
Cc: Greg Nancarrow <gregn4422@gmail.com>, "tanghy.fnst@fujitsu.com" <tanghy.fnst@fujitsu.com>, "osumi.takamichi@fujitsu.com" <osumi.takamichi@fujitsu.com>, Alexey Lesovsky <lesovsky@gmail.com>, Peter Eisentraut <peter.eisentraut@enterprisedb.com>, PostgreSQL-development <pgsql-hackers@postgresql.org>, Amit Kapila <amit.kapila16@gmail.com>
Date: 2021-09-05T13:57:28Z
Lists: pgsql-hackers 
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/

Commits

  1. Test ALIGNOF_DOUBLE==4 compatibility under ALIGNOF_DOUBLE==8.

  2. Reorder subskiplsn in pg_subscription to avoid alignment issues.

  3. Add ALTER SUBSCRIPTION ... SKIP.

  4. Optionally disable subscriptions on error.

  5. Update docs of logical replication for commit 8d74fc96db.

  6. Respect permissions within logical replication.

  7. Fix regression test failure caused by commit 8d74fc96db.

  8. Add a view to show the stats of subscription workers.

  9. Add logical change details to logical replication worker errcontext.

  10. Rename LOGICAL_REP_MSG_STREAM_END to LOGICAL_REP_MSG_STREAM_STOP.

  11. Fix typo in protocol.sgml.

  12. Remove unused argument in apply_handle_commit_internal().

  13. Fix replication of in-progress transactions in tablesync worker.

  14. Reorder pg_sequence columns to avoid alignment issue