Horton

Re: Logical replication timeout problem

Peter Smith <smithpb2250@gmail.com>

View thread View on postgr.es
From: Peter Smith <smithpb2250@gmail.com>
To: Amit Kapila <amit.kapila16@gmail.com>
Cc: "wangw.fnst@fujitsu.com" <wangw.fnst@fujitsu.com>, 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-20T05:28:09Z
Lists: pgsql-hackers 
On Fri, Jan 20, 2023 at 3: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.
>
> >
> > ~
> >
> > 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.
>

Yes, I saw that, but I thought it was better to keep the early exit
from update_progress_cb_wrapper, so incurring just one additional
boolean check for every 100 changes was not anything to worry about.

------
Kind Regards,
Peter Smith.
Fujitsu Australia.

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