Use more CppAsString2() in pg_amcheck.c
Hi all,
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.
Thoughts or comments?
--
Michael
Attachments:
0001-Use-more-CppAsString2-in-pg_amcheck.c.patchtext/x-diff; charset=us-asciiDownload
From 9f56c8befb5b59b6c8013c3f7d58d62e5e42c30a Mon Sep 17 00:00:00 2001
From: Michael Paquier <michael@paquier.xyz>
Date: Fri, 18 Oct 2024 18:49:45 +0900
Subject: [PATCH] Use more CppAsString2() in pg_amcheck.c
---
src/bin/pg_amcheck/pg_amcheck.c | 42 +++++++++++++++++++++++----------
1 file changed, 29 insertions(+), 13 deletions(-)
diff --git a/src/bin/pg_amcheck/pg_amcheck.c b/src/bin/pg_amcheck/pg_amcheck.c
index 0c05cf58bc..e74a0af1f8 100644
--- a/src/bin/pg_amcheck/pg_amcheck.c
+++ b/src/bin/pg_amcheck/pg_amcheck.c
@@ -16,6 +16,7 @@
#include <time.h>
#include "catalog/pg_am_d.h"
+#include "catalog/pg_class_d.h"
#include "catalog/pg_namespace_d.h"
#include "common/logging.h"
#include "common/username.h"
@@ -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 != " CppAsString2(RELPERSISTENCE_TEMP),
rel->reloid);
}
@@ -890,7 +891,7 @@ prepare_btree_command(PQExpBuffer sql, RelationInfo *rel, PGconn *conn)
"\nFROM pg_catalog.pg_class c, pg_catalog.pg_index i "
"WHERE c.oid = %u "
"AND c.oid = i.indexrelid "
- "AND c.relpersistence != 't' "
+ "AND c.relpersistence != " CppAsString2(RELPERSISTENCE_TEMP) " "
"AND i.indisready AND i.indisvalid AND i.indislive",
rel->datinfo->amcheck_schema,
(opts.heapallindexed ? "true" : "false"),
@@ -905,7 +906,7 @@ prepare_btree_command(PQExpBuffer sql, RelationInfo *rel, PGconn *conn)
"\nFROM pg_catalog.pg_class c, pg_catalog.pg_index i "
"WHERE c.oid = %u "
"AND c.oid = i.indexrelid "
- "AND c.relpersistence != 't' "
+ "AND c.relpersistence != " CppAsString2(RELPERSISTENCE_TEMP) " "
"AND i.indisready AND i.indisvalid AND i.indislive",
rel->datinfo->amcheck_schema,
(opts.heapallindexed ? "true" : "false"),
@@ -1952,7 +1953,8 @@ compile_relation_list_one_db(PGconn *conn, SimplePtrList *relations,
* until firing off the amcheck command, as the state of an index may
* change by then.
*/
- appendPQExpBufferStr(&sql, "\nWHERE c.relpersistence != 't'");
+ appendPQExpBufferStr(&sql, "\nWHERE c.relpersistence != "
+ CppAsString2(RELPERSISTENCE_TEMP) " ");
if (opts.excludetbl || opts.excludeidx || opts.excludensp)
appendPQExpBufferStr(&sql, "\nAND ep.pattern_id IS NULL");
@@ -1972,15 +1974,29 @@ compile_relation_list_one_db(PGconn *conn, SimplePtrList *relations,
if (opts.allrel)
appendPQExpBuffer(&sql,
" AND c.relam = %u "
- "AND c.relkind IN ('r', 'S', 'm', 't') "
+ "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 ('r', 'S', 'm', 't', 'i') "
- "AND ((c.relam = %u AND c.relkind IN ('r', 'S', 'm', 't')) OR "
- "(c.relam = %u AND c.relkind = 'i'))",
+ "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);
@@ -2007,7 +2023,7 @@ compile_relation_list_one_db(PGconn *conn, SimplePtrList *relations,
"\nAND (t.relname ~ ep.rel_regex OR ep.rel_regex IS NULL)"
"\nAND ep.heap_only"
"\nWHERE ep.pattern_id IS NULL"
- "\nAND t.relpersistence != 't'");
+ "\nAND t.relpersistence != " CppAsString2(RELPERSISTENCE_TEMP));
appendPQExpBufferStr(&sql,
"\n)");
}
@@ -2026,7 +2042,7 @@ compile_relation_list_one_db(PGconn *conn, SimplePtrList *relations,
"ON r.oid = i.indrelid "
"INNER JOIN pg_catalog.pg_class c "
"ON i.indexrelid = c.oid "
- "AND c.relpersistence != 't'");
+ "AND c.relpersistence != " CppAsString2(RELPERSISTENCE_TEMP));
if (opts.excludeidx || opts.excludensp)
appendPQExpBufferStr(&sql,
"\nINNER JOIN pg_catalog.pg_namespace n "
@@ -2041,7 +2057,7 @@ compile_relation_list_one_db(PGconn *conn, SimplePtrList *relations,
"\nWHERE true");
appendPQExpBuffer(&sql,
" AND c.relam = %u "
- "AND c.relkind = 'i'",
+ "AND c.relkind = " CppAsString2(RELKIND_INDEX),
BTREE_AM_OID);
if (opts.no_toast_expansion)
appendPQExpBuffer(&sql,
@@ -2065,7 +2081,7 @@ compile_relation_list_one_db(PGconn *conn, SimplePtrList *relations,
"ON t.oid = i.indrelid"
"\nINNER JOIN pg_catalog.pg_class c "
"ON i.indexrelid = c.oid "
- "AND c.relpersistence != 't'");
+ "AND c.relpersistence != " CppAsString2(RELPERSISTENCE_TEMP));
if (opts.excludeidx)
appendPQExpBufferStr(&sql,
"\nLEFT OUTER JOIN exclude_pat ep "
@@ -2078,7 +2094,7 @@ compile_relation_list_one_db(PGconn *conn, SimplePtrList *relations,
"\nWHERE true");
appendPQExpBuffer(&sql,
" AND c.relam = %u"
- " AND c.relkind = 'i')",
+ " AND c.relkind = " CppAsString2(RELKIND_INDEX) ")",
BTREE_AM_OID);
}
--
2.45.2
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
Attachments:
v2-0001-Use-more-CppAsString2-in-pg_amcheck.c.patchtext/x-diff; charset=us-asciiDownload
From 7ef323354f1710ee755876132ae719b02c3e0468 Mon Sep 17 00:00:00 2001
From: Michael Paquier <michael@paquier.xyz>
Date: Sat, 19 Oct 2024 10:15:53 +0900
Subject: [PATCH v2] Use more CppAsString2() in pg_amcheck.c
---
src/bin/pg_amcheck/pg_amcheck.c | 42 +++++++++++++++++++++++----------
1 file changed, 29 insertions(+), 13 deletions(-)
diff --git a/src/bin/pg_amcheck/pg_amcheck.c b/src/bin/pg_amcheck/pg_amcheck.c
index 0c05cf58bc..27a7d5e925 100644
--- a/src/bin/pg_amcheck/pg_amcheck.c
+++ b/src/bin/pg_amcheck/pg_amcheck.c
@@ -16,6 +16,7 @@
#include <time.h>
#include "catalog/pg_am_d.h"
+#include "catalog/pg_class_d.h"
#include "catalog/pg_namespace_d.h"
#include "common/logging.h"
#include "common/username.h"
@@ -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 != " CppAsString2(RELPERSISTENCE_TEMP),
rel->reloid);
}
@@ -890,7 +891,7 @@ prepare_btree_command(PQExpBuffer sql, RelationInfo *rel, PGconn *conn)
"\nFROM pg_catalog.pg_class c, pg_catalog.pg_index i "
"WHERE c.oid = %u "
"AND c.oid = i.indexrelid "
- "AND c.relpersistence != 't' "
+ "AND c.relpersistence != " CppAsString2(RELPERSISTENCE_TEMP) " "
"AND i.indisready AND i.indisvalid AND i.indislive",
rel->datinfo->amcheck_schema,
(opts.heapallindexed ? "true" : "false"),
@@ -905,7 +906,7 @@ prepare_btree_command(PQExpBuffer sql, RelationInfo *rel, PGconn *conn)
"\nFROM pg_catalog.pg_class c, pg_catalog.pg_index i "
"WHERE c.oid = %u "
"AND c.oid = i.indexrelid "
- "AND c.relpersistence != 't' "
+ "AND c.relpersistence != " CppAsString2(RELPERSISTENCE_TEMP) " "
"AND i.indisready AND i.indisvalid AND i.indislive",
rel->datinfo->amcheck_schema,
(opts.heapallindexed ? "true" : "false"),
@@ -1952,7 +1953,8 @@ compile_relation_list_one_db(PGconn *conn, SimplePtrList *relations,
* until firing off the amcheck command, as the state of an index may
* change by then.
*/
- appendPQExpBufferStr(&sql, "\nWHERE c.relpersistence != 't'");
+ appendPQExpBufferStr(&sql, "\nWHERE c.relpersistence != "
+ CppAsString2(RELPERSISTENCE_TEMP));
if (opts.excludetbl || opts.excludeidx || opts.excludensp)
appendPQExpBufferStr(&sql, "\nAND ep.pattern_id IS NULL");
@@ -1972,15 +1974,29 @@ compile_relation_list_one_db(PGconn *conn, SimplePtrList *relations,
if (opts.allrel)
appendPQExpBuffer(&sql,
" AND c.relam = %u "
- "AND c.relkind IN ('r', 'S', 'm', 't') "
+ "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 ('r', 'S', 'm', 't', 'i') "
- "AND ((c.relam = %u AND c.relkind IN ('r', 'S', 'm', 't')) OR "
- "(c.relam = %u AND c.relkind = 'i'))",
+ "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);
@@ -2007,7 +2023,7 @@ compile_relation_list_one_db(PGconn *conn, SimplePtrList *relations,
"\nAND (t.relname ~ ep.rel_regex OR ep.rel_regex IS NULL)"
"\nAND ep.heap_only"
"\nWHERE ep.pattern_id IS NULL"
- "\nAND t.relpersistence != 't'");
+ "\nAND t.relpersistence != " CppAsString2(RELPERSISTENCE_TEMP));
appendPQExpBufferStr(&sql,
"\n)");
}
@@ -2026,7 +2042,7 @@ compile_relation_list_one_db(PGconn *conn, SimplePtrList *relations,
"ON r.oid = i.indrelid "
"INNER JOIN pg_catalog.pg_class c "
"ON i.indexrelid = c.oid "
- "AND c.relpersistence != 't'");
+ "AND c.relpersistence != " CppAsString2(RELPERSISTENCE_TEMP));
if (opts.excludeidx || opts.excludensp)
appendPQExpBufferStr(&sql,
"\nINNER JOIN pg_catalog.pg_namespace n "
@@ -2041,7 +2057,7 @@ compile_relation_list_one_db(PGconn *conn, SimplePtrList *relations,
"\nWHERE true");
appendPQExpBuffer(&sql,
" AND c.relam = %u "
- "AND c.relkind = 'i'",
+ "AND c.relkind = " CppAsString2(RELKIND_INDEX),
BTREE_AM_OID);
if (opts.no_toast_expansion)
appendPQExpBuffer(&sql,
@@ -2065,7 +2081,7 @@ compile_relation_list_one_db(PGconn *conn, SimplePtrList *relations,
"ON t.oid = i.indrelid"
"\nINNER JOIN pg_catalog.pg_class c "
"ON i.indexrelid = c.oid "
- "AND c.relpersistence != 't'");
+ "AND c.relpersistence != " CppAsString2(RELPERSISTENCE_TEMP));
if (opts.excludeidx)
appendPQExpBufferStr(&sql,
"\nLEFT OUTER JOIN exclude_pat ep "
@@ -2078,7 +2094,7 @@ compile_relation_list_one_db(PGconn *conn, SimplePtrList *relations,
"\nWHERE true");
appendPQExpBuffer(&sql,
" AND c.relam = %u"
- " AND c.relkind = 'i')",
+ " AND c.relkind = " CppAsString2(RELKIND_INDEX) ")",
BTREE_AM_OID);
}
--
2.45.2
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