RE: logical replication restrictions

Hi,

On Thursday, August 11, 2022 7:33 PM Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
> On Tue, Aug 9, 2022 at 3:52 AM Euler Taveira <euler(at)eulerto(dot)com> wrote:
> >
> > On Wed, Aug 3, 2022, at 10:27 AM, Amit Kapila wrote:
> >
> > Your explanation makes sense to me. The other point to consider is
> > that there can be cases where we may not apply operation for the
> > transaction because of empty transactions (we don't yet skip empty
> > xacts for prepared transactions). So, won't it be better to apply the
> > delay just before we apply the first change for a transaction? Do we
> > want to apply the delay during table sync as we sometimes do need to
> > enter apply phase while doing table sync?
> >
> > I thought about the empty transactions but decided to not complicate
> > the code mainly because skipping transactions is not a code path that
> > will slow down this feature. As explained in the documentation, there
> > is no harm in delaying a transaction for more than min_apply_delay; it
> > cannot apply earlier. Having said that I decided to do nothing. I'm
> > also not sure if it deserves a comment or if this email is a possible explanation
> for this decision.
> >
>
> I don't know what makes you think it will complicate the code. But anyway
> thinking further about the way apply_delay is used at various places in the patch,
> as pointed out by Peter Smith it seems it won't work for the parallel apply
> feature where we start applying the transaction immediately after start stream.
> I was wondering why don't we apply delay after each commit of the transaction
> rather than at the begin command. We can remember if the transaction has
> made any change and if so then after commit, apply the delay. If we can do that
> then it will alleviate the concern of empty and skipped xacts as well.
I agree with this direction. I'll update this point in a subsequent patch.

> Another thing I was wondering how to determine what is a good delay time for
> tests and found that current tests in replay_delay.pl uses 3s, so should we use
> the same for apply delay tests in this patch as well?
Fixed in the patch posted in [1].

[1] - https://www.postgresql.org/message-id/TYCPR01MB8373775ECC6972289AF8CB30ED0F9%40TYCPR01MB8373.jpnprd01.prod.outlook.com

Best Regards,
Takamichi Osumi