Horton

RE: Logical replication timeout problem

Wei Wang (Fujitsu) <wangw.fnst@fujitsu.com>

View thread View on postgr.es
From: "wangw.fnst@fujitsu.com" <wangw.fnst@fujitsu.com>
To: Amit Kapila <amit.kapila16@gmail.com>, Peter Smith <smithpb2250@gmail.com>
Cc: Ashutosh Bapat <ashutosh.bapat.oss@gmail.com>, Fabrice Chapuis <fabrice636861@gmail.com>, Euler Taveira <euler@eulerto.com>, Masahiko Sawada <sawada.mshk@gmail.com>, "Hayato Kuroda (Fujitsu)" <kuroda.hayato@fujitsu.com>, Simon Riggs <simon.riggs@enterprisedb.com>, Petr Jelinek <petr.jelinek@enterprisedb.com>, "tanghy.fnst@fujitsu.com" <tanghy.fnst@fujitsu.com>, PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>, Ajin Cherian <itsajin@gmail.com>
Date: 2023-01-20T07:18:21Z
Lists: pgsql-hackers 
On Fri, Jan 20, 2023 at 12:35 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
> On Fri, Jan 20, 2023 at 7:40 AM Peter Smith <smithpb2250@gmail.com> wrote:
> >
> > Here are some review comments for patch v3-0001.
> >
> > ======
> > src/backend/replication/logical/logical.c
> >
> > 3. forward declaration
> >
> > +/* update progress callback */
> > +static void update_progress_cb_wrapper(ReorderBuffer *cache,
> > +    ReorderBufferTXN *txn,
> > +    ReorderBufferChange *change);
> >
> > I felt this function wrapper name was a bit misleading... AFAIK every
> > other wrapper really does just wrap their respective functions. But
> > this one seems a bit different because it calls the wrapped function
> > ONLY if some threshold is exceeded. IMO maybe this function could have
> > some name that conveys this better:
> >
> > e.g. update_progress_cb_wrapper_with_threshold
> >
> 
> I am wondering whether it would be better to move the threshold logic
> to the caller. Previously this logic was inside the function because
> it was being invoked from multiple places but now that won't be the
> case. Also, then your concern about the name would also be addressed.

Agree. Moved the threshold logic to the function ReorderBufferProcessTXN.

> >
> > ~
> >
> > 7b.
> > Would it be neater to just call OutputPluginUpdateProgress here instead?
> >
> > e.g.
> > BEFORE
> > ctx->update_progress(ctx, ctx->write_location, ctx->write_xid, false);
> > AFTER
> > OutputPluginUpdateProgress(ctx, false);
> >
> 
> We already check whether ctx->update_progress is defined or not which
> is the only extra job done by OutputPluginUpdateProgress but probably
> we can consolidate the checks and directly invoke
> OutputPluginUpdateProgress.

Changed. Invoke the function OutputPluginUpdateProgress directly in the new
callback.

Regards,
Wang Wei

Commits

  1. Fix the logical replication timeout during large DDLs.

  2. Fix the logical replication timeout during large transactions.

  3. Rethink the delay-checkpoint-end mechanism in the back-branches.

  4. Revert "Logical decoding of sequences"

  5. Skip empty transactions for logical replication.

  6. Allow specifying column lists for logical replication

  7. Add decoding of sequences to built-in replication

  8. Fix ABI break introduced by commit 4daa140a2f.

  9. Lag tracking for logical replication