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-11-17 18:22:26
Message-ID: 20161117182226.oxefipxl2rsdpyxv@alvherre.pgsql
Views: Raw Message | Whole Thread | Download mbox
Lists: pgsql-hackers

I've been going over this patch. I think it'd be better to restructure
the <sect2> before adding the docs for this new function; I already
split it out, so don't do anything about this.

Next, looking at struct TableExprBuilder I noticed that the comments are
already obsolete, as they talk about function params that do not exist
(missing_columns) and they fail to mention the ones that do exist.
Also, function member SetContent is not documented at all. Overall,
these comments do not convey a lot -- apparently, whoever reads them is
already supposed to know how it works: "xyz sets a row generating
filter" doesn't tell me anything. Since this is API documentation, it
needs to be much clearer.

ExecEvalTableExpr and ExecEvalTableExprProtected have no comments
whatsoever. Needs fixed.

I wonder if it'd be a good idea to install TableExpr first without the
implementing XMLTABLE, so that it's clearer what is API and what is

The number of new keywords in this patch is depressing. I suppose
there's no way around that -- as I understand, this is caused by the SQL
standard's definition of the syntax for this feature.

Have to go now for a bit -- will continue looking afterwards. Please
submit delta patches on top of your latest v12 to fix the comments I

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

In response to


Browse pgsql-hackers by date

  From Date Subject
Next Message Michael Paquier 2016-11-17 18:41:02 Re: Fun fact about autovacuum and orphan temp tables
Previous Message Michael Paquier 2016-11-17 17:51:50 Re: Password identifiers, protocol aging and SCRAM protocol