Postgres Pro
Downloads
Home   >   mailing lists

Thread: Deduplicate min restart_lsn calculation code

Deduplicate min restart_lsn calculation code

From
Bharath Rupireddy
Date:
Hi,

It seems like the two functions ReplicationSlotsComputeRequiredLSN and
ReplicationSlotsComputeLogicalRestartLSN more or less does the same
thing which makes me optimize (saving 40 LOC) it as attached. I'm
pretty much okay if it gets rejected on the grounds that it creates a
lot of diff with the older versions and the new API may not look
nicer, still I want to give it a try.

Thoughts?

Regards,
Bharath Rupireddy.
Attachment

Re: Deduplicate min restart_lsn calculation code

From
Alvaro Herrera
Date:
On 2022-Jan-06, Bharath Rupireddy wrote:

> Hi,
> 
> It seems like the two functions ReplicationSlotsComputeRequiredLSN and
> ReplicationSlotsComputeLogicalRestartLSN more or less does the same
> thing which makes me optimize (saving 40 LOC) it as attached. I'm
> pretty much okay if it gets rejected on the grounds that it creates a
> lot of diff with the older versions and the new API may not look
> nicer, still I want to give it a try.
> 
> Thoughts?

Hmm, it seems sensible to me.  But I would not have the second boolean
argument in the new function, and instead have the caller save the
return value in a local variable to do the XLogSetReplicationSlotMinimumLSN
step separately.  Then the new function API is not so strange.

-- 
Álvaro Herrera           39°49'30"S 73°17'W  —  https://www.EnterpriseDB.com/
"Linux transformó mi computadora, de una `máquina para hacer cosas',
en un aparato realmente entretenido, sobre el cual cada día aprendo
algo nuevo" (Jaime Salinas)

Re: Deduplicate min restart_lsn calculation code

From
Bharath Rupireddy
Date:
On Thu, Jan 6, 2022 at 11:54 PM Alvaro Herrera <alvherre@alvh.no-ip.org> wrote:
>
> On 2022-Jan-06, Bharath Rupireddy wrote:
>
> > Hi,
> >
> > It seems like the two functions ReplicationSlotsComputeRequiredLSN and
> > ReplicationSlotsComputeLogicalRestartLSN more or less does the same
> > thing which makes me optimize (saving 40 LOC) it as attached. I'm
> > pretty much okay if it gets rejected on the grounds that it creates a
> > lot of diff with the older versions and the new API may not look
> > nicer, still I want to give it a try.
> >
> > Thoughts?
>
> Hmm, it seems sensible to me.  But I would not have the second boolean
> argument in the new function, and instead have the caller save the
> return value in a local variable to do the XLogSetReplicationSlotMinimumLSN
> step separately.  Then the new function API is not so strange.

Thanks for taking a look at it. Here's the v2 patch, please review.

Regards,
Bharath Rupireddy.
Attachment