Use more CppAsString2() in pg_amcheck.c

On 18 Oct 2024, at 11:50, Michael Paquier <michael@paquier.xyz> wrote:

pg_amcheck.c is one of these places where relkind and relpersistence
values are hardcoded in queries based on the contents of pg_class_d.h.
Similarly to other places in src/bin/, let's sprinkle some
CppAsString2() and feed to the binary the values from the header. The
patch attached does that.

I haven't studied the patch in detail (yet), nothing stood out from a quick
skim, but +1 on the concept.

--
Daniel Gustafsson

On 2024-Oct-18, Michael Paquier wrote:

pg_amcheck.c is one of these places where relkind and relpersistence
values are hardcoded in queries based on the contents of pg_class_d.h.
Similarly to other places in src/bin/, let's sprinkle some
CppAsString2() and feed to the binary the values from the header. The
patch attached does that.

Hmm, aren't you losing the single quotes? I think you would need to do

@@ -857,7 +858,7 @@ prepare_heap_command(PQExpBuffer sql, RelationInfo *rel, PGconn *conn)

appendPQExpBuffer(sql,
"\n) v WHERE c.oid = %u "
- "AND c.relpersistence != 't'",
+ "AND c.relpersistence != '%s'",
rel->reloid, CppAsString2(RELPERSISTENCE_TEMP));

in order for this to work properly, no?

--
Álvaro Herrera PostgreSQL Developer — https://www.EnterpriseDB.com/
"La vida es para el que se aventura"

On 2024-Oct-18, Alvaro Herrera wrote:

Hmm, aren't you losing the single quotes?

Ah, the defines themselves include the quotes, so this is not needed.
Sorry for the noise.

--
Álvaro Herrera 48°01'N 7°57'E — https://www.EnterpriseDB.com/
Syntax error: function hell() needs an argument.
Please choose what hell you want to involve.

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 Fri, Oct 18, 2024 at 12:22:26PM -0500, Nathan Bossart wrote:

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.

I may be missing something, of course, but I don't quite see how this
matters for this specific one. The possible paths after the temporary
persistence check involve a qual on ep.pattern_id, then an addition
based on opts.allrel. You are right that the extra whitespace after
the RELPERSISTENCE_TEMP bit makes little sense.

Removed that in the v2 attached.
--
Michael

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/

On Mon, Nov 25, 2024 at 06:25:46PM +0300, Karina Litskevich wrote:

The patch looks good to me.

Thanks for the review.

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:

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

What do you think?

Both look the same to me, TBH. :D

The result in the queries generated is also the same before and after
the patch. I see the inconsistency in style, but I'm not sure that
it's worth bothering about that. Applied v2.
--
Michael