Skip site navigation (1) Skip section navigation (2)

Re: patch: function xmltable

From: Alvaro Herrera <alvherre(at)2ndquadrant(dot)com>
To: Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com>
Cc: Craig Ringer <craig(at)2ndquadrant(dot)com>,PostgreSQL Hackers <pgsql-hackers(at)postgresql(dot)org>
Subject: Re: patch: function xmltable
Date: 2016-12-02 22:25:50
Message-ID: 20161202222550.t5v23evjddgmkfly@alvherre.pgsql (view raw, whole thread or download thread mbox)
Lists: pgsql-hackers
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

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

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.

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.

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

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

Álvaro Herrera      
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Attachment: 0001-Create-sect3-in-the-functions-xml-section.patch
Description: text/plain (6.6 KB)
Attachment: 0002-xmltable-17.patch
Description: text/plain (166.7 KB)

In response to


pgsql-hackers by date

Next:From: Jim NasbyDate: 2016-12-02 22:33:54
Subject: Re: HaveNFreeProcs() iterates through entire freeProcs list
Previous:From: Nico WilliamsDate: 2016-12-02 21:32:24
Subject: Re: Tackling JsonPath support

Privacy Policy | About PostgreSQL
Copyright © 1996-2018 The PostgreSQL Global Development Group