Hi Man,
Your reading of the local logic is totally reasonable and correct: if PQescapeLiteral() returns NULL for any reason, the existing if (!q_role || !q_dbname) path will indeed fall back to ALL, so quoting failures are already handled.
The only thing I’m cautious about is treating “pset.db is NULL/invalid” as just another “quoting failure” case. In this completion branch we call PQescapeLiteral(pset.db, ...) before we ever reach exec_query(), so an explicit guard is about avoiding passing an unusable handle into libpq in the first place. Even if libpq were to return NULL in that situation, it’s not something I’d want to rely on implicitly.
That’s why I suggested the explicit guard: it matches the general psql style of checking !pset.db before calling libpq APIs (e.g. psql_get_variable() in src/bin/psql/common.c checks !pset.db before calling PQescapeLiteral()), and it makes the intent obviously safe. Behavior-wise it’s the same (fall back to ALL), just more defensive/clear & explicit.
Thanks,
Dharin
> That guard was actually my suggestion in review, and the reason is that it’s protecting the PQescapeLiteral() calls, not exec_query().
> In the RESET completion branch we call PQescapeLiteral(pset.db, ...) to safely quote the role/dbname before we even build the SQL string. That happens before COMPLETE_WITH_QUERY_PLUS ever reaches complete_from_query() / exec_query(). So while exec_query() does guard query execution when pset.db is null or not CONNECTION_OK, it doesn’t p> revent us from passing a null conn into PQescapeLiteral(), which could crash or otherwise misbehave.
> If we don’t have a usable connection, we can’t reliably quote and run the catalog query anyway, so falling back to a static completion like ALL seems like the safest behavior. Hence I think it's valid.
Hi,
Thank you both for clarifying my confusion. I hadn't paid much attention to `PQescapeLiteral` earlier.
I checked the function, and it should return NULL on failure.
```
q_role = PQescapeLiteral(pset.db, role, strlen(role));
q_dbname = PQescapeLiteral(pset.db, dbname, strlen(dbname));
if (!q_role || !q_dbname)
{
/* If quoting fails, just fall back to ALL */
if (q_role)
PQfreemem(q_role);
if (q_dbname)
PQfreemem(q_dbname);
COMPLETE_WITH("ALL");
}
```
When `pset.db` is NULL or for other reasons (resulting in q_role/q_dbname being NULL), `!q_role || !q_dbname` will be hit — this already meets our needs.
I wonder if this understanding is correct — what do you think?
--
Regards,
Man Zeng
www.openhalo.org
