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

Re: patch: function xmltable

From: Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com>
To: Alvaro Herrera <alvherre(at)2ndquadrant(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-18 20:49:36
Message-ID: CAFj8pRCeRBTNin1VRt79GxersBtOjbkkgZNhy9=8AR1=jm6=-A@mail.gmail.com (view raw, whole thread or download thread mbox)
Thread:
Lists: pgsql-hackers
Hi

2016-11-17 19:22 GMT+01:00 Alvaro Herrera <alvherre(at)2ndquadrant(dot)com>:

> 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 am sending the patch with more comments - but it needs a care someone
with good English skills.


>
> 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
> implementation.
>

I am not sure about this step - the API is clean from name. In this moment,
for this API is not any other tests than XMLTABLE implementation.


>
> 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
> mentioned.
>
>
Regards

Pavel


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

Attachment: fix-comments.patch
Description: text/x-patch (4.1 KB)

In response to

Responses

pgsql-hackers by date

Next:From: Alvaro HerreraDate: 2016-11-18 20:53:56
Subject: Re: patch: function xmltable
Previous:From: Joshua DrakeDate: 2016-11-18 20:45:33
Subject: Re: Mail thread references in commits

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