Re: Hook for extensible parsing.

On Wed, Sep 15, 2021 at 10:14 PM Jim Mlodgenski <jimmy76(at)gmail(dot)com> wrote:
>
> On Wed, Sep 15, 2021 at 9:25 AM Simon Riggs
> <simon(dot)riggs(at)enterprisedb(dot)com> wrote:
> >
> > The general rule has always been that we don't just put hooks in, we
> > always require an in-core use for those hooks. I was reminded of that
> > myself recently.
> >
> That's not historically what has happened. There are several hooks with
> no in core use such as emit_log_hook and ExplainOneQuery_hook. The recent
> openssl_tls_init_hook only has a usage in src/test/modules

Yes, I also think that it's not a strict requirement that all hooks
have a caller in the core, even if it's obviously better if that's the
case.

> > What we need is something in core that actually makes use of this. The
> > reason for that is not politics, but a simple test of whether the
> > feature makes sense AND includes all required bells and whistles to be
> > useful in the real world.
> >
> Agreed. There should be something in src/test/modules to exercise this
> but probably more to flush things out. Maybe extending adminpack to use
> this so if enabled, it can use syntax like:
> FILE READ 'foo.txt'

For this hook, maintaining a real alternative parser seems like way
too much trouble to justify an in-core user. The fact that many
people have asked for such a feature over the year should be enough to
justify the use case. We could try to invent some artificial need
like the one you suggest for adminpack, but it also feels like a waste
of resources.

As far as I'm concerned a naive strcmp-based parser in
src/test/modules should be enough to validate that the hook is
working, there's no need for more. In any case if the only
requirement for it to be committed is to write a real parser, whether
in contrib or src/test/modules, I'll be happy to do it.

> > Core doesn't need a LOL parser and I don't think we should commit that.
> >
> +1

I entirely agree, and I repeatedly mentioned in that thread that I did
*not* want to add this parser in core. The only purpose of patches
0002 and 0004 is to make the third-party bison based parser
requirements less abstract, and demonstrate that this approach can
successfully make two *radically different* parsers cohabit.

> > If we do this, I think it should have CREATE LANGUAGE support, so that
> > each plugin can be seen as an in-core object and allow security around
> > which users can execute which language types, allow us to switch
> > between languages and have default languages for specific users or
> > databases.
> >
> This hook allows extension developers to supplement syntax in addition
> to adding a whole new language allowing the extension to appear more
> native to the end user. For example, pglogical could use this to add
> syntax to do a CREATE NODE instead of calling the function create_node.
> Adding CREATE LANGUAGE support around this would just be for a narrow
> set of use cases where a new language is added.

Yes, this hook can be used to implement multiple things as I mentioned
my initial email. Additionally, if this is eventually committed I'd
like to add support for CREATE HYPOTHETICAL INDEX grammar in hypopg.
Such a parser would only support one command (that extends an existing
one), so it can't really be called a language. Of course if would be
better to have the core parser accept a CREATE [ HYPOTHETICAL ] INDEX
and setup a flag so that third-parrty module can intercept this
utility command, but until that happens I could provide that syntactic
sugar for my users as long as I'm motivated enough to write this
parser.

Also, a hook based approach is still compatible with per database /
role configuration. It can be done either via specific
session_preload_libraries, or via a custom GUC if for some reason the
module requires to be in shared_preload_libraries.