Re: patch: function xmltable

Hi

2016-12-02 23:25 GMT+01:00 Alvaro Herrera <alvherre(at)2ndquadrant(dot)com>:

> Here's version 17. I have made significant changes here.
>
> 1. Restructure the execQual code. Instead of a PG_TRY wrapper, I have
> split this code in three pieces; there's the main code with the PG_TRY
> wrappers and is mainly in charge of the builderCxt pointer. In the
> previous coding there was a shim that examined builderCxt but was not
> responsible for setting it up, which was ugly. The second part is the
> "initializer" which sets the row and column filters and does namespace
> processing. The third part is the "FetchRow" logic. It seems to me
> much cleaner this way.
>
> 2. rename the "builder" stuff to use the "routine" terminology. This is
> in line with what we do for other function-pointer-filled structs, such
> as FdwRoutine, IndexAmRoutine etc. I also cleaned up the names a bit
> more.
>
> 3. Added a magic number to the table builder context struct, so that we
> can barf appropriately. This is in line with PgXmlErrorContext --
> mostly for future-proofing. I didn't test this too hard. Also, moved
> the XmlTableContext struct declaration nearer the top of the file, as is
> customary. (We don't really need it that way, since the functions are
> all declared taking void *, but it seems cleaner to me anyway).
>
> 4. I added, edited, and fixed a large number of code comments.
>
> This is looking much better now, but it still needs at least the
> following changes.
>
> First, we need to fix is the per_rowset_memcxt thingy. I think the way
> it's currently being used is rather ugly; it looks to me like the memory
> context does not belong into the XmlTableContext struct at all.
> Instead, the executor code should keep the memcxt pointer in a state
> struct of its own, and it should be the executor's responsibility to
> change to the appropriate context before calling the table builder
> functions. In particular, this means that the table context can no
> longer be a void * pointer; it needs to be a struct that's defined by
> the executor (probably a primnodes.h one). The void * pointer is
> stashed inside that struct. Also, the "routine" pointer should not be
> part of the void * struct, but of the executor's struct. So the
> execQual code can switch to the memory context, and destroy it
> appropriately.
>
> Second, we should make gram.y set a new "function type" value in the
> TableExpr it creates, so that the downstream code (transformTableExpr,
> ExecInitExpr, ruleutils.c) really knows that the given function is
> XmlTableExpr, instead of guessing just because it's the only implemented
> case. Probably this "function type" is an enum (currently with a single
> value TableExprTypeXml or something like that) in primnodes.
>

It has sense - I was not sure about it - because currently it is only one
value, you mentioned it.

>
> Finally, there's the pending task of renaming and moving
> ExecTypeFromTableExpr to some better place. Not sure that moving it
> back to nodeFuncs is a nice idea. Looks to me like calling it from
> ExprTypmod is a rather ugly idea.
>

The code is related to prim nodes - it is used more times than in executor.

>
> Hmm, ruleutils ... not sure what to think of this one.
>

it is little bit more complex - but it is related to complexity of XMLTABLE

>
> The typedefs.list changes are just used to pgindent the affected code
> correctly. It's not for commit.
>

The documentation is very precious. Nice

+ /* XXX OK to do this? looks a bit out of place ... */
+ assign_record_type_typmod(typeInfo);

I am thinking it is ok. It is tupdesc without fixed typid, typname used in
returned value - you should to register this tupdesc in typcache.

Regards

Pavel

> --
> Álvaro Herrera https://www.2ndQuadrant.com/
> PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
>