Thread: Re: Use more CppAsString2() in pg_amcheck.c

On Fri, Oct 18, 2024 at 06:50:50PM +0900, Michael Paquier wrote:
> -    appendPQExpBufferStr(&sql, "\nWHERE c.relpersistence != 't'");
> +    appendPQExpBufferStr(&sql, "\nWHERE c.relpersistence != "
> +                         CppAsString2(RELPERSISTENCE_TEMP) " ");

I think these whitespace changes may cause invalid statements to be
produced in some code paths.  IMHO those should be reserved for a separate
patch, anyway.

-- 
nathan

On Sat, Oct 19, 2024 at 4:17 AM Michael Paquier <michael@paquier.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/