Re: Differentiate MERGE queries with different structures

[bt22nakamorit]

2022-09-27 17:48 に Alvaro Herrera さんは書きました:

Pushed, thanks.

The code and the tests went fine on my environment.
Thank you Alvaro for your help, and thank you Julien for your review!

Best,
Tatsuhiro Nakamori

Re: Differentiate MERGE queries with different structures

[Alvaro Herrera]

Pushed, thanks.

-- 
Álvaro HerreraBreisgau, Deutschland  —  https://www.EnterpriseDB.com/
 It does it in a really, really complicated way
 why does it need to be complicated?
 Because it's MakeMaker.

Re: Differentiate MERGE queries with different structures

[Alvaro Herrera]

On 2022-Sep-26, Julien Rouhaud wrote:

> On Mon, Sep 26, 2022 at 03:12:46PM +0900, bt22nakamorit wrote:

> > I attached a patch file that adds information about MERGE queries on the
> > documentation of pg_stat_statements, and lines of code that helps with the
> > calculation of queryid hash value to differentiate MERGE queries.
> > Any kind of feedback is appreciated.
> 
> I didn't test the patch (and never looked at MERGE implementation either), but
> I'm wondering if MergeAction->matched and MergeAction->override should be
> jumbled too?

->matched distinguishes these two queries:

MERGE INTO foo USING bar ON (something)
  WHEN MATCHED THEN DO NOTHING;
MERGE INTO foo USING bar ON (something)
  WHEN NOT MATCHED THEN DO NOTHING;

because only DO NOTHING can be used with both MATCHED and NOT MATCHED,
though on the whole the distinction seems pointless.  However I think if
you sent both these queries and got a single pgss entry with the text of
one of them and not the other, you're going to be confused about where
the other went.   So +1 for jumbling it too.

->overriding is used in OVERRIDING SYSTEM VALUES (used for GENERATED
columns).  I don't think there's any case where it's interesting
currently: if you specify the column it's going to be in the column list
(which does affect the query ID).

> Also, the patch should contain some extra tests to fully cover MERGE
> jumbling.

Agreed.  I struggle to find a balance between not having anything and
going overboard, but I decided to add different for the different things
that should be jumbled, so that they would all appear in the view.


I moved the code around; instead of adding it at the end of the switch,
I did what the comment says, which is to mirror expression_tree_walker.
This made me realize that the latter is using the wrong order for fields
according to the struct definition, so I flipped that also.

-- 
Álvaro Herrera   48°01'N 7°57'E  —  https://www.EnterpriseDB.com/
"La gente vulgar sólo piensa en pasar el tiempo;
el que tiene talento, en aprovecharlo"
>From b1aff66110ec54e4b58517289a18451906876ed0 Mon Sep 17 00:00:00 2001
From: Alvaro Herrera 
Date: Mon, 26 Sep 2022 13:27:24 +0200
Subject: [PATCH v2] Fix pg_stat_statements for MERGE

Author: Tatsu 
Reviewed-by: Julien Rouhaud 
Discussion: https://postgr.es/m/d87e391694db75a038abc3b259782...@oss.nttdata.com
---
 .../expected/pg_stat_statements.out   | 41 ++-
 .../sql/pg_stat_statements.sql| 22 ++
 doc/src/sgml/pgstatstatements.sgml|  4 +-
 src/backend/nodes/nodeFuncs.c |  4 +-
 src/backend/utils/misc/queryjumble.c  | 11 +
 5 files changed, 77 insertions(+), 5 deletions(-)

diff --git a/contrib/pg_stat_statements/expected/pg_stat_statements.out b/contrib/pg_stat_statements/expected/pg_stat_statements.out
index ff0166fb9d..9ac5c87c3a 100644
--- a/contrib/pg_stat_statements/expected/pg_stat_statements.out
+++ b/contrib/pg_stat_statements/expected/pg_stat_statements.out
@@ -222,12 +222,51 @@ SELECT * FROM test WHERE a IN (1, 2, 3, 4, 5);
  3 | c   
 (8 rows)
 
+-- MERGE
+MERGE INTO test USING test st ON (st.a = test.a AND st.a >= 4)
+ WHEN MATCHED THEN UPDATE SET b = st.b || st.a::text;
+MERGE INTO test USING test st ON (st.a = test.a AND st.a >= 4)
+ WHEN MATCHED THEN UPDATE SET b = test.b || st.a::text;
+MERGE INTO test USING test st ON (st.a = test.a AND st.a >= 4)
+ WHEN MATCHED AND length(st.b) > 1 THEN UPDATE SET b = test.b || st.a::text;
+MERGE INTO test USING test st ON (st.a = test.a)
+ WHEN NOT MATCHED THEN INSERT (a, b) VALUES (0, NULL);
+MERGE INTO test USING test st ON (st.a = test.a)
+ WHEN NOT MATCHED THEN INSERT VALUES (0, NULL);	-- same as above
+MERGE INTO test USING test st ON (st.a = test.a)
+ WHEN NOT MATCHED THEN INSERT (b, a) VALUES (NULL, 0);
+MERGE INTO test USING test st ON (st.a = test.a)
+ WHEN NOT MATCHED THEN INSERT (a) VALUES (0);
+MERGE INTO test USING test st ON (st.a = test.a AND st.a >= 4)
+ WHEN MATCHED THEN DELETE;
+MERGE INTO test USING test st ON (st.a = test.a AND st.a >= 4)
+ WHEN MATCHED THEN DO NOTHING;
+MERGE INTO test USING test st ON (st.a = test.a AND st.a >= 4)
+ WHEN NOT MATCHED THEN DO NOTHING;
 SELECT query, calls, rows FROM pg_stat_statements ORDER BY query COLLATE "C";
 query | calls | rows 
 --+---+--
  DELETE FROM test WHERE a > $1| 1 |1
  INSERT INTO test (a, b) VALUES ($1, $2), ($3, $4), ($5, $6)  | 1 |3
  INSERT INTO test VALUES(generate_series($1, $2), $3) | 1 |   10
+ MERGE INTO test USING test st ON (st.a = test.a AND st.a >= $1) +| 1 |6
+  WHEN MATCHED AND length(st.b) > $2 THEN UPDATE SET b = test.b || st.a::text |   | 
+ MERGE INTO test 

Re: Differentiate MERGE queries with different structures

[Julien Rouhaud]

Hi,

On Mon, Sep 26, 2022 at 03:12:46PM +0900, bt22nakamorit wrote:
>
> pg_stat_statements module distinguishes queries with different structures,
> but some visibly different MERGE queries were combined as one
> pg_stat_statements entry.
> For example,
> MERGE INTO test1 USING test2 ON test1.id = test2.id WHEN MATCHED THEN UPDATE
> var = 1;
> MERGE INTO test1 USING test2 ON test1.id = test2.id WHEN MATCHED THEN
> DELETE;
> These two queries have different command types after WHEN (UPDATE and
> DELETE), but they were regarded as one entry in pg_stat_statements.
> I think that they should be sampled as distinct queries.

Agreed.

> I attached a patch file that adds information about MERGE queries on the
> documentation of pg_stat_statements, and lines of code that helps with the
> calculation of queryid hash value to differentiate MERGE queries.
> Any kind of feedback is appreciated.

I didn't test the patch (and never looked at MERGE implementation either), but
I'm wondering if MergeAction->matched and MergeAction->override should be
jumbled too?

Also, the patch should contain some extra tests to fully cover MERGE jumbling.

Re: [PATCH] Allow queries in WHEN expression of FOR EACH STATEMENT triggers

[Robert Haas]

On Thu, Sep 23, 2021 at 5:34 AM Joe Wildish  wrote:
> Regarding the deparse-and-reparse --- if I understand correctly, the core 
> problem is that we have no way of going from a node tree to a string, such 
> that the string is guaranteed to have the same meaning as the node tree? (I 
> did try just now to produce such a scenario with the patch but I couldn't get 
> ruleutils to emit the wrong thing).  Moreover, we couldn't store the string 
> for use with SPI, as the string would be subject to trigger-time search path 
> lookups.  That pretty much rules out SPI for this then.  Do you have a 
> suggestion for an alternative? I guess it would be go to the planner/executor 
> directly with the node tree?

I think hoping that you can ever make deparse and reparse reliably
produce the same result is a hopeless endeavor. Tom mentioned hazards
related to ambiguous constructs, but there's also often the risk of
concurrent DDL. Commit 5f173040e324f6c2eebb90d86cf1b0cdb5890f0a is a
cautionary tale, demonstrating that you can't even count on
schema_name.table_name to resolve to the same OID for the entire
duration of a single DDL command. The same hazard exists for
functions, operators, and anything else that gets looked up in a
system catalog.

I don't know what all of that means for your patch, but just wanted to
get my $0.02 in on the general topic.

-- 
Robert Haas
EDB: http://www.enterprisedb.com

Re: Re: [PATCH] Allow queries in WHEN expression of FOR EACH STATEMENT triggers

[Tom Lane]

Joe Wildish"  writes:
> On Wed, 22 Sep 2021, at 17:09, Tom Lane wrote:
> The main change is a switch to using SPI for expression evaluation.  The 
> plans are also cached along the same lines as the RI trigger plans.
>> 
>> I really dislike this implementation technique.  Aside from the likely
>> performance hit for existing triggers, I think it opens serious security
>> holes, because we can't fully guarantee that deparse-and-reparse doesn't
>> change the semantics.

> Where do you consider the performance hit to be?

The deparse/reparse cost might not be negligible, and putting SPI into
the equation where it was not before is certainly going to add overhead.
Now maybe those things are negligible in context, but I wouldn't believe
it without seeing some performance numbers.

> Do you have a suggestion for an alternative? I guess it would be go to the 
> planner/executor directly with the node tree?

What I'd be thinking about is what it'd take to extend expression_planner
and related infrastructure to allow expressions containing SubLinks.
I fear there are a lot of moving parts there though, since the restriction
has been in place so long.

>> (Relevant to that, I wonder why this patch is only concerned with
>> WHEN clauses and not all the other places where we forbid subqueries
>> for implementation simplicity.)

> I don't know how many other places WHEN clauses are used. Rules, perhaps?

I'm thinking of things like CHECK constraints.  Grepping for calls to
expression_planner would give you a clearer idea of the scope.

> The short answer is this patch was written to solve a specific problem I had 
> rather than it being a more general attempt to remove all "subquery 
> forbidden" restrictions.

I won't carp too much if the initial patch only removes the restriction
for WHEN; but I'd like to see it lay the groundwork to remove the
restriction elsewhere as well.

regards, tom lane

Re: [PATCH] Allow queries in WHEN expression of FOR EACH STATEMENT triggers

[Joe Wildish]

Hi Tom,

On Wed, 22 Sep 2021, at 17:09, Tom Lane wrote:
> > The main change is a switch to using SPI for expression evaluation.  The 
> > plans are also cached along the same lines as the RI trigger plans.
> 
> I really dislike this implementation technique.  Aside from the likely
> performance hit for existing triggers, I think it opens serious security
> holes, because we can't fully guarantee that deparse-and-reparse doesn't
> change the semantics.  For comparison, the RI trigger code goes to
> ridiculous lengths to force exact parsing of the queries it makes,
> and succeeds only because it needs just a very stylized subset of SQL.
> With a generic user-written expression, we'd be at risk for several
> inherently-ambiguous SQL constructs such as IS DISTINCT FROM (see
> relevant reading at [1]).

Where do you consider the performance hit to be? I just read the code again. 
The only time the new code paths are hit are when a FOR EACH STATEMENT trigger 
fires that has a WHEN condition. Given the existing restrictions for such a 
scenario, that can only mean a WHEN condition that is a simple function call; 
so, "SELECT foo()" vs "foo()"? Or have I misunderstood?

Regarding the deparse-and-reparse --- if I understand correctly, the core 
problem is that we have no way of going from a node tree to a string, such that 
the string is guaranteed to have the same meaning as the node tree? (I did try 
just now to produce such a scenario with the patch but I couldn't get ruleutils 
to emit the wrong thing).  Moreover, we couldn't store the string for use with 
SPI, as the string would be subject to trigger-time search path lookups.  That 
pretty much rules out SPI for this then.  Do you have a suggestion for an 
alternative? I guess it would be go to the planner/executor directly with the 
node tree?

>  In general, users may expect that
> once those are parsed by the accepting DDL command, they'll hold still,
> not get re-interpreted at runtime.

I agree entirely. I wasn't aware of the deparsing/reparsing problem.

> ...
> (Relevant to that, I wonder why this patch is only concerned with
> WHEN clauses and not all the other places where we forbid subqueries
> for implementation simplicity.)

I don't know how many other places WHEN clauses are used. Rules, perhaps? The 
short answer is this patch was written to solve a specific problem I had rather 
than it being a more general attempt to remove all "subquery forbidden" 
restrictions.

> 
> > b. If a WHEN expression is defined as "n = (SELECT ...)", there is the 
> > possibility that a user gets the error "more than one row returned by a 
> > subquery used as an expression" when performing DML, which would be rather 
> > cryptic if they didn't know there was a trigger involved.  To avoid this, 
> > we could disallow scalar expressions, with a hint to use the ANY/ALL 
> > quantifiers.
> 
> How is that any more cryptic than any other error that can occur
> in a WHEN expression?

It isn't.  What *is* different about it, is that -- AFAIK -- it is the only 
cryptic message that can come about due to the syntactic structure of an 
expression.  Yes, someone could have a function that does "RAISE ERROR 'foo'".  
There's not a lot that can be done about that.  But scalar subqueries are 
detectable and they have an obvious rewrite using the quantifiers, hence the 
suggestion. However, it was just that; a suggestion.

-Joe

Re: Re: [PATCH] Allow queries in WHEN expression of FOR EACH STATEMENT triggers

[Tom Lane]

"Joe Wildish"  writes:
> The main change is a switch to using SPI for expression evaluation.  The 
> plans are also cached along the same lines as the RI trigger plans.

I really dislike this implementation technique.  Aside from the likely
performance hit for existing triggers, I think it opens serious security
holes, because we can't fully guarantee that deparse-and-reparse doesn't
change the semantics.  For comparison, the RI trigger code goes to
ridiculous lengths to force exact parsing of the queries it makes,
and succeeds only because it needs just a very stylized subset of SQL.
With a generic user-written expression, we'd be at risk for several
inherently-ambiguous SQL constructs such as IS DISTINCT FROM (see
relevant reading at [1]).

You could argue that the same hazards apply if the user writes the same
query within the body of the trigger, and you'd have a point.  But
we've made a policy decision that users are on the hook to write their
functions securely.  No such decision has ever been taken with respect
to pre-parsed expression trees.  In general, users may expect that
once those are parsed by the accepting DDL command, they'll hold still,
not get re-interpreted at runtime.

> a. I originally disallowed functions and table-valued functions from 
> appearing in the expression as they could potentially do anything and 
> everything.  However, I noticed that we allow functions in FOR EACH ROW 
> triggers so we are already in that position.  Do we want to continue allowing 
> that in FOR EACH STATEMENT triggers?  If so, then the choice to restrict the 
> expression to just OLD, NEW and the table being triggered against might be 
> wrong.

Meh ... users have always been able to write CHECK constraints, WHEN
clauses, etc, that have side-effects --- they just have to bury that
inside a function.  It's only their own good taste and the lack of
predictability of when the side-effects will happen that stop them.
I don't see much point in enforcing restrictions that are easily
evaded by making a function.

(Relevant to that, I wonder why this patch is only concerned with
WHEN clauses and not all the other places where we forbid subqueries
for implementation simplicity.)

> b. If a WHEN expression is defined as "n = (SELECT ...)", there is the 
> possibility that a user gets the error "more than one row returned by a 
> subquery used as an expression" when performing DML, which would be rather 
> cryptic if they didn't know there was a trigger involved.  To avoid this, we 
> could disallow scalar expressions, with a hint to use the ANY/ALL quantifiers.

How is that any more cryptic than any other error that can occur
in a WHEN expression?

regards, tom lane

[1] https://www.postgresql.org/message-id/10492.1531515255%40sss.pgh.pa.us

Re: 回复：Queries that should be canceled will get stuck on secure_write function

[Fujii Masao]

On 2021/09/22 1:14, 蔡梦娟(玊于) wrote:

Hi all, I want to know your opinion on this patch, or in what way do you think 
we should solve this problem?


I agree that something like the patch (i.e., introduction of promotion
from cancel request to terminate one) is necessary for the fix. One concern
on the patch is that the cancel request can be promoted to the terminate one
even when secure_write() doesn't actually get stuck. Is this acceptable?
Maybe I'm tempted to set up the duration until the promotion happens
Or we should introduce the dedicated timer for communication on the socket?

Regards,

--
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION

Re: [PATCH] Allow queries in WHEN expression of FOR EACH STATEMENT triggers

[Joe Wildish]

Hi Hackers,

Attached is a new version of this patch. I resurrected it after removing it 
from the commitfest last year; I'll add it back in to the next CF.

The main change is a switch to using SPI for expression evaluation.  The plans 
are also cached along the same lines as the RI trigger plans.

Some random thoughts on the allowable expressions:

a. I originally disallowed functions and table-valued functions from appearing 
in the expression as they could potentially do anything and everything.  
However, I noticed that we allow functions in FOR EACH ROW triggers so we are 
already in that position.  Do we want to continue allowing that in FOR EACH 
STATEMENT triggers?  If so, then the choice to restrict the expression to just 
OLD, NEW and the table being triggered against might be wrong.

b. If a WHEN expression is defined as "n = (SELECT ...)", there is the 
possibility that a user gets the error "more than one row returned by a 
subquery used as an expression" when performing DML, which would be rather 
cryptic if they didn't know there was a trigger involved.  To avoid this, we 
could disallow scalar expressions, with a hint to use the ANY/ALL quantifiers.

-Joe

From 32cc660e51dc8a157e98cf3f1862fc149b4f68ea Mon Sep 17 00:00:00 2001
From: Joe Wildish 
Date: Wed, 2 Jun 2021 12:48:34 +0100
Subject: [PATCH] Allow queries in WHEN expression of FOR EACH STATEMENT
 triggers

Adds support to the trigger system to allow queries in the WHEN condition
of FOR EACH STATEMENT triggers. The expression can contain references to
the transition tables NEW and OLD, as well as the table which the
trigger is attached to, but other table references are disallowed.
---
 doc/src/sgml/ref/create_trigger.sgml   |  45 +-
 doc/src/sgml/trigger.sgml  |   7 +-
 src/backend/commands/tablecmds.c   |   2 +
 src/backend/commands/trigger.c | 852 -
 src/backend/parser/parse_expr.c|   4 +-
 src/backend/utils/adt/ruleutils.c  | 100 +--
 src/include/nodes/execnodes.h  |   2 +-
 src/include/utils/reltrigger.h |   1 +
 src/test/regress/expected/triggers.out |  66 +-
 src/test/regress/sql/triggers.sql  |  57 ++
 10 files changed, 898 insertions(+), 238 deletions(-)

diff --git a/doc/src/sgml/ref/create_trigger.sgml b/doc/src/sgml/ref/create_trigger.sgml
index 561af989a4..47f9a65fe4 100644
--- a/doc/src/sgml/ref/create_trigger.sgml
+++ b/doc/src/sgml/ref/create_trigger.sgml
@@ -160,13 +160,14 @@ CREATE [ OR REPLACE ] [ CONSTRAINT ] TRIGGER name
   
 
   
-   Also, a trigger definition can specify a Boolean WHEN
-   condition, which will be tested to see whether the trigger should
+   A trigger definition can specify a Boolean WHEN
+   condition which will be tested to see whether the trigger should
be fired.  In row-level triggers the WHEN condition can
-   examine the old and/or new values of columns of the row.  Statement-level
-   triggers can also have WHEN conditions, although the feature
-   is not so useful for them since the condition cannot refer to any values
-   in the table.
+   examine the old and/or new values of the columns of each row  which the
+   statement affects.  Statement-level triggers can also have
+   WHEN conditions, and are able to examine old and/or new
+   transition relations, that comprise of all rows either deleted or inserted
+   respectively by the triggering statement.
   
 
   
@@ -375,23 +376,41 @@ UPDATE OF column_name1 [, column_name2WHEN is specified, the
   function will only be called if the condition returns true.
-  In FOR EACH ROW triggers, the WHEN
+ 
+
+ 
+  In FOR EACH ROW triggers the WHEN
   condition can refer to columns of the old and/or new row values
   by writing OLD.column_name or
   NEW.column_name respectively.
-  Of course, INSERT triggers cannot refer to OLD
-  and DELETE triggers cannot refer to NEW.
+  The WHEN expression of a FOR EACH ROW
+  trigger cannot contain a subquery.
  
 
- INSTEAD OF triggers do not support WHEN
-  conditions.
+ 
+  In FOR EACH STATEMENT triggers the
+  WHEN condition can refer to the transition relations
+  OLD and NEW, and the relation that the
+  trigger is for. No other relations can be referenced. As OLD
+  and NEW are relations rather than row values, a
+  condition will typically comprise of
+  subquery expressions defined over those relations. Refer to
+   for subquery expression examples.
  
 
  
-  Currently, WHEN expressions cannot contain
-  subqueries.
+   In both FOR EACH ROW and FOR EACH STATEMENT
+   triggers, INSERT triggers cannot refer to OLD
+   row values or transition tables, and DELETE triggers cannot refer
+   to NEW row values or transition tables. However,
+   UPDATE triggers are able to refer to both OLD
+   and NEW
+ 
+
+ INSTEAD OF triggers do not support WHEN
+  conditions.
  
 
  

Re: websearch_to_tsquery() returns queries that don't match to_tsvector()

[Alexander Korotkov]

On Sun, May 2, 2021 at 9:37 PM Tom Lane  wrote:
> Alexander Korotkov  writes:
> > On Sun, May 2, 2021 at 9:04 PM Tom Lane  wrote:
> >> -   state.in_quotes = false;
> >>
> >> This change seems wrong/unsafe too.
>
> > It seems OK, because this patch removes in_quotes field altogether.
>
> Oh, sorry, I misread the patch --- I thought that earlier hunk
> was removing a local variable.  Agreed, if you can do without this
> state field altogether, that's fine.

OK, thank you for review!

--
Regards,
Alexander Korotkov

Re: websearch_to_tsquery() returns queries that don't match to_tsvector()

[Tom Lane]

Alexander Korotkov  writes:
> On Sun, May 2, 2021 at 9:04 PM Tom Lane  wrote:
>> -   state.in_quotes = false;
>> 
>> This change seems wrong/unsafe too.

> It seems OK, because this patch removes in_quotes field altogether.

Oh, sorry, I misread the patch --- I thought that earlier hunk
was removing a local variable.  Agreed, if you can do without this
state field altogether, that's fine.

regards, tom lane

Re: websearch_to_tsquery() returns queries that don't match to_tsvector()

[Alexander Korotkov]

On Sun, May 2, 2021 at 9:17 PM Zhihong Yu  wrote:
> One minor comment:
> +   /* iterate to the closing quotes or end of the string*/
>
> closing quotes -> closing quote

Yep, I've missed the third place to change from plural to single form :)

--
Regards,
Alexander Korotkov


0001-Make-websearch_to_tsquery-parse-text-in-quotes-as-v4.patch
Description: Binary data

Re: websearch_to_tsquery() returns queries that don't match to_tsvector()

[Zhihong Yu]

On Sun, May 2, 2021 at 11:12 AM Alexander Korotkov 
wrote:

> On Sun, May 2, 2021 at 9:06 PM Zhihong Yu  wrote:
> > +   /* Everything is quotes is processed as a single
> token */
> >
> > is quotes -> in quotes
> >
> > +   /* iterate to the closing quotes or end of the
> string*/
> >
> > closing quotes -> closing quote
> >
> > +   /* or else ƒtsvector() will raise an error */
> >
> > The character before tsvector() seems to be special.
>
> Thank you for catching.  Fixed in v3.
>
> --
> Regards,
> Alexander Korotkov
>

Hi,
One minor comment:
+   /* iterate to the closing quotes or end of the string*/

closing quotes -> closing quote

Cheers

Re: websearch_to_tsquery() returns queries that don't match to_tsvector()

[Alexander Korotkov]

On Sun, May 2, 2021 at 9:06 PM Zhihong Yu  wrote:
> +   /* Everything is quotes is processed as a single token */
>
> is quotes -> in quotes
>
> +   /* iterate to the closing quotes or end of the string*/
>
> closing quotes -> closing quote
>
> +   /* or else ƒtsvector() will raise an error */
>
> The character before tsvector() seems to be special.

Thank you for catching.  Fixed in v3.

--
Regards,
Alexander Korotkov

Re: websearch_to_tsquery() returns queries that don't match to_tsvector()

[Alexander Korotkov]

On Sun, May 2, 2021 at 9:04 PM Tom Lane  wrote:
> Alexander Korotkov  writes:
> > Ooops, I've included this by oversight.  The next revision is attached.
> > Anything besides that?
>
> Some quick eyeball review:
>
> +/* Everything is quotes is processed as a single token */
>
> Should read "Everything in quotes ..."
>
> -/* or else gettoken_tsvector() will raise an error */
> +/* or else ƒtsvector() will raise an error */
>
> Looks like an unintentional change?

Thank you for catching this!

> @@ -846,7 +812,6 @@ parse_tsquery(char *buf,
> state.buffer = buf;
> state.buf = buf;
> state.count = 0;
> -   state.in_quotes = false;
> state.state = WAITFIRSTOPERAND;
> state.polstr = NIL;
>
> This change seems wrong/unsafe too.

It seems OK, because this patch removes in_quotes field altogether.
We don't have to know whether we in quotes in the state, since we
process everything in quotes as a single token.

--
Regards,
Alexander Korotkov


0001-Make-websearch_to_tsquery-parse-text-in-quotes-as-v3.patch
Description: Binary data

Re: websearch_to_tsquery() returns queries that don't match to_tsvector()

[Zhihong Yu]

On Sun, May 2, 2021 at 10:57 AM Alexander Korotkov 
wrote:

> On Sun, May 2, 2021 at 8:52 PM Tom Lane  wrote:
> > Alexander Korotkov  writes:
> > > It seems there is another bug with phrase search and query parsing.
> > > It seems to me that since 0c4f355c6a websearch_to_tsquery() should
> > > just parse text in quotes as a single token.  Besides fixing this bug,
> > > it simplifies the code.
> >
> > OK ...
> >
> > > Trying to fix this bug before 0c4f355c6a doesn't seem to worth the
> efforts.
> >
> > Agreed, plus it doesn't sound like the sort of behavior change that
> > we want to push out in minor releases.
>
> +1
>
> > > I propose to push the attached patch to v14.  Objections?
> >
> > This patch seems to include some unrelated fooling around in GiST?
>
> Ooops, I've included this by oversight.  The next revision is attached.
>
> Anything besides that?
>
> --
> Regards,
> Alexander Korotkov
>

Hi,
+   /* Everything is quotes is processed as a single token
*/

is quotes -> in quotes

+   /* iterate to the closing quotes or end of the string*/

closing quotes -> closing quote

+   /* or else ƒtsvector() will raise an error */

The character before tsvector() seems to be special.

Cheers

Re: websearch_to_tsquery() returns queries that don't match to_tsvector()

[Tom Lane]

Alexander Korotkov  writes:
> Ooops, I've included this by oversight.  The next revision is attached.
> Anything besides that?

Some quick eyeball review:

+/* Everything is quotes is processed as a single token */

Should read "Everything in quotes ..."

-/* or else gettoken_tsvector() will raise an error */
+/* or else ƒtsvector() will raise an error */

Looks like an unintentional change?

@@ -846,7 +812,6 @@ parse_tsquery(char *buf,
state.buffer = buf;
state.buf = buf;
state.count = 0;
-   state.in_quotes = false;
state.state = WAITFIRSTOPERAND;
state.polstr = NIL;

This change seems wrong/unsafe too.

regards, tom lane

Re: websearch_to_tsquery() returns queries that don't match to_tsvector()

[Alexander Korotkov]

On Sun, May 2, 2021 at 8:52 PM Tom Lane  wrote:
> Alexander Korotkov  writes:
> > It seems there is another bug with phrase search and query parsing.
> > It seems to me that since 0c4f355c6a websearch_to_tsquery() should
> > just parse text in quotes as a single token.  Besides fixing this bug,
> > it simplifies the code.
>
> OK ...
>
> > Trying to fix this bug before 0c4f355c6a doesn't seem to worth the efforts.
>
> Agreed, plus it doesn't sound like the sort of behavior change that
> we want to push out in minor releases.

+1

> > I propose to push the attached patch to v14.  Objections?
>
> This patch seems to include some unrelated fooling around in GiST?

Ooops, I've included this by oversight.  The next revision is attached.

Anything besides that?

--
Regards,
Alexander Korotkov


0001-Make-websearch_to_tsquery-parse-text-in-quotes-as-v2.patch
Description: Binary data

Re: websearch_to_tsquery() returns queries that don't match to_tsvector()

[Tom Lane]

Alexander Korotkov  writes:
> It seems there is another bug with phrase search and query parsing.
> It seems to me that since 0c4f355c6a websearch_to_tsquery() should
> just parse text in quotes as a single token.  Besides fixing this bug,
> it simplifies the code.

OK ...

> Trying to fix this bug before 0c4f355c6a doesn't seem to worth the efforts.

Agreed, plus it doesn't sound like the sort of behavior change that
we want to push out in minor releases.

> I propose to push the attached patch to v14.  Objections?

This patch seems to include some unrelated fooling around in GiST?

regards, tom lane

Re: websearch_to_tsquery() returns queries that don't match to_tsvector()

[Alexander Korotkov]

Hi!

On Mon, Apr 19, 2021 at 9:57 AM Valentin Gatien-Baron
 wrote:
> Looking at the tsvector and tsquery, we can see that the problem is
> that the ":" counts as one position for the ts_query but not the
> ts_vector:
>
> select to_tsvector('english', 'aaa: bbb'), websearch_to_tsquery('english', 
> '"aaa: bbb"');
>to_tsvector   | websearch_to_tsquery
> -+--
>  'aaa':1 'bbb':2 | 'aaa' <2> 'bbb'
> (1 row)

It seems there is another bug with phrase search and query parsing.
It seems to me that since 0c4f355c6a websearch_to_tsquery() should
just parse text in quotes as a single token.  Besides fixing this bug,
it simplifies the code.

Trying to fix this bug before 0c4f355c6a doesn't seem to worth the efforts.

I propose to push the attached patch to v14.  Objections?

--
Regards,
Alexander Korotkov


0001-Make-websearch_to_tsquery-parse-text-in-quotes-as-a-.patch
Description: Binary data

Re: [PATCH] Allow queries in WHEN expression of FOR EACH STATEMENT triggers

[Joe Wildish]

Hi Surafel,

On 3 Sep 2020, at 19:22, Surafel Temesgen wrote:


This is my review of your patch


Thanks for the review.


subqueries in row trigger's is not supported in your patch so the the
documentation have to reflect it


It is still the case that the documentation says this. But, that may 
have been unclear as the documentation wouldn't compile (as you noted), 
so it wasn't possible to read it in the rendered form.




+ 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


Fixed.

I've also amended the server programming section to accurately reflect 
how WHEN conditions can be used.


Instead of planning every time the trigger fire I suggest to store 
plan or

prepared statement node so planning time can be saved


Yes, that would make sense. I'll look in to what needs to be done.

Do you know if there are other areas of the code that cache plans that 
could act as a guide as to how best to achieve it?



There are server crash on the following sequence of command

...

INSERT INTO main_table (a, b) VALUES

(101, 498),

(102, 499);

server crashed


Thanks. It was an incorrect Assert about NULL returns. Fixed.

-Joe

From 56d010c925db41ffe689044ba215640600976748 Mon Sep 17 00:00:00 2001
From: Joe Wildish 
Date: Wed, 30 Dec 2020 19:20:10 +
Subject: [PATCH] Allow queries in WHEN expression of FOR EACH STATEMENT
 triggers

Adds support to the trigger system to allow queries in the WHEN condition
of FOR EACH STATEMENT triggers. The expression can contain references to
the transition tables NEW and OLD, as well as the table which the
trigger is attached to, but other table references are disallowed.
---
 doc/src/sgml/ref/create_trigger.sgml   |  45 +-
 doc/src/sgml/trigger.sgml  |   7 +-
 src/backend/commands/trigger.c | 597 -
 src/backend/parser/parse_expr.c|   4 +-
 src/backend/utils/adt/ruleutils.c  |  94 ++--
 src/include/nodes/execnodes.h  |   2 +-
 src/test/regress/expected/triggers.out |  73 ++-
 src/test/regress/sql/triggers.sql  |  63 +++
 8 files changed, 699 insertions(+), 186 deletions(-)

diff --git a/doc/src/sgml/ref/create_trigger.sgml 
b/doc/src/sgml/ref/create_trigger.sgml
index 561af989a4..47f9a65fe4 100644
--- a/doc/src/sgml/ref/create_trigger.sgml
+++ b/doc/src/sgml/ref/create_trigger.sgml
@@ -160,13 +160,14 @@ CREATE [ OR REPLACE ] [ CONSTRAINT ] TRIGGER name
   
 
   
-   Also, a trigger definition can specify a Boolean WHEN
-   condition, which will be tested to see whether the trigger should
+   A trigger definition can specify a Boolean WHEN
+   condition which will be tested to see whether the trigger should
be fired.  In row-level triggers the WHEN condition can
-   examine the old and/or new values of columns of the row.  Statement-level
-   triggers can also have WHEN conditions, although the 
feature
-   is not so useful for them since the condition cannot refer to any values
-   in the table.
+   examine the old and/or new values of the columns of each row  which the
+   statement affects.  Statement-level triggers can also have
+   WHEN conditions, and are able to examine old and/or new
+   transition relations, that comprise of all rows either deleted or inserted
+   respectively by the triggering statement.
   
 
   
@@ -375,23 +376,41 @@ UPDATE OF column_name1 [, 
column_name2WHEN is specified, the
   function will only be called if the condition returns 
true.
-  In FOR EACH ROW triggers, the WHEN
+ 
+
+ 
+  In FOR EACH ROW triggers the WHEN
   condition can refer to columns of the old and/or new row values
   by writing OLD.column_name or
   NEW.column_name respectively.
-  Of course, INSERT triggers cannot refer to 
OLD
-  and DELETE triggers cannot refer to 
NEW.
+  The WHEN expression of a FOR EACH 
ROW
+  trigger cannot contain a subquery.
  
 
- INSTEAD OF triggers do not support 
WHEN
-  conditions.
+ 
+  In FOR EACH STATEMENT triggers the
+  WHEN condition can refer to the transition relations
+  OLD and NEW, and the relation that 
the
+  trigger is for. No other relations can be referenced. As 
OLD
+  and NEW are relations rather than row values, a
+  condition will typically 
comprise of
+  subquery expressions defined over those relations. Refer to
+   for subquery expression examples.
  
 
  
-  Currently, WHEN expressions cannot contain
-  subqueries.
+   In both FOR EACH ROW and FOR EACH 
STATEMENT
+   triggers, INSERT triggers cannot refer to 
OLD
+   row values or transition tables, and DELETE triggers 
cannot refer
+   to NEW row values or transition tables. However,
+   UPDATE triggers are able to refer to both 
OLD
+   and NEW
+ 
+
+ INSTEAD OF triggers do not support 

Re: [PATCH] Allow queries in WHEN expression of FOR EACH STATEMENT triggers

[Michael Paquier]

On Thu, Sep 03, 2020 at 09:22:31PM +0300, Surafel Temesgen wrote:
> server crashed

That's a problem.  As this feedback has not been answered after two
weeks, I am marking the patch as returned with feedback.
--
Michael


signature.asc
Description: PGP signature

Re: [PATCH] Allow queries in WHEN expression of FOR EACH STATEMENT triggers

[Surafel Temesgen]

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: [PATCH] Allow queries in WHEN expression of FOR EACH STATEMENT triggers

[Daniel Gustafsson]

> On 17 Jul 2020, at 00:22, Joe Wildish  wrote:

> Attached is a patch for supporting queries in the WHEN expression of 
> statement triggers.at?

Hi!,

Please create an entry for this patch in the 2020-09 commitfest to make sure
it's properly tracked:

https://commitfest.postgresql.org/29/

cheers ./daniel

Re: [Proposal] Arbitrary queries in postgres_fdw

[rtorre]

On Tue, Nov 5, 2019 at 7:41 PM David Fetter  wrote:
> Could you use IMPORT FOREIGN SCHEMA for that? I seem to recall that
> I've managed to import information_schema successfully.

Yes, I tried it and I can import and operate on the
information_schema, which actually covers part of my needs. It does so
at the expense of polluting the catalog with foreign tables, but I can
live with it. Thanks for pointing out.

There are other cases that can be covered with either this proposal or
CREATE ROUTINE MAPPING, but not with the current state of things (as
far as I know). E.g: calling version() or postgis_version() on the
foreign end.

It's largely a matter of convenience vs development effort. That said,
I understand this may not make the design quality cut.

Regards
-Rafa

Re: [Proposal] Arbitrary queries in postgres_fdw

[David Fetter]

On Tue, Nov 05, 2019 at 11:09:34AM +0100, rto...@carto.com wrote:
> On Sun, Oct 27, 2019 at 7:07 PM David Fetter  wrote:
> > There's a SQL MED standard feature for CREATE ROUTINE MAPPING that
> > does something similar to this.  Might it be possible to incorporate
> > it into the previous patch that implemented that feature?
> 
> Supporting CREATE ROUTINE MAPPING goes a level beyond
> postgres_fdw. It'd require adding new DDL syntax elements to the
> parser, catalog and callbacks for the FDW's to support them.
> 
> For the record, there's a very interesting thread on this topic (that
> you participated in):
> https://www.postgresql.org/message-id/flat/CADkLM%3DdK0dmkzLhaLPpnjN2wBF5GRpvzOr%3DeW0EWdCnG-OHnpQ%40mail.gmail.com
> 
> I know they have different semantics and may turn more limiting, but
> for certain use cases, the `extensions` parameter of postgres_fdw may
> come in handy (shipping function calls to the foreign end from
> extensions present in both local and foreign).
> 
> For my use case, which is retrieving catalog info before any CREATE
> FOREIGN TABLE,

Could you use IMPORT FOREIGN SCHEMA for that? I seem to recall that
I've managed to import information_schema successfully.

Best,
David.
-- 
David Fetter  http://fetter.org/
Phone: +1 415 235 3778

Remember to vote!
Consider donating to Postgres: http://www.postgresql.org/about/donate

Re: [Proposal] Arbitrary queries in postgres_fdw

[rtorre]

> On Fri, Oct 25, 2019 at 12:38 PM Tom Lane  wrote:
> > end of things.  And allowing arbitrary queries to go over a postgres_fdw
> > connection would be absolutely disastrous from a debuggability and
> > maintainability standpoint, because they might change the remote
> > session's state in ways that postgres_fdw isn't prepared to handle.
> > (In a dblink connection, the remote session's state is the user's
> > responsibility to manage, but this isn't the case for postgres_fdw.)
> > So I think this proposal has to be firmly rejected.

On Mon, Oct 28, 2019 at 1:54 PM Robert Haas  wrote:
> I think the reduction in debuggability and maintainability has to be
> balanced against a possible significant gain in usability.  I mean,
> you could document that if the values of certain GUCs are changed, or
> if you create and drop prepared statements with certain names, it
> might cause queries in the same session issued through the regular
> foreign table API to produce wrong answers. That would still leave an
> enormous number of queries that users could issue with absolutely no
> problems.

I understand both points, the alternatives and the tradeoffs.

My motivations not use dblink are twofold: to purposefully reuse the
connection pool in postgres_fdw, and to avoid installing another
extension. I cannot speak to whether this can be advantageous to
others to accept the tradeoffs.

If you are still interested, I'm willing to listen to the feedback and
continue improving the patch. Otherwise we can settle it here and (of
course!) I won't take any offense because of that.

Find attached v2 of the patch with the following changes:
- added support for commands, as it failed upon PGRES_COMMAND_OK (with
  tests with prepared statements)
- documentation for the new function, with the mentioned caveats
- removed the test with the `SELECT current_user`, because it produced
  different results depending on the execution environment.

Regards
-Rafa
diff --git a/contrib/postgres_fdw/Makefile b/contrib/postgres_fdw/Makefile
index 85394b4f1f..85a4ecb900 100644
--- a/contrib/postgres_fdw/Makefile
+++ b/contrib/postgres_fdw/Makefile
@@ -8,7 +8,7 @@ PG_CPPFLAGS = -I$(libpq_srcdir)
 SHLIB_LINK_INTERNAL = $(libpq)
 
 EXTENSION = postgres_fdw
-DATA = postgres_fdw--1.0.sql
+DATA = postgres_fdw--1.0.sql postgres_fdw--1.0--1.1.sql postgres_fdw--1.1.sql
 
 REGRESS = postgres_fdw
 
diff --git a/contrib/postgres_fdw/expected/postgres_fdw.out b/contrib/postgres_fdw/expected/postgres_fdw.out
index 88dbaa2493..2219235731 100644
--- a/contrib/postgres_fdw/expected/postgres_fdw.out
+++ b/contrib/postgres_fdw/expected/postgres_fdw.out
@@ -8801,3 +8801,72 @@ SELECT b, avg(a), max(a), count(*) FROM pagg_tab GROUP BY b HAVING sum(a) < 700
 
 -- Clean-up
 RESET enable_partitionwise_aggregate;
+-- ===
+-- test postgres_fdw_query(server name, sql text)
+-- ===
+-- Most simple SELECT through postgres_fdw_query
+SELECT * FROM postgres_fdw_query('loopback', 'SELECT 42') AS t(i int);
+ i  
+
+ 42
+(1 row)
+
+-- Select schemas owned by the role configured in the user mapping
+SELECT * FROM postgres_fdw_query('loopback', $$SELECT s.nspname
+FROM pg_catalog.pg_namespace s
+JOIN pg_catalog.pg_user u ON u.usesysid = s.nspowner
+WHERE u.usename = current_user
+ORDER BY s.nspname$$
+) AS schemas(schema_name name);
+schema_name 
+
+ S 1
+ import_dest1
+ import_dest2
+ import_dest3
+ import_dest4
+ import_dest5
+ import_source
+ information_schema
+ pg_catalog
+ pg_temp_1
+ pg_toast
+ pg_toast_temp_1
+ public
+(13 rows)
+
+-- Select tables and views in a given foreign schema that the role
+-- configured in the user mapping has access to
+SELECT * FROM postgres_fdw_query('loopback', $$SELECT table_name, table_type
+FROM information_schema.tables
+WHERE table_schema = 'S 1'
+ORDER BY table_name$$
+) AS schemas(table_name text, table_type text);
+ table_name | table_type 
++
+ T 1| BASE TABLE
+ T 2| BASE TABLE
+ T 3| BASE TABLE
+ T 4| BASE TABLE
+(4 rows)
+
+-- Test we can send commands (e.g: prepared statements)
+SELECT * FROM postgres_fdw_query('loopback', $$PREPARE fooplan (int) AS
+SELECT $1 + 42$$) AS t(res text);
+   res   
+-
+ PREPARE
+(1 row)
+
+SELECT * FROM postgres_fdw_query('loopback', 'EXECUTE fooplan (1)') AS t(i int);
+ i  
+
+ 43
+(1 row)
+
+SELECT * FROM postgres_fdw_query('loopback', 'DEALLOCATE fooplan') AS t(res text);
+res 
+
+ DEALLOCATE
+(1 row)
+
diff --git a/contrib/postgres_fdw/postgres_fdw--1.0--1.1.sql b/contrib/postgres_fdw/postgres_fdw--1.0--1.1.sql
new file mode 100644
index 00..15a7c83519
--- /dev/null
+++ b/contrib/postgres_fdw/postgres_fdw--1.0--1.1.sql
@@ -0,0 +1,7 @@
+-- complain if script is sourced in psql, rather than via CREATE EXTENSION
+\echo Use 

Re: [Proposal] Arbitrary queries in postgres_fdw

[rtorre]

On Sun, Oct 27, 2019 at 7:07 PM David Fetter  wrote:
> There's a SQL MED standard feature for CREATE ROUTINE MAPPING that
> does something similar to this.  Might it be possible to incorporate
> it into the previous patch that implemented that feature?

Supporting CREATE ROUTINE MAPPING goes a level beyond
postgres_fdw. It'd require adding new DDL syntax elements to the
parser, catalog and callbacks for the FDW's to support them.

For the record, there's a very interesting thread on this topic (that
you participated in):
https://www.postgresql.org/message-id/flat/CADkLM%3DdK0dmkzLhaLPpnjN2wBF5GRpvzOr%3DeW0EWdCnG-OHnpQ%40mail.gmail.com

I know they have different semantics and may turn more limiting, but
for certain use cases, the `extensions` parameter of postgres_fdw may
come in handy (shipping function calls to the foreign end from
extensions present in both local and foreign).

For my use case, which is retrieving catalog info before any CREATE
FOREIGN TABLE, CREATE ROUTINE MAPPING is not really a good fit.

Thank you for pointing out anyway.
-Rafa

Re: [Proposal] Arbitrary queries in postgres_fdw

[Robert Haas]

On Fri, Oct 25, 2019 at 12:38 PM Tom Lane  wrote:
> end of things.  And allowing arbitrary queries to go over a postgres_fdw
> connection would be absolutely disastrous from a debuggability and
> maintainability standpoint, because they might change the remote
> session's state in ways that postgres_fdw isn't prepared to handle.
> (In a dblink connection, the remote session's state is the user's
> responsibility to manage, but this isn't the case for postgres_fdw.)
> So I think this proposal has to be firmly rejected.

I think the reduction in debuggability and maintainability has to be
balanced against a possible significant gain in usability.  I mean,
you could document that if the values of certain GUCs are changed, or
if you create and drop prepared statements with certain names, it
might cause queries in the same session issued through the regular
foreign table API to produce wrong answers. That would still leave an
enormous number of queries that users could issue with absolutely no
problems. I really don't see a bona fide maintainability problem here.
When someone produces a reproducible test case showing that they did
one of the things we told them not to do, then we'll tell them to read
the fine manual and move on.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Re: [Proposal] Arbitrary queries in postgres_fdw

[rtorre]

On Sun, Oct 27, 2019 at 7:07 PM David Fetter  wrote:
>
> There's a SQL MED standard feature for CREATE ROUTINE MAPPING that
> does something similar to this.  Might it be possible to incorporate
> it into the previous patch that implemented that feature?

Thanks for the idea, David. I'll investigate it and hopefully
come up with a more standard proposal.

Best regards
-Rafa

Re: [Proposal] Arbitrary queries in postgres_fdw

[David Fetter]

On Fri, Oct 25, 2019 at 05:17:18PM +0200, rto...@carto.com wrote:
> Dear all,
> 
> We stumbled upon a few cases in which retrieving information from the
> foreign server may turn pretty useful before creating any foreign
> table, especially info related to the catalog. E.g: a list of schemas
> or tables the user has access to.
> 
> I thought of using dblink for it, but that requires duplication of
> server and user mapping details and it adds its own management of
> connections.
> 
> Then I thought a better approach may be a mix of both: a function to
> issue arbitrary queries to the foreign server reusing all the details
> encapsulated in the server and user mapping. It would use the same
> pool of connections.

There's a SQL MED standard feature for CREATE ROUTINE MAPPING that
does something similar to this.  Might it be possible to incorporate
it into the previous patch that implemented that feature?

Best,
David.
-- 
David Fetter  http://fetter.org/
Phone: +1 415 235 3778

Remember to vote!
Consider donating to Postgres: http://www.postgresql.org/about/donate

Re: [Proposal] Arbitrary queries in postgres_fdw

[Tom Lane]

rto...@carto.com writes:
> We stumbled upon a few cases in which retrieving information from the
> foreign server may turn pretty useful before creating any foreign
> table, especially info related to the catalog. E.g: a list of schemas
> or tables the user has access to.

> I thought of using dblink for it, but that requires duplication of
> server and user mapping details and it adds its own management of
> connections.

> Then I thought a better approach may be a mix of both: a function to
> issue arbitrary queries to the foreign server reusing all the details
> encapsulated in the server and user mapping. It would use the same
> pool of connections.

dblink can already reference a postgres_fdw "server" for connection
details, so I think this problem is already solved from the usability
end of things.  And allowing arbitrary queries to go over a postgres_fdw
connection would be absolutely disastrous from a debuggability and
maintainability standpoint, because they might change the remote
session's state in ways that postgres_fdw isn't prepared to handle.
(In a dblink connection, the remote session's state is the user's
responsibility to manage, but this isn't the case for postgres_fdw.)
So I think this proposal has to be firmly rejected.

regards, tom lane

Re: Documenting that queries can be run over replication protocol

[Peter Eisentraut]

On 24/07/2018 09:54, Chris Travers wrote:
> In the process of building some tooling based on replication we
> discovered that PostgreSQL 10 uses COPY to populate the initial table on
> logical replication, see commit 7c4f52409a8c7d85ed169bbbc1f6092274d03920
> 
> This is not currently documented.  Attached is a patch which adds that
> documentation to the logical replication part of the protocol section.

It already says earlier in that same section:

"In logical replication walsender mode, the replication commands shown
below as well as normal SQL commands can be issued."

-- 
Peter Eisentraut  http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Re: very slow queries when max_parallel_workers_per_gather is higher than zero

[Guilherme Pereira]

cost=1489622.85..1489622.86 rows=1
> width=8) (actual time=9600.255..9600.255 rows=1 loops=3)
>->  Hash Join  (cost=27.44..1489550.83 rows=28808 width=0)
> (actual time=9600.249..9600.249 rows=0 loops=3)
>  Hash Cond: (f.dt_event_id = d.id)
>  ->  Parallel Seq Scan on
> f_ticketupdate_aad5jtwal0ayaax f  (cost=0.00..1185867.47 rows=24054847
> width=4) (actual time=0.076..4955.525 rows=19243863 loops=3)
>  ->  Hash  (cost=27.35..27.35 rows=7 width=4) (actual
> time=0.099..0.099 rows=7 loops=3)
>Buckets: 1024  Batches: 1  Memory Usage: 9kB
>->  Bitmap Heap Scan on
> dwh_dm_aabv5kk9rxac4lz_aaonw7nchsan2n1_aad8xhr0m_aaewg8j61iagl1z d
> (cost=4.34..27.35 rows=7 width=4) (actual time=0.045..0.085 rows=7 loops=3)
>  Recheck Cond: (6171 = id_euweek)
>  Heap Blocks: exact=7
>  ->  Bitmap Index Scan on
> dwh_dm_aabv5kk9rxac4lz_aaonw7nchsan2n1_aad8xhr0m_a_id_euweek_idx
> (cost=0.00..4.33 rows=7 width=0) (actual time=0.032..0.032 rows=7 loops=3)
>Index Cond: (6171 = id_euweek)
>  Planning time: 0.616 ms
>  Execution time: 9611.924 ms
> (17 rows)
>
> On Mon, Apr 16, 2018 at 4:53 PM Pavel Stehule <pavel.steh...@gmail.com>
> wrote:
>
>>
>> -- Forwarded message -
>> From: Tomas Vondra <tomas.von...@2ndquadrant.com>
>> Date: po 16. 4. 2018 16:14
>> Subject: Re: very slow queries when max_parallel_workers_per_gather is
>> higher than zero
>> To: Pavel Stehule <pavel.steh...@gmail.com>
>> Cc: PostgreSQL Hackers <pgsql-hack...@postgresql.org>
>>
>>
>> Apologies, the reduced query was missing a where condition on id_week:
>>
>> SELECT count(*)
>> FROM f_ticketupdate_aad5jtwal0ayaax AS f
>> INNER JOIN
>>   dwh_dm_aabv5kk9rxac4lz_aaonw7nchsan2n1_aad8xhr0m_aaewg8j61iagl1z AS d
>>   ON (f.dt_event_id = d.id)
>> WHERE ( 6171 = d."id_euweek" )
>>
>>
>> regards
>>
>>
>> --
>> Tomas Vondra  http://www.2ndQuadrant.com
>> PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
>>
>

Re: very slow queries when max_parallel_workers_per_gather is higher than zero

[Tomas Vondra]

Thanks. So we now have a trivial query demonstrating the issue. IMHO
this is not really a costing issue, but due to a misestimate.

Essentially, the problem is that the two sides of the join mismatch,
causing this:

->  Bitmap Heap Scan on dwh_dm ... d  (... rows=7 width=4) (...)

->  Bitmap Heap Scan on f_ticketupdate_aad5jtwal0ayaax f
  (cost=1633.90..214617.67 rows=87472 width=4)
  (actual time=0.003..0.003 rows=0 loops=7)
Recheck Cond: (dt_event_id = d.id)

->  Bitmap Index Scan on f_ticketupdate_aad5jtwal0ayaa ...
  (cost=0.00..1612.03 rows=87472 width=0)
  (actual time=0.003..0.003 rows=0 loops=7)
Index Cond: (dt_event_id = d.id)

I.e. the database believes the bitmap index scan will match 87k rows.
But in fact it matches 0, which makes the bitmap heap scan entirely
unnecessary (thus costing nothing, because it's skipped).

Of course, the parallel plan is structured slightly differently, and
does not allow this skipping because it places the f_ table on the outer
side of the join (and scans it using sequential scan).

Now, try changing the parameters (particularly id_euweek) so that the
bitmap index scan actually matches something. I'm pretty sure that will
make the non-parallel case much more expensive.

Increasing the parallel_setup_cost makes the parallel plan a bit more
expensive, enough to switch to the non-parallel plan. But that's mostly
a fluke and not particularly principled way to fix this - if the cost
difference gets a bit larger (or if you increase the number of parallel
workers) it's probably going to use the parallel plan again.

Obviously, PostgreSQL 9.5 doesn't have parallel queries, so it does not
have a chance of making this mistake.

regards
-- 
Tomas Vondra  http://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Re: very slow queries when max_parallel_workers_per_gather is higher than zero

[Guilherme Pereira]

Hope it's fine to jump in.

db_sq3rjf5b7p309lq9wuqrh3qhk4gy9fbw=#  set
max_parallel_workers_per_gather=0;
SET
db_sq3rjf5b7p309lq9wuqrh3qhk4gy9fbw=# explain analyze SELECT count(*)
FROM f_ticketupdate_aad5jtwal0ayaax AS f
INNER JOIN
  dwh_dm_aabv5kk9rxac4lz_aaonw7nchsan2n1_aad8xhr0m_aaewg8j61iagl1z AS d
  ON (f.dt_event_id = d.id)
WHERE ( 6171 = d."id_euweek" );

 QUERY PLAN
-
 Aggregate  (cost=1508646.93..1508646.94 rows=1 width=8) (actual
time=0.145..0.145 rows=1 loops=1)
   ->  Nested Loop  (cost=1638.24..1508474.08 rows=69140 width=0) (actual
time=0.142..0.142 rows=0 loops=1)
 ->  Bitmap Heap Scan on
dwh_dm_aabv5kk9rxac4lz_aaonw7nchsan2n1_aad8xhr0m_aaewg8j61iagl1z d
(cost=4.34..27.35 rows=7 width=4) (actual time=0.043..0.103 rows=7 loops=1)
   Recheck Cond: (6171 = id_euweek)
   Heap Blocks: exact=7
   ->  Bitmap Index Scan on
dwh_dm_aabv5kk9rxac4lz_aaonw7nchsan2n1_aad8xhr0m_a_id_euweek_idx
(cost=0.00..4.33 rows=7 width=0) (actual time=0.036..0.036 rows=7 loops=1)
 Index Cond: (6171 = id_euweek)
 ->  Bitmap Heap Scan on f_ticketupdate_aad5jtwal0ayaax f
(cost=1633.90..214617.67 rows=87472 width=4) (actual time=0.003..0.003
rows=0 loops=7)
   Recheck Cond: (dt_event_id = d.id)
   ->  Bitmap Index Scan on
f_ticketupdate_aad5jtwal0ayaax_dt_event_id_idx  (cost=0.00..1612.03
rows=87472 width=0) (actual time=0.003..0.003 rows=0 loops=7)
 Index Cond: (dt_event_id = d.id)
 Planning time: 0.496 ms
 Execution time: 0.227 ms
(13 rows)

db_sq3rjf5b7p309lq9wuqrh3qhk4gy9fbw=#  set
max_parallel_workers_per_gather=2;
SET
db_sq3rjf5b7p309lq9wuqrh3qhk4gy9fbw=# explain analyze SELECT count(*)
FROM f_ticketupdate_aad5jtwal0ayaax AS f
INNER JOIN
  dwh_dm_aabv5kk9rxac4lz_aaonw7nchsan2n1_aad8xhr0m_aaewg8j61iagl1z AS d
  ON (f.dt_event_id = d.id)
WHERE ( 6171 = d."id_euweek" );

  QUERY PLAN
---
 Finalize Aggregate  (cost=1490623.06..1490623.07 rows=1 width=8) (actual
time=9604.745..9604.745 rows=1 loops=1)
   ->  Gather  (cost=1490622.85..1490623.06 rows=2 width=8) (actual
time=9604.707..9604.739 rows=3 loops=1)
 Workers Planned: 2
 Workers Launched: 2
 ->  Partial Aggregate  (cost=1489622.85..1489622.86 rows=1
width=8) (actual time=9600.255..9600.255 rows=1 loops=3)
   ->  Hash Join  (cost=27.44..1489550.83 rows=28808 width=0)
(actual time=9600.249..9600.249 rows=0 loops=3)
 Hash Cond: (f.dt_event_id = d.id)
 ->  Parallel Seq Scan on
f_ticketupdate_aad5jtwal0ayaax f  (cost=0.00..1185867.47 rows=24054847
width=4) (actual time=0.076..4955.525 rows=19243863 loops=3)
 ->  Hash  (cost=27.35..27.35 rows=7 width=4) (actual
time=0.099..0.099 rows=7 loops=3)
   Buckets: 1024  Batches: 1  Memory Usage: 9kB
   ->  Bitmap Heap Scan on
dwh_dm_aabv5kk9rxac4lz_aaonw7nchsan2n1_aad8xhr0m_aaewg8j61iagl1z d
(cost=4.34..27.35 rows=7 width=4) (actual time=0.045..0.085 rows=7 loops=3)
 Recheck Cond: (6171 = id_euweek)
 Heap Blocks: exact=7
 ->  Bitmap Index Scan on
dwh_dm_aabv5kk9rxac4lz_aaonw7nchsan2n1_aad8xhr0m_a_id_euweek_idx
(cost=0.00..4.33 rows=7 width=0) (actual time=0.032..0.032 rows=7 loops=3)
   Index Cond: (6171 = id_euweek)
 Planning time: 0.616 ms
 Execution time: 9611.924 ms
(17 rows)

On Mon, Apr 16, 2018 at 4:53 PM Pavel Stehule <pavel.steh...@gmail.com>
wrote:

>
> -- Forwarded message -
> From: Tomas Vondra <tomas.von...@2ndquadrant.com>
> Date: po 16. 4. 2018 16:14
> Subject: Re: very slow queries when max_parallel_workers_per_gather is
> higher than zero
> To: Pavel Stehule <pavel.steh...@gmail.com>
> Cc: PostgreSQL Hackers <pgsql-hack...@postgresql.org>
>
>
> Apologies, the reduced query was missing a where condition on id_week:
>
> SELECT count(*)
> FROM f_ticketupdate_aad5jtwal0ayaax AS f
> INNER JOIN
>   dwh_dm_aabv5kk9rxac4lz_aaonw7nchsan2n1_aad8xhr0m_aaewg8j61iagl1z AS d
>   ON (f.dt_event_id = d.id)
> WHERE ( 6171 = d."id_euweek" )
>
>
> regards
>
>
> --
> Tomas Vondra  http://www.2ndQuadrant.com
> PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
>

Re: very slow queries when max_parallel_workers_per_gather is higher than zero

[Tomas Vondra]

Apologies, the reduced query was missing a where condition on id_week:

SELECT count(*)
FROM f_ticketupdate_aad5jtwal0ayaax AS f
INNER JOIN
  dwh_dm_aabv5kk9rxac4lz_aaonw7nchsan2n1_aad8xhr0m_aaewg8j61iagl1z AS d
  ON (f.dt_event_id = d.id)
WHERE ( 6171 = d."id_euweek" )


regards

-- 
Tomas Vondra  http://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Re: very slow queries when max_parallel_workers_per_gather is higher than zero

[Pavel Stehule]

2018-04-16 15:52 GMT+02:00 Tomas Vondra :

> > Query Performs nicely, but no parallel workers are used:
> > GroupAggregate  (cost=2611148.87..2611152.89 rows=31 width=22) (actual
> > time=0.084..0.084 rows=0 loops=1)
> >Group Key:
> > f_zendesktickets_aaeljtllr5at3el.cstm_custom_38746665_primary_column
> >->  Sort  (cost=2611148.87..2611149.11 rows=99 width=28) (actual
> > time=0.082..0.082 rows=0 loops=1)
> >  Sort Key:
> > f_zendesktickets_aaeljtllr5at3el.cstm_custom_38746665_primary_column
> >  Sort Method: quicksort  Memory: 25kB
> >  ->  Nested Loop  (cost=1639.25..2611145.59 rows=99 width=28)
> > (actual time=0.076..0.076 rows=0 loops=1)
> >Join Filter:
> > (((f_ticketattributeshistory_aajzjp98uraszb6.attrnewvalue_id = ANY
> > ('{4757,4758,4759}'::integer[])) AND (4754 =
> > f_ticketattributeshistory_aajzjp98uraszb6.attroldvalue_id) AND (4790 =
> > f_ticketattributeshistory_aajzjp98uraszb6.ticketfield_id)) OR
> > (f_zendesktickets_aaeljtllr5at3el.dt_createda
> > t_id = f_ticketupdate_aad5jtwal0ayaax.dt_event_id))
> >->  Nested Loop  (cost=1638.81..1809540.39 rows=350270
> > width=20) (actual time=0.075..0.075 rows=0 loops=1)
> >  ->  Nested Loop  (cost=1638.24..1508474.08
> > rows=69140 width=8) (actual time=0.075..0.075 rows=0 loops=1)
> >->  Bitmap Heap Scan on
> > dwh_dm_aabv5kk9rxac4lz_aaonw7nchsan2n1_aad8xhr0m_aaewg8j61iagl1z
> > dwh_dm_aabv5kk9rxac4lz_aaonw7nchsan2n1_aad8xhr0m_aaewg8j61ia
> > (cost=4.34..27.35 rows=7 width=4) (actual time=0.026..0.038 rows=7
> loops=1)
> >  Recheck Cond: (6171 = id_euweek)
> >  Heap Blocks: exact=7
> >  ->  Bitmap Index Scan on
> > dwh_dm_aabv5kk9rxac4lz_aaonw7nchsan2n1_aad8xhr0m_a_id_euweek_idx
> > (cost=0.00..4.33 rows=7 width=0) (actual time=0.019..0.019 rows=7
> loops=1)
> >Index Cond: (6171 = id_euweek)
> >->  Bitmap Heap Scan on
> > f_ticketupdate_aad5jtwal0ayaax  (cost=1633.90..214617.67 rows=87472
> > width=8) (actual time=0.004..0.004 rows=0 loops=7)
> >  Recheck Cond: (dt_event_id =
> > dwh_dm_aabv5kk9rxac4lz_aaonw7nchsan2n1_aad8xhr0m_aaewg8j61ia.id
> >  >)
> >  ->  Bitmap Index Scan on
> > f_ticketupdate_aad5jtwal0ayaax_dt_event_id_idx  (cost=0.00..1612.03
> > rows=87472 width=0) (actual time=0.003..0.003 rows=0 loops=7)
> >Index Cond: (dt_event_id =
> > dwh_dm_aabv5kk9rxac4lz_aaonw7nchsan2n1_aad8xhr0m_aaewg8j61ia.id
> >  >)
> >  ->  Index Scan using
> > f_ticketattributeshistory_aajzjp98uraszb6_ticketupdate_id_idx on
> > f_ticketattributeshistory_aajzjp98uraszb6  (cost=0.57..4.12 rows=23
> > width=20) (never executed)
> >Index Cond: (ticketupdate_id =
> > f_ticketupdate_aad5jtwal0ayaax.id
> > )
> >->  Index Scan using
> > f_zendesktickets_aaeljtllr5at3el_pkey on
> > f_zendesktickets_aaeljtllr5at3el  (cost=0.43..2.27 rows=1 width=12)
> > (never executed)
> >  Index Cond: (id =
> > f_ticketattributeshistory_aajzjp98uraszb6.zendesktickets_id)
> >  Filter: ((4765 <> status_id) AND (group_id = 17429))
> >  Planning time: 8.516 ms
> >  Execution time: 1.895 ms
> >
> > the speed is back
> >
>
> Yeah, but the cost is higher (2611152 vs. 1949508). So clearly, the
> database believes it's going to be cheaper. I suspect a part of the
> issue might be that the join is misestimated - it's expected to produce
> ~29k rows, but produces 0.
>
> Can you check if this query has the same issue? It's just the
> problematic join, and it should be simpler to investigate:
>
> SELECT count(*)
>   FROM f_ticketupdate_aad5jtwal0ayaax AS f
>   INNER JOIN
>   dwh_dm_aabv5kk9rxac4lz_aaonw7nchsan2n1_aad8xhr0m_aaewg8j61iagl1z AS d
>   ON (f.dt_event_id = d.id)
>
>

db_sq3rjf5b7p309lq9wuqrh3qhk4gy9fbw=#  set  max_parallel_workers_per_
gather=2;
SET
db_sq3rjf5b7p309lq9wuqrh3qhk4gy9fbw=# explain analyze SELECT count(*)
db_sq3rjf5b7p309lq9wuqrh3qhk4gy9fbw-#   FROM f_ticketupdate_aad5jtwal0ayaax
AS f
db_sq3rjf5b7p309lq9wuqrh3qhk4gy9fbw-#   INNER JOIN
db_sq3rjf5b7p309lq9wuqrh3qhk4gy9fbw-#   dwh_dm_aabv5kk9rxac4lz_
aaonw7nchsan2n1_aad8xhr0m_aaewg8j61iagl1z AS d
db_sq3rjf5b7p309lq9wuqrh3qhk4gy9fbw-#   ON (f.dt_event_id = d.id);

QUERY PLAN


--
 Finalize Aggregate  (cost=1550912.23..1550912.24 

Re: very slow queries when max_parallel_workers_per_gather is higher than zero

[Tomas Vondra]

> Query Performs nicely, but no parallel workers are used:
> GroupAggregate  (cost=2611148.87..2611152.89 rows=31 width=22) (actual
> time=0.084..0.084 rows=0 loops=1)
>    Group Key:
> f_zendesktickets_aaeljtllr5at3el.cstm_custom_38746665_primary_column
>    ->  Sort  (cost=2611148.87..2611149.11 rows=99 width=28) (actual
> time=0.082..0.082 rows=0 loops=1)
>          Sort Key:
> f_zendesktickets_aaeljtllr5at3el.cstm_custom_38746665_primary_column
>          Sort Method: quicksort  Memory: 25kB
>          ->  Nested Loop  (cost=1639.25..2611145.59 rows=99 width=28)
> (actual time=0.076..0.076 rows=0 loops=1)
>                Join Filter:
> (((f_ticketattributeshistory_aajzjp98uraszb6.attrnewvalue_id = ANY
> ('{4757,4758,4759}'::integer[])) AND (4754 =
> f_ticketattributeshistory_aajzjp98uraszb6.attroldvalue_id) AND (4790 =
> f_ticketattributeshistory_aajzjp98uraszb6.ticketfield_id)) OR
> (f_zendesktickets_aaeljtllr5at3el.dt_createda
> t_id = f_ticketupdate_aad5jtwal0ayaax.dt_event_id))
>                ->  Nested Loop  (cost=1638.81..1809540.39 rows=350270
> width=20) (actual time=0.075..0.075 rows=0 loops=1)
>                      ->  Nested Loop  (cost=1638.24..1508474.08
> rows=69140 width=8) (actual time=0.075..0.075 rows=0 loops=1)
>                            ->  Bitmap Heap Scan on
> dwh_dm_aabv5kk9rxac4lz_aaonw7nchsan2n1_aad8xhr0m_aaewg8j61iagl1z
> dwh_dm_aabv5kk9rxac4lz_aaonw7nchsan2n1_aad8xhr0m_aaewg8j61ia 
> (cost=4.34..27.35 rows=7 width=4) (actual time=0.026..0.038 rows=7 loops=1)
>                                  Recheck Cond: (6171 = id_euweek)
>                                  Heap Blocks: exact=7
>                                  ->  Bitmap Index Scan on
> dwh_dm_aabv5kk9rxac4lz_aaonw7nchsan2n1_aad8xhr0m_a_id_euweek_idx 
> (cost=0.00..4.33 rows=7 width=0) (actual time=0.019..0.019 rows=7 loops=1)
>                                        Index Cond: (6171 = id_euweek)
>                            ->  Bitmap Heap Scan on
> f_ticketupdate_aad5jtwal0ayaax  (cost=1633.90..214617.67 rows=87472
> width=8) (actual time=0.004..0.004 rows=0 loops=7)
>                                  Recheck Cond: (dt_event_id =
> dwh_dm_aabv5kk9rxac4lz_aaonw7nchsan2n1_aad8xhr0m_aaewg8j61ia.id
> )
>                                  ->  Bitmap Index Scan on
> f_ticketupdate_aad5jtwal0ayaax_dt_event_id_idx  (cost=0.00..1612.03
> rows=87472 width=0) (actual time=0.003..0.003 rows=0 loops=7)
>                                        Index Cond: (dt_event_id =
> dwh_dm_aabv5kk9rxac4lz_aaonw7nchsan2n1_aad8xhr0m_aaewg8j61ia.id
> )
>                      ->  Index Scan using
> f_ticketattributeshistory_aajzjp98uraszb6_ticketupdate_id_idx on
> f_ticketattributeshistory_aajzjp98uraszb6  (cost=0.57..4.12 rows=23
> width=20) (never executed)
>                            Index Cond: (ticketupdate_id =
> f_ticketupdate_aad5jtwal0ayaax.id
> )
>                ->  Index Scan using
> f_zendesktickets_aaeljtllr5at3el_pkey on
> f_zendesktickets_aaeljtllr5at3el  (cost=0.43..2.27 rows=1 width=12)
> (never executed)
>                      Index Cond: (id =
> f_ticketattributeshistory_aajzjp98uraszb6.zendesktickets_id)
>                      Filter: ((4765 <> status_id) AND (group_id = 17429))
>  Planning time: 8.516 ms
>  Execution time: 1.895 ms
> 
> the speed is back
> 

Yeah, but the cost is higher (2611152 vs. 1949508). So clearly, the
database believes it's going to be cheaper. I suspect a part of the
issue might be that the join is misestimated - it's expected to produce
~29k rows, but produces 0.

Can you check if this query has the same issue? It's just the
problematic join, and it should be simpler to investigate:

SELECT count(*)
  FROM f_ticketupdate_aad5jtwal0ayaax AS f
  INNER JOIN
  dwh_dm_aabv5kk9rxac4lz_aaonw7nchsan2n1_aad8xhr0m_aaewg8j61iagl1z AS d
  ON (f.dt_event_id = d.id)


regards

-- 
Tomas Vondra  http://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Re: very slow queries when max_parallel_workers_per_gather is higher than zero

[Pavel Stehule]

2018-04-16 14:00 GMT+02:00 Tomas Vondra :

>
>
> On 04/16/2018 11:34 AM, Pavel Stehule wrote:
> > Hi,
> >
> > my customer does performance checks of PostgreSQL 9.5 and 10. Almost all
> > queries on 10 are faster, but there are few queries (40 from 1000) where
> > PostgreSQL 9.5 is significantly faster than. Unfortunately - pretty fast
> > queries (about 20ms) are too slow now (5 sec).
> >
> > attached execution plans
> >
> > It looks like some cost issue, slow queries prefers Seq scan against
> > bitmap heap scan
> >
> > Hash Cond: (f_ticketupdate_aad5jtwal0ayaax.dt_event_id =
> > dwh_dm_aabv5kk9rxac4lz_aaonw7nchsan2n1_aad8xhr0m_aaewg8j61ia.id
> >  >)
> >->  Parallel Seq Scan on f_ticketupdate_aad5jtwal0ayaax
> > (cost=0.00..1185867.47 rows=24054847 width=8) (actual
> > time=0.020..3741.409 rows=19243863 loops=3)
> >->  Hash  (cost=27.35..27.35 rows=7 width=4) (actual
> > time=0.089..0.089 rows=7 loops=3)
> > Buckets: 1024  Batches: 1  Memory Usage: 9kB
> >
> >
>
> What happens when you disable sequential scans on pg10?
>

set enable_seqscan=off;
set  max_parallel_workers_per_gather=2;

Query Performs nicely, but no parallel workers are used:
GroupAggregate  (cost=2611148.87..2611152.89 rows=31 width=22) (actual
time=0.084..0.084 rows=0 loops=1)
   Group Key: f_zendesktickets_aaeljtllr5at3el.cstm_custom_
38746665_primary_column
   ->  Sort  (cost=2611148.87..2611149.11 rows=99 width=28) (actual
time=0.082..0.082 rows=0 loops=1)
 Sort Key: f_zendesktickets_aaeljtllr5at3el.cstm_custom_
38746665_primary_column
 Sort Method: quicksort  Memory: 25kB
 ->  Nested Loop  (cost=1639.25..2611145.59 rows=99 width=28)
(actual time=0.076..0.076 rows=0 loops=1)
   Join Filter: (((f_ticketattributeshistory_
aajzjp98uraszb6.attrnewvalue_id = ANY ('{4757,4758,4759}'::integer[])) AND
(4754 = f_ticketattributeshistory_aajzjp98uraszb6.attroldvalue_id) AND
(4790 = f_ticketattributeshistory_aajzjp98uraszb6.ticketfield_id)) OR
(f_zendesktickets_aaeljtllr5at3el.dt_createda
t_id = f_ticketupdate_aad5jtwal0ayaax.dt_event_id))
   ->  Nested Loop  (cost=1638.81..1809540.39 rows=350270
width=20) (actual time=0.075..0.075 rows=0 loops=1)
 ->  Nested Loop  (cost=1638.24..1508474.08 rows=69140
width=8) (actual time=0.075..0.075 rows=0 loops=1)
   ->  Bitmap Heap Scan on dwh_dm_aabv5kk9rxac4lz_
aaonw7nchsan2n1_aad8xhr0m_aaewg8j61iagl1z dwh_dm_aabv5kk9rxac4lz_
aaonw7nchsan2n1_aad8xhr0m_aaewg8j61ia  (cost=4.34..27.35 rows=7 width=4)
(actual time=0.026..0.038 rows=7 loops=1)
 Recheck Cond: (6171 = id_euweek)
 Heap Blocks: exact=7
 ->  Bitmap Index Scan on
dwh_dm_aabv5kk9rxac4lz_aaonw7nchsan2n1_aad8xhr0m_a_id_euweek_idx
(cost=0.00..4.33 rows=7 width=0) (actual time=0.019..0.019 rows=7 loops=1)
   Index Cond: (6171 = id_euweek)
   ->  Bitmap Heap Scan on
f_ticketupdate_aad5jtwal0ayaax
(cost=1633.90..214617.67 rows=87472 width=8) (actual time=0.004..0.004
rows=0 loops=7)
 Recheck Cond: (dt_event_id =
dwh_dm_aabv5kk9rxac4lz_aaonw7nchsan2n1_aad8xhr0m_aaewg8j61ia.id)
 ->  Bitmap Index Scan on f_ticketupdate_
aad5jtwal0ayaax_dt_event_id_idx  (cost=0.00..1612.03 rows=87472 width=0)
(actual time=0.003..0.003 rows=0 loops=7)
   Index Cond: (dt_event_id =
dwh_dm_aabv5kk9rxac4lz_aaonw7nchsan2n1_aad8xhr0m_aaewg8j61ia.id)
 ->  Index Scan using f_ticketattributeshistory_
aajzjp98uraszb6_ticketupdate_id_idx on
f_ticketattributeshistory_aajzjp98uraszb6
(cost=0.57..4.12 rows=23 width=20) (never executed)
   Index Cond: (ticketupdate_id = f_ticketupdate_
aad5jtwal0ayaax.id)
   ->  Index Scan using f_zendesktickets_aaeljtllr5at3el_pkey
on f_zendesktickets_aaeljtllr5at3el  (cost=0.43..2.27 rows=1 width=12)
(never executed)
 Index Cond: (id = f_ticketattributeshistory_
aajzjp98uraszb6.zendesktickets_id)
 Filter: ((4765 <> status_id) AND (group_id = 17429))
 Planning time: 8.516 ms
 Execution time: 1.895 ms

the speed is back



>
> regards
>
> --
> Tomas Vondra  http://www.2ndQuadrant.com
> PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
>

Re: very slow queries when max_parallel_workers_per_gather is higher than zero

[Tomas Vondra]

On 04/16/2018 11:34 AM, Pavel Stehule wrote:
> Hi,
> 
> my customer does performance checks of PostgreSQL 9.5 and 10. Almost all
> queries on 10 are faster, but there are few queries (40 from 1000) where
> PostgreSQL 9.5 is significantly faster than. Unfortunately - pretty fast
> queries (about 20ms) are too slow now (5 sec).
> 
> attached execution plans
> 
> It looks like some cost issue, slow queries prefers Seq scan against
> bitmap heap scan
> 
> Hash Cond: (f_ticketupdate_aad5jtwal0ayaax.dt_event_id =
> dwh_dm_aabv5kk9rxac4lz_aaonw7nchsan2n1_aad8xhr0m_aaewg8j61ia.id
> )
>    ->  Parallel Seq Scan on f_ticketupdate_aad5jtwal0ayaax 
> (cost=0.00..1185867.47 rows=24054847 width=8) (actual
> time=0.020..3741.409 rows=19243863 loops=3)
>    ->  Hash  (cost=27.35..27.35 rows=7 width=4) (actual
> time=0.089..0.089 rows=7 loops=3)
>     Buckets: 1024  Batches: 1  Memory Usage: 9kB
> 
> 

What happens when you disable sequential scans on pg10?

regards

-- 
Tomas Vondra  http://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Re: AS OF queries

[Bruce Momjian]

On Fri, Jan 26, 2018 at 10:56:06AM +0300, Konstantin Knizhnik wrote:
> >>Yeh, I suspected that just disabling autovacuum was not enough.
> >>I heard (but do no know too much) about microvacuum and hot updates.
> >>This is why I was a little bit surprised when me test didn't show lost of 
> >>updated versions.
> >>May be it is because of vacuum_defer_cleanup_age.
> >Well vacuum and single-page pruning do 3 things:
> >
> >1.  remove expired updated rows
> >2.  remove deleted row
> >3.  remove rows from aborted transactions
> >
> >While time travel doesn't want #1 and #2, it probably wants #3.
> >
> Rows of aborted transactions are in any case excluded by visibility checks.
> Definitely skipping them costs some time, so large percent of aborted
> transactions  may affect query speed.
> But query speed is reduced in any case if in order to support time travel we
> prohibit or postpone vacuum.
> 
> What is the expected relation of committed and aborted transactions? I
> expected that it should be much bigger than one (especially if we take in
> account
> only read-write transaction which has really updated database). In this case
> number of versions created by aborted transaction should be much smaller
> than number of versions created by updated/delete of successful
> transactions. So them should not have significant impact on performance.

Uh, I think the big question is whether we are ready to agreed that a
time-travel database will _never_ have aborted rows removed.  The
aborted rows are clearly useless for time travel, so the question is
whether we ever want to remove them.  I would think at some point we do.

Also, I am not sure we have any statistics on how many aborted rows are
in each table.

-- 
  Bruce Momjian  http://momjian.us
  EnterpriseDB http://enterprisedb.com

+ As you are, so once was I.  As I am, so you will be. +
+  Ancient Roman grave inscription +

Re: AS OF queries

[Konstantin Knizhnik]

On 26.01.2018 03:55, Bruce Momjian wrote:

On Sat, Dec 23, 2017 at 11:53:19PM +0300, konstantin knizhnik wrote:

On Dec 23, 2017, at 2:08 AM, Greg Stark wrote:


On 20 December 2017 at 12:45, Konstantin Knizhnik
 wrote:


It seems to me that it will be not so difficult to implement them in
Postgres - we already have versions of tuples.
Looks like we only need to do three things:
1. Disable autovacuum (autovacuum = off)

"The Wheel of Time turns, and Ages come and pass, leaving memories
that become legend. Legend fades to myth, and even myth is long
forgotten when the Age that gave it birth comes again"

I think you'll find it a lot harder to get this to work than just
disabling autovacuum. Notably HOT updates can get cleaned up (and even
non-HOT updates can now leave tombstone dead line pointers iirc) even
if vacuum hasn't run.


Yeh, I suspected that just disabling autovacuum was not enough.
I heard (but do no know too much) about microvacuum and hot updates.
This is why I was a little bit surprised when me test didn't show lost of 
updated versions.
May be it is because of vacuum_defer_cleanup_age.

Well vacuum and single-page pruning do 3 things:

1.  remove expired updated rows
2.  remove deleted row
3.  remove rows from aborted transactions

While time travel doesn't want #1 and #2, it probably wants #3.


Rows of aborted transactions are in any case excluded by visibility checks.
Definitely skipping them costs some time, so large percent of aborted 
transactions  may affect query speed.
But query speed is reduced in any case if in order to support time 
travel we prohibit or postpone vacuum.


What is the expected relation of committed and aborted transactions? I 
expected that it should be much bigger than one (especially if we take 
in account
only read-write transaction which has really updated database). In this 
case number of versions created by aborted transaction should be much 
smaller than number of versions created by updated/delete of successful 
transactions. So them should not have significant impact on performance.




--
Konstantin Knizhnik
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company

Re: AS OF queries

[Bruce Momjian]

On Sat, Dec 23, 2017 at 11:53:19PM +0300, konstantin knizhnik wrote:
> 
> On Dec 23, 2017, at 2:08 AM, Greg Stark wrote:
> 
> > On 20 December 2017 at 12:45, Konstantin Knizhnik
> >  wrote:
> > 
> >> It seems to me that it will be not so difficult to implement them in
> >> Postgres - we already have versions of tuples.
> >> Looks like we only need to do three things:
> >> 1. Disable autovacuum (autovacuum = off)
> > 
> > "The Wheel of Time turns, and Ages come and pass, leaving memories
> > that become legend. Legend fades to myth, and even myth is long
> > forgotten when the Age that gave it birth comes again"
> > 
> > I think you'll find it a lot harder to get this to work than just
> > disabling autovacuum. Notably HOT updates can get cleaned up (and even
> > non-HOT updates can now leave tombstone dead line pointers iirc) even
> > if vacuum hasn't run.
> > 
> 
> Yeh, I suspected that just disabling autovacuum was not enough.
> I heard (but do no know too much) about microvacuum and hot updates.
> This is why I was a little bit surprised when me test didn't show lost of 
> updated versions.
> May be it is because of vacuum_defer_cleanup_age.

Well vacuum and single-page pruning do 3 things:

1.  remove expired updated rows
2.  remove deleted row
3.  remove rows from aborted transactions

While time travel doesn't want #1 and #2, it probably wants #3.

-- 
  Bruce Momjian  http://momjian.us
  EnterpriseDB http://enterprisedb.com

+ As you are, so once was I.  As I am, so you will be. +
+  Ancient Roman grave inscription +

Re: AS OF queries

[legrand legrand]

> Sorry, truncate is not compatible with AS OF. It is performed at file
> level and deletes old old version.
> So if you want to use time travel, you should not use truncate. 

As time travel doesn't support truncate, I would prefer it to be checked.
If no check is performed, ASOF queries (with timestamp before truncate ) 
would return no data even when there was: this could be considered as a
wrong result.

if a truncate is detected, an error should be raised, saying data is no more
available before truncate timestamp.


> Does it mean that no explicit check is needed that table metadata was
> not checked after specified timeslice? 

Not sure, it would depend on metadata modification type ...
adding/dropping a columns seems working, 
what about altering a column type or dropping / recreating a table ?

Regards
PAscal



--
Sent from: http://www.postgresql-archive.org/PostgreSQL-hackers-f1928748.html

Re: AS OF queries

[Konstantin Knizhnik]

On 10.01.2018 16:02, legrand legrand wrote:

But performing this query on each access to the table seems to be bad
idea: in case of nested loop join it can cause significant degrade of
performance.

this could be a pre-plan / pre-exec check, no more.


AS-OF timestamp can be taken from outer table, so it is necessary to 
repeat this check at each nested loop join iteration.





But I am not sure that this check is actually needed.
If table is changed in some incompatible way, then we will get error in
any case.

It seems that with path v3, a query with asof_timestamp
set before a truncate or alter table doesn't throw any error,
just gives an empty result (even if there was data).


Sorry, truncate is not compatible with AS OF. It is performed at file 
level and deletes old old version.

So if you want to use time travel, you should not use truncate.




If table change is not critical for this query (for example some column
was added or removed which is not used in this query),
then should we really throw error in this case?

no error is needed if result is correct.


Does it mean that no explicit check is needed that table metadata was 
not checked after specified timeslice?



Attached please find new version of the AS OF patch which throws error 
if specified AS OF timestamp is older that time travel horizon and 
"check_asof_timestamp" parameter is set to true (by default it is 
switched off).



--
Konstantin Knizhnik
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company

diff --git a/src/backend/executor/execScan.c b/src/backend/executor/execScan.c
index 837abc0..3ac7868 100644
--- a/src/backend/executor/execScan.c
+++ b/src/backend/executor/execScan.c
@@ -21,7 +21,8 @@
 #include "executor/executor.h"
 #include "miscadmin.h"
 #include "utils/memutils.h"
-
+#include "utils/snapmgr.h"
+#include "utils/timestamp.h"
 
 
 /*
@@ -296,3 +297,43 @@ ExecScanReScan(ScanState *node)
 		}
 	}
 }
+
+/*
+ * Evaluate ASOF timestamp,
+ * check that it belongs to the time travel period (if specified)
+ * and assign it to snapshot.
+ * This function throws error if specified snapshot is out of
+ * time_travel_period and check_asof_timestamp parameter is true
+ */
+void ExecAsofTimestamp(EState* estate, ScanState* ss)
+{
+	if (ss->asofExpr)
+	{
+		if (!ss->asofTimestampSet)
+		{
+			Datum		val;
+			bool		isNull;
+
+			val = ExecEvalExprSwitchContext(ss->asofExpr,
+			ss->ps.ps_ExprContext,
+			);
+			if (isNull)
+			{
+/* Interpret NULL timestamp as no timestamp */
+ss->asofTimestamp = 0;
+			}
+			else
+			{
+ss->asofTimestamp = DatumGetInt64(val);
+if (check_asof_timestamp && time_travel_period > 0)
+{
+	TimestampTz horizon = GetCurrentTimestamp()	- (TimestampTz)time_travel_period*USECS_PER_SEC;
+	if (timestamptz_cmp_internal(horizon, ss->asofTimestamp) > 0)
+		elog(ERROR, "Specified AS OF timestamp is out of time travel horizon");
+}
+			}
+			ss->asofTimestampSet = true;
+		}
+		estate->es_snapshot->asofTimestamp = ss->asofTimestamp;
+	}
+}
diff --git a/src/backend/executor/nodeBitmapHeapscan.c b/src/backend/executor/nodeBitmapHeapscan.c
index eb5bbb5..b880c18 100644
--- a/src/backend/executor/nodeBitmapHeapscan.c
+++ b/src/backend/executor/nodeBitmapHeapscan.c
@@ -78,6 +78,7 @@ BitmapHeapNext(BitmapHeapScanState *node)
 	ExprContext *econtext;
 	HeapScanDesc scan;
 	TIDBitmap  *tbm;
+	EState	   *estate;
 	TBMIterator *tbmiterator = NULL;
 	TBMSharedIterator *shared_tbmiterator = NULL;
 	TBMIterateResult *tbmres;
@@ -85,11 +86,13 @@ BitmapHeapNext(BitmapHeapScanState *node)
 	TupleTableSlot *slot;
 	ParallelBitmapHeapState *pstate = node->pstate;
 	dsa_area   *dsa = node->ss.ps.state->es_query_dsa;
+	TimestampTz outerAsofTimestamp = 0;
 
 	/*
 	 * extract necessary information from index scan node
 	 */
 	econtext = node->ss.ps.ps_ExprContext;
+	estate = node->ss.ps.state;
 	slot = node->ss.ss_ScanTupleSlot;
 	scan = node->ss.ss_currentScanDesc;
 	tbm = node->tbm;
@@ -99,6 +102,9 @@ BitmapHeapNext(BitmapHeapScanState *node)
 		shared_tbmiterator = node->shared_tbmiterator;
 	tbmres = node->tbmres;
 
+	outerAsofTimestamp = estate->es_snapshot->asofTimestamp;
+	ExecAsofTimestamp(estate, >ss);
+
 	/*
 	 * If we haven't yet performed the underlying index scan, do it, and begin
 	 * the iteration over the bitmap.
@@ -364,11 +370,21 @@ BitmapHeapNext(BitmapHeapScanState *node)
 			}
 		}
 
-		/* OK to return this tuple */
+		/*
+		 * Restore ASOF timestamp for the current snapshot
+		 */
+		estate->es_snapshot->asofTimestamp = outerAsofTimestamp;
+
+	/* OK to return this tuple */
 		return slot;
 	}
 
 	/*
+	 * Restore ASOF timestamp for the current snapshot
+	 */
+	estate->es_snapshot->asofTimestamp = outerAsofTimestamp;
+
+	/*
 	 * if we get here it means we are at the end of the scan..
 	 */
 	return ExecClearTuple(slot);
@@ -746,6 +762,8 @@ ExecReScanBitmapHeapScan(BitmapHeapScanState *node)
 {
 	PlanState  *outerPlan = 

Re: AS OF queries

[legrand legrand]

> But performing this query on each access to the table seems to be bad
> idea: in case of nested loop join it can cause significant degrade of
> performance.

this could be a pre-plan / pre-exec check, no more.

> But I am not sure that this check is actually needed.
> If table is changed in some incompatible way, then we will get error in
> any case.

It seems that with path v3, a query with asof_timestamp 
set before a truncate or alter table doesn't throw any error, 
just gives an empty result (even if there was data).

> If table change is not critical for this query (for example some column
> was added or removed which is not used in this query),
> then should we really throw error in this case? 

no error is needed if result is correct.

Regards
PAscal



--
Sent from: http://www.postgresql-archive.org/PostgreSQL-hackers-f1928748.html

Re: AS OF queries

[Konstantin Knizhnik]

On 03.01.2018 23:49, legrand legrand wrote:

Maybe that a simple check of the asof_timestamp value like:

asof_timestamp >= now() - time_travel_period
AND
asof_timestamp >= latest_table_ddl

would permit to raise a warning or an error message saying that query result
can not be garanteed with this asof_timestamp value.


latest_table_ddl being found with

SELECT greatest( max(pg_xact_commit_timestamp( rel.xmin )),
max(pg_xact_commit_timestamp( att.xmin ))) as latest_table_ddl
FROM  pg_catalog.pg_attribute att   
INNER JOIN pg_catalog.pg_class rel  
ON att.attrelid = rel.oid WHERE rel.relname = '' and
rel.relowner= ...

(tested with add/alter/drop column and drop/create/truncate table)


Well, it can be done.
But performing this query on each access to the table seems to be bad 
idea: in case of nested loop join it can cause significant degrade of 
performance.
The obvious solution is to calculate this latest_table_ddl timestamp 
once and store it it somewhere (in ScanState?)

But I am not sure that this check is actually needed.
If table is changed in some incompatible way, then we will get error in 
any case.
If table change is not critical for this query (for example some column 
was added or removed which is not used in this query),

then should we really throw error in this case?

--
Konstantin Knizhnik
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company

Re: AS OF queries

[legrand legrand]

Maybe that a simple check of the asof_timestamp value like:

asof_timestamp >= now() - time_travel_period
AND 
asof_timestamp >= latest_table_ddl

would permit to raise a warning or an error message saying that query result
can not be garanteed with this asof_timestamp value.


latest_table_ddl being found with

SELECT greatest( max(pg_xact_commit_timestamp( rel.xmin )),
max(pg_xact_commit_timestamp( att.xmin ))) as latest_table_ddl 
FROM  pg_catalog.pg_attribute att   
INNER JOIN pg_catalog.pg_class rel   
ON att.attrelid = rel.oid WHERE rel.relname = '' and
rel.relowner= ...

(tested with add/alter/drop column and drop/create/truncate table)

Regards
PAscal



--
Sent from: http://www.postgresql-archive.org/PostgreSQL-hackers-f1928748.html

Re: AS OF queries

[Peter Eisentraut]

On 12/29/17 06:28, Konstantin Knizhnik wrote:
>>   Can there be apparent RI
>> violations? 
> Right now AS OF is used only in selects, not in update statements. So I
> do not understand how integrity constraints can be violated.

I mean, if you join tables connected by a foreign key, you can expect a
certain shape of result, for example at least one match per PK row.  But
if you select from each table "as of" a different timestamp, then that
won't hold.  That could also throw off any optimizations we might come
up with in that area, such as cross-table statistics.  Not saying it
can't or shouldn't be done, but there might be some questions.

>>  What happens if no old data for the
>> selected AS OF is available? 
> It will just return the version closest to the specified timestamp.

That seems strange.  Shouldn't that be an error?

>>  How does this interact with catalog
>> changes, such as changes to row-level security settings?  (Do we apply
>> the current or the past settings?)
> Catalog changes are not currently supported.
> And I do not have good understanding how to support it if query involves
> two different timeslice with different versions of the table.
> Too much places in parser/optimizer have to be change to support such
> "historical collisions".

Right, it's probably very hard to do.  But I think it somehow should be
recognized that catalog changes took place between the selected
timestamp(s) and now and an error or notice should be produced.

-- 
Peter Eisentraut  http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Re: AS OF queries

[Konstantin Knizhnik]

On 28.12.2017 20:28, Peter Eisentraut wrote:

On 12/28/17 11:36, Konstantin Knizhnik wrote:

Attached please find new version of AS OF patch which allows to specify
time travel period.
Older versions outside this period may be reclaimed by autovacuum.
This behavior is controlled by "time_travel_period" parameter.

So where are we on using quasi SQL-standard syntax for a nonstandard
interpretation?  I think we could very well have a variety of standard
and nonstandard AS OF variants, including by commit timestamp, xid,
explicit range columns, etc.  But I'd like to see a discussion on that,
perhaps in a documentation update, which this patch is missing.

SQL:2011 ||defines rules for creation and querying of temporal tables.
I have not read this standard myself, I just take information about it 
from wikipedia:

https://en.wikipedia.org/wiki/SQL:2011
According to this standard time-sliced queries are specified using
|
AS OF SYSTEM TIME| |and| |VERSIONS BETWEEN SYSTEM TIME ... AND ...|clauses.

Looks like it is supported now only by Oracle. IBM DB, MS-SQL, are 
providing similar functionality in slightly different way.
I am not sure whether strict support of SQL:2011 standard is critical 
and which other functionality we need.



I have questions about corner cases.  What happens when multiple tables
are queried with different AS OF clauses?

It is possible.


   Can there be apparent RI
violations?
Right now AS OF is used only in selects, not in update statements. So I 
do not understand how integrity constraints can be violated.



  What happens when the time_travel_period is changed during
a session?
Right now it depends on autovacuum: how fast it will be able to reclaim 
old version.
Actually I I do not see much sense in changing time travel period during 
session.
In asof-4.patch time_travel_period is postmaster level GUC which can not 
be changed in session.
But I have changed policy for it for SIGHUP to make experiments with it 
more easier.




How can we check how much old data is available, and how can
we check how much space it uses?
Physical space used by the database/relation can be determined using 
standard functions, for example pg_total_relation_size.

I do not know any simple way to get total number of all stored versions.

  What happens if no old data for the
selected AS OF is available?

It will just return the version closest to the specified timestamp.


  How does this interact with catalog
changes, such as changes to row-level security settings?  (Do we apply
the current or the past settings?)

Catalog changes are not currently supported.
And I do not have good understanding how to support it if query involves 
two different timeslice with different versions of the table.
Too much places in parser/optimizer have to be change to support such 
"historical collisions".




This patch should probably include a bunch of tests to cover these and
other scenarios.

Right now I have added just one test: asof.sql.
It requires "track_commit_timestamp" option to be switched on and it is 
postmaster level GUC.

So I have added for it postgresql.asof.conf and asof_schedule.
This test should be launched using the following command:

make check EXTRA_REGRESS_OPTS="--schedule=asof_schedule 
--temp-config=postgresql.asof.config"


If there is some better way to include this test in standard regression 
tests, please let me know.

(Maybe "period" isn't the best name, because it implies a start and an
end.  How about something with "age"?)
Well I am not an English native speaker. So I can not conclude what is 
more natural.
"period" is widely used in topics related with temporal tables (just 
count occurrences of this word at https://en.wikipedia.org/wiki/SQL:2011)

Age is not used here at all.
From my point of view age is something applicable to person, building, 
monument,...
It is not possible to say about "ge of time travel". In science fiction 
"time machines" frequently have limitations: you can not got more than N 
years in the past.

How we can name this N? Is it "period", "age" or something else?

I attached yet another version of the patch which includes test for AS 
OF query.


--
Konstantin Knizhnik
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company

diff --git a/src/backend/executor/nodeBitmapHeapscan.c b/src/backend/executor/nodeBitmapHeapscan.c
index eb5bbb5..7acaf30 100644
--- a/src/backend/executor/nodeBitmapHeapscan.c
+++ b/src/backend/executor/nodeBitmapHeapscan.c
@@ -78,6 +78,7 @@ BitmapHeapNext(BitmapHeapScanState *node)
 	ExprContext *econtext;
 	HeapScanDesc scan;
 	TIDBitmap  *tbm;
+	EState	   *estate;
 	TBMIterator *tbmiterator = NULL;
 	TBMSharedIterator *shared_tbmiterator = NULL;
 	TBMIterateResult *tbmres;
@@ -85,11 +86,13 @@ BitmapHeapNext(BitmapHeapScanState *node)
 	TupleTableSlot *slot;
 	ParallelBitmapHeapState *pstate = node->pstate;
 	dsa_area   *dsa = node->ss.ps.state->es_query_dsa;
+	TimestampTz outerAsofTimestamp = 0;
 
 	/*
 	 * 

Re: AS OF queries

[Peter Eisentraut]

On 12/28/17 11:36, Konstantin Knizhnik wrote:
> Attached please find new version of AS OF patch which allows to specify 
> time travel period.
> Older versions outside this period may be reclaimed by autovacuum.
> This behavior is controlled by "time_travel_period" parameter.

So where are we on using quasi SQL-standard syntax for a nonstandard
interpretation?  I think we could very well have a variety of standard
and nonstandard AS OF variants, including by commit timestamp, xid,
explicit range columns, etc.  But I'd like to see a discussion on that,
perhaps in a documentation update, which this patch is missing.

I have questions about corner cases.  What happens when multiple tables
are queried with different AS OF clauses?  Can there be apparent RI
violations?  What happens when the time_travel_period is changed during
a session?  How can we check how much old data is available, and how can
we check how much space it uses?  What happens if no old data for the
selected AS OF is available?  How does this interact with catalog
changes, such as changes to row-level security settings?  (Do we apply
the current or the past settings?)

This patch should probably include a bunch of tests to cover these and
other scenarios.

(Maybe "period" isn't the best name, because it implies a start and an
end.  How about something with "age"?)

-- 
Peter Eisentraut  http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Re: AS OF queries

[Konstantin Knizhnik]

Attached please find new version of AS OF patch which allows to specify 
time travel period.

Older versions outside this period may be reclaimed by autovacuum.
This behavior is controlled by "time_travel_period" parameter.

Zero value of this parameter disables time travel and postgres behaves 
in standard way.
Actually you can still use AS AF construction but there is no warranty 
that requested versions are not reclaimed and result of query actually 
belongs to the specified time slice.


Value -1 means infinite history: versions are never reclaimed and 
autovacuum is disabled.


And positive value of this parameter specifies maximal time travel 
period in seconds.
As in case of disabled time travel, you can specify AS OF timestamp 
older than this period.

But there is no warranty that requested versions still exist.

--
Konstantin Knizhnik
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company

diff --git a/src/backend/executor/nodeBitmapHeapscan.c b/src/backend/executor/nodeBitmapHeapscan.c
index eb5bbb5..7acaf30 100644
--- a/src/backend/executor/nodeBitmapHeapscan.c
+++ b/src/backend/executor/nodeBitmapHeapscan.c
@@ -78,6 +78,7 @@ BitmapHeapNext(BitmapHeapScanState *node)
 	ExprContext *econtext;
 	HeapScanDesc scan;
 	TIDBitmap  *tbm;
+	EState	   *estate;
 	TBMIterator *tbmiterator = NULL;
 	TBMSharedIterator *shared_tbmiterator = NULL;
 	TBMIterateResult *tbmres;
@@ -85,11 +86,13 @@ BitmapHeapNext(BitmapHeapScanState *node)
 	TupleTableSlot *slot;
 	ParallelBitmapHeapState *pstate = node->pstate;
 	dsa_area   *dsa = node->ss.ps.state->es_query_dsa;
+	TimestampTz outerAsofTimestamp = 0;
 
 	/*
 	 * extract necessary information from index scan node
 	 */
 	econtext = node->ss.ps.ps_ExprContext;
+	estate = node->ss.ps.state;
 	slot = node->ss.ss_ScanTupleSlot;
 	scan = node->ss.ss_currentScanDesc;
 	tbm = node->tbm;
@@ -99,6 +102,25 @@ BitmapHeapNext(BitmapHeapScanState *node)
 		shared_tbmiterator = node->shared_tbmiterator;
 	tbmres = node->tbmres;
 
+	outerAsofTimestamp = estate->es_snapshot->asofTimestamp;
+
+	if (node->ss.asofExpr)
+	{
+		if (!node->ss.asofTimestampSet)
+		{
+			Datum		val;
+			bool		isNull;
+
+			val = ExecEvalExprSwitchContext(node->ss.asofExpr,
+			node->ss.ps.ps_ExprContext,
+			);
+			/* Interpret NULL timestamp as no timestamp */
+			node->ss.asofTimestamp = isNull ? 0 : DatumGetInt64(val);
+			node->ss.asofTimestampSet = true;
+		}
+		estate->es_snapshot->asofTimestamp = node->ss.asofTimestamp;
+	}
+
 	/*
 	 * If we haven't yet performed the underlying index scan, do it, and begin
 	 * the iteration over the bitmap.
@@ -364,11 +386,21 @@ BitmapHeapNext(BitmapHeapScanState *node)
 			}
 		}
 
-		/* OK to return this tuple */
+		/*
+		 * Restore ASOF timestamp for the current snapshot
+		 */
+		estate->es_snapshot->asofTimestamp = outerAsofTimestamp;
+
+	/* OK to return this tuple */
 		return slot;
 	}
 
 	/*
+	 * Restore ASOF timestamp for the current snapshot
+	 */
+	estate->es_snapshot->asofTimestamp = outerAsofTimestamp;
+
+	/*
 	 * if we get here it means we are at the end of the scan..
 	 */
 	return ExecClearTuple(slot);
@@ -746,6 +778,8 @@ ExecReScanBitmapHeapScan(BitmapHeapScanState *node)
 {
 	PlanState  *outerPlan = outerPlanState(node);
 
+	node->ss.asofTimestampSet = false;
+
 	/* rescan to release any page pin */
 	heap_rescan(node->ss.ss_currentScanDesc, NULL);
 
@@ -902,7 +936,8 @@ ExecInitBitmapHeapScan(BitmapHeapScan *node, EState *estate, int eflags)
 	 * most cases it's probably not worth working harder than that.
 	 */
 	scanstate->can_skip_fetch = (node->scan.plan.qual == NIL &&
- node->scan.plan.targetlist == NIL);
+ node->scan.plan.targetlist == NIL &&
+ node->scan.asofTimestamp == NULL);
 
 	/*
 	 * Miscellaneous initialization
@@ -920,6 +955,18 @@ ExecInitBitmapHeapScan(BitmapHeapScan *node, EState *estate, int eflags)
 		ExecInitQual(node->bitmapqualorig, (PlanState *) scanstate);
 
 	/*
+	 * Initlialize AS OF expression of any
+	 */
+	if (node->scan.asofTimestamp)
+	{
+		scanstate->ss.asofExpr = ExecInitExpr((Expr *) node->scan.asofTimestamp,
+		   >ss.ps);
+		scanstate->ss.asofTimestampSet = false;
+	}
+	else
+		scanstate->ss.asofExpr = NULL;
+
+	/*
 	 * tuple table initialization
 	 */
 	ExecInitResultTupleSlot(estate, >ss.ps);
@@ -1052,11 +1099,27 @@ ExecBitmapHeapInitializeDSM(BitmapHeapScanState *node,
 	ParallelBitmapHeapState *pstate;
 	EState	   *estate = node->ss.ps.state;
 	dsa_area   *dsa = node->ss.ps.state->es_query_dsa;
+	TimestampTz outerAsofTimestamp = estate->es_snapshot->asofTimestamp;
+	Scan* scan = (Scan*)node->ss.ps.plan;
 
 	/* If there's no DSA, there are no workers; initialize nothing. */
 	if (dsa == NULL)
 		return;
 
+	if (scan->asofTimestamp)
+	{
+		Datum		val;
+		bool		isNull;
+
+		ExprState* asofExpr = ExecInitExpr((Expr *) scan->asofTimestamp,
+	  >ss.ps);
+		val = ExecEvalExprSwitchContext(asofExpr,
+		

Re: AS OF queries

[Peter van Hardenberg]

On Wed, Dec 27, 2017 at 7:37 AM, Konstantin Knizhnik <
k.knizh...@postgrespro.ru> wrote:

>
>
> On 27.12.2017 00:52, Jeff Janes wrote:
>
> On Thu, Dec 21, 2017 at 6:00 AM, Konstantin Knizhnik <
> k.knizh...@postgrespro.ru> wrote:
>
>
>> There is still one significant difference of my prototype implementation
>> with SQL standard: it associates timestamp with select statement, not with
>> particular table.
>> It seems to be more difficult to support and I am not sure that joining
>> tables from different timelines has much sense.
>> But certainly it also can be fixed.
>
>
> I think the main use I would find for this feature is something like:
>
> select * from foo except select * from foo as old_foo as of '';
>
>
Just a quick report from the world of ORMs and web applications.

Today the idiomatic approach for an ORM like Ruby on Rails is to support
temporal(ish) queries using three additional TIMESTAMP_TZ columns:
"created_at", "updated_at" and "deleted_at". This idiom is bundled up into
a plugin called "acts_as_paranoid" (See: https://github.com/
rubysherpas/paranoia). We used this extensively at Heroku in our production
code for auditability reasons.

In general, this gets implemented on a per-table basis and usually has no
expiry short of manual cleanup. (It would be interesting to contemplate how
an end-user would clean up a table without losing their entire history in
the event of some kind of bug or bloat.)

I think a quality PostgreSQL-core implementation would be a fantastic
enhancement, though it would obviously introduce a bunch of interesting
decisions around how to handle things like referential integrity.

Personally, I frequently used these columns to query for things like "how
many users were created in each of the last twelve months", and the ability
to index on those dates was often important.

I'm confident that if this feature made it into PostgreSQL there would be
interested people in downstream communities that would take advantage of it.

Hope all that helps,

-- 
Peter van Hardenberg
San Francisco, California
"Everything was beautiful, and nothing hurt."—Kurt Vonnegut

Re: AS OF queries

[Konstantin Knizhnik]

On 27.12.2017 00:52, Jeff Janes wrote:
On Thu, Dec 21, 2017 at 6:00 AM, Konstantin Knizhnik 
> wrote:


There is still one significant difference of my prototype
implementation with SQL standard: it associates timestamp with
select statement, not with particular table.
It seems to be more difficult to support and I am not sure that
joining tables from different timelines has much sense.
But certainly it also can be fixed.


I think the main use I would find for this feature is something like:

select * from foo except select * from foo as old_foo as of '';

So I would be grateful if you can make that work. Also, I think 
conforming to the standards is pretty important where it is feasible 
to do that.


Cheers,

Jeff


I attach ne version of the patch which supports "standard" syntax, where 
AS OF clause is associated with table reference.

So it is possible to write query like:

    select * from SomeTable as t as of timestamp '2017-12-27 14:54:40' 
where id=100;


Also I introduced "time_travel" GUC which implicitly assigns some others 
GUCs:


        track_commit_timestamp = true;
        vacuum_defer_cleanup_age = 10;
        vacuum_freeze_min_age = 10;
        autovacuum_freeze_max_age = 20;
        autovacuum_multixact_freeze_max_age = 20;
        autovacuum_start_daemon = false;

So it disables autovacuum and microvacuum and enable commit timestamps 
tracking.

It provides access in the past up to milliard of transactions.

There is still no way to keep all versions only for particular tables or 
truncate too old versions.


--
Konstantin Knizhnik
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company

diff --git a/src/backend/executor/nodeBitmapHeapscan.c b/src/backend/executor/nodeBitmapHeapscan.c
index eb5bbb5..7acaf30 100644
--- a/src/backend/executor/nodeBitmapHeapscan.c
+++ b/src/backend/executor/nodeBitmapHeapscan.c
@@ -78,6 +78,7 @@ BitmapHeapNext(BitmapHeapScanState *node)
 	ExprContext *econtext;
 	HeapScanDesc scan;
 	TIDBitmap  *tbm;
+	EState	   *estate;
 	TBMIterator *tbmiterator = NULL;
 	TBMSharedIterator *shared_tbmiterator = NULL;
 	TBMIterateResult *tbmres;
@@ -85,11 +86,13 @@ BitmapHeapNext(BitmapHeapScanState *node)
 	TupleTableSlot *slot;
 	ParallelBitmapHeapState *pstate = node->pstate;
 	dsa_area   *dsa = node->ss.ps.state->es_query_dsa;
+	TimestampTz outerAsofTimestamp = 0;
 
 	/*
 	 * extract necessary information from index scan node
 	 */
 	econtext = node->ss.ps.ps_ExprContext;
+	estate = node->ss.ps.state;
 	slot = node->ss.ss_ScanTupleSlot;
 	scan = node->ss.ss_currentScanDesc;
 	tbm = node->tbm;
@@ -99,6 +102,25 @@ BitmapHeapNext(BitmapHeapScanState *node)
 		shared_tbmiterator = node->shared_tbmiterator;
 	tbmres = node->tbmres;
 
+	outerAsofTimestamp = estate->es_snapshot->asofTimestamp;
+
+	if (node->ss.asofExpr)
+	{
+		if (!node->ss.asofTimestampSet)
+		{
+			Datum		val;
+			bool		isNull;
+
+			val = ExecEvalExprSwitchContext(node->ss.asofExpr,
+			node->ss.ps.ps_ExprContext,
+			);
+			/* Interpret NULL timestamp as no timestamp */
+			node->ss.asofTimestamp = isNull ? 0 : DatumGetInt64(val);
+			node->ss.asofTimestampSet = true;
+		}
+		estate->es_snapshot->asofTimestamp = node->ss.asofTimestamp;
+	}
+
 	/*
 	 * If we haven't yet performed the underlying index scan, do it, and begin
 	 * the iteration over the bitmap.
@@ -364,11 +386,21 @@ BitmapHeapNext(BitmapHeapScanState *node)
 			}
 		}
 
-		/* OK to return this tuple */
+		/*
+		 * Restore ASOF timestamp for the current snapshot
+		 */
+		estate->es_snapshot->asofTimestamp = outerAsofTimestamp;
+
+	/* OK to return this tuple */
 		return slot;
 	}
 
 	/*
+	 * Restore ASOF timestamp for the current snapshot
+	 */
+	estate->es_snapshot->asofTimestamp = outerAsofTimestamp;
+
+	/*
 	 * if we get here it means we are at the end of the scan..
 	 */
 	return ExecClearTuple(slot);
@@ -746,6 +778,8 @@ ExecReScanBitmapHeapScan(BitmapHeapScanState *node)
 {
 	PlanState  *outerPlan = outerPlanState(node);
 
+	node->ss.asofTimestampSet = false;
+
 	/* rescan to release any page pin */
 	heap_rescan(node->ss.ss_currentScanDesc, NULL);
 
@@ -902,7 +936,8 @@ ExecInitBitmapHeapScan(BitmapHeapScan *node, EState *estate, int eflags)
 	 * most cases it's probably not worth working harder than that.
 	 */
 	scanstate->can_skip_fetch = (node->scan.plan.qual == NIL &&
- node->scan.plan.targetlist == NIL);
+ node->scan.plan.targetlist == NIL &&
+ node->scan.asofTimestamp == NULL);
 
 	/*
 	 * Miscellaneous initialization
@@ -920,6 +955,18 @@ ExecInitBitmapHeapScan(BitmapHeapScan *node, EState *estate, int eflags)
 		ExecInitQual(node->bitmapqualorig, (PlanState *) scanstate);
 
 	/*
+	 * Initlialize AS OF expression of any
+	 */
+	if (node->scan.asofTimestamp)
+	{
+		scanstate->ss.asofExpr = ExecInitExpr((Expr *) node->scan.asofTimestamp,
+	

Re: AS OF queries

[Konstantin Knizhnik]

On 27.12.2017 17:14, PostgreSQL - Hans-Jürgen Schönig wrote:


On 12/20/2017 01:45 PM, Konstantin Knizhnik wrote:

I wonder if Postgres community is interested in supporting time travel
queries in PostgreSQL (something like AS OF queries in Oracle:
https://docs.oracle.com/cd/B14117_01/appdev.101/b10795/adfns_fl.htm).
As far as I know something similar is now developed for MariaDB.

It seems to me that it will be not so difficult to implement them in
Postgres - we already have versions of tuples.
Looks like we only need to do three things:
1. Disable autovacuum (autovacuum = off)
2. Enable commit timestamp (track_commit_timestamp = on)
3. Add asofTimestamp to snapshot and patch XidInMVCCSnapshot to
compare commit timestamps when it is specified in snapshot.


that sounds really awesome ... i would love to see that.
my question is: while MVCC is fine when a tuple is still there ...
what are you going to do with TRUNCATE and so on?
it is not uncommon that a table is truncated frequently. in this case
MVCC won't help.
what are your thoughts on this ?


You should not use drop/truncate if you want to access old versions:)
Yes, truncate is much more faster than delete but it is because it 
operates on file level.

I think that it is quite natural limitation.

--
Konstantin Knizhnik
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company

Re: AS OF queries

[PostgreSQL - Hans-Jürgen Schönig]

On 12/20/2017 01:45 PM, Konstantin Knizhnik wrote:
> I wonder if Postgres community is interested in supporting time travel
> queries in PostgreSQL (something like AS OF queries in Oracle:
> https://docs.oracle.com/cd/B14117_01/appdev.101/b10795/adfns_fl.htm).
> As far as I know something similar is now developed for MariaDB.
>
> It seems to me that it will be not so difficult to implement them in
> Postgres - we already have versions of tuples.
> Looks like we only need to do three things:
> 1. Disable autovacuum (autovacuum = off)
> 2. Enable commit timestamp (track_commit_timestamp = on)
> 3. Add asofTimestamp to snapshot and patch XidInMVCCSnapshot to
> compare commit timestamps when it is specified in snapshot.
>

that sounds really awesome ... i would love to see that.
my question is: while MVCC is fine when a tuple is still there ...
what are you going to do with TRUNCATE and so on?
it is not uncommon that a table is truncated frequently. in this case
MVCC won't help.
what are your thoughts on this ?

    many thanks,

        hans

-- 
Hans-Jürgen Schönig
Cybertec Schönig & Schönig GmbH
Gröhrmühlgasse 26
A-2700 Wiener Neustadt
Web: https://www.cybertec-postgresql.com

Re: AS OF queries

[Hannu Krosing]

On 20.12.2017 14:45, Konstantin Knizhnik wrote:
> I wonder if Postgres community is interested in supporting time travel
> queries in PostgreSQL (something like AS OF queries in Oracle:
> https://docs.oracle.com/cd/B14117_01/appdev.101/b10795/adfns_fl.htm).
> As far as I know something similar is now developed for MariaDB.
>
> It seems to me that it will be not so difficult to implement them in
> Postgres - we already have versions of tuples.
> Looks like we only need to do three things:
> 1. Disable autovacuum (autovacuum = off)
In the design for original University Postgres ( which was a full
history database geared towards WORM drives )
it was the task of vacuum to move old tuples to "an archive" from where
the AS OF queries would then fetch
them as needed.

This might also be a good place to do Commit LSN to Commit Timestamp
translation

Hannu

> 2. Enable commit timestamp (track_commit_timestamp = on)
> 3. Add asofTimestamp to snapshot and patch XidInMVCCSnapshot to
> compare commit timestamps when it is specified in snapshot.
>
>
> Attached please find my prototype implementation of it.
> Most of the efforts are needed to support asof timestamp in grammar
> and add it to query plan.
> I failed to support AS OF clause (as in Oracle) because of
> shift-reduce conflicts with aliases,
> so I have to introduce new ASOF keyword. May be yacc experts can
> propose how to solve this conflict without introducing new keyword...
>
> Please notice that now ASOF timestamp is used only for data snapshot,
> not for catalog snapshot.
> I am not sure that it is possible (and useful) to travel through
> database schema history...
>
> Below is an example of how it works:
>
> postgres=# create table foo(pk serial primary key, ts timestamp
> default now(), val text);
> CREATE TABLE
> postgres=# insert into foo (val) values ('insert');
> INSERT 0 1
> postgres=# insert into foo (val) values ('insert');
> INSERT 0 1
> postgres=# insert into foo (val) values ('insert');
> INSERT 0 1
> postgres=# select * from foo;
>  pk | ts |  val
> ++
>   1 | 2017-12-20 14:59:17.715453 | insert
>   2 | 2017-12-20 14:59:22.933753 | insert
>   3 | 2017-12-20 14:59:27.87712  | insert
> (3 rows)
>
> postgres=# select * from foo asof timestamp '2017-12-20 14:59:25';
>  pk | ts |  val
> ++
>   1 | 2017-12-20 14:59:17.715453 | insert
>   2 | 2017-12-20 14:59:22.933753 | insert
> (2 rows)
>
> postgres=# select * from foo asof timestamp '2017-12-20 14:59:20';
>  pk | ts |  val
> ++
>   1 | 2017-12-20 14:59:17.715453 | insert
> (1 row)
>
> postgres=# update foo set val='upd',ts=now() where pk=1;
> UPDATE 1
> postgres=# select * from foo asof timestamp '2017-12-20 14:59:20';
>  pk | ts |  val
> ++
>   1 | 2017-12-20 14:59:17.715453 | insert
> (1 row)
>
> postgres=# select * from foo;
>  pk | ts |  val
> ++
>   2 | 2017-12-20 14:59:22.933753 | insert
>   3 | 2017-12-20 14:59:27.87712  | insert
>   1 | 2017-12-20 15:09:17.046047 | upd
> (3 rows)
>
> postgres=# update foo set val='upd2',ts=now() where pk=1;
> UPDATE 1
> postgres=# select * from foo asof timestamp '2017-12-20 15:10';
>  pk | ts |  val
> ++
>   2 | 2017-12-20 14:59:22.933753 | insert
>   3 | 2017-12-20 14:59:27.87712  | insert
>   1 | 2017-12-20 15:09:17.046047 | upd
> (3 rows)
>
>
> Comments and feedback are welcome:)
>

-- 
Hannu Krosing
PostgreSQL Consultant
Performance, Scalability and High Availability
https://2ndquadrant.com/

Re: AS OF queries

[Craig Ringer]

On 25 December 2017 at 15:59, Konstantin Knizhnik  wrote:

>
>
> On 25.12.2017 06:26, Craig Ringer wrote:
>
> On 24 December 2017 at 04:53, konstantin knizhnik <
> k.knizh...@postgrespro.ru> wrote:
>
>>
>>
>> But what if I just forbid to change recent_global_xmin?
>> If it is stalled at FirstNormalTransactionId and never changed?
>> Will it protect all versions from been deleted?
>>
>
> That's totally impractical, you'd have unbounded bloat and a nonfunctional
> system in no time.
>
> You'd need a mechanism - akin to what we have with replication slots - to
> set a threshold for age.
>
>
> Well, there are systems with "never delete" and "append only" semantic.
> For example, I have participated in SciDB project: database for scientific
> applications.
> One of the key requirements for scientific researches is reproducibility.
> From the database point of view it means that we need to store all raw
> data and never delete it.
>

PostgreSQL can't cope with that for more than 2^31 xacts, you have to
"forget" details of which xacts created/updated tuples and the contents of
deleted tuples, or you exceed our xid limit. You'd need 64-bit XIDs, or a
redo-buffer based heap model (like the zheap stuff) with redo buffers
marked with an xid epoch, or something like that.


> I am not sure that it should be similar with logical replication slot.
>
Here semantic is quite clear: we preserve segments of WAL until them are
> replicated to the subscribers.
>

Er, what?

This isn't to do with restart_lsn. That's why I mentioned *logical*
replication slots.

I'm talking about how they interact with GetOldestXmin using their xmin and
catalog_xmin.

You probably won't want to re-use slots, but you'll want something akin to
that, a transaction age threshold. Otherwise your system has a finite end
date where it can no longer function due to xid count, or if you solve
that, it'll slowly choke on table bloat etc. I guess if you're willing to
accept truly horrible performance...


> With time travel situation is less obscure: we may specify some threshold
> for age - keep data for example for one year.
>

Sure. You'd likely do that by mapping commit timestamps => xids and using
an xid threshold though.


> But unfortunately trick with snapshot (doesn't matter how we setup oldest
> xmin horizon) affect all tables.
>

You'd need to be able to pass more info into HeapTupleSatisfiesMVCC etc. I
expect you'd probably add a new snapshot type (like logical decoding did
with historic snapshots), that has a new Satisfies function. But you'd have
to be able to ensure all snapshot Satisfies callers had the required extra
info - like maybe a Relation - which could be awkward for some call sites.

The user would have to be responsible for ensuring sanity of FK
relationships etc when specifying different snapshots for different
relations.

Per-relation time travel doesn't seem totally impractical so long as you
can guarantee that there is some possible snapshot for which the catalogs
defining all the relations and types are simultaneously valid, i.e. there's
no disjoint set of catalog changes. Avoiding messy performance implications
with normal queries might not even be too bad if you use a separate
snapshot model, so long as you can avoid callsites having to do extra work
in the normal case.

Dealing with dropped columns and rewrites would be a pain though. You'd
have to preserve the dropped column data when you re-projected the rewrite
tuples.


> There is similar (but not the same) problem with logical replication:
> assume that we need to replicate only one small table. But we have to pin
> in WAL all updates of other huge table which is not involved in logical
> replication at all.
>

I don't really see how that's similar. It's concerned with WAL, wheras what
you're looking at is heaps and bloat from old versions. Completely
different, unless you propose to somehow reconstruct data from old WAL to
do historic queries, which would be o_O ...


> Well, I am really not sure about user's demands to time travel. This is
> one of the reasons of initiating this discussion in hackers... May be it is
> not the best place for such discussion, because there are mostly Postgres
> developers and not users...
> At least, from experience of few SciDB customers, I can tell that we
> didn't have problems with schema evolution: mostly schema is simple, static
> and well defined.
> There was problems with incorrect import of data (this is why we have to
> add real delete), with splitting data in chunks (partitioning),...
>

Every system I've ever worked with that has a "static" schema has landed up
not being so static after all.

I'm sure there are exceptions, but if you can't cope with catalog changes
you've excluded the immense majority of users. Even the ones who promise
they don't ever need to change anything ... land up changing things.


> The question is how we should handle such catalog changes if them are
>> happen. 

Re: AS OF queries

[David Fetter]

On Tue, Dec 26, 2017 at 03:43:36PM -0700, legrand legrand wrote:
> would actual syntax
> 
> WITH old_foo AS
> (select * from foo as of '')
> select * from foo except select * from old_foo;
> 
> work in replacement for
> 
> select * from foo except select * from foo as old_foo as of '';
> 
> ?

If there has to be a WITH, or (roughly) equivalently, a sub-select for
each relation, the queries get very hairy very quickly.  It would
nevertheless be better for the people who need the feature to have it
this way than not to have it at all.

Best,
David.
-- 
David Fetter  http://fetter.org/
Phone: +1 415 235 3778

Remember to vote!
Consider donating to Postgres: http://www.postgresql.org/about/donate

Re: AS OF queries

[legrand legrand]

would actual syntax

WITH old_foo AS
(select * from foo as of '')
select * from foo except select * from old_foo;

work in replacement for

select * from foo except select * from foo as old_foo as of '';

?

Regards
PAscal



--
Sent from: http://www.postgresql-archive.org/PostgreSQL-hackers-f1928748.html

Re: AS OF queries

[Jeff Janes]

On Thu, Dec 21, 2017 at 6:00 AM, Konstantin Knizhnik <
k.knizh...@postgrespro.ru> wrote:


> There is still one significant difference of my prototype implementation
> with SQL standard: it associates timestamp with select statement, not with
> particular table.
> It seems to be more difficult to support and I am not sure that joining
> tables from different timelines has much sense.
> But certainly it also can be fixed.


I think the main use I would find for this feature is something like:

select * from foo except select * from foo as old_foo as of '';

So I would be grateful if you can make that work.  Also, I think conforming
to the standards is pretty important where it is feasible to do that.

Cheers,

Jeff

Re: AS OF queries

[Masahiko Sawada]

On Thu, Dec 21, 2017 at 3:57 AM, Alvaro Hernandez  wrote:
>
>
> On 20/12/17 14:48, Konstantin Knizhnik wrote:
>
>
>
> On 20.12.2017 16:12, Laurenz Albe wrote:
>
> Konstantin Knizhnik wrote:
>
> I wonder if Postgres community is interested in supporting time travel
> queries in PostgreSQL (something like AS OF queries in Oracle:
> https://docs.oracle.com/cd/B14117_01/appdev.101/b10795/adfns_fl.htm).
> As far as I know something similar is now developed for MariaDB.
>
> I think that would be a good thing to have that could make
> the DBA's work easier - all the requests to restore a table
> to the state from an hour ago.
>
>
> Please notice that it is necessary to configure postgres in proper way in
> order to be able to perform time travels.
>
>
> This makes sense. BTW, I believe this feature would be an amazing
> addition to PostgreSQL.
>
>
> If you do not disable autovacuum, then old versions will be just cleaned-up.
> If transaction commit timestamps are not tracked, then it is not possible to
> locate required timeline.
>
> So DBA should make a decision in advance whether this feature is needed or
> not.
> It is not a proper instrument for restoring/auditing existed database which
> was not configured to keep all versions.
>
> May be it is better to add special configuration parameter for this feature
> which should implicitly toggle
> autovacuum and track_commit_timestamp parameters).
>
>
> Downthread a "moving xid horizon" is proposed. I believe this is not too
> user friendly. I'd rather use a timestamp horizon (e.g. "up to 2 days ago").
> Given that the commit timestamp is tracked, I don't think this is an issue.
> This is the same as the undo_retention in Oracle, which is expressed in
> seconds.

I agree but since we cannot have same xid beyond xid wraparounds we
would have to remove old tuples even if we're still in the time
interval

Regards,

--
Masahiko Sawada
NIPPON TELEGRAPH AND TELEPHONE CORPORATION
NTT Open Source Software Center

Re: AS OF queries

[Konstantin Knizhnik]

On 25.12.2017 06:26, Craig Ringer wrote:
On 24 December 2017 at 04:53, konstantin knizhnik 
> wrote:




But what if I just forbid to change recent_global_xmin?
If it is stalled at FirstNormalTransactionId and never changed?
Will it protect all versions from been deleted?


That's totally impractical, you'd have unbounded bloat and a 
nonfunctional system in no time.


You'd need a mechanism - akin to what we have with replication slots - 
to set a threshold for age.


Well, there are systems with "never delete" and "append only" semantic.
For example, I have participated in SciDB project: database for 
scientific applications.

One of the key requirements for scientific researches is reproducibility.
From the database point of view it means that we need to store all raw 
data and never delete it.
If you performed some measurements and made some conclusions based on 
this results, then everybody should be able to repeat it, even if later
you find some errors in input data and made corrections or just add more 
data.
So one of the SciDB requirements was to store all versions. Delete 
operation should just mark data as been deleted (although later we have 
to add true delete:)


But I agree with you: in most cases more flexible policy of managing 
versions is needed.

I am not sure that it should be similar with logical replication slot.
Here semantic is quite clear: we preserve segments of WAL until them are 
replicated to the subscribers.
With time travel situation is less obscure: we may specify some 
threshold for age - keep data for example for one year.
But what if somebody later wants to access  older data? At this moment 
them are already lost...


It seems to me that version pinning policy mostly depends on source of 
the data.
If  them have "append only" semantic (like as raw scientific data, 
trading data, measurements from IoT sensors...)

then it will be desirable to keep all version forever.
If we speak about OLTP tables (like accounts in pgbench), then may be 
time travel is not the proper mechanism for such data at all.


I think that in addition to logged/unlogged tables it will be useful to 
support historical/non-historical tables. Historical table should 
support time travel, while
non-historical (default) acts like normal table. It is already possible 
in Postgres to disable autovacuum for particular tables.
But unfortunately trick with snapshot (doesn't matter how we setup 
oldest xmin horizon) affect all tables.
There is similar (but not the same) problem with logical replication: 
assume that we need to replicate only one small table. But we have to 
pin in WAL all updates of other huge table which is not involved in 
logical replication at all.





> Then there's another issue that logical replication has had to deal
> with -- catalog changes. You can't start looking at tuples that
have a
> different structure than the current catalog unless you can
figure out
> how to use the logical replication infrastructure to use the old
> catalogs. That's a huge problem to bite off and probably can just be
> left for another day if you can find a way to reliably detect the
> problem and raise an error if the schema is inconsistent.


Yes, catalog changes this is another problem of time travel.
I do not know any suitable way to handle several different catalog
snapshots in one query.


I doubt it's practical unless you can extract it to subplans that can 
be materialized separately. Even then, UDTs, rowtype results, etc...


Well, I am really not sure about user's demands to time travel. This is 
one of the reasons of initiating this discussion in hackers... May be it 
is not the best place for such discussion, because there are mostly 
Postgres developers and not users...
At least, from experience of few SciDB customers, I can tell that we 
didn't have problems with schema evolution: mostly schema is simple, 
static and well defined.
There was problems with incorrect import of data (this is why we have to 
add real delete), with splitting data in chunks (partitioning),...



The question is how we should handle such catalog changes if them
are happen. Ideally we should not allow to move back beyond  this
point.
Unfortunately it is not so easy to implement.


I think you can learn a lot from studying logical decoding here.


Working with multimaster and shardman I have to learn a lot about 
logical replication.
It is really powerful and flexible mechanism ... with a lot of 
limitations and problems: lack of catalog replication, inefficient bulk 
insert, various race conditions,...
But I think that time travel and logical replication are really serving 
different goals so require different approaches.





--
 Craig Ringer http://www.2ndQuadrant.com/
 PostgreSQL Development, 24x7 Support, Training & Services


--
Konstantin Knizhnik
Postgres Professional: 

Re: AS OF queries

[konstantin knizhnik]

On Dec 23, 2017, at 2:08 AM, Greg Stark wrote:

> On 20 December 2017 at 12:45, Konstantin Knizhnik
>  wrote:
> 
>> It seems to me that it will be not so difficult to implement them in
>> Postgres - we already have versions of tuples.
>> Looks like we only need to do three things:
>> 1. Disable autovacuum (autovacuum = off)
> 
> "The Wheel of Time turns, and Ages come and pass, leaving memories
> that become legend. Legend fades to myth, and even myth is long
> forgotten when the Age that gave it birth comes again"
> 
> I think you'll find it a lot harder to get this to work than just
> disabling autovacuum. Notably HOT updates can get cleaned up (and even
> non-HOT updates can now leave tombstone dead line pointers iirc) even
> if vacuum hasn't run.
> 

Yeh, I suspected that just disabling autovacuum was not enough.
I heard (but do no know too much) about microvacuum and hot updates.
This is why I was a little bit surprised when me test didn't show lost of 
updated versions.
May be it is because of vacuum_defer_cleanup_age.

> We do have the infrastructure to deal with that. c.f.
> vacuum_defer_cleanup_age. So in _theory_ you could create a snapshot
> with xmin older than recent_global_xmin as long as it's not more than
> vacuum_defer_cleanup_age older. But the devil will be in the details.
> It does mean that you'll be making recent_global_xmin move backwards
> which it has always been promised to *not* do

But what if I just forbid to change recent_global_xmin?
If it is stalled at FirstNormalTransactionId and never changed?
Will it protect all versions from been deleted?

> 
> Then there's another issue that logical replication has had to deal
> with -- catalog changes. You can't start looking at tuples that have a
> different structure than the current catalog unless you can figure out
> how to use the logical replication infrastructure to use the old
> catalogs. That's a huge problem to bite off and probably can just be
> left for another day if you can find a way to reliably detect the
> problem and raise an error if the schema is inconsistent.


Yes, catalog changes this is another problem of time travel.
I do not know any suitable way to handle several different catalog snapshots in 
one query.
But I think that there are a lot of cases where time travels without 
possibility of database schema change still will be useful.
The question is how we should handle such catalog changes if them are happen. 
Ideally we should not allow to move back beyond  this point.
Unfortunately it is not so easy to implement.


> 
> Postgres used to have time travel. I think it's come up more than once
> in the pasts as something that can probably never come back due to
> other decisions made. If more decisions have made it possible again
> that will be fascinating.
> 
> -- 
> greg

Re: AS OF queries

[Greg Stark]

On 20 December 2017 at 12:45, Konstantin Knizhnik
 wrote:

> It seems to me that it will be not so difficult to implement them in
> Postgres - we already have versions of tuples.
> Looks like we only need to do three things:
> 1. Disable autovacuum (autovacuum = off)

"The Wheel of Time turns, and Ages come and pass, leaving memories
that become legend. Legend fades to myth, and even myth is long
forgotten when the Age that gave it birth comes again"

I think you'll find it a lot harder to get this to work than just
disabling autovacuum. Notably HOT updates can get cleaned up (and even
non-HOT updates can now leave tombstone dead line pointers iirc) even
if vacuum hasn't run.

We do have the infrastructure to deal with that. c.f.
vacuum_defer_cleanup_age. So in _theory_ you could create a snapshot
with xmin older than recent_global_xmin as long as it's not more than
vacuum_defer_cleanup_age older. But the devil will be in the details.
It does mean that you'll be making recent_global_xmin move backwards
which it has always been promised to *not* do

Then there's another issue that logical replication has had to deal
with -- catalog changes. You can't start looking at tuples that have a
different structure than the current catalog unless you can figure out
how to use the logical replication infrastructure to use the old
catalogs. That's a huge problem to bite off and probably can just be
left for another day if you can find a way to reliably detect the
problem and raise an error if the schema is inconsistent.

Postgres used to have time travel. I think it's come up more than once
in the pasts as something that can probably never come back due to
other decisions made. If more decisions have made it possible again
that will be fascinating.

-- 
greg

Re: AS OF queries

[David Fetter]

On Thu, Dec 21, 2017 at 05:00:35PM +0300, Konstantin Knizhnik wrote:
> On 20.12.2017 19:26, Tom Lane wrote:
> >Peter Eisentraut  writes:
> >>On 12/20/17 10:29, Tom Lane wrote:
> >>>Please say that's just an Oracle-ism and not SQL standard, because it's
> >>>formally ambiguous.
> >>The SQL standard syntax appears to be something like
> >>"tablename" [ AS OF SYSTEM TIME 'something' ] [ [ AS ] "alias" ]
> >>That's not going to be fun to parse.
> >Bleah.  In principle we could look two tokens ahead so as to recognize
> >"AS OF SYSTEM", but base_yylex is already a horrid mess with one-token
> >lookahead; I don't much want to try to extend it to that.
> >
> >Possibly the most workable compromise is to use lookahead to convert
> >"AS OF" to "AS_LA OF", and then we could either just break using OF
> >as an alias, or add an extra production that allows "AS_LA OF" to
> >be treated as "AS alias" if it's not followed by the appropriate
> >stuff.
> >
> >It's a shame that the SQL committee appears to be so ignorant of
> >standard parsing technology.
> >
> > regards, tom lane
> 
> Thank you for suggestion with AS_LA: it really works.
> Actually instead of AS_LA I just return ASOF token if next token after AS is
> OF.
> So now it is possible to write query in this way:
> 
>     select * from foo as of timestamp '2017-12-21 14:12:15.1867';

Thanks for your hard work so far on this!  It looks really exciting.

> There is still one significant difference of my prototype implementation
> with SQL standard: it associates timestamp with select statement, not with
> particular table.
> It seems to be more difficult to support and I am not sure that joining
> tables from different timelines has much sense.

I can think of a use case right offhand that I suspect would be very
common: comparing the state of a table at multiple times.

> But certainly it also can be fixed.

That would be really fantastic.

Best,
David.
-- 
David Fetter  http://fetter.org/
Phone: +1 415 235 3778

Remember to vote!
Consider donating to Postgres: http://www.postgresql.org/about/donate

Re: AS OF queries

[Konstantin Knizhnik]

On 20.12.2017 19:26, Tom Lane wrote:

Peter Eisentraut  writes:

On 12/20/17 10:29, Tom Lane wrote:

Please say that's just an Oracle-ism and not SQL standard, because it's
formally ambiguous.

The SQL standard syntax appears to be something like
"tablename" [ AS OF SYSTEM TIME 'something' ] [ [ AS ] "alias" ]
That's not going to be fun to parse.

Bleah.  In principle we could look two tokens ahead so as to recognize
"AS OF SYSTEM", but base_yylex is already a horrid mess with one-token
lookahead; I don't much want to try to extend it to that.

Possibly the most workable compromise is to use lookahead to convert
"AS OF" to "AS_LA OF", and then we could either just break using OF
as an alias, or add an extra production that allows "AS_LA OF" to
be treated as "AS alias" if it's not followed by the appropriate
stuff.

It's a shame that the SQL committee appears to be so ignorant of
standard parsing technology.

regards, tom lane


Thank you for suggestion with AS_LA: it really works.
Actually instead of AS_LA I just return ASOF token if next token after 
AS is OF.

So now it is possible to write query in this way:

    select * from foo as of timestamp '2017-12-21 14:12:15.1867';

There is still one significant difference of my prototype implementation 
with SQL standard: it associates timestamp with select statement, not 
with particular table.
It seems to be more difficult to support and I am not sure that joining 
tables from different timelines has much sense.

But certainly it also can be fixed.

--
Konstantin Knizhnik
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company

diff --git a/contrib/pg_stat_statements/pg_stat_statements.c b/contrib/pg_stat_statements/pg_stat_statements.c
index 3de8333..2126847 100644
--- a/contrib/pg_stat_statements/pg_stat_statements.c
+++ b/contrib/pg_stat_statements/pg_stat_statements.c
@@ -2353,6 +2353,7 @@ JumbleQuery(pgssJumbleState *jstate, Query *query)
 	JumbleExpr(jstate, (Node *) query->sortClause);
 	JumbleExpr(jstate, query->limitOffset);
 	JumbleExpr(jstate, query->limitCount);
+	JumbleExpr(jstate, query->asofTimestamp);
 	/* we ignore rowMarks */
 	JumbleExpr(jstate, query->setOperations);
 }
diff --git a/src/backend/executor/Makefile b/src/backend/executor/Makefile
index cc09895..d2e0799 100644
--- a/src/backend/executor/Makefile
+++ b/src/backend/executor/Makefile
@@ -29,6 +29,6 @@ OBJS = execAmi.o execCurrent.o execExpr.o execExprInterp.o \
nodeCtescan.o nodeNamedtuplestorescan.o nodeWorktablescan.o \
nodeGroup.o nodeSubplan.o nodeSubqueryscan.o nodeTidscan.o \
nodeForeignscan.o nodeWindowAgg.o tstoreReceiver.o tqueue.o spi.o \
-   nodeTableFuncscan.o
+   nodeTableFuncscan.o nodeAsof.o
 
 include $(top_srcdir)/src/backend/common.mk
diff --git a/src/backend/executor/execAmi.c b/src/backend/executor/execAmi.c
index f1636a5..38c79b8 100644
--- a/src/backend/executor/execAmi.c
+++ b/src/backend/executor/execAmi.c
@@ -285,6 +285,10 @@ ExecReScan(PlanState *node)
 			ExecReScanLimit((LimitState *) node);
 			break;
 
+		case T_AsofState:
+			ExecReScanAsof((AsofState *) node);
+			break;
+
 		default:
 			elog(ERROR, "unrecognized node type: %d", (int) nodeTag(node));
 			break;
diff --git a/src/backend/executor/execCurrent.c b/src/backend/executor/execCurrent.c
index a3e962e..1912ae4 100644
--- a/src/backend/executor/execCurrent.c
+++ b/src/backend/executor/execCurrent.c
@@ -329,6 +329,7 @@ search_plan_tree(PlanState *node, Oid table_oid)
 			 */
 		case T_ResultState:
 		case T_LimitState:
+		case T_AsofState:
 			return search_plan_tree(node->lefttree, table_oid);
 
 			/*
diff --git a/src/backend/executor/execProcnode.c b/src/backend/executor/execProcnode.c
index fcb8b56..586b5b3 100644
--- a/src/backend/executor/execProcnode.c
+++ b/src/backend/executor/execProcnode.c
@@ -75,6 +75,7 @@
 #include "executor/executor.h"
 #include "executor/nodeAgg.h"
 #include "executor/nodeAppend.h"
+#include "executor/nodeAsof.h"
 #include "executor/nodeBitmapAnd.h"
 #include "executor/nodeBitmapHeapscan.h"
 #include "executor/nodeBitmapIndexscan.h"
@@ -364,6 +365,11 @@ ExecInitNode(Plan *node, EState *estate, int eflags)
  estate, eflags);
 			break;
 
+		case T_Asof:
+			result = (PlanState *) ExecInitAsof((Asof *) node,
+estate, eflags);
+			break;
+
 		default:
 			elog(ERROR, "unrecognized node type: %d", (int) nodeTag(node));
 			result = NULL;		/* keep compiler quiet */
@@ -727,6 +733,10 @@ ExecEndNode(PlanState *node)
 			ExecEndLimit((LimitState *) node);
 			break;
 
+		case T_AsofState:
+			ExecEndAsof((AsofState *) node);
+			break;
+
 		default:
 			elog(ERROR, "unrecognized node type: %d", (int) nodeTag(node));
 			break;
diff --git a/src/backend/executor/nodeAsof.c b/src/backend/executor/nodeAsof.c
new file mode 100644
index 000..8957a91
--- /dev/null
+++ b/src/backend/executor/nodeAsof.c
@@ -0,0 +1,157 @@

Re: AS OF queries

[Craig Ringer]

On 21 December 2017 at 00:17, Peter Eisentraut <
peter.eisentr...@2ndquadrant.com> wrote:

> On 12/20/17 10:29, Tom Lane wrote:
> > Please say that's just an Oracle-ism and not SQL standard, because it's
> > formally ambiguous.  This is required to work by spec:
> >
> > regression=# select x as of from (values(1)) t(x);
> >  of
> > 
> >   1
> > (1 row)
> >
> > so it's not possible for us ever to support an expression that includes
> > top-level "AS OF" (or, pretty much, "AS anything") without some rather
> > enormous pushups.
>
> The SQL standard syntax appears to be something like
>
> "tablename" [ AS OF SYSTEM TIME 'something' ] [ [ AS ] "alias" ]
>
> That's not going to be fun to parse.


Well, the SQL committe seem to specialise in parser torture.

Window functions, anybody?

-- 
 Craig Ringer   http://www.2ndQuadrant.com/
 PostgreSQL Development, 24x7 Support, Training & Services

Re: AS OF queries

[Alvaro Hernandez]

On 20/12/17 14:48, Konstantin Knizhnik wrote:



On 20.12.2017 16:12, Laurenz Albe wrote:

Konstantin Knizhnik wrote:

I wonder if Postgres community is interested in supporting time travel
queries in PostgreSQL (something like AS OF queries in Oracle:
https://docs.oracle.com/cd/B14117_01/appdev.101/b10795/adfns_fl.htm).
As far as I know something similar is now developed for MariaDB.

I think that would be a good thing to have that could make
the DBA's work easier - all the requests to restore a table
to the state from an hour ago.


Please notice that it is necessary to configure postgres in proper way 
in order to be able to perform time travels.


    This makes sense. BTW, I believe this feature would be an amazing 
addition to PostgreSQL.



If you do not disable autovacuum, then old versions will be just 
cleaned-up.
If transaction commit timestamps are not tracked, then it is not 
possible to locate required timeline.


So DBA should make a decision in advance whether this feature is 
needed or not.
It is not a proper instrument for restoring/auditing existed database 
which was not configured to keep all versions.


May be it is better to add special configuration parameter for this 
feature which should implicitly toggle

autovacuumand track_commit_timestamp parameters).


    Downthread a "moving xid horizon" is proposed. I believe this is 
not too user friendly. I'd rather use a timestamp horizon (e.g. "up to 2 
days ago"). Given that the commit timestamp is tracked, I don't think 
this is an issue. This is the same as the undo_retention in Oracle, 
which is expressed in seconds.





The obvious drawbacks of keeping all versions are
1. Increased size of database.
2. Decreased query execution speed because them need to traverse a lot 
of not visible versions.


    In other words, what is nowadays called "bloat". I have seen in the 
field a lot of it. Not everybody tunes vacuum to keep up to date. So I 
don't expect this feature to be too expensive for many. While at the 
same time an awesome addition, not to fire a new separate server and 
exercise PITR, and then find the ways to move the old data around.



    Regards,

    Álvaro


--

Alvaro Hernandez


---
OnGres

Re: AS OF queries

[Pantelis Theodosiou]

On Wed, Dec 20, 2017 at 4:26 PM, Tom Lane  wrote:

> Peter Eisentraut  writes:
> > On 12/20/17 10:29, Tom Lane wrote:
> >> Please say that's just an Oracle-ism and not SQL standard, because it's
> >> formally ambiguous.
>
> > The SQL standard syntax appears to be something like
>
> > "tablename" [ AS OF SYSTEM TIME 'something' ] [ [ AS ] "alias" ]
>
> > That's not going to be fun to parse.
>

Examples from DB2 documentation (which may be closer to the standard):

SELECT coverage_amt
FROM policy FOR SYSTEM_TIME AS OF '2010-12-01'
WHERE id = ;


SELECT count(*)
FROM policy FOR SYSTEM_TIME FROM '2011-11-30'
  TO '-12-30'
WHERE vin = 'A';


So besides AS .. AS , it could also be  FROM .. FROM


> Bleah.  In principle we could look two tokens ahead so as to recognize
> "AS OF SYSTEM", but base_yylex is already a horrid mess with one-token
> lookahead; I don't much want to try to extend it to that.
>
> Possibly the most workable compromise is to use lookahead to convert
> "AS OF" to "AS_LA OF", and then we could either just break using OF
> as an alias, or add an extra production that allows "AS_LA OF" to
> be treated as "AS alias" if it's not followed by the appropriate
> stuff.
>
> It's a shame that the SQL committee appears to be so ignorant of
> standard parsing technology.
>
> regards, tom lane
>
>

Re: AS OF queries

[Tom Lane]

Peter Eisentraut  writes:
> On 12/20/17 10:29, Tom Lane wrote:
>> Please say that's just an Oracle-ism and not SQL standard, because it's
>> formally ambiguous.

> The SQL standard syntax appears to be something like

> "tablename" [ AS OF SYSTEM TIME 'something' ] [ [ AS ] "alias" ]

> That's not going to be fun to parse.

Bleah.  In principle we could look two tokens ahead so as to recognize
"AS OF SYSTEM", but base_yylex is already a horrid mess with one-token
lookahead; I don't much want to try to extend it to that.

Possibly the most workable compromise is to use lookahead to convert
"AS OF" to "AS_LA OF", and then we could either just break using OF
as an alias, or add an extra production that allows "AS_LA OF" to
be treated as "AS alias" if it's not followed by the appropriate
stuff.

It's a shame that the SQL committee appears to be so ignorant of
standard parsing technology.

regards, tom lane

Re: AS OF queries

[Magnus Hagander]

On Wed, Dec 20, 2017 at 5:17 PM, Peter Eisentraut <
peter.eisentr...@2ndquadrant.com> wrote:

> On 12/20/17 10:29, Tom Lane wrote:
> > Please say that's just an Oracle-ism and not SQL standard, because it's
> > formally ambiguous.  This is required to work by spec:
> >
> > regression=# select x as of from (values(1)) t(x);
> >  of
> > 
> >   1
> > (1 row)
> >
> > so it's not possible for us ever to support an expression that includes
> > top-level "AS OF" (or, pretty much, "AS anything") without some rather
> > enormous pushups.
>
> The SQL standard syntax appears to be something like
>
> "tablename" [ AS OF SYSTEM TIME 'something' ] [ [ AS ] "alias" ]
>
> That's not going to be fun to parse.
>

There was a presentation about this given at FOSDEM PGDay a couple of years
back. Slides at
https://wiki.postgresql.org/images/6/64/Fosdem20150130PostgresqlTemporal.pdf
.


-- 
 Magnus Hagander
 Me: https://www.hagander.net/ 
 Work: https://www.redpill-linpro.com/ 

Re: AS OF queries

[Peter Eisentraut]

On 12/20/17 10:29, Tom Lane wrote:
> Please say that's just an Oracle-ism and not SQL standard, because it's
> formally ambiguous.  This is required to work by spec:
> 
> regression=# select x as of from (values(1)) t(x);
>  of 
> 
>   1
> (1 row)
> 
> so it's not possible for us ever to support an expression that includes
> top-level "AS OF" (or, pretty much, "AS anything") without some rather
> enormous pushups.

The SQL standard syntax appears to be something like

"tablename" [ AS OF SYSTEM TIME 'something' ] [ [ AS ] "alias" ]

That's not going to be fun to parse.

-- 
Peter Eisentraut  http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Re: AS OF queries

[David Fetter]

On Wed, Dec 20, 2017 at 03:03:50PM +0100, Laurenz Albe wrote:
> Konstantin Knizhnik wrote:
> > Please notice that it is necessary to configure postgres in proper
> > way in order to be able to perform time travels.  If you do not
> > disable autovacuum, then old versions will be just cleaned-up.  If
> > transaction commit timestamps are not tracked, then it is not
> > possible to locate required timeline. 
> > 
> > So DBA should make a decision in advance whether this feature is
> > needed or not.  It is not a proper instrument for
> > restoring/auditing existed database which was not configured to
> > keep all versions.
> 
> Of course; you'd have to anticipate the need to travel in time, and
> you have to pay the price for it.  Anybody who has read science
> fiction stories know that time travel does not come free.

A few extra terabytes' worth of storage space is a pretty small price
to pay, at least on the scale of time travel penalties.

> > May be it is better to add special configuration parameter for
> > this feature which should implicitly toggle autovacuum and
> > track_commit_timestamp parameters).
> 
> The feature would be most useful with some kind of "moving xid
> horizon" that guarantees that only dead tuples whose xmax lies more
> than a certain time interval in the past can be vacuumed.

+1 for this horizon.  It would be very nice, but maybe not strictly
necessary, for this to be adjustable downward without a restart.

It's not clear that adjusting it upward should work at all, but if it
did, the state of dead tuples would have to be known, and they'd have
to be vacuumed a way that was able to establish a guarantee of
gaplessness at least back to the new horizon.  Maybe there could be
some kind of "high water mark" for it.  Would that impose overhead or
design constraints on vacuum that we don't want?

Also nice but not strictly necessary, making it tunable per relation,
or at least per table.  I'm up in the air as to whether queries with
an AS OF older than the horizon[1] should error out or merely throw
warnings.

Best,
David.

[1] If we allow setting this at granularities coarser than DB
instance, this means going as far back as the relationship with the
newest "last" tuple among the relations involved in the query.
-- 
David Fetter  http://fetter.org/
Phone: +1 415 235 3778

Remember to vote!
Consider donating to Postgres: http://www.postgresql.org/about/donate

Re: AS OF queries

[Tom Lane]

Laurenz Albe  writes:
> Konstantin Knizhnik wrote:
>> I failed to support AS OF clause (as in Oracle) because of shift-reduce 
>> conflicts with aliases,
>> so I have to introduce new ASOF keyword. May be yacc experts can propose 
>> how to solve this conflict without introducing new keyword...

> I think it would be highly desirable to have AS OF, because that's
> the way the SQL standard has it.

Please say that's just an Oracle-ism and not SQL standard, because it's
formally ambiguous.  This is required to work by spec:

regression=# select x as of from (values(1)) t(x);
 of 

  1
(1 row)

so it's not possible for us ever to support an expression that includes
top-level "AS OF" (or, pretty much, "AS anything") without some rather
enormous pushups.

If we absolutely had to do it, the path to a solution would involve some
lexer-level lookahead, cf base_yylex() --- but that's messy and tends to
introduce its own set of corner case misbehaviors.  I'd much rather use a
syntax that wasn't chosen with blind disregard for SQL's existing
syntactic constraints.

regards, tom lane

Re: AS OF queries

[Laurenz Albe]

Konstantin Knizhnik wrote:
> Please notice that it is necessary to configure postgres in proper way in 
> order to be able to perform time travels.
> If you do not disable autovacuum, then old versions will be just cleaned-up.
> If transaction commit timestamps are not tracked, then it is not possible to 
> locate required timeline. 
> 
> So DBA should make a decision in advance whether this feature is needed or 
> not.
> It is not a proper instrument for restoring/auditing existed database which 
> was not configured to keep all versions.

Of course; you'd have to anticipate the need to travel in time,
and you have to pay the price for it.
Anybody who has read science fiction stories know that time travel
does not come free.

> May be it is better to add special configuration parameter for this feature 
> which should implicitly toggle 
> autovacuum and track_commit_timestamp parameters).

The feature would be most useful with some kind of "moving xid
horizon" that guarantees that only dead tuples whose xmax lies
more than a certain time interval in the past can be vacuumed.

Yours,
Laurenz Albe

Re: AS OF queries

[Joe Wildish]

On 20 Dec 2017, at 13:48, Konstantin Knizhnik  wrote:
> 
> On 20.12.2017 16:12, Laurenz Albe wrote:
>> Konstantin Knizhnik wrote:
>>> I wonder if Postgres community is interested in supporting time travel 
>>> queries in PostgreSQL (something like AS OF queries in Oracle: 
>>> https://docs.oracle.com/cd/B14117_01/appdev.101/b10795/adfns_fl.htm 
>>> ).
>>> As far as I know something similar is now developed for MariaDB.
>> I think that would be a good thing to have that could make
>> the DBA's work easier - all the requests to restore a table
>> to the state from an hour ago.
> 
> Please notice that it is necessary to configure postgres in proper way in 
> order to be able to perform time travels.
> If you do not disable autovacuum, then old versions will be just cleaned-up.
> If transaction commit timestamps are not tracked, then it is not possible to 
> locate required timeline. 
> 
> So DBA should make a decision in advance whether this feature is needed or 
> not.
> It is not a proper instrument for restoring/auditing existed database which 
> was not configured to keep all versions.
> 
> May be it is better to add special configuration parameter for this feature 
> which should implicitly toggle 
> autovacuum and track_commit_timestamp parameters).
> 


I seem to recall that Oracle handles this by requiring tables that want the 
capability to live within a tablespace that supports flashback. That tablespace 
is obviously configured to retain redo/undo logs. It would be nice if the 
vacuuming process could be configured in a similar manner. I have no idea if it 
would make sense on a tablespace basis or not, though — I’m not entirely sure 
how analogous they are between Postgres & Oracle as I’ve never used tablespaces 
in Postgres.

-Joe

Re: AS OF queries

[Laurenz Albe]

Konstantin Knizhnik wrote:
> I wonder if Postgres community is interested in supporting time travel 
> queries in PostgreSQL (something like AS OF queries in Oracle: 
> https://docs.oracle.com/cd/B14117_01/appdev.101/b10795/adfns_fl.htm).
> As far as I know something similar is now developed for MariaDB.

I think that would be a good thing to have that could make
the DBA's work easier - all the requests to restore a table
to the state from an hour ago.

> I failed to support AS OF clause (as in Oracle) because of shift-reduce 
> conflicts with aliases,
> so I have to introduce new ASOF keyword. May be yacc experts can propose 
> how to solve this conflict without introducing new keyword...

I think it would be highly desirable to have AS OF, because that's
the way the SQL standard has it.

Yours,
Laurenz Albe

Re: [HACKERS] parallelize queries containing initplans

[Amit Kapila]

On Thu, Nov 16, 2017 at 10:44 PM, Robert Haas  wrote:
> On Thu, Nov 16, 2017 at 5:23 AM, Kuntal Ghosh
>  wrote:
>> I've tested the above-mentioned scenario with this patch and it is
>> working fine. Also, I've created a text column named 'vartext',
>> inserted some random length texts(max length 100) and tweaked the
>> above query as follows:
>> select ten,count(*) from tenk1 a where a.ten in (select
>> b.ten from tenk1 b where (select a.vartext from tenk1 c where c.ten =
>> a.ten limit 1) = b.vartext limit 1) group by a.ten;
>> This query is equivalent to select ten,count(*) from tenk1 group by
>> a.ten. It also produced the expected result without throwing any
>> error.
>
> Great!  I have committed the patch; thanks for testing.
>

Thanks.

> As I said in the commit message, there's a lot more work that could be
> done here.  I think we should consider trying to revise this whole
> system so that instead of serializing the values and passing them to
> the workers, we allocate an array of slots where each slot has a Datum
> flag, an isnull flag, and a dsa_pointer (which maybe could be union'd
> to the Datum?).  If we're passing a parameter by value, we could just
> store it in the Datum field; if it's null, we just set isnull.  If
> it's being passed by reference, we dsa_allocate() space for it, copy
> it into that space, and then store the dsa_pointer.
>
> The advantage of this is that it would be possible to imagine the
> contents of a slot changing while parallelism is running, which
> doesn't really work with the current serialized-blob representation.
> That would in turn allow us to imagine letting parallel-safe InitPlans
> being evaluated by the first participant that needs the value rather
> than before launching workers, which would be good, not only because
> of the possibility of deferring work for InitPlans attached at or
> above the Gather but also because it could be used for InitPlans below
> the Gather (as long as they don't depend on any parameters computed
> below the Gather).
>

That would be cool, but I think here finding whether it is dependent
on any parameter computed below gather could be tricky.

-- 
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com

Re: [HACKERS] parallelize queries containing initplans

[Kuntal Ghosh]

On Thu, Nov 16, 2017 at 3:34 PM, Amit Kapila  wrote:
> On Wed, Nov 15, 2017 at 12:25 AM, Robert Haas  wrote:
>> On Tue, Nov 14, 2017 at 11:00 AM, Tom Lane  wrote:
>>> Yeah, I'm sure it is.  I have a vague recollection that there might be
>>> existing regression test cases exercising such things, though I won't
>>> swear to that.  The "orderstest" bit in subselect.sql looks like it
>>> might be meant to do that...
>>
>
> I agree that such cases can cause a problem with fixed memory.
>
I'm able to come up with a test case to produce the problem.

regression[107494]=# select ten,count(*) from tenk1 a where a.ten in (select
b.ten from tenk1 b where (select a.ten from tenk1 c where c.ten =
a.ten limit 1) = b.ten limit 1) group by a.ten;

QUERY PLAN
---
HashAggregate
  Group Key: a.ten
  ->  Seq Scan on tenk1 a
Filter: (SubPlan 2)
SubPlan 2
  ->  Limit
InitPlan 1 (returns $1)
  ->  Limit
->  Seq Scan on tenk1 c
  Filter: (ten = a.ten)
->  Seq Scan on tenk1 b
  Filter: ($1 = ten)
In the above case, Subplan 2 returns the same value of a.ten that is
used in initplan as a reference. So, this query is nothing but select
ten,count(*) from tenk1 group by a.ten. The output should be 10 rows
each row having a count=1000.

By enforcing parallelism, I got the following plan:
  QUERY PLAN
--
HashAggregate
  Group Key: a.ten
  ->  Seq Scan on tenk1 a
Filter: (SubPlan 2)
SubPlan 2
  ->  Limit
InitPlan 1 (returns $1)
  ->  Limit
->  Seq Scan on tenk1 c
  Filter: (ten = a.ten)
->  Gather
  Workers Planned: 2
  Params Evaluated: $1
  ->  Parallel Seq Scan on tenk1 b
Filter: ($1 = ten)

When I set parallel_leader_participation to off, I get the incorrect
result as follows:
 ten | count

   0  | 1000
(1 row)
In the above case, the initplan gets evaluated only once from
ExecInitParallelPlan path. Hence, we see the incorrect result.

>> Here's an updated patch that attempts to work around this problem using DSA.
>>
>
> There were a couple of problems with your changes:
> 1.
> BufferUsage *buffer_usage; /* points to bufusage area in DSM */
> + dsa_handle param_exec; /* serialized PARAM_EXEC parameters */
> @@ -35,12 +36,13 @@ typedef struct ParallelExecutorInfo
>  } ParallelExecutorInfo;
>
> This should be dsa_pointer, otherwise, the value returned by
> SerializeParamExecParams will get truncated.
>
> 2. In ExecParallelReinitialize(), we need to evaluate the params
> before serializing them.
>
> 3. I think we should free the dsa pointer at the end of the gather.
>
> Attached patch fixes the mentioned problems.
>
>> It could use a test case that actually tickles the new logic in
>> ExecParallelReinitialize ... this is mostly just to show the concept.
>>
>
> Thanks, it was quite helpful.
>
I've tested the above-mentioned scenario with this patch and it is
working fine. Also, I've created a text column named 'vartext',
inserted some random length texts(max length 100) and tweaked the
above query as follows:
select ten,count(*) from tenk1 a where a.ten in (select
b.ten from tenk1 b where (select a.vartext from tenk1 c where c.ten =
a.ten limit 1) = b.vartext limit 1) group by a.ten;
This query is equivalent to select ten,count(*) from tenk1 group by
a.ten. It also produced the expected result without throwing any
error.


-- 
Thanks & Regards,
Kuntal Ghosh
EnterpriseDB: http://www.enterprisedb.com

Re: [HACKERS] parallelize queries containing initplans

[Robert Haas]

On Tue, Nov 14, 2017 at 10:37 AM, Robert Haas  wrote:
> The problem would happen if the plan for InitPlan $1 in the above
> example itself referenced a parameter from an upper query level, and
> the value of that parameter changed, and then this section of the plan
> tree was rescanned.  I'm not sure I can write a query like that
> off-hand, but I think it's possible to do so.

OK, here's an example:

regression=# explain select * from tenk1 a where unique1 = (select
unique1 from tenk1 b where (select string4 from tenk1 c where c.ten =
a.ten order by unique1 limit 1) < b.string4 limit 1);
QUERY PLAN
--
 Seq Scan on tenk1 a  (cost=0.00..22051.31 rows=50 width=244)
   Filter: (unique1 = (SubPlan 2))
   SubPlan 2
 ->  Limit  (cost=2.01..2.16 rows=1 width=4)
   InitPlan 1 (returns $1)
 ->  Limit  (cost=0.29..2.01 rows=1 width=68)
   ->  Index Scan using tenk1_unique1 on tenk1 c
(cost=0.29..1727.20 rows=1000 width=68)
 Filter: (ten = a.ten)
   ->  Seq Scan on tenk1 b  (cost=0.00..483.00 rows= width=4)
 Filter: ($1 < string4)
(10 rows)

InitPlan 1 has got to be re-evaluated for every row in the "Seq Scan
on tenk1 a", and each iteration could return a different value for $1,
and some of those values might be wider than others -- well, not
really, because in this example string4 is actually declared as type
"name".  But if you imagine it as type "text" then you can see the
problem.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Re: [HACKERS] parallelize queries containing initplans

[Robert Haas]

On Tue, Nov 14, 2017 at 12:00 AM, Amit Kapila  wrote:
> I think this would have been a matter of concern if we use initplans
> below Gather/Gather Merge.  The patch uses initplans which are between
> current query level and root.  So, I think we don't need to reevaluate
> such parameters.  Let us try to see it via example:
>
>
>QUERY PLAN
> 
>  Aggregate  (cost=62.08..62.08 rows=1 width=8)
>InitPlan 1 (returns $1)
>  ->  Finalize Aggregate  (cost=20.64..20.65 rows=1 width=8)
>->  Gather  (cost=20.63..20.64 rows=2 width=8)
>  Workers Planned: 2
>  ->  Partial Aggregate  (cost=20.63..20.64 rows=1 width=8)
>->  Parallel Seq Scan on t3  (cost=0.00..18.50
> rows=850 width=4)
>->  Hash Join  (cost=20.75..41.42 rows=1 width=4)
>  Hash Cond: (t1.j = t2.j)
>  ->  Gather  (cost=0.00..20.63 rows=10 width=12)
>Workers Planned: 2
>Params Evaluated: $1
>->  Parallel Seq Scan on t1  (cost=0.00..20.63 rows=4 width=12)
>  Filter: (k = $1)
>  ->  Hash  (cost=20.63..20.63 rows=10 width=8)
>->  Gather  (cost=0.00..20.63 rows=10 width=8)
>  Workers Planned: 2
>  Params Evaluated: $1
>  ->  Parallel Seq Scan on t2  (cost=0.00..20.63
> rows=4 width=8)
>Filter: (k = $1)
> (20 rows)
>
>
> Now, here even if initplan would have been an undirect correlated
> plan, it wouldn't have been a problem, because we don't need to
> reevaluate it for Gather node below Hash.
>
> Am I missing something?  Do you have some test or shape of the plan in
> mind which can cause a problem?

The problem would happen if the plan for InitPlan $1 in the above
example itself referenced a parameter from an upper query level, and
the value of that parameter changed, and then this section of the plan
tree was rescanned.  I'm not sure I can write a query like that
off-hand, but I think it's possible to do so.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Re: [HACKERS] parallelize queries containing initplans

[Amit Kapila]

On Tue, Nov 14, 2017 at 2:39 AM, Robert Haas  wrote:
> On Sat, Nov 11, 2017 at 7:19 AM, Amit Kapila  wrote:
>> I have tried to follow the practice we have used for param extern
>> params (SerializeParamList is in params.c) and most of the handling of
>> initplans is done in nodeSubplan.c, so I choose to keep the newly
>> added functions there.  However, I think keeping it in execParallel.c
>> is also okay as we do it for serialize plan.
>
> To me it feels like there is only a very loose coupling between this
> code and what's currently in nodeSubplan.c, but maybe I'm wrong.  I am
> also not sure execParallel.c is the perfect place either; we might
> eventually split execParallel.c into multiple files if it accumulates
> too many things that are too different from each other.
>

Another possibility is subselect.c, because it contains initplan
stuff, however, most of the things are related to planning, so not
sure.  I think nodeSubplan.c won't be a bad choice as it is mentioned
in the file header that it is concerned about initplan stuff.

>> I think it would need some work in execution as well because the size
>> won't be fixed in that case for varchar type of params.  We might end
>> up with something different as well.
>
> Hmm, true.  But don't we also have that problem with the patch as
> written?  I mean, didn't we determine at some point that the value of
> an InitPlan can change, if the initplan depends on a parameter that is
> correlated but not directly correlated?  The existence of
> ExecReScanSetParamPlan() seems to suggest that this is indeed
> possible, and that means that the parameter could be reevaluated and
> evaluate, on the second time through, to a value that requires more
> storage than before.  That would be a problem, because
> ExecParallelReInitializeDSM reuses the old DSM.
>

I think this would have been a matter of concern if we use initplans
below Gather/Gather Merge.  The patch uses initplans which are between
current query level and root.  So, I think we don't need to reevaluate
such parameters.  Let us try to see it via example:


   QUERY PLAN

 Aggregate  (cost=62.08..62.08 rows=1 width=8)
   InitPlan 1 (returns $1)
 ->  Finalize Aggregate  (cost=20.64..20.65 rows=1 width=8)
   ->  Gather  (cost=20.63..20.64 rows=2 width=8)
 Workers Planned: 2
 ->  Partial Aggregate  (cost=20.63..20.64 rows=1 width=8)
   ->  Parallel Seq Scan on t3  (cost=0.00..18.50
rows=850 width=4)
   ->  Hash Join  (cost=20.75..41.42 rows=1 width=4)
 Hash Cond: (t1.j = t2.j)
 ->  Gather  (cost=0.00..20.63 rows=10 width=12)
   Workers Planned: 2
   Params Evaluated: $1
   ->  Parallel Seq Scan on t1  (cost=0.00..20.63 rows=4 width=12)
 Filter: (k = $1)
 ->  Hash  (cost=20.63..20.63 rows=10 width=8)
   ->  Gather  (cost=0.00..20.63 rows=10 width=8)
 Workers Planned: 2
 Params Evaluated: $1
 ->  Parallel Seq Scan on t2  (cost=0.00..20.63
rows=4 width=8)
   Filter: (k = $1)
(20 rows)


Now, here even if initplan would have been an undirect correlated
plan, it wouldn't have been a problem, because we don't need to
reevaluate it for Gather node below Hash.

Am I missing something?  Do you have some test or shape of the plan in
mind which can cause a problem?

>>>  I broke a lot of long lines in your version of
>>> the patch into multiple lines; please try to be attentive to this
>>> issue when writing patches in general, as it is a bit tedious to go
>>> through and insert line breaks in many places.
>>
>> Okay, but I sometimes rely on pgindent for such things as for few
>> things it becomes difficult to decide which way it will be better.
>
> pgindent doesn't in general break long lines.  There are a few cases
> where it does, like reflowing comments.  But mostly you have to do
> that manually.  For instance, if you write a =
> verylongfunctionname(longargument(thing), stuff(thing), foobar(thing,
> thing, thing)) it's going to keep all that on one line.  If you insert
> line breaks between the arguments it will keep them, though.  I think
> it's worth doing something like:
>
> git diff | awk 'length($0)>81'
>
> on your patches before you submit them.  If you see things in there
> that can be reformatted to make the long lines shorter, do it.
>

Okay, point noted.

>> I have fixed the first two in attached patch and left the last one as
>> I was not sure what you have in mind
>
> Oh, yeah, that should be changed too, something like "Serialize
> PARAM_EXEC parameters."  Sorry, I thought I caught all of the
> references to initplans specifically, but I guess I missed a few.
>

No issues, I can go through it