Hi 2016-12-02 23:25 GMT+01:00 Alvaro Herrera <alvhe...@2ndquadrant.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 >
