Feedback-ID: i131946ab:Fastmail
Message-ID: <3baf8f25-3bd7-7db5-0b31-e4c92b6269ba@enterprisedb.com>
Date: Thu, 23 Feb 2023 08:51:47 +0100
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:102.0)
 Gecko/20100101 Thunderbird/102.6.1
Subject: Re: [PATCH] Add pretty-printed XML output option
Content-Language: en-US
To: Jim Jones <jim.jones@uni-muenster.de>, Peter Smith <smithpb2250@gmail.com>
Cc: Nikolay Samokhvalov <samokhvalov@gmail.com>,
 PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>,
 Andrey Borodin <amborodin86@gmail.com>
References: <2f5df461-dad8-6d7d-4568-08e10608a69b@uni-muenster.de>
 <62643598-b80d-90f4-8901-b85dccadf4c4@enterprisedb.com>
 <CANNMO+Kwb4_87G8qDeN+Vk1B1vX3HvgoGW+13fJ-b6rj7qbAww@mail.gmail.com>
 <beb5a47d-4684-4b2f-8647-bacb6bd60acf@uni-muenster.de>
 <CANNMO+L5X2ynyXL2RK7gTVA-pj=Z8o8FCCr6RwLfRK_LinjSbg@mail.gmail.com>
 <d79950fd-b3f5-783c-919a-a95de23c0c2c@enterprisedb.com>
 <4c2efca5-8257-2424-6687-07c65e319e95@uni-muenster.de>
 <CAHut+PtMHeBBZ2zv0Bk7-4KMkSrrxZ9PZxUCzhpwTGVk+8G3cw@mail.gmail.com>
 <2eac4d78-2b20-e8ec-54cc-22a88653a1cc@uni-muenster.de>
 <CAHut+PsgXxbzzsG+VqHUa4Kz3G4B-anNmF3-GENwuUdfMJtowA@mail.gmail.com>
 <fb2a543b-e615-d9ba-8402-909631f9b212@uni-muenster.de>
 <CAHut+Pvryt_+wA7TwKVaaF0e6Pm5LaPZQLKatp=78oFd8vMh3A@mail.gmail.com>
 <6cb296fe-4b49-3691-633e-2e2366997b04@uni-muenster.de>
From: Peter Eisentraut <peter.eisentraut@enterprisedb.com>
In-Reply-To: <6cb296fe-4b49-3691-633e-2e2366997b04@uni-muenster.de>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
Archived-At: 
 <https://www.postgresql.org/message-id/3baf8f25-3bd7-7db5-0b31-e4c92b6269ba%40enterprisedb.com>
Precedence: bulk

On 23.02.23 07:36, Jim Jones wrote:
> On 23.02.23 02:52, Peter Smith wrote:
>> Here are my review comments for patch v17-0001.
>>
>> ======
>> src/test/regress/sql/xml.sql
>>
>> The blank line(s) which previously separated the xmlserialize tests
>> from the xml IS [NOT] DOCUMENT tests are now missing...
> 
> v18 adds a new line in the xml.sql file to separate the xmlserialize 
> test cases from the rest.

In kwlist.h you have

     PG_KEYWORD("indent", INDENT, UNRESERVED_KEYWORD, AS_LABEL)

but you can actually make it BARE_LABEL, which is preferable.

More importantly, you need to add the new keyword to the 
bare_label_keyword production in gram.y.  I thought we had some tooling 
in the build system to catch this kind of omission, but it's apparently 
not working right now.

Elsewhere, let's rename the xmlformat() C function to xmlserialize() (or 
maybe something like xmlserialize_indent()), so the association is clearer.