Re: dropdb --force
Also works fine according to my testing. Documentation is also clear. Thanks for this useful patch.
Re: dropdb --force
Hi, patch no longer applies (as of 12beta2). postgres@ubuntudev:~/pg_testing/source/postgresql-12beta2$ patch -p1 < drop-database-force-20190310_01.patch patching file doc/src/sgml/ref/drop_database.sgml patching file doc/src/sgml/ref/dropdb.sgml patching file src/backend/commands/dbcommands.c Hunk #1 succeeded at 489 (offset 2 lines). Hunk #2 succeeded at 779 (offset 2 lines). Hunk #3 succeeded at 787 (offset 2 lines). Hunk #4 succeeded at 871 (offset 2 lines). Hunk #5 succeeded at 1056 (offset 2 lines). Hunk #6 succeeded at 1186 (offset 2 lines). patching file src/backend/nodes/copyfuncs.c Hunk #1 succeeded at 3852 (offset 10 lines). patching file src/backend/nodes/equalfuncs.c Hunk #1 succeeded at 1666 (offset 3 lines). patching file src/backend/parser/gram.y Hunk #1 succeeded at 10194 (offset 45 lines). Hunk #2 succeeded at 10202 (offset 45 lines). patching file src/backend/storage/ipc/procarray.c Hunk #1 succeeded at 2906 (offset -14 lines). Hunk #2 succeeded at 2948 (offset -14 lines). patching file src/backend/tcop/utility.c patching file src/bin/scripts/dropdb.c Hunk #1 succeeded at 34 (offset 1 line). Hunk #2 succeeded at 50 (offset 1 line). Hunk #3 succeeded at 63 (offset 2 lines). Hunk #4 succeeded at 88 (offset 2 lines). Hunk #5 succeeded at 128 (offset 2 lines). Hunk #6 succeeded at 136 (offset 2 lines). Hunk #7 succeeded at 164 (offset 1 line). patching file src/bin/scripts/t/050_dropdb.pl patching file src/include/commands/dbcommands.h patching file src/include/nodes/parsenodes.h Hunk #1 succeeded at 3133 (offset 16 lines). patching file src/include/storage/procarray.h Hunk #1 FAILED at 112. 1 out of 1 hunk FAILED -- saving rejects to file src/include/storage/procarray.h.rej Could you please update it ? Thanks. Anthony The new status of this patch is: Waiting on Author
Re: progress report for ANALYZE
Hi,
In monitoring.sgml, "a" is missing in "row for ech backend that is
currently running that command[...]".
Anthony
On Tuesday, July 2, 2019, Julien Rouhaud wrote:
> On Fri, Jun 21, 2019 at 8:52 PM Alvaro Herrera
wrote:
>>
>> Here's a patch that implements progress reporting for ANALYZE.
>
> Patch applies, code and doc and compiles cleanly. I have few comments:
>
> @@ -512,7 +529,18 @@ do_analyze_rel(Relation onerel, VacuumParams *params,
> if (numrows > 0)
> {
> MemoryContext col_context,
> - old_context;
> + old_context;
> + const int index[] = {
> + PROGRESS_ANALYZE_PHASE,
> + PROGRESS_ANALYZE_TOTAL_BLOCKS,
> + PROGRESS_ANALYZE_BLOCKS_DONE
> + };
> + const int64 val[] = {
> + PROGRESS_ANALYZE_PHASE_ANALYSIS,
> + 0, 0
> + };
> +
> + pgstat_progress_update_multi_param(3, index, val);
> [...]
> }
> + pgstat_progress_update_param(PROGRESS_ANALYZE_PHASE,
> +PROGRESS_ANALYZE_PHASE_COMPLETE);
> +
> If there wasn't any row but multiple blocks were scanned, the
> PROGRESS_ANALYZE_PHASE_COMPLETE will still show the informations about
> the blocks that were scanned. I'm not sure if we should stay
> consistent here.
>
> diff --git a/src/backend/utils/adt/pgstatfuncs.c
> b/src/backend/utils/adt/pgstatfuncs.c
> index 05240bfd14..98b01e54fa 100644
> --- a/src/backend/utils/adt/pgstatfuncs.c
> +++ b/src/backend/utils/adt/pgstatfuncs.c
> @@ -469,6 +469,8 @@ pg_stat_get_progress_info(PG_FUNCTION_ARGS)
> /* Translate command name into command type code. */
> if (pg_strcasecmp(cmd, "VACUUM") == 0)
> cmdtype = PROGRESS_COMMAND_VACUUM;
> + if (pg_strcasecmp(cmd, "ANALYZE") == 0)
> + cmdtype = PROGRESS_COMMAND_ANALYZE;
> else if (pg_strcasecmp(cmd, "CLUSTER") == 0)
> cmdtype = PROGRESS_COMMAND_CLUSTER;
> else if (pg_strcasecmp(cmd, "CREATE INDEX") == 0)
>
> it should be an "else if" here.
>
> Everything else LGTM.
>
>
>
Re: Conflict handling for COPY FROM
Hi,
I'm very interested in this patch and would like to give a review within a
week. On the feature side, how about simply using the less verbose "ERRORS"
instead of "ERROR LIMIT" ?
On Wed, Jul 3, 2019 at 1:42 PM Surafel Temesgen
wrote:
> Hi Alexey,
> Thank you for looking at it
>
> On Tue, Jul 2, 2019 at 7:57 PM Alexey Kondratov <
> a.kondra...@postgrespro.ru> wrote:
>
>> On 28.06.2019 16:12, Alvaro Herrera wrote:
>> >> On Wed, Feb 20, 2019 at 7:04 PM Andres Freund
>> wrote:
>> >>> Or even just return it as a row. CopyBoth is relatively widely
>> supported
>> >>> these days.
>> >> i think generating warning about it also sufficiently meet its propose
>> of
>> >> notifying user about skipped record with existing logging facility
>> >> and we use it for similar propose in other place too. The different
>> >> i see is the number of warning that can be generated
>> > Warnings seem useless for this purpose. I'm with Andres: returning rows
>> > would make this a fine feature. If the user wants the rows in a table
>> > as Andrew suggests, she can use wrap the whole thing in an insert.
>>
>> I agree with previous commentators that returning rows will make this
>> feature more versatile.
>
>
> I agree. am looking at the options
>
> Also, I would prefer having an option to ignore all errors, e.g. with
>> option ERROR_LIMIT set to -1. Because it is rather difficult to estimate
>> a number of future errors if you are playing with some badly structured
>> data, while always setting it to 100500k looks ugly.
>>
>>
> Good idea
>
> I also +1 having an option to ignore all errors. Other RDBMS might use a
large number, but "-1" seems cleaner so far.
>
>
>> 1) Calculation of processed rows isn't correct (I've checked). You do it
>> in two places, and
>>
>> -processed++;
>> +if (!cstate->error_limit)
>> +processed++;
>>
>> is never incremented if ERROR_LIMIT is specified and no errors
>> occurred/no constraints exist, so the result will always be 0. However,
>> if primary column with constraints exists, then processed is calculated
>> correctly, since another code path is used:
>>
>>
> Correct. i will fix
>
> +if (specConflict)
>> +{
>> +...
>> +}
>> +else
>> +processed++;
>>
>> I would prefer this calculation in a single place (as it was before
>> patch) for simplicity and in order to avoid such problems.
>>
>>
> ok
>
>
>> 2) This ExecInsertIndexTuples call is only executed now if ERROR_LIMIT
>> is specified and was exceeded, which doesn't seem to be correct, does it?
>>
>> -if (resultRelInfo->ri_NumIndices > 0)
>> +if (resultRelInfo->ri_NumIndices > 0 &&
>> cstate->error_limit == 0)
>> recheckIndexes =
>> ExecInsertIndexTuples(myslot,
>>
>>
> No it alwase executed . I did it this way to avoid
> inserting index tuple twice but i see its unlikely
>
>
>> 3) Trailing whitespaces added to error messages and tests for some reason:
>>
>> +ereport(WARNING,
>> +(errcode(ERRCODE_BAD_COPY_FILE_FORMAT),
>> + errmsg("skipping \"%s\" --- missing data
>> for column \"%s\" ",
>>
>> +ereport(ERROR,
>> +(errcode(ERRCODE_BAD_COPY_FILE_FORMAT),
>> + errmsg("missing data for column \"%s\" ",
>>
>> -ERROR: missing data for column "e"
>> +ERROR: missing data for column "e"
>> CONTEXT: COPY x, line 1: "20002302323"
>>
>> -ERROR: missing data for column "e"
>> +ERROR: missing data for column "e"
>> CONTEXT: COPY x, line 1: "2001231\N\N"
>>
>>
>
> regards
> Surafel
>
Thanks,
Anthony
Re: Conflict handling for COPY FROM
Hi, sorry for answering a bit later than I hoped. Here is my review so far: Contents == This patch starts to address in my opinion one of COPY's shortcoming, which is error handling. PK and exclusion errors are taken care of, but not (yet?) other types of errors. Documentation is updated, "\h copy" also and some regression tests are added. Initial Run === Patch applies (i've tested v6) cleanly. make: OK make install: OK make check: OK make installcheck: OK Performance I've tested the patch on a 1.1G file with 10 000 000 lines. Each test was done 15 times on a small local VM. Table is without constraints. head: 38,93s head + patch: 38,76s Another test was one a 0.1GB file with 1 000 000 lines. Each test done 10 times on a small local VM and the table has a pk. COPY4,550s COPY CONFLICT 4,595s COPY CONFLICT with only one pk error 10,529s COPY CONFLICT pk error every 100 lines 10,859s COPY CONFLICT pk error every 1000 lines10,879s I did not test exclusions so far. Thoughts == I find the feature useful in itself. One big question for me is can it be improved later on to handle other types of errors (like check constraints for example) ? A "-1" for the error limit would be very useful in my opinion. I am also afraid that the name "error_limit" might mislead users into thinking that all error types are handled. But I do not have a better suggestion without making this clause much longer... I've had a short look at the code, but this will need review by someone else. Anyway, thanks a lot for taking the time to work on it. Anthony On Sun, Jul 14, 2019 at 3:48 AM Thomas Munro wrote: > On Fri, Jul 12, 2019 at 1:42 AM Surafel Temesgen > wrote: > > Here are the patch that contain all the comment given except adding a > way to specify > > to ignoring all error because specifying a highest number can do the > work and may be > > try to store such badly structure data is a bad idea > > Hi Surafel, > > FYI GCC warns: > > copy.c: In function ‘CopyFrom’: > copy.c:3383:8: error: ‘dest’ may be used uninitialized in this > function [-Werror=maybe-uninitialized] > (void) dest->receiveSlot(myslot, dest); > ^ > copy.c:2702:16: note: ‘dest’ was declared here > DestReceiver *dest; > ^ > > -- > Thomas Munro > https://enterprisedb.com > > >
