GetStandbyFlushRecPtr() : OUT param is not optional like elsewhere.
- Jump to firstname.lastname@example.orgT11:08:17+00:00Hi, If you look at GetFlushRecPtr() function the OUT parameter for TimeLineID is optional and this is not only one, see GetWalRcvFlushRecPtr(), GetXLogReplayRecPtr(), etc. I think we have missed that for GetStandbyFlushRecPtr(), to be inlined, we should change this as follow: --- a/src/backend/replication/walsender.c +++ b/src/backend/replication/walsender.c @@ -3156,7 +3156,8 @@ GetStandbyFlushRecPtr(TimeLineID *tli) receivePtr = GetWalRcvFlushRecPtr(NULL, &receiveTLI); replayPtr = GetXLogReplayRecPtr(&replayTLI); - *tli = replayTLI; + if (tli) + *tli = replayTLI; Thoughts? -- Regards, Amul Sul EDB: http://www.enterprisedb.com
- Jump to email@example.comT13:36:16+00:00Hello On 2022-Jul-20, Amul Sul wrote: > If you look at GetFlushRecPtr() function the OUT parameter for > TimeLineID is optional and this is not only one, see > GetWalRcvFlushRecPtr(), GetXLogReplayRecPtr(), etc. > > I think we have missed that for GetStandbyFlushRecPtr(), to be > inlined, we should change this as follow: This is something we decide mostly on a case-by-case basis. There's no fixed rule that all out params have to be optional. If anything is improved by this change, let's see what it is. -- Álvaro Herrera Breisgau, Deutschland — https://www.EnterpriseDB.com/
- Jump to firstname.lastname@example.orgT04:08:17+00:00Thanks Aleksander and Álvaro for your inputs. I understand this change is not making any improvement to the current code. I was a bit concerned regarding the design and consistency of the function that exists for the server in recovery and for the server that is not in recovery. I was trying to write following snippet where I am interested only for XLogRecPtr: recPtr = RecoveryInProgress() ? GetStandbyFlushRecPtr(NULL) : GetFlushRecPtr(NULL); But I can't write this since I have to pass an argument for GetStandbyFlushRecPtr() but that is not compulsory for GetFlushRecPtr(). I agree to reject proposed changes since that is not useful immediately and have no rule for the optional argument for the similar-looking function. Regards, Amul
- Jump to email@example.comT11:35:24+00:00Hi Amul, > - *tli = replayTLI; > + if (tli) > + *tli = replayTLI; I would guess the difference here is that GetStandbyFlushRecPtr is static. It is used 3 times in walsender.c and in all cases the argument can't be NULL. So I'm not certain what we will gain from the proposed check. -- Best regards, Aleksander Alekseev