Horton

Thread

Commits

  1. psql: Sprinkle more CppAsString2() in describe.c

  1. More CppAsString2() in psql's describe.c

    Michael Paquier <michael@paquier.xyz> — 2024-10-22T00:49:10Z

    Hi all,

This was on my stack of things for some time, but please find attached
a patch to clean up some code with $subject because HEAD's describe.c
is a mixed bag of relying on CppAsString2() and hardcoded values.
Switching to CppAsString2() has the advantage to make the code more
self-documented, so as it is not necessary to remember what a single
byte means in a catalog.

I should have caught most of them, with exceptions related to
policies, subscriptions and dependencies being intentional.

I have noticed that describe.c includes pg_am.h and not pg_am_d.h.
That's not a good idea even if it is OK now because this has the risk
of pulling backend-side definitions into psql.  psql -E reported
consistent formats in the queries generated, so things look in rather
good shape here.

Note that there were a couple of value checks not part of the queries
that relied on values from the catalogs for some relpersistences and
replidents.  I've fixed them while on it.

Thoughts or comments are welcome.
--
Michael

  2. Re: More CppAsString2() in psql's describe.c

    Michael Paquier <michael@paquier.xyz> — 2024-11-28T06:34:55Z

    On Tue, Oct 22, 2024 at 09:49:10AM +0900, Michael Paquier wrote:
> Note that there were a couple of value checks not part of the queries
> that relied on values from the catalogs for some relpersistences and
> replidents.  I've fixed them while on it.
> 
> Thoughts or comments are welcome.

So, this one has been sitting in the CF for a couple of weeks now.
I've looked at it again and the queries are written the same.  There
was one inconsistency with the ordering of the headers and one
indentation issue with a query for extended stats.

Any objections against applying it?  This is the last area of the code
where we rely on such hardcoded values rather than CppAsString2().
Note also the pg_am.h inclusion which is incorrect.
--
Michael

  3. Re: More CppAsString2() in psql's describe.c

    Corey Huinker <corey.huinker@gmail.com> — 2024-11-28T07:19:30Z

    >
> Any objections against applying it?
>

+1. No objections. It's a great help in communicating meaning.

  4. Re: More CppAsString2() in psql's describe.c

    Daniel Gustafsson <daniel@yesql.se> — 2024-11-28T09:40:35Z

    > On 28 Nov 2024, at 07:34, Michael Paquier <michael@paquier.xyz> wrote:
> 
> On Tue, Oct 22, 2024 at 09:49:10AM +0900, Michael Paquier wrote:
>> Note that there were a couple of value checks not part of the queries
>> that relied on values from the catalogs for some relpersistences and
>> replidents.  I've fixed them while on it.
>> 
>> Thoughts or comments are welcome.
> 
> So, this one has been sitting in the CF for a couple of weeks now.
> I've looked at it again and the queries are written the same.  There
> was one inconsistency with the ordering of the headers and one
> indentation issue with a query for extended stats.
> 
> Any objections against applying it?  This is the last area of the code
> where we rely on such hardcoded values rather than CppAsString2().
> Note also the pg_am.h inclusion which is incorrect.

LGTM, I didn't scan for omissions but the ones in the patch look right to me.
I sort of wish we had a shorter macro as CppAsString2() get's pretty verbose
when used frequently like this.

--
Daniel Gustafsson

  5. Re: More CppAsString2() in psql's describe.c

    Corey Huinker <corey.huinker@gmail.com> — 2024-11-28T18:25:56Z

    >
>
> LGTM, I didn't scan for omissions but the ones in the patch look right to
> me.
> I sort of wish we had a shorter macro as CppAsString2() get's pretty
> verbose
> when used frequently like this.
>

I don't quite understand the etymology of the name (it's some variation on
C++'s std::to_string, plus...something), but if I did, I'd probably find
the name less icky.

STR(), C_STR(), STRING(), and CSTRING() all seem to be available...

  6. Re: More CppAsString2() in psql's describe.c

    Daniel Gustafsson <daniel@yesql.se> — 2024-11-28T19:18:22Z

    > On 28 Nov 2024, at 19:25, Corey Huinker <corey.huinker@gmail.com> wrote:

>> LGTM, I didn't scan for omissions but the ones in the patch look right to me.
>> I sort of wish we had a shorter macro as CppAsString2() get's pretty verbose
>> when used frequently like this.
> 
> I don't quite understand the etymology of the name (it's some variation on C++'s std::to_string, plus...something), but if I did, I'd probably find the name less icky.

AFAIK, Cpp stands for "C Pre Processor" as it is a preprocessor expansion into
a string, and the 2 is simply because CppAsString2() calls CppAsString().  The
reason for the call is perform macro expansion before converting to string.

I agree that it's not icky, it just get's very verbose as opposed how
translation of strings is done with _(<string>) as an example.

--
Daniel Gustafsson

  7. Re: More CppAsString2() in psql's describe.c

    Michael Paquier <michael@paquier.xyz> — 2024-11-28T23:57:08Z

    On Thu, Nov 28, 2024 at 10:40:35AM +0100, Daniel Gustafsson wrote:
> LGTM, I didn't scan for omissions but the ones in the patch look right to me.
> I sort of wish we had a shorter macro as CppAsString2() get's pretty verbose
> when used frequently like this.

Thanks for the reviews, applied what I had.  I've managed to miss a
c.contype = 'n' in describeOneTableDetails(), actually, so I've fixed
it while on it.

Here are some notes about the state of things in describe.c:
- RELKIND_SPECIAL does not exist anymore, see listTables().
- pg_depend.deptype, that cannot be changed currently as
  DependencyType is in dependency.h.
- pol.polcmd wants some ACLs now in acl.h.
- tgenabled has some hardcoded values now in trigger.h.
- Event triggers with evtenabled has values in trigger.h
(TRIGGER_FIRES_ON_ORIGIN).
- attcompression has two values for lz4 and pglz.
- substream, where LOGICALREP_STREAM_ON & friends should be added to
pg_subscription_d.h to allow the move.  Perhaps we should just do that
for the subscription case.  That's worth a patch of its own as it
changes pg_subscription_d.h.  I'll spawn a new thread about that.
--
Michael

  8. Re: More CppAsString2() in psql's describe.c

    Tom Lane <tgl@sss.pgh.pa.us> — 2024-11-29T00:51:34Z

    Daniel Gustafsson <daniel@yesql.se> writes:
> On 28 Nov 2024, at 19:25, Corey Huinker <corey.huinker@gmail.com> wrote:
>>> I sort of wish we had a shorter macro as CppAsString2() get's pretty verbose
>>> when used frequently like this.

>> I don't quite understand the etymology of the name (it's some variation on C++'s std::to_string, plus...something), but if I did, I'd probably find the name less icky.

> AFAIK, Cpp stands for "C Pre Processor" as it is a preprocessor expansion into
> a string, and the 2 is simply because CppAsString2() calls CppAsString().  The
> reason for the call is perform macro expansion before converting to string.

Yeah.  That name was fine when there were just a few occurrences,
but now it seems we've got hundreds, so +1 for something shorter.

I don't like suggestions as generic as STR() though; that's not very
meaningful and also seems like it might collide with some other usage.
How about something like SYM2LIT ("symbol to literal") or SYM2STR?

			regards, tom lane

  9. Re: More CppAsString2() in psql's describe.c

    Peter Eisentraut <peter@eisentraut.org> — 2024-12-02T07:37:46Z

    On 28.11.24 07:34, Michael Paquier wrote:
> -						  "WHERE p.prokind = 'a'\n",
> +						  "WHERE p.prokind = " CppAsString2(PROKIND_AGGREGATE) "\n",

I noticed something here.  If the symbol that is the argument of 
CppAsString2() is not defined, maybe because there is a typo or because 
the required header file is not included, then there is no compiler 
diagnostic.  Instead, the symbol is just used as is, which might then 
result in an SQL error or go unnoticed if there is no test coverage.

Now, the same is technically true for the previous code with the 
hardcoded character values, but I think at least something like

     "WHERE p.prokind = 'a'\n",

is easier to eyeball for correctness, whereas, you know, 
CppAsString2(PROPARALLEL_RESTRICTED), is quite something.

I think this would be more robust if it were written something like

     "WHERE p.prokind = '%c'\n", PROKIND_AGGREGATE

(Moreover, the current structure assumes that the C character literal 
syntax used by the PROKIND_* and other symbols happens to be the same as 
the SQL string literal syntax required in those queries, which is just 
an accident.)

  10. Re: More CppAsString2() in psql's describe.c

    Daniel Gustafsson <daniel@yesql.se> — 2024-12-02T07:45:11Z

    > On 2 Dec 2024, at 08:37, Peter Eisentraut <peter@eisentraut.org> wrote:

> the current structure assumes that the C character literal syntax used by the PROKIND_* and other symbols happens to be the same as the SQL string literal syntax required in those queries, which is just an accident.)

I'm not sure I would call it an accident, rather there is no guarantee that it
will always be true.

--
Daniel Gustafsson

  11. Re: More CppAsString2() in psql's describe.c

    Tom Lane <tgl@sss.pgh.pa.us> — 2024-12-02T07:52:50Z

    Peter Eisentraut <peter@eisentraut.org> writes:
> On 28.11.24 07:34, Michael Paquier wrote:
>> -						  "WHERE p.prokind = 'a'\n",
>> +						  "WHERE p.prokind = " CppAsString2(PROKIND_AGGREGATE) "\n",

> I noticed something here.  If the symbol that is the argument of 
> CppAsString2() is not defined, maybe because there is a typo or because 
> the required header file is not included, then there is no compiler 
> diagnostic.  Instead, the symbol is just used as is, which might then 
> result in an SQL error or go unnoticed if there is no test coverage.

Yeah, if memory serves we've actually been bit by that at least once.
But isn't there a way to improve the macro so this'd lead to an error?

> I think this would be more robust if it were written something like
>      "WHERE p.prokind = '%c'\n", PROKIND_AGGREGATE

The problem is that you frequently have several of these in a
statement, which'd lead to a lot of %c items that readers have
to mentally match up with not-very-close-by printf arguments.
We already have that with translatable column headers, for instance,
and it's a pain in the rear for readability and maintainability.
I do not buy that this is a preferable answer.

> (Moreover, the current structure assumes that the C character literal 
> syntax used by the PROKIND_* and other symbols happens to be the same as 
> the SQL string literal syntax required in those queries, which is just 
> an accident.)

So?  There isn't much about C syntax that isn't an accident.
Neither literal syntax is going to change, so I don't see why
it's problematic to rely on them being the same.

			regards, tom lane

  12. Re: More CppAsString2() in psql's describe.c

    Peter Eisentraut <peter@eisentraut.org> — 2024-12-02T11:26:47Z

    On 02.12.24 08:52, Tom Lane wrote:
>> (Moreover, the current structure assumes that the C character literal
>> syntax used by the PROKIND_* and other symbols happens to be the same as
>> the SQL string literal syntax required in those queries, which is just
>> an accident.)
> So?  There isn't much about C syntax that isn't an accident.
> Neither literal syntax is going to change, so I don't see why
> it's problematic to rely on them being the same.

For example, if you write

#define          RELKIND_RELATION        '\x72'

then it won't work anymore.

I was also curious whether

#define FOO 'r'
#define RELKIND_RELATION FOO

would work.  It appears it does.  But this syntactic construction is 
quite hard to completely understand currently.

  13. Re: More CppAsString2() in psql's describe.c

    Daniel Gustafsson <daniel@yesql.se> — 2024-12-02T14:51:18Z

    > On 2 Dec 2024, at 08:52, Tom Lane <tgl@sss.pgh.pa.us> wrote:

> But isn't there a way to improve the macro so this'd lead to an error?

That sounds like a pretty decent improvement in general.  I experimented with
quick hack using a typeof check on the passed symbol which catches symbolname
typos. It might be totally unfit for purpose but it was an interesting hack.

#define CppAsString2(x) ((__builtin_types_compatible_p(__typeof__(x),char *) ?: CppAsString(x)))

It does require changing the any uses of the macro in string generation from
f("pre" CppAsString2(SYM) "post"); into f_v("pre%spost", CppAsString2(SYM));
however, and using it as part of another macro (TABLESPACE_VERSION_DIRECTORY)
doesn't work.

--
Daniel Gustafsson