MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII;
 format=flowed
Content-Transfer-Encoding: 7bit
Date: Mon, 22 Aug 2022 21:46:56 +0900
From: torikoshia <torikoshia@oss.nttdata.com>
To: Damir Belyalov <dam.bel07@gmail.com>
Cc: PostgreSQL Hackers <pgsql-hackers@postgresql.org>,
 pavel.stehule@gmail.com
Subject: Re: POC PATCH: copy from ... exceptions to: (was Re: VLDB Features)
In-Reply-To: 
 <CALH1Lgu0dqsQUm9QYdoSvgu-O9Ac5OsbE3TxxZqChUrVF+Qrjw@mail.gmail.com>
References: <1197371508.4255.1327.camel@ebony.site>
 <1197660423.1536.8.camel@dell.linuxdev.us.dell.com>
 <15007.1197674561@sss.pgh.pa.us>
 <200712141830.16877.trent_shipley@qwest.net>
 <87k31ftoe0.fsf_-_@commandprompt.com>
 <CAFj8pRC3ksKhaSxDUo3Y1DekvV12OcEERryT8gPGdUAVEe2SLg@mail.gmail.com>
 <CAFj8pRD9Pi9besMvv0Yo0A4wqQ-D=cBXGRQhz_oS+BNgv_41_Q@mail.gmail.com>
 <CALH1LgsKDk4XthrM4m8tAqfoPyNgDCSFV3Go1nevXze-8dwKrA@mail.gmail.com>
 <CAFj8pRC96C+C6Gg8OHkv26YZMQsrRJ9YtJhBKrrrje1hc2yrcw@mail.gmail.com>
 <CALH1LgvmNcnO8dYyckcEmiJ6PGDnpRWA3V1td1SDKzqt6FrMJw@mail.gmail.com>
 <e78a86704ab2b2b967f0d674e5a72643@oss.nttdata.com>
 <CALH1Lgu0dqsQUm9QYdoSvgu-O9Ac5OsbE3TxxZqChUrVF+Qrjw@mail.gmail.com>
User-Agent: Roundcube Webmail/1.4.11
Message-ID: <dc876e926f284ee60eef4ece755fa419@oss.nttdata.com>
Archived-At: 
 <https://www.postgresql.org/message-id/dc876e926f284ee60eef4ece755fa419%40oss.nttdata.com>
Precedence: bulk

On 2022-08-15 22:23, Damir Belyalov wrote:
>> I expected I could see 9999 rows after COPY, but just saw 999 rows.
> Also I implemented your case and it worked correctly.

Thanks for the new patch!

Here are some comments on it.

> +           if (safecstate->saved_tuples < REPLAY_BUFFER_SIZE)
> +           {
> +               valid_row = NextCopyFrom(cstate, econtext, values, 
> nulls);
> +               if (valid_row)
> +               {
> +                   /* Fill replay_buffer in oldcontext*/
> +                   MemoryContextSwitchTo(safecstate->oldcontext);
> +                   
> safecstate->replay_buffer[safecstate->saved_tuples++] = 
> heap_form_tuple(RelationGetDescr(cstate->rel), values, nulls);
> 
> +               /* Buffer was filled, commit subtransaction and prepare 
> to replay */
> +               ReleaseCurrentSubTransaction();

What is actually being committed by this ReleaseCurrentSubTransaction()?
It seems to me that just safecstate->replay_buffer is fulfilled before 
this commit.
As a test, I rewrote this ReleaseCurrentSubTransaction() to 
RollbackAndReleaseCurrentSubTransaction() and COPYed over 1000 rows of 
data, but same data were loaded.

> +#define            REPLAY_BUFFER_SIZE 1000

I feel it might be better to have it as a parameter rather than fixed at 
1000.

> +/*
> + * Analog of NextCopyFrom() but ignore rows with errors while copying.
> + */
> +static bool
> +safeNextCopyFrom(CopyFromState cstate, ExprContext *econtext, Datum 
> *values, bool *nulls)

NextCopyFrom() is in copyfromparse.c while safeNextCopyFrom() is in 
copyfrom.c.
Since safeNextCopyFrom() is analog of NextCopyFrom() as commented, would 
it be natural to put them in the same file?

> 188 +               safecstate->errors++;
> 189 +               if (safecstate->errors <= 100)
> 190 +                   ereport(WARNING,
> 191 +                           (errcode(errdata->sqlerrcode),
> 192 +                           errmsg("%s", errdata->context)));

It would be better to state in the manual that errors exceeding 100 are 
not displayed.
Or, it might be acceptable to output all errors that exceed 100.

> +typedef struct SafeCopyFromState
> +{
> +#define            REPLAY_BUFFER_SIZE 1000
> +   HeapTuple       replay_buffer[REPLAY_BUFFER_SIZE]; /* accumulates 
> tuples for replaying it after an error */
> +   int             saved_tuples;               /* # of tuples in 
> replay_buffer */
> +   int             replayed_tuples;            /* # of tuples was 
> replayed from buffer */
> +   int             errors;                     /* total # of errors */
> +   bool            replay_is_active;
> +   bool            begin_subtransaction;
> +   bool            processed_remaining_tuples; /* for case of 
> replaying last tuples */
> +   bool            skip_row;

It would be helpful to add comments about skip_row, etc.

```
$ git apply ../patch/0003-COPY_IGNORE_ERRORS.patch

../patch/0003-COPY_IGNORE_ERRORS.patch:86: indent with spaces.
					         Datum *values, bool *nulls);
warning: 1 line adds whitespace errors.
```

There was a warning when applying the patch.


```
=# copy test from '/tmp/10000.data' with (ignore_errors);
WARNING:  FIND 0 ERRORS
COPY 1003
```

When there were no errors, this WARNING seems not necessary.


-- 
Regards,

--
Atsushi Torikoshi
NTT DATA CORPORATION