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

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.)

> 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
bethe 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

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

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.

> 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