Thread: replace IDENTIFY_SYSTEM code in receivelog.c with RunIdentifySystem()

Hi,

I see a couple of improvements to receivelog.c and pg_receivewal.c:

1) ReceiveXlogStream in receivelog.c has a duplicate code to execute
IDENTIFY_SYSTEM replication command on the server which can be
replaced with RunIdentifySystem().
2) bool returning ReceiveXlogStream() in pg_receivewal.c is being used
without type-casting its return return value which might generate a
warning with some compilers. This kind of type-casting is more common
in other places in the postgres code base.

Attaching a patch to fix the above. Thoughts?

Regards,
Bharath Rupireddy.

On Mon, Aug 30, 2021 at 11:00:40AM +0530, Bharath Rupireddy wrote:
> 1) ReceiveXlogStream in receivelog.c has a duplicate code to execute
> IDENTIFY_SYSTEM replication command on the server which can be
> replaced with RunIdentifySystem().

I have looked at that.

> 2) bool returning ReceiveXlogStream() in pg_receivewal.c is being used
> without type-casting its return return value which might generate a
> warning with some compilers. This kind of type-casting is more common
> in other places in the postgres code base.

This is usually a pattern used for Coverity, to hint it that we don't
care about the error code in a given code path.  IMV, that's not
something to bother about for older code.

> Attaching a patch to fix the above. Thoughts?

The original refactoring of IDENTIFY_SYSTEM is from 0c013e08, and it
feels like I just missed ReceiveXlogStream().  What you have here is
an improvement.

+       if (!RunIdentifySystem(conn, &sysidentifier, &servertli, NULL, NULL))
        {
-           pg_log_error("could not send replication command \"%s\": %s",
-                        "IDENTIFY_SYSTEM", PQerrorMessage(conn));
-           PQclear(res);
+           pg_free(sysidentifier);
            return false;

Here you want to free sysidentifier only if it has been set, and
RunIdentifySystem() may fail before doing that, so you should assign
NULL to sysidentifier when it is declared.
--
Michael

On Mon, Aug 30, 2021 at 11:59 AM Michael Paquier <michael@paquier.xyz> wrote:
> > 2) bool returning ReceiveXlogStream() in pg_receivewal.c is being used
> > without type-casting its return return value which might generate a
> > warning with some compilers. This kind of type-casting is more common
> > in other places in the postgres code base.
>
> This is usually a pattern used for Coverity, to hint it that we don't
> care about the error code in a given code path.  IMV, that's not
> something to bother about for older code.

Shound't we fix it in master branch to keep the code in sync with
other places where we usually follow that kind of type-casting? IMO,
we should just make that change, because it isn't a major change or we
aren't going to back patch it.

> +       if (!RunIdentifySystem(conn, &sysidentifier, &servertli, NULL, NULL))
>         {
> -           pg_log_error("could not send replication command \"%s\": %s",
> -                        "IDENTIFY_SYSTEM", PQerrorMessage(conn));
> -           PQclear(res);
> +           pg_free(sysidentifier);
>             return false;
>
> Here you want to free sysidentifier only if it has been set, and
> RunIdentifySystem() may fail before doing that, so you should assign
> NULL to sysidentifier when it is declared.

Isn't the pg_free going to take care of sysidentifier being null?
    if (ptr != NULL)
        free(ptr);

Do we still need this?
if (sysidentifier)
   pg_free(sysidentifier);

IMO, let the v1 patch be as-is and not do above.

Thoughts?

Regards,
Bharath Rupireddy.

On Mon, Aug 30, 2021 at 01:01:16PM +0530, Bharath Rupireddy wrote:
> Shound't we fix it in master branch to keep the code in sync with
> other places where we usually follow that kind of type-casting? IMO,
> we should just make that change, because it isn't a major change or we
> aren't going to back patch it.

One thing is that this creates conflicts with back-branches, and
that's always annoying.  I'd be fine with changing new code for that,
though.

>> Here you want to free sysidentifier only if it has been set, and
>> RunIdentifySystem() may fail before doing that, so you should assign
>> NULL to sysidentifier when it is declared.
>
> Isn't the pg_free going to take care of sysidentifier being null?
>     if (ptr != NULL)
>         free(ptr);

It would, but you don't initialize the variable to begin with, so you
may finish with freeing a pointer that points to nothing, and crash
any code using ReceiveXlogStream() while some code paths should be
able to handle retries.  I guess that compilers would not complain
here because they cannot understand that RunIdentifySystem() may not
set up the variable before this function returns.  I have fixed this
initialization, and committed the patch.  Note that there is only one
other code path using RunIdentifySystem() with the system ID as of
pg_basebackup.c, but this one would just exit() if we fail to run the
command, so we don't need to care about freeing the system ID there.
--
Michael