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.
Attachments:
v1-0001-Remove-an-unused-function-GetWalRcvWriteRecPtr.patchapplication/octet-stream; name=v1-0001-Remove-an-unused-function-GetWalRcvWriteRecPtr.patchDownload
From ec566328b561470a40bee9545e530d060b8ce243 Mon Sep 17 00:00:00 2001
From: Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com>
Date: Sat, 26 Mar 2022 05:15:36 +0000
Subject: [PATCH v1] Remove an unused function GetWalRcvWriteRecPtr
---
src/backend/replication/walreceiverfuncs.c | 12 ------------
src/include/replication/walreceiver.h | 1 -
2 files changed, 13 deletions(-)
diff --git a/src/backend/replication/walreceiverfuncs.c b/src/backend/replication/walreceiverfuncs.c
index 90798b9d53..a921460216 100644
--- a/src/backend/replication/walreceiverfuncs.c
+++ b/src/backend/replication/walreceiverfuncs.c
@@ -345,18 +345,6 @@ GetWalRcvFlushRecPtr(XLogRecPtr *latestChunkStart, TimeLineID *receiveTLI)
return recptr;
}
-/*
- * Returns the last+1 byte position that walreceiver has written.
- * This returns a recently written value without taking a lock.
- */
-XLogRecPtr
-GetWalRcvWriteRecPtr(void)
-{
- WalRcvData *walrcv = WalRcv;
-
- return pg_atomic_read_u64(&walrcv->writtenUpto);
-}
-
/*
* Returns the replication apply delay in ms or -1
* if the apply delay info is not available
diff --git a/src/include/replication/walreceiver.h b/src/include/replication/walreceiver.h
index 92f73a55b8..a6b15b2df7 100644
--- a/src/include/replication/walreceiver.h
+++ b/src/include/replication/walreceiver.h
@@ -464,7 +464,6 @@ extern void RequestXLogStreaming(TimeLineID tli, XLogRecPtr recptr,
const char *conninfo, const char *slotname,
bool create_temp_slot);
extern XLogRecPtr GetWalRcvFlushRecPtr(XLogRecPtr *latestChunkStart, TimeLineID *receiveTLI);
-extern XLogRecPtr GetWalRcvWriteRecPtr(void);
extern int GetReplicationApplyDelay(void);
extern int GetReplicationTransferLatency(void);
extern void WalRcvForceReply(void);
--
2.25.1
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