Re: Use more CppAsString2() in pg_amcheck.c

On Sat, Oct 19, 2024 at 4:17 AM Michael Paquier <michael(at)paquier(dot)xyz> wrote:
> Removed that in the v2 attached.

Hi Michael,

The patch looks good to me.

I'd like to suggest discussing a little cosmetic change in the
affected lines. Compare the following.

Lines 2095-2098:

appendPQExpBuffer(&sql,
" AND c.relam = %u"
" AND c.relkind = " CppAsString2(RELKIND_INDEX) ")",
BTREE_AM_OID);

Lines 2058-2061:

appendPQExpBuffer(&sql,
" AND c.relam = %u "
"AND c.relkind = " CppAsString2(RELKIND_INDEX),
BTREE_AM_OID);

They are not identical: space before AND vs space at the end of the
previous line. I'd say that it would be better if they were
identical. Personally, I prefer the one with a space before AND.
Though we have two more similar cases in lines 1974-2001:

if (opts.allrel)
appendPQExpBuffer(&sql,
" AND c.relam = %u "
"AND c.relkind IN ("
CppAsString2(RELKIND_RELATION) ", "
CppAsString2(RELKIND_SEQUENCE) ", "
CppAsString2(RELKIND_MATVIEW) ", "
CppAsString2(RELKIND_TOASTVALUE) ") "
"AND c.relnamespace != %u",
HEAP_TABLE_AM_OID, PG_TOAST_NAMESPACE);
else
appendPQExpBuffer(&sql,
" AND c.relam IN (%u, %u)"
"AND c.relkind IN ("
CppAsString2(RELKIND_RELATION) ", "
CppAsString2(RELKIND_SEQUENCE) ", "
CppAsString2(RELKIND_MATVIEW) ", "
CppAsString2(RELKIND_TOASTVALUE) ", "
CppAsString2(RELKIND_INDEX) ") "
"AND ((c.relam = %u AND c.relkind IN ("
CppAsString2(RELKIND_RELATION) ", "
CppAsString2(RELKIND_SEQUENCE) ", "
CppAsString2(RELKIND_MATVIEW) ", "
CppAsString2(RELKIND_TOASTVALUE) ")) OR "
"(c.relam = %u AND c.relkind = "
CppAsString2(RELKIND_INDEX) "))",
HEAP_TABLE_AM_OID, BTREE_AM_OID,
HEAP_TABLE_AM_OID, BTREE_AM_OID);

And I'm not sure how they should be handled if we choose a space
before AND. Does the following still look fine?

if (opts.allrel)
appendPQExpBuffer(&sql,
" AND c.relam = %u"
" AND c.relkind IN ("
CppAsString2(RELKIND_RELATION) ", "
CppAsString2(RELKIND_SEQUENCE) ", "
CppAsString2(RELKIND_MATVIEW) ", "
CppAsString2(RELKIND_TOASTVALUE) ")"
" AND c.relnamespace != %u",
HEAP_TABLE_AM_OID, PG_TOAST_NAMESPACE);

What do you think?

Best regards,
Karina Litskevich
Postgres Professional: http://postgrespro.com/