Re: Calendar support in localization
On Tue, Mar 30, 2021 at 11:16 AM Daniel Verite wrote: > > The conversions from julian dates are not necessarily hard, but the > I/O functions means having localized names for all days, months, eras > of all calendars in all supported languages. If you're thinking of > implementing this from scratch (without the ICU dependency), where > would these names come from? OTOH if we're using ICU, then why > bother reinventing the julian-to-calendars conversions that ICU > already does? > > i donno why but currently we are using our own function for converting (see j2date and date2j) maybe it's written before ICU but i think ICU helps in adding other calendar support easly. Regarding I/O functions postgresql hard coded days and months names on array and just parse and string compare, if it is not on the list then error(see datetime.c) and it will be the same for other calendar but i think we don't need all that if we use ICU regards Surafel
Re: Calendar support in localization
Hi Daniel, On Fri, Mar 26, 2021 at 8:51 PM Daniel Verite wrote: > Thomas Munro wrote: > > > Right, so if this is done by trying to extend Daniel Verite's icu_ext > > extension (link given earlier) and Robert's idea of a fast-castable > > type, I suppose you might want now()::icu_date + '1 month'::internal > > to advance you by one Ethiopic month if you have done SET > > icu_ext.ICU_LC_TIME = 'am_ET@calendar=traditional'. > > I've pushed a calendar branch on icu_ext [1] with preliminary support > for non-gregorian calendars through ICU, so far with only format and parse > of timetamptz. > Thanks > > > I understand that adding months or years with some of the non-gregorian > calendars should lead to different points in time than with the gregorian > calendar. > > For instance with the ethiopic calendar, the query above displays today as > 17/mägabit/2013 and 1 month from now as 18/miyazya/2013, > while the correct result is probably 17/miyazya/2013 (?) > > yes it should be 17/miyazya/2013 (?) > I'm not sure at this point that there should be a new set of > data/interval/timestamp types though, especially if considering > the integration in core. > > About intervals, if there were locale-aware functions like > add_interval(timestamptz, interval [, locale]) returns timestamptz > or > sub_timestamp(timestamptz, timestamptz [,locale]) returns interval > that would use ICU to compute the results according to the locale, > wouldn't it be good enough? > > Yes it can be enough for now but there are patches proposed to support the system and application time period which are in SQL standard and if we have that feature the calendar has to be in core and It doesn't appear hard for me to support the calendar locally. Postgresql itself does't store Gregorian date it stores julian date(which is more accurate than gregorian calendar) and almost all of function and operator is done using julian date converted to second(TimestampTz) so what it takes to support calendar locally is input/output function and a converter from and to julian calendar and that may not be that much hard since most of the world calendar is based on julian or gregorian calendar[0]. Thought? 0.https://en.wikipedia.org/wiki/List_of_calendars regards Surafel
Re: Calendar support in localization
On Wed, Mar 17, 2021 at 3:39 PM Thomas Munro wrote: > On Thu, Mar 18, 2021 at 3:48 AM Tom Lane wrote: > > Right, so if this is done by trying to extend Daniel Verite's icu_ext > extension (link given earlier) and Robert's idea of a fast-castable > type, I suppose you might want now()::icu_date + '1 month'::internal > to advance you by one Ethiopic month if you have done SET > icu_ext.ICU_LC_TIME = 'am_ET@calendar=traditional'. Or if using my > first idea of just sticking with the core types, perhaps you'd have to > replace stuff via search path... I admit that sounds rather error > prone and fragile (I was thinking mainly of different functions, not > operators). Either way, I suppose there'd also be more explicit > functions for various operations including ones that take an extra > argument if you want to use an explicit locale instead of relying on > the ICU_LC_TIME setting. I dunno. > > As you know internally timestamptz data type does't existe instead it stored as integer kind and we depend on operating system and external library for our date data type support so i think that put as on the position for not be the first one to implement timestamptz data type thing and i don't know who give as the casting for free? regards Surafel
Re: Calendar support in localization
On Tue, Mar 16, 2021 at 12:20 PM Thomas Munro wrote: > On Wed, Mar 17, 2021 at 6:31 AM Surafel Temesgen > wrote: > > Ethiopice calendar have 13 months so it can not be stored as date and > timestamp type and you approach seems more complicated and i suggest to > have this feature on the purpose of PostgreSQL popularities too not only > for my need > > I know, but the DATE and TIMESTAMPTZ datatypes don't intrinsically > know anything about months or other calendar concepts. Internally, > they are just a single number that counts the number of days or > seconds since an arbitrary epoch time. We are all in agreement about > how many times the Earth has rotated since then*. The calendar > concepts such as "day", "month", "year", whether Gregorian, Ethiopic, > Islamic, ... are all derivable from those numbers, if you know the > rules. > > Okay > > > > I think you suggesting this by expecting the implementation is difficult > but it's not that much difficult once you fully understand Gregorian > calendar and the calendar you work on > > Yeah, I am sure it's all just a bunch of simple integer maths. But > I'm talking about things like software architecture, maintainability, > cohesion, and getting maximum impact for the work we do. > > I may be missing some key detail though: why do you think it should be > a different type? The two reasons I can think of are: (1) the > slightly tricky detail that the date apparently changes at 1:00am > (which I don't think is a show stopper for this approach, I could > elaborate), (2) you may want dates to be formatted on the screen with > the Ethiopic calendar in common software like psql and GUI clients, > which may be easier to arrange with different types, but that seems to > be a cosmetic thing that could eventually be handled with tighter > locale integration with ICU. In the early stages you'd access > calendar logic though special functions with names like > icu_format_date(), or whatever. > > As you mention above whatever the calendar type is we ended up storing an integer that represent the date so rather than re-implementing every function and operation for every calendar we can use existing Gerigorian implementation as a base implementation and if new calendar want to perform same function or operation it translate to Gregorian one and use the existing function and operation and translate to back to working calendar. In this approach the only function we want for supporting a new calendar is a translator from the new calendar to Gregorian one and from Gerigorian calendar to the new calendar and may be input/ output function. What do you think of this implementation? regards Surafel
Re: Calendar support in localization
Hi Thomas
On Mon, Mar 15, 2021 at 2:58 PM Thomas Munro wrote:
>
> One key question here is whether you need a different date type or
> just different operations (functions, operators etc) on the existing
> types.
>
>
I am thinking of having a converter to a specific calendar after each
operation and function for display or storage. It works on
Ethiopice calendar and i expect it will work on other calendar too
> > I cc Thomas Munro and Vik because they have interest on this area
>
> Last time it came up[1], I got as far as wondering if the best way
> would be to write a set of ICU-based calendar functions. Would it be
> enough for your needs to have Ethiopic calendar-aware date arithmetic
> (add, subtract a month etc), date part extraction (get the current
> Ethiopic day/month/year of a date), display and parsing, and have all
> of these as functions that you have to call explicitly, but have them
> take the standard built-in date and timestamp types, so that your
> tables would store regular date and timestamp values? If not, what
> else do you need?
>
>
Ethiopice calendar have 13 months so it can not be stored as date and
timestamp type and you approach seems more complicated and i suggest to
have this feature on the purpose of PostgreSQL popularities too not only
for my need
> ICU is very well maintained and widely used software, and PostgreSQL
> already depends on it optionally, and that's enabled in all common
> distributions. In other words, maybe all the logic you want exists
> already in your process's memory, we just have to figure out how to
> reach it from SQL. Another reason to use ICU is that we can solve
> this problem once and then it'll work for many other calendars.
>
>
Each calendar-aware date arithmetic is different so solving one calendar
problem didn't help on other calendar
> > Please don't suggests to fork from PostgreSQL just for this feature
>
> I would start with an extension, and I'd try to do a small set of
> simple functions, to let me write things like:
>
> icu_format(now(), 'fr_FR@calendar=buddhist') to get a Buddhist
> calendar with French words
>
> icu_date_part('year', current_date, 'am_ET@calendar=traditional') to
> get the current year in the Ethiopic calendar (2013 apparently)
>
> Well, the first one probably also needs a format string too, actual
> details to be worked out by reading the ICU manual...
>
I think you suggesting this by expecting the implementation is difficult
but it's not that much difficult once you fully understand Gregorian
calendar and the calendar you work on
>
> Maybe instead of making a new extension, I might try to start from
> https://github.com/dverite/icu_ext and see if it makes sense to extend
> it to cover calendars.
>
> Maybe one day ICU will become a hard dependency of PostgreSQL and
> someone will propose all that stuff into core, and then maybe we could
> start to think about the possibility of tighter integration with the
> built-in date/time functions (and LC_TIME setting? seems complicated,
> see also problems with setting LC_COLLATE/datcollate to an ICU
> collation name, but I digress and that's a far off problem). I would
> also study the SQL standard and maybe DB2 (highly subjective comment:
> at a wild guess, the most likely commercial RDBMS to have done a good
> job of this if anyone has) to see if they contemplate non-Gregorian
> calendars, to get some feel for whether that would eventually be a
> possibility to conform with whatever the standard says.
>
> In summary, getting something of very high quality by using a widely
> used open source library that we already use seems like a better plan
> than trying to write and maintain our own specialist knowledge about
> individual calendars. If there's something you need that can't be
> done with its APIs working on top of our regular date and timestamp
> types, could you elaborate?
>
> [1]
> https://www.postgresql.org/message-id/flat/CA%2BhUKG%2BybW0LJuLtj3yAUsbOw3DrzK00pGk8JyfpCREzi_LSsg%40mail.gmail.com#393d827f1be589d0ad6ca6b016905e80
I don't know how you see this but for me the feature deserves a specialist
and it is not that much difficult to have one because i guess every majore
calendar have english documentation
regards
Surafel
Calendar support in localization
Hi all, My country(Ethiopia) is one of the nations that uses different kind of calendar than what PostgreSQL have so we are deprived from the benefit of data datatype. We just uses String to store date that limits our application quality greatly. The lag became even worst once application and system time support is available and it seems to me it is not fair to suggest to add other date data type kind and implementation for just different calendar that even not minor user group. Having calendar support to localization will be very very very very exciting feature for none Gregorian calendar user group and make so loved. As far as i can see the difficult thing is understanding different calendar. I can prepare a patch for Ethiopian calendar once we have consensus. I cc Thomas Munro and Vik because they have interest on this area Please don't suggests to fork from PostgreSQL just for this feature regards Surafel
Re: WIP: System Versioned Temporal Table
On Wed, Mar 10, 2021 at 9:02 AM Vik Fearing wrote: > > I have plenty of objection. I'm sorry that I am taking so long with my > review. I am still working on it and it is coming soon, I promise. > > okay take your time regards Surafel
Re: Evaluate expression at planning time for two more cases
Hi Ibrar, On Mon, Mar 8, 2021 at 8:13 AM Ibrar Ahmed wrote: > > It was a minor change therefore I rebased the patch, please take a look. > It is perfect thank you regards Surafel
Re: WIP: System Versioned Temporal Table
hi Ibrar, thank you for rebasing On Mon, Mar 8, 2021 at 9:34 AM Ibrar Ahmed wrote: > >> Since the get_row_start_time_col_name() and get_row_end_time_col_name() >> are similar, IMO we can pass a flag to get StartTime/EndTime column name, >> thought? >> >> For me your option is better. i will change to it in my next patch if no objection regards Surafel
Re: FETCH FIRST clause PERCENT option
On Mon, Jan 25, 2021 at 2:39 PM Kyotaro Horiguchi wrote: > Sorry for the dealy. I started to look this. > > Hi kyotaro, Thanks for looking into this but did we agree to proceed on this approach? I fear that it will be the west of effort if Andrew comes up with the patch for his approach. Andrew Gierth: Are you working on your approach? regards Surafel
Re: WIP: System Versioned Temporal Table
On Mon, Jan 18, 2021 at 1:43 AM Vik Fearing wrote: > > This is not good, and I see that DROP SYSTEM VERSIONING also removes > these columns which is even worse. Please read the standard that you > are trying to implement! > > The standard states the function of ALTER TABLE ADD SYSTEM VERSIONING as "Alter a regular persistent base table to a system-versioned table" and system versioned table is described in the standard by two generated stored constraint columns and implemented as such. > I will do a more thorough review of the functionalities in this patch > (not necessarily the code) this week. > > Please do regards Surafel
Re: WIP: System Versioned Temporal Table
On Sat, Jan 16, 2021 at 10:12 PM Vik Fearing wrote: > > I haven't looked at this patch in a while, but I hope that ALTER TABLE t > ADD SYSTEM VERSIONING is not adding any columns. That is a bug if it does. > > Yes, that is how I implement it. I don't understand how it became a bug? regards Surafel
Re: WIP: System Versioned Temporal Table
On Fri, Jan 15, 2021 at 8:02 PM Simon Riggs wrote: > > There are no existing applications, so for PostgreSQL, it wouldn't be an > issue. > > Yes we don't have but the main function of ALTER TABLE foo ADD SYSTEM VERSIONING is to add system versioning functionality to existing application regards Surafel
Re: WIP: System Versioned Temporal Table
On Fri, Jan 15, 2021 at 12:22 AM Simon Riggs wrote: > SELECT * FROM foo FOR SYSTEM_TIME AS OF ... > should NOT include the Start and End timestamp columns > because this acts like a normal query just with a different snapshot > timestamp > > SELECT * FROM foo FOR SYSTEM_TIME BETWEEN x AND y > SHOULD include the Start and End timestamp columns > since this form of query can include multiple row versions for the > same row, so it makes sense to see the validity times > > One disadvantage of returning system time columns is it breaks upward compatibility. if an existing application wants to switch to system versioning it will break. regards Surafel
Re: WIP: System Versioned Temporal Table
On Fri, Jan 15, 2021 at 12:27 AM Simon Riggs wrote: > > Yes, I think it can. The current situation is that the Start or End is > set to the Transaction Start Timestamp. > So if t2 starts before t1, then if t1 creates a row and t2 deletes it > then we will have start=t1 end=t2, but t2 Your tests don't show that because it must happen concurrently. > We need to add an isolation test to show this, or to prove it doesn't > happen. > > Does MVCC allow that? i am not expert on MVCC but i don't think t2 can see the row create by translation started before itself regards Surafel
Re: WIP: System Versioned Temporal Table
Hi Ryan On Fri, Jan 8, 2021 at 7:50 PM Ryan Lambert wrote: > I prefer to have them hidden by default. This was mentioned up-thread > with no decision, it seems the standard is ambiguous. MS SQL appears to > have flip-flopped on this decision [1]. > > I will change it to hidden by default if there are no objection regards Surafel
Re: WIP: System Versioned Temporal Table
Hi Simon, Thank you for all the work you does On Mon, Jan 11, 2021 at 5:02 PM Simon Riggs wrote: > > > * Anomalies around use of CURRENT_TIMESTAMP are not discussed or resolved. > Probably need to add a test that end_timestamp > start_timestamp or ERROR, > which effectively enforces serializability. > > This scenario doesn't happen. There are no possibility of a record being deleted or updated before inserting > * No discussion, comments or tests around freezing and whether that > causes issues here > > This feature introduced no new issue regarding freezing. Adding the doc about the table size growth because of a retention of old record seems enough for me > > * ALTER TABLE needs some work, it's a bit klugey at the moment and > needs extra tests. > Should refuse DROP COLUMN on a system time column, but currently doesn't > > * UPDATE foo SET start_timestamp = DEFAULT should fail but currently > doesn't > > okay i will fix it regards Surafel
Re: WIP: System Versioned Temporal Table
Hi Andrew, On Fri, Jan 8, 2021 at 4:38 PM Andrew Dunstan wrote: > > On 1/8/21 7:33 AM, Simon Riggs wrote: > > > > * What happens if you ask for a future time? > > It will give an inconsistent result as it scans, so we should refuse a > > query for time > current_timestamp. > > > That seems like a significant limitation. Can we fix it instead of > refusing the query? > > Querying a table without system versioning with a value of non existent data returns no record rather than error out or have other behavior. i don't understand the needs for special treatment here regards Surafel
Re: WIP: System Versioned Temporal Table
Hi Ryan, On Fri, Dec 18, 2020 at 10:28 PM Ryan Lambert wrote: > On Thu, Nov 19, 2020 at 11:04 AM Surafel Temesgen > wrote: > > The docs have two instances of "EndtTime" that should be "EndTime". > Since my first language is not english i'm glad you find only this error on doc. I will send rebased pach soon regards Surafel
Re: Evaluate expression at planning time for two more cases
Hi Pavel Borisov, It's always good to select the optimal way even if it didn't have performance gain but in case of this patch i see 4x speed up on my laptop and it will work on any table that have NULL constraint regards Surafel
Re: FETCH FIRST clause PERCENT option
Hi Michael
On Thu, Sep 24, 2020 at 6:58 AM Michael Paquier wrote:
> On Mon, Aug 10, 2020 at 01:23:44PM +0300, Surafel Temesgen wrote:
> > I also Implement PERCENT WITH TIES option. patch is attached
> > i don't start a new tread because the patches share common code
>
> This fails to apply per the CF bot. Could you send a rebase?
> --
>
Attaches are rebased patches
regards
Surafel
diff --git a/contrib/postgres_fdw/postgres_fdw.c b/contrib/postgres_fdw/postgres_fdw.c
index a31abce7c9..135b98da5d 100644
--- a/contrib/postgres_fdw/postgres_fdw.c
+++ b/contrib/postgres_fdw/postgres_fdw.c
@@ -3096,7 +3096,8 @@ estimate_path_cost_size(PlannerInfo *root,
if (fpextra && fpextra->has_limit)
{
adjust_limit_rows_costs(, _cost, _cost,
- fpextra->offset_est, fpextra->count_est);
+ LIMIT_OPTION_COUNT, fpextra->offset_est,
+ fpextra->count_est);
retrieved_rows = rows;
}
}
diff --git a/doc/src/sgml/ref/select.sgml b/doc/src/sgml/ref/select.sgml
index b93e4ca208..9811380ec6 100644
--- a/doc/src/sgml/ref/select.sgml
+++ b/doc/src/sgml/ref/select.sgml
@@ -44,7 +44,7 @@ SELECT [ ALL | DISTINCT [ ON ( expressionexpression [ ASC | DESC | USING operator ] [ NULLS { FIRST | LAST } ] [, ...] ]
[ LIMIT { count | ALL } ]
[ OFFSET start [ ROW | ROWS ] ]
-[ FETCH { FIRST | NEXT } [ count ] { ROW | ROWS } { ONLY | WITH TIES } ]
+[ FETCH { FIRST | NEXT } [ count ] [ PERCENT ] { ROW | ROWS } { ONLY | WITH TIES } ]
[ FOR { UPDATE | NO KEY UPDATE | SHARE | KEY SHARE } [ OF table_name [, ...] ] [ NOWAIT | SKIP LOCKED ] [...] ]
where from_item can be one of:
@@ -1434,7 +1434,7 @@ OFFSET start
which PostgreSQL also supports. It is:
OFFSET start { ROW | ROWS }
-FETCH { FIRST | NEXT } [ count ] { ROW | ROWS } { ONLY | WITH TIES }
+FETCH { FIRST | NEXT } [ count ] [ PERCENT ] { ROW | ROWS } { ONLY | WITH TIES }
In this syntax, the start
or count value is required by
@@ -1444,6 +1444,9 @@ FETCH { FIRST | NEXT } [ count ] {
ambiguity.
If count is
omitted in a FETCH clause, it defaults to 1.
+With PERCENT count specifies the maximum number of rows to return
+in percentage round up to the nearest integer. Using PERCENT
+is best suited to returning single-digit percentages of the query's total row count.
The WITH TIES option is used to return any additional
rows that tie for the last place in the result set according to
the ORDER BY clause; ORDER BY
diff --git a/src/backend/catalog/sql_features.txt b/src/backend/catalog/sql_features.txt
index b6e58e8493..68e0c5e2e1 100644
--- a/src/backend/catalog/sql_features.txt
+++ b/src/backend/catalog/sql_features.txt
@@ -344,7 +344,7 @@ F862 in subqueries YES
F863 Nested in YES
F864 Top-level in views YES
F865 in YES
-F866 FETCH FIRST clause: PERCENT option NO
+F866 FETCH FIRST clause: PERCENT option YES
F867 FETCH FIRST clause: WITH TIES option YES
R010 Row pattern recognition: FROM clause NO
R020 Row pattern recognition: WINDOW clause NO
diff --git a/src/backend/executor/nodeLimit.c b/src/backend/executor/nodeLimit.c
index d85cf7d93e..cfb28a9fd4 100644
--- a/src/backend/executor/nodeLimit.c
+++ b/src/backend/executor/nodeLimit.c
@@ -21,6 +21,8 @@
#include "postgres.h"
+#include
+
#include "executor/executor.h"
#include "executor/nodeLimit.h"
#include "miscadmin.h"
@@ -29,6 +31,8 @@
static void recompute_limits(LimitState *node);
static int64 compute_tuples_needed(LimitState *node);
+#define IsPercentOption(opt) \
+ (opt == LIMIT_OPTION_PERCENT || opt == LIMIT_OPTION_PER_WITH_TIES)
/*
* ExecLimit
@@ -53,6 +57,7 @@ ExecLimit(PlanState *pstate)
*/
direction = node->ps.state->es_direction;
outerPlan = outerPlanState(node);
+ slot = node->subSlot;
/*
* The main logic is a simple state machine.
@@ -82,7 +87,15 @@ ExecLimit(PlanState *pstate)
/*
* Check for empty window; if so, treat like empty subplan.
*/
- if (node->count <= 0 && !node->noCount)
+ if (IsPercentOption(node->limitOption))
+ {
+if (node->percent == 0.0)
+{
+ node->lstate = LIMIT_EMPTY;
+ return NULL;
+}
+ }
+ else if (node->count <= 0 && !node->noCount)
{
node->lstate = LIMIT_EMPTY;
return NULL;
@@ -118,6 +131,16 @@ ExecLimit(PlanState *pstate)
break;
}
+ /*
+ * We may needed this tuple in subsequent scan so put it into
+ * tuplestore.
+ */
+ if (IsPercentOption(node->limitOption))
+ {
+tuplestore_puttupleslot(node->tupleStore, slot);
+tuplestore_advance(node->tupleStore, true);
+ }
+
/*
* Okay, we have the first tuple of the window.
*/
@@ -135,6 +158,85 @@ ExecLimit(PlanState *pstate)
case LIMIT_INWIND
Re: [PATCH] distinct aggregates within a window function WIP
On Thu, Mar 5, 2020 at 4:17 AM Krasiyan Andreev wrote: > I have currently suspended development of this patch, based on it's > review, > but I will continue development of the other Oliver Ford's work about > adding support of respect/ignore nulls > for lag(),lead(),first_value(),last_value() and nth_value() and from > first/last for nth_value() patch, > but I am not sure how to proceed with it's implementation and any feedback > will be very helpful. > > * I applied your patch on top of 58c47ccfff20b8c125903 . It applied cleanly , compiled, make check pass, but it have white space errors: *Added functions on windowfuncs.c have no comments so it's not easily understandable. * Regression test addition seems huge to me. Can you reduce that? You can use existing tables and fewer records. * I don’t understand why this patch has to change makeBoolAConst? It already make “bool” constant node regards Surafel
Re: pg_dump --where option
On Fri, Jul 31, 2020 at 1:38 AM Daniel Gustafsson wrote:
>
> > $ pg_dump -d cary --where="test1:a3 = ( select max(aa1) from test2 )" >
> testdump2
> > $ pg_dump: error: processing of table "public.test1" failed
> >
> > both test1 and test2 exist in the database and the same subquery works
> under psql.
> >
>
This is because pg_dump uses schema-qualified object name I add
documentation about to use schema-qualified name when using sub query
> > I also notice that the regression tests for pg_dump is failing due to
> the patch, I think it is worth looking into the failure messages and also
> add some test cases on the new "where" clause to ensure that it can cover
> as many use cases as possible.
>
>
I fix regression test failure on the attached patch.
I don’t add tests because single-quotes and double-quotes are
meta-characters for PROVE too.
regards
Surafel
diff --git a/doc/src/sgml/ref/pg_dump.sgml b/doc/src/sgml/ref/pg_dump.sgml
index 0b2e2de87b..7dc3041247 100644
--- a/doc/src/sgml/ref/pg_dump.sgml
+++ b/doc/src/sgml/ref/pg_dump.sgml
@@ -1104,6 +1104,24 @@ PostgreSQL documentation
+
+ --where=table:filter_clause
+
+
+When dumping data for table, only include rows
+that meet the filter_clause condition.
+if --where contains subquery, uses schema-qualified name otherwise
+it is error because pg_dump uses schema-qualified object name to identifies the tables.
+This option is useful when you want to dump only a subset of a particular table.
+--where can be given more than once to provide different filters
+for multiple tables. Note that if multiple options refer to the same table,
+only the first filter_clause will be applied. If necessary, use quotes in your shell to
+provide an argument that contains spaces.
+E.g. --where=mytable:"created_at >= '2018-01-01' AND test = 'f'"
+
+
+
+
-?
--help
diff --git a/src/bin/pg_dump/pg_dump.c b/src/bin/pg_dump/pg_dump.c
index d3ca54e4dc..418684e272 100644
--- a/src/bin/pg_dump/pg_dump.c
+++ b/src/bin/pg_dump/pg_dump.c
@@ -121,6 +121,8 @@ static SimpleStringList tabledata_exclude_patterns = {NULL, NULL};
static SimpleOidList tabledata_exclude_oids = {NULL, NULL};
static SimpleStringList foreign_servers_include_patterns = {NULL, NULL};
static SimpleOidList foreign_servers_include_oids = {NULL, NULL};
+static SimpleStringList tabledata_where_patterns = {NULL, NULL};
+static SimpleOidList tabledata_where_oids = {NULL, NULL};
static const CatalogId nilCatalogId = {0, 0};
@@ -156,7 +158,8 @@ static void expand_foreign_server_name_patterns(Archive *fout,
static void expand_table_name_patterns(Archive *fout,
SimpleStringList *patterns,
SimpleOidList *oids,
- bool strict_names);
+ bool strict_names,
+ bool match_data);
static NamespaceInfo *findNamespace(Archive *fout, Oid nsoid);
static void dumpTableData(Archive *fout, TableDataInfo *tdinfo);
static void refreshMatViewData(Archive *fout, TableDataInfo *tdinfo);
@@ -387,6 +390,7 @@ main(int argc, char **argv)
{"on-conflict-do-nothing", no_argument, _nothing, 1},
{"rows-per-insert", required_argument, NULL, 10},
{"include-foreign-data", required_argument, NULL, 11},
+ {"where", required_argument, NULL, 12},
{NULL, 0, NULL, 0}
};
@@ -604,6 +608,10 @@ main(int argc, char **argv)
optarg);
break;
+ case 12:/* table data WHERE clause */
+simple_string_list_append(_where_patterns, optarg);
+break;
+
default:
fprintf(stderr, _("Try \"%s --help\" for more information.\n"), progname);
exit_nicely(1);
@@ -806,17 +814,26 @@ main(int argc, char **argv)
{
expand_table_name_patterns(fout, _include_patterns,
_include_oids,
- strict_names);
+ strict_names, false);
if (table_include_oids.head == NULL)
fatal("no matching tables were found");
}
+
+ if (tabledata_where_patterns.head != NULL)
+ {
+ expand_table_name_patterns(fout, _where_patterns,
+ _where_oids,
+ true, true);
+ if (tabledata_where_oids.head == NULL)
+ fatal("no matching table was found");
+ }
expand_table_name_patterns(fout, _exclude_patterns,
_exclude_oids,
- false);
+ false, false);
expand_table_name_patterns(fout, _exclude_patterns,
_exclude_oids,
- false);
+ false, false);
expand_foreign_server_name_patterns(fout, _servers_include_patterns,
_servers_include_oids);
@@ -1047,6 +1064,7 @@ help(const char *progname)
printf(_(" --use-set-session-authorization\n"
" use SET SESSION AUTHORIZATION commands instead of\n"
" ALTER OWNER commands to set ownership\n"));
+ printf(_(" --where=TABLE:WHERE_CLAUSE only dump selected rows for the given table\n"));
Re: Improvements in Copy From
On Thu, Sep 10, 2020 at 1:17 PM vignesh C wrote: > > > > > We have a patch for column matching feature [1] that may need a header > line to be further processed. Even without that I think it is preferable to > process the header line for nothing than adding those checks to the loop, > performance-wise. > > I had seen that patch, I feel that change to match the header if the > header is specified can be addressed in this patch if that patch gets > committed first or vice versa. We are doing a lot of processing for > the data which we need not do anything. Shouldn't this be skipped if > not required. Similar check is present in NextCopyFromRawFields also > to skip header. > The existing check is unavoidable but we can live better without the checks added by the patch. For very large files the loop may iterate millions of times if it is not in billion and I am sure doing the check that many times will incur noticeable performance degradation than further processing a single line. regards Surafel
Re: Evaluate expression at planning time for two more cases
On Tue, Sep 8, 2020 at 12:59 PM Surafel Temesgen
wrote:
> Hi Tom
>
> On Tue, Sep 8, 2020 at 4:46 AM Tom Lane wrote:
>
>
>> The "check_null_side" code you're proposing seems really horrid.
>> For one thing, it seems quite out of place for eval_const_expressions
>> to be doing that. For another, it's wrong in principle because
>> eval_const_expressions doesn't know which part of the query tree
>> it's being invoked on, so it cannot know whether outer-join
>> nullability is an issue. For another, doing that work over again
>> from scratch every time we see a potentially optimizable NullTest
>> looks expensive. (I wonder whether you have tried to measure the
>> performance penalty imposed by this patch in cases where it fails
>> to make any proof.)
>>
>>
> I was thinking about collecting data about joins only once at the start of
> eval_const_expressions but I assume most queries don't have NULL check
> expressions and postpone it until we find one. Thinking about it again I
> think it can be done better by storing check_null_side_state into
> eval_const_expressions_context to use it for subsequent evaluation.
>
>
Attached patch does like the above and includes NOT NULL constraint column.
regards
Surafel
diff --git a/src/backend/optimizer/util/clauses.c b/src/backend/optimizer/util/clauses.c
index 750586fceb..5fe4d88b5d 100644
--- a/src/backend/optimizer/util/clauses.c
+++ b/src/backend/optimizer/util/clauses.c
@@ -20,8 +20,10 @@
#include "postgres.h"
#include "access/htup_details.h"
+#include "access/table.h"
#include "catalog/pg_aggregate.h"
#include "catalog/pg_class.h"
+#include "catalog/pg_constraint.h"
#include "catalog/pg_language.h"
#include "catalog/pg_operator.h"
#include "catalog/pg_proc.h"
@@ -39,6 +41,7 @@
#include "optimizer/plancat.h"
#include "optimizer/planmain.h"
#include "parser/analyze.h"
+#include "parser/parsetree.h"
#include "parser/parse_agg.h"
#include "parser/parse_coerce.h"
#include "parser/parse_func.h"
@@ -50,6 +53,7 @@
#include "utils/fmgroids.h"
#include "utils/lsyscache.h"
#include "utils/memutils.h"
+#include "utils/rel.h"
#include "utils/syscache.h"
#include "utils/typcache.h"
@@ -61,6 +65,14 @@ typedef struct
AggClauseCosts *costs;
} get_agg_clause_costs_context;
+typedef struct check_null_side_state
+{
+ Relids relids; /* base relids within this subtree */
+ bool contains_outer; /* does subtree contain outer join(s)? */
+ JoinType jointype; /* type of join */
+ List *sub_states; /* List of states for subtree components */
+} check_null_side_state;
+
typedef struct
{
ParamListInfo boundParams;
@@ -68,6 +80,7 @@ typedef struct
List *active_fns;
Node *case_val;
bool estimate;
+ check_null_side_state *state;
} eval_const_expressions_context;
typedef struct
@@ -156,6 +169,9 @@ static Query *substitute_actual_srf_parameters(Query *expr,
int nargs, List *args);
static Node *substitute_actual_srf_parameters_mutator(Node *node,
substitute_actual_srf_parameters_context *context);
+static bool check_null_side(PlannerInfo *root, int relid, eval_const_expressions_context *context);
+static check_null_side_state * collect_jointree_data(Node *jtnode);
+
/*
@@ -2296,6 +2312,7 @@ eval_const_expressions(PlannerInfo *root, Node *node)
context.active_fns = NIL; /* nothing being recursively simplified */
context.case_val = NULL; /* no CASE being examined */
context.estimate = false; /* safe transformations only */
+ context.state= NULL;
return eval_const_expressions_mutator(node, );
}
@@ -2626,6 +2643,7 @@ eval_const_expressions_mutator(Node *node,
{
has_null_input |= ((Const *) lfirst(arg))->constisnull;
all_null_input &= ((Const *) lfirst(arg))->constisnull;
+
}
else
has_nonconst_input = true;
@@ -3382,7 +3400,52 @@ eval_const_expressions_mutator(Node *node,
return makeBoolConst(result, false);
}
+if (IsA(arg, Var) &&
+ ((Var *) arg)->varlevelsup == 0 && context->root)
+{
+ /*
+ * Evaluate the test if it is on NOT NULL Constraint column and the
+ * relation is not mentioned on nullable side of outer
+ * join
+ */
+ Var *var = (Var *) arg;
+ Query *parse = context->root->parse;
+ int relid;
+ RangeTblEntry *rte;
+ relid = var->varno;
+ rte = rt_fetch(relid, parse->rtable);
+ if (rte->relkind ==RELKIND_RELATION)
+ {
+ Relationrel;
+ Form_pg_attribute att;
+ rel = t
Re: Evaluate expression at planning time for two more cases
Hi Tom On Tue, Sep 8, 2020 at 4:46 AM Tom Lane wrote: > Surafel Temesgen writes: > > [ null_check_on_pkey_optimization_v1.patch ] > > I took a very brief look at this. > > > I don’t add NOT NULL constraint optimization to the patch because cached > > plan is not invalidation in case of a change in NOT NULL constraint > > That's actually not a problem, even though some people (including me) > have bandied about such suppositions in the past. Relying on attnotnull > in the planner is perfectly safe [1]. Plus it'd likely be cheaper as > well as more general than looking up pkey information. If we did need > to explicitly record the plan's dependency on a constraint, this patch > would be wrong anyhow because it fails to make any such notation about > the pkey constraint it relied on. > > ok thank you. I will change my next patch accordingly > The "check_null_side" code you're proposing seems really horrid. > For one thing, it seems quite out of place for eval_const_expressions > to be doing that. For another, it's wrong in principle because > eval_const_expressions doesn't know which part of the query tree > it's being invoked on, so it cannot know whether outer-join > nullability is an issue. For another, doing that work over again > from scratch every time we see a potentially optimizable NullTest > looks expensive. (I wonder whether you have tried to measure the > performance penalty imposed by this patch in cases where it fails > to make any proof.) > > I was thinking about collecting data about joins only once at the start of eval_const_expressions but I assume most queries don't have NULL check expressions and postpone it until we find one. Thinking about it again I think it can be done better by storing check_null_side_state into eval_const_expressions_context to use it for subsequent evaluation. I'm not sure what I think about Ashutosh's ideas about doing this > somewhere else than eval_const_expressions. I do not buy the argument > that it's interesting to do this separately for each child partition. > Child partitions that have attnotnull constraints different from their > parent's are at best a tiny minority use-case, if indeed we allow them > at all (I tend to think we shouldn't). On the other hand it's possible > that postponing the check would allow bypassing the outer-join problem, > ie if we only do it for quals that have dropped down to the relation > scan level then we don't need to worry about outer join effects. > > At eval_const_expressions we check every expression and optimize it if possible. Introducing other check and optimization mechanism to same other place just for this optimization seems expensive with respect to performance penalty to me > Another angle here is that eval_const_expressions runs before > reduce_outer_joins, meaning that if it's doing things that depend > on outer-join-ness then it will sometimes fail to optimize cases > that could be optimized. As a not incidental example, consider > > select ... from t1 left join t2 on (...) where t2.x is not null; > > reduce_outer_joins will realize that the left join can be reduced > to a plain join, whereupon (if t2.x is attnotnull) the WHERE clause > really is constant-true --- and this seems like a poster-child case > for it being useful to optimize away the WHERE clause. But > we won't be able to detect that if we apply the optimization during > eval_const_expressions. So maybe that's a good reason to do it > somewhere later. > In this case the expression not changed to constant-true because the relation is on nullable side of outer join regards Surafel
Re: proposal: possibility to read dumped table's name from file
Hi Pavel On Fri, Sep 4, 2020 at 6:22 AM Pavel Stehule wrote: > > Here is updated patch for pg_dump > > pg_dumpall also has –exclude-database=pattern and –no-comments option doesn't that qualify it to benefits from this feature? And please add a test case for this option regards Surafel
Re: Improvements in Copy From
Hi Vignesh On Wed, Jul 1, 2020 at 3:46 PM vignesh C wrote: > Hi, > > While reviewing copy from I identified few improvements for copy from > that can be done : > a) copy from stdin copies lesser amount of data to buffer even though > space is available in buffer because minread was passed as 1 to > CopyGetData, Hence it only reads until the data read from libpq is > less than minread. This can be fixed by passing the actual space > available in buffer, this reduces the unnecessary frequent calls to > CopyGetData. > why not applying the same optimization on file read ? > c) Copy from reads header line and do nothing for the header line, we > need not clear EOL & need not convert to server encoding for the > header line. > We have a patch for column matching feature [1] that may need a header line to be further processed. Even without that I think it is preferable to process the header line for nothing than adding those checks to the loop, performance-wise. [1]. https://www.postgresql.org/message-id/flat/caf1-j-0ptcwmeltswwgv2m70u26n4g33gpe1rckqqe6wvqd...@mail.gmail.com regards Surafel
Re: [PATCH] Allow queries in WHEN expression of FOR EACH STATEMENT triggers
Hi Joe,
This is my review of your patch
On Fri, Jul 17, 2020 at 1:22 AM Joe Wildish wrote:
> Hi hackers,
>
> Attached is a patch for supporting queries in the WHEN expression of
> statement triggers.
- Currently, WHEN expressions cannot contain
- subqueries.
subqueries in row trigger's is not supported in your patch so the the
documentation have to reflect it
+ UPDATE triggers are able to refer to both
OLD
+ and NEW
Opening and ending tag mismatch on UPDATE and OLD literal so documentation
build fails and please update the documentation on server programming
section too
+ /*
+ * Plan the statement. No need to rewrite as it can only refer to the
+ * transition tables OLD and NEW, and the relation which is being
+ * triggered upon.
+ */
+ stmt = pg_plan_query(query, trigger->tgqual, 0, NULL);
+ dest = CreateDestReceiver(DestTuplestore);
+ store = tuplestore_begin_heap(false, false, work_mem);
+ tupdesc = CreateTemplateTupleDesc(1);
+ whenslot = MakeSingleTupleTableSlot(tupdesc, );
Instead of planning every time the trigger fire I suggest to store plan or
prepared statement node so planning time can be saved
There are server crash on the following sequence of command
CREATE TABLE main_table (a int unique, b int);
CREATE FUNCTION trigger_func() RETURNS trigger LANGUAGE plpgsql AS '
BEGIN
RAISE NOTICE ''trigger_func(%) called: action = %, when = %, level = %'',
TG_ARGV[0], TG_OP, TG_WHEN, TG_LEVEL;
RETURN NULL;
END;';
INSERT INTO main_table DEFAULT VALUES;
CREATE TRIGGER after_insert AFTER INSERT ON main_table
REFERENCING NEW TABLE AS NEW FOR EACH STATEMENT
WHEN (500 <= ANY(SELECT b FROM NEW union SELECT a FROM main_table))
EXECUTE PROCEDURE trigger_func('after_insert');
INSERT INTO main_table (a, b) VALUES
(101, 498),
(102, 499);
server crashed
regards
Surafel
Re: Evaluate expression at planning time for two more cases
Hi , Thank you for looking into this On Fri, Aug 28, 2020 at 9:48 AM Ashutosh Bapat wrote: > } > else > has_nonconst_input = true; > @@ -3382,7 +3395,47 @@ eval_const_expressions_mutator(Node *node, > > + > +if (pkattnos != NULL && > bms_is_member(var->varattno - FirstLowInvalidHeapAttributeNumber, > pkattnos) > +&& !check_null_side(context->root, relid)) > > Since this is working on parse->rtable this will work only for top level > tables > as against the inherited tables or partitions which may have their own > primary > key constraints if the parent doesn't have those. > > In that case the table have to be specified in from clause otherwise its error e.g postgres=# CREATE TABLE cities ( name text, population float, altitude int ); CREATE TABLE postgres=# CREATE TABLE capitals ( id serial primary key, state char(2) ) INHERITS (cities); CREATE TABLE postgres=# EXPLAIN SELECT * FROM cities WHERE id is not null; ERROR: column "id" does not exist LINE 1: EXPLAIN SELECT * FROM cities WHERE id is not null; Even it will not work on the child table because the primary key constraint on the parent table is not in-force in the child table. > This better be done when planning individual relations, plain or join or > upper, > where all the necessary information is already available with each of the > relations and also the quals, derived as well as user specified, are > distributed to individual relations where they should be evalutated. My > memory > is hazy but it might be possible do this while distributing the quals > themselves (distribute_qual_to_rels()). > > The place where all the necessary information available is on reduce_outer_joins as the comment of the function states but the downside is its will only be inexpensive if the query contains outer join > Said that, to me, this looks more like something we should be able to do > at the > time of constraint exclusion. But IIRC, we just prove whether constraints > refute a qual and not necessarily whether constraints imply a qual, making > it > redundant, as is required here. E.g. primary key constraint implies key NOT > NULL rendering a "key IS NOT NULL" qual redundant. It might be better to > test > the case when col IS NOT NULL is specified on a column which already has a > NOT > NULL constraint. That may be another direction to take. We may require much > lesser code. > > I don’t add NOT NULL constraint optimization to the patch because cached plan is not invalidation in case of a change in NOT NULL constraint > Please add the patch to the next commitfest > https://commitfest.postgresql.org/. > > I add it is here https://commitfest.postgresql.org/29/2699/ Thank you regards Surafel
Evaluate expression at planning time for two more cases
Hi,
In good written query IS NULL and IS NOT NULL check on primary and non null
constraints columns should not happen but if it is mentioned PostgreSQL
have to be smart enough for not checking every return result about null
value on primary key column. Instead it can be evaluate its truth value and
set the result only once. The attached patch evaluate and set the truth
value for null and not null check on primary column on planning time if the
relation attribute is not mention on nullable side of outer join.
Thought?
regards
Surafel
diff --git a/src/backend/optimizer/util/clauses.c b/src/backend/optimizer/util/clauses.c
index 750586fceb..417758f705 100644
--- a/src/backend/optimizer/util/clauses.c
+++ b/src/backend/optimizer/util/clauses.c
@@ -22,6 +22,7 @@
#include "access/htup_details.h"
#include "catalog/pg_aggregate.h"
#include "catalog/pg_class.h"
+#include "catalog/pg_constraint.h"
#include "catalog/pg_language.h"
#include "catalog/pg_operator.h"
#include "catalog/pg_proc.h"
@@ -42,6 +43,7 @@
#include "parser/parse_agg.h"
#include "parser/parse_coerce.h"
#include "parser/parse_func.h"
+#include "parser/parsetree.h"
#include "rewrite/rewriteManip.h"
#include "tcop/tcopprot.h"
#include "utils/acl.h"
@@ -90,6 +92,14 @@ typedef struct
char *prosrc;
} inline_error_callback_arg;
+typedef struct check_null_side_state
+{
+ Relids relids; /* base relids within this subtree */
+ bool contains_outer; /* does subtree contain outer join(s)? */
+ JoinType jointype; /* type of join */
+ List *sub_states; /* List of states for subtree components */
+} check_null_side_state;
+
typedef struct
{
char max_hazard; /* worst proparallel hazard found so far */
@@ -156,6 +166,9 @@ static Query *substitute_actual_srf_parameters(Query *expr,
int nargs, List *args);
static Node *substitute_actual_srf_parameters_mutator(Node *node,
substitute_actual_srf_parameters_context *context);
+static bool check_null_side(PlannerInfo *root, int relid);
+static check_null_side_state * collect_jointree_data(Node *jtnode);
+
/*
@@ -2245,7 +2258,6 @@ rowtype_field_matches(Oid rowtypeid, int fieldnum,
return true;
}
-
/*
* eval_const_expressions
*
@@ -2626,6 +2638,7 @@ eval_const_expressions_mutator(Node *node,
{
has_null_input |= ((Const *) lfirst(arg))->constisnull;
all_null_input &= ((Const *) lfirst(arg))->constisnull;
+
}
else
has_nonconst_input = true;
@@ -3382,7 +3395,47 @@ eval_const_expressions_mutator(Node *node,
return makeBoolConst(result, false);
}
+if (IsA(arg, Var) &&
+ ((Var *) arg)->varlevelsup == 0 && context->root)
+{
+ /*
+ * Evaluate the test if it is on primary column and the
+ * relation is not mentioned on nullable side of outer
+ * join
+ */
+ Var *var = (Var *) arg;
+ Query *parse = context->root->parse;
+ int relid;
+ Bitmapset *pkattnos;
+ Oid constraintOid;
+ RangeTblEntry *rte;
+ relid = var->varno;
+ rte = rt_fetch(relid, parse->rtable);
+ pkattnos = get_primary_key_attnos(rte->relid, false, );
+
+ if (pkattnos != NULL && bms_is_member(var->varattno - FirstLowInvalidHeapAttributeNumber, pkattnos)
+ && !check_null_side(context->root, relid))
+ {
+ bool result;
+
+ switch (ntest->nulltesttype)
+ {
+ case IS_NULL:
+result = false;
+break;
+ case IS_NOT_NULL:
+result = true;
+break;
+ default:
+elog(ERROR, "unrecognized nulltesttype: %d",
+ (int) ntest->nulltesttype);
+result = false; /* keep compiler quiet */
+break;
+ }
+ return makeBoolConst(result, false);
+ }
+}
newntest = makeNode(NullTest);
newntest->arg = (Expr *) arg;
newntest->nulltesttype = ntest->nulltesttype;
@@ -3572,6 +3625,118 @@ eval_const_expressions_mutator(Node *node,
return ece_generic_processing(node);
}
+/*
+ * Check a relation attributes on nullable side of the outer join.
+ */
+static bool
+check_null_side(PlannerInfo *root, int relid)
+{
+ check_null_side_state *state;
+ ListCell *l;
+
+ state = collect_jointree_data((Node *) root->parse->jointree);
+
+ /* if no outer joins the relation is not on nullable side */
+ if (state == NULL || !state->contains_outer)
+ return false;
+
+ /* scan the state and check relation on nullable outer join side */
+ foreach(l, state->sub_states)
+ {
+ check_null_side_state *sub_state = (check_null_side_state *) lfirst(l);
+
+ if (sub_state->contains_outer)
+ {
+ if (sub_state->jointype == JOIN_LEFT)
+ {
+check_null_side_state *right_state = (check_null_side_state *) lsecond(sub_state->sub_states);
+
+if (bms_is_member(relid, right_state->relids))
+ return true;
+ }
+ if (sub_state->jointype == JOIN_RIGHT)
+ {
+
Re: FETCH FIRST clause PERCENT option
Hi
> PERCENT and WITH TIES can play together, per spec.
>
I also Implement PERCENT WITH TIES option. patch is attached
i don't start a new tread because the patches share common code
regards
Surafel
diff --git a/contrib/postgres_fdw/postgres_fdw.c b/contrib/postgres_fdw/postgres_fdw.c
index 9fc53cad68..d512f4ddd0 100644
--- a/contrib/postgres_fdw/postgres_fdw.c
+++ b/contrib/postgres_fdw/postgres_fdw.c
@@ -3097,7 +3097,8 @@ estimate_path_cost_size(PlannerInfo *root,
if (fpextra && fpextra->has_limit)
{
adjust_limit_rows_costs(, _cost, _cost,
- fpextra->offset_est, fpextra->count_est);
+ LIMIT_OPTION_COUNT, fpextra->offset_est,
+ fpextra->count_est);
retrieved_rows = rows;
}
}
diff --git a/doc/src/sgml/ref/select.sgml b/doc/src/sgml/ref/select.sgml
index b93e4ca208..9811380ec6 100644
--- a/doc/src/sgml/ref/select.sgml
+++ b/doc/src/sgml/ref/select.sgml
@@ -44,7 +44,7 @@ SELECT [ ALL | DISTINCT [ ON ( expressionexpression [ ASC | DESC | USING operator ] [ NULLS { FIRST | LAST } ] [, ...] ]
[ LIMIT { count | ALL } ]
[ OFFSET start [ ROW | ROWS ] ]
-[ FETCH { FIRST | NEXT } [ count ] { ROW | ROWS } { ONLY | WITH TIES } ]
+[ FETCH { FIRST | NEXT } [ count ] [ PERCENT ] { ROW | ROWS } { ONLY | WITH TIES } ]
[ FOR { UPDATE | NO KEY UPDATE | SHARE | KEY SHARE } [ OF table_name [, ...] ] [ NOWAIT | SKIP LOCKED ] [...] ]
where from_item can be one of:
@@ -1434,7 +1434,7 @@ OFFSET start
which PostgreSQL also supports. It is:
OFFSET start { ROW | ROWS }
-FETCH { FIRST | NEXT } [ count ] { ROW | ROWS } { ONLY | WITH TIES }
+FETCH { FIRST | NEXT } [ count ] [ PERCENT ] { ROW | ROWS } { ONLY | WITH TIES }
In this syntax, the start
or count value is required by
@@ -1444,6 +1444,9 @@ FETCH { FIRST | NEXT } [ count ] {
ambiguity.
If count is
omitted in a FETCH clause, it defaults to 1.
+With PERCENT count specifies the maximum number of rows to return
+in percentage round up to the nearest integer. Using PERCENT
+is best suited to returning single-digit percentages of the query's total row count.
The WITH TIES option is used to return any additional
rows that tie for the last place in the result set according to
the ORDER BY clause; ORDER BY
diff --git a/src/backend/catalog/sql_features.txt b/src/backend/catalog/sql_features.txt
index b6e58e8493..68e0c5e2e1 100644
--- a/src/backend/catalog/sql_features.txt
+++ b/src/backend/catalog/sql_features.txt
@@ -344,7 +344,7 @@ F862 in subqueries YES
F863 Nested in YES
F864 Top-level in views YES
F865 in YES
-F866 FETCH FIRST clause: PERCENT option NO
+F866 FETCH FIRST clause: PERCENT option YES
F867 FETCH FIRST clause: WITH TIES option YES
R010 Row pattern recognition: FROM clause NO
R020 Row pattern recognition: WINDOW clause NO
diff --git a/src/backend/executor/nodeLimit.c b/src/backend/executor/nodeLimit.c
index d85cf7d93e..cfb28a9fd4 100644
--- a/src/backend/executor/nodeLimit.c
+++ b/src/backend/executor/nodeLimit.c
@@ -21,6 +21,8 @@
#include "postgres.h"
+#include
+
#include "executor/executor.h"
#include "executor/nodeLimit.h"
#include "miscadmin.h"
@@ -29,6 +31,8 @@
static void recompute_limits(LimitState *node);
static int64 compute_tuples_needed(LimitState *node);
+#define IsPercentOption(opt) \
+ (opt == LIMIT_OPTION_PERCENT || opt == LIMIT_OPTION_PER_WITH_TIES)
/*
* ExecLimit
@@ -53,6 +57,7 @@ ExecLimit(PlanState *pstate)
*/
direction = node->ps.state->es_direction;
outerPlan = outerPlanState(node);
+ slot = node->subSlot;
/*
* The main logic is a simple state machine.
@@ -82,7 +87,15 @@ ExecLimit(PlanState *pstate)
/*
* Check for empty window; if so, treat like empty subplan.
*/
- if (node->count <= 0 && !node->noCount)
+ if (IsPercentOption(node->limitOption))
+ {
+if (node->percent == 0.0)
+{
+ node->lstate = LIMIT_EMPTY;
+ return NULL;
+}
+ }
+ else if (node->count <= 0 && !node->noCount)
{
node->lstate = LIMIT_EMPTY;
return NULL;
@@ -118,6 +131,16 @@ ExecLimit(PlanState *pstate)
break;
}
+ /*
+ * We may needed this tuple in subsequent scan so put it into
+ * tuplestore.
+ */
+ if (IsPercentOption(node->limitOption))
+ {
+tuplestore_puttupleslot(node->tupleStore, slot);
+tuplestore_advance(node->tupleStore, true);
+ }
+
/*
* Okay, we have the first tuple of the window.
*/
@@ -135,6 +158,85 @@ ExecLimit(PlanState *pstate)
case LIMIT_INWINDOW:
if (ScanDirectionIsForward(direction))
{
+/*
+ * In case of coming back from backward scan the tuple is
+ * already in tuple store.
+ */
+if (IsPercentOption(node->limitOption) && node->backwardPosition > 0)
+{
+ if (tuplestore_gettupleslot_heaptuple(node->tupleStore, true, true, slot))
+ {
+ node->subSlot
Re: Decomposing xml into table
Hey Tom On Mon, Jun 22, 2020 at 10:13 PM Tom Lane wrote: > Big -1 on that. COPY is not for general-purpose data transformation. > The more unrelated features we load onto it, the slower it will get, > and probably also the more buggy and unmaintainable. what new format handling takes to add regards to performance is a check to a few place and I don’t think that have noticeable performance impact and as far as I can see copy is extendable by design and I don’t think adding additional format will be a huge undertaking > There's also a > really fundamental mismatch, in that COPY is designed to do row-by-row > processing with essentially no cross-row state. How would you square > that with the inherently nested nature of XML? > > In xml case the difference is row delimiter . In xml mode user specifies row delimiter tag name and starting from start tag of specified name up to its end tag treated as single row and every text content in between will be our columns value filed > > The big-picture question here, though, is why expend effort on XML at all? > It seems like JSON is where it's at these days for that problem space. > there are a legacy systems and I think xml is still popular regards Surafel
Re: Decomposing xml into table
hey Pavel On Mon, Jun 22, 2020 at 9:59 PM Pavel Stehule wrote: > > Did you try the xmltable function? > > yes i know it but i am proposing changing given xml data in to relational form and insert it to desired table at once regards Surafel
Decomposing xml into table
In PostgreSQL there are a function table_to_xml to map the table content to xml value but there are no functionality to decompose xml back into table which can be used in system that uses xml for transport only or there are a need to migrate to database system to use database functionality. I propose to have this by extending copy to handle xml format as well because file parsing and tuple formation functions is in there and it also seems to me that implement it without using xml library is simpler Comments? regards Surafel
pg_dump --where option
Internally pg_dump have capability to filter the table data to dump by same
filter clause but it have no interface to use it and the patch here [1]
adds interface to it but it have at-least two issue, one is error message
in case of incorrect where clause specification is somehow hard to debug
and strange to pg_dump .Other issue is it applies the same filter clause to
multiple tables if pattern matching return multiple tables and it seems
undesired behavior to me because mostly we don’t want to applied the same
where clause specification to multiple table. The attached patch contain a
fix for both issue
[1].
https://www.postgresql.org/message-id/flat/CAGiT_HNav5B=OfCdfyFoqTa+oe5W1vG=pxktetcxxg4kcut...@mail.gmail.com
regards
Surafel
diff --git a/doc/src/sgml/ref/pg_dump.sgml b/doc/src/sgml/ref/pg_dump.sgml
index 2f0807e912..1c43eaa9de 100644
--- a/doc/src/sgml/ref/pg_dump.sgml
+++ b/doc/src/sgml/ref/pg_dump.sgml
@@ -1103,6 +1103,21 @@ PostgreSQL documentation
+
+ --where=table:filter_clause
+
+
+When dumping data for table, only include rows
+that meet the filter_clause condition.
+This option is useful when you want to dump only a subset of a particular table.
+--where can be given more than once to provide different filters for multiple tables.
+Note that if multiple options refer to the same table, only the first filter_clause will be applied.
+If necessary, use quotes in your shell to provide an argument that contains spaces.
+E.g. --where=mytable:"created_at >= '2018-01-01' AND test = 'f'"
+
+
+
+
-?
--help
diff --git a/src/bin/pg_dump/pg_dump.c b/src/bin/pg_dump/pg_dump.c
index 89d598f856..566469cdb7 100644
--- a/src/bin/pg_dump/pg_dump.c
+++ b/src/bin/pg_dump/pg_dump.c
@@ -121,6 +121,8 @@ static SimpleStringList tabledata_exclude_patterns = {NULL, NULL};
static SimpleOidList tabledata_exclude_oids = {NULL, NULL};
static SimpleStringList foreign_servers_include_patterns = {NULL, NULL};
static SimpleOidList foreign_servers_include_oids = {NULL, NULL};
+static SimpleStringList tabledata_where_patterns = {NULL, NULL};
+static SimpleOidList tabledata_where_oids = {NULL, NULL};
static const CatalogId nilCatalogId = {0, 0};
@@ -156,7 +158,8 @@ static void expand_foreign_server_name_patterns(Archive *fout,
static void expand_table_name_patterns(Archive *fout,
SimpleStringList *patterns,
SimpleOidList *oids,
- bool strict_names);
+ bool strict_names,
+ bool match_data);
static NamespaceInfo *findNamespace(Archive *fout, Oid nsoid);
static void dumpTableData(Archive *fout, TableDataInfo *tdinfo);
static void refreshMatViewData(Archive *fout, TableDataInfo *tdinfo);
@@ -386,6 +389,7 @@ main(int argc, char **argv)
{"on-conflict-do-nothing", no_argument, _nothing, 1},
{"rows-per-insert", required_argument, NULL, 10},
{"include-foreign-data", required_argument, NULL, 11},
+ {"where", required_argument, NULL, 12},
{NULL, 0, NULL, 0}
};
@@ -603,6 +607,10 @@ main(int argc, char **argv)
optarg);
break;
+ case 12:/* table data WHERE clause */
+simple_string_list_append(_where_patterns, optarg);
+break;
+
default:
fprintf(stderr, _("Try \"%s --help\" for more information.\n"), progname);
exit_nicely(1);
@@ -805,17 +813,26 @@ main(int argc, char **argv)
{
expand_table_name_patterns(fout, _include_patterns,
_include_oids,
- strict_names);
+ strict_names, false);
if (table_include_oids.head == NULL)
fatal("no matching tables were found");
}
+
+ if (tabledata_where_patterns.head != NULL)
+ {
+ expand_table_name_patterns(fout, _where_patterns,
+ _where_oids,
+ true, true);
+ if (tabledata_where_oids.head == NULL)
+ fatal("no matching table was found");
+ }
expand_table_name_patterns(fout, _exclude_patterns,
_exclude_oids,
- false);
+ false, false);
expand_table_name_patterns(fout, _exclude_patterns,
_exclude_oids,
- false);
+ false, false);
expand_foreign_server_name_patterns(fout, _servers_include_patterns,
_servers_include_oids);
@@ -1046,6 +1063,7 @@ help(const char *progname)
printf(_(" --use-set-session-authorization\n"
" use SET SESSION AUTHORIZATION commands instead of\n"
" ALTER OWNER commands to set ownership\n"));
+ printf(_(" --where=TABLE:WHERE_CLAUSE only dump selected rows for the given table\n"));
printf(_("\nConnection options:\n"));
printf(_(" -d, --dbname=DBNAME database to dump\n"));
@@ -1393,16 +1411,20 @@ expand_foreign_server_name_patterns(Archive *fout,
/*
* Find the OIDs of all tables matching the given list of patterns,
* and append them to the given OID list. See also
Re: Conflict handling for COPY FROM
On Fri, Mar 27, 2020 at 3:27 PM Tom Lane wrote:
> Surafel Temesgen writes:
> > [ conflict-handling-copy-from-v16.patch ]
>
> I took a quick look at this patch, since it was marked "ready for
> committer", but I don't see how it can possibly be considered committable.
>
> 1. Covering only the errors that are thrown in DoCopy itself doesn't
> seem to me to pass the smell test. Yes, I'm sure there's some set of
> use-cases for which that'd be helpful, but I think most people would
> expect a "skip errors" option to be able to handle cases like malformed
> numbers or bad encoding. I understand the implementation reasons that
> make it impractical to cover other errors, but do we really want a
> feature that most people will see as much less than half-baked? I fear
> it'll be an embarrassment.
>
> I did small research and most major database management system didn't
claim
they handle every problem in loading file at least in every usage scenario.
> 2. If I'm reading the patch correctly, (some) rejected rows are actually
> sent back to the client. This is a wire protocol break of the first
> magnitude, and can NOT be accepted. At least not without some provisions
> for not doing it with a client that isn't prepared for it. I also am
> fairly worried about the possibilities for deadlock (ie, both ends stuck
> waiting for the other one to absorb data) if the return traffic volume is
> high enough.
>
>
if my understanding is correct copy_in occur in sub protocol inside simple
or
extended query protocol that know and handle the responds
> 3. I don't think enough thought has been put into the reporting, either.
>
> +ereport(WARNING,
> +(errcode(ERRCODE_BAD_COPY_FILE_FORMAT),
> + errmsg("skipping \"%s\" --- extra data after
> last expected column",
> +cstate->line_buf.data)));
>
> That's not going to be terribly helpful if the input line runs to many
> megabytes. Or even if no individual line is very long, but you get
> millions of such warnings. It's pretty much at variance with our
> message style guidelines (among other things, those caution you to keep
> the primary error message short); and it's randomly different from
> COPY's existing practice, which is to show the faulty line as CONTEXT.
> Plus it seems plenty weird that some errors are reported this way while
> others are reported by sending back the bad tuple (with, it looks like,
> no mention of what the specific problem is ... what if you have a lot of
> unique indexes?).
>
> Currently we can’t get problem description in speculative insertion
infrastructure and am afraid adding problem description to return tuple
will make the data less usable without further processing.Warning raised
for error that happen before tuple contracted. Other option is to skip those
record silently but reporting to user give user the chance to correct it.
> BTW, while I don't know much about the ON CONFLICT (speculative
> insertion) infrastructure, I wonder how well it really works to
> not specify an arbiter index. I see that you're getting away with
> it in a trivial test case that has exactly one index, but that's
> not stressing the point very hard.
>
> If arbiter index is not specified that means all indexes as the comment in
ExecCheckIndexConstraints stated
/*
* ExecCheckIndexConstraints
*
* This routine checks if a tuple violates any unique or
* exclusion constraints. Returns true if there is no conflict.
* Otherwise returns false, and the TID of the conflicting
* tuple is returned in *conflictTid.
*
* If 'arbiterIndexes' is given, only those indexes are checked.
* NIL means all indexes.
regards
Surafel
Re: Conflict handling for COPY FROM
Hi Takanori Asaba,
>
>
> Although it is a small point, it may be better like this:
> +70005 27 36 46 56 -> 70005 27 37 47 57
>
>
done
> I want to discuss about copy from binary file.
> It seems that this patch tries to avoid the error that number of field is
> different .
>
> + {
> + if (cstate->error_limit > 0 ||
> cstate->ignore_all_error)
> + {
> + ereport(WARNING,
> +
> (errcode(ERRCODE_BAD_COPY_FILE_FORMAT),
> +errmsg("skipping \"%s\"
> --- row field count is %d, expected %d",
> +
> cstate->line_buf.data, (int) fld_count, attr_count)));
> + cstate->error_limit--;
> + goto next_line;
> + }
> + else
> + ereport(ERROR,
> +
> (errcode(ERRCODE_BAD_COPY_FILE_FORMAT),
> +errmsg("row field count
> is %d, expected %d",
> + (int)
> fld_count, attr_count)));
> +
> + }
>
> I checked like this:
>
> postgres=# CREATE TABLE x (
> postgres(# a serial UNIQUE,
> postgres(# b int,
> postgres(# c text not null default 'stuff',
> postgres(# d text,
> postgres(# e text
> postgres(# );
> CREATE TABLE
> postgres=# COPY x from stdin;
> Enter data to be copied followed by a newline.
> End with a backslash and a period on a line by itself, or an EOF signal.
> >> 7000425 35 45 55
> >> 7000526 36 46 56
> >> \.
> COPY 2
> postgres=# SELECT * FROM x;
>a | b | c | d | e
> ---++++
> 70004 | 25 | 35 | 45 | 55
> 70005 | 26 | 36 | 46 | 56
> (2 rows)
>
> postgres=# COPY x TO '/tmp/copyout' (FORMAT binary);
> COPY 2
> postgres=# CREATE TABLE y (
> postgres(# a serial UNIQUE,
> postgres(# b int,
> postgres(# c text not null default 'stuff',
> postgres(# d text
> postgres(# );
> CREATE TABLE
> postgres=# COPY y FROM '/tmp/copyout' WITH (FORMAT binary,ERROR_LIMIT -1);
> 2020-03-12 16:55:55.457 JST [2319] WARNING: skipping "" --- row field
> count is 5, expected 4
> 2020-03-12 16:55:55.457 JST [2319] CONTEXT: COPY y, line 1
> 2020-03-12 16:55:55.457 JST [2319] WARNING: skipping "" --- row field
> count is 0, expected 4
> 2020-03-12 16:55:55.457 JST [2319] CONTEXT: COPY y, line 2
> 2020-03-12 16:55:55.457 JST [2319] ERROR: unexpected EOF in COPY data
> 2020-03-12 16:55:55.457 JST [2319] CONTEXT: COPY y, line 3, column a
> 2020-03-12 16:55:55.457 JST [2319] STATEMENT: COPY y FROM '/tmp/copyout'
> WITH (FORMAT binary,ERROR_LIMIT -1);
> WARNING: skipping "" --- row field count is 5, expected 4
> WARNING: skipping "" --- row field count is 0, expected 4
> ERROR: unexpected EOF in COPY data
> CONTEXT: COPY y, line 3, column a
>
> It seems that the error isn't handled.
> 'WARNING: skipping "" --- row field count is 5, expected 4' is correct,
> but ' WARNING: skipping "" --- row field count is 0, expected 4' is not
> correct.
>
>
Thank you for the detailed example
> Also, is it needed to skip the error that happens when input is binary
> file?
> Is the case that each row has different number of field and only specific
> rows are copied occurred?
>
>
An error that can be surly handled without transaction rollback can
be included in error handling but i will like to proceed without binary file
errors handling for the time being
regards
Surafel
conflict-handling-copy-from-v16.patch
Description: Binary data
Re: A rather hackish POC for alternative implementation of WITH TIES
On Wed, Jan 22, 2020 at 3:35 PM Andrew Gierth wrote: > > "Alvaro" == Alvaro Herrera writes: > > > I was largely holding off on doing further work hoping for some > discussion of which way we should go. If you think my approach is worth > pursuing (I haven't seriously tested the performance, but I'd expect it > to be slower than Surafel's - the price you pay for flexibility) then I > can look at it further, but figuring out the planner stuff will take > some time. > > Other alternative can be pushing the existing implementation which will be open to change in case of better-finished implementation. regards Surafel
Re: WIP: System Versioned Temporal Table
Hi, On Tue, Mar 3, 2020 at 9:33 PM David Steele wrote: > Hi Surafel, > > On 1/3/20 5:57 AM, Surafel Temesgen wrote: > > Rebased and conflict resolved i hope it build clean this time > > This patch no longer applies according to cfbot and there are a number > of review comments that don't seem to have been addressed yet. > > The patch is not exactly new for this CF but since the first version was > posted 2020-01-01 and there have been no updates (except a rebase) since > then it comes pretty close. > > Were you planning to work on this for PG13? If so we'll need to see a > rebased and updated patch pretty soon. My recommendation is that we > move this patch to PG14. > > I agree with moving to PG14 . Its hard to make it to PG13. regards Surafel
Re: Conflict handling for COPY FROM
On Fri, Mar 6, 2020 at 11:30 AM asaba.takan...@fujitsu.com <
asaba.takan...@fujitsu.com> wrote:
> Hello Surafel,
>
> Sorry for my late reply.
>
> From: Surafel Temesgen
> >On Thu, Dec 12, 2019 at 7:51 AM mailto:asaba.takan...@fujitsu.com
> <mailto:asaba.takan...@fujitsu.com> wrote:
> >>2. I have a question about copy meta-command.
> >>When I executed copy meta-command, output wasn't displayed.
> >>Does it correspond to copy meta-command?
> >
> >Fixed
> Thank you.
>
> I think we need regression test that constraint violating row is returned
> back to the caller.
> How about this?
>
>
okay attached is a rebased patch with it
regards
Surafel
diff --git a/doc/src/sgml/ref/copy.sgml b/doc/src/sgml/ref/copy.sgml
index a99f8155e4..845902b824 100644
--- a/doc/src/sgml/ref/copy.sgml
+++ b/doc/src/sgml/ref/copy.sgml
@@ -44,6 +44,7 @@ COPY { table_name [ ( column_name [, ...] )
FORCE_NULL ( column_name [, ...] )
ENCODING 'encoding_name'
+ERROR_LIMIT limit_number
@@ -355,6 +356,28 @@ COPY { table_name [ (
+
+ERROR_LIMIT
+
+
+ Enables ignoring of errored out rows up to limit_number. If limit_number is set
+ to -1, then all errors will be ignored.
+
+
+
+ Currently, only unique or exclusion constraint violation
+ and rows formatting errors are ignored. Malformed
+ rows will rise warnings, while constraint violating rows
+ will be returned back to the caller unless any error is raised;
+ i.e. if any error is raised due to error_limit exceeds, no rows
+ will be returned back to the caller.
+
+
+
+
+
WHERE
diff --git a/src/backend/commands/copy.c b/src/backend/commands/copy.c
index e79ede4cb8..4184e2e755 100644
--- a/src/backend/commands/copy.c
+++ b/src/backend/commands/copy.c
@@ -20,6 +20,7 @@
#include "access/heapam.h"
#include "access/htup_details.h"
+#include "access/printtup.h"
#include "access/sysattr.h"
#include "access/tableam.h"
#include "access/xact.h"
@@ -48,6 +49,8 @@
#include "port/pg_bswap.h"
#include "rewrite/rewriteHandler.h"
#include "storage/fd.h"
+#include "storage/lmgr.h"
+#include "tcop/pquery.h"
#include "tcop/tcopprot.h"
#include "utils/builtins.h"
#include "utils/lsyscache.h"
@@ -153,6 +156,7 @@ typedef struct CopyStateData
List *convert_select; /* list of column names (can be NIL) */
bool *convert_select_flags; /* per-column CSV/TEXT CS flags */
Node *whereClause; /* WHERE condition (or NULL) */
+ int error_limit; /* total number of error to ignore */
/* these are just for error messages, see CopyFromErrorCallback */
const char *cur_relname; /* table name for error messages */
@@ -182,6 +186,9 @@ typedef struct CopyStateData
bool volatile_defexprs; /* is any of defexprs volatile? */
List *range_table;
ExprState *qualexpr;
+ bool ignore_error; /* is ignore error specified? */
+ bool ignore_all_error; /* is error_limit -1 (ignore all error)
+ * specified? */
TransitionCaptureState *transition_capture;
@@ -836,7 +843,7 @@ CopyLoadRawBuf(CopyState cstate)
void
DoCopy(ParseState *pstate, const CopyStmt *stmt,
int stmt_location, int stmt_len,
- uint64 *processed)
+ uint64 *processed, DestReceiver *dest)
{
CopyState cstate;
bool is_from = stmt->is_from;
@@ -1068,7 +1075,7 @@ DoCopy(ParseState *pstate, const CopyStmt *stmt,
cstate = BeginCopyFrom(pstate, rel, stmt->filename, stmt->is_program,
NULL, stmt->attlist, stmt->options);
cstate->whereClause = whereClause;
- *processed = CopyFrom(cstate); /* copy from file to database */
+ *processed = CopyFrom(cstate, dest); /* copy from file to database */
EndCopyFrom(cstate);
}
else
@@ -1290,6 +1297,18 @@ ProcessCopyOptions(ParseState *pstate,
defel->defname),
parser_errposition(pstate, defel->location)));
}
+ 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;
+ }
else
ereport(ERROR,
(errcode(ERRCODE_SYNTAX_ERROR),
@@ -1440,6 +1459,10 @@ ProcessCopyOptions(ParseState *pstate,
ereport(ERROR,
(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
errmsg("CSV quote character must not appear in the NULL specification")));
+ if (cstate->ignore_error && !cstate->is_copy_from)
+ ereport(ERROR,
+(errcode(ERRCODE_SYNTAX_ERRO
Re: [PATCH v1] Allow COPY "text" to output a header and add header matching mode to COPY FROM
On Mon, Mar 2, 2020 at 2:45 AM Rémi Lapeyre wrote: > > I created an entry for this patch in the new CommiFest but it seems that > it is not finding it. Is there anything that I need to do? > > Is is added on next open commit fest which is https://commitfest.postgresql.org/28/ now regards Surafel
Re: Conflict handling for COPY FROM
On Mon, Feb 17, 2020 at 10:00 AM Tatsuo Ishii wrote:
> >> test=# copy t1 from '/tmp/a' with (error_limit 1);
> >> ERROR: duplicate key value violates unique constraint "t1_pkey"
> >> DETAIL: Key (i)=(2) already exists.
> >> CONTEXT: COPY t1, line 2: "2 2"
> >>
> >> So if the number of errors raised exceeds error_limit, no constaraint
> >> violating rows (in this case i=1, j=1) are returned.
> >>
> >
> > error_limit is specified to dictate the number of error allowed in copy
> > operation
> > to precede. If it exceed the number the operation is stopped. there may
> > be more conflict afterward and returning limited number of conflicting
> rows
> > have no much use
>
> Still I see your explanation differs from what the document patch says.
>
> + Currently, only unique or exclusion constraint violation
> + and rows formatting errors are ignored. Malformed
> + rows will rise warnings, while constraint violating rows
> + will be returned back to the caller.
>
> I am afraid once this patch is part of next version of PostgreSQL, we
> get many complains/inqueires from users. What about changing like this:
>
> Currently, only unique or exclusion constraint violation and
> rows formatting errors are ignored. Malformed rows will rise
> warnings, while constraint violating rows will be returned back
> to the caller unless any error is raised; i.e. if any error is
> raised due to error_limit exceeds, no rows will be returned back
> to the caller.
>
Its better so amended .
regards
Surafel
diff --git a/doc/src/sgml/ref/copy.sgml b/doc/src/sgml/ref/copy.sgml
index a99f8155e4..845902b824 100644
--- a/doc/src/sgml/ref/copy.sgml
+++ b/doc/src/sgml/ref/copy.sgml
@@ -44,6 +44,7 @@ COPY { table_name [ ( column_name [, ...] )
FORCE_NULL ( column_name [, ...] )
ENCODING 'encoding_name'
+ERROR_LIMIT limit_number
@@ -355,6 +356,28 @@ COPY { table_name [ (
+
+ERROR_LIMIT
+
+
+ Enables ignoring of errored out rows up to limit_number. If limit_number is set
+ to -1, then all errors will be ignored.
+
+
+
+ Currently, only unique or exclusion constraint violation
+ and rows formatting errors are ignored. Malformed
+ rows will rise warnings, while constraint violating rows
+ will be returned back to the caller unless any error is raised;
+ i.e. if any error is raised due to error_limit exceeds, no rows
+ will be returned back to the caller.
+
+
+
+
+
WHERE
diff --git a/src/backend/commands/copy.c b/src/backend/commands/copy.c
index 40a8ec1abd..72225a85a0 100644
--- a/src/backend/commands/copy.c
+++ b/src/backend/commands/copy.c
@@ -24,6 +24,7 @@
#include "access/tableam.h"
#include "access/xact.h"
#include "access/xlog.h"
+#include "access/printtup.h"
#include "catalog/dependency.h"
#include "catalog/pg_authid.h"
#include "catalog/pg_type.h"
@@ -48,7 +49,9 @@
#include "port/pg_bswap.h"
#include "rewrite/rewriteHandler.h"
#include "storage/fd.h"
+#include "storage/lmgr.h"
#include "tcop/tcopprot.h"
+#include "tcop/pquery.h"
#include "utils/builtins.h"
#include "utils/lsyscache.h"
#include "utils/memutils.h"
@@ -153,6 +156,7 @@ typedef struct CopyStateData
List *convert_select; /* list of column names (can be NIL) */
bool *convert_select_flags; /* per-column CSV/TEXT CS flags */
Node *whereClause; /* WHERE condition (or NULL) */
+ int error_limit; /* total number of error to ignore */
/* these are just for error messages, see CopyFromErrorCallback */
const char *cur_relname; /* table name for error messages */
@@ -182,6 +186,9 @@ typedef struct CopyStateData
bool volatile_defexprs; /* is any of defexprs volatile? */
List *range_table;
ExprState *qualexpr;
+ bool ignore_error; /* is ignore error specified? */
+ bool ignore_all_error; /* is error_limit -1 (ignore all error)
+ * specified? */
TransitionCaptureState *transition_capture;
@@ -836,7 +843,7 @@ CopyLoadRawBuf(CopyState cstate)
void
DoCopy(ParseState *pstate, const CopyStmt *stmt,
int stmt_location, int stmt_len,
- uint64 *processed)
+ uint64 *processed, DestReceiver *dest)
{
CopyState cstate;
bool is_from = stmt->is_from;
@@ -1068,7 +1075,7 @@ DoCopy(ParseState *pstate, const CopyStmt *stmt,
cstate = BeginCopyFrom(pstate, rel, stmt->filename, stmt->is_program,
NULL, stmt->attlist, stmt->options);
cstate->whereClause = whereClause;
- *processed = CopyFrom(cstate); /* copy from file to database */
+ *processed = CopyFrom(cstate, dest); /* copy from file to database */
EndCopyFrom(cstate);
}
else
@@ -1290,6 +1297,18 @@ ProcessCopyOptions(ParseState *pstate,
defel->defname),
parser_errposition(pstate, defel->location)));
}
+ else if (strcmp(defel->defname, "error_limit") == 0)
+ {
+ if (cstate->ignore_error)
+ereport(ERROR,
+ (errcode(ERRCODE_SYNTAX_ERROR),
+
Re: Conflict handling for COPY FROM
Hi,
> > ERROR_LIMIT ' class="parameter">limit_number'
> >
> > I think this should be:
> >
> > ERROR_LIMIT limit_number
> >
> > (no single quote)
>
>
Thank you .Fixed
> More comments:
>
> - I think the document should stat that if limit_number = 0, all
> errors are immediately raised (behaves same as current befavior without
> the patch).
>
>
if we want all error to be raised error limit_number not need to be
specified.
but if it is specified like limit_number = 0 i think it is self-explanatory
> - "constraint violating rows will be returned back to the caller."
> This does explains the current implementation. I am not sure if it's
> intended or not though:
>
> cat /tmp/a
> 1 1
> 2 2
> 3 3
> 3 4
>
> psql test
> $ psql test
> psql (13devel)
> Type "help" for help.
>
> test=# select * from t1;
> i | j
> ---+---
> 1 | 1
> 2 | 2
> 3 | 3
> (3 rows)
>
> test=# copy t1 from '/tmp/a' with (error_limit 1);
> ERROR: duplicate key value violates unique constraint "t1_pkey"
> DETAIL: Key (i)=(2) already exists.
> CONTEXT: COPY t1, line 2: "2 2"
>
> So if the number of errors raised exceeds error_limit, no constaraint
> violating rows (in this case i=1, j=1) are returned.
>
error_limit is specified to dictate the number of error allowed in copy
operation
to precede. If it exceed the number the operation is stopped. there may
be more conflict afterward and returning limited number of conflicting rows
have no much use
regards
Surafel
diff --git a/doc/src/sgml/ref/copy.sgml b/doc/src/sgml/ref/copy.sgml
index a99f8155e4..c53e5f6d92 100644
--- a/doc/src/sgml/ref/copy.sgml
+++ b/doc/src/sgml/ref/copy.sgml
@@ -44,6 +44,7 @@ COPY { table_name [ ( column_name [, ...] )
FORCE_NULL ( column_name [, ...] )
ENCODING 'encoding_name'
+ERROR_LIMIT limit_number
@@ -355,6 +356,26 @@ COPY { table_name [ (
+
+ERROR_LIMIT
+
+
+ Enables ignoring of errored out rows up to limit_number. If limit_number is set
+ to -1, then all errors will be ignored.
+
+
+
+ Currently, only unique or exclusion constraint violation
+ and rows formatting errors are ignored. Malformed
+ rows will rise warnings, while constraint violating rows
+ will be returned back to the caller.
+
+
+
+
+
WHERE
diff --git a/src/backend/commands/copy.c b/src/backend/commands/copy.c
index 40a8ec1abd..72225a85a0 100644
--- a/src/backend/commands/copy.c
+++ b/src/backend/commands/copy.c
@@ -24,6 +24,7 @@
#include "access/tableam.h"
#include "access/xact.h"
#include "access/xlog.h"
+#include "access/printtup.h"
#include "catalog/dependency.h"
#include "catalog/pg_authid.h"
#include "catalog/pg_type.h"
@@ -48,7 +49,9 @@
#include "port/pg_bswap.h"
#include "rewrite/rewriteHandler.h"
#include "storage/fd.h"
+#include "storage/lmgr.h"
#include "tcop/tcopprot.h"
+#include "tcop/pquery.h"
#include "utils/builtins.h"
#include "utils/lsyscache.h"
#include "utils/memutils.h"
@@ -153,6 +156,7 @@ typedef struct CopyStateData
List *convert_select; /* list of column names (can be NIL) */
bool *convert_select_flags; /* per-column CSV/TEXT CS flags */
Node *whereClause; /* WHERE condition (or NULL) */
+ int error_limit; /* total number of error to ignore */
/* these are just for error messages, see CopyFromErrorCallback */
const char *cur_relname; /* table name for error messages */
@@ -182,6 +186,9 @@ typedef struct CopyStateData
bool volatile_defexprs; /* is any of defexprs volatile? */
List *range_table;
ExprState *qualexpr;
+ bool ignore_error; /* is ignore error specified? */
+ bool ignore_all_error; /* is error_limit -1 (ignore all error)
+ * specified? */
TransitionCaptureState *transition_capture;
@@ -836,7 +843,7 @@ CopyLoadRawBuf(CopyState cstate)
void
DoCopy(ParseState *pstate, const CopyStmt *stmt,
int stmt_location, int stmt_len,
- uint64 *processed)
+ uint64 *processed, DestReceiver *dest)
{
CopyState cstate;
bool is_from = stmt->is_from;
@@ -1068,7 +1075,7 @@ DoCopy(ParseState *pstate, const CopyStmt *stmt,
cstate = BeginCopyFrom(pstate, rel, stmt->filename, stmt->is_program,
NULL, stmt->attlist, stmt->options);
cstate->whereClause = whereClause;
- *processed = CopyFrom(cstate); /* copy from file to database */
+ *processed = CopyFrom(cstate, dest); /* copy from file to database */
EndCopyFrom(cstate);
}
else
@@ -1290,6 +1297,18 @@ ProcessCopyOptions(ParseState *pstate,
defel->defname),
parser_errposition(pstate, defel->location)));
}
+ 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
Re: [PATCH v1] Allow COPY "text" format to output a header
On Wed, Feb 5, 2020 at 4:19 PM Rémi Lapeyre wrote: > > > > FWIW there was more recent propose patch at > https://www.postgresql.org/message-id/flat/caf1-j-0ptcwmeltswwgv2m70u26n4g33gpe1rckqqe6wvqd...@mail.gmail.com > > and among feedback given is to adding header matching feature on to > this. > > Thanks for the feedback. What should happen now? Can I just move the patch > to the current Commitfest and send a new patch to the old thread? Both way is possible you can add this tread with feedback incorporated patch or you can add old tread with a new patch regards Surafel
Re: [PATCH v1] Allow COPY "text" format to output a header
Hi, On Tue, Feb 4, 2020 at 4:25 PM Rémi Lapeyre wrote: > This patch adds the possibility to use the "header" option when using COPY > with > the text format. A todo entry was opened for this and I updated the tests > and > the documentation. > > This was previously discussed at > https://www.postgresql.org/message-id/flat/CACfv%2BpJ31tesLvncJyP24quo8AE%2BM0GP6p6MEpwPv6yV8%3DsVHQ%40mail.gmail.com > > FWIW there was more recent propose patch at https://www.postgresql.org/message-id/flat/caf1-j-0ptcwmeltswwgv2m70u26n4g33gpe1rckqqe6wvqd...@mail.gmail.com and among feedback given is to adding header matching feature on to this. regards Surafel
Re: can we use different function in place of atoi in vacuumdb.c file
Hi, On Thu, Jan 23, 2020 at 3:56 PM Mahendra Singh Thalor wrote: > Hi all, > While reviewing one patch, I found that if we give any non-integer string > to atoi (say aa), then it is returning zero(0) as output so we are not > giving any error(assuming 0 as valid argument) and continuing our > operations. > > Ex: > Let say, we gave "-P aa" (patch is in review[1]), then it will disable > parallel vacuum because atoi is returning zero as parallel degree but > ideally it should give error or at least it should not disable parallel > vacuum. > > I think, in-place of atoi, we should use different function ( defGetInt32, > strtoint) or we can write own function. > > Thoughts? > > [1]: > https://www.postgresql.org/message-id/CA%2Bfd4k6DgwtQSr4%3DUeY%2BWbGuF7-oD%3Dm-ypHPy%2BsYHiXZc%2BhTUQ%40mail.gmail.com > -- > For server side there are also scanint8 function for string parsing and for Clint side we can use strtoint function that didn't have the above issue and it accept char pointer which is I think suitable for [1] usage. regards Surafel
Re: FETCH FIRST clause WITH TIES option
On Thu, Nov 28, 2019 at 5:49 PM Alvaro Herrera
wrote:
> On 2019-Nov-28, Surafel Temesgen wrote:
>
> > On Thu, Nov 28, 2019 at 12:36 AM Alvaro Herrera <
> alvhe...@2ndquadrant.com>
> > wrote:
> >
> > > I think you should add a /* fall-though */ comment after changing
> state.
> > > Like this (this flow seems clearer; also DRY):
> > >
> > > if (!node->noCount &&
> > > node->position - node->offset >= node->count)
> > > {
> > > if (node->limitOption == LIMIT_OPTION_COUNT)
> > > {
> > > node->lstate = LIMIT_WINDOWEND;
> > > return NULL;
> > > }
> > > else
> > > {
> > > node->lstate = LIMIT_WINDOWEND_TIES;
> > > /* fall-through */
> > > }
> > > }
> > > else
> > > ...
> >
> > changed
>
> But you did not read my code snippet, did you ...?
>
I don't see it. changed to it and rebased to current master
regards
Surafel
From 944626bf7568604bd0a1b5785c6db009baf39d0c Mon Sep 17 00:00:00 2001
From: Surafel Temesgen
Date: Wed, 22 Jan 2020 09:10:59 +0300
Subject: [PATCH] fetch first with ties
---
doc/src/sgml/ref/select.sgml| 15 ++-
src/backend/catalog/sql_features.txt| 2 +-
src/backend/executor/nodeLimit.c| 169 +---
src/backend/nodes/copyfuncs.c | 7 +
src/backend/nodes/equalfuncs.c | 2 +
src/backend/nodes/outfuncs.c| 7 +
src/backend/nodes/readfuncs.c | 6 +
src/backend/optimizer/plan/createplan.c | 44 +-
src/backend/optimizer/plan/planner.c| 1 +
src/backend/optimizer/util/pathnode.c | 2 +
src/backend/parser/analyze.c| 3 +
src/backend/parser/gram.y | 117
src/backend/utils/adt/ruleutils.c | 10 +-
src/include/nodes/execnodes.h | 4 +
src/include/nodes/nodes.h | 13 ++
src/include/nodes/parsenodes.h | 2 +
src/include/nodes/pathnodes.h | 1 +
src/include/nodes/plannodes.h | 5 +
src/include/optimizer/pathnode.h| 1 +
src/include/optimizer/planmain.h| 3 +-
src/test/regress/expected/limit.out | 104 +++
src/test/regress/sql/limit.sql | 31 +
22 files changed, 489 insertions(+), 60 deletions(-)
diff --git a/doc/src/sgml/ref/select.sgml b/doc/src/sgml/ref/select.sgml
index 691e402803..2b11c38087 100644
--- a/doc/src/sgml/ref/select.sgml
+++ b/doc/src/sgml/ref/select.sgml
@@ -44,7 +44,7 @@ SELECT [ ALL | DISTINCT [ ON ( expressionexpression [ ASC | DESC | USING operator ] [ NULLS { FIRST | LAST } ] [, ...] ]
[ LIMIT { count | ALL } ]
[ OFFSET start [ ROW | ROWS ] ]
-[ FETCH { FIRST | NEXT } [ count ] { ROW | ROWS } ONLY ]
+[ FETCH { FIRST | NEXT } [ count ] { ROW | ROWS } { ONLY | WITH TIES } ]
[ FOR { UPDATE | NO KEY UPDATE | SHARE | KEY SHARE } [ OF table_name [, ...] ] [ NOWAIT | SKIP LOCKED ] [...] ]
where from_item can be one of:
@@ -1438,7 +1438,7 @@ OFFSET start
which PostgreSQL also supports. It is:
OFFSET start { ROW | ROWS }
-FETCH { FIRST | NEXT } [ count ] { ROW | ROWS } ONLY
+FETCH { FIRST | NEXT } [ count ] { ROW | ROWS } { ONLY | WITH TIES }
In this syntax, the start
or count value is required by
@@ -1448,10 +1448,13 @@ FETCH { FIRST | NEXT } [ count ] {
ambiguity.
If count is
omitted in a FETCH clause, it defaults to 1.
-ROW
-and ROWS as well as FIRST
-and NEXT are noise words that don't influence
-the effects of these clauses.
+The WITH TIES option is used to return any additional
+rows that tie for the last place in the result set according to
+ORDER BY clause; ORDER BY
+is mandatory in this case.
+ROW and ROWS as well as
+FIRST and NEXT are noise
+words that don't influence the effects of these clauses.
According to the standard, the OFFSET clause must come
before the FETCH clause if both are present; but
PostgreSQL is laxer and allows either order.
diff --git a/src/backend/catalog/sql_features.txt b/src/backend/catalog/sql_features.txt
index 9f840ddfd2..746c9a2516 100644
--- a/src/backend/catalog/sql_features.txt
+++ b/src/backend/catalog/sql_features.txt
@@ -345,7 +345,7 @@ F863 Nested in YES
F864 Top-level in views YES
F865 in YES
F866 FETCH FIRST clause: PERCENT option NO
-F867 FETCH FIRST clause: WITH TIES option NO
+F867 FETCH FIRST clause: WITH TIES option YES
R010 Row pattern recognition: FRO
Re: A rather hackish POC for alternative implementation of WITH TIES
On Fri, Nov 29, 2019 at 8:40 AM Andrew Gierth wrote: > This patch is a rather hacky implementation of the basic idea for > implementing FETCH ... WITH TIES, and potentially also PERCENT, by using > a window function expression to compute a stopping point. > > Large chunks of this (the parser/ruleutils changes, docs, tests) are > taken from Surafel Temesgen's patch. The difference is that the executor > change in my version is minimal: Limit allows a boolean column in the > input to signal the point at which to stop. The planner inserts a > WindowAgg node to compute the necessary condition using the rank() > function. > > The way this is done in the planner isn't (IMO) the best and should > probably be improved; in particular it currently misses some possible > optimizations (most notably constant-folding of the offset+limit > subexpression). I also haven't tested it properly to see whether I broke > anything, though it does pass regression. > > > Unlike most other executor node limit node has implementation for handling backward scan that support cursor operation but your approach didn't do this inherently because it outsource limitNode functionality to window function and window function didn't do this eg. postgres=# begin; BEGIN postgres=# declare c cursor for select i from generate_series(1,100) s(i) order by i fetch first 2 rows with ties; DECLARE CURSOR postgres=# fetch all in c; i --- 1 2 (2 rows) postgres=# fetch backward all in c; ERROR: cursor can only scan forward HINT: Declare it with SCROLL option to enable backward scan. Even with SCROLL option it is not working as limitNode does. It store the result and return in backward scan that use more space than current limit and limit with ties implementation. If am not mistaken the patch also reevaluate limit every time returning row beside its not good for performance its will return incorrect result with limit involving volatile function regards Surafel
Re: WIP: System Versioned Temporal Table
On Mon, Oct 28, 2019 at 6:36 PM Vik Fearing wrote: > On 28/10/2019 13:48, Surafel Temesgen wrote: > > > > > > On Fri, Oct 25, 2019 at 10:45 PM Vik Fearing > > mailto:vik.fear...@2ndquadrant.com>> > wrote: > > > > > > > > I don't understand what you mean by this. > > > > > > > > > > > > The primary purpose of adding row end time to primary key is to > > allow > > > duplicate value to be inserted into a table with keeping > > constraint in > > > current data but it can be duplicated in history data. Adding row > > > start time column to primary key will eliminate this uniqueness for > > > current data which is not correct > > > > > > How? The primary/unique keys must always be unique at every point > > in time. > > > > > > From user prospect it is acceptable to delete and reinsert a record > > with the same key value multiple time which means there will be > > multiple record with the same key value in a history data but there is > > only one values in current data as a table without system versioning > > do .I add row end time column to primary key to allow user supplied > > primary key values to be duplicated in history data which is acceptable > > > > Yes, I understand that. I'm saying you should also add the row start > time column. > > that allow the same primary key value row to be insert as long as insertion time is different regards Surafel
Re: WIP: System Versioned Temporal Table
On Fri, Jan 3, 2020 at 4:22 PM Vik Fearing wrote: > > > > Rebased and conflict resolved i hope it build clean this time > > > > It does but you haven't included your tests file so `make check` fails. > > > what tests file? i add system_versioned_table.sql and system_versioned_table.out test files and it tested and pass on appveyor[1] only failed on travis because of warning. i will add more test > It seems clear to me that you haven't tested it at all anyway. The > temporal conditions do not return the correct results, and the syntax is > wrong, too. Also, none of my previous comments have been addressed > except for "system versioning" instead of "system_versioning". Why? > > I also correct typo and add row end column time to unique key that make it unique for current data. As you mentioned other comment is concerning about application-time periods which the patch not addressing . i refer sql 2011 standard for syntax can you tell me which syntax you find it wrong? [1]. https://ci.appveyor.com/project/postgresql-cfbot/postgresql/build/1.0.73247 regards Surafel
Re: WIP: System Versioned Temporal Table
On Thu, Jan 2, 2020 at 12:12 AM Vik Fearing
wrote:
> This does not compile against current head (0ce38730ac).
>
>
> gram.y: error: shift/reduce conflicts: 6 found, 0 expected
>
>
Rebased and conflict resolved i hope it build clean this time
regards
Surafel
From 640f8fc466acc90f0e46d65f21077f652651f34f Mon Sep 17 00:00:00 2001
From: Surafel Temesgen
Date: Fri, 3 Jan 2020 13:50:19 +0300
Subject: [PATCH] system versioned temporal table
---
doc/src/sgml/ref/alter_table.sgml | 23 ++
doc/src/sgml/ref/create_table.sgml| 47
doc/src/sgml/ref/select.sgml | 37 +++
src/backend/access/common/tupdesc.c | 4 +
src/backend/commands/copy.c | 5 +
src/backend/commands/tablecmds.c | 49 +++-
src/backend/commands/view.c | 6 +
src/backend/executor/nodeModifyTable.c| 180 +
src/backend/nodes/copyfuncs.c | 31 +++
src/backend/nodes/equalfuncs.c| 28 ++
src/backend/nodes/makefuncs.c | 120 +
src/backend/nodes/outfuncs.c | 1 +
src/backend/optimizer/plan/planner.c | 8 +
src/backend/optimizer/plan/subselect.c| 19 ++
src/backend/optimizer/util/plancat.c | 189 +
src/backend/parser/analyze.c | 9 +
src/backend/parser/gram.y | 252 ++
src/backend/parser/parse_clause.c | 76 ++
src/backend/parser/parse_utilcmd.c| 133 -
src/backend/tcop/utility.c| 61 +
src/backend/utils/cache/relcache.c| 3 +
src/bin/pg_dump/pg_dump.c | 4 +
src/bin/psql/describe.c | 6 +-
src/include/access/tupdesc.h | 1 +
src/include/catalog/pg_attribute.h| 3 +
src/include/executor/nodeModifyTable.h| 2 +
src/include/nodes/makefuncs.h | 6 +
src/include/nodes/nodes.h | 2 +
src/include/nodes/parsenodes.h| 42 ++-
src/include/optimizer/plancat.h | 4 +-
src/include/parser/kwlist.h | 3 +
src/include/parser/parse_node.h | 2 +-
.../expected/system_versioned_table.out | 188 +
src/test/regress/parallel_schedule| 2 +-
src/test/regress/serial_schedule | 1 +
.../regress/sql/system_versioned_table.sql| 104
36 files changed, 1583 insertions(+), 68 deletions(-)
diff --git a/doc/src/sgml/ref/alter_table.sgml b/doc/src/sgml/ref/alter_table.sgml
index 8403c797e2..6182cda9cb 100644
--- a/doc/src/sgml/ref/alter_table.sgml
+++ b/doc/src/sgml/ref/alter_table.sgml
@@ -41,7 +41,9 @@ ALTER TABLE [ IF EXISTS ] name
where action is one of:
ADD [ COLUMN ] [ IF NOT EXISTS ] column_name data_type [ COLLATE collation ] [ column_constraint [ ... ] ]
+ADD SYSTEM VERSIONING
DROP [ COLUMN ] [ IF EXISTS ] column_name [ RESTRICT | CASCADE ]
+DROP SYSTEM VERSIONING
ALTER [ COLUMN ] column_name [ SET DATA ] TYPE data_type [ COLLATE collation ] [ USING expression ]
ALTER [ COLUMN ] column_name SET DEFAULT expression
ALTER [ COLUMN ] column_name DROP DEFAULT
@@ -158,6 +160,16 @@ WITH ( MODULUS numeric_literal, REM
+
+ADD SYSTEM VERSIONING
+
+
+ This form adds system versioning columns to the table, using default column
+ name of system versioning which is StartTime and EndtTime.
+
+
+
+
DROP COLUMN [ IF EXISTS ]
@@ -177,6 +189,17 @@ WITH ( MODULUS numeric_literal, REM
+
+DROP SYSTEM VERSIONING
+
+
+ This form drops system versioning columns from a table. Indexes and
+ table constraints involving the columns will be automatically
+ dropped as well.
+
+
+
+
SET DATA TYPE
diff --git a/doc/src/sgml/ref/create_table.sgml b/doc/src/sgml/ref/create_table.sgml
index 4a2b6f0dae..cd1035b8d9 100644
--- a/doc/src/sgml/ref/create_table.sgml
+++ b/doc/src/sgml/ref/create_table.sgml
@@ -31,6 +31,7 @@ CREATE [ [ GLOBAL | LOCAL ] { TEMPORARY | TEMP } | UNLOGGED ] TABLE [ IF NOT EXI
[ PARTITION BY { RANGE | LIST | HASH } ( { column_name | ( expression ) } [ COLLATE collation ] [ opclass ] [, ... ] ) ]
[ USING method ]
[ WITH ( storage_parameter [= value] [, ... ] ) | WITHOUT OIDS ]
+[ WITH SYSTEM VERSIONING ]
[ ON COMMIT { PRESERVE ROWS | DELETE ROWS | DROP } ]
[ TABLESPACE tablespace_name ]
@@ -67,8 +68,11 @@ CREATE [ [ GLOBAL | LOCAL ] { TEMPORARY | TEMP } | UNLOGGED ] TABLE [ IF NOT EXI
DEFAULT default_expr |
GENERATED ALWAYS AS ( generation_expr ) STORED |
GENERATED { ALWAYS | BY DEFAULT } AS IDENTITY [ ( sequence_options ) ] |
+ GENERATED ALWAYS AS ROW START |
+ GENERATED ALWAYS AS ROW END |
UNIQUE index_parameters |
PRIMARY KEY index_para
Re: WIP: System Versioned Temporal Table
Hi,
Attached is a complete patch and also contain a fix for your comments
regards
Surafel
From 0a1e51b6fcce03014e3ee355d42443add4da7a28 Mon Sep 17 00:00:00 2001
From: Surafel Temesgen
Date: Wed, 1 Jan 2020 13:27:25 +0300
Subject: [PATCH] system versioned temporal table
---
doc/src/sgml/ref/alter_table.sgml | 23 ++
doc/src/sgml/ref/create_table.sgml| 47
doc/src/sgml/ref/select.sgml | 37 +++
src/backend/access/common/tupdesc.c | 4 +
src/backend/commands/copy.c | 5 +
src/backend/commands/tablecmds.c | 49 +++-
src/backend/commands/view.c | 6 +
src/backend/executor/nodeModifyTable.c| 180 ++
src/backend/nodes/copyfuncs.c | 31 +++
src/backend/nodes/equalfuncs.c| 28 +++
src/backend/nodes/makefuncs.c | 120 ++
src/backend/nodes/outfuncs.c | 1 +
src/backend/optimizer/plan/planner.c | 8 +
src/backend/optimizer/plan/subselect.c| 19 ++
src/backend/optimizer/util/plancat.c | 188 +++
src/backend/parser/analyze.c | 9 +
src/backend/parser/gram.y | 220 +-
src/backend/parser/parse_clause.c | 73 +-
src/backend/parser/parse_relation.c | 4 +
src/backend/parser/parse_utilcmd.c| 133 ++-
src/backend/tcop/utility.c| 61 +
src/backend/utils/cache/relcache.c| 3 +
src/bin/pg_dump/pg_dump.c | 4 +
src/bin/psql/describe.c | 6 +-
src/include/access/tupdesc.h | 1 +
src/include/catalog/pg_attribute.h| 3 +
src/include/executor/nodeModifyTable.h| 2 +
src/include/nodes/makefuncs.h | 6 +
src/include/nodes/nodes.h | 2 +
src/include/nodes/parsenodes.h| 42 +++-
src/include/optimizer/plancat.h | 4 +-
src/include/parser/kwlist.h | 3 +
src/include/parser/parse_node.h | 2 +-
.../expected/system_versioned_table.out | 188 +++
src/test/regress/parallel_schedule| 2 +-
src/test/regress/serial_schedule | 1 +
.../regress/sql/system_versioned_table.sql| 104 +
37 files changed, 1553 insertions(+), 66 deletions(-)
diff --git a/doc/src/sgml/ref/alter_table.sgml b/doc/src/sgml/ref/alter_table.sgml
index 8403c797e2..6182cda9cb 100644
--- a/doc/src/sgml/ref/alter_table.sgml
+++ b/doc/src/sgml/ref/alter_table.sgml
@@ -41,7 +41,9 @@ ALTER TABLE [ IF EXISTS ] name
where action is one of:
ADD [ COLUMN ] [ IF NOT EXISTS ] column_name data_type [ COLLATE collation ] [ column_constraint [ ... ] ]
+ADD SYSTEM VERSIONING
DROP [ COLUMN ] [ IF EXISTS ] column_name [ RESTRICT | CASCADE ]
+DROP SYSTEM VERSIONING
ALTER [ COLUMN ] column_name [ SET DATA ] TYPE data_type [ COLLATE collation ] [ USING expression ]
ALTER [ COLUMN ] column_name SET DEFAULT expression
ALTER [ COLUMN ] column_name DROP DEFAULT
@@ -158,6 +160,16 @@ WITH ( MODULUS numeric_literal, REM
+
+ADD SYSTEM VERSIONING
+
+
+ This form adds system versioning columns to the table, using default column
+ name of system versioning which is StartTime and EndtTime.
+
+
+
+
DROP COLUMN [ IF EXISTS ]
@@ -177,6 +189,17 @@ WITH ( MODULUS numeric_literal, REM
+
+DROP SYSTEM VERSIONING
+
+
+ This form drops system versioning columns from a table. Indexes and
+ table constraints involving the columns will be automatically
+ dropped as well.
+
+
+
+
SET DATA TYPE
diff --git a/doc/src/sgml/ref/create_table.sgml b/doc/src/sgml/ref/create_table.sgml
index 4a2b6f0dae..cd1035b8d9 100644
--- a/doc/src/sgml/ref/create_table.sgml
+++ b/doc/src/sgml/ref/create_table.sgml
@@ -31,6 +31,7 @@ CREATE [ [ GLOBAL | LOCAL ] { TEMPORARY | TEMP } | UNLOGGED ] TABLE [ IF NOT EXI
[ PARTITION BY { RANGE | LIST | HASH } ( { column_name | ( expression ) } [ COLLATE collation ] [ opclass ] [, ... ] ) ]
[ USING method ]
[ WITH ( storage_parameter [= value] [, ... ] ) | WITHOUT OIDS ]
+[ WITH SYSTEM VERSIONING ]
[ ON COMMIT { PRESERVE ROWS | DELETE ROWS | DROP } ]
[ TABLESPACE tablespace_name ]
@@ -67,8 +68,11 @@ CREATE [ [ GLOBAL | LOCAL ] { TEMPORARY | TEMP } | UNLOGGED ] TABLE [ IF NOT EXI
DEFAULT default_expr |
GENERATED ALWAYS AS ( generation_expr ) STORED |
GENERATED { ALWAYS | BY DEFAULT } AS IDENTITY [ ( sequence_options ) ] |
+ GENERATED ALWAYS AS ROW START |
+ GENERATED ALWAYS AS ROW END |
UNIQUE index_parameters |
PRIMARY KEY index_parameters |
+ PERIOD FOR SYSTEM_TIME ( row_start_time_column, row_end_time_column ) |
REFERENCES reftable [ ( refcolumn ) ] [ MATCH
Re: Conflict handling for COPY FROM
On Thu, Dec 12, 2019 at 7:51 AM asaba.takan...@fujitsu.com <
asaba.takan...@fujitsu.com> wrote:
> 2. I have a question about copy meta-command.
> When I executed copy meta-command, output wasn't displayed.
> Does it correspond to copy meta-command?
>
>
Fixed
regards
Surafel
diff --git a/doc/src/sgml/ref/copy.sgml b/doc/src/sgml/ref/copy.sgml
index d9b7c4d0d4..a0ac5b4ef7 100644
--- a/doc/src/sgml/ref/copy.sgml
+++ b/doc/src/sgml/ref/copy.sgml
@@ -44,6 +44,7 @@ COPY { table_name [ ( column_name [, ...] )
FORCE_NULL ( column_name [, ...] )
ENCODING 'encoding_name'
+ERROR_LIMIT 'limit_number'
@@ -355,6 +356,26 @@ COPY { table_name [ (
+
+ERROR_LIMIT
+
+
+ Enables ignoring of errored out rows up to limit_number. If limit_number is set
+ to -1, then all errors will be ignored.
+
+
+
+ Currently, only unique or exclusion constraint violation
+ and rows formatting errors are ignored. Malformed
+ rows will rise warnings, while constraint violating rows
+ will be returned back to the caller.
+
+
+
+
+
WHERE
diff --git a/src/backend/commands/copy.c b/src/backend/commands/copy.c
index e17d8c760f..c911b3d0c2 100644
--- a/src/backend/commands/copy.c
+++ b/src/backend/commands/copy.c
@@ -24,6 +24,7 @@
#include "access/tableam.h"
#include "access/xact.h"
#include "access/xlog.h"
+#include "access/printtup.h"
#include "catalog/dependency.h"
#include "catalog/pg_authid.h"
#include "catalog/pg_type.h"
@@ -48,7 +49,9 @@
#include "port/pg_bswap.h"
#include "rewrite/rewriteHandler.h"
#include "storage/fd.h"
+#include "storage/lmgr.h"
#include "tcop/tcopprot.h"
+#include "tcop/pquery.h"
#include "utils/builtins.h"
#include "utils/lsyscache.h"
#include "utils/memutils.h"
@@ -154,6 +157,7 @@ typedef struct CopyStateData
List *convert_select; /* list of column names (can be NIL) */
bool *convert_select_flags; /* per-column CSV/TEXT CS flags */
Node *whereClause; /* WHERE condition (or NULL) */
+ int error_limit; /* total number of error to ignore */
/* these are just for error messages, see CopyFromErrorCallback */
const char *cur_relname; /* table name for error messages */
@@ -183,6 +187,9 @@ typedef struct CopyStateData
bool volatile_defexprs; /* is any of defexprs volatile? */
List *range_table;
ExprState *qualexpr;
+ bool ignore_error; /* is ignore error specified? */
+ bool ignore_all_error; /* is error_limit -1 (ignore all error)
+ * specified? */
TransitionCaptureState *transition_capture;
@@ -837,7 +844,7 @@ CopyLoadRawBuf(CopyState cstate)
void
DoCopy(ParseState *pstate, const CopyStmt *stmt,
int stmt_location, int stmt_len,
- uint64 *processed)
+ uint64 *processed, DestReceiver *dest)
{
CopyState cstate;
bool is_from = stmt->is_from;
@@ -1068,7 +1075,7 @@ DoCopy(ParseState *pstate, const CopyStmt *stmt,
cstate = BeginCopyFrom(pstate, rel, stmt->filename, stmt->is_program,
NULL, stmt->attlist, stmt->options);
cstate->whereClause = whereClause;
- *processed = CopyFrom(cstate); /* copy from file to database */
+ *processed = CopyFrom(cstate, dest); /* copy from file to database */
EndCopyFrom(cstate);
}
else
@@ -1290,6 +1297,18 @@ ProcessCopyOptions(ParseState *pstate,
defel->defname),
parser_errposition(pstate, defel->location)));
}
+ 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;
+ }
else
ereport(ERROR,
(errcode(ERRCODE_SYNTAX_ERROR),
@@ -1440,6 +1459,10 @@ ProcessCopyOptions(ParseState *pstate,
ereport(ERROR,
(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
errmsg("CSV quote character must not appear in the NULL specification")));
+ if (cstate->ignore_error && !cstate->is_copy_from)
+ ereport(ERROR,
+(errcode(ERRCODE_SYNTAX_ERROR),
+ errmsg("ERROR LIMIT only available using COPY FROM")));
}
/*
@@ -2653,7 +2676,7 @@ CopyMultiInsertInfoStore(CopyMultiInsertInfo *miinfo, ResultRelInfo *rri,
* Copy FROM file to relation.
*/
uint64
-CopyFrom(CopyState cstate)
+CopyFrom(CopyState cstate, DestReceiver *dest)
{
ResultRelInfo *resultRelInfo;
ResultRelInfo *target_resultRelInfo;
@@ -2675,6 +2698,7 @@ CopyFrom(CopyState cstate)
bool has_before_insert_row_trig;
bool has_instead_insert_row_trig;
bool leafpart_use_multi_insert = false;
+ Portal portal = NULL;
Assert(cstate->rel);
@@ -2838,7 +2862,19 @@ CopyFrom(CopyState cstate)
/* Verify the named relation is a valid target for INSERT */
CheckValidResultRel(resultRelInfo, CMD_INSERT);
- ExecOpenIndices(resultRelInfo, false);
+
Re: Conflict handling for COPY FROM
Hi Asaba, On Thu, Dec 12, 2019 at 7:51 AM asaba.takan...@fujitsu.com < asaba.takan...@fujitsu.com> wrote: > Hello Surafel, > > I'm very interested in this patch. > Although I'm a beginner,I would like to participate in the development of > PostgreSQL. > > > 1. I want to suggest new output format. > In my opinion, it's kind to display description of output and add "line > number" and "error" to output. > For example, > > error lines > > line number | first | second | third | error > +---++---+ > 1 | 1 | 10 | 0.5 | UNIQUE > 2 | 2 | 42 | 0.1 |CHECK > 3 | 3 | NULL | 0 | NOT NULL > (3 rows) > > Although only unique or exclusion constraint violation returned back to > the caller currently, > I think that column "error" will be useful when it becomes possible to > handle other types of errors(check, not-null and so on). > currently we can't get violation kind in speculative insertion > If you assume that users re-execute COPY FROM with the output lines as > input, these columns are obstacles. > Therefore I think that this output format should be displayed only when we > set new option(for example ERROR_VERBOSE) like "COPY FROM ... > ERROR_VERBOSE;". > > i agree adding optional feature for this is useful in same scenario but i think its a material for future improvement after basic feature done. > > 2. I have a question about copy meta-command. > When I executed copy meta-command, output wasn't displayed. > Does it correspond to copy meta-command? > > okay . i will look at it thank you regards Surafel
Re: FETCH FIRST clause WITH TIES option
On Thu, Nov 28, 2019 at 12:36 AM Alvaro Herrera
wrote:
> Thanks.
>
> (I would suggest renaming the new state LIMIT_WINDOWEND_TIES, because it
> seems to convey what it does a little better.)
>
> I think you should add a /* fall-though */ comment after changing state.
> Like this (this flow seems clearer; also DRY):
>
> if (!node->noCount &&
> node->position - node->offset >= node->count)
> {
> if (node->limitOption == LIMIT_OPTION_COUNT)
> {
> node->lstate = LIMIT_WINDOWEND;
> return NULL;
> }
> else
> {
> node->lstate = LIMIT_WINDOWEND_TIES;
> /* fall-through */
> }
> }
> else
> ...
>
>
changed
> I've been playing with this a little more, and I think you've overlooked
> a few places in ExecLimit that need to be changed. In the regression
> database, I tried this:
>
> 55432 13devel 17282=# begin;
> BEGIN
> 55432 13devel 17282=# declare c1 cursor for select * from int8_tbl order
> by q1 fetch first 2 rows with ties;
> DECLARE CURSOR
> 55432 13devel 17282=# fetch all in c1;
> q1 │q2
> ─┼──
> 123 │ 456
> 123 │ 4567890123456789
> (2 filas)
>
> 55432 13devel 17282=# fetch all in c1;
> q1 │ q2
> ┼
> (0 filas)
>
> 55432 13devel 17282=# fetch forward all in c1;
> q1 │ q2
> ┼
> (0 filas)
>
> So far so good .. things look normal. But here's where things get
> crazy:
>
> 55432 13devel 17282=# fetch backward all in c1;
> q1│q2
> ──┼──
> 4567890123456789 │ 123
> 123 │ 4567890123456789
> (2 filas)
>
> (huh???)
>
> 55432 13devel 17282=# fetch backward all in c1;
> q1 │ q2
> ┼
> (0 filas)
>
> Okay -- ignoring the fact that we got a row that should not be in the
> result, we've exhausted the cursor going back. Let's scan it again from
> the start:
>
> 55432 13devel 17282=# fetch forward all in c1;
> q1│q2
> ──┼───
> 123 │ 4567890123456789
> 4567890123456789 │ 123
> 4567890123456789 │ 4567890123456789
> 4567890123456789 │ -4567890123456789
> (4 filas)
>
> This is just crazy.
>
> I think you need to stare a that thing a little harder.
>
>
This is because the new state didn't know about backward scan
and there was off by one error it scan one position past to detect
ties. The attached patch fix both
regards
Surafel
From cad411bf6bd5ffc7f5bb5cb8d9ce456fc55ac694 Mon Sep 17 00:00:00 2001
From: Surafel Temesgen
Date: Thu, 28 Nov 2019 16:18:45 +0300
Subject: [PATCH] FETCH FIRST clause WITH TIES option
---
doc/src/sgml/ref/select.sgml| 15 ++-
src/backend/catalog/sql_features.txt| 2 +-
src/backend/executor/nodeLimit.c| 156 +---
src/backend/nodes/copyfuncs.c | 7 ++
src/backend/nodes/equalfuncs.c | 2 +
src/backend/nodes/outfuncs.c| 7 ++
src/backend/nodes/readfuncs.c | 6 +
src/backend/optimizer/plan/createplan.c | 44 ++-
src/backend/optimizer/plan/planner.c| 1 +
src/backend/optimizer/util/pathnode.c | 2 +
src/backend/parser/analyze.c| 3 +
src/backend/parser/gram.y | 117 ++
src/backend/utils/adt/ruleutils.c | 10 +-
src/include/nodes/execnodes.h | 4 +
src/include/nodes/nodes.h | 13 ++
src/include/nodes/parsenodes.h | 2 +
src/include/nodes/pathnodes.h | 1 +
src/include/nodes/plannodes.h | 5 +
src/include/optimizer/pathnode.h| 1 +
src/include/optimizer/planmain.h| 3 +-
src/test/regress/expected/limit.out | 104
src/test/regress/sql/limit.sql | 31 +
22 files changed, 482 insertions(+), 54 deletions(-)
diff --git a/doc/src/sgml/ref/select.sgml b/doc/src/sgml/ref/select.sgml
index 691e402803..2b11c38087 100644
--- a/doc/src/sgml/ref/select.sgml
+++ b/doc/src/sgml/ref/select.sgml
@@ -44,7 +44,7 @@ SELECT [ ALL | DISTINCT [ ON ( expressionexpression [ ASC | DESC | USING operator ] [ NULLS { FIRST | LAST } ] [, ...] ]
[ LIMIT { count | ALL } ]
[ OFFSET start [ ROW | ROWS ] ]
-[ FETCH { FIRST | NEXT } [ count ] { ROW | ROWS } ONLY ]
+[ FETCH { FIRST | NEXT } [ count ] { ROW | ROWS } { ONLY | WITH TIES } ]
[ FOR { UPDATE | NO KEY UPDATE | SHARE | KEY SHARE } [ OF table_
Re: FETCH FIRST clause WITH TIES option
On Tue, Nov 26, 2019 at 6:09 PM Alvaro Herrera
wrote:
> I rebased this patch, and my proposed changes are in 0002.
>
Thank you
>
> Looking at the changes in ExecLimit, I'm wondering if it would be better
> to add a new state to the state machine there -- instead of doing all
> the work in duplicative code in the LIMIT_INWINDOW case, have that one
> only save the current end-of-window tuple and jump to
> LIMIT_WINDOWEND_WITH_TIE (or something) which then returns all tuples that
> match the current one. I think that would end up being cleaner. Please
> research.
>
Done
regards
Surafel
From 9d02731c2d96bedb72d5f526a64f1fd28ee334c5 Mon Sep 17 00:00:00 2001
From: Surafel Temesgen
Date: Wed, 27 Nov 2019 11:46:45 +0300
Subject: [PATCH] limit with ties
---
doc/src/sgml/ref/select.sgml| 15 +--
src/backend/catalog/sql_features.txt| 2 +-
src/backend/executor/nodeLimit.c| 117
src/backend/nodes/copyfuncs.c | 7 ++
src/backend/nodes/equalfuncs.c | 2 +
src/backend/nodes/outfuncs.c| 7 ++
src/backend/nodes/readfuncs.c | 6 ++
src/backend/optimizer/plan/createplan.c | 44 -
src/backend/optimizer/plan/planner.c| 1 +
src/backend/optimizer/util/pathnode.c | 2 +
src/backend/parser/analyze.c| 3 +
src/backend/parser/gram.y | 117 ++--
src/backend/utils/adt/ruleutils.c | 10 +-
src/include/nodes/execnodes.h | 4 +
src/include/nodes/nodes.h | 13 +++
src/include/nodes/parsenodes.h | 2 +
src/include/nodes/pathnodes.h | 1 +
src/include/nodes/plannodes.h | 5 +
src/include/optimizer/pathnode.h| 1 +
src/include/optimizer/planmain.h| 3 +-
src/test/regress/expected/limit.out | 52 +++
src/test/regress/sql/limit.sql | 21 +
22 files changed, 381 insertions(+), 54 deletions(-)
diff --git a/doc/src/sgml/ref/select.sgml b/doc/src/sgml/ref/select.sgml
index 691e402803..2b11c38087 100644
--- a/doc/src/sgml/ref/select.sgml
+++ b/doc/src/sgml/ref/select.sgml
@@ -44,7 +44,7 @@ SELECT [ ALL | DISTINCT [ ON ( expressionexpression [ ASC | DESC | USING operator ] [ NULLS { FIRST | LAST } ] [, ...] ]
[ LIMIT { count | ALL } ]
[ OFFSET start [ ROW | ROWS ] ]
-[ FETCH { FIRST | NEXT } [ count ] { ROW | ROWS } ONLY ]
+[ FETCH { FIRST | NEXT } [ count ] { ROW | ROWS } { ONLY | WITH TIES } ]
[ FOR { UPDATE | NO KEY UPDATE | SHARE | KEY SHARE } [ OF table_name [, ...] ] [ NOWAIT | SKIP LOCKED ] [...] ]
where from_item can be one of:
@@ -1438,7 +1438,7 @@ OFFSET start
which PostgreSQL also supports. It is:
OFFSET start { ROW | ROWS }
-FETCH { FIRST | NEXT } [ count ] { ROW | ROWS } ONLY
+FETCH { FIRST | NEXT } [ count ] { ROW | ROWS } { ONLY | WITH TIES }
In this syntax, the start
or count value is required by
@@ -1448,10 +1448,13 @@ FETCH { FIRST | NEXT } [ count ] {
ambiguity.
If count is
omitted in a FETCH clause, it defaults to 1.
-ROW
-and ROWS as well as FIRST
-and NEXT are noise words that don't influence
-the effects of these clauses.
+The WITH TIES option is used to return any additional
+rows that tie for the last place in the result set according to
+ORDER BY clause; ORDER BY
+is mandatory in this case.
+ROW and ROWS as well as
+FIRST and NEXT are noise
+words that don't influence the effects of these clauses.
According to the standard, the OFFSET clause must come
before the FETCH clause if both are present; but
PostgreSQL is laxer and allows either order.
diff --git a/src/backend/catalog/sql_features.txt b/src/backend/catalog/sql_features.txt
index ab3e381cff..cd720eb19a 100644
--- a/src/backend/catalog/sql_features.txt
+++ b/src/backend/catalog/sql_features.txt
@@ -345,7 +345,7 @@ F863 Nested in YES
F864 Top-level in views YES
F865 in YES
F866 FETCH FIRST clause: PERCENT option NO
-F867 FETCH FIRST clause: WITH TIES option NO
+F867 FETCH FIRST clause: WITH TIES option YES
R010 Row pattern recognition: FROM clause NO
R020 Row pattern recognition: WINDOW clause NO
R030 Row pattern recognition: full aggregate support NO
diff --git a/src/backend/executor/nodeLimit.c b/src/backend/executor/nodeLimit.c
index 5e4d02ce4a..1b96a6ee8f 100644
--- a/src/backend/executor/nodeLimit.c
+++ b/src/backend/executor/nodeLimit.c
@@ -41,6 +41,7 @@ static TupleTableSlot * /* return: a tuple or NULL */
ExecLimit(PlanState *pstate)
{
LimitState *node = castNode(LimitState, pstate);
+ ExprContext *econtext = node->ps.ps_ExprContext;
ScanDirection direction;
TupleTableSlot *slot;
PlanState *outerPlan;
@@ -102,6 +103,15 @@ ExecLimit(PlanState *pstate)
node->lstate = LIMIT_EMPTY;
return NULL;
}
+/*
+ * Tuple at limit is needed
Re: Conflict handling for COPY FROM
On Thu, Nov 21, 2019 at 4:22 PM Alexey Kondratov
wrote:
>
> Now the whole patch works exactly as expected for me and I cannot find
> any new technical flaws. However, the doc is rather vague, especially
> these places:
>
> + specifying it to -1 returns all error record.
>
> Actually, we return only rows with constraint violation, but malformed
> rows are ignored with warning. I guess that we simply cannot return
> malformed rows back to the caller in the same way as with constraint
> violation, since we cannot figure out (in general) which column
> corresponds to which type if there are extra or missing columns.
>
> + and same record formatting error is ignored.
>
> I can get it, but it definitely should be reworded.
>
> What about something like this?
>
> +
> + ERROR_LIMIT
> +
> +
> + Enables ignoring of errored out rows up to + class="parameter">limit_number. If + class="parameter">limit_number is set
> + to -1, then all errors will be ignored.
> +
> +
> +
> + Currently, only unique or exclusion constraint violation
> + and rows formatting errors are ignored. Malformed
> + rows will rise warnings, while constraint violating rows
> + will be returned back to the caller.
> +
> +
> +
> +
>
>
>
It is better so changed
regards
Surafel
diff --git a/doc/src/sgml/ref/copy.sgml b/doc/src/sgml/ref/copy.sgml
index d9b7c4d0d4..a0ac5b4ef7 100644
--- a/doc/src/sgml/ref/copy.sgml
+++ b/doc/src/sgml/ref/copy.sgml
@@ -44,6 +44,7 @@ COPY { table_name [ ( column_name [, ...] )
FORCE_NULL ( column_name [, ...] )
ENCODING 'encoding_name'
+ERROR_LIMIT 'limit_number'
@@ -355,6 +356,26 @@ COPY { table_name [ (
+
+ERROR_LIMIT
+
+
+ Enables ignoring of errored out rows up to limit_number. If limit_number is set
+ to -1, then all errors will be ignored.
+
+
+
+ Currently, only unique or exclusion constraint violation
+ and rows formatting errors are ignored. Malformed
+ rows will rise warnings, while constraint violating rows
+ will be returned back to the caller.
+
+
+
+
+
WHERE
diff --git a/src/backend/commands/copy.c b/src/backend/commands/copy.c
index e17d8c760f..c911b3d0c2 100644
--- a/src/backend/commands/copy.c
+++ b/src/backend/commands/copy.c
@@ -24,6 +24,7 @@
#include "access/tableam.h"
#include "access/xact.h"
#include "access/xlog.h"
+#include "access/printtup.h"
#include "catalog/dependency.h"
#include "catalog/pg_authid.h"
#include "catalog/pg_type.h"
@@ -48,7 +49,9 @@
#include "port/pg_bswap.h"
#include "rewrite/rewriteHandler.h"
#include "storage/fd.h"
+#include "storage/lmgr.h"
#include "tcop/tcopprot.h"
+#include "tcop/pquery.h"
#include "utils/builtins.h"
#include "utils/lsyscache.h"
#include "utils/memutils.h"
@@ -154,6 +157,7 @@ typedef struct CopyStateData
List *convert_select; /* list of column names (can be NIL) */
bool *convert_select_flags; /* per-column CSV/TEXT CS flags */
Node *whereClause; /* WHERE condition (or NULL) */
+ int error_limit; /* total number of error to ignore */
/* these are just for error messages, see CopyFromErrorCallback */
const char *cur_relname; /* table name for error messages */
@@ -183,6 +187,9 @@ typedef struct CopyStateData
bool volatile_defexprs; /* is any of defexprs volatile? */
List *range_table;
ExprState *qualexpr;
+ bool ignore_error; /* is ignore error specified? */
+ bool ignore_all_error; /* is error_limit -1 (ignore all error)
+ * specified? */
TransitionCaptureState *transition_capture;
@@ -837,7 +844,7 @@ CopyLoadRawBuf(CopyState cstate)
void
DoCopy(ParseState *pstate, const CopyStmt *stmt,
int stmt_location, int stmt_len,
- uint64 *processed)
+ uint64 *processed, DestReceiver *dest)
{
CopyState cstate;
bool is_from = stmt->is_from;
@@ -1068,7 +1075,7 @@ DoCopy(ParseState *pstate, const CopyStmt *stmt,
cstate = BeginCopyFrom(pstate, rel, stmt->filename, stmt->is_program,
NULL, stmt->attlist, stmt->options);
cstate->whereClause = whereClause;
- *processed = CopyFrom(cstate); /* copy from file to database */
+ *processed = CopyFrom(cstate, dest); /* copy from file to database */
EndCopyFrom(cstate);
}
else
@@ -1290,6 +1297,18 @@ ProcessCopyOptions(ParseState *pstate,
defel->defname),
parser_errposition(pstate, defel->location)));
}
+ 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;
+ }
else
ereport(ERROR,
(errcode(ERRCODE_SYNTAX_ERROR),
@@ -1440,6 +1459,10 @@
Re: Conflict handling for COPY FROM
On Fri, Nov 15, 2019 at 6:24 PM Alexey Kondratov
wrote:
> On 11.11.2019 16:00, Surafel Temesgen wrote:
> >
> >
> > 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
>
> Thanks, now it works with my tests.
>
> 1) Maybe it is fine, but now I do not like this part:
>
> +portal = GetPortalByName("");
> +dest = CreateDestReceiver(DestRemote);
> +SetRemoteDestReceiverParams(dest, portal);
> +dest->rStartup(dest, (int) CMD_SELECT, tupDesc);
>
> Here you implicitly use the fact that portal with a blank name is always
> created in exec_simple_query before we get to this point. Next, you
> create new DestReceiver and set it to this portal, but it is also
> already created and set in the exec_simple_query.
>
> Would it be better if you just explicitly pass ready DestReceiver to
> DoCopy (similarly to how it is done for T_ExecuteStmt / ExecuteQuery),
>
>
Good idea .Thank you
>
> 2) My second concern is that you use three internal flags to track
> errors limit:
>
> +interror_limit;/* total number of error to ignore */
> +boolignore_error;/* is ignore error specified? */
> +boolignore_all_error;/* is error_limit -1 (ignore all
> error)
> + * specified? */
>
> Though it seems that we can just leave error_limit as a user-defined
> constant and track errors with something like errors_count. In that case
> you do not need auxiliary ignore_all_error flag. But probably it is a
> matter of personal choice.
>
>
using bool flags will save as from using integer type as a boolean and hold
the fact
error limit was specified even if it became zero and it seems to me it is
straightforward
to treat ignore_all_error separately.
Attache is the patch that use already created DestReceiver
regards
Surafel
diff --git a/doc/src/sgml/ref/copy.sgml b/doc/src/sgml/ref/copy.sgml
index d9b7c4d0d4..ffcfe1e8d3 100644
--- a/doc/src/sgml/ref/copy.sgml
+++ b/doc/src/sgml/ref/copy.sgml
@@ -44,6 +44,7 @@ COPY { table_name [ ( column_name [, ...] )
FORCE_NULL ( column_name [, ...] )
ENCODING 'encoding_name'
+ERROR_LIMIT 'limit_number'
@@ -355,6 +356,23 @@ COPY { table_name [ (
+
+ERROR_LIMIT
+
+
+ Specifies to return error record up to limit_number number.
+ specifying it to -1 returns all error record.
+
+
+
+ Currently, only unique or exclusion constraint violation
+ and same record formatting error is ignored.
+
+
+
+
+
WHERE
diff --git a/src/backend/commands/copy.c b/src/backend/commands/copy.c
index e17d8c760f..c911b3d0c2 100644
--- a/src/backend/commands/copy.c
+++ b/src/backend/commands/copy.c
@@ -24,6 +24,7 @@
#include "access/tableam.h"
#include "access/xact.h"
#include "access/xlog.h"
+#include "access/printtup.h"
#include "catalog/dependency.h"
#include "catalog/pg_authid.h"
#include "catalog/pg_type.h"
@@ -48,7 +49,9 @@
#include "port/pg_bswap.h"
#include "rewrite/rewriteHandler.h"
#include "storage/fd.h"
+#include "storage/lmgr.h"
#include "tcop/tcopprot.h"
+#include "tcop/pquery.h"
#include "utils/builtins.h"
#include "utils/lsyscache.h"
#include "utils/memutils.h"
@@ -154,6 +157,7 @@ typedef struct CopyStateData
List *convert_select; /* list of column names (can be NIL) */
bool *convert_select_flags; /* per-column CSV/TEXT CS flags */
Node *whereClause; /* WHERE condition (or NULL) */
+ int error_limit; /* total number of error to ignore */
/* these are just for error messages, see CopyFromErrorCallback */
const char *cur_relname; /* table name for error messages */
@@ -183,6 +187,9 @@ typedef struct CopyStateData
bool volatile_defexprs; /* is any of defexprs volatile? */
List *range_table;
ExprState *qualexpr;
+ bool ignore_error; /* is ignore error specified?
Re: FETCH FIRST clause WITH TIES option
On Mon, Nov 11, 2019 at 5:56 PM Alvaro Herrera wrote: > First, I noticed that there's a significant unanswered issue from Andrew > Gierth about this using a completely different mechanism, namely an > implicit window function. I see that Robert was concerned about the > performance of Andrew's proposed approach, but I don't see any reply > from Surafel on the whole issue. > > i don't know much about window function implementation but am sure teaching window function about limit and implementing limit *variant on top of it will not be much simpler * *and performant than the current implementation. i also think more appropriate place to * *include limit variant is limitNode not other place which can case redundancy and * *maintainability issue * *regards * *Surafel *
Re: Conflict handling for COPY FROM
On Fri, Sep 20, 2019 at 4:16 PM Alexey Kondratov
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
diff --git a/doc/src/sgml/ref/copy.sgml b/doc/src/sgml/ref/copy.sgml
index d9b7c4d0d4..ffcfe1e8d3 100644
--- a/doc/src/sgml/ref/copy.sgml
+++ b/doc/src/sgml/ref/copy.sgml
@@ -44,6 +44,7 @@ COPY { table_name [ ( column_name [, ...] )
FORCE_NULL ( column_name [, ...] )
ENCODING 'encoding_name'
+ERROR_LIMIT 'limit_number'
@@ -355,6 +356,23 @@ COPY { table_name [ (
+
+ERROR_LIMIT
+
+
+ Specifies to return error record up to limit_number number.
+ specifying it to -1 returns all error record.
+
+
+
+ Currently, only unique or exclusion constraint violation
+ and same record formatting error is ignored.
+
+
+
+
+
WHERE
diff --git a/src/backend/commands/copy.c b/src/backend/commands/copy.c
index e17d8c760f..c2314480b2 100644
--- a/src/backend/commands/copy.c
+++ b/src/backend/commands/copy.c
@@ -24,6 +24,7 @@
#include "access/tableam.h"
#include "access/xact.h"
#include "access/xlog.h"
+#include "access/printtup.h"
#include "catalog/dependency.h"
#include "catalog/pg_authid.h"
#include "catalog/pg_type.h"
@@ -48,7 +49,9 @@
#include "port/pg_bswap.h"
#include "rewrite/rewriteHandler.h"
#include "storage/fd.h"
+#include "storage/lmgr.h"
#include "tcop/tcopprot.h"
+#include "tcop/pquery.h"
#include "utils/builtins.h"
#include "utils/lsyscache.h"
#include "utils/memutils.h"
@@ -154,6 +157,7 @@ typedef struct CopyStateData
List *convert_select; /* list of column names (can be NIL) */
bool *convert_select_flags; /* per-column CSV/TEXT CS flags */
Node *whereClause; /* WHERE condition (or NULL) */
+ int error_limit; /* total number of error to ignore */
/* these are just for error messages, see CopyFromErrorCallback */
const char *cur_relname; /* table name for error messages */
@@ -183,6 +187,9 @@ typedef struct CopyStateData
bool volatile_defexprs; /* is any of defexprs volatile? */
List *range_table;
ExprState *qualexpr;
+ bool ignore_error; /* is ignore error specified? */
+ bool ignore_all_error; /* is error_limit -1 (ignore all error)
+ * specified? */
Re: WIP: System Versioned Temporal Table
On Fri, Oct 25, 2019 at 10:45 PM Vik Fearing wrote: > > > > I don't understand what you mean by this. > > > > > > > > The primary purpose of adding row end time to primary key is to allow > > duplicate value to be inserted into a table with keeping constraint in > > current data but it can be duplicated in history data. Adding row > > start time column to primary key will eliminate this uniqueness for > > current data which is not correct > > > How? The primary/unique keys must always be unique at every point in time. > >From user prospect it is acceptable to delete and reinsert a record with the same key value multiple time which means there will be multiple record with the same key value in a history data but there is only one values in current data as a table without system versioning do .I add row end time column to primary key to allow user supplied primary key values to be duplicated in history data which is acceptable regards Surafel
Re: WIP: System Versioned Temporal Table
On Thu, Oct 24, 2019 at 6:49 PM Vik Fearing wrote: > On 24/10/2019 16:54, Surafel Temesgen wrote: > > > > hi Vik, > > On Wed, Oct 23, 2019 at 9:02 PM Vik Fearing > > mailto:vik.fear...@2ndquadrant.com>> > wrote: > > > > > > > > If we're going to be implicitly adding stuff to the PK, we also > > need to > > add that stuff to the other unique constraints, no? And I think it > > would be better to add both the start and the end column to these > > keys. > > Most of the temporal queries will be accessing both. > > > > > > yes it have to be added to other constraint too but adding both system > > time > > to PK will violate constraint because it allow multiple data in > > current data > > > I don't understand what you mean by this. > > The primary purpose of adding row end time to primary key is to allow duplicate value to be inserted into a table with keeping constraint in current data but it can be duplicated in history data. Adding row start time column to primary key will eliminate this uniqueness for current data which is not correct regards Surafel
Re: WIP: System Versioned Temporal Table
hi Vik, On Wed, Oct 23, 2019 at 9:02 PM Vik Fearing wrote: > > If we're going to be implicitly adding stuff to the PK, we also need to > add that stuff to the other unique constraints, no? And I think it > would be better to add both the start and the end column to these keys. > Most of the temporal queries will be accessing both. > > yes it have to be added to other constraint too but adding both system time to PK will violate constraint because it allow multiple data in current data > > Why aren't you following the standard syntax here? > > > because we do have TIME and SYSTEM_P as a key word and am not sure of whether its a right thing to add other keyword that contain those two word concatenated > > Any enlightenment? > > > > There are quite a lot of typos and other things that aren't written "the > Postgres way". But before I comment on any of that, I'd like to see the > features be implemented correctly according to the SQL standard. > it is almost in sql standard syntax except the above small difference. i can correct it and post more complete patch soon. regards Surafel
WIP: System Versioned Temporal Table
Hi all ,
Temporal table is one of the main new features added in sql standard 2011.
>From that I will like to implement system versioned temporal table which
allows to keep past and present data so old data can be queried. Am propose
to implement it like below
CREATE
In create table only one table is create and both historical and current
data will be store in it. In order to make history and current data
co-exist row end time column will be added implicitly to primary key.
Regarding performance one can partition the table by row end time column
order to make history data didn't slowed performance.
INSERT
In insert row start time column and row end time column behave like a kind
of generated stored column except they store current transaction time and
highest value supported by the data type which is +infinity respectively.
DELETE and UPDATE
The old data is inserted with row end time column seated to current
transaction time
SELECT
If the query didn’t contain a filter condition that include system time
column, a filter condition will be added in early optimization that filter
history data.
Attached is WIP patch that implemented just the above and done on top of
commit b8e19b932a99a7eb5a. Temporal clause didn’t implemented yet so one
can use regular filter condition for the time being
NOTE: I implement sql standard syntax except it is PERIOD FOR SYSTEM TIME
rather than PERIOD FOR SYSTEM_TIME in CREATE TABLE statement and system
time is not selected unless explicitly asked
Any enlightenment?
regards
Surafel
diff --git a/src/backend/access/common/tupdesc.c b/src/backend/access/common/tupdesc.c
index 6bc4e4c036..c229c90e2d 100644
--- a/src/backend/access/common/tupdesc.c
+++ b/src/backend/access/common/tupdesc.c
@@ -167,6 +167,7 @@ CreateTupleDescCopyConstr(TupleDesc tupdesc)
cpy->has_not_null = constr->has_not_null;
cpy->has_generated_stored = constr->has_generated_stored;
+ cpy->is_system_versioned = constr->is_system_versioned;
if ((cpy->num_defval = constr->num_defval) > 0)
{
@@ -484,6 +485,8 @@ equalTupleDescs(TupleDesc tupdesc1, TupleDesc tupdesc2)
return false;
if (constr1->has_generated_stored != constr2->has_generated_stored)
return false;
+ if (constr1->is_system_versioned != constr2->is_system_versioned)
+ return false;
n = constr1->num_defval;
if (n != (int) constr2->num_defval)
return false;
@@ -864,6 +867,7 @@ BuildDescForRelation(List *schema)
constr->has_not_null = true;
constr->has_generated_stored = false;
+ constr->is_system_versioned= false;
constr->defval = NULL;
constr->missing = NULL;
constr->num_defval = 0;
diff --git a/src/backend/executor/nodeModifyTable.c b/src/backend/executor/nodeModifyTable.c
index c9d024ead5..340f0340bd 100644
--- a/src/backend/executor/nodeModifyTable.c
+++ b/src/backend/executor/nodeModifyTable.c
@@ -330,6 +330,120 @@ ExecComputeStoredGenerated(EState *estate, TupleTableSlot *slot)
MemoryContextSwitchTo(oldContext);
}
+/*
+ * Set row start time in row start time columns for a tuple.
+ */
+void
+ExecSetRowStartTime(EState *estate, TupleTableSlot *slot)
+{
+ ResultRelInfo *resultRelInfo = estate->es_result_relation_info;
+ Relation rel = resultRelInfo->ri_RelationDesc;
+ TupleDesc tupdesc = RelationGetDescr(rel);
+ int natts = tupdesc->natts;
+ MemoryContext oldContext;
+ Datum *values;
+ bool *nulls;
+
+ Assert(tupdesc->constr && tupdesc->constr->is_system_versioned);
+
+ oldContext = MemoryContextSwitchTo(GetPerTupleMemoryContext(estate));
+
+ values = palloc(sizeof(*values) * natts);
+ nulls = palloc(sizeof(*nulls) * natts);
+
+ slot_getallattrs(slot);
+ memcpy(nulls, slot->tts_isnull, sizeof(*nulls) * natts);
+
+ for (int i = 0; i < natts; i++)
+ {
+ Form_pg_attribute attr = TupleDescAttr(tupdesc, i);
+
+ /*
+ * We set infinity for row end time columns for a tuple because row end time is not yet known.
+ */
+ if (attr->attgenerated == ATTRIBUTE_ROW_START_TIME)
+ {
+ Datum val;
+
+ val = GetCurrentTransactionStartTimestamp();
+
+ values[i] = val;
+ nulls[i] = false;
+ }
+ else if (attr->attgenerated == ATTRIBUTE_ROW_END_TIME)
+ {
+ Datum val;
+
+ val = DirectFunctionCall3(timestamp_in,
+ CStringGetDatum("infinity"),
+ ObjectIdGetDatum(InvalidOid),
+ Int32GetDatum(-1));
+
+ values[i] = val;
+ nulls[i] = false;
+ }
+ else
+ values[i] = datumCopy(slot->tts_values[i], attr->attbyval, attr->attlen);
+ }
+
+ ExecClearTuple(slot);
+ memcpy(slot->tts_values, values, sizeof(*values) * natts);
+ memcpy(slot->tts_isnull, nulls, sizeof(*nulls) * natts);
+ ExecStoreVirtualTuple(slot);
+ ExecMaterializeSlot(slot);
+
+ MemoryContextSwitchTo(oldContext);
+}
+
+/*
+ * Set row end time in row end time columns for a tuple.
+ */
+void
+ExecSetRowEndTime(EState *estate, TupleTableSlot *slot)
+{
+ ResultRelInfo *resultRelInfo = estate->es_result_relation_info;
+ Relation rel = resultRelInfo->ri_RelationDesc;
+ TupleDesc tupdesc =
Re: FETCH FIRST clause PERCENT option
Hi Tom,
In the attached patch i include the comments given
regards
Surafel
diff --git a/contrib/postgres_fdw/postgres_fdw.c b/contrib/postgres_fdw/postgres_fdw.c
index 82d8140ba2..692d6492bd 100644
--- a/contrib/postgres_fdw/postgres_fdw.c
+++ b/contrib/postgres_fdw/postgres_fdw.c
@@ -3035,7 +3035,8 @@ estimate_path_cost_size(PlannerInfo *root,
if (fpextra && fpextra->has_limit)
{
adjust_limit_rows_costs(, _cost, _cost,
- fpextra->offset_est, fpextra->count_est);
+ fpextra->offset_est, fpextra->count_est,
+ LIMIT_OPTION_COUNT);
retrieved_rows = rows;
}
}
diff --git a/doc/src/sgml/ref/select.sgml b/doc/src/sgml/ref/select.sgml
index 06d611b64c..4ef42df51d 100644
--- a/doc/src/sgml/ref/select.sgml
+++ b/doc/src/sgml/ref/select.sgml
@@ -44,7 +44,7 @@ SELECT [ ALL | DISTINCT [ ON ( expressionexpression [ ASC | DESC | USING operator ] [ NULLS { FIRST | LAST } ] [, ...] ]
[ LIMIT { count | ALL } ]
[ OFFSET start [ ROW | ROWS ] ]
-[ FETCH { FIRST | NEXT } [ count ] { ROW | ROWS } ONLY ]
+[ FETCH { FIRST | NEXT } [ count ] [ PERCENT ] { ROW | ROWS } ONLY ]
[ FOR { UPDATE | NO KEY UPDATE | SHARE | KEY SHARE } [ OF table_name [, ...] ] [ NOWAIT | SKIP LOCKED ] [...] ]
where from_item can be one of:
@@ -1430,7 +1430,7 @@ OFFSET start
which PostgreSQL also supports. It is:
OFFSET start { ROW | ROWS }
-FETCH { FIRST | NEXT } [ count ] { ROW | ROWS } ONLY
+FETCH { FIRST | NEXT } [ count ] [ PERCENT ] { ROW | ROWS } ONLY
In this syntax, the start
or count value is required by
@@ -1440,8 +1440,10 @@ FETCH { FIRST | NEXT } [ count ] {
ambiguity.
If count is
omitted in a FETCH clause, it defaults to 1.
-ROW
-and ROWS as well as FIRST
+With PERCENT count specifies the maximum number of rows to return
+in percentage round up to the nearest integer. Using PERCENT
+is best suited to returning single-digit percentages of the query's total row count.
+ROW and ROWS as well as FIRST
and NEXT are noise words that don't influence
the effects of these clauses.
According to the standard, the OFFSET clause must come
diff --git a/src/backend/catalog/sql_features.txt b/src/backend/catalog/sql_features.txt
index 059ec02cd0..1adc312f7a 100644
--- a/src/backend/catalog/sql_features.txt
+++ b/src/backend/catalog/sql_features.txt
@@ -348,7 +348,7 @@ F862 in subqueries YES
F863 Nested in YES
F864 Top-level in views YES
F865 in YES
-F866 FETCH FIRST clause: PERCENT option NO
+F866 FETCH FIRST clause: PERCENT option YES
F867 FETCH FIRST clause: WITH TIES option NO
R010 Row pattern recognition: FROM clause NO
R020 Row pattern recognition: WINDOW clause NO
diff --git a/src/backend/executor/nodeLimit.c b/src/backend/executor/nodeLimit.c
index baa669abe8..c03bb2c971 100644
--- a/src/backend/executor/nodeLimit.c
+++ b/src/backend/executor/nodeLimit.c
@@ -21,6 +21,8 @@
#include "postgres.h"
+#include
+
#include "executor/executor.h"
#include "executor/nodeLimit.h"
#include "miscadmin.h"
@@ -52,6 +54,7 @@ ExecLimit(PlanState *pstate)
*/
direction = node->ps.state->es_direction;
outerPlan = outerPlanState(node);
+ slot = node->subSlot;
/*
* The main logic is a simple state machine.
@@ -81,7 +84,15 @@ ExecLimit(PlanState *pstate)
/*
* Check for empty window; if so, treat like empty subplan.
*/
- if (node->count <= 0 && !node->noCount)
+ if (node->limitOption == LIMIT_OPTION_PERCENT)
+ {
+if (node->percent == 0.0)
+{
+ node->lstate = LIMIT_EMPTY;
+ return NULL;
+}
+ }
+ else if (node->count <= 0 && !node->noCount)
{
node->lstate = LIMIT_EMPTY;
return NULL;
@@ -107,6 +118,16 @@ ExecLimit(PlanState *pstate)
break;
}
+ /*
+ * We may needed this tuple in subsequent scan so put it into
+ * tuplestore.
+ */
+ if (node->limitOption == LIMIT_OPTION_PERCENT)
+ {
+tuplestore_puttupleslot(node->tupleStore, slot);
+tuplestore_advance(node->tupleStore, true);
+ }
+
/*
* Okay, we have the first tuple of the window.
*/
@@ -124,6 +145,82 @@ ExecLimit(PlanState *pstate)
case LIMIT_INWINDOW:
if (ScanDirectionIsForward(direction))
{
+/*
+ * In case of coming back from backward scan the tuple is
+ * already in tuple store.
+ */
+if (node->limitOption == LIMIT_OPTION_PERCENT && node->backwardPosition > 0)
+{
+ if (tuplestore_gettupleslot_heaptuple(node->tupleStore, true, true, slot))
+ {
+ node->subSlot = slot;
+ node->position++;
+ node->backwardPosition--;
+ return slot;
+ }
+ else
+ {
+ node->lstate = LIMIT_SUBPLANEOF;
+ return NULL;
+ }
+}
+
+/*
+ * In LIMIT_OPTION_PERCENT case no need of executing outerPlan multiple
+ * times.
+ */
+if (node->limitOption == LIMIT_OPTION_PERCENT && node->reachEnd)
+{
+ node->lstate = LIMIT_WINDOWEND;
+
+
Re: Allow CLUSTER, VACUUM FULL and REINDEX to change tablespace on the fly
Hi Alexey Here are a few comment On Sat, Aug 31, 2019 at 11:54 PM wrote: > Hi hackers, > > > Anyway, new patch, which adds SET TABLESPACE to REINDEX is attached and > this functionality seems really useful, so I will be very appreciate if > someone will take a look on it. > * There are NOWAIT option in alter index, is there a reason not to have similar option here? * SET TABLESPACE command is not documented * There are multiple checking for whether the relation is temporary tables of other sessions, one in check_relation_is_movable and other independently *+ char *tablespacename; calling it new_tablespacename will make it consistent with other places *The patch did't applied cleanly http://cfbot.cputube.org/patch_24_2269.log regards Surafel
Re: FETCH FIRST clause WITH TIES option
On Fri, Sep 6, 2019 at 5:07 PM Alvaro Herrera from 2ndQuadrant <
alvhe...@alvh.no-ip.org> wrote:
> On 2019-Sep-06, Surafel Temesgen wrote:
>
> > > ... yet this doesn't appear to have resulted in any change in the code,
> > > or I just missed it. Are you going to update the patch per that?
> >
> > Its already done in v9 of the patch attached by Tomas
>
> Oh, I missed that. Great, so we're expecting some small updates from
> you then and I think you can mark ready for committer when you post it.
> Thanks.
>
>
Done
regards
Surafel
diff --git a/doc/src/sgml/ref/select.sgml b/doc/src/sgml/ref/select.sgml
index 06d611b64c..e83d309c5b 100644
--- a/doc/src/sgml/ref/select.sgml
+++ b/doc/src/sgml/ref/select.sgml
@@ -44,7 +44,7 @@ SELECT [ ALL | DISTINCT [ ON ( expressionexpression [ ASC | DESC | USING operator ] [ NULLS { FIRST | LAST } ] [, ...] ]
[ LIMIT { count | ALL } ]
[ OFFSET start [ ROW | ROWS ] ]
-[ FETCH { FIRST | NEXT } [ count ] { ROW | ROWS } ONLY ]
+[ FETCH { FIRST | NEXT } [ count ] { ROW | ROWS } { ONLY | WITH TIES } ]
[ FOR { UPDATE | NO KEY UPDATE | SHARE | KEY SHARE } [ OF table_name [, ...] ] [ NOWAIT | SKIP LOCKED ] [...] ]
where from_item can be one of:
@@ -1430,7 +1430,7 @@ OFFSET start
which PostgreSQL also supports. It is:
OFFSET start { ROW | ROWS }
-FETCH { FIRST | NEXT } [ count ] { ROW | ROWS } ONLY
+FETCH { FIRST | NEXT } [ count ] { ROW | ROWS } { ONLY | WITH TIES }
In this syntax, the start
or count value is required by
@@ -1440,7 +1440,10 @@ FETCH { FIRST | NEXT } [ count ] {
ambiguity.
If count is
omitted in a FETCH clause, it defaults to 1.
-ROW
+ROW .
+WITH TIES option is used to return two or more rows
+that tie for the last place in the result set according to ORDER BY
+clause (ORDER BY clause must be specified in this case).
and ROWS as well as FIRST
and NEXT are noise words that don't influence
the effects of these clauses.
diff --git a/src/backend/catalog/sql_features.txt b/src/backend/catalog/sql_features.txt
index 059ec02cd0..54628657e6 100644
--- a/src/backend/catalog/sql_features.txt
+++ b/src/backend/catalog/sql_features.txt
@@ -349,7 +349,7 @@ F863 Nested in YES
F864 Top-level in views YES
F865 in YES
F866 FETCH FIRST clause: PERCENT option NO
-F867 FETCH FIRST clause: WITH TIES option NO
+F867 FETCH FIRST clause: WITH TIES option YES
R010 Row pattern recognition: FROM clause NO
R020 Row pattern recognition: WINDOW clause NO
R030 Row pattern recognition: full aggregate support NO
diff --git a/src/backend/executor/nodeLimit.c b/src/backend/executor/nodeLimit.c
index baa669abe8..04ff17a229 100644
--- a/src/backend/executor/nodeLimit.c
+++ b/src/backend/executor/nodeLimit.c
@@ -41,6 +41,7 @@ static TupleTableSlot * /* return: a tuple or NULL */
ExecLimit(PlanState *pstate)
{
LimitState *node = castNode(LimitState, pstate);
+ ExprContext *econtext = node->ps.ps_ExprContext;
ScanDirection direction;
TupleTableSlot *slot;
PlanState *outerPlan;
@@ -102,6 +103,15 @@ ExecLimit(PlanState *pstate)
node->lstate = LIMIT_EMPTY;
return NULL;
}
+/*
+ * Tuple at limit is needed for comparation in subsequent execution
+ * to detect ties.
+ */
+if (node->limitOption == LIMIT_OPTION_WITH_TIES &&
+ node->position - node->offset == node->count - 1)
+{
+ ExecCopySlot(node->last_slot, slot);
+}
node->subSlot = slot;
if (++node->position > node->offset)
break;
@@ -126,12 +136,16 @@ ExecLimit(PlanState *pstate)
{
/*
* Forwards scan, so check for stepping off end of window. If
- * we are at the end of the window, return NULL without
- * advancing the subplan or the position variable; but change
- * the state machine state to record having done so.
+ * we are at the end of the window, the behavior depends whether
+ * ONLY or WITH TIES was specified. In case of ONLY, we return
+ * NULL without advancing the subplan or the position variable;
+ * but change the state machine state to record having done so.
+ * In the WITH TIES mode, we need to advance the subplan until
+ * we find the first row with different ORDER BY pathkeys.
*/
if (!node->noCount &&
- node->position - node->offset >= node->count)
+ node->position - node->offset >= node->count &&
+ node->limitOption == LIMIT_OPTION_COUNT)
{
node->lstate = LIMIT_WINDOWEND;
@@ -144,18 +158,69 @@ ExecLimit(PlanState *pstate)
return NULL;
}
+else if (!node->noCount &&
+ node->position - node->offset >= node->count &&
+ node->limitOption == LIMIT_OPTION_WITH_TIES)
+{
+ /*
+ * Get next tuple from subplan, if any.
+
Re: FETCH FIRST clause WITH TIES option
Hi Alvaro, On Fri, Sep 6, 2019 at 1:52 AM Alvaro Herrera from 2ndQuadrant < alvhe...@alvh.no-ip.org> wrote: > As Tom just said in the thread for PERCENT, the gram.y changes need a > better representation. Also, rename EXACT_NUMBER, per that thread. > > As far as I can tell, this concerns feature F867. I think we should > mark that as supported after this patch -- please edit > src/backend/catalog/sql_features.txt. > ok > > Earlier in the thread, Tomas Vondra said: > > > 3) I'm a bit confused by the initialization added to ExecInitLimit. It > > first gets the tuple descriptor from the limitstate (it should not do > so > > > directly but use ExecGetResultType). But when it creates the extra > slot, > > > it uses ops extracted from the outer plan. That's strange, I guess ... > > > > > > > > And then it extracts the descriptor from the outer plan and uses it > when > > > calling execTuplesMatchPrepare. But AFAIK it's going to be compared to > > > > the last_slot, which is using a descriptor from the limitstate. > > > > > > > > IMHO all of this should use descriptor/ops from the outer plan, no? It > > > > probably does not change anything because limit does not project, but > it > > > seems confusing. > > > > and you replied: > > > agree > > ... yet this doesn't appear to have resulted in any change in the code, > or I just missed it. Are you going to update the patch per that? > > Its already done in v9 of the patch attached by Tomas regards Surafel
Re: FETCH FIRST clause PERCENT option
Hi Tom, On Fri, Sep 6, 2019 at 1:26 AM Tom Lane wrote: > Surafel Temesgen writes: > > [ percent-incremental-v8.patch ] > > I took a quick look through this. > > * Why is this adding new functionality in tuplestore.c? Especially > functionality to get out a different tuple representation than what was > put in? If am not mistaken tuplestore store MinimalTuple(tuple without transaction status information) for minimal memory usage. it change what we put in into MinimalTuple so we need the same format for getting out. The other way I can think of doing it is allocating new MinimalTuple slot every time to get the tuple which have resource like. The operation is very similar to RunFromStore without the ability of reusing once allocated slot. > I can't see a reason why nodeLimit should need that, and for > sure I don't see a reason why it would need it for this feature if it > didn't before. > Only this feature needs tuple storage > * The business with query.limitCount having different data types > depending on LimitOption seems entirely bletcherous and bug-inducing. > (There are live bugs of that sort in what you have now: notably, that > kluge in adjust_limit_rows_costs will crash and burn if float8 is > pass-by-reference. Breaking the Datum abstraction is bad practice.) > I wonder if we could get away with making limitCount be float8 all the > time. This would mean that you'd lose exactness for counts above 2^53 > (or so, depending on whether float8 is IEEE or not), but I doubt that > anybody would ever notice. > > * Names like "PERCENTAGE" seem mighty generic to be exposing as globally > known symbols; also you're disregarding a fairly widespread PG > convention that members of an enum should have names derived from the > enum type's name. So I'd be inclined to make the values of enum > LimitOption be LIMIT_OPTION_COUNT and LIMIT_OPTION_PERCENT. (I don't > especially like EXACT_NUMBER, first because it's going to be quite long > with the prefix, and second because it suggests that maybe there's an > INEXACT_NUMBER alternative.) > > * The "void *limitOption" business in gram.y also seems pretty ugly; > it'd be better for that to be enum LimitOption. I gather you want > the null to indicate "no option given", but likely it'd be better > to invent a LIMIT_OPTION_DEFAULT enum alternative for that purpose. > > i like to fix the above three more because of scaling * An alternative to the three points above is just to separate > Query.limitCount (an int8) from Query.limitPercent (a float8), with the > understanding that at most one of the two can be non-null, and use > similar representations at other steps of the processing chain. Then > you don't need enum limitOption at all. That might not scale especially > nicely to additional variants, but trying to overload limitCount even > further isn't going to be nice either. > > * The proposed change in grouping_planner() seems like a complete hack. > Why would you not change the way that limit_tuples was computed earlier, > instead? > That is, it seems like the part of the planner to teach about > this feature is preprocess_limit (and limit_needed), not someplace else. > It is surely not sane that only this one planner decision would react to > a percentage-based limit. > > * I didn't really study the changes in nodeLimit.c, but doing > "tuplestore_rescan" in ExecReScanLimit is surely just wrong. You > probably want to delete and recreate the tuplestore, instead, since > whatever data you already collected is of no further use. Maybe, in > the case where no rescan of the child node is needed, you could re-use > the data already collected; but that would require a bunch of additional > logic. I'm inclined to think that v1 of the patch shouldn't concern > itself with that sort of optimization. > > ok > * I don't see any delta in ruleutils.c, but surely that needs to know > about this feature so that it can correctly print views using it. > ok regards Surafel
Take skip header out of a loop in COPY FROM
Hello,
Even if skipping header is done only once its checked and skipped in a
loop. If I don’t miss something it can be done out side a loop like
attached patch
regards
Surafel
diff --git a/src/backend/commands/copy.c b/src/backend/commands/copy.c
index 4f04d122c3..4e7709d7bf 100644
--- a/src/backend/commands/copy.c
+++ b/src/backend/commands/copy.c
@@ -3004,6 +3004,13 @@ CopyFrom(CopyState cstate)
errcallback.previous = error_context_stack;
error_context_stack =
+ /* on input just throw the header line away */
+ if ( cstate->header_line)
+ {
+ cstate->cur_lineno++;
+ (void) CopyReadLine(cstate);
+ }
+
for (;;)
{
TupleTableSlot *myslot;
@@ -3642,14 +3649,6 @@ NextCopyFromRawFields(CopyState cstate, char ***fields, int *nfields)
/* only available for text or csv input */
Assert(!cstate->binary);
- /* on input just throw the header line away */
- if (cstate->cur_lineno == 0 && cstate->header_line)
- {
- cstate->cur_lineno++;
- if (CopyReadLine(cstate))
- return false; /* done */
- }
-
cstate->cur_lineno++;
/* Actually read the line into memory here */
Re: FETCH FIRST clause PERCENT option
On Tue, Aug 20, 2019 at 9:10 AM Kyotaro Horiguchi
wrote:
> Hi,
>
> At Wed, 7 Aug 2019 10:20:09 +0300, Surafel Temesgen
> wrote in <
> calay4q98xbvhtz4yj9dccmg2-s1_jurr7fyanfw+bkmr22o...@mail.gmail.com>
> > Hi
> > On Wed, Aug 7, 2019 at 6:11 AM Kyotaro Horiguchi <
> horikyota@gmail.com>
> > wrote:
> >
> > >
> > > I have some comments.
> > >
> > > This patch uses distinct parameters for exact number and
> > > percentage. On the othe hand planner has a notion of
> > > tuple_fraction covering the both. The same handling is also used
> > > for tuple number estimation. Using the same scheme will make
> > > things far simpler. See the comment of grouping_planner().
> > >
> > >
> > Its because of data type difference .In planner the data type is the same
>
> I meant that, with the usage of tuple_fraction, one variable can
> represent both the absolute limit and relative limit. This
> simplifies parse structs.
>
In grouping_planner the patch set tuple bound to -1 in create_ordered_paths
because it iterate until the end in percentage. Did that answer your
question?
>
>
> > > In executor part, addition to LimiteState.position, this patch
> > > uses LimiteState.backwardPosition to count how many tuples we're
> > > back from the end of the current tuplestore. But things will get
> > > simpler by just asking the tuplestore for the number of holding
> > > tuples.
> > >
> > >
> > backwardPosition hold how many tuple returned in backward scan
>
> I meant that we don't need to hold the number in estate.
>
I did it this way because I didn't find an API in tuplestore to do this
>
> > > +slot = node->subSlot;
> > >
> > > Why is this needed? The variable is properly set before use and
> > > the assignment is bogus in the first place.
> > >
> > >
> > its because Tuplestore needs initialized slot.
>
> I meant that the initilized slot is overwritten before first use.
>
> > > The new code block in LIMIT_RESCAN in ExecLimit is useless since
> > > it is exatctly the same with existing code block. Why didn't you
> > > use the existing if block?
> > >
> >
> > But they test different scenario
>
> I meant that the two different scenario can share the code block.
>
Sorry for not clarifying will .One have to be check before offsetting and
the other is after offsetting
>
> > > >if (node->limitOption == PERCENTAGE)
> > > +{
> > > +node->perExactCount = ceil(node->percent *
> > > node->position / 100.0);
> > > +
> > > +
> > >
> > > node->position is the number of tuples returned to upper node so
> > > far (not the number of tuples this node has read from the lower
> > > node so far). I don't understand what the expression means.
> > >
> >
> > node->position hold the number of tuples this node has read from the
> lower
> > node so far. see LIMIT_RESCAN state
>
> Reallly? node->position is incremented when
> tuplestore_gettupleslot_heaptuple() succeeded and reutnrs the
> tuple to the caller immediately...
>
> > > +if (node->perExactCount == node->perExactCount +
> 1)
> > > +node->perExactCount++;
> > >
> > > What? The condition never be true. As the result, the following
> > > if block below won't run.
> > >
> >
> > it became true according to the number of tuple returned in from the
> lower
> > node so far
> > and percentage specification.
>
> Mmm. How do you think X can be equal to (X + 1)?
>
Oops my bad .The attached patch remove the need of doing that
>
> > > >/*
> > > + * Return the tuple up to the number of exact count in
> > > OFFSET
> > > + * clause without percentage value consideration.
> > > + */
> > > +if (node->perExactCount > 0)
> > > +{
> > > +
> > >
> > >
> > >
> > >
> > > +/*
> > > + * We may needed this tuple in backward scan so put it
> into
> > > + * tuplestore.
> > > + */
> > > +if (node->limitOption == PERCENTAGE)
> > > +{
> > > +tuplestore_puttupleslot(node->tupleStore, slot);
> > > +
Re: FETCH FIRST clause PERCENT option
On Mon, Aug 19, 2019 at 1:55 PM Erik Rijkers wrote: > Another little thing, not sure it's a bug: > > limit interprets its argument by rounding up or down as one would > expect: > > table onek limit 10.4; --> gives 10 rows > table onek limit 10.6; --> gives 11 rows > > but FETCH count PERCENT does not do that; it rounds always up. > > select * from (table onek limit 100) f fetch first 10.4 percent rows > only; --> gives 11 rows > select * from (table onek limit 100) f fetch first 10.6 percent rows > only; --> gives 11 rows > > I see that it's documented in the .sgml to behave as it does, but it > seems wrong to me; > shouldn't that 10.4-percent-query yield 10 rows instead of 11? > > According to sql standard we have to use the result ceiling value regards Surafel
Re: FETCH FIRST clause PERCENT option
Hi Erik
On Mon, Aug 19, 2019 at 10:47 AM Erik Rijkers wrote:
> Hi,
>
> (running with those two patches applied)
>
>select * from onek where thousand < 5 order by thousand fetch first -1
> percent rows only
> is correctly caught (with "ERROR: PERCENT must not be negative") but:
>
>select * from onek where thousand < 5 order by thousand fetch first
> 101 percent rows only
> is not. It doesn't return, and cannot be Ctrl-C'ed out of, which I guess
> is another bug?
>
Fixed
regards
Surafel
diff --git a/contrib/postgres_fdw/postgres_fdw.c b/contrib/postgres_fdw/postgres_fdw.c
index 1759b9e1b6..fb9c2f5319 100644
--- a/contrib/postgres_fdw/postgres_fdw.c
+++ b/contrib/postgres_fdw/postgres_fdw.c
@@ -3035,7 +3035,8 @@ estimate_path_cost_size(PlannerInfo *root,
if (fpextra && fpextra->has_limit)
{
adjust_limit_rows_costs(, _cost, _cost,
- fpextra->offset_est, fpextra->count_est);
+ fpextra->offset_est, fpextra->count_est,
+ EXACT_NUMBER);
retrieved_rows = rows;
}
}
diff --git a/doc/src/sgml/ref/select.sgml b/doc/src/sgml/ref/select.sgml
index 06d611b64c..4ef42df51d 100644
--- a/doc/src/sgml/ref/select.sgml
+++ b/doc/src/sgml/ref/select.sgml
@@ -44,7 +44,7 @@ SELECT [ ALL | DISTINCT [ ON ( expressionexpression [ ASC | DESC | USING operator ] [ NULLS { FIRST | LAST } ] [, ...] ]
[ LIMIT { count | ALL } ]
[ OFFSET start [ ROW | ROWS ] ]
-[ FETCH { FIRST | NEXT } [ count ] { ROW | ROWS } ONLY ]
+[ FETCH { FIRST | NEXT } [ count ] [ PERCENT ] { ROW | ROWS } ONLY ]
[ FOR { UPDATE | NO KEY UPDATE | SHARE | KEY SHARE } [ OF table_name [, ...] ] [ NOWAIT | SKIP LOCKED ] [...] ]
where from_item can be one of:
@@ -1430,7 +1430,7 @@ OFFSET start
which PostgreSQL also supports. It is:
OFFSET start { ROW | ROWS }
-FETCH { FIRST | NEXT } [ count ] { ROW | ROWS } ONLY
+FETCH { FIRST | NEXT } [ count ] [ PERCENT ] { ROW | ROWS } ONLY
In this syntax, the start
or count value is required by
@@ -1440,8 +1440,10 @@ FETCH { FIRST | NEXT } [ count ] {
ambiguity.
If count is
omitted in a FETCH clause, it defaults to 1.
-ROW
-and ROWS as well as FIRST
+With PERCENT count specifies the maximum number of rows to return
+in percentage round up to the nearest integer. Using PERCENT
+is best suited to returning single-digit percentages of the query's total row count.
+ROW and ROWS as well as FIRST
and NEXT are noise words that don't influence
the effects of these clauses.
According to the standard, the OFFSET clause must come
diff --git a/src/backend/executor/nodeLimit.c b/src/backend/executor/nodeLimit.c
index baa669abe8..7a9626d7f9 100644
--- a/src/backend/executor/nodeLimit.c
+++ b/src/backend/executor/nodeLimit.c
@@ -21,6 +21,8 @@
#include "postgres.h"
+#include
+
#include "executor/executor.h"
#include "executor/nodeLimit.h"
#include "miscadmin.h"
@@ -52,6 +54,7 @@ ExecLimit(PlanState *pstate)
*/
direction = node->ps.state->es_direction;
outerPlan = outerPlanState(node);
+ slot = node->subSlot;
/*
* The main logic is a simple state machine.
@@ -81,7 +84,15 @@ ExecLimit(PlanState *pstate)
/*
* Check for empty window; if so, treat like empty subplan.
*/
- if (node->count <= 0 && !node->noCount)
+ if (node->limitOption == PERCENTAGE)
+ {
+if (node->percent == 0.0)
+{
+ node->lstate = LIMIT_EMPTY;
+ return NULL;
+}
+ }
+ else if (node->count <= 0 && !node->noCount)
{
node->lstate = LIMIT_EMPTY;
return NULL;
@@ -102,11 +113,32 @@ ExecLimit(PlanState *pstate)
node->lstate = LIMIT_EMPTY;
return NULL;
}
+
+/*
+ * Count the number of rows to return in exact number so far
+ */
+if (node->limitOption == PERCENTAGE)
+{
+ node->perExactCount = ceil(node->percent * node->position / 100.0);
+
+ if (node->perExactCount == node->perExactCount + 1)
+ node->perExactCount++;
+}
node->subSlot = slot;
if (++node->position > node->offset)
break;
}
+ /*
+ * We may needed this tuple in subsequent scan so put it into
+ * tuplestore.
+ */
+ if (node->limitOption == PERCENTAGE)
+ {
+tuplestore_puttupleslot(node->tupleStore, slot);
+tuplestore_advance(node->tupleStore, true);
+ }
+
/*
* Okay, we have the first tuple of the window.
*/
@@ -124,6 +156,106 @@ ExecLimit(PlanState *pstate)
case LIMIT_INWINDOW:
if (ScanDirectionIsForward(direction))
{
+/*
+ * In case of coming back from backward scan the tuple is
+ * already in tuple store.
+ */
+if (node->limitOption == PERCENTAGE && node->backwardPosition > 0)
+{
+ if (tuplestore_gettupleslot_heaptuple(node->tupleStore, true, true, slot))
+ {
+ node->subSlot = slot;
+ node->position++;
+ node->backwardPosition--;
+ return slot;
+ }
+ else
+ {
+ node->lstate = LIMIT_SUBPLANEOF;
+ return NULL;
Re: FETCH FIRST clause PERCENT option
Hi
On Wed, Aug 7, 2019 at 6:11 AM Kyotaro Horiguchi
wrote:
>
> I have some comments.
>
> This patch uses distinct parameters for exact number and
> percentage. On the othe hand planner has a notion of
> tuple_fraction covering the both. The same handling is also used
> for tuple number estimation. Using the same scheme will make
> things far simpler. See the comment of grouping_planner().
>
>
Its because of data type difference .In planner the data type is the same
In executor part, addition to LimiteState.position, this patch
> uses LimiteState.backwardPosition to count how many tuples we're
> back from the end of the current tuplestore. But things will get
> simpler by just asking the tuplestore for the number of holding
> tuples.
>
>
backwardPosition hold how many tuple returned in backward scan
>
> +slot = node->subSlot;
>
> Why is this needed? The variable is properly set before use and
> the assignment is bogus in the first place.
>
>
its because Tuplestore needs initialized slot.
> The new code block in LIMIT_RESCAN in ExecLimit is useless since
> it is exatctly the same with existing code block. Why didn't you
> use the existing if block?
>
But they test different scenario
>
> >if (node->limitOption == PERCENTAGE)
> +{
> +node->perExactCount = ceil(node->percent *
> node->position / 100.0);
> +
> +
>
> node->position is the number of tuples returned to upper node so
> far (not the number of tuples this node has read from the lower
> node so far). I don't understand what the expression means.
>
node->position hold the number of tuples this node has read from the lower
node so far. see LIMIT_RESCAN state
>
>
> +if (node->perExactCount == node->perExactCount + 1)
> +node->perExactCount++;
>
> What? The condition never be true. As the result, the following
> if block below won't run.
>
it became true according to the number of tuple returned in from the lower
node so far
and percentage specification.
>
> >/*
> + * Return the tuple up to the number of exact count in
> OFFSET
> + * clause without percentage value consideration.
> + */
> +if (node->perExactCount > 0)
> +{
> +
>
>
>
>
> +/*
> + * We may needed this tuple in backward scan so put it into
> + * tuplestore.
> + */
> +if (node->limitOption == PERCENTAGE)
> +{
> +tuplestore_puttupleslot(node->tupleStore, slot);
> +tuplestore_advance(node->tupleStore, true);
> +}
>
> "needed"->"need" ? The comment says that this is needed for
> backward scan, but it is actually required even in forward
> scan. More to the point, tuplestore_advance lacks comment.
ok
>
> Anyway, the code in LIMIT_RESCAN is broken in some ways. For
> example, if it is used as the inner scan of a NestLoop, the
> tuplestore accumulates tuples by every scan. You will see that
> the tuplestore stores up to 1000 tuples (10 times of the inner)
> by the following query.
>
It this because in percentage we scan the whole table
regards
Surafel
Re: FETCH FIRST clause PERCENT option
Hi Ryan,
sorry for not be fast to replay
> I was suggesting a warning in the documentation so users aren't caught
> unaware about the performance characteristics. My first version was very
> rough, how about adding this in doc/src/sgml/ref/select.sgml?
>
> "Using PERCENT is best suited to returning single-digit
> percentages of the query's total row count."
>
>
> The following paragraphs in that same section give suggestions and
> warnings regarding LIMIT and OFFSET usage, I think this is more in line
> with the wording of those existing warnings.
>
>
Agreed and done
> Other than that, we can rip the clause if it is 100%
>
>
> You mean if PERCENT=100 it should short circuit and run the query
> normally? I like that.
>
The patch did not did it automatically. Its query writer obligation to do
that currently
regards
Surafel
diff --git a/contrib/postgres_fdw/postgres_fdw.c b/contrib/postgres_fdw/postgres_fdw.c
index 1759b9e1b6..fb9c2f5319 100644
--- a/contrib/postgres_fdw/postgres_fdw.c
+++ b/contrib/postgres_fdw/postgres_fdw.c
@@ -3035,7 +3035,8 @@ estimate_path_cost_size(PlannerInfo *root,
if (fpextra && fpextra->has_limit)
{
adjust_limit_rows_costs(, _cost, _cost,
- fpextra->offset_est, fpextra->count_est);
+ fpextra->offset_est, fpextra->count_est,
+ EXACT_NUMBER);
retrieved_rows = rows;
}
}
diff --git a/doc/src/sgml/ref/select.sgml b/doc/src/sgml/ref/select.sgml
index 06d611b64c..4ef42df51d 100644
--- a/doc/src/sgml/ref/select.sgml
+++ b/doc/src/sgml/ref/select.sgml
@@ -44,7 +44,7 @@ SELECT [ ALL | DISTINCT [ ON ( expressionexpression [ ASC | DESC | USING operator ] [ NULLS { FIRST | LAST } ] [, ...] ]
[ LIMIT { count | ALL } ]
[ OFFSET start [ ROW | ROWS ] ]
-[ FETCH { FIRST | NEXT } [ count ] { ROW | ROWS } ONLY ]
+[ FETCH { FIRST | NEXT } [ count ] [ PERCENT ] { ROW | ROWS } ONLY ]
[ FOR { UPDATE | NO KEY UPDATE | SHARE | KEY SHARE } [ OF table_name [, ...] ] [ NOWAIT | SKIP LOCKED ] [...] ]
where from_item can be one of:
@@ -1430,7 +1430,7 @@ OFFSET start
which PostgreSQL also supports. It is:
OFFSET start { ROW | ROWS }
-FETCH { FIRST | NEXT } [ count ] { ROW | ROWS } ONLY
+FETCH { FIRST | NEXT } [ count ] [ PERCENT ] { ROW | ROWS } ONLY
In this syntax, the start
or count value is required by
@@ -1440,8 +1440,10 @@ FETCH { FIRST | NEXT } [ count ] {
ambiguity.
If count is
omitted in a FETCH clause, it defaults to 1.
-ROW
-and ROWS as well as FIRST
+With PERCENT count specifies the maximum number of rows to return
+in percentage round up to the nearest integer. Using PERCENT
+is best suited to returning single-digit percentages of the query's total row count.
+ROW and ROWS as well as FIRST
and NEXT are noise words that don't influence
the effects of these clauses.
According to the standard, the OFFSET clause must come
diff --git a/src/backend/executor/nodeLimit.c b/src/backend/executor/nodeLimit.c
index baa669abe8..c503512588 100644
--- a/src/backend/executor/nodeLimit.c
+++ b/src/backend/executor/nodeLimit.c
@@ -21,6 +21,8 @@
#include "postgres.h"
+#include
+
#include "executor/executor.h"
#include "executor/nodeLimit.h"
#include "miscadmin.h"
@@ -52,6 +54,7 @@ ExecLimit(PlanState *pstate)
*/
direction = node->ps.state->es_direction;
outerPlan = outerPlanState(node);
+ slot = node->subSlot;
/*
* The main logic is a simple state machine.
@@ -81,7 +84,15 @@ ExecLimit(PlanState *pstate)
/*
* Check for empty window; if so, treat like empty subplan.
*/
- if (node->count <= 0 && !node->noCount)
+ if (node->limitOption == PERCENTAGE)
+ {
+if (node->percent == 0.0)
+{
+ node->lstate = LIMIT_EMPTY;
+ return NULL;
+}
+ }
+ else if (node->count <= 0 && !node->noCount)
{
node->lstate = LIMIT_EMPTY;
return NULL;
@@ -102,11 +113,32 @@ ExecLimit(PlanState *pstate)
node->lstate = LIMIT_EMPTY;
return NULL;
}
+
+/*
+ * Count the number of rows to return in exact number so far
+ */
+if (node->limitOption == PERCENTAGE)
+{
+ node->perExactCount = ceil(node->percent * node->position / 100.0);
+
+ if (node->perExactCount == node->perExactCount + 1)
+ node->perExactCount++;
+}
node->subSlot = slot;
if (++node->position > node->offset)
break;
}
+ /*
+ * We may needed this tuple in backward scan so put it into
+ * tuplestore.
+ */
+ if (node->limitOption == PERCENTAGE)
+ {
+tuplestore_puttupleslot(node->tupleStore, slot);
+tuplestore_advance(node->tupleStore, true);
+ }
+
/*
* Okay, we have the first tuple of the window.
*/
@@ -124,6 +156,106 @@ ExecLimit(PlanState *pstate)
case LIMIT_INWINDOW:
if (ScanDirectionIsForward(direction))
{
+/*
+ * In case of coming back from backward scan the tuple is
+ * already in tuple store.
+ */
+if (node->limitOption
Re: FETCH FIRST clause PERCENT option
Hi Ryan, On Tue, Jul 9, 2019 at 4:13 PM Ryan Lambert wrote: > > "It is possible for FETCH FIRST N PERCENT to create poorly performing > query plans when the N supplied exceeds 50 percent. In these cases query > execution can take an order of magnitude longer to execute than simply > returning the full table. If performance is critical using an explicit row > count for limiting is recommended." > I don’t understand how fetch first n percent functionality can be replaced with explicit row count limiting. There may be a way to do it in a client side but we can not be sure of its performance advantage regards Surafel
Re: Conflict handling for COPY FROM
On Sun, Jul 14, 2019 at 7:40 PM Alvaro Herrera
wrote:
>
> error_limit being an integer, please don't use it as a boolean:
>
> if (cstate->error_limit)
>
...
>
> Add an explicit comparison to zero instead, for code readability.
> Also, since each error decrements the same variable, it becomes hard to
> reason about the state: at the end, are we ending with the exact number
> of errors, or did we start with the feature disabled? I suggest that
> it'd make sense to have a boolean indicating whether this feature has
> been requested, and the integer is just the remaining allowed problems.
>
>
done
Line 3255 or thereabouts contains an excess " char
>
>
fixed
> The "warn about it" comment is obsolete, isn't it? There's no warning
> there.
>
>
fixed
i also add an option to ignore all errors in ERROR set to -1
diff --git a/doc/src/sgml/ref/copy.sgml b/doc/src/sgml/ref/copy.sgml
index 5e2992ddac..7aaebf56d8 100644
--- a/doc/src/sgml/ref/copy.sgml
+++ b/doc/src/sgml/ref/copy.sgml
@@ -44,6 +44,7 @@ COPY { table_name [ ( column_name [, ...] )
FORCE_NULL ( column_name [, ...] )
ENCODING 'encoding_name'
+ERROR 'limit_number'
@@ -355,6 +356,23 @@ COPY { table_name [ (
+
+ERROR
+
+
+ Specifies to return error record up to limit_number number.
+ specifying it to -1 returns all error record.
+
+
+
+ Currently, only unique or exclusion constraint violation
+ and same record formatting error is ignored.
+
+
+
+
+
WHERE
diff --git a/src/backend/commands/copy.c b/src/backend/commands/copy.c
index 4f04d122c3..5884493307 100644
--- a/src/backend/commands/copy.c
+++ b/src/backend/commands/copy.c
@@ -48,6 +48,7 @@
#include "port/pg_bswap.h"
#include "rewrite/rewriteHandler.h"
#include "storage/fd.h"
+#include "storage/lmgr.h"
#include "tcop/tcopprot.h"
#include "utils/builtins.h"
#include "utils/lsyscache.h"
@@ -154,6 +155,7 @@ typedef struct CopyStateData
List *convert_select; /* list of column names (can be NIL) */
bool *convert_select_flags; /* per-column CSV/TEXT CS flags */
Node *whereClause; /* WHERE condition (or NULL) */
+ int error_limit; /* total number of error to ignore */
/* these are just for error messages, see CopyFromErrorCallback */
const char *cur_relname; /* table name for error messages */
@@ -184,6 +186,9 @@ typedef struct CopyStateData
bool volatile_defexprs; /* is any of defexprs volatile? */
List *range_table;
ExprState *qualexpr;
+ bool ignore_error; /* is ignore error specified? */
+ bool ignore_all_error; /* is error_limit -1 (ignore all error)
+ * specified? */
TransitionCaptureState *transition_capture;
@@ -1291,6 +1296,18 @@ ProcessCopyOptions(ParseState *pstate,
defel->defname),
parser_errposition(pstate, defel->location)));
}
+ 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;
+ }
else
ereport(ERROR,
(errcode(ERRCODE_SYNTAX_ERROR),
@@ -1441,6 +1458,10 @@ ProcessCopyOptions(ParseState *pstate,
ereport(ERROR,
(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
errmsg("CSV quote character must not appear in the NULL specification")));
+ if (cstate->ignore_error && !cstate->is_copy_from)
+ ereport(ERROR,
+(errcode(ERRCODE_SYNTAX_ERROR),
+ errmsg("ERROR LIMIT only available using COPY FROM")));
}
/*
@@ -2678,6 +2699,7 @@ CopyFrom(CopyState cstate)
bool has_before_insert_row_trig;
bool has_instead_insert_row_trig;
bool leafpart_use_multi_insert = false;
+ DestReceiver *dest = NULL;
Assert(cstate->rel);
@@ -2841,7 +2863,17 @@ CopyFrom(CopyState cstate)
/* Verify the named relation is a valid target for INSERT */
CheckValidResultRel(resultRelInfo, CMD_INSERT);
- ExecOpenIndices(resultRelInfo, false);
+ if (cstate->ignore_error)
+ {
+ TupleDesc tupDesc;
+
+ tupDesc = RelationGetDescr(cstate->rel);
+ ExecOpenIndices(resultRelInfo, true);
+ dest = CreateDestReceiver(DestRemoteSimple);
+ dest->rStartup(dest, (int) CMD_SELECT, tupDesc);
+ }
+ else
+ ExecOpenIndices(resultRelInfo, false);
estate->es_result_relations = resultRelInfo;
estate->es_num_result_relations = 1;
@@ -2946,6 +2978,13 @@ CopyFrom(CopyState cstate)
*/
insertMethod = CIM_SINGLE;
}
+ else if (cstate->ignore_error)
+ {
+ /*
+ * Can't support speculative insertion in multi-inserts.
+ */
+ insertMethod = CIM_SINGLE;
+ }
else
{
/*
@@ -3289,6 +3328,63 @@ CopyFrom(CopyState cstate)
*/
myslot->tts_tableOid = RelationGetRelid(resultRelInfo->ri_RelationDesc);
}
+ else if ((cstate->error_limit > 0 ||
Re: Conflict handling for COPY FROM
Hi
>
> 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.
>
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
regards
Surafel
diff --git a/doc/src/sgml/ref/copy.sgml b/doc/src/sgml/ref/copy.sgml
index 5e2992ddac..f16108b23b 100644
--- a/doc/src/sgml/ref/copy.sgml
+++ b/doc/src/sgml/ref/copy.sgml
@@ -44,6 +44,7 @@ COPY { table_name [ ( column_name [, ...] )
FORCE_NULL ( column_name [, ...] )
ENCODING 'encoding_name'
+ERROR 'limit_number'
@@ -355,6 +356,22 @@ COPY { table_name [ (
+
+ERROR
+
+
+ Specifies to ignore error record up to limit_number number.
+
+
+
+ Currently, only unique or exclusion constraint violation
+ and same record formatting error is ignored.
+
+
+
+
+
WHERE
diff --git a/src/backend/commands/copy.c b/src/backend/commands/copy.c
index 4f04d122c3..2a6bc48f78 100644
--- a/src/backend/commands/copy.c
+++ b/src/backend/commands/copy.c
@@ -48,6 +48,7 @@
#include "port/pg_bswap.h"
#include "rewrite/rewriteHandler.h"
#include "storage/fd.h"
+#include "storage/lmgr.h"
#include "tcop/tcopprot.h"
#include "utils/builtins.h"
#include "utils/lsyscache.h"
@@ -154,6 +155,7 @@ typedef struct CopyStateData
List *convert_select; /* list of column names (can be NIL) */
bool *convert_select_flags; /* per-column CSV/TEXT CS flags */
Node *whereClause; /* WHERE condition (or NULL) */
+ int error_limit; /* total number of error to ignore */
/* these are just for error messages, see CopyFromErrorCallback */
const char *cur_relname; /* table name for error messages */
@@ -1291,6 +1293,21 @@ ProcessCopyOptions(ParseState *pstate,
defel->defname),
parser_errposition(pstate, defel->location)));
}
+ else if (strcmp(defel->defname, "error_limit") == 0)
+ {
+ if (cstate->error_limit > 0)
+ereport(ERROR,
+ (errcode(ERRCODE_SYNTAX_ERROR),
+ errmsg("conflicting or redundant options"),
+ parser_errposition(pstate, defel->location)));
+ cstate->error_limit = defGetInt64(defel);
+ if (cstate->error_limit <= 0)
+ereport(ERROR,
+ (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+ errmsg("argument to option \"%s\" must be positive integer or -1",
+defel->defname),
+ parser_errposition(pstate, defel->location)));
+ }
else
ereport(ERROR,
(errcode(ERRCODE_SYNTAX_ERROR),
@@ -1441,6 +1458,10 @@ ProcessCopyOptions(ParseState *pstate,
ereport(ERROR,
(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
errmsg("CSV quote character must not appear in the NULL specification")));
+ if (cstate->error_limit && !cstate->is_copy_from)
+ ereport(ERROR,
+(errcode(ERRCODE_SYNTAX_ERROR),
+ errmsg("ERROR LIMIT only available using COPY FROM")));
}
/*
@@ -2678,6 +2699,7 @@ CopyFrom(CopyState cstate)
bool has_before_insert_row_trig;
bool has_instead_insert_row_trig;
bool leafpart_use_multi_insert = false;
+ DestReceiver *dest;
Assert(cstate->rel);
@@ -2841,7 +2863,17 @@ CopyFrom(CopyState cstate)
/* Verify the named relation is a valid target for INSERT */
CheckValidResultRel(resultRelInfo, CMD_INSERT);
- ExecOpenIndices(resultRelInfo, false);
+ if (cstate->error_limit)
+ {
+ TupleDesc tupDesc;
+
+ tupDesc = RelationGetDescr(cstate->rel);
+ ExecOpenIndices(resultRelInfo, true);
+ dest = CreateDestReceiver(DestRemoteSimple);
+ dest->rStartup(dest, (int) CMD_SELECT, tupDesc);
+ }
+ else
+ ExecOpenIndices(resultRelInfo, false);
estate->es_result_relations = resultRelInfo;
estate->es_num_result_relations = 1;
@@ -2946,6 +2978,13 @@ CopyFrom(CopyState cstate)
*/
insertMethod = CIM_SINGLE;
}
+ else if (cstate->error_limit)
+ {
+ /*
+ * Can't support speculative insertion in multi-inserts.
+ */
+ insertMethod = CIM_SINGLE;
+ }
else
{
/*
@@ -3289,6 +3328,63 @@ CopyFrom(CopyState cstate)
*/
myslot->tts_tableOid = RelationGetRelid(resultRelInfo->ri_RelationDesc);
}
+ else if (cstate->error_limit && resultRelInfo->ri_NumIndices > 0)
+ {
+ /* Perform a speculative insertion. */
+ uint32 specToken;
+ ItemPointerData conflictTid;
+ bool specConflict;
+
+ /*
+ * Do a non-conclusive check for conflicts first.
+ */
+ specConflict = false;
+
+ if (!ExecCheckIndexConstraints(myslot, estate, ,
+ NIL))
+ {
+ (void) dest->receiveSlot(myslot, dest);
+ cstate->error_limit--;
+ continue;
+ }
+
+ /*
+ * Acquire our speculative insertion lock".
+ */
+ specToken =
Re: FETCH FIRST clause PERCENT option
Hi Alvaro, On Wed, Jul 10, 2019 at 6:44 PM Alvaro Herrera wrote: > What's with the new tuplestore function for getting heap tuples? That > looks really odd. > Previously I create new TTSOpsMinimalTuple type slots for every tuple returned in order to fetch it from tuplestore because tuplestore store tuple in MinimalTuple format and created slot format changes in subsequent operation. This case resource leak as Andres mention and I create this function to remove the need for creating new slot for every returned tuple because of form difference regards Surafel
Re: extension patch of CREATE OR REPLACE TRIGGER
Hi Takamichi Osumi,
On Tue, Jul 9, 2019
> I've rebased the previous patch to be applied
>
I don't test your patch fully yet but here are same comment.
There are same white space issue like here
- bool is_internal)
+ bool is_internal,
+ Oid existing_constraint_oid)
in a few place
+ // trigoid = HeapTupleGetOid(tuple); // raw code
please remove this line if you don't use it.
+ if(!existing_constraint_oid){
+ conOid = GetNewOidWithIndex(conDesc, ConstraintOidIndexId,
+ Anum_pg_constraint_oid);
+ values[Anum_pg_constraint_oid - 1] = ObjectIdGetDatum(conOid);
+ }
incorrect bracing style here and its appear in a few other places too
and it seems to me that the change in regression test is
huge can you reduce it?
regards
Surafel
Re: FETCH FIRST clause PERCENT option
Hi Ryan, On Tue, Jul 9, 2019 at 1:27 AM Ryan Lambert wrote: > Hi Surafel, > > The v5 patch [1] applies cleanly and passes make installcheck-world. > > My concern with this is its performance. As a user I would expect using > this feature to enable queries to run faster than they would simply pulling > the full table. I tested on tables ranging from 10k rows up to 10 million > with the same basic result that using FETCH FIRST N PERCENT is slower than > using the full table. At best it ran slightly slower than querying the > full table, at worst it increased execution times by 1400% when using a > large high percentage (95%). > > The cost of FITCH FIRST N PERCENT execution in current implementation is the cost of pulling the full table plus the cost of storing and fetching the tuple from tuplestore so it can not perform better than pulling the full table in any case . This is because we can't determined the number of rows to return without executing the plan until the end. We can find the estimation of rows that will be return in planner estimation but that is not exact. regards Surafel
Re: FETCH FIRST clause PERCENT option
Hi Thomas,
Thank you for informing me
Hi Surafel,
>
> There's a call to adjust_limit_rows_costs() hiding under
> contrib/postgres_fdw, so this fails check-world.
>
Fixed . I also include the review given by Ryan in attached patch
regards
Surafel
diff --git a/contrib/postgres_fdw/postgres_fdw.c b/contrib/postgres_fdw/postgres_fdw.c
index 1759b9e1b6..fb9c2f5319 100644
--- a/contrib/postgres_fdw/postgres_fdw.c
+++ b/contrib/postgres_fdw/postgres_fdw.c
@@ -3035,7 +3035,8 @@ estimate_path_cost_size(PlannerInfo *root,
if (fpextra && fpextra->has_limit)
{
adjust_limit_rows_costs(, _cost, _cost,
- fpextra->offset_est, fpextra->count_est);
+ fpextra->offset_est, fpextra->count_est,
+ EXACT_NUMBER);
retrieved_rows = rows;
}
}
diff --git a/doc/src/sgml/ref/select.sgml b/doc/src/sgml/ref/select.sgml
index 06d611b64c..1c240a3dab 100644
--- a/doc/src/sgml/ref/select.sgml
+++ b/doc/src/sgml/ref/select.sgml
@@ -44,7 +44,7 @@ SELECT [ ALL | DISTINCT [ ON ( expressionexpression [ ASC | DESC | USING operator ] [ NULLS { FIRST | LAST } ] [, ...] ]
[ LIMIT { count | ALL } ]
[ OFFSET start [ ROW | ROWS ] ]
-[ FETCH { FIRST | NEXT } [ count ] { ROW | ROWS } ONLY ]
+[ FETCH { FIRST | NEXT } [ count ] [ PERCENT ] { ROW | ROWS } ONLY ]
[ FOR { UPDATE | NO KEY UPDATE | SHARE | KEY SHARE } [ OF table_name [, ...] ] [ NOWAIT | SKIP LOCKED ] [...] ]
where from_item can be one of:
@@ -1430,7 +1430,7 @@ OFFSET start
which PostgreSQL also supports. It is:
OFFSET start { ROW | ROWS }
-FETCH { FIRST | NEXT } [ count ] { ROW | ROWS } ONLY
+FETCH { FIRST | NEXT } [ count ] [ PERCENT ] { ROW | ROWS } ONLY
In this syntax, the start
or count value is required by
@@ -1440,7 +1440,8 @@ FETCH { FIRST | NEXT } [ count ] {
ambiguity.
If count is
omitted in a FETCH clause, it defaults to 1.
-ROW
+With PERCENT count specifies the maximum number of rows to return
+in percentage round up to the nearest integer.ROW
and ROWS as well as FIRST
and NEXT are noise words that don't influence
the effects of these clauses.
diff --git a/src/backend/executor/nodeLimit.c b/src/backend/executor/nodeLimit.c
index baa669abe8..c503512588 100644
--- a/src/backend/executor/nodeLimit.c
+++ b/src/backend/executor/nodeLimit.c
@@ -21,6 +21,8 @@
#include "postgres.h"
+#include
+
#include "executor/executor.h"
#include "executor/nodeLimit.h"
#include "miscadmin.h"
@@ -52,6 +54,7 @@ ExecLimit(PlanState *pstate)
*/
direction = node->ps.state->es_direction;
outerPlan = outerPlanState(node);
+ slot = node->subSlot;
/*
* The main logic is a simple state machine.
@@ -81,7 +84,15 @@ ExecLimit(PlanState *pstate)
/*
* Check for empty window; if so, treat like empty subplan.
*/
- if (node->count <= 0 && !node->noCount)
+ if (node->limitOption == PERCENTAGE)
+ {
+if (node->percent == 0.0)
+{
+ node->lstate = LIMIT_EMPTY;
+ return NULL;
+}
+ }
+ else if (node->count <= 0 && !node->noCount)
{
node->lstate = LIMIT_EMPTY;
return NULL;
@@ -102,11 +113,32 @@ ExecLimit(PlanState *pstate)
node->lstate = LIMIT_EMPTY;
return NULL;
}
+
+/*
+ * Count the number of rows to return in exact number so far
+ */
+if (node->limitOption == PERCENTAGE)
+{
+ node->perExactCount = ceil(node->percent * node->position / 100.0);
+
+ if (node->perExactCount == node->perExactCount + 1)
+ node->perExactCount++;
+}
node->subSlot = slot;
if (++node->position > node->offset)
break;
}
+ /*
+ * We may needed this tuple in backward scan so put it into
+ * tuplestore.
+ */
+ if (node->limitOption == PERCENTAGE)
+ {
+tuplestore_puttupleslot(node->tupleStore, slot);
+tuplestore_advance(node->tupleStore, true);
+ }
+
/*
* Okay, we have the first tuple of the window.
*/
@@ -124,6 +156,106 @@ ExecLimit(PlanState *pstate)
case LIMIT_INWINDOW:
if (ScanDirectionIsForward(direction))
{
+/*
+ * In case of coming back from backward scan the tuple is
+ * already in tuple store.
+ */
+if (node->limitOption == PERCENTAGE && node->backwardPosition > 0)
+{
+ if (tuplestore_gettupleslot_heaptuple(node->tupleStore, true, true, slot))
+ {
+ node->subSlot = slot;
+ node->position++;
+ node->backwardPosition--;
+ return slot;
+ }
+ else
+ {
+ node->lstate = LIMIT_SUBPLANEOF;
+ return NULL;
+ }
+}
+
+/*
+ * In PERCENTAGE case no need of executing outerPlan multiple
+ * times.
+ */
+if (node->limitOption == PERCENTAGE && node->reachEnd)
+{
+ node->lstate = LIMIT_WINDOWEND;
+
+ /*
+ * If we know we won't need to back up, we can release
+ * resources at this point.
+ */
+ if (!(node->ps.state->es_top_eflags & EXEC_FLAG_BACKWARD))
+ (void) ExecShutdownNode(outerPlan);
+
+
Re: Conflict handling for COPY FROM
Hi Alexey,
Thank you for looking at it
On Tue, Jul 2, 2019 at 7:57 PM Alexey Kondratov
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
> 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
Re: FETCH FIRST clause WITH TIES option
Hi Erik,
Thank you for looking at it.
Can you have a look?
>
>
It is because of the patch didn't handle that edge case.
attache patch contain a fix for it
regards
Surafel
diff --git a/doc/src/sgml/ref/select.sgml b/doc/src/sgml/ref/select.sgml
index 06d611b64c..e83d309c5b 100644
--- a/doc/src/sgml/ref/select.sgml
+++ b/doc/src/sgml/ref/select.sgml
@@ -44,7 +44,7 @@ SELECT [ ALL | DISTINCT [ ON ( expressionexpression [ ASC | DESC | USING operator ] [ NULLS { FIRST | LAST } ] [, ...] ]
[ LIMIT { count | ALL } ]
[ OFFSET start [ ROW | ROWS ] ]
-[ FETCH { FIRST | NEXT } [ count ] { ROW | ROWS } ONLY ]
+[ FETCH { FIRST | NEXT } [ count ] { ROW | ROWS } { ONLY | WITH TIES } ]
[ FOR { UPDATE | NO KEY UPDATE | SHARE | KEY SHARE } [ OF table_name [, ...] ] [ NOWAIT | SKIP LOCKED ] [...] ]
where from_item can be one of:
@@ -1430,7 +1430,7 @@ OFFSET start
which PostgreSQL also supports. It is:
OFFSET start { ROW | ROWS }
-FETCH { FIRST | NEXT } [ count ] { ROW | ROWS } ONLY
+FETCH { FIRST | NEXT } [ count ] { ROW | ROWS } { ONLY | WITH TIES }
In this syntax, the start
or count value is required by
@@ -1440,7 +1440,10 @@ FETCH { FIRST | NEXT } [ count ] {
ambiguity.
If count is
omitted in a FETCH clause, it defaults to 1.
-ROW
+ROW .
+WITH TIES option is used to return two or more rows
+that tie for the last place in the result set according to ORDER BY
+clause (ORDER BY clause must be specified in this case).
and ROWS as well as FIRST
and NEXT are noise words that don't influence
the effects of these clauses.
diff --git a/src/backend/executor/nodeLimit.c b/src/backend/executor/nodeLimit.c
index baa669abe8..cf557ecb1c 100644
--- a/src/backend/executor/nodeLimit.c
+++ b/src/backend/executor/nodeLimit.c
@@ -41,6 +41,7 @@ static TupleTableSlot * /* return: a tuple or NULL */
ExecLimit(PlanState *pstate)
{
LimitState *node = castNode(LimitState, pstate);
+ ExprContext *econtext = node->ps.ps_ExprContext;
ScanDirection direction;
TupleTableSlot *slot;
PlanState *outerPlan;
@@ -102,6 +103,16 @@ ExecLimit(PlanState *pstate)
node->lstate = LIMIT_EMPTY;
return NULL;
}
+
+/*
+ * Tuple at limit is needed for comparation in subsequent execution
+ * to detect ties.
+ */
+if (node->limitOption == WITH_TIES &&
+ node->position - node->offset == node->count - 1)
+{
+ ExecCopySlot(node->last_slot, slot);
+}
node->subSlot = slot;
if (++node->position > node->offset)
break;
@@ -126,12 +137,16 @@ ExecLimit(PlanState *pstate)
{
/*
* Forwards scan, so check for stepping off end of window. If
- * we are at the end of the window, return NULL without
- * advancing the subplan or the position variable; but change
- * the state machine state to record having done so.
+ * we are at the end of the window, the behavior depends whether
+ * ONLY or WITH TIES was specified. In case of ONLY, we return
+ * NULL without advancing the subplan or the position variable;
+ * but change the state machine state to record having done so.
+ * In the WITH TIES mode, we need to advance the subplan until
+ * we find the first row with different ORDER BY pathkeys.
*/
if (!node->noCount &&
- node->position - node->offset >= node->count)
+ node->position - node->offset >= node->count &&
+ node->limitOption == EXACT_NUMBER)
{
node->lstate = LIMIT_WINDOWEND;
@@ -144,18 +159,69 @@ ExecLimit(PlanState *pstate)
return NULL;
}
+else if (!node->noCount &&
+ node->position - node->offset >= node->count &&
+ node->limitOption == WITH_TIES)
+{
+ /*
+ * Get next tuple from subplan, if any.
+ */
+ slot = ExecProcNode(outerPlan);
+ if (TupIsNull(slot))
+ {
+ node->lstate = LIMIT_SUBPLANEOF;
+ return NULL;
+ }
+ /*
+ * Test if the new tuple and the last tuple match.
+ * If so we return the tuple.
+ */
+ econtext->ecxt_innertuple = slot;
+ econtext->ecxt_outertuple = node->last_slot;
+ if (ExecQualAndReset(node->eqfunction, econtext))
+ {
+ node->subSlot = slot;
+ node->position++;
+ }
+ else
+ {
+ node->lstate = LIMIT_WINDOWEND;
+
+ /*
+ * If we know we won't need to back up, we can release
+ * resources at this point.
+ */
+ if (!(node->ps.state->es_top_eflags & EXEC_FLAG_BACKWARD))
+ (void) ExecShutdownNode(outerPlan);
+
+ return NULL;
+ }
-/*
- * Get next tuple from subplan, if any.
- */
-slot = ExecProcNode(outerPlan);
-if (TupIsNull(slot))
+}
+else
{
- node->lstate = LIMIT_SUBPLANEOF;
- return NULL;
+ /*
+ * Get next tuple from subplan, if any.
+ */
+ slot = ExecProcNode(outerPlan);
+ if (TupIsNull(slot))
+ {
+ node->lstate = LIMIT_SUBPLANEOF;
+ return NULL;
Re: Introduce MIN/MAX aggregate functions to pg_lsn
Hi,
Here are same review comment
- any numeric, string, date/time, network, or enum type,
+ any numeric, string, date/time, network, lsn, or enum type,
or arrays of these types
same as argument type
In the documentation it refereed as pg_lsn type rather than lsn alone
+Datum
+pg_lsn_larger(PG_FUNCTION_ARGS)
+{
+ XLogRecPtr lsn1 = PG_GETARG_LSN(0);
+ XLogRecPtr lsn2 = PG_GETARG_LSN(1);
+ XLogRecPtr result;
+
+ result = ((lsn1 > lsn2) ? lsn1 : lsn2);
+
+ PG_RETURN_LSN(result);
+}
rather than using additional variable its more readable and effective to
return the argument
itself like we do in date data type and other place
regards
Surafel
Change atoi to strtol in same place
Hello,
we use atoi for user argument processing in same place which return zero
for both invalid input and input value zero. In most case its ok because we
error out with appropriate error message for input zero but in same place
where we accept zero as valued input it case a problem by preceding for
invalid input as input value zero. The attached patch change those place to
strtol which can handle invalid input
regards
Surafel
diff --git a/contrib/pg_standby/pg_standby.c b/contrib/pg_standby/pg_standby.c
index 23f706b21d..2bcacbfbb5 100644
--- a/contrib/pg_standby/pg_standby.c
+++ b/contrib/pg_standby/pg_standby.c
@@ -27,6 +27,7 @@
#include
#include
#include
+#include
#include
#include
@@ -638,6 +639,14 @@ sigquit_handler(int sig)
int
main(int argc, char **argv)
{
+ char *keepfilesendptr;
+ char *maxretriesendptr;
+ char *sleeptimeendptr;
+ char *maxwaittimeendptr;
+ long numkeepfiles;
+ long nummaxretries;
+ long numsleeptime;
+ long nummaxwaittime;
int c;
progname = get_progname(argv[0]);
@@ -688,12 +697,17 @@ main(int argc, char **argv)
debug = true;
break;
case 'k': /* keepfiles */
-keepfiles = atoi(optarg);
-if (keepfiles < 0)
+errno = 0;
+numkeepfiles = strtol(optarg, , 10);
+
+if (keepfilesendptr == optarg || *keepfilesendptr != '\0' ||
+ numkeepfiles < 0 || numkeepfiles > INT_MAX ||
+ errno == ERANGE)
{
- fprintf(stderr, "%s: -k keepfiles must be >= 0\n", progname);
+ fprintf(stderr, "%s: -k keepfiles must be in range %d..%d\n", progname, 0, INT_MAX);
exit(2);
}
+keepfiles = (int) numkeepfiles;
break;
case 'l': /* Use link */
@@ -707,31 +721,46 @@ main(int argc, char **argv)
#endif
break;
case 'r': /* Retries */
-maxretries = atoi(optarg);
-if (maxretries < 0)
+errno = 0;
+nummaxretries = strtol(optarg, , 10);
+
+if (maxretriesendptr == optarg || *maxretriesendptr != '\0' ||
+ nummaxretries < 0 || nummaxretries > INT_MAX ||
+ errno == ERANGE)
{
- fprintf(stderr, "%s: -r maxretries must be >= 0\n", progname);
+ fprintf(stderr, "%s: -r maxretries must be in range %d..%d\n", progname, 0, INT_MAX);
exit(2);
}
+maxretries = (int) nummaxretries;
break;
case 's': /* Sleep time */
-sleeptime = atoi(optarg);
-if (sleeptime <= 0 || sleeptime > 60)
+errno = 0;
+numsleeptime = strtol(optarg, , 10);
+
+if (sleeptimeendptr == optarg || *sleeptimeendptr != '\0' ||
+ numsleeptime <= 0 || numsleeptime > 60 ||
+ errno == ERANGE)
{
- fprintf(stderr, "%s: -s sleeptime incorrectly set\n", progname);
+ fprintf(stderr, "%s: -s sleeptime must be in range %d..%d\n", progname, 1, 59);
exit(2);
}
+sleeptime = (int) numsleeptime;
break;
case 't': /* Trigger file */
triggerPath = pg_strdup(optarg);
break;
case 'w': /* Max wait time */
-maxwaittime = atoi(optarg);
-if (maxwaittime < 0)
+errno = 0;
+nummaxwaittime = strtol(optarg, , 10);
+
+if (maxwaittimeendptr == optarg || *maxwaittimeendptr != '\0' ||
+ nummaxwaittime < 0 || nummaxwaittime > INT_MAX ||
+ errno == ERANGE)
{
- fprintf(stderr, "%s: -w maxwaittime incorrectly set\n", progname);
+ fprintf(stderr, "%s: -w maxwaittime must be in range %d..%d\n", progname, 0, INT_MAX);
exit(2);
}
+maxwaittime = (int) nummaxwaittime;
break;
default:
fprintf(stderr, "Try \"%s --help\" for more information.\n", progname);
diff --git a/src/bin/pg_basebackup/pg_basebackup.c b/src/bin/pg_basebackup/pg_basebackup.c
index 63f554307c..16d44c8617 100644
--- a/src/bin/pg_basebackup/pg_basebackup.c
+++ b/src/bin/pg_basebackup/pg_basebackup.c
@@ -2193,6 +2193,10 @@ main(int argc, char **argv)
{"no-verify-checksums", no_argument, NULL, 3},
{NULL, 0, NULL, 0}
};
+ char *compressEndptr;
+ char *timeoutEndptr;
+ long compressNumber;
+ long timeoutNumber;
int c;
int option_index;
@@ -2305,12 +2309,18 @@ main(int argc, char **argv)
#endif
break;
case 'Z':
-compresslevel = atoi(optarg);
-if (compresslevel < 0 || compresslevel > 9)
+errno = 0;
+compressNumber = strtol(optarg, , 10);
+
+if (compressEndptr == optarg || *compressEndptr != '\0' ||
+ compressNumber < 0 || compressNumber > 9 ||
+ errno == ERANGE)
{
- pg_log_error("invalid compression level \"%s\"\n", optarg);
+ pg_log_error("compression level must be in range %d..%d \"%s\"\n",
+ 0, 9, optarg);
exit(1);
}
+compresslevel = (int) compressNumber;
break;
case 'c':
if (pg_strcasecmp(optarg, "fast") == 0)
@@ -2343,12 +2353,18 @@ main(int argc, char **argv)
dbgetpassword = 1;
break;
case 's':
-standby_message_timeout = atoi(optarg) * 1000;
-if (standby_message_timeout < 0)
+errno = 0;
+timeoutNumber = strtol(optarg, , 10);
+
+if (timeoutEndptr ==
Re: FETCH FIRST clause WITH TIES option
Hi Thomas
On Mon, Jul 1, 2019 at 1:31 PM Thomas Munro wrote:
> On Mon, Apr 8, 2019 at 8:26 PM Surafel Temesgen
> wrote:
> > agree
>
> Hi Surafel,
>
> A new Commitfest is starting. Can you please post a fresh rebase of this
> patch?
>
>
Thank you for informing. attach is a rebased patch against current master
regards
Surafel
diff --git a/doc/src/sgml/ref/select.sgml b/doc/src/sgml/ref/select.sgml
index 06d611b64c..e83d309c5b 100644
--- a/doc/src/sgml/ref/select.sgml
+++ b/doc/src/sgml/ref/select.sgml
@@ -44,7 +44,7 @@ SELECT [ ALL | DISTINCT [ ON ( expressionexpression [ ASC | DESC | USING operator ] [ NULLS { FIRST | LAST } ] [, ...] ]
[ LIMIT { count | ALL } ]
[ OFFSET start [ ROW | ROWS ] ]
-[ FETCH { FIRST | NEXT } [ count ] { ROW | ROWS } ONLY ]
+[ FETCH { FIRST | NEXT } [ count ] { ROW | ROWS } { ONLY | WITH TIES } ]
[ FOR { UPDATE | NO KEY UPDATE | SHARE | KEY SHARE } [ OF table_name [, ...] ] [ NOWAIT | SKIP LOCKED ] [...] ]
where from_item can be one of:
@@ -1430,7 +1430,7 @@ OFFSET start
which PostgreSQL also supports. It is:
OFFSET start { ROW | ROWS }
-FETCH { FIRST | NEXT } [ count ] { ROW | ROWS } ONLY
+FETCH { FIRST | NEXT } [ count ] { ROW | ROWS } { ONLY | WITH TIES }
In this syntax, the start
or count value is required by
@@ -1440,7 +1440,10 @@ FETCH { FIRST | NEXT } [ count ] {
ambiguity.
If count is
omitted in a FETCH clause, it defaults to 1.
-ROW
+ROW .
+WITH TIES option is used to return two or more rows
+that tie for the last place in the result set according to ORDER BY
+clause (ORDER BY clause must be specified in this case).
and ROWS as well as FIRST
and NEXT are noise words that don't influence
the effects of these clauses.
diff --git a/src/backend/executor/nodeLimit.c b/src/backend/executor/nodeLimit.c
index baa669abe8..60660e710f 100644
--- a/src/backend/executor/nodeLimit.c
+++ b/src/backend/executor/nodeLimit.c
@@ -41,6 +41,7 @@ static TupleTableSlot * /* return: a tuple or NULL */
ExecLimit(PlanState *pstate)
{
LimitState *node = castNode(LimitState, pstate);
+ ExprContext *econtext = node->ps.ps_ExprContext;
ScanDirection direction;
TupleTableSlot *slot;
PlanState *outerPlan;
@@ -126,12 +127,16 @@ ExecLimit(PlanState *pstate)
{
/*
* Forwards scan, so check for stepping off end of window. If
- * we are at the end of the window, return NULL without
- * advancing the subplan or the position variable; but change
- * the state machine state to record having done so.
+ * we are at the end of the window, the behavior depends whether
+ * ONLY or WITH TIES was specified. In case of ONLY, we return
+ * NULL without advancing the subplan or the position variable;
+ * but change the state machine state to record having done so.
+ * In the WITH TIES mode, we need to advance the subplan until
+ * we find the first row with different ORDER BY pathkeys.
*/
if (!node->noCount &&
- node->position - node->offset >= node->count)
+ node->position - node->offset >= node->count &&
+ node->limitOption == EXACT_NUMBER)
{
node->lstate = LIMIT_WINDOWEND;
@@ -144,18 +149,69 @@ ExecLimit(PlanState *pstate)
return NULL;
}
+else if (!node->noCount &&
+ node->position - node->offset >= node->count &&
+ node->limitOption == WITH_TIES)
+{
+ /*
+ * Get next tuple from subplan, if any.
+ */
+ slot = ExecProcNode(outerPlan);
+ if (TupIsNull(slot))
+ {
+ node->lstate = LIMIT_SUBPLANEOF;
+ return NULL;
+ }
+ /*
+ * Test if the new tuple and the last tuple match.
+ * If so we return the tuple.
+ */
+ econtext->ecxt_innertuple = slot;
+ econtext->ecxt_outertuple = node->last_slot;
+ if (ExecQualAndReset(node->eqfunction, econtext))
+ {
+ node->subSlot = slot;
+ node->position++;
+ }
+ else
+ {
+ node->lstate = LIMIT_WINDOWEND;
+
+ /*
+ * If we know we won't need to back up, we can release
+ * resources at this point.
+ */
+ if (!(node->ps.state->es_top_eflags & EXEC_FLAG_BACKWARD))
+ (void) ExecShutdownNode(outerPlan);
+
+ return NULL;
+ }
-/*
- * Get next tuple from subplan, if any.
- */
-slot = ExecProcNode(outerPlan);
-if (TupIsNull(slot))
+}
+else
{
- node->lstate = LIMIT_SUBPLANEOF;
- return NULL;
+ /*
+ * Get next tuple from subplan, if any.
+ */
+ slot = ExecProcNode(outerPlan);
+ if (TupIsNull(slot))
+ {
+ node->lstate = LIMIT_SUBPLANEOF;
+ return NULL;
+ }
+
+ /*
+ * Tuple at limit is needed for comparation in subsequent execution
+ * to detect ties.
+ */
+ if (no
Re: Conflict handling for COPY FROM
On Wed, Feb 20, 2019 at 7:04 PM Andres Freund wrote:
>
>
> On February 20, 2019 6:05:53 AM PST, Andrew Dunstan <
> andrew.duns...@2ndquadrant.com> wrote:
> >
> >On 2/20/19 8:01 AM, Surafel Temesgen wrote:
> >>
> >>
> >> On Tue, Feb 19, 2019 at 3:47 PM Andres Freund >> <mailto:and...@anarazel.de>> wrote:
> >>
> >>
> >>
> >> Err, what? Again, that requires super user permissions (in
> >> contrast to copy from/to stdin/out). Backends run as the user
> >> postgres runs under
> >>
> >>
> >>
> >> okay i see it now and modified the patch similarly
> >>
> >>
> >
> >
> >Why log to a file at all? We do have, you know, a database handy, where
> >we might more usefully log errors. You could usefully log the offending
> >row as an array of text, possibly.
>
> Or even just return it as a row. CopyBoth is relatively widely supported
> these days.
>
>
hello,
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
In addition to the above change in the attached patch i also change
the syntax to ERROR LIMIT because it is no longer only skip
unique and exclusion constrain violation
regards
Surafel
diff --git a/doc/src/sgml/ref/copy.sgml b/doc/src/sgml/ref/copy.sgml
index 5e2992ddac..dc3b943279 100644
--- a/doc/src/sgml/ref/copy.sgml
+++ b/doc/src/sgml/ref/copy.sgml
@@ -44,6 +44,7 @@ COPY { table_name [ ( column_name [, ...] )
FORCE_NULL ( column_name [, ...] )
ENCODING 'encoding_name'
+ERROR_LIMIT 'limit_number'
@@ -355,6 +356,21 @@ COPY { table_name [ (
+
+ERROR_LIMIT
+
+
+ Specifies to ignore error record up to limit_number number.
+
+
+
+
+
+Currently, only unique or exclusion constraint violation
+and same record formatting error is ignored.
+
+
WHERE
diff --git a/src/backend/commands/copy.c b/src/backend/commands/copy.c
index f1161f0fee..05a5f29d4c 100644
--- a/src/backend/commands/copy.c
+++ b/src/backend/commands/copy.c
@@ -48,6 +48,7 @@
#include "port/pg_bswap.h"
#include "rewrite/rewriteHandler.h"
#include "storage/fd.h"
+#include "storage/lmgr.h"
#include "tcop/tcopprot.h"
#include "utils/builtins.h"
#include "utils/lsyscache.h"
@@ -154,6 +155,7 @@ typedef struct CopyStateData
List *convert_select; /* list of column names (can be NIL) */
bool *convert_select_flags; /* per-column CSV/TEXT CS flags */
Node *whereClause; /* WHERE condition (or NULL) */
+ int error_limit; /* total number of error to ignore */
/* these are just for error messages, see CopyFromErrorCallback */
const char *cur_relname; /* table name for error messages */
@@ -1291,6 +1293,21 @@ ProcessCopyOptions(ParseState *pstate,
defel->defname),
parser_errposition(pstate, defel->location)));
}
+ else if (strcmp(defel->defname, "error_limit") == 0)
+ {
+ if (cstate->error_limit > 0)
+ereport(ERROR,
+ (errcode(ERRCODE_SYNTAX_ERROR),
+ errmsg("conflicting or redundant options"),
+ parser_errposition(pstate, defel->location)));
+ cstate->error_limit = defGetInt64(defel);
+ if (cstate->error_limit <= 0)
+ereport(ERROR,
+ (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+ errmsg("argument to option \"%s\" must be positive integer",
+defel->defname),
+ parser_errposition(pstate, defel->location)));
+ }
else
ereport(ERROR,
(errcode(ERRCODE_SYNTAX_ERROR),
@@ -1441,6 +1458,10 @@ ProcessCopyOptions(ParseState *pstate,
ereport(ERROR,
(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
errmsg("CSV quote character must not appear in the NULL specification")));
+ if (cstate->error_limit && !cstate->is_copy_from)
+ ereport(ERROR,
+(errcode(ERRCODE_SYNTAX_ERROR),
+ errmsg("ERROR LIMIT only available using COPY FROM")));
}
/*
@@ -2837,7 +2858,10 @@ CopyFrom(CopyState cstate)
/* Verify the named relation is a valid target for INSERT */
CheckValidResultRel(resultRelInfo, CMD_INSERT);
- ExecOpenIndices(resultRelInfo, false);
+ if (cstate->error_limit)
+ ExecOpenIndices(resultRelInfo, true);
+ else
+ ExecOpenIndices(resultRelInfo, false);
estate->es_result_relations = resultRelInfo;
estate->es_num_result_relations = 1;
@@ -2942,6 +2966,13 @@ CopyFrom(CopyState cstate)
*/
insertMethod = CIM_SINGLE;
}
+ else if (cstate->error_limit)
+ {
+ /*
+ * Can't support speculative insertion in multi-inserts.
+ */
Re: FETCH FIRST clause PERCENT option
Hello,
The attached patch include the fix for all the comment given
regards
Surafel
On Mon, Apr 1, 2019 at 12:34 AM Andres Freund wrote:
> Hi,
>
> On 2019-03-29 12:04:50 +0300, Surafel Temesgen wrote:
>
> > + if (node->limitOption == PERCENTAGE)
> > + {
> > + while (node->position -
> node->offset < node->count)
> > + {
> > + slot =
> MakeSingleTupleTableSlot(tupleDescriptor, );
> > + if
> (tuplestore_gettupleslot(node->tupleStore, true, true, slot))
>
> There's several blocks like this - how's this not going to be a resource
> leak? As far as I can tell you're just going to create new slots, and
> their previous contents aren't going to be cleared? I think there very
> rarely are cases where an executor node's *Next function (or its
> subsidiaries) creates slots. Normally you ought to create them *once* on
> the *Init function.
>
> You might not directly leak memory, because this is probably running in
> a short lived context, but you'll leak tuple desc references etc. (Or if
> it were a heap buffer slot, buffer pins).
>
>
> > + /* In PERCENTAGE case the result is already in
> tuplestore */
> > + if (node->limitOption == PERCENTAGE)
> > + {
> > + slot =
> MakeSingleTupleTableSlot(tupleDescriptor, );
> > + if
> (tuplestore_gettupleslot(node->tupleStore, false, false, slot))
> > + {
> > + node->subSlot = slot;
> > + node->lstate = LIMIT_INWINDOW;
> > + }
> > + else
> > + elog(ERROR, "LIMIT subplan failed
> to run backwards");
> > + }
> > + else if (node->limitOption == EXACT_NUMBER)
> > + {
> > /*
> >* Backing up from subplan EOF, so re-fetch
> previous tuple; there
> >* should be one! Note previous tuple must be in
> window.
> > @@ -194,6 +329,7 @@ ExecLimit(PlanState *pstate)
> > node->subSlot = slot;
> > node->lstate = LIMIT_INWINDOW;
> > /* position does not change 'cause we didn't
> advance it before */
> > + }
>
> The indentation here looks wrong...
>
> > break;
> >
> > case LIMIT_WINDOWEND:
> > @@ -278,17 +414,29 @@ recompute_limits(LimitState *node)
> > /* Interpret NULL count as no count (LIMIT ALL) */
> > if (isNull)
> > {
> > - node->count = 0;
> > + node->count = 1;
> > node->noCount = true;
>
> Huh?
>
>
> > }
> > else
> > {
> > - node->count = DatumGetInt64(val);
> > - if (node->count < 0)
> > - ereport(ERROR,
> > -
> (errcode(ERRCODE_INVALID_ROW_COUNT_IN_LIMIT_CLAUSE),
> > - errmsg("LIMIT must not be
> negative")));
> > - node->noCount = false;
> > + if (node->limitOption == PERCENTAGE)
> > + {
> > + /*
> > + * We expect to return at least one row
> (unless there
> > + * are no rows in the subplan), and we'll
> update this
> > + * count later as we go.
> > + */
> > + node->count = 0;
> > + node->percent = DatumGetFloat8(val);
> > + }
> > + else
> > + {
> > + node->count = DatumGetInt64(val);
> > + if (node->count < 0)
> > + ereport(ERROR,
> > +
> (errcode(ERRCODE_INVALID_ROW_COUNT_IN_LIMIT_CLAUSE),
> > +
Re: with oids option not removed in pg_dumpall
Thank you for applying regards Surafel On Thu, May 23, 2019 at 3:43 AM Michael Paquier wrote: > On Tue, May 21, 2019 at 05:24:57PM +0900, Michael Paquier wrote: > > Good catch. Your cleanup looks correct to me. Andres, perhaps you > > would prefer doing the cleanup yourself? > > As I am cleaning up the area for another issue, applied. > -- > Michael >
with oids option not removed in pg_dumpall
Hello,
Commit 578b229718e8f remove oids option from pg_dump but its is
still in pg_dumpall .The attach patch remove it
regards
Surafel
diff --git a/doc/src/sgml/ref/pg_dumpall.sgml b/doc/src/sgml/ref/pg_dumpall.sgml
index 24c8c031d6..b35c702f99 100644
--- a/doc/src/sgml/ref/pg_dumpall.sgml
+++ b/doc/src/sgml/ref/pg_dumpall.sgml
@@ -130,20 +130,6 @@ PostgreSQL documentation
-
- -o
- --oids
-
-
-Dump object identifiers (OIDs) as part of the
-data for every table. Use this option if your application references
-the OID
-columns in some way (e.g., in a foreign key constraint).
-Otherwise, this option should not be used.
-
-
-
-
-O
--no-owner
diff --git a/src/bin/pg_dump/pg_dumpall.c b/src/bin/pg_dump/pg_dumpall.c
index 102731ea0c..38c5bc6eb2 100644
--- a/src/bin/pg_dump/pg_dumpall.c
+++ b/src/bin/pg_dump/pg_dumpall.c
@@ -107,7 +107,6 @@ main(int argc, char *argv[])
{"host", required_argument, NULL, 'h'},
{"dbname", required_argument, NULL, 'd'},
{"database", required_argument, NULL, 'l'},
- {"oids", no_argument, NULL, 'o'},
{"no-owner", no_argument, NULL, 'O'},
{"port", required_argument, NULL, 'p'},
{"roles-only", no_argument, NULL, 'r'},
@@ -211,7 +210,7 @@ main(int argc, char *argv[])
pgdumpopts = createPQExpBuffer();
- while ((c = getopt_long(argc, argv, "acd:E:f:gh:l:oOp:rsS:tU:vwWx", long_options, )) != -1)
+ while ((c = getopt_long(argc, argv, "acd:E:f:gh:l:Op:rsS:tU:vwWx", long_options, )) != -1)
{
switch (c)
{
@@ -252,10 +251,6 @@ main(int argc, char *argv[])
pgdb = pg_strdup(optarg);
break;
- case 'o':
-appendPQExpBufferStr(pgdumpopts, " -o");
-break;
-
case 'O':
appendPQExpBufferStr(pgdumpopts, " -O");
break;
@@ -628,7 +623,6 @@ help(void)
printf(_(" -c, --clean clean (drop) databases before recreating\n"));
printf(_(" -E, --encoding=ENCODING dump the data in encoding ENCODING\n"));
printf(_(" -g, --globals-only dump only global objects, no databases\n"));
- printf(_(" -o, --oids include OIDs in dump\n"));
printf(_(" -O, --no-owner skip restoration of object ownership\n"));
printf(_(" -r, --roles-only dump only roles, no databases or tablespaces\n"));
printf(_(" -s, --schema-onlydump only the schema, no data\n"));
Re: FETCH FIRST clause WITH TIES option
On Sun, Apr 7, 2019 at 1:39 AM Tomas Vondra wrote: > > 1) I've removed the costing changes. As Tom mentions elsewhere in this > thread, that's probably not needed for v1 and it's true those estimates > are probably somewhat sketchy anyway. > > > 2) v8 did this (per my comment): > > - ExecSetTupleBound(compute_tuples_needed(node), > outerPlanState(node)); > + if(node->limitOption == EXACT_NUMBER) > + ExecSetTupleBound(compute_tuples_needed(node), > outerPlanState(node)); > > but I noticed the comment immediately above that says > > * Notify child node about limit. Note: think not to "optimize" by > * skipping ExecSetTupleBound if compute_tuples_needed returns < 0. We > * must update the child node anyway, in case this is a rescan and the > * previous time we got a different result. > > which applies to WITH_TIES too, no? So I've modified compute_tuples_needed > to return -1, and reverted this change. Not sure, though. > > > I agree that passing -1 to ExecSetTupleBound is more appropriate implementation 3) I'm a bit confused by the initialization added to ExecInitLimit. It > first gets the tuple descriptor from the limitstate (it should not do so > directly but use ExecGetResultType). But when it creates the extra slot, > it uses ops extracted from the outer plan. That's strange, I guess ... > > And then it extracts the descriptor from the outer plan and uses it when > calling execTuplesMatchPrepare. But AFAIK it's going to be compared to > the last_slot, which is using a descriptor from the limitstate. > > IMHO all of this should use descriptor/ops from the outer plan, no? It > probably does not change anything because limit does not project, but it > seems confusing. > agree regards Surafel
Re: FETCH FIRST clause PERCENT option
On Tue, Mar 26, 2019 at 5:46 PM Tomas Vondra wrote: > On Tue, Mar 26, 2019 at 03:06:52PM +0300, Surafel Temesgen wrote: > >On Thu, Feb 28, 2019 at 11:16 PM Tomas Vondra < > tomas.von...@2ndquadrant.com> > >wrote: > > > >> > >> To give you a (admittedly, somewhat contrived and artificial example): > >> > >> SELECT * FROM t1 WHERE id IN ( > >> SELECT id FROM t2 ORDER BY x FETCH FIRST 10 PERCENT ROWS ONLY > >> ); > >> > >> Maybe this example is bogus and/or does not really matter in practice. I > >> don't know, but I've been unable to convince myself that's the case. > > > > > >does this means we abandon incremental approach? and am not sure of > >calculating > >percentage after OFFSET clause is acceptable or not > > > > I don't follow. I don't think I've suggested to abandon the incremental > approach - I've explained why I think it's what the patch should be doing, > and illustrated that with an example. > but it is more complex than the previous approach and it will be more complex in starting calculating percentage before offset row count. Doesn't simplicity prefer? regards Surafel
Re: FETCH FIRST clause PERCENT option
Thank you for looking at it
On Mon, Apr 1, 2019 at 12:34 AM Andres Freund wrote:
> Hi,
>
> On 2019-03-29 12:04:50 +0300, Surafel Temesgen wrote:
>
> > + if (node->limitOption == PERCENTAGE)
> > + {
> > + while (node->position -
> node->offset < node->count)
> > + {
> > + slot =
> MakeSingleTupleTableSlot(tupleDescriptor, );
> > + if
> (tuplestore_gettupleslot(node->tupleStore, true, true, slot))
>
> There's several blocks like this - how's this not going to be a resource
> leak? As far as I can tell you're just going to create new slots, and
> their previous contents aren't going to be cleared? I think there very
> rarely are cases where an executor node's *Next function (or its
> subsidiaries) creates slots. Normally you ought to create them *once* on
> the *Init function.
>
>
create it in init stage is good idea. i make it this way because
tuplestore_gettupleslot
work with TTSOpsMinimalTuple
> You might not directly leak memory, because this is probably running in
> a short lived context, but you'll leak tuple desc references etc. (Or if
> it were a heap buffer slot, buffer pins).
>
>
> > + /* In PERCENTAGE case the result is already in
> tuplestore */
> > + if (node->limitOption == PERCENTAGE)
> > + {
> > + slot =
> MakeSingleTupleTableSlot(tupleDescriptor, );
> > + if
> (tuplestore_gettupleslot(node->tupleStore, false, false, slot))
> > + {
> > + node->subSlot = slot;
> > + node->lstate = LIMIT_INWINDOW;
> > + }
> > + else
> > + elog(ERROR, "LIMIT subplan failed
> to run backwards");
> > + }
> > + else if (node->limitOption == EXACT_NUMBER)
> > + {
> > /*
> >* Backing up from subplan EOF, so re-fetch
> previous tuple; there
> >* should be one! Note previous tuple must be in
> window.
> > @@ -194,6 +329,7 @@ ExecLimit(PlanState *pstate)
> > node->subSlot = slot;
> > node->lstate = LIMIT_INWINDOW;
> > /* position does not change 'cause we didn't
> advance it before */
> > + }
>
> The indentation here looks wrong...
>
> > break;
> >
> > case LIMIT_WINDOWEND:
> > @@ -278,17 +414,29 @@ recompute_limits(LimitState *node)
> > /* Interpret NULL count as no count (LIMIT ALL) */
> > if (isNull)
> > {
> > - node->count = 0;
> > + node->count = 1;
> > node->noCount = true;
>
> Huh?
>
>
> > }
> > else
> > {
> > - node->count = DatumGetInt64(val);
> > - if (node->count < 0)
> > - ereport(ERROR,
> > -
> (errcode(ERRCODE_INVALID_ROW_COUNT_IN_LIMIT_CLAUSE),
> > - errmsg("LIMIT must not be
> negative")));
> > - node->noCount = false;
> > + if (node->limitOption == PERCENTAGE)
> > + {
> > + /*
> > + * We expect to return at least one row
> (unless there
> > + * are no rows in the subplan), and we'll
> update this
> > + * count later as we go.
> > + */
> > + node->count = 0;
> > + node->percent = DatumGetFloat8(val);
> > + }
> > + else
> > + {
> > + node->count = DatumGetInt64(val);
> > + if (node->count < 0)
> > + ereport(ERROR,
> > +
> (errcode(ERRCODE_INVALID_ROW_COU
Re: FETCH FIRST clause PERCENT option
On Tue, Mar 26, 2019 at 5:46 PM Tomas Vondra wrote: > On Tue, Mar 26, 2019 at 03:06:52PM +0300, Surafel Temesgen wrote: > > As for the OFFSET, I don't see why that would be incompatible with PERCENT > clause. I'm not sure if it should compute the percentage from the total > number of rows (including those skipped by the OFFSET) or the rest, but I > assume SQL standard says something about that. > > I check SQL standard and it is different in PERCENT cause. It state that the percentage is computed using the number of rows before removing the rows specified by offset row count regards Surafel
Re: Re: FETCH FIRST clause WITH TIES option
On Sun, Mar 31, 2019 at 3:14 AM Tomas Vondra
wrote:
>
>
> Hi,
>
> I got to look at the patch today, with the intent to commit, but sadly I
> ran into a couple of minor issues that I don't feel comfortable fixing
> on my own. Attached is a patch highlighling some of the places (0001 is
> your v7 patch, to keep the cfbot happy).
>
>
Thank you
>
> 1) the docs documented this as
>
>... [ ONLY | WITH TIES ]
>
> but that's wrong, because it implies those options are optional (i.e.
> the user may not specify anything). That's not the case, exactly one
> of those options needs to be specified, so it should have been
>
>... { ONLY | WITH TIES }
>
>
> 2) The comment in ExecLimit() needs to be updated to explain that WITH
> TIES changes the behavior.
>
>
> 3) Minor code style issues (no space before * on comment lines, {}
> around single-line if statements, ...).
>
>
> 4) The ExecLimit() does this
>
> if (node->limitOption == WITH_TIES)
> ExecCopySlot(node->last_slot, slot);
>
> but I think we only really need to do that for the last tuple in the
> window, no? Would it be a useful optimization?
>
>
>
I think it is good optimization .Fixed
5) Two issues in _outLimit(). Firstly, when printing uniqCollations the
> code actually prints uniqOperators. Secondly, why does the code use
> these loops at all, instead of using WRITE_ATTRNUMBER_ARRAY and
> WRITE_OID_ARRAY, like other places? Perhaps there's an issue with empty
> arrays? I haven't tested this, but looking at the READ_ counterparts, I
> don't see why that would be the case.
>
>
>
Fixed
regards
Surafel
diff --git a/doc/src/sgml/ref/select.sgml b/doc/src/sgml/ref/select.sgml
index 06d611b64c..e83d309c5b 100644
--- a/doc/src/sgml/ref/select.sgml
+++ b/doc/src/sgml/ref/select.sgml
@@ -44,7 +44,7 @@ SELECT [ ALL | DISTINCT [ ON ( expressionexpression [ ASC | DESC | USING operator ] [ NULLS { FIRST | LAST } ] [, ...] ]
[ LIMIT { count | ALL } ]
[ OFFSET start [ ROW | ROWS ] ]
-[ FETCH { FIRST | NEXT } [ count ] { ROW | ROWS } ONLY ]
+[ FETCH { FIRST | NEXT } [ count ] { ROW | ROWS } { ONLY | WITH TIES } ]
[ FOR { UPDATE | NO KEY UPDATE | SHARE | KEY SHARE } [ OF table_name [, ...] ] [ NOWAIT | SKIP LOCKED ] [...] ]
where from_item can be one of:
@@ -1430,7 +1430,7 @@ OFFSET start
which PostgreSQL also supports. It is:
OFFSET start { ROW | ROWS }
-FETCH { FIRST | NEXT } [ count ] { ROW | ROWS } ONLY
+FETCH { FIRST | NEXT } [ count ] { ROW | ROWS } { ONLY | WITH TIES }
In this syntax, the start
or count value is required by
@@ -1440,7 +1440,10 @@ FETCH { FIRST | NEXT } [ count ] {
ambiguity.
If count is
omitted in a FETCH clause, it defaults to 1.
-ROW
+ROW .
+WITH TIES option is used to return two or more rows
+that tie for the last place in the result set according to ORDER BY
+clause (ORDER BY clause must be specified in this case).
and ROWS as well as FIRST
and NEXT are noise words that don't influence
the effects of these clauses.
diff --git a/src/backend/executor/nodeLimit.c b/src/backend/executor/nodeLimit.c
index baa669abe8..994ac7f089 100644
--- a/src/backend/executor/nodeLimit.c
+++ b/src/backend/executor/nodeLimit.c
@@ -41,6 +41,7 @@ static TupleTableSlot * /* return: a tuple or NULL */
ExecLimit(PlanState *pstate)
{
LimitState *node = castNode(LimitState, pstate);
+ ExprContext *econtext = node->ps.ps_ExprContext;
ScanDirection direction;
TupleTableSlot *slot;
PlanState *outerPlan;
@@ -126,12 +127,16 @@ ExecLimit(PlanState *pstate)
{
/*
* Forwards scan, so check for stepping off end of window. If
- * we are at the end of the window, return NULL without
- * advancing the subplan or the position variable; but change
- * the state machine state to record having done so.
+ * we are at the end of the window, the behavior depends whether
+ * ONLY or WITH TIES was specified. In case of ONLY, we return
+ * NULL without advancing the subplan or the position variable;
+ * but change the state machine state to record having done so.
+ * In the WITH TIES mode, we need to advance the subplan until
+ * we find the first row with different ORDER BY pathkeys.
*/
if (!node->noCount &&
- node->position - node->offset >= node->count)
+ node->position - node->offset >= node->count &&
+ node->limitOption == EXACT_NUMBER)
{
node->lstate = LIMIT_WINDOWEND;
@@ -144,18 +149,69 @@ ExecLimit(PlanState *pstate)
return NULL;
}
+else if (!node->noCount &&
+ node->position - node->offset >= node->count &&
+ node->limitOption == WITH_TIES)
+{
+ /*
+ * Get next tuple from subplan, if any.
+ */
+ slot = ExecProcNode(outerPlan);
+ if (TupIsNull(slot))
+ {
+ node->lstate = LIMIT_SUBPLANEOF;
+ return NULL;
+ }
+ /*
+ * Test if the new tuple and the last tuple match.
+ * If so we return the
Re: FETCH FIRST clause PERCENT option
Hi,
On Thu, Feb 28, 2019 at 2:50 PM Kyotaro HORIGUCHI <
horiguchi.kyot...@lab.ntt.co.jp> wrote:
>
> -* previous time we got a different result.
> +* previous time we got a different result.In PERCENTAGE option
> there are
> +* no bound on the number of output tuples */
> */
>
>
>
Thank you for testing .Fixed
diff --git a/doc/src/sgml/ref/select.sgml b/doc/src/sgml/ref/select.sgml
index 06d611b64c..e3ce4d7e36 100644
--- a/doc/src/sgml/ref/select.sgml
+++ b/doc/src/sgml/ref/select.sgml
@@ -44,7 +44,7 @@ SELECT [ ALL | DISTINCT [ ON ( expressionexpression [ ASC | DESC | USING operator ] [ NULLS { FIRST | LAST } ] [, ...] ]
[ LIMIT { count | ALL } ]
[ OFFSET start [ ROW | ROWS ] ]
-[ FETCH { FIRST | NEXT } [ count ] { ROW | ROWS } ONLY ]
+[ FETCH { FIRST | NEXT } [ count ] [ PERCENT ] { ROW | ROWS } ONLY ]
[ FOR { UPDATE | NO KEY UPDATE | SHARE | KEY SHARE } [ OF table_name [, ...] ] [ NOWAIT | SKIP LOCKED ] [...] ]
where from_item can be one of:
@@ -1430,7 +1430,7 @@ OFFSET start
which PostgreSQL also supports. It is:
OFFSET start { ROW | ROWS }
-FETCH { FIRST | NEXT } [ count ] { ROW | ROWS } ONLY
+FETCH { FIRST | NEXT } [ count ] [ PERCENT ] { ROW | ROWS } ONLY
In this syntax, the start
or count value is required by
@@ -1440,7 +1440,8 @@ FETCH { FIRST | NEXT } [ count ] {
ambiguity.
If count is
omitted in a FETCH clause, it defaults to 1.
-ROW
+with PERCENT count specifies the maximum number of rows to return
+in percentage.ROW
and ROWS as well as FIRST
and NEXT are noise words that don't influence
the effects of these clauses.
diff --git a/src/backend/executor/nodeLimit.c b/src/backend/executor/nodeLimit.c
index baa669abe8..7bfd7fd034 100644
--- a/src/backend/executor/nodeLimit.c
+++ b/src/backend/executor/nodeLimit.c
@@ -21,6 +21,8 @@
#include "postgres.h"
+#include
+
#include "executor/executor.h"
#include "executor/nodeLimit.h"
#include "miscadmin.h"
@@ -44,6 +46,7 @@ ExecLimit(PlanState *pstate)
ScanDirection direction;
TupleTableSlot *slot;
PlanState *outerPlan;
+ TupleDesc tupleDescriptor;
CHECK_FOR_INTERRUPTS();
@@ -52,6 +55,8 @@ ExecLimit(PlanState *pstate)
*/
direction = node->ps.state->es_direction;
outerPlan = outerPlanState(node);
+ slot = node->subSlot;
+ tupleDescriptor = node->ps.ps_ResultTupleDesc;
/*
* The main logic is a simple state machine.
@@ -81,7 +86,15 @@ ExecLimit(PlanState *pstate)
/*
* Check for empty window; if so, treat like empty subplan.
*/
- if (node->count <= 0 && !node->noCount)
+ if (node->limitOption == PERCENTAGE)
+ {
+if (node->percent == 0.0)
+{
+ node->lstate = LIMIT_EMPTY;
+ return NULL;
+}
+ }
+ else if (node->count <= 0 && !node->noCount)
{
node->lstate = LIMIT_EMPTY;
return NULL;
@@ -107,6 +120,15 @@ ExecLimit(PlanState *pstate)
break;
}
+ /*
+ * We may needed this tuple in backward scan so put it into tuplestore.
+ */
+ if (node->limitOption == PERCENTAGE)
+ {
+tuplestore_puttupleslot(node->tupleStore, slot);
+tuplestore_advance(node->tupleStore, true);
+ }
+
/*
* Okay, we have the first tuple of the window.
*/
@@ -124,6 +146,72 @@ ExecLimit(PlanState *pstate)
case LIMIT_INWINDOW:
if (ScanDirectionIsForward(direction))
{
+
+/*
+ * In case of coming back from backward scan the tuple is already
+ * in tuple store.
+ */
+if (node->limitOption == PERCENTAGE && node->backwardPosition > 0)
+{
+ slot = MakeSingleTupleTableSlot(tupleDescriptor, );
+ if (tuplestore_gettupleslot(node->tupleStore, true, false, slot))
+ {
+ node->subSlot = slot;
+ node->position++;
+ node->backwardPosition--;
+ return slot;
+ }
+ else
+ {
+ node->lstate = LIMIT_SUBPLANEOF;
+ return NULL;
+ }
+}
+
+/*
+ * In PERCENTAGE case no need of executing outerPlan multiple times.
+ */
+if (node->limitOption == PERCENTAGE && node->reachEnd)
+{
+ node->lstate = LIMIT_WINDOWEND;
+
+ /*
+ * If we know we won't need to back up, we can release
+ * resources at this point.
+ */
+ if (!(node->ps.state->es_top_eflags & EXEC_FLAG_BACKWARD))
+ (void) ExecShutdownNode(outerPlan);
+
+ return NULL;
+}
+
+/*
+ * When in percentage mode, we need to see if we can get any
+ * additional rows from the subplan (enough to increase the
+ * node->count value).
+ */
+if (node->limitOption == PERCENTAGE)
+{
+ /* loop until the node->count increments */
+ while (node->position - node->offset >= node->count)
+ {
+ int64 cnt;
+
+ slot = ExecProcNode(outerPlan);
+ if (TupIsNull(slot))
+ {
+ node->reachEnd = true;
+ break;
+ }
+
+ tuplestore_puttupleslot(node->tupleStore, slot);
+
+ cnt =
Re: FETCH FIRST clause PERCENT option
On Thu, Feb 28, 2019 at 11:16 PM Tomas Vondra wrote: > > To give you a (admittedly, somewhat contrived and artificial example): > > SELECT * FROM t1 WHERE id IN ( > SELECT id FROM t2 ORDER BY x FETCH FIRST 10 PERCENT ROWS ONLY > ); > > Maybe this example is bogus and/or does not really matter in practice. I > don't know, but I've been unable to convince myself that's the case. does this means we abandon incremental approach? and am not sure of calculating percentage after OFFSET clause is acceptable or not regards Surafel
Re: Re: FETCH FIRST clause WITH TIES option
On Mon, Mar 25, 2019 at 11:56 AM David Steele wrote:
> This patch no longer passes testing so marked Waiting on Author.
>
>
Thank you for informing. Fixed
diff --git a/doc/src/sgml/ref/select.sgml b/doc/src/sgml/ref/select.sgml
index 06d611b64c..b3b045ea87 100644
--- a/doc/src/sgml/ref/select.sgml
+++ b/doc/src/sgml/ref/select.sgml
@@ -44,7 +44,7 @@ SELECT [ ALL | DISTINCT [ ON ( expressionexpression [ ASC | DESC | USING operator ] [ NULLS { FIRST | LAST } ] [, ...] ]
[ LIMIT { count | ALL } ]
[ OFFSET start [ ROW | ROWS ] ]
-[ FETCH { FIRST | NEXT } [ count ] { ROW | ROWS } ONLY ]
+[ FETCH { FIRST | NEXT } [ count ] { ROW | ROWS } [ ONLY | WITH TIES ] ]
[ FOR { UPDATE | NO KEY UPDATE | SHARE | KEY SHARE } [ OF table_name [, ...] ] [ NOWAIT | SKIP LOCKED ] [...] ]
where from_item can be one of:
@@ -1430,7 +1430,7 @@ OFFSET start
which PostgreSQL also supports. It is:
OFFSET start { ROW | ROWS }
-FETCH { FIRST | NEXT } [ count ] { ROW | ROWS } ONLY
+FETCH { FIRST | NEXT } [ count ] { ROW | ROWS } [ ONLY | WITH TIES ]
In this syntax, the start
or count value is required by
@@ -1440,7 +1440,10 @@ FETCH { FIRST | NEXT } [ count ] {
ambiguity.
If count is
omitted in a FETCH clause, it defaults to 1.
-ROW
+ROW .
+WITH TIES option is used to return two or more rows
+that tie for last place in the limit results set according to ORDER BY
+clause (ORDER BY clause must be specified in this case).
and ROWS as well as FIRST
and NEXT are noise words that don't influence
the effects of these clauses.
diff --git a/src/backend/executor/nodeLimit.c b/src/backend/executor/nodeLimit.c
index baa669abe8..e8aed10177 100644
--- a/src/backend/executor/nodeLimit.c
+++ b/src/backend/executor/nodeLimit.c
@@ -41,6 +41,7 @@ static TupleTableSlot * /* return: a tuple or NULL */
ExecLimit(PlanState *pstate)
{
LimitState *node = castNode(LimitState, pstate);
+ ExprContext *econtext = node->ps.ps_ExprContext;
ScanDirection direction;
TupleTableSlot *slot;
PlanState *outerPlan;
@@ -131,7 +132,8 @@ ExecLimit(PlanState *pstate)
* the state machine state to record having done so.
*/
if (!node->noCount &&
- node->position - node->offset >= node->count)
+ node->position - node->offset >= node->count &&
+ node->limitOption == WITH_ONLY)
{
node->lstate = LIMIT_WINDOWEND;
@@ -144,18 +146,64 @@ ExecLimit(PlanState *pstate)
return NULL;
}
+else if (!node->noCount &&
+ node->position - node->offset >= node->count &&
+ node->limitOption == WITH_TIES)
+{
+ /*
+ * Get next tuple from subplan, if any.
+ */
+ slot = ExecProcNode(outerPlan);
+ if (TupIsNull(slot))
+ {
+ node->lstate = LIMIT_SUBPLANEOF;
+ return NULL;
+ }
+ /*
+ * Test if the new tuple and the last tuple match.
+ * If so we return the tuple.
+ */
+ econtext->ecxt_innertuple = slot;
+ econtext->ecxt_outertuple = node->last_slot;
+ if (ExecQualAndReset(node->eqfunction, econtext))
+ {
+ ExecCopySlot(node->last_slot, slot);
+ node->subSlot = slot;
+ node->position++;
+ }
+ else
+ {
+ node->lstate = LIMIT_WINDOWEND;
+
+ /*
+ * If we know we won't need to back up, we can release
+ * resources at this point.
+ */
+ if (!(node->ps.state->es_top_eflags & EXEC_FLAG_BACKWARD))
+ (void) ExecShutdownNode(outerPlan);
-/*
- * Get next tuple from subplan, if any.
- */
-slot = ExecProcNode(outerPlan);
-if (TupIsNull(slot))
+ return NULL;
+ }
+
+}
+else
{
- node->lstate = LIMIT_SUBPLANEOF;
- return NULL;
+ /*
+ * Get next tuple from subplan, if any.
+ */
+ slot = ExecProcNode(outerPlan);
+ if (TupIsNull(slot))
+ {
+ node->lstate = LIMIT_SUBPLANEOF;
+ return NULL;
+ }
+ if (node->limitOption == WITH_TIES)
+ {
+ ExecCopySlot(node->last_slot, slot);
+ }
+ node->subSlot = slot;
+ node->position++;
}
-node->subSlot = slot;
-node->position++;
}
else
{
@@ -311,7 +359,8 @@ recompute_limits(LimitState *node)
* must update the child node anyway, in case this is a rescan and the
* previous time we got a different result.
*/
- ExecSetTupleBound(compute_tuples_needed(node), outerPlanState(node));
+ if(node->limitOption == WITH_ONLY)
+ ExecSetTupleBound(compute_tuples_needed(node), outerPlanState(node));
}
/*
@@ -374,6 +423,7 @@ ExecInitLimit(Limit *node, EState *estate, int eflags)
(PlanState *) limitstate);
limitstate->limitCount = ExecInitExpr((Expr *) node->limitCount,
(PlanState *) limitstate);
+ limitstate->limitOption = node->limitOption;
/*
* Initialize result type.
@@ -390,6 +440,25 @@ ExecInitLimit(Limit *node, EState *estate, int eflags)
*/
limitstate->ps.ps_ProjInfo = NULL;
+ /*
+ * Initialize
