Re: Conflict handling for COPY FROM

On Fri, Sep 20, 2019 at 4:16 PM Alexey Kondratov <a(dot)kondratov(at)postgrespro(dot)ru>
wrote:

>
> First of all, there is definitely a problem with grammar. In docs ERROR
> is defined as option and
>
> COPY test FROM '/path/to/copy-test-simple.csv' ERROR -1;
>
> works just fine, but if modern 'WITH (...)' syntax is used:
>
> COPY test FROM '/path/to/copy-test-simple.csv' WITH (ERROR -1);
> ERROR: option "error" not recognized
>
> while 'WITH (error_limit -1)' it works again.
>
> It happens, since COPY supports modern and very-very old syntax:
>
> * In the preferred syntax the options are comma-separated
> * and use generic identifiers instead of keywords. The pre-9.0
> * syntax had a hard-wired, space-separated set of options.
>
> So I see several options here:
>
> 1) Everything is left as is, but then docs should be updated and
> reflect, that error_limit is required for modern syntax.
>
> 2) However, why do we have to support old syntax here? I guess it exists
> for backward compatibility only, but this is a completely new feature.
> So maybe just 'WITH (error_limit 42)' will be enough?
>
> 3) You also may simply change internal option name from 'error_limit' to
> 'error' or SQL keyword from 'ERROR' tot 'ERROR_LIMIT'.
>
> I would prefer the second option.
>

agreed and Done

>
>
> Next, you use DestRemoteSimple for returning conflicting tuples back:
>
> + dest = CreateDestReceiver(DestRemoteSimple);
> + dest->rStartup(dest, (int) CMD_SELECT, tupDesc);
>
> However, printsimple supports very limited subset of built-in types, so
>
> CREATE TABLE large_test (id integer primary key, num1 bigint, num2
> double precision);
> COPY large_test FROM '/path/to/copy-test.tsv';
> COPY large_test FROM '/path/to/copy-test.tsv' ERROR 3;
>
> fails with following error 'ERROR: unsupported type OID: 701', which
> seems to be very confusing from the end user perspective. I've tried to
> switch to DestRemote, but couldn't figure it out quickly.
>
>
fixed

>
> Finally, I simply cannot get into this validation:
>
> + else if (strcmp(defel->defname, "error_limit") == 0)
> + {
> + if (cstate->ignore_error)
> + ereport(ERROR,
> + (errcode(ERRCODE_SYNTAX_ERROR),
> + errmsg("conflicting or redundant options"),
> + parser_errposition(pstate, defel->location)));
> + cstate->error_limit = defGetInt64(defel);
> + cstate->ignore_error = true;
> + if (cstate->error_limit == -1)
> + cstate->ignore_all_error = true;
> + }
>
> If cstate->ignore_error is defined, then we have already processed
> options list, since this is the only one place, where it's set. So we
> should never get into this ereport, doesn't it?
>
>
yes the check only needed for modern syntax

regards
Surafel