Обсуждение: GetStandbyFlushRecPtr() : OUT param is not optional like elsewhere.

Hi,

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

Hi 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

Hello

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/

Thanks 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