[PATCH] Clean up property graph error messages

Overview

This thread is a small, narrowly-scoped cleanup patch against src/backend/commands/propgraphcmds.c, the command-layer implementation of SQL/PGQ property graphs (a feature added for PostgreSQL 18 by Peter Eisentraut implementing parts of the SQL:2023 Property Graph Queries standard). Despite being a minor patch, it touches on a few recurring PostgreSQL hygiene themes worth unpacking: error-message style consistency, proper use of the syscache API, and the defensive-vs-dead-code question around resource cleanup on the ERROR path.

The Core Issues

The patch addresses three small defects in the property graph DDL code:

1. Grammatical error in a user-visible error message

The existing message read "mismatching properties names", which is doubly awkward — "properties names" is ungrammatical (should be the attributive singular property), and mismatching is a less natural choice than mismatched in this context. The message is emitted from check_element_properties() when the property-name lists across a label's element definitions do not align.

2. Unreachable ReleaseSysCache() after ereport(ERROR)

In check_element_properties(), the original code had a pattern along the lines of:

ereport(ERROR, (... "mismatching properties names" ...));
ReleaseSysCache(tuple);

Because ereport(ERROR, ...) performs a longjmp out of the current execution context, any statement following it is dead code. The reviewer (Ayush) correctly notes that this is not a leak — PostgreSQL's resource-owner machinery (ResourceOwner) tracks syscache references and releases them automatically during transaction/subtransaction abort cleanup (see ResourceOwnerReleaseInternal handling of CatCache references). So the explicit post-ERROR release was simultaneously unreachable and unnecessary.

The fix is to move the ReleaseSysCache() before the ereport(ERROR) so the release actually runs on the non-error path, and relies on resource owner cleanup on the error path. This is the idiomatic pattern used throughout the backend.

3. Terminology: "vertexes" vs. "vertices"

Ayush flagged vertexes as a candidate for normalization to vertices but intentionally excluded it from the patch because vertexes is a valid (if less preferred) English plural. Peter Eisentraut confirmed he fixed this separately, citing the project's style rule of using "preferred terms" (PostgreSQL's documentation and message style guide explicitly prefers the more standard mathematical/CS term vertices).

Architectural Context: Why These Details Matter

Error-message consistency as a project norm

PostgreSQL has an explicit error message style guide. Primary error messages should be short, grammatical, and use consistent terminology. Because property graphs are a newly introduced surface in v18, getting the terminology nailed down before the feature ships is important — once released, error-message wording becomes part of the de facto API that tools, tests, and translations depend on. This is why Peter (the feature's author and committer) is attentive to this even for cosmetic-seeming fixes.

The ReleaseSysCache dead-code pattern

This is a recurring minor bug class in backend code. The syscache API (SearchSysCache* / ReleaseSysCache) pins a catalog tuple in the CatCache; the pin must be released, but the resource owner provides a safety net on error paths. The canonical pattern is:

tuple = SearchSysCache1(...);
if (!HeapTupleIsValid(tuple))
    elog(ERROR, ...);          /* no release needed; we have no pin */
/* ... use tuple ... */
if (bad_condition)
{
    ReleaseSysCache(tuple);    /* release BEFORE raising */
    ereport(ERROR, ...);
}
ReleaseSysCache(tuple);

Putting ReleaseSysCache after ereport(ERROR) is both dead code and misleading to readers — it implies the author thought control could return, which invites copy-paste propagation of the wrong pattern. Fixing it improves code clarity even though it has no runtime consequence.

Patch Evolution

• v1: Changed "mismatching properties names" → "mismatching property names"; moved ReleaseSysCache above the ereport(ERROR); updated regression expected output.
• v2: Incorporated reviewer Evan Li's suggestion to further change mismatching → mismatched, producing "mismatched property names in definition of label \"%s\"". The vertexes→vertices change was left out because Peter Eisentraut had already committed it separately.

Participant Weight

• Peter Eisentraut is the authoritative voice here: he is a core committer, the author of PostgreSQL's SQL/PGQ implementation, and maintainer of the relevant files. His quick confirmation that "we should stick to the preferred terms" and his separate handling of vertexes/vertices signals this is uncontroversial cleanup he'll shepherd in.
• Ayush Tiwari is the patch author, acting as a careful code-reader who noticed adjacent issues (dead code, terminology) but appropriately scoped the patch.
• Evan Li contributed a good ESL-sensitive grammar refinement (mismatching → mismatched, which is the more natural English choice for a past-participle adjective describing already-compared names).

Takeaways

This thread is a textbook example of pre-release polish on a new feature: no architectural stakes, but it closes three small defects (grammar, dead code, terminology) before they become frozen into a released PostgreSQL version's user-facing surface. The ReleaseSysCache-after-ERROR pattern is the most technically interesting item and worth remembering as a code-review smell in any backend C code touching the syscache.