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

On Mon, Aug 30, 2021 at 11:59 AM Michael Paquier <michael(at)paquier(dot)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.