Thread: parse/analyze API refactoring

I have found some of the parse/analyze API calls confusing one too many 
times, so here I'm proposing some renaming and refactoring.

Notionally, there are three parallel ways to call the parse/analyze 
phase: with fixed parameters (for example, used by SPI), with variable 
parameters (for example, used by PREPARE), and with a parser callback 
(for example, used to parse the body of SQL functions).  Some of the 
involved functions were confusingly named and made this API structure 
more confusing.

For example, at the top level there are pg_analyze_and_rewrite() and 
pg_analyze_and_rewrite_params().  You'd think the first one doesn't take 
parameters and the second one takes parameters.  But the truth is, the 
first one takes fixed parameters and the second one takes a parser 
callback.  The parser callback can be used to parse parameters, but also 
other things.  There isn't any variant that takes variable parameters; 
that code is sprinkled around other places altogether.

One level below that, there is parse_analyze() (for fixed parameters) 
and parse_analyze_varparams() (good name).  But there is no analogous 
function for the callback variant; that code is spread out in 
pg_analyze_and_rewrite_params().

And then there are parse_fixed_parameters() and 
parse_variable_parameters().  But they don't do any parsing at all. 
They just set up callbacks for the parsing to follow.

This doesn't need to be so confusing.  With the attached patch set, the 
calls end up:

pg_analyze_and_rewrite_fixedparams()
   -> parse_analyze_fixedparams()
        -> setup_parse_fixed_parameters()

pg_analyze_and_rewrite_varparams() [new]
   -> parse_analyze_varparams()
        -> setup_parse_variable_parameters()

pg_analyze_and_rewrite_withcb()
   -> parse_analyze_withcb() [new]
        -> (nothing needed here)

(The "withcb" naming maybe isn't great; better ideas welcome.)

Not included in this patch set, but food for further thought:  The 
pg_analyze_and_rewrite_*() functions aren't all that useful (anymore). 
One might as well write

     pg_rewrite_query(parse_analyze_xxx(...))

The only things that pg_analyze_and_rewrite_*() do in addition to that 
is handle log_parser_stats, which could be moved into parse_analyze_*(), 
and TRACE_POSTGRESQL_QUERY_REWRITE_*(), which IMO doesn't make sense to 
begin with and should be in pg_rewrite_query().

On 12/28/21, 8:25 AM, "Peter Eisentraut" <peter.eisentraut@enterprisedb.com> wrote:
> (The "withcb" naming maybe isn't great; better ideas welcome.)

FWIW I immediately understood that this meant "with callback," so it
might be okay.

> Not included in this patch set, but food for further thought:  The
> pg_analyze_and_rewrite_*() functions aren't all that useful (anymore).
> One might as well write
>
>      pg_rewrite_query(parse_analyze_xxx(...))

I had a similar thought while reading through the patches.  If further
deduplication isn't too much trouble, I'd vote for that.

Nathan

You set this commit fest entry to Waiting on Author, but there were no 
reviews posted and the patch still applies and builds AFAICT, so I don't 
know what you meant by that.


On 13.01.22 00:49, Bossart, Nathan wrote:
> On 12/28/21, 8:25 AM, "Peter Eisentraut" <peter.eisentraut@enterprisedb.com> wrote:
>> (The "withcb" naming maybe isn't great; better ideas welcome.)
> 
> FWIW I immediately understood that this meant "with callback," so it
> might be okay.
> 
>> Not included in this patch set, but food for further thought:  The
>> pg_analyze_and_rewrite_*() functions aren't all that useful (anymore).
>> One might as well write
>>
>>       pg_rewrite_query(parse_analyze_xxx(...))
> 
> I had a similar thought while reading through the patches.  If further
> deduplication isn't too much trouble, I'd vote for that.
> 
> Nathan
> 

On Mon, Feb 28, 2022 at 07:46:40AM +0100, Peter Eisentraut wrote:
> You set this commit fest entry to Waiting on Author, but there were no
> reviews posted and the patch still applies and builds AFAICT, so I don't
> know what you meant by that.

Apologies for the lack of clarity.  I believe my only feedback was around
deduplicating the pg_analyze_and_rewrite_*() functions.  Would you rather
handle that in a separate patch?

-- 
Nathan Bossart
Amazon Web Services: https://aws.amazon.com

On 28.02.22 19:51, Nathan Bossart wrote:
> On Mon, Feb 28, 2022 at 07:46:40AM +0100, Peter Eisentraut wrote:
>> You set this commit fest entry to Waiting on Author, but there were no
>> reviews posted and the patch still applies and builds AFAICT, so I don't
>> know what you meant by that.
> 
> Apologies for the lack of clarity.  I believe my only feedback was around
> deduplicating the pg_analyze_and_rewrite_*() functions.  Would you rather
> handle that in a separate patch?

I have committed my original patches.  I'll leave the above-mentioned 
topic as ideas for the future.

On Wed, Mar 09, 2022 at 11:35:32AM +0100, Peter Eisentraut wrote:
> I have committed my original patches.  I'll leave the above-mentioned topic
> as ideas for the future.

Sounds good.

-- 
Nathan Bossart
Amazon Web Services: https://aws.amazon.com