Thread: The use of atooid() on non-Oid results

When looking at the report in [0] an API choice in the relevant pg_upgrade code
path stood out as curious.  check_is_install_user() runs this query to ensure
that only the install user is present in the cluster:

    res = executeQueryOrDie(conn,
                            "SELECT COUNT(*) "
                            "FROM pg_catalog.pg_roles "
                            "WHERE rolname !~ '^pg_'");

The result is then verified with the following:

    if (cluster == &new_cluster && atooid(PQgetvalue(res, 0, 0)) != 1)
        pg_fatal("Only the install user can be defined in the new cluster.");

This was changed from atoi() in ee646df59 with no specific comment on why.
This is not a bug, since atooid() will do the right thing here, but it threw me
off reading the code and might well confuse others.  Is there a reason not to
change this back to atoi() for code clarity as we're not reading an Oid here?

--
Daniel Gustafsson

[0] VE1P191MB1118E9752D4EAD45205E995CD6BF9@VE1P191MB1118.EURP191.PROD.OUTLOOK.COM

Daniel Gustafsson <daniel@yesql.se> writes:
> When looking at the report in [0] an API choice in the relevant pg_upgrade code
> path stood out as curious.  check_is_install_user() runs this query to ensure
> that only the install user is present in the cluster:

>     res = executeQueryOrDie(conn,
>                             "SELECT COUNT(*) "
>                             "FROM pg_catalog.pg_roles "
>                             "WHERE rolname !~ '^pg_'");

> The result is then verified with the following:

>     if (cluster == &new_cluster && atooid(PQgetvalue(res, 0, 0)) != 1)
>         pg_fatal("Only the install user can be defined in the new cluster.");

> This was changed from atoi() in ee646df59 with no specific comment on why.
> This is not a bug, since atooid() will do the right thing here, but it threw me
> off reading the code and might well confuse others.  Is there a reason not to
> change this back to atoi() for code clarity as we're not reading an Oid here?

Hmm ... in principle, you could have more than 2^31 entries in pg_roles,
but not more than 2^32 since they all have to have distinct OIDs.  So
I can see the point of avoiding that theoretical overflow hazard.  But
personally I'd probably avoid assuming anything about how wide the COUNT()
result could be, and instead writing

    ... && strcmp(PQgetvalue(res, 0, 0), "1") != 0)

            regards, tom lane

> On 16 Mar 2023, at 15:58, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>
> Daniel Gustafsson <daniel@yesql.se> writes:
>> When looking at the report in [0] an API choice in the relevant pg_upgrade code
>> path stood out as curious.  check_is_install_user() runs this query to ensure
>> that only the install user is present in the cluster:
>
>>    res = executeQueryOrDie(conn,
>>                            "SELECT COUNT(*) "
>>                            "FROM pg_catalog.pg_roles "
>>                            "WHERE rolname !~ '^pg_'");
>
>> The result is then verified with the following:
>
>>    if (cluster == &new_cluster && atooid(PQgetvalue(res, 0, 0)) != 1)
>>        pg_fatal("Only the install user can be defined in the new cluster.");
>
>> This was changed from atoi() in ee646df59 with no specific comment on why.
>> This is not a bug, since atooid() will do the right thing here, but it threw me
>> off reading the code and might well confuse others.  Is there a reason not to
>> change this back to atoi() for code clarity as we're not reading an Oid here?
>
> Hmm ... in principle, you could have more than 2^31 entries in pg_roles,
> but not more than 2^32 since they all have to have distinct OIDs.  So
> I can see the point of avoiding that theoretical overflow hazard.  But
> personally I'd probably avoid assuming anything about how wide the COUNT()
> result could be, and instead writing
>
>     ... && strcmp(PQgetvalue(res, 0, 0), "1") != 0)

Yeah, that makes sense.  I'll go ahead with that solution instead and possibly
a brief addition to the comment.

--
Daniel Gustafsson