Horton

Thread

Commits

  1. pg_amcheck: Use CppAsString2() for relkind and relpersistence in queries

  1. Use more CppAsString2() in pg_amcheck.c

    Michael Paquier <michael@paquier.xyz> — 2024-10-18T09:50:50Z

    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

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

    Daniel Gustafsson <daniel@yesql.se> — 2024-10-18T09:59:48Z

    > 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

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

    Alvaro Herrera <alvherre@alvh.no-ip.org> — 2024-10-18T13:18:26Z

    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"

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

    Alvaro Herrera <alvherre@alvh.no-ip.org> — 2024-10-18T13:20:00Z

    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.

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

    Nathan Bossart <nathandbossart@gmail.com> — 2024-10-18T17:22:26Z

    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

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

    Michael Paquier <michael@paquier.xyz> — 2024-10-19T01:16:40Z

    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

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

    Karina Litskevich <litskevichkarina@gmail.com> — 2024-11-25T15:25:46Z

    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/

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

    Michael Paquier <michael@paquier.xyz> — 2024-11-26T00:49:04Z

    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