Horton

Thread

Commits

  1. Split up pgfdw_report_error so that we can mark it pg_noreturn.

  1. Making pgfdw_report_error statically analyzable

    Tom Lane <tgl@sss.pgh.pa.us> — 2025-07-28T14:54:51Z

    In the wake of commits 7d8f59577+80aa9848b, Coverity has begun
bleating like this:

*** CID 1659393:         Memory - illegal accesses  (USE_AFTER_FREE)
/srv/coverity/git/pgsql-git/postgresql/contrib/postgres_fdw/postgres_fdw.c: 4930             in postgresAnalyzeForeignTable()
4924     	res = pgfdw_exec_query(conn, sql.data, NULL);
4925     	if (PQresultStatus(res) != PGRES_TUPLES_OK)
4926     		pgfdw_report_error(ERROR, res, conn, sql.data);
4927     
4928     	if (PQntuples(res) != 1 || PQnfields(res) != 1)
4929     		elog(ERROR, "unexpected result from deparseAnalyzeSizeSql query");
>>>     CID 1659393:         Memory - illegal accesses  (USE_AFTER_FREE)
>>>     Calling "libpqsrv_PQgetvalue" dereferences freed pointer "res->res".
4930     	*totalpages = strtoul(PQgetvalue(res, 0, 0), NULL, 10);
4931     	PQclear(res);
4932     
4933     	ReleaseConnection(conn);
4934     
4935     	return true;

(and similarly in other places in postgres_fdw).  Now, this complaint
is nonsense because we won't get past the PQresultStatus and
pgfdw_report_error bit if we don't have a fully valid PGresult.
Evidently, Coverity is not managing to figure out that
pgfdw_report_error(ERROR, ...) doesn't return.  That's a bit
surprising because it normally does fairly deep interprocedural
analysis, but the conclusion is hard to avoid.  Certainly, any
C compiler that doesn't do deep LTO is not going to figure it
out either.  I think we have previously just dismissed similar
Coverity complaints, but it seems like we ought to try to fix
it properly this time.

I can see two, or two-and-a-half, ways to do that:

1. Wrap pgfdw_report_error in a macro that makes use of pg_unreachable
in a way similar to what we've done for elog/ereport.  The argument
for this is that we needn't touch the 30-or-so pgfdw_report_error
call sites; the argument against is that those elog/ereport macros
are messy, compiler-dependent, and keep needing changes.

2a. Split pgfdw_report_error into two functions, say
pgfdw_report_error() that hard-wires elevel as ERROR and is
labeled noreturn, and pgfdw_report_noerror() that has an
elevel argument that it asserts is less than ERROR.

2b. As 2a except the two functions are pgfdw_report_error()
and pgfdw_report_warning(), both with hard-wired elevel values.
This'd be sufficient right now, but it's plausible that this path
would lead to needing pgfdw_report_log() and some other variants
in future.

The argument for 2a/2b is that they're really simple and are
unlikely to break in the future.  The argument against is that
the changes would create back-patching hazards --- but maybe
it'd be reasonable to back-patch the changes to forestall that.

I'm kind of leaning to doing 2a and back-patching it, but
that's certainly a judgment call.  Anyone want to argue
for a different approach?

			regards, tom lane

  2. Re: Making pgfdw_report_error statically analyzable

    Robert Haas <robertmhaas@gmail.com> — 2025-07-28T20:01:51Z

    On Mon, Jul 28, 2025 at 10:55 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> I can see two, or two-and-a-half, ways to do that:
>
> 1. Wrap pgfdw_report_error in a macro that makes use of pg_unreachable
> in a way similar to what we've done for elog/ereport.  The argument
> for this is that we needn't touch the 30-or-so pgfdw_report_error
> call sites; the argument against is that those elog/ereport macros
> are messy, compiler-dependent, and keep needing changes.
>
> 2a. Split pgfdw_report_error into two functions, say
> pgfdw_report_error() that hard-wires elevel as ERROR and is
> labeled noreturn, and pgfdw_report_noerror() that has an
> elevel argument that it asserts is less than ERROR.
>
> 2b. As 2a except the two functions are pgfdw_report_error()
> and pgfdw_report_warning(), both with hard-wired elevel values.
> This'd be sufficient right now, but it's plausible that this path
> would lead to needing pgfdw_report_log() and some other variants
> in future.

I would be fine with any of these, but my order of preference would
probably be #2b-#1-#2a i.e. I like your most-preferred alternative
least. However, it's a very mild preference so I am more than fine if
you want to just go ahead and do #2a as you proposed.

-- 
Robert Haas
EDB: http://www.enterprisedb.com

  3. Re: Making pgfdw_report_error statically analyzable

    Tom Lane <tgl@sss.pgh.pa.us> — 2025-07-28T20:08:03Z

    Robert Haas <robertmhaas@gmail.com> writes:
> On Mon, Jul 28, 2025 at 10:55 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> 2a. Split pgfdw_report_error into two functions, say
>> pgfdw_report_error() that hard-wires elevel as ERROR and is
>> labeled noreturn, and pgfdw_report_noerror() that has an
>> elevel argument that it asserts is less than ERROR.
>> 
>> 2b. As 2a except the two functions are pgfdw_report_error()
>> and pgfdw_report_warning(), both with hard-wired elevel values.
>> This'd be sufficient right now, but it's plausible that this path
>> would lead to needing pgfdw_report_log() and some other variants
>> in future.

> I would be fine with any of these, but my order of preference would
> probably be #2b-#1-#2a i.e. I like your most-preferred alternative
> least. However, it's a very mild preference so I am more than fine if
> you want to just go ahead and do #2a as you proposed.

The difference between 2a and 2b is just cosmetic really, so
I'm fine to go with 2b if that's the consensus.

			regards, tom lane

  4. Re: Making pgfdw_report_error statically analyzable

    Álvaro Herrera <alvherre@kurilemu.de> — 2025-07-28T21:25:58Z

    On 2025-Jul-28, Tom Lane wrote:

> 2b. As 2a except the two functions are pgfdw_report_error()
> and pgfdw_report_warning(), both with hard-wired elevel values.
> This'd be sufficient right now, but it's plausible that this path
> would lead to needing pgfdw_report_log() and some other variants
> in future.

Hmm, what about 2c. having pgfdw_report_error() with hardcoded elevel,
but complement it with pgfdw_report() that takes the elevel as argument,
asserting that it's less than ERROR?  Then the calls look like
  pgfdw_report(WARNING, "doo dee");

which makes sense IMO and we don't have to worry about the future.

-- 
Álvaro Herrera        Breisgau, Deutschland  —  https://www.EnterpriseDB.com/
"La experiencia nos dice que el hombre peló millones de veces las patatas,
pero era forzoso admitir la posibilidad de que en un caso entre millones,
las patatas pelarían al hombre" (Ijon Tichy)

  5. Re: Making pgfdw_report_error statically analyzable

    Tom Lane <tgl@sss.pgh.pa.us> — 2025-07-28T21:30:17Z

    =?utf-8?Q?=C3=81lvaro?= Herrera <alvherre@kurilemu.de> writes:
> Hmm, what about 2c. having pgfdw_report_error() with hardcoded elevel,
> but complement it with pgfdw_report() that takes the elevel as argument,
> asserting that it's less than ERROR?  Then the calls look like
>   pgfdw_report(WARNING, "doo dee");

> which makes sense IMO and we don't have to worry about the future.

This is the same as my 2a except for the choice of function name.
I'd be fine with it, but Robert didn't like 2a.

			regards, tom lane

  6. Re: Making pgfdw_report_error statically analyzable

    Robert Haas <robertmhaas@gmail.com> — 2025-07-29T13:36:11Z

    On Mon, Jul 28, 2025 at 5:30 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> =?utf-8?Q?=C3=81lvaro?= Herrera <alvherre@kurilemu.de> writes:
> > Hmm, what about 2c. having pgfdw_report_error() with hardcoded elevel,
> > but complement it with pgfdw_report() that takes the elevel as argument,
> > asserting that it's less than ERROR?  Then the calls look like
> >   pgfdw_report(WARNING, "doo dee");
>
> > which makes sense IMO and we don't have to worry about the future.
>
> This is the same as my 2a except for the choice of function name.
> I'd be fine with it, but Robert didn't like 2a.

I think I like this a little better than your 2a. It's not a big deal, anyway.

-- 
Robert Haas
EDB: http://www.enterprisedb.com

  7. Re: Making pgfdw_report_error statically analyzable

    Tom Lane <tgl@sss.pgh.pa.us> — 2025-07-29T13:45:24Z

    Robert Haas <robertmhaas@gmail.com> writes:
> On Mon, Jul 28, 2025 at 5:30 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> =?utf-8?Q?=C3=81lvaro?= Herrera <alvherre@kurilemu.de> writes:
>>> Hmm, what about 2c. having pgfdw_report_error() with hardcoded elevel,
>>> but complement it with pgfdw_report() that takes the elevel as argument,
>>> asserting that it's less than ERROR?  Then the calls look like
>>> pgfdw_report(WARNING, "doo dee");
>>> which makes sense IMO and we don't have to worry about the future.

>> This is the same as my 2a except for the choice of function name.
>> I'd be fine with it, but Robert didn't like 2a.

> I think I like this a little better than your 2a. It's not a big deal, anyway.

I'll run with Alvaro's suggestion, then.

			regards, tom lane

  8. Re: Making pgfdw_report_error statically analyzable

    Tom Lane <tgl@sss.pgh.pa.us> — 2025-07-29T14:37:41Z

    I wrote:
> I'll run with Alvaro's suggestion, then.

And done.  In the event I realized that there's no point in
back-patching this, because 80aa9848b already changed all
the call sites of pgfdw_report_error, so we're already going
to have issues back-patching any fixes that touch those.

			regards, tom lane