Обсуждение: Remove an unused function GetWalRcvWriteRecPtr

Hi,

The function GetWalRcvWriteRecPtr isn't being used anywhere, however
pg_atomic_read_u64(&walrcv->writtenUpto); (reading writtenUpto without
spinlock) is being used directly in pg_stat_get_wal_receiver
walreceiver.c. We either make use of the function instead of
pg_atomic_read_u64(&walrcv->writtenUpto); or remove it. Since there's
only one function using walrcv->writtenUpto right now, I prefer to
remove the function to save some LOC (13).

Attaching patch. Thoughts?

Regards,
Bharath Rupireddy.

On Sat, Mar 26, 2022 at 10:51:15AM +0530, Bharath Rupireddy wrote:
> The function GetWalRcvWriteRecPtr isn't being used anywhere, however
> pg_atomic_read_u64(&walrcv->writtenUpto); (reading writtenUpto without
> spinlock) is being used directly in pg_stat_get_wal_receiver
> walreceiver.c. We either make use of the function instead of
> pg_atomic_read_u64(&walrcv->writtenUpto); or remove it. Since there's
> only one function using walrcv->writtenUpto right now, I prefer to
> remove the function to save some LOC (13).
>
> Attaching patch. Thoughts?

This could be used by some external module, no?
--
Michael

On Sat, Mar 26, 2022 at 02:52:29PM +0900, Michael Paquier wrote:
> On Sat, Mar 26, 2022 at 10:51:15AM +0530, Bharath Rupireddy wrote:
> > The function GetWalRcvWriteRecPtr isn't being used anywhere, however
> > pg_atomic_read_u64(&walrcv->writtenUpto); (reading writtenUpto without
> > spinlock) is being used directly in pg_stat_get_wal_receiver
> > walreceiver.c. We either make use of the function instead of
> > pg_atomic_read_u64(&walrcv->writtenUpto); or remove it. Since there's
> > only one function using walrcv->writtenUpto right now, I prefer to
> > remove the function to save some LOC (13).
> > 
> > Attaching patch. Thoughts?
> 
> This could be used by some external module, no?

Maybe, but WalRcv is exposed so if an external module needs it it could still
maintain its own version of GetWalRcvWriteRecPtr.

On Sat, Mar 26, 2022 at 12:55 PM Julien Rouhaud <rjuju123@gmail.com> wrote:
>
> On Sat, Mar 26, 2022 at 02:52:29PM +0900, Michael Paquier wrote:
> > On Sat, Mar 26, 2022 at 10:51:15AM +0530, Bharath Rupireddy wrote:
> > > The function GetWalRcvWriteRecPtr isn't being used anywhere, however
> > > pg_atomic_read_u64(&walrcv->writtenUpto); (reading writtenUpto without
> > > spinlock) is being used directly in pg_stat_get_wal_receiver
> > > walreceiver.c. We either make use of the function instead of
> > > pg_atomic_read_u64(&walrcv->writtenUpto); or remove it. Since there's
> > > only one function using walrcv->writtenUpto right now, I prefer to
> > > remove the function to save some LOC (13).
> > >
> > > Attaching patch. Thoughts?
> >
> > This could be used by some external module, no?
>
> Maybe, but WalRcv is exposed so if an external module needs it it could still
> maintain its own version of GetWalRcvWriteRecPtr.

Yes. And the core extensions aren't using GetWalRcvWriteRecPtr. IMO,
let's not maintain that function.

Regards,
Bharath Rupireddy.

Julien Rouhaud <rjuju123@gmail.com> writes:
> On Sat, Mar 26, 2022 at 02:52:29PM +0900, Michael Paquier wrote:
>> This could be used by some external module, no?

> Maybe, but WalRcv is exposed so if an external module needs it it could still
> maintain its own version of GetWalRcvWriteRecPtr.

We'd need to mark WalRcv as PGDLLIMPORT if we want to take that
seriously.

            regards, tom lane

Hi,

On 2022-03-26 10:51:15 +0530, Bharath Rupireddy wrote:
> The function GetWalRcvWriteRecPtr isn't being used anywhere, however
> pg_atomic_read_u64(&walrcv->writtenUpto); (reading writtenUpto without
> spinlock) is being used directly in pg_stat_get_wal_receiver
> walreceiver.c. We either make use of the function instead of
> pg_atomic_read_u64(&walrcv->writtenUpto); or remove it. Since there's
> only one function using walrcv->writtenUpto right now, I prefer to
> remove the function to save some LOC (13).

-1. I think it's a perfectly reasonable function to have, it doesn't cause
architectural / maintenance issues to have it and there's several plausible
future uses for it (moving fsyncing of received WAL to different process,
optionally allowing logical decoding up to the written LSN, reporting function
for monitoring on the standby itself).

Greetings,

Andres Freund

On Sat, Mar 26, 2022 at 10:57 PM Andres Freund <andres@anarazel.de> wrote:
>
> Hi,
>
> On 2022-03-26 10:51:15 +0530, Bharath Rupireddy wrote:
> > The function GetWalRcvWriteRecPtr isn't being used anywhere, however
> > pg_atomic_read_u64(&walrcv->writtenUpto); (reading writtenUpto without
> > spinlock) is being used directly in pg_stat_get_wal_receiver
> > walreceiver.c. We either make use of the function instead of
> > pg_atomic_read_u64(&walrcv->writtenUpto); or remove it. Since there's
> > only one function using walrcv->writtenUpto right now, I prefer to
> > remove the function to save some LOC (13).
>
> -1. I think it's a perfectly reasonable function to have, it doesn't cause
> architectural / maintenance issues to have it and there's several plausible
> future uses for it (moving fsyncing of received WAL to different process,
> optionally allowing logical decoding up to the written LSN, reporting function
> for monitoring on the standby itself).

Given the use-cases that it may have in future, I can use that
function right now in pg_stat_get_wal_receiver instead of
pg_atomic_read_u64(&WalRcv->writtenUpto);

I was also thinking of a function to expose it but backed off because
it can't be used reliably for data integrity checks or do we want to
specify about this in the functions docs as well leave it to the user?
Thoughts?

    /*
     * Like flushedUpto, but advanced after writing and before flushing,
     * without the need to acquire the spin lock.  Data can be read by another
     * process up to this point, but shouldn't be used for data integrity
     * purposes.
     */
    pg_atomic_uint64 writtenUpto;

Regards,
Bharath Rupireddy.

Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com> writes:
> On Sat, Mar 26, 2022 at 10:57 PM Andres Freund <andres@anarazel.de> wrote:
>> -1. I think it's a perfectly reasonable function to have, it doesn't cause
>> architectural / maintenance issues to have it and there's several plausible
>> future uses for it (moving fsyncing of received WAL to different process,
>> optionally allowing logical decoding up to the written LSN, reporting function
>> for monitoring on the standby itself).

> Given the use-cases that it may have in future, I can use that
> function right now in pg_stat_get_wal_receiver instead of
> pg_atomic_read_u64(&WalRcv->writtenUpto);

I do not really see a reason to change anything at all here.
We have far better things to spend our (finite) time on.

            regards, tom lane