Add a pg_get_query_def function (was Re: Deparsing rewritten query)

From: Julien Rouhaud <rjuju123(at)gmail(dot)com>
To: Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>
Cc: Bharath Rupireddy <bharath(dot)rupireddyforpostgres(at)gmail(dot)com>, Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com>, Gilles Darold <gilles(at)darold(dot)net>, PostgreSQL Hackers <pgsql-hackers(at)lists(dot)postgresql(dot)org>
Subject: Add a pg_get_query_def function (was Re: Deparsing rewritten query)
Date: 2022-03-27 05:21:09
Message-ID: 20220327052109.phgehhkrvgtz4bsz@jrouhaud
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

On Fri, Mar 25, 2022 at 05:49:04PM -0400, Tom Lane wrote:
> Julien Rouhaud <rjuju123(at)gmail(dot)com> writes:
> > I'm attaching the correct patch this time, sorry about that.
>
> While I'm okay with this in principle, as it stands it fails
> headerscheck/cpluspluscheck:
>
> $ src/tools/pginclude/headerscheck
> In file included from /tmp/headerscheck.Oi8jj3/test.c:2:
> ./src/include/utils/ruleutils.h:42:35: error: unknown type name 'StringInfo'; did you mean 'String'?
> void get_query_def(Query *query, StringInfo buf, List *parentnamespace,
> ^~~~~~~~~~
> String
> ./src/include/utils/ruleutils.h:43:9: error: unknown type name 'TupleDesc'
> TupleDesc resultDesc,
> ^~~~~~~~~

Ah thanks for the info. I actually never tried headerscheck/cplupluscheck
before.

> We could of course add the necessary #include's to ruleutils.h,
> but considering that we seem to have been at some pains to minimize
> its #include footprint, I'm not really happy with that approach.
> I'm inclined to think that maybe a wrapper function with a slightly
> simplified interface would be a better way to go. The deparsed string
> could just be returned as a palloc'd "char *", unless you have some reason
> to need it to be in a StringInfo. I wonder which of the other parameters
> really need to be exposed, too. Several of them seem to be more internal
> to ruleutils.c than something that outside callers would care to
> manipulate. For instance, since struct deparse_namespace isn't exposed,
> there really isn't any way to pass anything except NIL for
> parentnamespace.
>
> The bigger picture here is that get_query_def's API has changed over time
> internally to ruleutils.c, and I kind of expect that that might continue
> in future, so having a wrapper function with a more stable API could be a
> good thing.

Fair point. That's a much better approach and goes well with the rest of the
exposed functions in that file. I went with a pg_get_querydef, getting rid of
the StringInfo and the List and using the same "bool pretty" flag as used
elsewhere. While doing so, I saw that there were a lot of copy/pasted code for
the pretty flags, so I added a GET_PRETTY_FLAGS(pretty) macro to avoid adding
yet another occurrence. I also kept the wrapColument and startIdent as they
can be easily used by callers.

Attachment Content-Type Size
v4-0001-Add-a-pg_get_query_def-wrapper-around-get_query_d.patch text/plain 6.4 KB

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Tatsuo Ishii 2022-03-27 06:28:41 Re: [HACKERS] WIP aPatch: Pgbench Serialization and deadlock errors
Previous Message Julien Rouhaud 2022-03-27 04:15:25 Re: make MaxBackends available in _PG_init