Re: Add header support to text format and matching feature

From: Michael Paquier <michael(at)paquier(dot)xyz>
To: Julien Rouhaud <rjuju123(at)gmail(dot)com>
Cc: Andrew Dunstan <andrew(at)dunslane(dot)net>, Peter Eisentraut <peter(dot)eisentraut(at)enterprisedb(dot)com>, Greg Stark <stark(at)mit(dot)edu>, Rémi Lapeyre <remi(dot)lapeyre(at)lenstra(dot)fr>, Zhihong Yu <zyu(at)yugabyte(dot)com>, David Steele <david(at)pgmasters(dot)net>, vignesh C <vignesh21(at)gmail(dot)com>, PostgreSQL-development <pgsql-hackers(at)postgresql(dot)org>, Daniel Verite <daniel(at)manitou-mail(dot)org>
Subject: Re: Add header support to text format and matching feature
Date: 2022-06-13 07:46:46
Message-ID: YqbrZm4VdoEEcnJl@paquier.xyz
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

On Mon, Jun 13, 2022 at 10:32:13AM +0800, Julien Rouhaud wrote:
> On Sun, Jun 12, 2022 at 09:36:13AM -0400, Andrew Dunstan wrote:
> I'm fine with it. I added such a check and mentioned it in the documentation.

An error looks like the right call at this stage of the game. I am
not sure what the combination of MATCH with COPY TO would mean,
actually. And with the concept of SELECT queries on top of it, the
whole idea gets blurrier.

> I think it would still be problematic if the target table has dropped columns.
> Fortunately, as I initially thought the problem is only due to a thinko in the
> original commit which used a wrong variable for the raw_fields offset. Once
> fixed (attached v1) I didn't see any other problem in the rest of the logic and
> all the added regression tests work as expected.

Interesting catch. One thing that I've always found useful when it
comes to tests that stress dropped columns is to have tests where we
reduce the number of total columns that still exist. An extra thing
is to look after ........pg.dropped.N........ a bit, and I would put
something in one of the headers.

> if (pg_strcasecmp(sval, "match") == 0)
> + {
> + /* match is only valid for COPY FROM */
> + if (!is_from)
> + ereport(ERROR,
> + (errcode(ERRCODE_SYNTAX_ERROR),
> + errmsg("%s match is only valid for COPY FROM",
> + def->defname)));

Some nits. I would suggest to reword this error message, like "cannot
use \"match\" with HEADER in COPY TO". There is no need for the extra
comment, and the error code should be ERRCODE_FEATURE_NOT_SUPPORTED.
--
Michael

Attachment Content-Type Size
v2-0001-Fix-processing-of-header-match-option-for-COPY.patch text/x-diff 7.6 KB

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Michael Paquier 2022-06-13 07:51:30 Re: Improve TAP tests of pg_upgrade for cross-version tests
Previous Message Peter Eisentraut 2022-06-13 07:35:16 Re: Add header support to text format and matching feature