Re: deparsing utility commands

Alvaro, all,

* Alvaro Herrera (alvherre(at)2ndquadrant(dot)com) wrote:
> This is a repost of the patch to add CREATE command deparsing support to
> event triggers. It now supports not only CREATE but also ALTER and
> other commands such as GRANT/REVOKE, COMMENT ON and SECURITY LABEL.
> This patch series is broken up in a rather large number of patches, but
> my intention would be to commit separately only the first 6 patches; the
> remaining parts are split up only so that it's easy to see how deparsing
> each patch is implemented, and would be committed as a single changeset
> (but before that I need a bunch of extra fixes and additions to other
> parts.)

Alright, I spent a solid few hours yesterday going back through the past
year+ discussion around this. I'm planning to spend more time on
review, though I see Andres has done a good review and I'm generally
on-board with the comments he made. There's a few things which aren't
really "review" but more commentary on the approach that I wanted to
bring up independently.

First off, I took a look at what ends up being required for CREATE
POLICY, as it's code I would have been writing if the timing had ended
up a bit differently. Below is the CREATE POLICY support and it looks
pretty darn reasonable to me and no worse than dealing with pg_dump, or
psql, or the other areas which we have to update whenever we're adding
new commands. To be clear, I'm no more interested in adding more work
to be done whenever we add a new command than the next committer, but
this isn't bad and gives us a new capability (well, almost, more below)
which I've wished for since I started hacking on PG some 13+ years ago.

It'd be *really* nice to be able to pass an object identifier to some
function and get back the CREATE (in particular, though perhaps DROP, or
whatever) command for it. This gets us *awful* close to that without
actually giving it to us and that's bugging me. The issue is the
parsetree, which I understand may be required in some cases (ALTER,
primairly, it seems?), but isn't always.

Looking at the CREATE POLICY patch, here's what I see.

> diff --git a/src/backend/tcop/deparse_utility.c b/src/backend/tcop/deparse_utility.c
> index 2a0a2b9..cd600ff 100644
> --- a/src/backend/tcop/deparse_utility.c
> +++ b/src/backend/tcop/deparse_utility.c
> @@ -4402,6 +4402,91 @@ deparse_CommentStmt(Oid objectId, Oid objectSubId, Node *parsetree)
> }
>
> static ObjTree *
> +deparse_CreatePolicyStmt(Oid objectId, Node *parsetree)
> +{
> + CreatePolicyStmt *node = (CreatePolicyStmt *) parsetree;
> + ObjTree *policy;
> + ObjTree *tmp;
> + Relation polRel = heap_open(PolicyRelationId, AccessShareLock);
> + HeapTuple polTup = get_catalog_object_by_oid(polRel, objectId);
> + Form_pg_policy polForm;
> + ListCell *cell;
> + List *list;
> +
> + if (!HeapTupleIsValid(polTup))
> + elog(ERROR, "cache lookup failed for policy %u", objectId);
> + polForm = (Form_pg_policy) GETSTRUCT(polTup);

Alright, we get the parsetree and build a CreatePolicyStmt with it, but
what do we use it for? We also get the polForm from pg_policy.

> + policy = new_objtree_VA("CREATE POLICY %{identity}I ON %{table}D %{for_command}s "
> + "%{to_role}s %{using}s %{with_check}s", 1,
> + "identity", ObjTypeString, node->policy_name);

The policy_name is certainly available from pg_policy instead.

> + /* add the "ON table" clause */
> + append_object_object(policy, "table",
> + new_objtree_for_qualname_id(RelationRelationId,
> + polForm->polrelid));

Here we're getting the polrelid from the polForm.

> + /* add "FOR command" clause */
> + tmp = new_objtree_VA("FOR %{command}s", 1,
> + "command", ObjTypeString, node->cmd);
> + append_object_object(policy, "for_command", tmp);

The cmd is also available from the polForm.

> + /* add the "TO role" clause. Always contains at least PUBLIC */
> + tmp = new_objtree_VA("TO %{role:, }I", 0);
> + list = NIL;
> + foreach (cell, node->roles)
> + {
> + list = lappend(list,
> + new_string_object(strVal(lfirst(cell))));
> + }
> + append_array_object(tmp, "role", list);
> + append_object_object(policy, "to_role", tmp);
> +
> + /* add the USING clause, if any */
> + tmp = new_objtree_VA("USING (%{expression}s)", 0);
> + if (node->qual)
> + {
> + Datum deparsed;
> + Datum storedexpr;
> + bool isnull;
> +
> + storedexpr = heap_getattr(polTup, Anum_pg_policy_polqual,
> + RelationGetDescr(polRel), &isnull);
> + if (isnull)
> + elog(ERROR, "invalid NULL polqual expression in policy %u", objectId);
> + deparsed = DirectFunctionCall2(pg_get_expr, storedexpr, polForm->polrelid);
> + append_string_object(tmp, "expression",
> + TextDatumGetCString(deparsed));
> + }
> + else
> + append_bool_object(tmp, "present", false);
> + append_object_object(policy, "using", tmp);
> +
> + /* add the WITH CHECK clause, if any */
> + tmp = new_objtree_VA("WITH CHECK (%{expression}s)", 0);
> + if (node->with_check)
> + {
> + Datum deparsed;
> + Datum storedexpr;
> + bool isnull;
> +
> + storedexpr = heap_getattr(polTup, Anum_pg_policy_polwithcheck,
> + RelationGetDescr(polRel), &isnull);
> + if (isnull)
> + elog(ERROR, "invalid NULL polwithcheck expression in policy %u", objectId);
> + deparsed = DirectFunctionCall2(pg_get_expr, storedexpr, polForm->polrelid);
> + append_string_object(tmp, "expression",
> + TextDatumGetCString(deparsed));
> + }
> + else
> + append_bool_object(tmp, "present", false);
> + append_object_object(policy, "with_check", tmp);
> +
> + heap_close(polRel, AccessShareLock);

The USING and WITH CHECK quals can be extracted from the polForm also,
of course, it's what psql and pg_dump are doing, after all.

So, why depend on the parsetree? What about have another argument which
a user could use without the parsetree, for things that it's absolutely
required for today (eg: ALTER sub-command differentiation)? Maybe
that's possible and maybe it isn't, but at least for the CREATE cases we
should be able to avoid forcing a user to provide a built parsetree, and
that'd be *really* nice and open up this feature to quite a few other
use-cases that I can think of. I'd further suggest that we provide
these command to the SQL level, and then have a wrapper which will take
the name of an object, resolve it to Oid, and then pass back the CREATE
command for it.

For as much commentary as there has been about event triggers and
replication and BDR, and about how this is going to end up being a
little-used side-feature, I'm amazed that there has been very little
discussion about how this would finally put into the backend the ability
to build up CREATE commands for objects and remove the need to use
pg_dump for that (something which isn't exactly fun for GUI applications
and other use-cases). Further, this would increase the number of people
using the actually complicated part of this, which is building up of the
command from the catalog, and that helps address both the concerns about
bad lurking bugs that go unnoticed and about how this wouldn't be used
very much and therefore might bitrot.

I don't know about the rest of you, but I'd love to see a psql backslash
command to compliment this capability, to which you can specify a table
and get back a CREATE TABLE statement.

And, yes, I'm fine with the JSON aspect of this (even though I realize
my above commentary glosses over it) and think that's likely to be
picked up by other clients also- so we should certainly provide a way to
expose that structure to clients too (imagine pgAdmin4 built on top of
this!).

Thanks!

Stephen